-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
filter available shipping methods based on tags #6039
Changes from 15 commits
5ed8cf6
17a59e0
83fdf2f
99539a8
38d6314
5c0fe5f
b4f89d0
f6e613b
60e9ff0
e171bf1
4c36541
e973f74
74e82b9
d5687a3
a36c2bf
3022dbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -593,7 +593,9 @@ def create_proposed_shipments | |
adjustments.shipping.delete_all | ||
shipments.destroy_all | ||
|
||
packages = OrderManagement::Stock::Coordinator.new(self).packages | ||
packages = OrderManagement::Stock::Coordinator.new(self).packages( | ||
checkout: true, apply_tags: !created_by&.admin? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that this is correct. If I'm a super admin I see all shipping methods in the admin interface and in the checkout even if I shouldn't based on my current tags, right? That makes it impossible to test tags as admin user. It's also not good if you can't tell which methods another enterprise wants you to use. So this is a problem when a super admin account is also used for shopping. This also doesn't allow enterprise users to add any shipping method of their own enterprise to orders created in the backoffice. I think that's what @lin-d-hop was after. I would say:
So this logic needs to be triggered by the controller. The model is not the right place for this logic. Spree's design decision to include all checkout logic in a state machine is making it very difficult here. It's not pretty but we can move this out of the model into the OrderWorkflow service. It already contains logic concerning shipping method selection and it would make sense to have it in one place. It can then be called with different options from the frontend and backend. What do you think? |
||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think this is correct, this step is also executed when you create an order in the backoffice and so we would want these checkout/tags args to be false in that case... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay that makes sense - I think I was focused on the case where we wanted to be able to change the shipping method on an existing order in the backend and have all methods available... forgot about creating one. I think we can still make it work, but definitely trickier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this commit a36c2bf should address this. |
||
packages.each do |package| | ||
shipments << package.to_shipment | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# frozen_string_literal: true | ||
|
||
class DistributorShippingMethods | ||
def self.shipping_methods(distributor:, customer: nil, checkout: false, apply_tags: true) | ||
return [] if distributor.blank? | ||
|
||
shipping_methods = distributor.shipping_methods | ||
shipping_methods = shipping_methods.display_on_checkout if checkout | ||
shipping_methods = shipping_methods.to_a | ||
|
||
if apply_tags | ||
OpenFoodNetwork::TagRuleApplicator.new( | ||
distributor, "FilterShippingMethods", customer&.tag_list | ||
).filter!(shipping_methods) | ||
end | ||
|
||
shipping_methods.uniq | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point the That, unfortunately, will only work when |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
describe DistributorShippingMethods do | ||
let!(:enterprise) { create(:distributor_enterprise) } | ||
let!(:shipping_method) { create(:shipping_method, distributors: [enterprise]) } | ||
let!(:member_shipping_method) { create(:shipping_method, distributors: [enterprise]) } | ||
let!(:backend_shipping_method) { create(:shipping_method, distributors: [enterprise]) } | ||
let!(:customer) { create(:customer) } | ||
let!(:sm_tag_rule) { | ||
create( | ||
:filter_shipping_methods_tag_rule, | ||
enterprise: enterprise, | ||
priority: 2, | ||
preferred_customer_tags: "member", | ||
preferred_shipping_method_tags: "member", | ||
preferred_matched_shipping_methods_visibility: "visible" | ||
) | ||
} | ||
|
||
let!(:default_sm_tag_rule) { | ||
create( | ||
:filter_shipping_methods_tag_rule, | ||
enterprise: enterprise, | ||
priority: 1, | ||
is_default: true, | ||
preferred_shipping_method_tags: [], | ||
preferred_customer_tags: [], | ||
preferred_matched_shipping_methods_visibility: "hidden" | ||
) | ||
} | ||
|
||
it "returns all shipping methods for a distributor" do | ||
expect(DistributorShippingMethods.shipping_methods(distributor: enterprise).count).to eq(3) | ||
end | ||
|
||
it "does not return a shipping method tagged as 'member' for a customer without that tag" do | ||
member_shipping_method.tag_list << "member" | ||
member_shipping_method.save | ||
result = | ||
DistributorShippingMethods.shipping_methods(distributor: enterprise, customer: customer) | ||
expect(result).to include(shipping_method) | ||
expect(result).to include(backend_shipping_method) | ||
end | ||
|
||
it "returns the shipping method tagged 'member' for a customer with that tag" do | ||
member_customer = create(:customer) | ||
member_customer.tag_list << "member" | ||
member_customer.save | ||
member_shipping_method.tag_list << "member" | ||
member_shipping_method.save | ||
result = DistributorShippingMethods.shipping_methods( | ||
distributor: enterprise, customer: member_customer | ||
) | ||
expect(result).to include(member_shipping_method) | ||
end | ||
|
||
it "does not return a non-checkout shipping method if passed checkout=true" do | ||
backend_shipping_method.display_on = "back_end" | ||
backend_shipping_method.save | ||
result = DistributorShippingMethods.shipping_methods( | ||
distributor: enterprise, checkout: true, customer: customer | ||
) | ||
expect(result).to include(shipping_method) | ||
end | ||
end |
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.
Really minor but I thought it might be worth sharing that I would found this easier to read like this: