From 3e9963b09e9e2e6563eaa106673a694b8ae3febf Mon Sep 17 00:00:00 2001 From: JP <85654561+jp524@users.noreply.github.com> Date: Fri, 31 May 2024 13:35:08 -0500 Subject: [PATCH 1/4] Modify Audit.finalized_since? to use updated_at value To be consistent with query in DistributionsController#edit action --- app/models/audit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/audit.rb b/app/models/audit.rb index ab0634915f..3c8a613cf2 100644 --- a/app/models/audit.rb +++ b/app/models/audit.rb @@ -41,7 +41,7 @@ def self.finalized_since?(itemizable, *location_ids) item_ids = itemizable.line_items.pluck(:item_id) where(status: "finalized") .where(storage_location_id: location_ids) - .where(created_at: itemizable.created_at..) + .where(updated_at: itemizable.created_at..) .joins(:line_items) .where(line_items: {item_id: item_ids}) .exists? From b003d6622072eac63bcdcde12fe38151fcc2f8b3 Mon Sep 17 00:00:00 2001 From: JP <85654561+jp524@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:16:40 -0500 Subject: [PATCH 2/4] Raise error if intervening audit on items during ItemizableUpdateService --- app/services/itemizable_update_service.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/services/itemizable_update_service.rb b/app/services/itemizable_update_service.rb index df1fe1a8a3..07e7f0da05 100644 --- a/app/services/itemizable_update_service.rb +++ b/app/services/itemizable_update_service.rb @@ -15,6 +15,8 @@ def self.call(itemizable:, type: :increase, params: {}, event_class: nil) from_location = to_location = itemizable.storage_location to_location = StorageLocation.find(params[:storage_location_id]) if params[:storage_location_id] + verify_intervening_audit_on_storage_location_items(itemizable: itemizable, from_location_id: from_location.id, to_location_id: to_location.id) + apply_change_method = (type == :increase) ? :increase_inventory : :decrease_inventory undo_change_method = (type == :increase) ? :decrease_inventory : :increase_inventory @@ -58,4 +60,21 @@ def self.update_storage_location(itemizable:, apply_change_method:, undo_change_ # Apply the new changes to the storage location inventory to_location.public_send(apply_change_method, itemizable.line_item_values) end + + # @param itemizable [Itemizable] + # @param from_location [StorageLocation] + # @param to_location [StorageLocation] + def self.verify_intervening_audit_on_storage_location_items(itemizable:, from_location_id:, to_location_id:) + return unless from_location_id != to_location_id && Audit.finalized_since?(itemizable, [from_location_id, to_location_id]) + + itemizable_type = itemizable.class.name.downcase + case itemizable_type + when "distribution" + raise "Cannot change the storage location because there has been an intervening audit of some items. " \ + "If you need to change the storage location, please reclaim this distribution and create a new distribution from the new storage location." + else + raise "Cannot change the storage location because there has been an intervening audit of some items. " \ + "If you need to change the storage location, please delete this #{itemizable_type} and create a new #{itemizable_type} with the new storage location." + end + end end From 900f380925df2205510648bc38201f09182346d9 Mon Sep 17 00:00:00 2001 From: JP <85654561+jp524@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:56:32 -0500 Subject: [PATCH 3/4] Add tests --- .../itemizable_update_service_spec.rb | 56 ++++++++++++++----- 1 file changed, 42 insertions(+), 14 deletions(-) diff --git a/spec/services/itemizable_update_service_spec.rb b/spec/services/itemizable_update_service_spec.rb index 54d41e53ce..2eac909587 100644 --- a/spec/services/itemizable_update_service_spec.rb +++ b/spec/services/itemizable_update_service_spec.rb @@ -63,13 +63,27 @@ expect(UpdateExistingEvent.count).to eq(1) end - it "should update quantity in different locations" do - attributes[:storage_location_id] = new_storage_location.id - subject - expect(itemizable.reload.line_items.count).to eq(2) - expect(itemizable.line_items.sum(&:quantity)).to eq(4) - expect(storage_location.size).to eq(10) - expect(new_storage_location.size).to eq(24) + context "when storage location changes" do + context "when there is no intervening audit" do + it "should update quantity in different locations" do + attributes[:storage_location_id] = new_storage_location.id + subject + expect(itemizable.reload.line_items.count).to eq(2) + expect(itemizable.line_items.sum(&:quantity)).to eq(4) + expect(storage_location.size).to eq(10) + expect(new_storage_location.size).to eq(24) + end + end + + context "when there is an intervening audit on one of the items involved" do + it "raises an error" do + msg = "Cannot change the storage location because there has been an intervening audit of some items. " \ + "If you need to change the storage location, please delete this donation and create a new donation with the new storage location." + create(:audit, :with_items, item: itemizable.items.first, organization: organization, storage_location: storage_location, status: "finalized") + attributes[:storage_location_id] = new_storage_location.id + expect { subject }.to raise_error(msg) + end + end end it "should raise an error if any item is inactive" do @@ -114,13 +128,27 @@ expect(itemizable.issued_at).to eq(2.days.ago) end - it "should update quantity in different locations" do - attributes[:storage_location_id] = new_storage_location.id - subject - expect(itemizable.reload.line_items.count).to eq(2) - expect(itemizable.line_items.sum(&:quantity)).to eq(4) - expect(storage_location.size).to eq(30) - expect(new_storage_location.size).to eq(16) + context "when storage location changes" do + context "when there is no intervening audit" do + it "should update quantity in different locations" do + attributes[:storage_location_id] = new_storage_location.id + subject + expect(itemizable.reload.line_items.count).to eq(2) + expect(itemizable.line_items.sum(&:quantity)).to eq(4) + expect(storage_location.size).to eq(30) + expect(new_storage_location.size).to eq(16) + end + end + + context "when there is an intervening audit on one of the items involved" do + it "raises an error" do + msg = "Cannot change the storage location because there has been an intervening audit of some items. " \ + "If you need to change the storage location, please reclaim this distribution and create a new distribution from the new storage location." + create(:audit, :with_items, item: itemizable.items.first, organization: organization, storage_location: storage_location, status: "finalized") + attributes[:storage_location_id] = new_storage_location.id + expect { subject }.to raise_error(msg) + end + end end it "should raise an error if any item is inactive" do From 72dc3921381cb313e8d3ff1109276e36f3fc7af4 Mon Sep 17 00:00:00 2001 From: JP <85654561+jp524@users.noreply.github.com> Date: Fri, 7 Jun 2024 16:38:42 -0500 Subject: [PATCH 4/4] Replace unless by if in ItemizbleUpdateService --- app/services/itemizable_update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/itemizable_update_service.rb b/app/services/itemizable_update_service.rb index 07e7f0da05..45dc7133da 100644 --- a/app/services/itemizable_update_service.rb +++ b/app/services/itemizable_update_service.rb @@ -65,7 +65,7 @@ def self.update_storage_location(itemizable:, apply_change_method:, undo_change_ # @param from_location [StorageLocation] # @param to_location [StorageLocation] def self.verify_intervening_audit_on_storage_location_items(itemizable:, from_location_id:, to_location_id:) - return unless from_location_id != to_location_id && Audit.finalized_since?(itemizable, [from_location_id, to_location_id]) + return if from_location_id == to_location_id || !Audit.finalized_since?(itemizable, [from_location_id, to_location_id]) itemizable_type = itemizable.class.name.downcase case itemizable_type