Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Money cell and Money type options UI #1336

Merged
merged 13 commits into from
Jun 29, 2022
Merged

Add Money cell and Money type options UI #1336

merged 13 commits into from
Jun 29, 2022

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Apr 30, 2022

Fixes: #259

Before

  • No way to make a Money column

After

  • You can change the column type to "Money".
  • You can configure the type options for it.
  • The cell shows the currency symbol during display (but not edit).

Notes

  • Many of the requirements have changed since the ticket was written, and it's difficult to summarize all of them here. @pavish I'm hoping that you'll have enough of an idea of the requirements to review this PR, and we can address subsequent issues as we discover them.

  • This PR only handles the case where the user is creating a new column with the MATHESAR_TYPES schema installed. We use the type MATHESAR_TYPES.MATHESAR_MONEY. I haven't yet tested this functionality with the built in Postgres MONEY type, but I suspect more work will be needed to handle existing data of that type. I created Allow the user to work with Postgres MONEY values with our Money UI type #1403 for that work.

  • While setting the default Money column value and while setting a filter condition for a Money column, the input does not display the currency symbol. This behavior is not ideal, but it still works, in my opinion. I'm not sure of the best design to employ to improve this situation. If we want to improve it, we'll need to give the UX some more thought.

  • The ticket says "shown in a monospace font". That's not implemented here. I'd argue it's not necessary. We can create another ticket for it if needed.

  • There's quite a bit of code duplication between Number functionality and Money functionality. I'm open to cleaning some of it up, but I'd like the dust to settle a bit first.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@codecov-commenter

This comment was marked as off-topic.

Base automatically changed from number_allowFloat_prop to master May 3, 2022 09:01
@seancolsen seancolsen added the needs: unblocking Blocked by other work label May 6, 2022
@seancolsen seancolsen changed the base branch from master to 1332_fraction_digits May 27, 2022 18:52
@seancolsen seancolsen removed the needs: unblocking Blocked by other work label May 27, 2022
@seancolsen seancolsen marked this pull request as ready for review May 27, 2022 20:58
@seancolsen seancolsen requested review from a team and pavish and removed request for a team May 27, 2022 20:58
@seancolsen seancolsen added the pr-status: review A PR awaiting review label May 27, 2022
Base automatically changed from 1332_fraction_digits to master June 6, 2022 07:50
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seancolsen @silentninja I am not able to save a Money type column here.

  1. When trying to convert a column to money type with empty values for decimal places and max digits, server throws error: the fields cannot be null.
  2. When values are entered for both, server throws error: Unknown type_option passed.

@pavish pavish assigned seancolsen and silentninja and unassigned pavish Jun 21, 2022
@pavish pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Jun 21, 2022
@seancolsen
Copy link
Contributor Author

@pavish Yeah that's an issue that I brought up with @silentninja back when I was first working on this PR a couple months ago. My memory is hazy, but I vaguely recall there being an open ticket or open PR for that somewhere. I just went looking and couldn't find it though. @silentninja does this jog your memory? Do you know of a ticket or PR for this bug? If we don't have a ticket for that yet, then I'm happy to create one. But I don't think it needs to hold up this PR. @pavish you can continue your review by manually submitting the columns PATCH request without the type_options parameter. Then you can convert the column to Money and play with its behavior. You can even change its display options as long as you make sure to remove the type_options parameter from other PATCH requests to the columns endpoint too.

@silentninja
Copy link
Contributor

@pavish @seancolsen I think the issue is #1359

@seancolsen
Copy link
Contributor Author

Thanks @silentninja I'll make fixes in this PR. @pavish I'll ping you when it's ready for review.

@seancolsen
Copy link
Contributor Author

@pavish this is ready for re-review now.

  • In 0cc393c I removed the DbTypeSelect component as you recommended via chat. I also removed DbOptionsForm, placing its contents inline within AbstractTypeOptions because it was much simpler without DbTypeSelect.
  • In ae9fcb4 I removed the SCALE and PRECISION DB type options.

@seancolsen seancolsen added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Jun 22, 2022
@seancolsen seancolsen assigned pavish and unassigned seancolsen and silentninja Jun 22, 2022
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seancolsen Looks good to me.

@pavish pavish merged commit 83ef19f into master Jun 29, 2022
@pavish pavish deleted the money_ui branch June 29, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Frontend implementation for Money Mathesar type
4 participants