Add support for sorting store credits with different algorithms #4677

tmtrademarked · 2022-10-17T19:35:24Z

Summary

In our store, we have a set of custom eligibility rules for store credits based on the contents of the order. So we need a hook to inject a different way to prioritize store credits, as opposed to simply the type priority. While it might be possible to monkey patch the in_priority_order scope, that feels somewhat wrong - and the add_store_credit_payment method of Spree::Order does a lot that we would prefer not to monkey patch.

This PR adds a very simple configuration hook in the middle of this method to allow stores to inject some new behavior. By default, this simply does what the existing method did - sort by credit priority.

Checklist

[ X ] I have written a thorough PR description.
[ X ] I have kept my commits small and atomic.
[ X ] I have used clear, explanatory commit messages.
[ X ] I have added automated tests to cover my changes.

codecov · 2022-10-17T20:25:43Z

Codecov Report

Merging #4677 (f385be9) into master (425c82f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f385be9 differs from pull request most recent head 3ae11f3. Consider uploading reports for the commit 3ae11f3 to get more accurate results

@@           Coverage Diff           @@
##           master    #4677   +/-   ##
=======================================
  Coverage   86.24%   86.25%           
=======================================
  Files         574      575    +1     
  Lines       14596    14606   +10     
=======================================
+ Hits        12588    12598   +10     
  Misses       2008     2008

Impacted Files	Coverage Δ
core/app/models/spree/order.rb	`94.22% <100.00%> (+0.01%)`	⬆️
core/app/models/spree/store_credit_prioritizer.rb	`100.00% <100.00%> (ø)`
core/lib/spree/app_configuration.rb	`99.29% <100.00%> (+<0.01%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

gsmendoza · 2022-10-18T06:49:10Z

Hi @tmtrademarked ! I think your PR makes sense! I only have a few suggestions to get it approved faster :)

Do squash the "Fix spec and remove usless order spec" commit into the "Adding support for sorting store credits with different algorithms" commit.
Check the links in https://github.com/solidusio/solidus/blob/master/CONTRIBUTING.md#pull-request-guidelines about commit message formatting. In particular,
a. Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug."
b. Wrap the explanatory to about 72 characters or so.

Cheers!

kennyadsl

Thanks Tom, this is a good change!

I left a couple of ideas, let me know what you do think!

kennyadsl · 2022-10-18T07:36:00Z

core/app/models/spree/order.rb

@@ -596,8 +596,9 @@ def add_store_credit_payments

      if matching_store_credits.any?
        payment_method = Spree::PaymentMethod::StoreCredit.first
+        sorter = Spree::Config.order_credit_sorter_class.new(self)


What about passing store credits to sort in the initializer instead of the order? Having the sorter know about the order is something that only applies in your domain, while the store credits will be needed in any scenario.

I'm definitely fine with passing the credits in the constructor! Will update after we're aligned on the other comment, though.

kennyadsl · 2022-10-18T07:37:13Z

core/app/models/spree/order.rb


-        matching_store_credits.order_by_priority.each do |credit|
+        sorter.in_sorted_order(matching_store_credits).each do |credit|


What about just using call here? It is a sorter, and sorting is the only thing it is expected to do, so I guess sorter.call is enough semantic.

Also, if you think my previous suggestion makes sense, this could be without any argument in the core, you could change the signature in your own implementation, passing the order. WDYT?

I like the idea of renaming to call or sort or something similar with no arguments. I'll apply that in a follow-up.

But I'm slightly confused by your suggestion, which makes me think I don't quite understand something. How could I change the signature of call in my implementation if the gem itself doesn't call the method with that signature? So my proposal would be to update the constructor to take in both the order and the credits.

Here's why I'd advocate for passing the order as well as the credits. My suspicion is that almost any client that needs to override this logic likely needs context which is external to the credits themselves. I can easily imagine rules that are product dependent, or address dependent, or shipping method dependent. So it seems to me that designing the sorter to give the two primary pieces of context (the available credits and the order) makes the most sense for the broadest number of potential use cases.

What do you think - does that make sense to you? If so, I'll update the constructor to take in order, credits and update the signature of the primary method to just call with no args!

tmtrademarked · 2022-10-18T14:15:43Z

2. https://github.com/solidusio/solidus/blob/master/CONTRIBUTING.md#pull-request-guidelines

Thanks, @gsmendoza! I've squashed the commits, and tweaked the commit message. Let me know what you think?

tmtrademarked

Thanks for the feedback, @kennyadsl ! I've got a few clarifications, but let me know what you think and I can make the changes?

tmtrademarked · 2022-10-18T14:17:08Z

core/app/models/spree/order.rb

@@ -596,8 +596,9 @@ def add_store_credit_payments

      if matching_store_credits.any?
        payment_method = Spree::PaymentMethod::StoreCredit.first
+        sorter = Spree::Config.order_credit_sorter_class.new(self)


I'm definitely fine with passing the credits in the constructor! Will update after we're aligned on the other comment, though.

tmtrademarked · 2022-10-18T14:21:04Z

core/app/models/spree/order.rb


-        matching_store_credits.order_by_priority.each do |credit|
+        sorter.in_sorted_order(matching_store_credits).each do |credit|


I like the idea of renaming to call or sort or something similar with no arguments. I'll apply that in a follow-up.

But I'm slightly confused by your suggestion, which makes me think I don't quite understand something. How could I change the signature of call in my implementation if the gem itself doesn't call the method with that signature? So my proposal would be to update the constructor to take in both the order and the credits.

Here's why I'd advocate for passing the order as well as the credits. My suspicion is that almost any client that needs to override this logic likely needs context which is external to the credits themselves. I can easily imagine rules that are product dependent, or address dependent, or shipping method dependent. So it seems to me that designing the sorter to give the two primary pieces of context (the available credits and the order) makes the most sense for the broadest number of potential use cases.

What do you think - does that make sense to you? If so, I'll update the constructor to take in order, credits and update the signature of the primary method to just call with no args!

kennyadsl · 2022-10-18T14:38:57Z

But I'm slightly confused by your suggestion, which makes me think I don't quite understand something.

No, you are totally right! The core would continue to pass no arguments to the call method so it won't work. I guess that passing both in the constructor is fine. Just another thought: maybe the signature will be better having credits as the first argument and order as the second? I know it might look counterintuitive because order seems like a more broad object which contains credits (even if at this stage this is not true) but, for this method, order is more of an extra parameter. In fact, it is not even used in the default implementation, and it would be a bit weird being the first argument. Also, if we want to add more extra parameters in the future, it would be strange to have these extra parameters with credits, the main one, in between, eg .new(order, credits, subscription). WDYT?

tmtrademarked · 2022-10-18T14:45:16Z

But I'm slightly confused by your suggestion, which makes me think I don't quite understand something.

No, you are totally right! The core would continue to pass no arguments to the call method so it won't work. I guess that passing both in the constructor is fine. Just another thought: maybe the signature will be better having credits as the first argument and order as the second? I know it might look counterintuitive because order seems like a more broad object which contains credits (even if at this stage this is not true) but, for this method, order is more of an extra parameter. In fact, it is not even used in the default implementation, and it would be a bit weird being the first argument. Also, if we want to add more extra parameters in the future, it would be strange to have these extra parameters with credits, the main one, in between, eg .new(order, credits, subscription). WDYT?

Yeah, that makes sense to me! I can understand the argument that credits is primary in this circumstance. Let me apply the suggestion!

tmtrademarked · 2022-10-18T14:51:02Z

core/app/models/spree/order_credit_sorter.rb

+      @credits = credits
+    end
+
+    def sort


@kennyadsl - I went with sort as the method name which felt like a slightly clearer verb to me (personally). Let me know if you think call would be more inline with the overall conventions of Solidus, though?

@waiting-for-dev What do you think? I'm still for using call for consistency, but we still don't have a real convention in Solidus.

Yeah, I'm more in favor of using #call. That's the convention we want to use for objects that do a single action, where the class name describes that action.

tmtrademarked · 2022-10-18T15:21:31Z

It seems like the spec failures here are flakes unrelated to this change, from all the logs I can see.

gsmendoza · 2022-10-19T08:48:14Z

Thanks, @gsmendoza! I've squashed the commits, and tweaked the commit message. Let me know what you think?

Looks good @tmtrademarked ! Thank you!

tmtrademarked · 2022-10-21T21:34:06Z

It seems like a few different tests have failed, but none appear related to my changes. I don't have the ability to re-run the tests myself, unfortunately. Is anyone from Solidus able to trigger a re-run to see if that makes it pass?

tmtrademarked · 2022-11-03T16:18:38Z

@kennyadsl @gsmendoza - any further thoughts about this one? Would love to be able to land this in a forthcoming release if at all possible. :-)

waiting-for-dev

Thanks @tmtrademarked, I left some suggestions 🙂

waiting-for-dev · 2022-11-04T05:06:38Z

core/app/models/spree/order_credit_sorter.rb

+      @credits = credits
+    end
+
+    def sort


Yeah, I'm more in favor of using #call. That's the convention we want to use for objects that do a single action, where the class name describes that action.

waiting-for-dev · 2022-11-04T05:09:17Z

core/lib/spree/app_configuration.rb

+    # @!attribute [rw] order_credit_sorter_class
+    # @return [Class] a class with the same public interfaces as
+    #   Spree::OrderCreditSorter.
+    class_name_attribute :order_credit_sorter_class, default: 'Spree::OrderCreditSorter'


What about :store_credit_sorter_class & Spree::StoreCreditSorter as the names? I'm not very familiar with that sub-system, so reading "order credit" made me think that maybe it was something different than store credits.

I'm fine with that name if we prefer - my only possible reservation about it is that there could be multiple contexts in which you'd want to "sort store credits". For example, maybe the UI for rendering the list of a users credits. That could be slightly confusing, potentially.

What do you think, @waiting-for-dev ? I'm more than happy to change the name if you think that makes the purpose more obvious!

That's a good point. What about something along the lines of Spree::StoreCreditUtilizationSorter? (feel free to find a better name 🙂 )

Renamed to Spree::StoreCreditPrioritizer - thoughts?

waiting-for-dev · 2022-11-04T05:10:19Z

core/spec/models/spree/order_credit_sorter_spec.rb

+    end
+
+    it 'uses the default sorting order' do
+      expect(credits).to receive(:order_by_priority)


Maybe a matter of taste, but I'd prefer to see the actual behavior being tested instead of the implementation details. I'd assert on the returned order instead.

I can do that - will update!

waiting-for-dev

It looks perfect now. Thanks, @tmtrademarked! Last ask, could you please squash the commits so that we don't pollute the git history with the details of the discussion in this PR?

kennyadsl

I second what @waiting-for-dev asked about squashing. Excellent work @tmtrademarked, thanks!

In our store, we have a set of custom eligibility rules for store credits based on the contents of the order. So we need a hook to inject a different way to prioritize store credits, as opposed to simply the type priority. While it might be possible to monkey patch the `in_priority_order` scope, that feels somewhat wrong - and the `add_store_credit_payment` method of `Spree::Order` does a *lot* that we would prefer not to monkey patch. This PR adds a very simple configuration hook in the middle of this method to allow stores to inject some new behavior. By default, this simply does what the existing method did - sort by credit priority.

tmtrademarked · 2022-11-08T15:08:22Z

Thanks guys - commits are squashed!

We introduced an extension point for promotion adjustments in solidusio#4460 [1]. We change the expected interface from `#adjust!` to `#call` for consistency, as that's the new standard we want to follow (see solidusio#4677 [2] for an example). That's going to ease the adoption of a service layer if we eventually introduce them. [1] - solidusio#4460 [2] - solidusio#4677

We introduced an extension point for promotion adjustments in solidusio#4460 [1]. We change the expected interface from `#adjust!` to `#call` for consistency, as that's the new standard we want to follow (see solidusio#4677 [2] for an example). That's going to ease the adoption of a service layer if we eventually introduce it. [1] - solidusio#4460 [2] - solidusio#4677

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 17, 2022

gsmendoza added the type:enhancement Proposed or newly added feature label Oct 18, 2022

kennyadsl reviewed Oct 18, 2022

View reviewed changes

tmtrademarked force-pushed the add_credit_prioritizer branch from c6cd7e0 to 3900ce8 Compare October 18, 2022 14:15

tmtrademarked changed the title ~~Adding support for sorting store credits with different algorithms~~ Add support for sorting store credits with different algorithms Oct 18, 2022

tmtrademarked commented Oct 18, 2022

View reviewed changes

tmtrademarked force-pushed the add_credit_prioritizer branch from 48c4741 to 12fedb6 Compare October 21, 2022 20:25

waiting-for-dev reviewed Nov 4, 2022

View reviewed changes

waiting-for-dev approved these changes Nov 8, 2022

View reviewed changes

kennyadsl approved these changes Nov 8, 2022

View reviewed changes

tmtrademarked force-pushed the add_credit_prioritizer branch from f385be9 to 3ae11f3 Compare November 8, 2022 15:07

jarednorman approved these changes Nov 8, 2022

View reviewed changes

waiting-for-dev merged commit aee8e0d into solidusio:master Nov 9, 2022

This was referenced Dec 1, 2022

remove trailing zeroes in tax amount #4757

Closed

Remove trailing zeroes in tax amount #4758

Merged

tmtrademarked deleted the add_credit_prioritizer branch January 4, 2023 23:18

waiting-for-dev mentioned this pull request Jan 16, 2023

Use call in the adjustments recalculator's interface #4855

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sorting store credits with different algorithms #4677

Add support for sorting store credits with different algorithms #4677

tmtrademarked commented Oct 17, 2022

codecov bot commented Oct 17, 2022 •

edited

Loading

gsmendoza commented Oct 18, 2022 •

edited

Loading

kennyadsl left a comment

kennyadsl Oct 18, 2022

tmtrademarked Oct 18, 2022

kennyadsl Oct 18, 2022

tmtrademarked Oct 18, 2022

tmtrademarked commented Oct 18, 2022

tmtrademarked left a comment

tmtrademarked Oct 18, 2022

tmtrademarked Oct 18, 2022

kennyadsl commented Oct 18, 2022

tmtrademarked commented Oct 18, 2022

tmtrademarked Oct 18, 2022

kennyadsl Nov 3, 2022

waiting-for-dev Nov 4, 2022

tmtrademarked commented Oct 18, 2022

gsmendoza commented Oct 19, 2022

tmtrademarked commented Oct 21, 2022

tmtrademarked commented Nov 3, 2022

waiting-for-dev left a comment

waiting-for-dev Nov 4, 2022

waiting-for-dev Nov 4, 2022

tmtrademarked Nov 4, 2022

waiting-for-dev Nov 4, 2022

tmtrademarked Nov 4, 2022

waiting-for-dev Nov 4, 2022

tmtrademarked Nov 4, 2022

waiting-for-dev left a comment

kennyadsl left a comment

tmtrademarked commented Nov 8, 2022


		matching_store_credits.order_by_priority.each do \|credit\|
		sorter.in_sorted_order(matching_store_credits).each do \|credit\|

Add support for sorting store credits with different algorithms #4677

Add support for sorting store credits with different algorithms #4677

Conversation

tmtrademarked commented Oct 17, 2022

Summary

Checklist

codecov bot commented Oct 17, 2022 • edited Loading

Codecov Report

gsmendoza commented Oct 18, 2022 • edited Loading

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

tmtrademarked commented Oct 18, 2022

tmtrademarked left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Oct 18, 2022

tmtrademarked commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmtrademarked commented Oct 18, 2022

gsmendoza commented Oct 19, 2022

tmtrademarked commented Oct 21, 2022

tmtrademarked commented Nov 3, 2022

waiting-for-dev 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

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

tmtrademarked commented Nov 8, 2022

codecov bot commented Oct 17, 2022 •

edited

Loading

gsmendoza commented Oct 18, 2022 •

edited

Loading