From 73372a58ef1814e9ea2f46e29742c2c7ad7b2f41 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 27 Jun 2019 17:32:17 +1000 Subject: [PATCH] Remove unused and dangerous `Exchange#eql?` override The `eql?` override has been added in very early commits but was actually not used except in a test. It also caused performance problems since each call to `eql?` would issue two database queries. A developer would unknowingly trigger these when using `exchanges.uniq`. A mistake that could have happened again in the future. I moved the implementation to the test that was actually using it and made a second test a bit more explicit. --- app/models/exchange.rb | 15 -------- spec/models/exchange_spec.rb | 64 +++++---------------------------- spec/models/order_cycle_spec.rb | 23 +++++++----- 3 files changed, 24 insertions(+), 78 deletions(-) diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 0a4245d948..78e9f4aba7 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -99,21 +99,6 @@ class Exchange < ActiveRecord::Base incoming? ? sender : receiver end - def to_h(core_only = false) - h = attributes.merge('variant_ids' => variant_ids.sort, - 'enterprise_fee_ids' => enterprise_fee_ids.sort) - h.reject! { |k| %w(id order_cycle_id created_at updated_at).include? k } if core_only - h - end - - def eql?(e) - if e.respond_to? :to_h - to_h(true) == e.to_h(true) - else - super e - end - end - def refresh_products_cache OpenFoodNetwork::ProductsCache.exchange_changed self end diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index 3966dbf8e9..f7add7ae47 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -285,61 +285,15 @@ describe Exchange do ex1.update_attribute(:tag_list, "wholesale") ex2 = ex1.clone! new_oc - expect(ex1.eql?(ex2)).to be true + expect(ex1.sender_id).to eq ex2.sender_id + expect(ex1.receiver_id).to eq ex2.receiver_id + expect(ex1.pickup_time).to eq ex2.pickup_time + expect(ex1.pickup_instructions).to eq ex2.pickup_instructions + expect(ex1.incoming).to eq ex2.incoming + expect(ex1.receival_instructions).to eq ex2.receival_instructions + expect(ex1.variant_ids).to eq ex2.variant_ids + expect(ex1.enterprise_fee_ids).to eq ex2.enterprise_fee_ids + expect(ex2.reload.tag_list).to eq ["wholesale"] end - - describe "converting to hash" do - let(:oc) { create(:order_cycle) } - let(:exchange) do - exchange = oc.exchanges.last - exchange.save! - allow(exchange).to receive(:variant_ids) { [1835, 1834] } # Test id ordering - allow(exchange).to receive(:enterprise_fee_ids) { [1493, 1492] } # Test id ordering - exchange - end - - it "converts to a hash" do - expect(exchange.to_h).to eq( - 'id' => exchange.id, 'order_cycle_id' => oc.id, - 'sender_id' => exchange.sender_id, 'receiver_id' => exchange.receiver_id, - 'incoming' => exchange.incoming, - 'variant_ids' => exchange.variant_ids.sort, - 'enterprise_fee_ids' => exchange.enterprise_fee_ids.sort, - 'pickup_time' => exchange.pickup_time, 'pickup_instructions' => exchange.pickup_instructions, - 'receival_instructions' => exchange.receival_instructions, - 'created_at' => exchange.created_at, 'updated_at' => exchange.updated_at - ) - end - - it "converts to a hash of core attributes only" do - expect(exchange.to_h(true)).to eq( - 'sender_id' => exchange.sender_id, 'receiver_id' => exchange.receiver_id, - 'incoming' => exchange.incoming, - 'variant_ids' => exchange.variant_ids.sort, - 'enterprise_fee_ids' => exchange.enterprise_fee_ids.sort, - 'pickup_time' => exchange.pickup_time, 'pickup_instructions' => exchange.pickup_instructions, - 'receival_instructions' => exchange.receival_instructions - ) - end - end - - describe "comparing equality" do - it "compares Exchanges using to_h" do - e1 = Exchange.new - e2 = Exchange.new - - allow(e1).to receive(:to_h) { { 'sender_id' => 456 } } - allow(e2).to receive(:to_h) { { 'sender_id' => 456 } } - - expect(e1.eql?(e2)).to be true - end - - it "compares other objects using super" do - exchange = Exchange.new - exchange_fee = ExchangeFee.new - - expect(exchange.eql?(exchange_fee)).to be false - end - end end diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 98aa6d4b4a..fef465a81f 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -372,14 +372,11 @@ describe OrderCycle do expect(occ.coordinator_fee_ids).to eq(oc.coordinator_fee_ids) expect(occ.preferred_product_selection_from_coordinator_inventory_only).to eq(oc.preferred_product_selection_from_coordinator_inventory_only) - # to_h gives us a unique hash for each exchange - # check that the clone has no additional exchanges - occ.exchanges.map(&:to_h).all? do |ex| - oc.exchanges.map(&:to_h).include? ex - end - # check that the clone has original exchanges - occ.exchanges.map(&:to_h).include? ex1.to_h - occ.exchanges.map(&:to_h).include? ex2.to_h + # Check that the exchanges have been cloned. + original_exchange_attributes = oc.exchanges.map { |ex| core_exchange_attributes(ex) } + cloned_exchange_attributes = occ.exchanges.map { |ex| core_exchange_attributes(ex) } + + expect(cloned_exchange_attributes).to match_array original_exchange_attributes end describe "finding recently closed order cycles" do @@ -467,4 +464,14 @@ describe OrderCycle do expect(item_with_overridden_variant.variant.on_hand).to eq(1000) end end + + def core_exchange_attributes(exchange) + exterior_attribute_keys = %w(id order_cycle_id created_at updated_at) + exchange.attributes. + reject { |k| exterior_attribute_keys.include? k }. + merge( + 'variant_ids' => exchange.variant_ids.sort, + 'enterprise_fee_ids' => exchange.enterprise_fee_ids.sort + ) + end end