Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 16 commits into from
Closed

filter available shipping methods based on tags #6039

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5ed8cf6
filter available shipping methods based on tags
andrewpbrett Sep 16, 2020
17a59e0
create distributor shipping methods service
andrewpbrett Sep 18, 2020
83fdf2f
return empty array if distributor is blank
andrewpbrett Sep 18, 2020
99539a8
remove dead code (ships_with method in Stock::Package)
andrewpbrett Sep 19, 2020
38d6314
code cleanup in DistributorShippingMethods
andrewpbrett Sep 19, 2020
5c0fe5f
remove unused shipping_categories method on Stock::Package
andrewpbrett Sep 19, 2020
b4f89d0
remove spec for Stock::Package shipping_categories
andrewpbrett Sep 19, 2020
f6e613b
remove unused `let` in tag_rule_applicator_spec
andrewpbrett Sep 24, 2020
60e9ff0
move to named arguments
andrewpbrett Sep 24, 2020
e171bf1
pass apply_tags and checkout params to methods; fix #6137; fix #6136
andrewpbrett Oct 11, 2020
4c36541
mark shops with no checkout ship methods as not ready for checkout
andrewpbrett Oct 12, 2020
e973f74
resolve merge conflict
andrewpbrett Oct 15, 2020
74e82b9
remove args from refresh_rates
andrewpbrett Oct 17, 2020
d5687a3
make package a required, not keyword, argument
andrewpbrett Oct 17, 2020
a36c2bf
don't apply tags if the order is created by admin
andrewpbrett Oct 17, 2020
3022dbc
remove dead code
andrewpbrett Jan 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/spree/admin/orders_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ def load_distribution_choices
def ensure_distribution
unless @order
@order = Spree::Order.new
@order.created_by = spree_current_user
@order.generate_order_number
@order.save!
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/spree/order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ def create_proposed_shipments
shipments.destroy_all

packages = OrderManagement::Stock::Coordinator.new(self).packages(
checkout: true, apply_tags: true
checkout: true, apply_tags: !created_by&.admin?
Copy link
Member

Choose a reason for hiding this comment

The 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:

  • The front-office always applies filters to shipping methods.
  • The back-office does not apply filters to shipping methods.

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?

)
Copy link
Contributor

@luisramos0 luisramos0 Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down