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 deprecated variable _PS_PRICE_COMPUTE_PRECISION_ #27630

Closed
wants to merge 3 commits into from
Closed

Remove deprecated variable _PS_PRICE_COMPUTE_PRECISION_ #27630

wants to merge 3 commits into from

Conversation

idnovate
Copy link
Contributor

@idnovate idnovate commented Feb 9, 2022

Deprecated _PS_PRICE_COMPUTE_PRECISION_ instead of Context::getContext()->getComputingPrecision() was used. This cause an error when value from the 2 variables didn't match.

Questions Answers
Branch? develop
Description? When the value from _PS_PRICE_COMPUTE_PRECISION_ and Context::getContext()->getComputingPrecision() was different, the order was created as Payment Error
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no

This change is Reviewable

Deprecated `_PS_PRICE_COMPUTE_PRECISION_` instead of `Context::getContext()->getComputingPrecision()` was used. This cause an error when value from the 2 variables didn't match.
@idnovate idnovate requested a review from a team as a code owner February 9, 2022 10:34
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

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

@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Feb 9, 2022
Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

Hi and thanks for your contribution.
Could you please remove the define in config.inc.php as the constant has been tagged as deprecated in 1.7.7.
If you feel comfortable, you can also do it for _PS_PRICE_DISPLAY_PRECISION_ which was also deprecated in the same version

Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

thanks @idnovate , this is indeed deprecated and should be removed

@idnovate do you think we should have a reproducible step by step use case for the QA team, so that they can test it?

Since the PR is described as a bug fix...

@idnovate
Copy link
Contributor Author

idnovate commented Feb 9, 2022

thanks @idnovate , this is indeed deprecated and should be removed

@idnovate do you think we should have a reproducible step by step use case for the QA team, so that they can test it?

Since the PR is described as a bug fix...

Ok

@idnovate
Copy link
Contributor Author

Hi and thanks for your contribution. Could you please remove the define in config.inc.php as the constant has been tagged as deprecated in 1.7.7. If you feel comfortable, you can also do it for _PS_PRICE_DISPLAY_PRECISION_ which was also deprecated in the same version

But this could be a BC break if modules or custom developments use this variable.

@idnovate
Copy link
Contributor Author

idnovate commented Feb 14, 2022

thanks @idnovate , this is indeed deprecated and should be removed

@idnovate do you think we should have a reproducible step by step use case for the QA team, so that they can test it?

Since the PR is described as a bug fix...

How to reproduce it and test it?

  1. Set 0 decimals for a currency:

image

  1. Set Order status as logable:

image

  1. Place an order with a product above 999€:

image

  1. Order is created with Payment error:

image

@idnovate Could you create a dedicated issue for validating it by the @PrestaShop/qa-functional team ?

#27902

@Progi1984
Copy link
Contributor

@idnovate Could you create a dedicated issue for validating it by the @PrestaShop/qa-functional team ?

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

In the createOrderCartRules you have this variable already available.

In the first method, you could create if it's in a few places.

classes/PaymentModule.php Show resolved Hide resolved
@kpodemski
Copy link
Contributor

Thanks @idnovate

I added a little feedback.

Also, this is a duplication of:
#27436

But I see that you changed the variable in the remaining places, good 👍

@kpodemski kpodemski added the Waiting for author Status: action required, waiting for author feedback label Apr 1, 2022
@matks
Copy link
Contributor

matks commented Apr 19, 2022

Friendly reminder @idnovate 😉 also this PR requires a git rebase

@Progi1984
Copy link
Contributor

@idnovate Could you rebase your PR and remove your merge commits, please ?

@idnovate
Copy link
Contributor Author

@idnovate Could you rebase your PR and remove your merge commits, please ?

Is it OK now? 😕

@Progi1984
Copy link
Contributor

@idnovate No you have always merge commits. Check here : https://github.com/PrestaShop/PrestaShop/pull/27630/commits.

@idnovate
Copy link
Contributor Author

idnovate commented May 2, 2022

#28388

@idnovate idnovate closed this May 2, 2022
@idnovate idnovate deleted the patch-6 branch May 4, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch Waiting for author Status: action required, waiting for author feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants