From 8bf24959d38b7e83c4e33cb0931f2bc02534e7ea Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 23 Nov 2016 15:43:47 -0800 Subject: [PATCH 1/5] Add ShippingMethod#available_to_users This replaces ShippingMethod#display_on with ShippingMethod#available_to_users to match a similar change made to payment methods. Unlike with payment_methods, this doesn't add an #available_to_admin column, because the existing functionality did not provide a way to hide shipping methods from admins. Having display_on="front_end" was identical to having display_on="both" (or actually any string other than "back_end"). --- core/app/models/spree/shipping_method.rb | 12 ++++++ ...remove_display_on_from_shipping_methods.rb | 20 ++++++++++ .../spec/models/spree/shipping_method_spec.rb | 37 +++++++++++++++++++ 3 files changed, 69 insertions(+) create mode 100644 core/db/migrate/20161123154034_add_available_to_users_and_remove_display_on_from_shipping_methods.rb diff --git a/core/app/models/spree/shipping_method.rb b/core/app/models/spree/shipping_method.rb index 6131ee28ac7..908402330eb 100644 --- a/core/app/models/spree/shipping_method.rb +++ b/core/app/models/spree/shipping_method.rb @@ -79,6 +79,18 @@ def build_tracking_url(tracking) tracking_url.gsub(/:tracking/, ERB::Util.url_encode(tracking)) # :url_encode exists in 1.8.7 through 2.1.0 end + def display_on + if available_to_users? + "both" + else + "back_end" + end + end + + def display_on=(value) + self.available_to_users = (value != "back_end") + end + # Some shipping methods are only meant to be set via backend def frontend? display_on != "back_end" diff --git a/core/db/migrate/20161123154034_add_available_to_users_and_remove_display_on_from_shipping_methods.rb b/core/db/migrate/20161123154034_add_available_to_users_and_remove_display_on_from_shipping_methods.rb new file mode 100644 index 00000000000..d203e14ac86 --- /dev/null +++ b/core/db/migrate/20161123154034_add_available_to_users_and_remove_display_on_from_shipping_methods.rb @@ -0,0 +1,20 @@ +class AddAvailableToUsersAndRemoveDisplayOnFromShippingMethods < ActiveRecord::Migration[5.0] + def up + add_column(:spree_shipping_methods, :available_to_users, :boolean, default: true) + execute("UPDATE spree_shipping_methods "\ + "SET available_to_users=#{quoted_false} "\ + "WHERE display_on='back_end'") + remove_column(:spree_shipping_methods, :display_on) + end + + def down + add_column(:spree_shipping_methods, :display_on, :string) + execute("UPDATE spree_shipping_methods "\ + "SET display_on='both' "\ + "WHERE (available_to_users=#{quoted_true}") + execute("UPDATE spree_shipping_methods "\ + "SET display_on='back_end' "\ + "WHERE (available_to_users=#{quoted_false})") + remove_column(:spree_shipping_methods, :available_to_users) + end +end diff --git a/core/spec/models/spree/shipping_method_spec.rb b/core/spec/models/spree/shipping_method_spec.rb index e18a8daf43b..5cb33ad9f61 100644 --- a/core/spec/models/spree/shipping_method_spec.rb +++ b/core/spec/models/spree/shipping_method_spec.rb @@ -210,4 +210,41 @@ def matching(categories) it { is_expected.to_not include(shipping_method) } end end + + describe "display_on=" do + subject do + described_class.new(display_on: display_on).available_to_users + end + + context "with 'back_end'" do + let(:display_on) { 'back_end' } + it { should be false } + end + + context "with 'both'" do + let(:display_on) { 'both' } + it { should be true } + end + + context "with 'front_end'" do + let(:display_on) { 'front_end' } + it { should be true } + end + end + + describe "display_on" do + subject do + described_class.new(available_to_users: available_to_users).display_on + end + + context "when available_to_users is true" do + let(:available_to_users) { true } + it { should == 'both' } + end + + context "when available_to_users is false" do + let(:available_to_users) { false } + it { should == 'back_end' } + end + end end From 577ff1b13113cb468d92c6f2e833f5f8bc8709ec Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 23 Nov 2016 16:20:28 -0800 Subject: [PATCH 2/5] Use ShippingMethod#available_to_users in admin --- .../spree/admin/shipping_methods/_form.html.erb | 14 ++++++-------- .../spree/admin/shipping_methods/index.html.erb | 4 ++-- .../admin/configuration/shipping_methods_spec.rb | 2 +- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/backend/app/views/spree/admin/shipping_methods/_form.html.erb b/backend/app/views/spree/admin/shipping_methods/_form.html.erb index 20fbcef4823..bb1e6333fec 100644 --- a/backend/app/views/spree/admin/shipping_methods/_form.html.erb +++ b/backend/app/views/spree/admin/shipping_methods/_form.html.erb @@ -7,14 +7,6 @@ <% end %> -
- <%= f.field_container :display_on do %> - <%= f.label :display_on %>
- <%= select(:shipping_method, :display_on, Spree::ShippingMethod::DISPLAY.collect { |display| [Spree.t(display), display == :both ? nil : display.to_s] }, {}, {:class => 'select2 fullwidth'}) %> - <%= error_message_on :shipping_method, :display_on %> - <% end %> -
-
<%= f.field_container :admin_name do %> <%= f.label :admin_name %>
@@ -56,6 +48,12 @@
+<%= f.field_container :available_to_users do %> + <%= f.check_box(:available_to_users) %> + <%= f.label :available_to_users %> + <%= error_message_on :shipping_method, :available_to_users %> +<% end %> +
diff --git a/backend/app/views/spree/admin/shipping_methods/index.html.erb b/backend/app/views/spree/admin/shipping_methods/index.html.erb index e4b4903ffe4..9c14ed8211d 100644 --- a/backend/app/views/spree/admin/shipping_methods/index.html.erb +++ b/backend/app/views/spree/admin/shipping_methods/index.html.erb @@ -27,7 +27,7 @@ <%= Spree::ShippingMethod.human_attribute_name(:name) %> <%= Spree::Zone.model_name.human %> <%= Spree::Calculator.model_name.human %> - <%= Spree::ShippingMethod.human_attribute_name(:display_on) %> + <%= Spree::ShippingMethod.human_attribute_name(:available_to_users) %> @@ -37,7 +37,7 @@ <%= shipping_method.admin_name + ' / ' if shipping_method.admin_name.present? %><%= shipping_method.name %> <%= shipping_method.zones.collect(&:name).join(", ") if shipping_method.zones %> <%= shipping_method.calculator.description %> - <%= shipping_method.display_on.blank? ? Spree.t(:both) : Spree.t(shipping_method.display_on.downcase.tr(" ", "_")) %> + <%= shipping_method.available_to_users? ? Spree.t(:say_yes) : Spree.t(:say_no) %> <% if can?(:update, shipping_method) %> <%= link_to_edit shipping_method, :no_text => true %> diff --git a/backend/spec/features/admin/configuration/shipping_methods_spec.rb b/backend/spec/features/admin/configuration/shipping_methods_spec.rb index d2b6202a270..abba4b58756 100644 --- a/backend/spec/features/admin/configuration/shipping_methods_spec.rb +++ b/backend/spec/features/admin/configuration/shipping_methods_spec.rb @@ -26,7 +26,7 @@ expect(column_text(1)).to eq(shipping_method.name) expect(column_text(2)).to eq(zone.name) expect(column_text(3)).to eq("Flat rate") - expect(column_text(4)).to eq("Both") + expect(column_text(4)).to eq("Yes") end end end From fd7da43fc24ae48dc8444e137f4a38bcd6be76e3 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 23 Nov 2016 16:21:30 -0800 Subject: [PATCH 3/5] Fix layout padding issues in shipping method form --- .../app/views/spree/admin/shipping_methods/_form.html.erb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/app/views/spree/admin/shipping_methods/_form.html.erb b/backend/app/views/spree/admin/shipping_methods/_form.html.erb index bb1e6333fec..f25132ea549 100644 --- a/backend/app/views/spree/admin/shipping_methods/_form.html.erb +++ b/backend/app/views/spree/admin/shipping_methods/_form.html.erb @@ -56,7 +56,7 @@
-
+
<%= plural_resource_name(Spree::ShippingCategory) %> <%= f.field_container :categories do %> @@ -71,7 +71,7 @@
-
+
<%= plural_resource_name(Spree::Zone) %> <%= f.field_container :zones do %> @@ -89,10 +89,10 @@
-
+
<%= render :partial => 'spree/admin/shared/calculator_fields', :locals => { :f => f } %>
-
+
<%= Spree::TaxCategory.model_name.human %> <%= f.field_container :tax_categories do %> From 58ab9eb6de5cd48377e86b9ad6e3cd825225a828 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 23 Nov 2016 16:23:47 -0800 Subject: [PATCH 4/5] Deprecate ShippingMethod#display_on --- .../spec/features/admin/orders/customer_details_spec.rb | 2 +- core/app/models/spree/shipping_method.rb | 5 ++++- core/app/models/spree/stock/estimator.rb | 2 +- core/spec/models/spree/shipping_method_spec.rb | 8 ++++++-- core/spec/models/spree/stock/estimator_spec.rb | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/backend/spec/features/admin/orders/customer_details_spec.rb b/backend/spec/features/admin/orders/customer_details_spec.rb index a45669093e8..7af11cbcd3b 100644 --- a/backend/spec/features/admin/orders/customer_details_spec.rb +++ b/backend/spec/features/admin/orders/customer_details_spec.rb @@ -5,7 +5,7 @@ let(:country) { create(:country, name: "Kangaland") } let(:state) { create(:state, name: "Alabama", country: country) } - let!(:shipping_method) { create(:shipping_method, display_on: "front_end") } + let!(:shipping_method) { create(:shipping_method) } let!(:order) { create(:order, ship_address: ship_address, bill_address: bill_address, state: 'complete', completed_at: "2011-02-01 12:36:15") } let!(:product) { create(:product_in_stock) } diff --git a/core/app/models/spree/shipping_method.rb b/core/app/models/spree/shipping_method.rb index 908402330eb..cbe8db06749 100644 --- a/core/app/models/spree/shipping_method.rb +++ b/core/app/models/spree/shipping_method.rb @@ -86,15 +86,18 @@ def display_on "back_end" end end + deprecate display_on: :available_to_users?, deprecator: Spree::Deprecation def display_on=(value) self.available_to_users = (value != "back_end") end + deprecate 'display_on=': :available_to_users=, deprecator: Spree::Deprecation # Some shipping methods are only meant to be set via backend def frontend? - display_on != "back_end" + available_to_users? end + deprecate frontend?: :available_to_users?, deprecator: Spree::Deprecation private diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb index cc071c4c145..5ba5b88417e 100644 --- a/core/app/models/spree/stock/estimator.rb +++ b/core/app/models/spree/stock/estimator.rb @@ -16,7 +16,7 @@ def shipping_rates(package, frontend_only = true) raise OrderRequired if package.shipment.order.nil? rates = calculate_shipping_rates(package) - rates.select! { |rate| rate.shipping_method.frontend? } if frontend_only + rates.select! { |rate| rate.shipping_method.available_to_users? } if frontend_only choose_default_shipping_rate(rates) Spree::Config.shipping_rate_sorter_class.new(rates).sort end diff --git a/core/spec/models/spree/shipping_method_spec.rb b/core/spec/models/spree/shipping_method_spec.rb index 5cb33ad9f61..ac95fa59221 100644 --- a/core/spec/models/spree/shipping_method_spec.rb +++ b/core/spec/models/spree/shipping_method_spec.rb @@ -213,7 +213,9 @@ def matching(categories) describe "display_on=" do subject do - described_class.new(display_on: display_on).available_to_users + Spree::Deprecation.silence do + described_class.new(display_on: display_on).available_to_users + end end context "with 'back_end'" do @@ -234,7 +236,9 @@ def matching(categories) describe "display_on" do subject do - described_class.new(available_to_users: available_to_users).display_on + Spree::Deprecation.silence do + described_class.new(available_to_users: available_to_users).display_on + end end context "when available_to_users is true" do diff --git a/core/spec/models/spree/stock/estimator_spec.rb b/core/spec/models/spree/stock/estimator_spec.rb index 5bc5e2932fd..675b12c5941 100644 --- a/core/spec/models/spree/stock/estimator_spec.rb +++ b/core/spec/models/spree/stock/estimator_spec.rb @@ -126,7 +126,7 @@ module Stock context "involves backend only shipping methods" do before{ Spree::ShippingMethod.destroy_all } - let!(:backend_method) { create(:shipping_method, display_on: "back_end", cost: 0.00) } + let!(:backend_method) { create(:shipping_method, available_to_users: false, cost: 0.00) } let!(:generic_method) { create(:shipping_method, cost: 5.00) } it "does not return backend rates at all" do From f52bad1d246a14e3d4e7bfaf27d1328359db5241 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 30 Nov 2016 15:31:21 -0800 Subject: [PATCH 5/5] Deprecate Spree::ShippingMethod::DISPLAY --- core/app/models/spree/shipping_method.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/shipping_method.rb b/core/app/models/spree/shipping_method.rb index cbe8db06749..572ee7ae620 100644 --- a/core/app/models/spree/shipping_method.rb +++ b/core/app/models/spree/shipping_method.rb @@ -4,7 +4,11 @@ module Spree class ShippingMethod < Spree::Base acts_as_paranoid include Spree::CalculatedAdjustments - DISPLAY = [:both, :front_end, :back_end] + DISPLAY = ActiveSupport::Deprecation::DeprecatedObjectProxy.new( + [:both, :front_end, :back_end], + "Spree::ShippingMethod::DISPLAY is deprecated", + Spree::Deprecation + ) has_many :shipping_method_categories, dependent: :destroy has_many :shipping_categories, through: :shipping_method_categories