From ce8a2b325131d8f99a1a2671c4199e84038a9dbe Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Fri, 5 Jul 2024 16:35:40 +0200 Subject: [PATCH 1/2] Fixes Rails/SkipsModelValidations offenses - increments! & decrement! skip validations - replaced increment! method calls - one call was for a redefined increment! method - the other for a regular(ActiveRecord::Persistence) - removes increments/decrements definition now useless --- .rubocop_todo.yml | 8 -------- .../concerns/line_item_stock_changes.rb | 19 ------------------- app/models/spree/line_item.rb | 1 - app/models/variant_override.rb | 6 +----- spec/models/spree/line_item_spec.rb | 8 ++++---- 5 files changed, 5 insertions(+), 37 deletions(-) delete mode 100644 app/models/concerns/line_item_stock_changes.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f9a3ebf631..f3efe519c0 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -644,14 +644,6 @@ Rails/RootPathnameMethods: Exclude: - 'spec/lib/reports/orders_and_fulfillment/order_cycle_customer_totals_report_spec.rb' -# Offense count: 4 -# Configuration parameters: ForbiddenMethods, AllowedMethods. -# ForbiddenMethods: decrement!, decrement_counter, increment!, increment_counter, insert, insert!, insert_all, insert_all!, toggle!, touch, touch_all, update_all, update_attribute, update_column, update_columns, update_counters, upsert, upsert_all -Rails/SkipsModelValidations: - Exclude: - - 'app/models/variant_override.rb' - - 'spec/models/spree/line_item_spec.rb' - # Offense count: 7 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: EnforcedStyle. diff --git a/app/models/concerns/line_item_stock_changes.rb b/app/models/concerns/line_item_stock_changes.rb deleted file mode 100644 index a61bb21be3..0000000000 --- a/app/models/concerns/line_item_stock_changes.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -# Rails 5 introduced some breaking changes to these built-in methods, and the new versions -# no longer work correctly in relation to decrementing stock with LineItems / VariantOverrides. -# The following methods re-instate the pre-Rails-5 versions, which work as expected. -# https://apidock.com/rails/v4.2.9/ActiveRecord/Persistence/increment%21 -# https://apidock.com/rails/v4.2.9/ActiveRecord/Persistence/decrement%21 - -module LineItemStockChanges - extend ActiveSupport::Concern - - def increment!(attribute, by = 1) - increment(attribute, by).update_attribute(attribute, self[attribute]) - end - - def decrement!(attribute, by = 1) - decrement(attribute, by).update_attribute(attribute, self[attribute]) - end -end diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb index fc44eb109a..23fed5fbc6 100644 --- a/app/models/spree/line_item.rb +++ b/app/models/spree/line_item.rb @@ -5,7 +5,6 @@ require 'open_food_network/scope_variant_to_hub' module Spree class LineItem < ApplicationRecord include VariantUnits::VariantAndLineItemNaming - include LineItemStockChanges searchable_attributes :price, :quantity, :order_id, :variant_id, :tax_category_id searchable_associations :order, :order_cycle, :variant, :product, :supplier, :tax_category diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index ea5dca61b2..b4e3553e1c 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -52,11 +52,7 @@ class VariantOverride < ApplicationRecord return end - if quantity > 0 - increment! :count_on_hand, quantity - elsif quantity < 0 - decrement! :count_on_hand, -quantity - end + update!(count_on_hand: (count_on_hand || 0) + quantity) end def default_stock? diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index d4a757cf99..d715fec503 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -349,7 +349,7 @@ module Spree it "draws stock from the variant override" do expect(vo.reload.count_on_hand).to eq 3 - expect{ line_item.increment!(:quantity) } + expect{ line_item.update!(quantity: line_item.quantity + 1) } .not_to change{ Spree::Variant.find(variant.id).on_hand } expect(vo.reload.count_on_hand).to eq 2 end @@ -357,9 +357,9 @@ module Spree context "when a variant override does not apply" do it "draws stock from the variant" do - expect{ line_item.increment!(:quantity) }.to change{ - Spree::Variant.find(variant.id).on_hand - }.by(-1) + expect{ line_item.update!(quantity: line_item.quantity + 1) }.to change{ + Spree::Variant.find(variant.id).on_hand + }.by(-1) end end end From 6d22652dfa619815ce5bd25d094a32515751df5f Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Tue, 9 Jul 2024 16:06:45 +0200 Subject: [PATCH 2/2] Requested changes on VariantOverrride - change was not exactly equivalent, so reverting + rubo comment --- app/models/variant_override.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index b4e3553e1c..cabc309a93 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -52,7 +52,14 @@ class VariantOverride < ApplicationRecord return end - update!(count_on_hand: (count_on_hand || 0) + quantity) + # rubocop:disable Rails/SkipsModelValidations + # Cf. conversation https://github.com/openfoodfoundation/openfoodnetwork/pull/12647 + if quantity > 0 + increment! :count_on_hand, quantity + elsif quantity < 0 + decrement! :count_on_hand, -quantity + end + # rubocop:enable Rails/SkipsModelValidations end def default_stock?