Make split_promotions_with_any_match_policy task faster #4737
Conversation
Do not re-apply promotions to incomplete orders if there are no promotions effected. This is not necessary and speeds up the task for shop with lots of incomplete orders (this could easily be in the thousands). Also make the effected promotions update faster by using find_each and include actions as well.
github-actions
bot
added
the
changelog:solidus_core
|
@mamhoff would love your feedback on this :) |
Codecov Report
@@ Coverage Diff @@
## master #4737 +/- ##
=======================================
Coverage 86.14% 86.14%
=======================================
Files 578 578
Lines 14654 14656 +2
=======================================
+ Hits 12623 12625 +2
Misses 2031 2031
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -28,6 +29,8 @@ namespace :solidus do | |||
| end | |||
| end | |||
|
|
|||
| Spree::Order.where(completed_at: nil).each { |order| Spree::PromotionHandler::Cart.new(order).activate } | |||
| if promotions.any? | |||
| Spree::Order.where(completed_at: nil).each { |order| Spree::PromotionHandler::Cart.new(order).activate } | |||
There was a problem hiding this comment.
What about using #find_each also here?
There was a problem hiding this comment.
@tvdeyen did you see this comment? would love to merge this before we release 3.4, otherwise we need to backport there as well. 🙏
| @@ -3,7 +3,8 @@ | |||
| namespace :solidus do | |||
| desc "Split Promotions with 'any' match policy" | |||
| task split_promotions_with_any_match_policy: :environment do | |||
| Spree::Promotion.where(match_policy: :any).includes(:promotion_rules).all.each do |promotion| | |||
| promotions = Spree::Promotion.where(match_policy: :any) | |||
| promotions.includes(:promotion_rules, :promotion_actions).find_each do |promotion| | |||
| if promotion.promotion_rules.length <= 1 | |||
| promotion.update!(match_policy: :all) | |||
| elsif promotion.active? | |||
There was a problem hiding this comment.
I can't link it, but isn't line 15 unnecessarily nested? I don't think we should be iterating over each action to create a duplicate for every rule 🤔
new_promotion.promotion_actions = promotion.actions.map do |action|If we want to make it faster, we could move the iteration out.
There was a problem hiding this comment.
This is about creating a setup where promotions with a match_policy of any are transformed into multiple promotions, one per rule of the existing promotions. It might look like a lot of data, but that's really what match_policy: any means...
There was a problem hiding this comment.
Oh of course! 🤦 Thanks for your reply, forgive my mind going blank on this 😅
|
closing as stale |
Summary
Do not re-apply promotions to incomplete orders if there are no promotions effected. This is not necessary and speeds up the task for shop with lots of incomplete orders (this could easily be in the thousands).
Also make the effected promotions update faster by using find_each and include actions as well.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs: