From 40dd307a35eb5091229804e0e194fd8d92e6559b Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 8 May 2020 11:40:44 +0100 Subject: [PATCH 1/4] Remove shipping method display on front_end option, it is not working and it's not straight forward to make it work correctly --- .../spree/admin/shipping_methods/_form.html.haml | 2 +- config/locales/en.yml | 10 ++++------ ...01630_convert_frontend_shipping_method_to_both.rb | 12 ++++++++++++ db/schema.rb | 2 +- 4 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20200508101630_convert_frontend_shipping_method_to_both.rb diff --git a/app/views/spree/admin/shipping_methods/_form.html.haml b/app/views/spree/admin/shipping_methods/_form.html.haml index dfef7418a14..450febb31bd 100644 --- a/app/views/spree/admin/shipping_methods/_form.html.haml +++ b/app/views/spree/admin/shipping_methods/_form.html.haml @@ -18,7 +18,7 @@ .alpha.three.columns = f.label :display_on, t(:display) .omega.eight.columns - = select(:shipping_method, :display_on, Spree::ShippingMethod::DISPLAY.collect { |display| [t(".#{display}"), display == :both ? nil : display.to_s] }, {}, {class: 'select2 fullwidth'}) + = select(:shipping_method, :display_on, [[t(".both"), nil], [t(".back_end"), "back_end"]], {}, {class: 'select2 fullwidth'}) = error_message_on :shipping_method, :display_on .row diff --git a/config/locales/en.yml b/config/locales/en.yml index fa92cb72a1b..685004c6ab4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3221,9 +3221,8 @@ See the %{link} to find out more about %{sitename}'s features and to start using zone: "Zone" calculator: "Calculator" display: "Display" - both: "Both" - front_end: "Front End" - back_end: "Back End" + both: "Both Checkout and Back office" + back_end: "Back office only" no_shipping_methods_found: "No shipping methods found" new: new_shipping_method: "New Shipping Method" @@ -3235,9 +3234,8 @@ See the %{link} to find out more about %{sitename}'s features and to start using form: categories: "Categories" zones: "Zones" - both: "Both" - front_end: "Front End" - back_end: "Back End" + both: "Both Checkout and Back office" + back_end: "Back office only" payment_methods: new: new_payment_method: "New Payment Method" diff --git a/db/migrate/20200508101630_convert_frontend_shipping_method_to_both.rb b/db/migrate/20200508101630_convert_frontend_shipping_method_to_both.rb new file mode 100644 index 00000000000..5e2418c3b85 --- /dev/null +++ b/db/migrate/20200508101630_convert_frontend_shipping_method_to_both.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class ConvertFrontendShippingMethodToBoth < ActiveRecord::Migration + def up + # The display_on value front_end is not working + # (it's not being used in the back office to ignore shipping methods marked as front_end) + # So, here we are converting all entries to the more generic "both" option + # both is represented as nil in the database + # # This enables us to remove the front_end option from the code + execute("UPDATE spree_shipping_methods SET display_on = null WHERE display_on = 'front_end'") + end +end diff --git a/db/schema.rb b/db/schema.rb index e08122c5d68..99415dc9fab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20200430105459) do +ActiveRecord::Schema.define(:version => 20200508101630) do create_table "adjustment_metadata", :force => true do |t| t.integer "adjustment_id" From 34d8b1957e01938adc4589ad7c170b3739c79e81 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 8 May 2020 12:40:46 +0100 Subject: [PATCH 2/4] Improve variable names --- spec/helpers/enterprises_helper_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/helpers/enterprises_helper_spec.rb b/spec/helpers/enterprises_helper_spec.rb index 0933abb7480..6bd6a46dd4d 100644 --- a/spec/helpers/enterprises_helper_spec.rb +++ b/spec/helpers/enterprises_helper_spec.rb @@ -8,8 +8,8 @@ before { allow(helper).to receive(:spree_current_user) { user } } describe "loading available shipping methods" do - let!(:sm1) { create(:shipping_method, require_ship_address: false, distributors: [distributor]) } - let!(:sm2) { create(:shipping_method, require_ship_address: false, distributors: [some_other_distributor]) } + let!(:distributor_shipping_method) { create(:shipping_method, require_ship_address: false, distributors: [distributor]) } + let!(:other_distributor_shipping_method) { create(:shipping_method, require_ship_address: false, distributors: [some_other_distributor]) } context "when the order has no current_distributor" do before do @@ -25,8 +25,8 @@ before { allow(helper).to receive(:current_distributor) { distributor } } it "finds the shipping methods for the current distributor" do - expect(helper.available_shipping_methods).to_not include sm2 - expect(helper.available_shipping_methods).to include sm1 + expect(helper.available_shipping_methods).to_not include other_distributor_shipping_method + expect(helper.available_shipping_methods).to include distributor_shipping_method end end @@ -44,8 +44,8 @@ is_default: true, preferred_shipping_method_tags: "local-delivery") } - let!(:tagged_sm) { sm1 } - let!(:untagged_sm) { sm2 } + let!(:tagged_sm) { distributor_shipping_method } + let!(:untagged_sm) { other_distributor_shipping_method } before do tagged_sm.update_attribute(:tag_list, 'local-delivery') From 0a6bd1424c469f6a02ed5fa1fb8a3bf49c61ad5b Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 8 May 2020 11:41:32 +0100 Subject: [PATCH 3/4] Make ship method display on back_end work correcly by making checkout ignore ship methods configured for backoffice only Adding both unit and feature tests as this is important enough for that --- app/helpers/enterprises_helper.rb | 2 +- app/models/spree/shipping_method_decorator.rb | 1 + spec/features/consumer/shopping/checkout_spec.rb | 9 +++++++++ spec/helpers/enterprises_helper_spec.rb | 8 ++++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/helpers/enterprises_helper.rb b/app/helpers/enterprises_helper.rb index 64d78e7e938..eaaa4c3e865 100644 --- a/app/helpers/enterprises_helper.rb +++ b/app/helpers/enterprises_helper.rb @@ -14,7 +14,7 @@ def current_customer def available_shipping_methods return [] if current_distributor.blank? - shipping_methods = current_distributor.shipping_methods + 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) diff --git a/app/models/spree/shipping_method_decorator.rb b/app/models/spree/shipping_method_decorator.rb index ba7836899d9..0057f8507bb 100644 --- a/app/models/spree/shipping_method_decorator.rb +++ b/app/models/spree/shipping_method_decorator.rb @@ -30,6 +30,7 @@ } scope :by_name, -> { order('spree_shipping_methods.name ASC') } + scope :display_on_checkout, -> { where("display_on is null OR display_on = ''") } # Return the services (pickup, delivery) that different distributors provide, in the format: # {distributor_id => {pickup: true, delivery: false}, ...} diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index 61a89bc6adc..a6135bf88c3 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -229,6 +229,15 @@ end end + it "filters out 'Back office only' shipping methods" do + expect(page).to have_content shipping_with_fee.name + shipping_with_fee.update_attribute :display_on, 'back_end' # Back office only + + visit checkout_path + checkout_as_guest + expect(page).not_to have_content shipping_with_fee.name + end + context "using FilterShippingMethods" do let(:user) { create(:user) } let(:customer) { create(:customer, user: user, enterprise: distributor) } diff --git a/spec/helpers/enterprises_helper_spec.rb b/spec/helpers/enterprises_helper_spec.rb index 6bd6a46dd4d..2ad922a259c 100644 --- a/spec/helpers/enterprises_helper_spec.rb +++ b/spec/helpers/enterprises_helper_spec.rb @@ -28,6 +28,14 @@ expect(helper.available_shipping_methods).to_not include other_distributor_shipping_method expect(helper.available_shipping_methods).to include distributor_shipping_method end + + it "does not return 'back office only' shipping method" do + backoffice_only_shipping_method = create(:shipping_method, require_ship_address: false, distributors: [distributor], display_on: 'back_end') + + expect(helper.available_shipping_methods).to_not include backoffice_only_shipping_method + expect(helper.available_shipping_methods).to_not include other_distributor_shipping_method + expect(helper.available_shipping_methods).to include distributor_shipping_method + end end context "when FilterShippingMethods tag rules are in effect" do From 37350fcbb0767e3822b70a893a4ce3b4963e067d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 18 May 2020 10:55:03 +0100 Subject: [PATCH 4/4] Add table name to condition so it doesnt cause trouble in the future if mixed with other tables in the same query --- app/models/spree/shipping_method_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/shipping_method_decorator.rb b/app/models/spree/shipping_method_decorator.rb index 0057f8507bb..ec1b0a8dca4 100644 --- a/app/models/spree/shipping_method_decorator.rb +++ b/app/models/spree/shipping_method_decorator.rb @@ -30,7 +30,7 @@ } scope :by_name, -> { order('spree_shipping_methods.name ASC') } - scope :display_on_checkout, -> { where("display_on is null OR display_on = ''") } + scope :display_on_checkout, -> { where("spree_shipping_methods.display_on is null OR spree_shipping_methods.display_on = ''") } # Return the services (pickup, delivery) that different distributors provide, in the format: # {distributor_id => {pickup: true, delivery: false}, ...}