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

Add association between stores and shipping #2557

Merged
merged 3 commits into from
Feb 21, 2018
Merged
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
8 changes: 8 additions & 0 deletions backend/app/views/spree/admin/shipping_methods/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@
<% end %>
</div>

<div class="col-5">
<%= f.field_container :store_ids do %>
<%= f.label :store_ids, plural_resource_name(Spree::Store) %>
<%= f.collection_select :store_ids, Spree::Store.all, :id, :name, {}, multiple: true, class: "select2 fullwidth" %>
<%= error_message_on :shipping_method, :store_ids %>
<% end %>
</div>

<div data-hook="admin_shipping_method_form_tracking_url_field" class="col-10">
<%= f.field_container :tracking_url do %>
<%= f.label :tracking_url %><br />
Expand Down
8 changes: 8 additions & 0 deletions core/app/models/spree/shipping_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,18 @@ class ShippingMethod < Spree::Base
has_many :shipping_method_stock_locations, dependent: :destroy, class_name: "Spree::ShippingMethodStockLocation"
has_many :stock_locations, through: :shipping_method_stock_locations

has_many :store_shipping_methods, inverse_of: :shipping_method
has_many :stores, through: :store_shipping_methods

validates :name, presence: true

validate :at_least_one_shipping_category

scope :available_to_store, ->(store) do
raise ArgumentError, "You must provide a store" if store.nil?
store.shipping_methods.empty? ? all : where(id: store.shipping_method_ids)
Copy link
Contributor

@jhawthorn jhawthorn Feb 6, 2018

Choose a reason for hiding this comment

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

I think this is the opposite of the direction that we should have for the "none means all" rule.

I'd rather have "shipping method with no store is available to all stores" not "a store with no shipping methods can use all shpiping methods"

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently payment methods do the same

https://github.com/solidusio/solidus/blob/master/core/app/models/spree/payment_method.rb#L39-L42

I'm not really a fan of that either

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the direction we use for most of these types of associations. (As you found with payment methods.)

I agree that it's confusing and I'm also not a fan of the "none means all" pattern we've adopted, but it's probably best to at least keep them consistent.

end

# @param shipping_category_ids [Array<Integer>] ids of desired shipping categories
# @return [ActiveRecord::Relation] shipping methods which are associated
# with all of the provided shipping categories
Expand Down
1 change: 1 addition & 0 deletions core/app/models/spree/stock/estimator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def calculate_shipping_rates(package)

def shipping_methods(package)
package.shipping_methods
.available_to_store(package.shipment.order.store)
.available_for_address(package.shipment.order.ship_address)
.includes(:calculator)
.to_a
Expand Down
4 changes: 4 additions & 0 deletions core/app/models/spree/store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ module Spree
class Store < Spree::Base
has_many :store_payment_methods, inverse_of: :store
has_many :payment_methods, through: :store_payment_methods

has_many :store_shipping_methods, inverse_of: :store
has_many :shipping_methods, through: :store_shipping_methods

has_many :orders, class_name: "Spree::Order"

validates :code, presence: true, uniqueness: { allow_blank: true }
Expand Down
6 changes: 6 additions & 0 deletions core/app/models/spree/store_shipping_method.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module Spree
class StoreShippingMethod < Spree::Base
belongs_to :store, inverse_of: :store_shipping_methods
belongs_to :shipping_method, inverse_of: :store_shipping_methods
end
end
10 changes: 10 additions & 0 deletions core/db/migrate/20180202222641_create_store_shipping_methods.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class CreateStoreShippingMethods < ActiveRecord::Migration[5.1]
def change
create_table :spree_store_shipping_methods do |t|
t.references :store, foreign_key: { to_table: "spree_stores" }
Copy link
Contributor

@jhawthorn jhawthorn Feb 22, 2018

Choose a reason for hiding this comment

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

I'm not sure this syntax is valid. It fails under MySQL with

Mysql2::Error: Can't create table `solidus_core_test`.`spree_store_shipping_methods` (errno: 150 "Foreign key constraint is incorrectly formed")

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading more, to_table does seem like a valid argument (I had thought it was not), but for some reason this isn't working.

Copy link
Contributor

@jhawthorn jhawthorn Feb 22, 2018

Choose a reason for hiding this comment

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

Fails after

   (60.4ms)  CREATE TABLE `spree_store_shipping_methods` (`id` bigint NOT NULL AUTO_INCREMENT PRIMARY KEY, `store_id` bigint, `shipping_method_id` bigint, `created_at` datetime NOT NULL, `updated_at` datetime NOT NULL,  INDEX `index_spree_store_shipping_methods_on_store_id`  (`store_id`),  INDEX `index_spree_store_shipping_methods_on_shipping_method_id`  (`shipping_method_id`), CONSTRAINT `fk_rails_1ec4e4430e`
FOREIGN KEY (`store_id`)
  REFERENCES `spree_stores` (`id`)
, CONSTRAINT `fk_rails_24cc4a3cd1`
FOREIGN KEY (`shipping_method_id`)
  REFERENCES `spree_shipping_methods` (`id`)
) ENGINE=InnoDB

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that this is creating an FK from a bigint to a normal size int.

Removed the FK entirely in in #2596

t.references :shipping_method, foreign_key: { to_table: "spree_shipping_methods" }

t.timestamps
end
end
end
28 changes: 28 additions & 0 deletions core/spec/models/spree/stock/estimator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,34 @@ module Stock
end
end

context "excludes shipping methods from other stores" do
before{ Spree::ShippingMethod.all.each(&:really_destroy!) }

let!(:other_method) do
create(
:shipping_method,
cost: 0.00,
stores: [build(:store, name: "Other")]
)
end

let!(:main_method) do
create(
:shipping_method,
cost: 5.00,
stores: [order.store]
)
end

it "does not return the other rate at all" do
expect(subject.shipping_rates(package).map(&:shipping_method_id)).to eq([main_method.id])
end

it "doesn't select the other rate even if it's more affordable" do
expect(subject.shipping_rates(package).map(&:selected)).to eq [true]
end
end

context "includes tax adjustments if applicable" do
let(:zone) { create(:zone, countries: [order.tax_address.country])}

Expand Down