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 custom MATHESAR_TYPES.MONEY type #647

Merged
merged 13 commits into from
Sep 9, 2021
Merged

Add custom MATHESAR_TYPES.MONEY type #647

merged 13 commits into from
Sep 9, 2021

Conversation

mathemancer
Copy link
Contributor

@mathemancer mathemancer commented Sep 8, 2021

Related to #417

This adds a basic composite type in the DB called MATHESAR_TYPES.MONEY. The composite type is:

( value NUMERIC, currency CHAR(3) )

Technical details

At the API (and Python) level, we use JSON to represent an entry, for example:

{
    "value": 134.56,
    "currency": "USD"
}

Note that in the DB layer, JSON (and JSONB) numbers are in fact NUMERIC types, so there shouldn't be any loss of precision in the translation at the DB layer. All conversion between JSON and the actual composite type is happening via SQL functions in the DB.

The intent is to use the standard ISO 3-character codes to represent real currencies. If a user wants to use a custom currency, they can either create or replace these codes.

TODO

Screenshots

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.

@mathemancer mathemancer requested review from a team, kgodey and pavish September 8, 2021 18:59
Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

@mathemancer Looks good, the implementation is straightforward and easy to read.

I attempted to cast a VARCHAR type (the data was numeric) to money via the column PATCH API locally and received the following error:


[
    "This type cast is not implemented"
]

It doesn't seem like we do any validating of currency yet, I'd also add that to your TODOs or create a separate issue for it.

I'm approving to unblock merge. If you want to make changes in a future PR, feel free to merge and do that.

@mathemancer
Copy link
Contributor Author

I attempted to cast a VARCHAR type (the data was numeric) to money via the column PATCH API locally and received the following error:


[
    "This type cast is not implemented"
]

I can't replicate this (i.e., when I cast a VARCHAR to MATHESAR_TYPES.MONEY, via PATCH in the endpoint, it works as expected. Would you try again with a fresh DB volume? (I just double-checked by knocking mine out and starting fresh).

@kgodey
Copy link
Contributor

kgodey commented Sep 9, 2021

@mathemancer It works, sorry, I forgot to install the type. 🤦

We should be able to set the currency using type_options in the future as well, please also add that to your TODO or create a new issue. Thanks!

@mathemancer
Copy link
Contributor Author

We should be able to set the currency using type_options in the future as well, please also add that to your TODO or create a new issue. Thanks!

I don't quite understand. Do you mean setting a default value for the currency part of the composite, or something else? It's not possible (using standard DEFAULT setting in the DB) to set the default for one component of a composite type. If we want, we could have some other column types (USD, EUR, etc.) that are customized to those currencies, implemented as DOMAINs.

@mathemancer
Copy link
Contributor Author

It doesn't seem like we do any validating of currency yet, I'd also add that to your TODOs or create a separate issue for it.

Given that users are going to be able to define their own currencies by inserting into the column, I'm not sure what validation would entail. I had given thought to a "validated money" type (associated with the implementation of inference), or even a few specific money types (USD, EUR, JPY, etc.). We could give those as optional money types. As I think about it, having the ability to have a USD column (or a whole column for any money type from some small list) would probably be useful. The DB implementation would just be a DOMAIN over a NUMERIC to give a name so the Front End knows what type it's seeing.

@mathemancer mathemancer merged commit 0299b32 into master Sep 9, 2021
@mathemancer mathemancer deleted the money_type branch September 9, 2021 14:45
@kgodey
Copy link
Contributor

kgodey commented Sep 9, 2021

@mathemancer I'm referring to the following requirements (quoted from #417's description):

Store a list of currencies and decimal precision for users to select from when creating their column. We could use something like https://pypi.org/project/money/

This is to make sure that whatever is inserted into the currency attribute is a money type we know how to display on the frontend. The user should be able to define new currency types, but I think we should figure out how to let them do that later. They'll also need to define formatting rules and precision for their currency.

I'm not sure what the best way to get a list of currencies is, but my inclination is to use a Python package (https://github.com/kalaspuff/stockholm seems newer than the one in the issue description) and sync the currencies it provides to the DB on install, that way we can just update the package when we need to and we can also use it in Python for validation etc. I'm open to other ideas, though.

Users should be able to change the currency and decimal precision of a column of type MATHESAR_TYPES.MONEY via a type_options field in the API.

We decided against allowing the user to change the decimal precision in favor of the maximum (this should be documented in the issue, btw, please update the description) but they should still be able to change the currency. If the user wanted to alter their MATHESAR_TYPES.MONEY type column from a currency of USD to a currency of GBP, wouldn't we want them to use type_options to do so for consistency?

I'm not sure what the benefit of a currency-specific type would be.

@kgodey
Copy link
Contributor

kgodey commented Sep 9, 2021

Also on a completely separate note, I think numeric types should be valid target types for money, we'd just use the numeric component to figure out how to convert the data.

@mathemancer
Copy link
Contributor Author

Also on a completely separate note, I think numeric types should be valid target types for money, we'd just use the numeric component to figure out how to convert the data.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants