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 price precision in admin shop preferences #15084

Merged
merged 26 commits into from Oct 10, 2019

Conversation

@matthieu-rolland
Copy link
Contributor

matthieu-rolland commented Aug 12, 2019

Questions Answers
Branch? develop
Description? Remove the possibility to choose price display precision in back office, see detailed explanation below
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? fixes #12223
How to test? Check that it is not possible to change the price display precision in back office, and that prices display are not broken everywhere backend and frontend.

So what this PR does, more precisely:

  • Remove display price precision from shop preferences form in back office

  • Deprecate the PS_PRICE_DISPLAY_PRECISION constant

  • Deprecates the PS_PRICE_COMPUTE_PRECISION constant, set its value to 6 (instead of giving it the same value as PS_PRICE_DISPLAY_PRECISION)

  • Add a service that calculates the price computing precision according to a currency's max digit number.

  • Replace usages of PS_PRICE_DISPLAY_PRECISION with calculated price computing value using the formerly mentioned service. (the display is dealt with by formatting method)

  • minor improvements


This change is Reviewable

@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Aug 12, 2019

Hi!

Your pull request description seems to be incomplete or malformed:

  • The category should be one of: FO, BO, CO, IN, TE, WS, LO

Would you mind completing the contribution table ? This would help us understand how interesting your contribution is.

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@prestonBot prestonBot added the Bug label Aug 12, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:cldr-price branch 3 times, most recently from 160c9b5 to 3445fbd Aug 13, 2019
@prestonBot prestonBot added the develop label Aug 13, 2019
@matthieu-rolland matthieu-rolland removed the WIP label Aug 13, 2019
@matthieu-rolland matthieu-rolland marked this pull request as ready for review Aug 13, 2019
@matthieu-rolland matthieu-rolland requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 13, 2019
@matthieu-rolland matthieu-rolland added Improvement WIP and removed Bug labels Aug 13, 2019
@matthieu-rolland matthieu-rolland removed the WIP label Aug 16, 2019
classes/Context.php Outdated Show resolved Hide resolved
classes/Context.php Outdated Show resolved Hide resolved
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:cldr-price branch 3 times, most recently from e976a4e to 59b0d0f Aug 16, 2019
behatoutput.txt Outdated Show resolved Hide resolved
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:cldr-price branch 2 times, most recently from 2387d38 to 9b4d58c Aug 21, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Aug 21, 2019

@PierreRambaud I treated all your feedbacks

Copy link
Contributor

jolelievre left a comment

Great work @matthieu-rolland
I'm a bit worried about two things however:

You replaced PS_PRICE_DISPLAY_PRECISION with the computing precision which will be higher than the former display precision(2 digits in most cases) So I'm worried it will change the expected display, besides if we're talking about display then the CLDR should be in charge of the expected display.

Second why did you remove all the legacy tests for cart? Are they irrelevant now? and all validated by behat?

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:cldr-price branch from f307a1d to 1bf38f1 Sep 23, 2019
@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Oct 9, 2019
@PierreRambaud PierreRambaud merged commit 397d5fe into PrestaShop:develop Oct 10, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Oct 10, 2019

@rblaurin

This comment has been minimized.

Copy link
Contributor

rblaurin commented on install-dev/upgrade/sql/1.7.7.0.sql in 38b9a1b Nov 6, 2019

Did you miss a "FROM" here?

This comment has been minimized.

Copy link
Contributor

matks replied Nov 7, 2019

That is a very nice catch 🎯

This comment has been minimized.

Copy link
Contributor

matks replied Nov 7, 2019

Created a fix PR for that: #16305

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.