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

Consider cart rule allow list as a block list #32068

Closed
wants to merge 1 commit into from
Closed

Consider cart rule allow list as a block list #32068

wants to merge 1 commit into from

Conversation

alisamie97
Copy link

Questions Answers
Branch? develop
Description? Fix the cart rule compatibility issues by considering that ps_cart_rule_combination now wants to hold uncombinable cart rules.
Type? refacto
Category? CO
BC breaks? yes (kind of, for the reason that merchants need to empty the table and add uncombinable items to block list)
Deprecations? no
How to test? just try to add new cart rules, and try to add other cart rules to block list and allow list. Meanwhile watch the ps_cart_rule_combination table to verify that only blocked items are added and combinable cart rules are not added to this table.
Fixed ticket? Fixes #19935
Related PRs I did not found any
Sponsor company Beisat.com

@alisamie97 alisamie97 requested a review from a team as a code owner April 6, 2023 08:12
@prestonBot
Copy link
Collaborator

Hello @stifler97!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring BC break Type: Introduces a backwards-incompatible break labels Apr 6, 2023
@alisamie97
Copy link
Author

this edit has two benefits as explained here #19935 (comment)

  1. Merchants create vouchers mostly using automated tools, and mostly they would like them to be compatible with each other. The problem with old design is that for each new voucher that you add, for the number of all other vouchers that you have new rows will be added to the allow list in ps_cart_rule_combination table. This will become a big problem for performance when you run your shop and start generating cart rules. So its better to have a block list instead of allow list.
  2. The other positive aspect of this new allow list is that, it follows the same logic of prestashop and by default when you create a voucher its compatible with all others. The good thing is that if you create another new cart rule it will be compatible with old ones with out you going back and adding it to the white list of old existing cart rules.

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.

More than a refacto, this looks like a feature modification am I right ?

@MatShir I think you would be interested in this PR

and thank you @stifler97 for your contribution 👍

@PrestaShop/product-team

@alisamie97
Copy link
Author

this looks like a feature modification am I right ?

Yes you are exactly right. Do I need to change the type in my PR ?

@MatShir MatShir added the Waiting for PM Status: action required, waiting for product feedback label Apr 11, 2023
@FabienPapet
Copy link
Member

Although that would be a simpler solution, @stifler97 we need to consider merchants upgrading their store. And their existing cart rules.

How would you fix that problem ?

@alisamie97
Copy link
Author

Although that would be a simpler solution, @stifler97 we need to consider merchants upgrading their store. And their existing cart rules.

How would you fix that problem ?

actually there are a few use cases where you want some vouchers to be uncombinable with any voucher, therefore you can empty the table easily as it means anything outside this circle is ready to be joined with another voucher with no issues
then you find your special vouchers and for those only, you fill the block list

the main benefit is that we save database storage for other important data. however maybe some merchants need the current behavior.

@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 12, 2023

I like the idea, but:

  • It's a big BC break for all modules creating vouchers.
  • Please make sure to really think it through with the product team.
  • Make sure to PROPERLY MIGRATE during upgrades.

👍

@alisamie97
Copy link
Author

It's a big BC break for all modules creating vouchers.

of course we might face some breaks, but the thing is we can do nothing about it because its a big change and we are doing something completely reversed comparing to the default

regarding the breaks. i tested with our loyalty module which generates vouchers. it works just like before when creating vouchers

the only issue is that old vouchers are not combinable with each other now

@MatShir
Copy link
Contributor

MatShir commented Apr 12, 2023

I like the idea as well, it will solve a lot of issues. I'm wondering if could add this behind a feature flag. So in cases of huge BC, people can still revert the logic. I guess also it can be interesting depending on the business.
For the next version, people could have the feature by default and people upgrading can enable the feature, accepting their own risks

@MatShir
Copy link
Contributor

MatShir commented Apr 12, 2023

BUT the page is under migration for the next version. @zuk3975 & @jolelievre how can we make it work?

@alisamie97
Copy link
Author

I'm wondering if could add this behind a feature flag. So in cases of huge BC, people can still revert the logic.

Yes this is a very good idea, but it is easier said than done
imagine you have a list of vouchers in your shop, lets say 5k, and you are using the old logic. At maximum size (I mean if all your vouchers are combinable with each other) you will have 12,497,500 rows in ps_cart_rule_combination (how? 5000 * 4999 / 2)

now lets imagine you want to disable or enable this new feature, you need to deal with such numbers every time you toggle the switch. this will cause server timeouts i guess. generally I think it is not practical

I think instead of using one table for two purposes at a time (old way = allow list) and (new way = block list) we better have two tables separately for this. so if one merchant wants to use the block list only, we do not need to think about updating that table, and instead we just dont care about it any more, we show a warning to the merchant to clear that table on their own risk

what do you think?

@MatShir
Copy link
Contributor

MatShir commented Apr 26, 2023

Hi @stifler97, (sorry the late response)
Great idea for the feature flag, it makes sense.
We are working on migrating the page #10547. So still need to find away to merge both stuff with @zuk3975.

@MatShir
Copy link
Contributor

MatShir commented Apr 26, 2023

So I guess we have to wait a bit that the migration goes futher to add your improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Type: Introduces a backwards-incompatible break develop Branch Refactoring Type: Refactoring Waiting for PM Status: action required, waiting for product feedback
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow a cart rule not to be compatible with all other cart rules (old and new)
8 participants