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

Implement display options for the custom Money type #1105

Closed
Tracked by #249
kgodey opened this issue Feb 28, 2022 · 16 comments · Fixed by #1205
Closed
Tracked by #249

Implement display options for the custom Money type #1105

kgodey opened this issue Feb 28, 2022 · 16 comments · Fixed by #1205
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Feb 28, 2022

Problem

We need to implement support for display options for the custom Money type.

Solution

The custom money type supports the following display options. We need to represent them in the API, validate them, and also set them automatically on inference of Money types.

Format Type

This stores the currency that the Money type should be displayed as. We will store a pre-set number of currencies that the frontend can pick from, or we will allow users to use "Custom" formatting that doesn't obey the rules of a specific currency.

Options

  • Either a currency code or "Custom"
  • If the user manually updates any of the other display options, we should automatically set this to "Custom"

Symbol

Users can set a custom currency symbol here.

Options

  • Any text.
  • Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate currency symbol for that code.

Symbol Location

Location of the currency symbol.

Options

  • "Beginning" or "End"
  • Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate symbol location for that code.

Decimal Symbol

Location of the decimal symbol.

Options

  • "." or ","
  • Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate decimal symbol for that code.

Digit Grouping

How the digits are grouped.

Options

  • ‘123456789" (represented as [0])
  • "123,456,789" (represented as [3])
  • "123456,789" (represented as [3, 0])
  • ‘12,34,56,789" (represented as [3, 2])

Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate grouping for that code.

Note: These representations are based on .NET docs.

Digit Grouping Symbol

What symbol is used for digit grouping

Options

  • None
  • Thin Space
  • ","
  • "."

Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate grouping for that code.

Expected Outcomes

  • The frontend should be able to set any of these display options.
  • The frontend should know what the choices are for any display option that supports a set of choices.
  • The display options should be validated.
  • If any of the display options end up changing other display options, the backend should handle that.
    • This means the backend should know this data for a set of known currencies, we should use a library or dataset for that.
  • When we infer a column as a money type, we should set the display options for that column based on the currency symbols in the original CSV/other file.

Additional context

@kgodey kgodey added needs: unblocking Blocked by other work type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: database labels Feb 28, 2022
@kgodey kgodey added this to the [07] Initial Data Types milestone Feb 28, 2022
@silentninja silentninja added ready Ready for implementation and removed needs: unblocking Blocked by other work labels Mar 8, 2022
@silentninja silentninja added status: started and removed ready Ready for implementation labels Mar 9, 2022
@silentninja
Copy link
Contributor

If any of the display options end up changing other display options, the backend should handle that.
This means the backend should know this data for a set of known currencies, we should use a library or dataset for that.

Can you give an example of how this should work? If the Format Type is set to custom I don't think it would be a good idea for us to change things, I am thinking of a scenario where the user can either use the predefined formats and won't be able to change any other display option or use custom and we don't interfere with the options

@silentninja
Copy link
Contributor

silentninja commented Mar 14, 2022

we should use a library or dataset for that

Installing os locale package seems to be the best option, there are very few alternate options like parsing our own from a dataset(https://lh.2xlibre.net/locales/) which would anyway end up creating a subset of the locale

@kgodey
Copy link
Contributor Author

kgodey commented Mar 14, 2022

Can you give an example of how this should work?

@silentninja Examples of these are given in the issue, e.g. "Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate currency symbol for that code."

If the Format Type is set to custom I don't think it would be a good idea for us to change things, I am thinking of a scenario where the user can either use the predefined formats and won't be able to change any other display option or use custom and we don't interfere with the options

I agree with this, mostly. I think if the user is using a predefined format and then after that, they change another display option, we should allow them to do so but also change the "format type" to custom at that point. Does that make sense?

Installing os locale package seems to be the best option

Do you mean the standard library locale? That sounds fine to me.

@silentninja
Copy link
Contributor

Answering a comment on #1207, as this issue is a better place for that conversation

Doesn't this need more fleshing out? What's the intended behavior if we encounter a column with more than one currency symbol?

For now, we support inferring display options formathesar_money based on the #1205 PR which supports only one currency per column.

@silentninja
Copy link
Contributor

silentninja commented Mar 22, 2022

Moved @seancolsen comment from #1207 as this issue is a better place to track the information

I'm responding here to a Matrix thread.

I agree with @silentninja in that I think it's important for us to infer the currency unit during import. If the column contains multiple currency units that should be an error (perhaps a recoverable one, in which case we revert to a text format). I can appreciate that it may be difficult to implement that, given that the importer can't see multiple rows at once (though I know very little about those implementation details). But I do think that it's important to get this right. The currency unit is not just display. It's data.

@mathemancer
Copy link
Contributor

Copied from the other spot: (my comment)

The currency unit is not just display.

If we come to an agreement on that, then we shouldn't be storing it as a display option at all.

  • It's not actually stored in the user database
  • It's not visible anywhere except Mathesar (e.g., through other clients)
  • If we create a new column and copy the data, we now need to get the type, data and display options correct.
  • If we create a foreign key column, we're now required to make decisions about syncing up display options.
    • What about list aggregations? Do we somehow pass display options through?

And so on. Display options are not an appropriate place to hold data.

@pavish
Copy link
Member

pavish commented Mar 22, 2022

Copied over from #1207

If we come to an agreement on that, then we shouldn't be storing it (currency unit) as a display option at all.
Display options are not an appropriate place to hold data.

I agree with this, and I think we should probably make the currency unit a database option.

Unrelated, I still see value in having a mathesar ui specific endpoint that can suggest display options as I mentioned in my previous comment (#1207 (comment)). But this is not at all a priority and can be taken up for future consideration.

@mathemancer
Copy link
Contributor

mathemancer commented Mar 22, 2022

As far as storing that as a database option, there are some limitations, due to other constraints. We're avoiding certain type definition methods, because they require root access to the underlying machine and aren't available in many cloud environments (e.g., it's impossible to define a custom type that takes a type option in RDS). For context, here are the ways to do money as I see it:

  • Depend on the native PostgreSQL MONEY type (this is locale-dependent, and the locale is the database locale, the settings for which are constrained by which locales are installed on the underlying OS).
  • Use the MATHESAR_TYPES.MULTICURRENCY type. This is a composite type that lets you have a per-row currency symbol. It's not really possible to set a currency symbol for the whole column, or even a default symbol for the whole column, since you can't partially set defaults in composite types in PostgreSQL.
  • Use the MATHESAR_TYPES.MATHESAR_MONEY type. This is just a NUMERIC that we call MATHESAR_MONEY so that the information that the column is storing some kind of currency is reflected to the service (and then to the UI). What currency, and how to show it aren't part of that reflection. The information that the column is intended to represent money is also visible through any other client, since the column is actually of the type MATHESAR_TYPES.MATHESAR_MONEY in the database.
  • Create a giant pile of types, one for each currency we intend to support.
    • This would mean the vast majority of the types we support would be various currencies
    • This is not going to be performant
    • This limits the currencies we support to whatever we implement
    • I strongly dislike this option.

Given the options listed, we decided to go with the MATHESAR_TYPES.MATHESAR_MONEY type (so far). I'm open to other ideas, though. I'm not particularly happy with the current solution, it's just the best compromise given various constraints.

@silentninja
Copy link
Contributor

If we come to an agreement on that, then we shouldn't be storing it as a display option at all.

@mathemancer I think we had a discussion around this and came to a conclusion to use display options. Our options are quite limited until we have the composite money type implemented from #417

@mathemancer
Copy link
Contributor

mathemancer commented Mar 22, 2022

If we come to an agreement on that, then we shouldn't be storing it as a display option at all.

@mathemancer I think we had a discussion around this and came to a conclusion to use display options. Our options are quite limited until we have the composite money type implemented from #417

Sure, but we agreed that it's a display option for a NUMERIC (essentially). I.e., if you want to view your NUMERIC as money, you can set the database type to MATHESAR_MONEY, which then opens up some display options so you can add currency symbols and so on in the display. That doesn't imply that we're considering the currency and info about how that currency should be displayed to be data.

In fact, if we want to store which currency the column is as data, we'd want to store simply which currency it is, and then still leave how to display that given currency to other non-database layers.

@kgodey
Copy link
Contributor Author

kgodey commented Mar 22, 2022

We did decide to treat currency as a display option; it is not data. I don't think we should be revisiting our money implementation right now, we've already had a lot of discussions about it and I think it's better to ship something imperfect and see what users think.

Is there a problem with implementing this issue? I read through the thread and couldn't find one.

@mathemancer
Copy link
Contributor

Is there a problem with implementing this issue? I read through the thread and couldn't find one.

I don't think we should do very heavy inference of currency symbol or other display options. I'd certainly like to avoid invalidating a column if it has HK$100 on one line and $100 HKD on another (I've seen both of those ways to write Hong Kong Dollars in the wild). I suppose we could just take the first line or something, but we would need to make it quite clear that the resulting display options aren't changing the actual data (i.e., it's not stored as "Hong Kong Dollars") since this could confuse users.

@silentninja
Copy link
Contributor

I'd certainly like to avoid invalidating a column if it has HK$100 on one line and $100 HKD on another

We won't be invalidating a column, rather we fuzzy match the symbol fetched with the currency symbols we know and suggest it to the users. At most, it will be a wrong suggestion, which can be corrected during the preview.

@kgodey
Copy link
Contributor Author

kgodey commented Mar 22, 2022

@silentninja is correct. Since we're showing it as a display option, the user can change the currency or formatting at any time.

I don't see any issues with proceeding with this issue as specced.

@silentninja
Copy link
Contributor

silentninja commented Mar 24, 2022

Whenever the "Format Type" setting is changed to a currency code, this should be set to the appropriate grouping for that code.

@seancolsen @pavish @kgodey Would it be better to set just the currency_code(locale) and send it, rather than setting other details based on the currency_code.

@kgodey
Copy link
Contributor Author

kgodey commented Mar 24, 2022

@silentninja No, because (unlike Numbers), our design (see here) allows for customization of all of those other details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants