-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Configurable Order Mergeable Scope #6384
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
base: main
Are you sure you want to change the base?
Configurable Order Mergeable Scope #6384
Conversation
f4b17da to
bf236b0
Compare
|
There's a conflict in the |
core/spec/models/spree/order_spec.rb
Outdated
| let(:current_order) { create(:order, user: user, store: store) } | ||
|
|
||
| before do | ||
| class TestMergeableOrdersFinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the linter will complain here "Don't define constants this way in a block".
suggestion:
let(:test_mergeable_orders_class) do
Class.new do
# test class implementation
end
end
before do
stub_spree_preferences(mergeable_orders_finder_class: test_mergeable_orders_class)
end| spree_current_user.orders.by_store(current_store).incomplete.where(frontend_viewable: true).where('id != ?', current_order.id).find_each do |order| | ||
| current_order.merge!(order, spree_current_user) | ||
| end | ||
| Spree::Config.mergeable_orders_finder_class.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, but what about passing in the current context instead? Since we're in helper/view/serializer territory, stores might have other needs than the current store/user as independent variables.
Suggestion:
# calling code
Spree::Config.mergeable_orders_finder_class.new(context: self)
# implementation
class MergeableOrdersFinder
def initialize(context:)
@user = context.current_user
@store = context.current_store
@current_order = context.current_order
end
# more implementation
endbf236b0 to
c9fdc95
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6384 +/- ##
=======================================
Coverage 89.45% 89.45%
=======================================
Files 974 975 +1
Lines 20322 20331 +9
=======================================
+ Hits 18179 18188 +9
Misses 2143 2143 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mamhoff,Thanks for the feedback. I have made changes accordingly. Let me know if there is any changes required. Thank you |
|
I think this is looking good. Please rebase the commits so they're all one commit. We don't really need to remember how the sausage was made :) |
5c847a1 to
4d65404
Compare
This patch introduces a customizable mechanism for determining which orders should be merged when a user logs in. Signed-off-by: Thukten Singye <[email protected]>
4d65404 to
d912145
Compare
Summary
This PR makes the mergeable order finding logic in
set_current_orderconfigurable by introducing a newmergeable_orders_finder_classconfiguration option andSpree::MergeableOrdersFinderclass.Fixes #6366
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: