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

UIU-2026 format currency values as currencies, not numbers #2611

Merged
merged 7 commits into from
Jan 18, 2024
Merged

Conversation

vashjs
Copy link
Contributor

@vashjs vashjs commented Jan 17, 2024

Ref: UIU-2026

So this module uses both classes and functional components; several localization methods were created for currency values.
useLocalizeCurrency hook - for functional
localizeCurrencyAmount function - for class ones (but it is also part of the hook)

After this PR is merged, all currency numbers will be with a prefix that corresponds to the currency (information about this exists in the stripes context).
Also in many places a small refactoring was made, and instead of parseFloat(value).toFixed(2) the util function formatCurrencyAmount was replaced.

Please see screenshot with differences:

Before:

image

After:

image

NOTE: there aren't functional changes, so problems with coverage cames from previous implementation.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

69.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@vashjs vashjs merged commit fb340ee into master Jan 18, 2024
3 of 4 checks passed
@vashjs vashjs deleted the UIU-2026 branch January 18, 2024 14:44
@Terala-Priyanka
Copy link
Contributor

Terala-Priyanka commented Jan 25, 2024

@vashjs The code coverage has to be at least 80%.
It is too quick to merge a PR the very next day inspite of it involving too many files to review. I do not know why this PR was approved without the minimal code coverage being acheived.
I would request you to work on the unit tests in increasing the code coverage. If that is not possible, please explain the reason for the same.
cc @mariia-aloshyna @Dmitriy-Litvinenko

@mariia-aloshyna
Copy link
Contributor

@vashjs please create a ticket to cover new code with unit tests

@vashjs
Copy link
Contributor Author

vashjs commented Feb 7, 2024

@Terala-Priyanka @mariia-aloshyna @Dmitriy-Litvinenko
If you look at my changes, there are a lot of them BUT they are not functional changes, I just added currency localization in all places. Regarding approvals, I had enough of them to merge the changes.

Since many files have changed, sonar showed all the places in the affected files where the team that owns the "ui-users" module did not cover the functionality with tests.

Before my changes, the ui-users module had 70% coverage, after my changes it was 70.2%, I did not cause any damage to the module regarding coverage.

PR before my stats:

image

My PR stats:

image

Next time please ping me in direct messages, people loosing mails time to time and I missed your comments, sorry about that.

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

Successfully merging this pull request may close these issues.

5 participants