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

Remove trailing zeroes in tax amount #4758

Conversation

Naokimi
Copy link
Contributor

@Naokimi Naokimi commented Dec 1, 2022

Summary

Issue
amount_for_adjustment_label uses ActiveSupport::NumberHelper::NumberToPercentageConverter which defaults to a precision of 3, leading to the tax amount displayed in the label in a way that isn't very pleasing:
image

Proposed Solution
Use amount's default precision
image

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the readme to account for my changes.

@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Dec 1, 2022
@Naokimi Naokimi changed the title remove trailing zeroes in tax amount Remove trailing zeroes in tax amount Dec 1, 2022
@Naokimi Naokimi marked this pull request as draft December 1, 2022 10:21
@Naokimi Naokimi force-pushed the remove-trailing-zeroes-in-tax-amount branch from 594e37c to 6910fee Compare December 1, 2022 10:27
@Naokimi Naokimi marked this pull request as ready for review December 1, 2022 10:30
@Naokimi Naokimi force-pushed the remove-trailing-zeroes-in-tax-amount branch from 6910fee to a11f3ba Compare December 2, 2022 01:57
@github-actions github-actions bot added the changelog:solidus_backend Changes to the solidus_backend gem label Dec 2, 2022
@waiting-for-dev waiting-for-dev added the type:bug Error, flaw or fault label Dec 2, 2022
@waiting-for-dev
Copy link
Contributor

@Naokimi, could you please rebase from master to fix CI errors? 🙏

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

NumberToPercentageConverter defaults to a precision of 3, leading to
displays of VAT amount like '21.000%' on the cart's label, which doesn't
look good. Using `precision: nil` will keep the default precision of the
number.
@Naokimi Naokimi force-pushed the remove-trailing-zeroes-in-tax-amount branch from a11f3ba to 5483bc1 Compare December 2, 2022 06:00
@Naokimi Naokimi requested a review from a team as a code owner December 2, 2022 06:00
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #4758 (5483bc1) into master (efe2b3a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4758   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files         578      578           
  Lines       14654    14654           
=======================================
  Hits        12623    12623           
  Misses       2031     2031           
Impacted Files Coverage Δ
core/app/models/spree/tax_rate.rb 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

💔 Some backports could not be created

Status Branch Result
v3.2
v3.1 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

backport --pr 4758

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

💔 Some backports could not be created

Status Branch Result
v3.2
v3.1 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

backport --pr 4758

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@waiting-for-dev
Copy link
Contributor

@Naokimi, could you please manually backport it to v3.1? The automatic backporting failed because of merge conflicts 🙏

@tvdeyen
Copy link
Member

tvdeyen commented Dec 29, 2022

This now adds a trailing zero to all VAT tax rates in ie. German stores, but we want no precision at all so we used the locale file to set the format.

# de.yml
de:
  number:
    percentage:
      format:
        precision: 0

Now we cannot control the format with locale files anymore.
The preposed solution should have been to set:

# en.yml
en:
  number:
    percentage:
      format:
        precision: 1

in the config/locales/en.yml file of your app.

Can we please revert this? @solidusio/core-team

@tvdeyen
Copy link
Member

tvdeyen commented Dec 29, 2022

Revert PR #4824

@tvdeyen
Copy link
Member

tvdeyen commented Dec 29, 2022

I added a commit to #4824 that changes the default precision of EN stores to 1 (instead of the Rails default of 3). This should fix what this PR tried to fix for all EN based stores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem type:bug Error, flaw or fault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants