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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
14 changes: 6 additions & 8 deletions app/helpers/enterprises_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ def current_customer
end

def available_shipping_methods
return [] if current_distributor.blank?

shipping_methods = current_distributor.shipping_methods.display_on_checkout.to_a

applicator = OpenFoodNetwork::TagRuleApplicator.new(current_distributor, "FilterShippingMethods", current_customer.andand.tag_list)
applicator.filter!(shipping_methods)

shipping_methods.uniq
DistributorShippingMethods.shipping_methods(
distributor: current_distributor,
checkout: true,
customer: current_customer,
apply_tags: true
)
end

def available_payment_methods
Expand Down
4 changes: 3 additions & 1 deletion app/models/enterprise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ def supplied_taxons
end

def ready_for_checkout?
shipping_methods.any? && payment_methods.available.any?
DistributorShippingMethods.shipping_methods(
distributor: self, checkout: true, apply_tags: false
).any? && payment_methods.available.any?
Comment on lines +351 to +353
Copy link
Member

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:

Suggested change
DistributorShippingMethods.shipping_methods(
distributor: self, checkout: true, apply_tags: false
).any? && payment_methods.available.any?
checkout_shipping_methods = DistributorShippingMethods.shipping_methods(
distributor: self, checkout: true, apply_tags: false
)
checkout_shipping_methods.any? && payment_methods.available.any?

end

def self.find_available_permalink(test_permalink)
Expand Down
4 changes: 3 additions & 1 deletion app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
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

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
end
Expand Down
4 changes: 3 additions & 1 deletion app/models/spree/shipment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ def refresh_rates

# The call to Stock::Estimator below will replace the current shipping_method
original_shipping_method_id = shipping_method.try(:id)
self.shipping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)
self.shipping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(
to_package, checkout: false, apply_tags: false
)

keep_original_shipping_method_selection(original_shipping_method_id)

Expand Down
1 change: 0 additions & 1 deletion app/models/spree/shipping_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
module Spree
class ShippingMethod < ActiveRecord::Base
include Spree::Core::CalculatedAdjustments
DISPLAY = [:both, :front_end, :back_end].freeze

acts_as_taggable

Expand Down
19 changes: 19 additions & 0 deletions app/services/distributor_shipping_methods.rb
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point the shipping_methods is already an Array because the ActiveRecord::Relation fired a DB query on line 9 due to to_a. I'd be much better if we asked to remove duplicates to PostgreSQL which is much faster than asking Ruby to do it (and instantiate a bunch of bloated ActiveModel objects that are meant to be discarded). We could do that by using #distinct.

That, unfortunately, will only work when apply_tags == false when true shipping_methods will be an array at this point (making TagRuleApplication rely on AR relation is totally different topic) so we'll need to nest it within an else branch. I think it's worth it.

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ def initialize(order)
@order = order
end

def packages
def packages(checkout: true, apply_tags: true)
packages = build_packages
packages = prioritize_packages(packages)
estimate_packages(packages)
estimate_packages(packages: packages, checkout: checkout, apply_tags: apply_tags)
end

# Build package with default stock location
Expand All @@ -32,10 +32,12 @@ def prioritize_packages(packages)
prioritizer.prioritized_packages
end

def estimate_packages(packages)
def estimate_packages(packages:, checkout: true, apply_tags: true)
estimator = OrderManagement::Stock::Estimator.new(order)
packages.each do |package|
package.shipping_rates = estimator.shipping_rates(package)
package.shipping_rates = estimator.shipping_rates(
package, checkout: checkout, apply_tags: apply_tags
)
end
packages
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ def initialize(order)
@currency = order.currency
end

def shipping_rates(package, frontend_only = true)
def shipping_rates(package, checkout: true, apply_tags: true)
andrewpbrett marked this conversation as resolved.
Show resolved Hide resolved
shipping_rates = []
shipping_methods = shipping_methods(package)
shipping_methods =
shipping_methods(package, checkout: checkout, apply_tags: apply_tags)
return [] unless shipping_methods

shipping_methods.each do |shipping_method|
Expand All @@ -23,7 +24,7 @@ def shipping_rates(package, frontend_only = true)
shipping_rates.sort_by! { |r| r.cost || 0 }

unless shipping_rates.empty?
if frontend_only
if checkout
shipping_rates.each do |rate|
if rate.shipping_method.frontend?
andrewpbrett marked this conversation as resolved.
Show resolved Hide resolved
rate.selected = true
Expand All @@ -40,8 +41,8 @@ def shipping_rates(package, frontend_only = true)

private

def shipping_methods(package)
shipping_methods = package.shipping_methods
def shipping_methods(package, checkout: true, apply_tags: true)
andrewpbrett marked this conversation as resolved.
Show resolved Hide resolved
shipping_methods = package.shipping_methods(checkout: checkout, apply_tags: apply_tags)
shipping_methods.delete_if { |ship_method| !ship_method.calculator.available?(package) }
shipping_methods.delete_if { |ship_method| !ship_method.include?(order.ship_address) }
shipping_methods.delete_if { |ship_method|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,24 +79,13 @@ def currency
# TODO calculate from first variant?
end

# Returns all existing shipping categories.
# It disables the matching of product shipping category with shipping method's category
# It allows checkout of products with categories that are not the ship method's categories
#
# @return [Array<Spree::ShippingCategory>]
def shipping_categories
Spree::ShippingCategory.all
end

# Skips the methods that are not used by the order's distributor
#
# @return [Array<Spree::ShippingMethod>]
def shipping_methods
available_shipping_methods = shipping_categories.flat_map(&:shipping_methods).uniq.to_a
andrewpbrett marked this conversation as resolved.
Show resolved Hide resolved

available_shipping_methods.keep_if do |shipping_method|
ships_with?(order.distributor.shipping_methods.to_a, shipping_method)
andrewpbrett marked this conversation as resolved.
Show resolved Hide resolved
end
def shipping_methods(checkout: true, apply_tags: true)
DistributorShippingMethods.shipping_methods(
distributor: order.distributor,
checkout: checkout,
customer: order.customer,
apply_tags: apply_tags
)
end

def inspect
Expand Down Expand Up @@ -124,17 +113,6 @@ def to_shipment

shipment
end

private

# Checks whether the given distributor provides the specified shipping method
#
# @param shipping_methods [Array<Spree::ShippingMethod>]
# @param shipping_method [Spree::ShippingMethod]
# @return [Boolean]
def ships_with?(shipping_methods, shipping_method)
shipping_methods.include?(shipping_method)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,6 @@ module Stock
expect(package.shipping_methods).to eq [shipping_method1]
end
end

describe '#shipping_categories' do
it "returns ship categories that are not the ship categories of the order's products" do
package
other_shipping_category = Spree::ShippingCategory.create(name: "Custom")

expect(package.shipping_categories).to eq [shipping_method1.shipping_categories.first,
other_shipping_category]
end
end
end
end
end
Expand Down
1 change: 0 additions & 1 deletion spec/lib/open_food_network/tag_rule_applicator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ module OpenFoodNetwork
let!(:product_tag_rule2) { create(:filter_products_tag_rule, enterprise: enterprise, priority: 4, preferred_customer_tags: "tag1", preferred_variant_tags: "tag3", preferred_matched_variants_visibility: "hidden" ) }
let!(:product_tag_rule3) { create(:filter_products_tag_rule, enterprise: enterprise, priority: 3, preferred_customer_tags: "tag2", preferred_variant_tags: "tag1", preferred_matched_variants_visibility: "visible" ) }
let!(:default_product_tag_rule) { create(:filter_products_tag_rule, enterprise: enterprise, priority: 2, is_default: true, preferred_variant_tags: "tag1", preferred_matched_variants_visibility: "hidden" ) }
let!(:sm_tag_rule) { create(:filter_shipping_methods_tag_rule, enterprise: enterprise, priority: 1, preferred_customer_tags: "tag1", preferred_shipping_method_tags: "tag1", preferred_matched_shipping_methods_visibility: "visible" ) }

describe "initialisation" do
context "when enterprise is nil" do
Expand Down
67 changes: 67 additions & 0 deletions spec/services/distributor_shipping_methods_spec.rb
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