Update usage count in Promotion eligibility check #3297

filippoliverani · 2019-08-08T17:31:42Z

Description

This PR addresses issue #3205.

Eligibility check on an Adjustment bound to a Promotion with a usage limit should not include in usage count the current Adjustment itself.

With this PR the eligibility check does not consider the Order of the Adjustment it is applied to.
I updated the check adding an optional array of Orders to exclude from the computation (in the same vein as Promotion #used_by?) to avoid any breaking change to Promotion and PromotionCode puplic APIs.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have added tests to cover this change

houndci-bot · 2019-08-08T17:31:49Z

core/app/models/spree/promotion.rb

-      return false if usage_limit_exceeded?
-      return false if promotion_code && promotion_code.usage_limit_exceeded?
+      return false if usage_limit_exceeded?([promotable])
+      return false if promotion_code && promotion_code.usage_limit_exceeded?([promotable])


Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

kennyadsl · 2019-08-09T09:16:46Z

@filippoliverani Hey there! Can you please rebase against master now that #3278 has been merged? It should fix the flaky spec, thanks!

kennyadsl

@filippoliverani thanks for fixing this issue! I just left some comments, please let me know what do you think. 🙂

Also, we should address the Hound complain. I know it was already there but we recently bumped the Rubocop target Ruby version and we are still in a phase where we have some warnings.

kennyadsl · 2019-08-09T09:42:52Z

core/app/models/spree/adjustment.rb

@@ -56,6 +56,14 @@ class Adjustment < Spree::Base
    extend DisplayMoney
    money_methods :amount

+    def self.adjusted_orders(excluded_orders: [])


I have a couple of questions about this class method/scope:

Maybe a different name could help understand what this method does: what about a simpler in_completed_orders()? I think that maybe with adjusted_orders() could not be immediately clear that it's just searching into completed orders.

I'm not sure that eligible should be part of this method. Chaining it from outside adds a bit of duplication but makes the code more flexible IMO, what do you think?

I agree on both points.
At first I had used a much longer name but it was quite hard to read, in_completed_orders strikes a good balance.

filippoliverani · 2019-08-09T11:14:10Z

@kennyadsl Thank you for your suggestions, I'll rebase and resolve the problems 👍 .

kennyadsl

Thanks @filippoliverani, this works for me! I left some additional comments (sorry for missing them in the first review!) on updating the YARD documentation and using keyword arguments as much as possible. WDYT?

@cedum I'd like to have your opinion here, since you opened the issue. Let us know if you think this solution makes sense for you as well!

kennyadsl · 2019-08-12T09:24:20Z

core/app/models/spree/promotion.rb

@@ -166,22 +168,20 @@ def products
    # Whether the promotion has exceeded it's usage restrictions.
    #
    # @return true or false
-    def usage_limit_exceeded?
+    def usage_limit_exceeded?(excluded_orders = [])


We need to also update the YARD documentation here.

Also, can we use a keyword argument here so it's clear, even when this method is called, that the array contains orders to exclude? I feel like this

usage_limit_exceeded?([promotable])

could be better for the reader in this form:

usage_limit_exceeded?(excluded_orders: [promotable])

We need to also update the YARD documentation here.

Missed that 😱

can we use a keyword argument here so it's clear, even when this method is called, that the array contains orders to exclude?

Absolutely agree, I used a positional argument just to be consistent with #used_by? but here I'd prefer a keyword one too.

kennyadsl · 2019-08-12T09:25:16Z

core/app/models/spree/promotion.rb

-        joins(:order).
-        merge(Spree::Order.complete).
-        distinct.
+    def usage_count(excluded_orders = [])


Same for YARD documentation and keyword argument

filippoliverani · 2019-08-17T15:10:20Z

Thank you @kennyadsl, I'm still very new to Solidus and your help is much appreciated!

cedum

@filippoliverani thanks for working on this issue! I did a quick review and left a few concerns.

I'd take a look also at the existing specs related to promos eligibility. When I was investigating this issue there were different spec examples testing nothing. I'll also look into it by the end of this week and let you know.

cedum · 2019-08-19T12:25:30Z

core/app/models/spree/promotion.rb

@@ -126,9 +126,10 @@ def activate(order:, line_item: nil, user: nil, path: nil, promotion_code: nil)
    # called anytime order.recalculate happens
    def eligible?(promotable, promotion_code: nil)
      return false if inactive?
-      return false if usage_limit_exceeded?
-      return false if promotion_code && promotion_code.usage_limit_exceeded?
+      return false if usage_limit_exceeded?(excluded_orders: [promotable])


I think we must handle here the different cases a promotable object can be:

Spree::Order

Spree::LineItem

Spree::Shipment

You can simulate these cases by creating different promos with different Action' adjustment types..

I had this doubt but I thought that I could ignore LineItems and Shipments when computing eligibility check on them because usage count was not directly affected.

Do you think that updating#eligible?to handle non Order promotables excluding Orders to which LineItems and Shipments belong to would be enough?

Do you think that updating #eligible? to handle non Order promotables excluding Orders to which LineItems and Shipments belong to would be enough?

👍 yes. AFAIK, a promotion's usage count is always the number of distinct orders that promo is applied. I cannot think of cases where it's counted by a different object.

cedum · 2019-08-19T12:36:09Z

core/app/models/spree/adjustment.rb

+      joins(:order).
+      merge(Spree::Order.complete).
+      where.not(spree_orders: { id: excluded_orders }).
+      distinct


Not strictly related to this PR, since the distinct comes from the previous implementation. Just a heads-up.

Last time I checked it, the generated query was very slow because of DISTINCT(*) on a relatively big table (~400k rows). It was something like ~1ms query response with group by vs ~500ms using distinct(*) on my local machine.
It's worth double checking it and, if confirmed, opening a separate issue.

This could be the right time to speed up the query, I'll profile both versions with some test data, thanks.

I tested it on about 500k rows of random genereated rows but I couldn't find any big difference. If you have a more realistic dump without sensitive data could you please share it with me?

🤔 thanks for looking into this. I'll try to create a reproducible case with dummy data then eventually will open a separate issue with steps to reproduce it.

filippoliverani · 2019-08-20T18:47:04Z

@cedum Thank you for the feedback, in the meantime I'll check the existing specs too 👍!

filippoliverani · 2019-08-27T14:23:37Z

@cedum now #eligible? should handle all promotable types. I updated and refactored a bit its specs to increase coverage.
I saw some other duplicated and maybe obsolete specs in promotion_spec.rb but maybe it would be better to clean them up in an other PR, WDYT?

cedum · 2019-08-29T09:24:02Z

@cedum now #eligible? should handle all promotable types. I updated and refactored a bit its specs to increase coverage.

💯LGTM

I saw some other duplicated and maybe obsolete specs in promotion_spec.rb but maybe it would be better to clean them up in an other PR, WDYT?

agree 🤝

kennyadsl

👍 Thanks @filippoliverani, and @cedum for the in-depth review!

Eligibility check on an Adjustment bound to a Promotion with an usage limit should not include in usage count the current Adjustment itself.

Move Promotion and PromotionCode common code in #usage_count query to a dedicated Adjustment class method to remove duplication.

Rename and make more flexible Adjustment class method to query completed orders.

Use safe navigation and empty lines after guard clauses

Prefer a keyword argument when passing optional excluded orders to Promotion and PromotionCode usage limit count methods.

Add YARD documentation to cover new Adjustment scope and new Promotion and PromotionCode usage limit arguments.

Exclude from Promotion eligiblity check Orders to which promotable LineItems and Shipments belong to.

Prefer 1.day syntax

houndci-bot reviewed Aug 8, 2019

View reviewed changes

kennyadsl reviewed Aug 9, 2019

View reviewed changes

filippoliverani force-pushed the promotion-eligibility-usage-count branch from f63676c to bef44c2 Compare August 9, 2019 11:31

kennyadsl reviewed Aug 12, 2019

View reviewed changes

filippoliverani force-pushed the promotion-eligibility-usage-count branch from bef44c2 to 382f4ad Compare August 17, 2019 16:33

cedum suggested changes Aug 19, 2019

View reviewed changes

filippoliverani force-pushed the promotion-eligibility-usage-count branch from 382f4ad to a63e792 Compare August 27, 2019 14:14

kennyadsl added the Code Review Needed label Sep 4, 2019

kennyadsl approved these changes Sep 20, 2019

View reviewed changes

filippoliverani force-pushed the promotion-eligibility-usage-count branch from a63e792 to 8fdd33d Compare November 20, 2019 09:51

Filippo Liverani and others added 8 commits November 20, 2019 12:00

Exclude current Adjustment from eligibility check

54a2821

Eligibility check on an Adjustment bound to a Promotion with an usage limit should not include in usage count the current Adjustment itself.

Extract duplicated Promotion #usage_count code

2bfea3a

Move Promotion and PromotionCode common code in #usage_count query to a dedicated Adjustment class method to remove duplication.

Update Adjustment completed Orders class method

9833e64

Rename and make more flexible Adjustment class method to query completed orders.

Fix Rubocop violations in Promotion

f02b77c

Use safe navigation and empty lines after guard clauses

Use keyword argument to pass optional excluded orders

b17c4f3

Prefer a keyword argument when passing optional excluded orders to Promotion and PromotionCode usage limit count methods.

Update Promotion usage limit YARD documentation

209f7e5

Add YARD documentation to cover new Adjustment scope and new Promotion and PromotionCode usage limit arguments.

Handle all promotables in Promotion eligibility check

35fb8b1

Exclude from Promotion eligiblity check Orders to which promotable LineItems and Shipments belong to.

Fix Rubocop violations in Promotion Spec

df5a01e

Prefer 1.day syntax

filippoliverani force-pushed the promotion-eligibility-usage-count branch from 8fdd33d to df5a01e Compare November 20, 2019 11:01

gmacdougall approved these changes Nov 27, 2019

View reviewed changes

kennyadsl merged commit 3f2223b into solidusio:master Nov 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update usage count in Promotion eligibility check #3297

Update usage count in Promotion eligibility check #3297

filippoliverani commented Aug 8, 2019

houndci-bot Aug 8, 2019

kennyadsl commented Aug 9, 2019

kennyadsl left a comment

kennyadsl Aug 9, 2019

filippoliverani Aug 9, 2019

filippoliverani commented Aug 9, 2019

kennyadsl left a comment

kennyadsl Aug 12, 2019

kennyadsl Aug 12, 2019

filippoliverani Aug 17, 2019

kennyadsl Aug 12, 2019

filippoliverani commented Aug 17, 2019

cedum left a comment

cedum Aug 19, 2019

filippoliverani Aug 20, 2019

cedum Aug 23, 2019

cedum Aug 19, 2019

filippoliverani Aug 20, 2019

filippoliverani Aug 27, 2019

cedum Aug 27, 2019

filippoliverani commented Aug 20, 2019

filippoliverani commented Aug 27, 2019

cedum commented Aug 29, 2019

kennyadsl left a comment

Update usage count in Promotion eligibility check #3297

Update usage count in Promotion eligibility check #3297

Conversation

filippoliverani commented Aug 8, 2019

Choose a reason for hiding this comment

kennyadsl commented Aug 9, 2019

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filippoliverani commented Aug 9, 2019

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filippoliverani commented Aug 17, 2019

cedum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filippoliverani commented Aug 20, 2019

filippoliverani commented Aug 27, 2019

cedum commented Aug 29, 2019

kennyadsl left a comment

Choose a reason for hiding this comment