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

feat(common): allow default currency code to be configurable #32584

Closed

Conversation

@Hayouung
Copy link
Contributor

Hayouung commented Sep 10, 2019

Default currency code in CurrencyPipe is currently hardcoded to USD
and is not configurable. This commit allows the default currency code
to be configurable by adding a CURRENCY_CODE injection token.

Example:

providers: [{ provide: CURRENCY_CODE, useValue: "GBP" }]
...
{{ 123.45 | currency }} // outputs £123.45 as opposed to always $123.45 before

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Hardcoded 'USD' as default currency code in CurrencyPipe and no way to change it

Issue Number: #25461

What is the new behavior?

Allows default currency code to be configured by providing CURRENCY_CODE in a module or component

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@Hayouung Hayouung requested review from angular/fw-core as code owners Sep 10, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Sep 10, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Sep 10, 2019
@Hayouung Hayouung force-pushed the Hayouung:configure-default-currency-code branch from 655996b to 9437c3e Sep 10, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Sep 10, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 10, 2019
@Hayouung Hayouung force-pushed the Hayouung:configure-default-currency-code branch from 9437c3e to 1be36c6 Sep 10, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 10, 2019
@Hayouung

This comment has been minimized.

Copy link
Contributor Author

Hayouung commented Sep 11, 2019

I messed up the author/committer by commiting under the wrong email and now I'm not quite sure how to change it back to normal. I managed to change the author but not the committer. Is there a way to change that or would it be better opening a new PR under the correct author and committer?

Disregard the above, think I fixed it!

@Hayouung Hayouung force-pushed the Hayouung:configure-default-currency-code branch from 1be36c6 to e96e180 Sep 11, 2019
@Hayouung Hayouung requested a review from angular/fw-public-api as a code owner Sep 11, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Oct 7, 2019

Thanks @Hayouung. This is an interesting change. But it would be a new public API point.
I wondered if we could actually get the default currency from the current locale instead?
Currently the locale files do not contain this but I believe that it could... perhaps from this file: https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/currencyData.json ?

@kara what do you think about this?

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Oct 7, 2019

@petebacondarwin

I wondered if we could actually get the default currency from the current locale instead?

I believe there could be several legal currencies for one locale, for example this "BT" one. Let alone there can be overlap period between the currency transition.

Technically speaking the current extraction logic to determine locale-default currency is not correct enough, which can only get the first non-deprecated currency, even if there are several valid currencies.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Oct 7, 2019

You make good points @trotyl - so I think we should aim for the new public API. @kara / @IgorMinar ??

Copy link
Member

petebacondarwin left a comment

I believe this is generally a good change. Just a couple of changes before I approve.

  • change the name of the token
  • rebase to remove the conflicts
  • squash the two commits - they are really part of the same change

Also we need to get @kara and @IgorMinar to approve the public API change.

packages/core/src/i18n/tokens.ts Outdated Show resolved Hide resolved
@Hayouung Hayouung force-pushed the Hayouung:configure-default-currency-code branch 2 times, most recently from 3f5bb57 to 99d0638 Nov 12, 2019
@Hayouung

This comment has been minimized.

Copy link
Contributor Author

Hayouung commented Nov 12, 2019

Any way to rerun the aio_preview test? Seems like it got an error downloading something but it was working in the build before I fixed the public api guard test (which only involved reordering).

Error: Request failed (status: 500): CircleCI artifact download failed (Error 502 - Bad Gateway)
    at triggerWebhook.then (/home/circleci/ng/aio/scripts/create-preview.js:26:20)
    at process._tickCallback (internal/process/next_tick.js:68:7)
Exited with code 1
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 12, 2019

This PR never had an AIO preview...

Oh I see it is just the CI job. I have restarted that

@Hayouung

This comment has been minimized.

Copy link
Contributor Author

Hayouung commented Nov 25, 2019

Hi @petebacondarwin, is there anything else I need to do with this PR or do I just need to wait for the public API change to be approved?

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 25, 2019

@Hayouung - thanks for your patience. Yes we need to get the necessary approvals but the team is busy preparing for the final release of 9.0.0.

@petebacondarwin petebacondarwin force-pushed the Hayouung:configure-default-currency-code branch from e01ab3f to ffd4025 Jan 10, 2020
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 10, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Jan 10, 2020
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 10, 2020

@petebacondarwin @IgorMinar I looked at a couple placed in g3 and it looks like CurrencyPipe is used as a utility function to convert values in TS code. It also looks like we should run a Global TAP, since there might be more instances that we may need to cleanup, so it might take time. Also, it's a bit unclear to me how to update apps code now to make it compatible with upcoming changes (we can not just add one more argument - it'd also be TS error I think).

Probably the easiest way would be to make the second argument optional in v9 (so we can also update g3 by adding extra argument to CurrencyPipe constructor) and make it required in v10 (if needed).

Thank you.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Jan 10, 2020

OK, I can make it optional over the weekend.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Jan 11, 2020

@AndrewKushnir - the constructor parameter is now optional. It will default to USD if not provided, which is basically the same as the previous behaviour, so no breaking change. PTAL.

@petebacondarwin petebacondarwin force-pushed the Hayouung:configure-default-currency-code branch from b1e0e5d to 1e048fe Jan 11, 2020
@IgorMinar IgorMinar added cla: yes and removed cla: no labels Jan 11, 2020
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 11, 2020

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Jan 11, 2020

merge-assistance: global approval

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Jan 13, 2020

@petebacondarwin it looks like there's a conflict with the patch branch. I'm switching this to master only. Could you create a new PR that targets patch only?

@atscott atscott closed this in ca1bc7e Jan 13, 2020
atscott added a commit that referenced this pull request Jan 13, 2020
atscott added a commit that referenced this pull request Jan 13, 2020
The locale data extraction has been modified to include the default
currency code in the generated locale data. This commit updates these
generated files accordingly.

PR Close #32584
atscott added a commit that referenced this pull request Jan 13, 2020
atscott added a commit that referenced this pull request Jan 13, 2020
In v10 the default currency code will be taken from the
current locale, rather than simply defaulting to `USD`.

PR Close #32584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.