From 69b57544f15790b5d1dd4fb68bdd41f15936da32 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 4 Mar 2020 15:18:04 +0000 Subject: [PATCH 1/4] Bring Spree::Variant#active so that we can make it return just variants without includes This makes the variants returned not readonly in rails 4 and thus fixes a spec in Spree::VariantsController#destroy --- app/models/spree/variant_decorator.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 566394d946..bd4b2bcb14 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -85,6 +85,10 @@ Spree::Variant.class_eval do ] end + def self.active(currency = nil) + where(id: joins(:prices).where(deleted_at: nil).where('spree_prices.currency' => currency || Spree::Config[:currency]).where('spree_prices.amount IS NOT NULL').select("spree_variants.id")) + end + # We override in_stock? to avoid depending on the non-overridable method Spree::Stock::Quantifier.can_supply? # VariantStock implements can_supply? itself which depends on overridable methods def in_stock?(quantity = 1) From a5184cce9d16882c38b05472e3d14c823f271c76 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 22 Mar 2020 10:01:46 +0000 Subject: [PATCH 2/4] Make method a bit more readable and add comment with details --- app/models/spree/variant_decorator.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index bd4b2bcb14..3bc8c10fe2 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -86,7 +86,13 @@ Spree::Variant.class_eval do end def self.active(currency = nil) - where(id: joins(:prices).where(deleted_at: nil).where('spree_prices.currency' => currency || Spree::Config[:currency]).where('spree_prices.amount IS NOT NULL').select("spree_variants.id")) + # "where(id:" is necessary so that the returned relation has no includes + # The relation without includes will not be readonly and allow updates on it + where(id: joins(:prices). + where(deleted_at: nil). + where('spree_prices.currency' => currency || Spree::Config[:currency]). + where('spree_prices.amount IS NOT NULL'). + select("spree_variants.id")) end # We override in_stock? to avoid depending on the non-overridable method Spree::Stock::Quantifier.can_supply? From fbbe586996dd4a1fd55c9d15a7386431de79259b Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 27 Mar 2020 09:52:45 +0000 Subject: [PATCH 3/4] Avoid rails 3 bug where the first where clause is overriden by a second where clause Co-Authored-By: Maikel --- app/models/spree/variant_decorator.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 3bc8c10fe2..a1c4aa2fab 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -88,11 +88,12 @@ Spree::Variant.class_eval do def self.active(currency = nil) # "where(id:" is necessary so that the returned relation has no includes # The relation without includes will not be readonly and allow updates on it - where(id: joins(:prices). - where(deleted_at: nil). - where('spree_prices.currency' => currency || Spree::Config[:currency]). - where('spree_prices.amount IS NOT NULL'). - select("spree_variants.id")) + where("spree_variants.id in (?)", joins(:prices). + where(deleted_at: nil). + where('spree_prices.currency' => + currency || Spree::Config[:currency]). + where('spree_prices.amount IS NOT NULL'). + select("spree_variants.id")) end # We override in_stock? to avoid depending on the non-overridable method Spree::Stock::Quantifier.can_supply? From 635ea9c5050044593b4df9d30066c85673da8204 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 27 Mar 2020 10:05:25 +0000 Subject: [PATCH 4/4] Fix some long lines on variant_decorator --- .rubocop_manual_todo.yml | 1 - app/models/spree/variant_decorator.rb | 23 ++++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 77b598ad3c..2ae337c9bd 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -89,7 +89,6 @@ Layout/LineLength: - app/models/spree/tax_rate_decorator.rb - app/models/spree/taxon_decorator.rb - app/models/spree/user.rb - - app/models/spree/variant_decorator.rb - app/models/subscription.rb - app/models/variant_override.rb - app/models/variant_override_set.rb diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index a1c4aa2fab..c23ab202cd 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -16,7 +16,8 @@ Spree::Variant.class_eval do has_many :variant_overrides has_many :inventory_items - attr_accessible :unit_value, :unit_description, :images_attributes, :display_as, :display_name, :import_date + attr_accessible :unit_value, :unit_description, :images_attributes, + :display_as, :display_name, :import_date accepts_nested_attributes_for :images validates :unit_value, presence: true, if: ->(variant) { @@ -53,13 +54,23 @@ Spree::Variant.class_eval do } scope :visible_for, lambda { |enterprise| - joins(:inventory_items).where('inventory_items.enterprise_id = (?) AND inventory_items.visible = (?)', enterprise, true) + joins(:inventory_items). + where( + 'inventory_items.enterprise_id = (?) AND inventory_items.visible = (?)', + enterprise, + true + ) } scope :not_hidden_for, lambda { |enterprise| return where("1=0") if enterprise.blank? - joins("LEFT OUTER JOIN (SELECT * from inventory_items WHERE enterprise_id = #{sanitize enterprise.andand.id}) AS o_inventory_items ON o_inventory_items.variant_id = spree_variants.id") + joins(" + LEFT OUTER JOIN (SELECT * + FROM inventory_items + WHERE enterprise_id = #{sanitize enterprise.andand.id}) + AS o_inventory_items + ON o_inventory_items.variant_id = spree_variants.id") .where("o_inventory_items.id IS NULL OR o_inventory_items.visible = (?)", true) } @@ -68,7 +79,8 @@ Spree::Variant.class_eval do scope :stockable_by, lambda { |enterprise| return where("1=0") if enterprise.blank? - joins(:product).where(spree_products: { id: Spree::Product.stockable_by(enterprise).pluck(:id) }) + joins(:product). + where(spree_products: { id: Spree::Product.stockable_by(enterprise).pluck(:id) }) } # Define sope as class method to allow chaining with other scopes filtering id. @@ -96,7 +108,8 @@ Spree::Variant.class_eval do select("spree_variants.id")) end - # We override in_stock? to avoid depending on the non-overridable method Spree::Stock::Quantifier.can_supply? + # We override in_stock? to avoid depending + # on the non-overridable method Spree::Stock::Quantifier.can_supply? # VariantStock implements can_supply? itself which depends on overridable methods def in_stock?(quantity = 1) can_supply?(quantity)