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

Migrate Sell > Catalog > Discounts > Catalog Price Rules list #13584

Merged
merged 31 commits into from Sep 23, 2019

Conversation

@zuk3975
Copy link
Contributor

commented Apr 26, 2019

Questions Answers
Branch? develop
Description? Migrate Sell > Catalog > Discounts > Catalog Price Rules list without actions. Add/Edit Actions are implemented in #13716
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10549
How to test? Use admin-dev/index.php/sell/catalog/catalog-price-rules to see the working list. Behavior should stay the same as in legacy page. Except add/edit actions are implemented in another PR.

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner Apr 26, 2019
@matks matks added the migration label Apr 26, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:m/catalog-price-rules branch from f6c72a0 to 5aa9c8f May 7, 2019
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@matks, ready for review

'group_name' => 'pr_group.name',
];
$exactMatchFilters = ['id_specific_price_rule', 'from_quantity', 'reduction', 'reduction_type'];

This comment has been minimized.

Copy link
@zuk3975

zuk3975 May 15, 2019

Author Contributor

@matks, according to legacy- reduction field in list is shown rounded to decimals, so filtering float which is, for example 50.0005 wont find it as exact match, but user will see 50.00 in unfiltered list. Seems like UX issue to me.
Currently i just didn't round the numbers. Any other offers?

This comment has been minimized.

Copy link
@matks

matks May 23, 2019

Contributor

@eternoendless This is a Grid migration PR. Grid do not depend on legacy code, only on the legacy SQL schema.

I think we can introduce https://github.com/PrestaShop/decimal here. Do you agree ?

This comment has been minimized.

Copy link
@eternoendless

eternoendless May 23, 2019

Member

Use Decimal as much as you can, specially when performing price operations.

This comment has been minimized.

Copy link
@eternoendless

eternoendless May 23, 2019

Member

Also, when rounding, you must keep in mind the rounding mode as configured in the shop settings.

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jun 19, 2019

Author Contributor

Confused with the rounding 😕. According to #12223 I CLDR should handle the prices rounding, but could it be also used for reduction 🤔 ?. (if possible maybe it's worth introducing some kind of DecimalColumn/PriceColumn ? also the rounding should also be considered when filtering).

This comment has been minimized.

Copy link
@matks

matks Jul 12, 2019

Contributor

Sorry @zuk3975 my answer is irrelevant
I think this is an issue that must be discussed further with UX and Product, in another PR. I added it to the todo-list #10548 so we wont forget

@matks matks changed the title [WIP] Migrate Sell > Catalog > Discounts > Catalog Price Rules list Migrate Sell > Catalog > Discounts > Catalog Price Rules list May 23, 2019
@zuk3975 zuk3975 changed the title Migrate Sell > Catalog > Discounts > Catalog Price Rules list [WIP]Migrate Sell > Catalog > Discounts > Catalog Price Rules list Jun 6, 2019
@zuk3975 zuk3975 changed the title [WIP]Migrate Sell > Catalog > Discounts > Catalog Price Rules list Migrate Sell > Catalog > Discounts > Catalog Price Rules list Jun 25, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

@zuk3975 We can go for QA once you rebase 😄

@zuk3975 zuk3975 force-pushed the zuk3975:m/catalog-price-rules branch from 338c39d to 62e4de1 Jul 15, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:m/catalog-price-rules branch from 6b32ab3 to 8d97d8d Sep 23, 2019
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@matks, rebased 😄

@sarahdib sarahdib added the QA ✔️ label Sep 23, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Sep 23, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

Thank you @zuk3975 ! 🎉

@matks
matks approved these changes Sep 23, 2019
@matks matks merged commit 37db7cd into PrestaShop:develop Sep 23, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
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.