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

Backend work for Mathesar money custom type #417

Closed
Tracked by #249
kgodey opened this issue Jul 17, 2021 · 16 comments
Closed
Tracked by #249

Backend work for Mathesar money custom type #417

kgodey opened this issue Jul 17, 2021 · 16 comments
Assignees
Labels
needs: unblocking Blocked by other work type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Jul 17, 2021

This issue is to create a custom Money data type to handle monetary data. At the DB level, this will refer to a number of types such as MATHESAR_TYPES.MONEY_USD, MATHESAR_TYPES.MONEY_JPY, etc. Each underlying type should have a name of the form MATHESAR_TYPES.MONEY_<currency code>, where currency code is one of the 3-character currency codes defined in ISO 4217.

As part of this issue, we need to:

  • Create a Money friendly type name in the service
  • Create a number of MATHESAR_TYPES.MONEY_<currency code> types in the database that store currency amounts. The amount should be a NUMERIC type.
    • The types should be case-insensitive at the DB layer and uppercase in the API
  • We'll create the types automatically, putting things together using some package like https://github.com/arshadkazmi42/currency-symbols or https://github.com/python-babel/babel to provide currency symbol info.
  • Users should be able to use the API to change a column to MATHESAR_TYPES.MONEY_<currency code> if it's possible to do so.
  • Automatic type inference during file import suggests MATHESAR_TYPES.MONEY_<currency code> when it makes sense to do so.

Additional Context

@kgodey kgodey added ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: database labels Jul 17, 2021
@kgodey kgodey added this to the 07. Initial Data Types milestone Jul 17, 2021
@kgodey kgodey changed the title Create MATHESAR_MONEY custom type and handle it in the backend Backend work for MATHESAR_MONEY custom type Jul 18, 2021
@mathemancer
Copy link
Contributor

See my comment on #409 . We should create types in the mathesar_types schema unless there's a good reason to do otherwise.

@kgodey kgodey changed the title Backend work for MATHESAR_MONEY custom type Backend work for Mathesar money custom type Jul 19, 2021
@kgodey
Copy link
Contributor Author

kgodey commented Jul 19, 2021

@mathemancer Updated the issue. I made it uppercase to match other types, do you have any issues with that?

@mathemancer
Copy link
Contributor

I think it should be case-insensitive at the DB layer, and displayed as upper case in the API. This will be the most consistent with the way the default type names work.

If we go that route, we'll have to either:

  • enforce that setup (i.e., case-insensitive naming at the DB layer) for eventual user-created types, or
  • have a separate way of handling user-created type names when that's included.

User-defined types are a ways off, though, so I think we should just enforce case-insensitive naming of types (and the mathesar_types schema) on ourselves for now. Being able to assume case-insensitivity for type names would actually reduce some complexity I had to introduce around quoting names.

@kgodey
Copy link
Contributor Author

kgodey commented Jul 20, 2021

@mathemancer Okay, thanks, makes sense. I'll update the existing custom types issues.

@kgodey kgodey added needs: unblocking Blocked by other work and removed ready Ready for implementation labels Jul 20, 2021
@mathemancer
Copy link
Contributor

@kgodey I'm going back-and-forth on whether to restrict the currencies allowed by the MATHESAR_TYPES.MONEY type. If we restrict, we won't support fantasy currencies, and many historical currencies (e.g., a number of pre-Euro European currencies) would probably not be supported. On the other hand, if we don't restrict, trying to infer whether a column is a 'money' column will probably be a bit ad-hoc.

Also, I'm currently assuming we'll want to read / write composite types as JSON at the moment. This isn't the default, but the implementation was easy and seems to be more useful in the service / API than the default tuples.

@mathemancer
Copy link
Contributor

mathemancer commented Sep 1, 2021

@kgodey Regarding precision:

We can't set the precision of the numeric portion of a composite type on a column-by-column basis. Thus, in order to support storing in different precision levels depending on the currency, we'd need to have a different type for (at a minimum) each group of currencies with a given precision. I did a (small) bit of reading, and it seems that the standard for financial calculations is to do them in infinite (or best possible) precision, then display the result truncated or rounded. Otherwise, you end up with some odd results. For example, if we use 2 decimal places to store dollars, you'd end up with:

$10.00 / 3 = $3.33
$3.33 * 3 = $9.99

But, it should be back to $10.00 in the end instead.

Thus, I suggest we make the decimal precision a UI concern rather than storage in the type. This would also put it together with worrying about where to put commas and the like, which seems cleaner somehow.

@kgodey
Copy link
Contributor Author

kgodey commented Sep 1, 2021

@kgodey I'm going back-and-forth on whether to restrict the currencies allowed by the MATHESAR_TYPES.MONEY type. If we restrict, we won't support fantasy currencies, and many historical currencies (e.g., a number of pre-Euro European currencies) would probably not be supported. On the other hand, if we don't restrict, trying to infer whether a column is a 'money' column will probably be a bit ad-hoc.

We should not restrict currencies, but we should ship Mathesar with a curated list of currencies that people can use. It's okay if we can only infer money within our curated list of currencies.

Also, I'm currently assuming we'll want to read / write composite types as JSON at the moment. This isn't the default, but the implementation was easy and seems to be more useful in the service / API than the default tuples.

I'm not sure what this means in practical terms, but it sounds fine to me.

Thus, I suggest we make the decimal precision a UI concern rather than storage in the type. This would also put it together with worrying about where to put commas and the like, which seems cleaner somehow.

That sounds fine to me.

@mathemancer
Copy link
Contributor

We should not restrict currencies, but we should ship Mathesar with a curated list of currencies that people can use. It's okay if we can only infer money within our curated list of currencies.

In that case, I think it would make sense to use an actual table in the MATHESAR_TYPES schema. That way, we can also store other info about the currencies (symbol, "friendly name", etc.), and try to use it for inference.

@kgodey
Copy link
Contributor Author

kgodey commented Sep 1, 2021

@mathemancer I think we should use something like https://github.com/kalaspuff/stockholm to source currency data so that we don't have to manage it ourselves. I assume having it in Python won't allow us to use it in inference, so that's why we'd need to create a table? Will it involve any more user permissions that we otherwise needed?

@mathemancer
Copy link
Contributor

I assume having it in Python won't allow us to use it in inference, so that's why we'd need to create a table?

Yes, that's correct. The testing of a type for a given value is done completely on the DB. Python just manages the looping and whatnot.

Will it involve any more user permissions that we otherwise needed?

The user needs access to SELECT on tables in the MATHESAR_TYPES schema, which is new. We can easily grant that access to all users, since Mathesar "owns" that schema. OTOH, there may be a reason at some point in the future we'd want to deny that for some users (though I can't think of any such reason at the moment). Any user denied SELECT in the MATHESAR_TYPES schema would be unable to do inference involving money types.

@kgodey
Copy link
Contributor Author

kgodey commented Sep 1, 2021

@mathemancer That works for now, please document the permission in #130 for future reference.

@mathemancer
Copy link
Contributor

Continuing a conversation from #647

@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.

I'd prefer to just use the ISO standard, which is available in XML format from the standards body. According to ISO, the standards are free-to-use. I've already written a script that makes the XML into a PostgreSQL table (that we can store in the MATHESAR_TYPES schema).

One caveat: It doesn't include currency symbols, but only the 3-character codes. However, neither of the python packages seem to include symbols either. We may have to source those from somewhere else.

For verification, I was imagining something where we just make it simple for users to use the currency code table in a foreign key relationship to make sure that the currency entries in their money entries are in the "official list" if desired.

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.

The point there is that there isn't any sub-type for a column. I.e., you can't set a column to have a specific currency, since the currency is defined for each entry separately as a component of the composite type. This is because we wanted to be able to support having multiple currencies in the same column. If we want to have a column that's set to USD, that's a column of type USD. The composite type implementation wouldn't make sense for that, since it would be storing a copy of the string 'USD' in each row. Better to have a USD type if we want to go that route, I think.

@mathemancer
Copy link
Contributor

After some discussion, I've updated the main description to reflect the intended implementation.

@kgodey
Copy link
Contributor Author

kgodey commented Sep 9, 2021

As @mathemancer mentioned, we talked on a call and decided to update the issue description. The confusion was that the implementation detail of each column having only a single currency (as decided in #118) was lost and we ended up with a type that could support a different currency for each record.

We also decided that since we have the "mixed" money type already implemented, we will leave it as-is but remove it from the API to avoid confusion. I will create issues for designing how to use that type in the frontend and we'll work on it after the alpha release. @mathemancer please make sure to do that work somewhere.

@kgodey
Copy link
Contributor Author

kgodey commented Sep 13, 2021

I've made the following issues to track frontend and design work coming out of this issue:

@kgodey kgodey added ready Ready for implementation and removed needs: unblocking Blocked by other work labels Sep 13, 2021
@mathemancer mathemancer mentioned this issue Sep 14, 2021
7 tasks
@kgodey kgodey added needs: unblocking Blocked by other work and removed ready Ready for implementation labels Oct 12, 2021
@kgodey
Copy link
Contributor Author

kgodey commented Feb 14, 2022

I'm going to close this issue since we decided against a custom money type after trying a couple of implementations. We'll be using display options on Number types to represent money (for now), see #1063.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: unblocking Blocked by other work type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants