Fix #21725 - Render price on category list with correct precision for current locale #31661

Bartlomiejsz · 2021-01-15T21:23:55Z

Description (*)

I recreated #21829 here, which got accidentally closed, when I tried to re-create changes on top of 2.5-develop. I adjusted changes to latest comment from @sivaschenko - while I will work on fixing test that might still fail, would be great if you could verify if such approach is what you meant.

Related Pull Requests

Fixed Issues (if relevant)

Fixes Price Precision wrong on product listing pages #21725

Manual testing scenarios (*)

Default locale set should be for Oman (ar_OM it's not by default available in Magento, need to be added in \Magento\Framework\Locale\Config but didn't check all locales to verify where it also occurs), Oman Rial (OMR) selected as currency
Visit product listing page - see that the products have prices with precision 3.
Visit product details page - see that the products have prices with precision 3.
Visit all other places on frontend where price is displayed - precision should be same there.

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

…ion for current locale

m2-assistant · 2021-01-15T21:23:57Z

Hi @Bartlomiejsz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.5-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

Bartlomiejsz · 2021-01-15T21:25:25Z

@magento run all tests

sivaschenko · 2021-01-17T18:40:07Z

app/code/Magento/Sales/Model/Order.php

@@ -387,7 +394,8 @@ public function __construct(
        OrderItemRepositoryInterface $itemRepository = null,
        SearchCriteriaBuilder $searchCriteriaBuilder = null,
        ScopeConfigInterface $scopeConfig = null,
-        RegionFactory $regionFactory = null
+        RegionFactory $regionFactory = null,
+        ?PricePrecisionInterface $pricePrecision = null


I don't think it makes sense to allow to pass null as argument explicitly

Suggested change

?PricePrecisionInterface $pricePrecision = null

PricePrecisionInterface $pricePrecision = null

Sure @sivaschenko, I thought that should be done according to Adding a constructor parameter point from https://devdocs.magento.com/contributor-guide/backward-compatible-development/, I can change that

Hm, this change was introduced in magento/devdocs#6033

I don't think that is correct.

@sidolov @gabrieldagama what do you think guys?

sivaschenko · 2021-01-17T18:41:39Z

lib/internal/Magento/Framework/Pricing/PriceCurrencyInterface.php

+     * @deprecated precision should be retrieved from current locale
+     * @see \Magento\Framework\Pricing\Price\PricePrecisionInterface::getPrecision
+     */
+    const DEFAULT_PRECISION = null;


I'd keep the constant value unchanged, it's enough to deprecate the constant.

@sivaschenko making this fix without changing this constant value would require many more code changes. This constant is used in many places in whole codebase (i.e.

magento2/app/code/Magento/Catalog/Model/ProductRender/FormattedPriceInfoBuilder.php

Line 64 in fbfac3e

PriceCurrencyInterface::DEFAULT_PRECISION,

), because precision is third parameter of the format method.

And I changed this parameter to be null as default, so my idea was to:

change const value to also null - so every place that uses it, just to pass it as default value to the methods I changed, will also work as expected

deprecate const - so make a note across codebase that every it occurrence should be replaced with simply null, but maybe in some next PR, to not make it over complicated.

I think eliminating this constant from the calls is a safer and more backward compatible change than changing the constant value, considering that we don't know how this constant might be currently used in extensions and customizations.

sivaschenko · 2021-01-17T18:50:26Z

@Bartlomiejsz thanks for the updates!

I've moved the pull request to To Approve column and launched a formal approval process for this change.

Internal ticket: https://jira.corp.magento.com/browse/MC-40452

# Conflicts: # app/code/Magento/Sales/Model/Order.php

…move usage of it on method calls

Bartlomiejsz · 2021-03-21T20:50:22Z

@magento run all tests

Bartlomiejsz · 2021-03-21T22:39:30Z

@magento run Unit Tests

Bartlomiejsz · 2021-03-21T23:37:11Z

@sivaschenko that seems to be ready for review :)

Bartlomiejsz · 2021-03-22T09:40:29Z

@magento run Functional Tests B2B

sivaschenko · 2021-04-27T09:28:13Z

This change was not approved for the 2.4 release line. Internal approval request MC-40452.

There is no active delivery of pull request to 2.5 release line currently, so the PR processing might be delayed.

ihor-sviziev · 2021-04-29T12:09:54Z

Added this PR to 2.5 milestone

Fix magento#21725 - Render price on category list with correct precis…

a99aa6e

…ion for current locale

m2-community-project bot added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S1 Affects critical data or functionality and forces users to employ a workaround. labels Jan 15, 2021

magento-engcom-team added Partner: Fast White Cat partners-contribution Pull Request is created by Magento Partner labels Jan 15, 2021

m2-community-project bot added the Progress: pending review label Jan 15, 2021

ihor-sviziev self-assigned this Jan 16, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels Jan 16, 2021

sivaschenko added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Jan 17, 2021

sivaschenko reviewed Jan 17, 2021

View reviewed changes

m2-community-project bot assigned sivaschenko Jan 17, 2021

sivaschenko reviewed Jan 17, 2021

View reviewed changes

m2-community-project bot added Progress: to approve and removed Progress: review labels Jan 17, 2021

ihor-sviziev removed their assignment Jan 17, 2021

m2-community-project bot added Progress: review and removed Progress: to approve labels Jan 17, 2021

ihor-sviziev added the Progress: to approve label Jan 18, 2021

m2-community-project bot removed the Progress: review label Jan 18, 2021

m2-community-project bot removed the Progress: to approve label Jan 19, 2021

m2-community-project bot added the Progress: review label Jan 19, 2021

Bartlomiejsz added 3 commits March 21, 2021 20:43

Merge branch '2.5-develop' into feature/fix_21725_price_precision_wrong

95c0ff7

# Conflicts: # app/code/Magento/Sales/Model/Order.php

Fix magento#21725 - restore previous value of deprecated constant, re…

f631ab7

…move usage of it on method calls

Fix magento#21725 - fix unit tests

b16fc25

Bartlomiejsz requested a review from sivaschenko March 21, 2021 23:37

ihor-sviziev added this to the 2.5 milestone Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #21725 - Render price on category list with correct precision for current locale #31661

Fix #21725 - Render price on category list with correct precision for current locale #31661

Bartlomiejsz commented Jan 15, 2021 •

edited

Loading

m2-assistant bot commented Jan 15, 2021

Bartlomiejsz commented Jan 15, 2021

sivaschenko Jan 17, 2021

Bartlomiejsz Jan 18, 2021

sivaschenko Jan 19, 2021

sivaschenko Jan 17, 2021

Bartlomiejsz Jan 18, 2021

sivaschenko Jan 19, 2021

sivaschenko commented Jan 17, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 22, 2021

sivaschenko commented Apr 27, 2021

ihor-sviziev commented Apr 29, 2021

	?PricePrecisionInterface $pricePrecision = null
	PricePrecisionInterface $pricePrecision = null

Fix #21725 - Render price on category list with correct precision for current locale #31661

Are you sure you want to change the base?

Fix #21725 - Render price on category list with correct precision for current locale #31661

Conversation

Bartlomiejsz commented Jan 15, 2021 • edited Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Jan 15, 2021

Bartlomiejsz commented Jan 15, 2021

sivaschenko Jan 17, 2021

Choose a reason for hiding this comment

Bartlomiejsz Jan 18, 2021

Choose a reason for hiding this comment

sivaschenko Jan 19, 2021

Choose a reason for hiding this comment

sivaschenko Jan 17, 2021

Choose a reason for hiding this comment

Bartlomiejsz Jan 18, 2021

Choose a reason for hiding this comment

sivaschenko Jan 19, 2021

Choose a reason for hiding this comment

sivaschenko commented Jan 17, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 21, 2021

Bartlomiejsz commented Mar 22, 2021

sivaschenko commented Apr 27, 2021

ihor-sviziev commented Apr 29, 2021

Bartlomiejsz commented Jan 15, 2021 •

edited

Loading