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

Replace ShippingMethod#display_on with ShippingMethod#available_to_users #1611

Merged
merged 5 commits into from
Dec 1, 2016
Merged
Show file tree
Hide file tree
Changes from 4 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
22 changes: 10 additions & 12 deletions backend/app/views/spree/admin/shipping_methods/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,6 @@
<% end %>
</div>

<div data-hook="admin_shipping_method_form_display_field" class="col-xs-5">
<%= f.field_container :display_on do %>
<%= f.label :display_on %><br />
<%= 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 %>
</div>

<div data-hook="admin_shipping_method_form_internal_name_field" class="col-xs-5">
<%= f.field_container :admin_name do %>
<%= f.label :admin_name %><br />
Expand Down Expand Up @@ -56,9 +48,15 @@
</div>
</div>

<%= 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 %>

<div class="row">
<div class="col-xs-5">
<div data-hook="admin_shipping_method_form_availability_fields" class="col-xs-5">
<div data-hook="admin_shipping_method_form_availability_fields">
<fieldset class="categories no-border-bottom">
<legend align="center"><%= plural_resource_name(Spree::ShippingCategory) %></legend>
<%= f.field_container :categories do %>
Expand All @@ -73,7 +71,7 @@
</fieldset>
</div>

<div class="col-xs-5">
<div>
<fieldset class="no-border-bottom">
<legend align="center"><%= plural_resource_name(Spree::Zone) %></legend>
<%= f.field_container :zones do %>
Expand All @@ -91,10 +89,10 @@
</div>
</div>
<div class="col-xs-5">
<div data-hook="admin_shipping_method_form_calculator_fields" class="col-xs-5">
<div data-hook="admin_shipping_method_form_calculator_fields">
<%= render :partial => 'spree/admin/shared/calculator_fields', :locals => { :f => f } %>
</div>
<div class="col-xs-5">
<div>
<fieldset class="tax_categories no-border-bottom">
<legend align="center"><%= Spree::TaxCategory.model_name.human %></legend>
<%= f.field_container :tax_categories do %>
Expand Down
4 changes: 2 additions & 2 deletions backend/app/views/spree/admin/shipping_methods/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<th><%= Spree::ShippingMethod.human_attribute_name(:name) %></th>
<th><%= Spree::Zone.model_name.human %></th>
<th><%= Spree::Calculator.model_name.human %></th>
<th><%= Spree::ShippingMethod.human_attribute_name(:display_on) %></th>
<th><%= Spree::ShippingMethod.human_attribute_name(:available_to_users) %></th>
<th data-hook="admin_shipping_methods_index_header_actions" class="actions"></th>
</tr>
</thead>
Expand All @@ -37,7 +37,7 @@
<td class="align-center"><%= shipping_method.admin_name + ' / ' if shipping_method.admin_name.present? %><%= shipping_method.name %></td>
<td class="align-center"><%= shipping_method.zones.collect(&:name).join(", ") if shipping_method.zones %></td>
<td class="align-center"><%= shipping_method.calculator.description %></td>
<td class="align-center"><%= shipping_method.display_on.blank? ? Spree.t(:both) : Spree.t(shipping_method.display_on.downcase.tr(" ", "_")) %></td>
<td class="align-center"><%= shipping_method.available_to_users? ? Spree.t(:say_yes) : Spree.t(:say_no) %></td>
<td data-hook="admin_shipping_methods_index_row_actions" class="actions">
<% if can?(:update, shipping_method) %>
<%= link_to_edit shipping_method, :no_text => true %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
17 changes: 16 additions & 1 deletion core/app/models/spree/shipping_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,25 @@ 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
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

Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/stock/estimator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
41 changes: 41 additions & 0 deletions core/spec/models/spree/shipping_method_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,45 @@ def matching(categories)
it { is_expected.to_not include(shipping_method) }
end
end

describe "display_on=" do
subject do
Spree::Deprecation.silence do
described_class.new(display_on: display_on).available_to_users
end
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
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
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
2 changes: 1 addition & 1 deletion core/spec/models/spree/stock/estimator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down