From 931c6c85ba24cd53539a0973425b518a06ed9b86 Mon Sep 17 00:00:00 2001 From: McEileen Date: Mon, 9 Dec 2024 20:31:51 -0500 Subject: [PATCH] Fix inventory quantity errors to be tied to organization, not storage location (Fixes #4647) (#4804) * fix item minimum error to consider quantity across organization * display correct alerts when distribution causes distinct items to dip below recommended and minimum levels * refine recommended quantity error to occur based on org quantity, not storage location quantity * fix distribution system specs that failed due to updated error message content * display minimum and recommended amount error messages on separate lines * refactor inventory check alert messages and related spec * fix lint errors --- app/controllers/distributions_controller.rb | 9 +- app/services/inventory_check_service.rb | 26 ++-- .../distributions_controller_spec.rb | 111 ++++++++++++--- spec/services/inventory_check_service_spec.rb | 131 +++++++++++++++--- spec/system/distribution_system_spec.rb | 4 +- 5 files changed, 222 insertions(+), 59 deletions(-) diff --git a/app/controllers/distributions_controller.rb b/app/controllers/distributions_controller.rb index 68a0bb39e8..6a3a55146f 100644 --- a/app/controllers/distributions_controller.rb +++ b/app/controllers/distributions_controller.rb @@ -316,11 +316,8 @@ def filter_params def perform_inventory_check inventory_check_result = InventoryCheckService.new(@distribution).call - if inventory_check_result.error.present? - flash[:error] = inventory_check_result.error - end - if inventory_check_result.alert.present? - flash[:alert] = inventory_check_result.alert - end + alerts = [inventory_check_result.minimum_alert, inventory_check_result.recommended_alert] + merged_alert = alerts.compact.join("\n") + flash[:alert] = merged_alert if merged_alert.present? end end diff --git a/app/services/inventory_check_service.rb b/app/services/inventory_check_service.rb index 8f331887cb..72886dcdb4 100644 --- a/app/services/inventory_check_service.rb +++ b/app/services/inventory_check_service.rb @@ -1,40 +1,40 @@ class InventoryCheckService - attr_reader :error, :alert + attr_reader :minimum_alert, :recommended_alert def initialize(distribution) @distribution = distribution - @alert = nil - @error = nil + @minimum_alert = nil + @recommended_alert = nil end def call @inventory = View::Inventory.new(@distribution.organization_id) unless items_below_minimum_quantity.empty? - set_error + set_minimum_alert end unless deduplicate_items_below_recommended_quantity.empty? - set_alert + set_recommended_alert end self end - def set_error - @error = "The following items have fallen below the minimum " \ - "on hand quantity: #{items_below_minimum_quantity.map(&:name).sort.join(", ")}" + def set_minimum_alert + @minimum_alert = "The following items have fallen below the minimum " \ + "on hand quantity, bank-wide: #{items_below_minimum_quantity.map(&:name).sort.join(", ")}" end - def set_alert - @alert = "The following items have fallen below the recommended " \ - "on hand quantity: #{deduplicate_items_below_recommended_quantity.map(&:name).sort.join(", ")}" + def set_recommended_alert + @recommended_alert = "The following items have fallen below the recommended " \ + "on hand quantity, bank-wide: #{deduplicate_items_below_recommended_quantity.map(&:name).sort.join(", ")}" end def items_below_minimum_quantity # Done this way to prevent N+1 query on items unless @items_below_minimum_quantity item_ids = @distribution.line_items.select do |line_item| - quantity = @inventory.quantity_for(storage_location: @distribution.storage_location_id, item_id: line_item.item_id) + quantity = @inventory.quantity_for(item_id: line_item.item_id) quantity < (line_item.item.on_hand_minimum_quantity || 0) end.map(&:item_id) @@ -48,7 +48,7 @@ def items_below_recommended_quantity # Done this way to prevent N+1 query on items unless @items_below_recommended_quantity item_ids = @distribution.line_items.select do |line_item| - quantity = @inventory.quantity_for(storage_location: @distribution.storage_location_id, item_id: line_item.item_id) + quantity = @inventory.quantity_for(item_id: line_item.item_id) quantity < (line_item.item.on_hand_recommended_quantity || 0) end.map(&:item_id) diff --git a/spec/controllers/distributions_controller_spec.rb b/spec/controllers/distributions_controller_spec.rb index ec8210a941..a04a165377 100644 --- a/spec/controllers/distributions_controller_spec.rb +++ b/spec/controllers/distributions_controller_spec.rb @@ -9,9 +9,57 @@ end describe "POST #create" do - context "when distribution causes inventory quantity to be below minimum quantity" do - let(:item) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5) } - let(:storage_location) { create(:storage_location, :with_items, item: item, item_quantity: 20, organization: organization) } + let(:available_item) { create(:item, name: "Available Item", organization: organization, on_hand_minimum_quantity: 5) } + let!(:first_storage_location) { create(:storage_location, :with_items, item: available_item, item_quantity: 20, organization: organization) } + let!(:second_storage_location) { create(:storage_location, :with_items, item: available_item, item_quantity: 20, organization: organization) } + context "when distribution causes inventory to remain above minimum quantity for an organization" do + let(:params) do + { + organization_name: organization.id, + distribution: { + partner_id: partner.id, + storage_location_id: first_storage_location.id, + line_items_attributes: + { + "0": { item_id: first_storage_location.items.first.id, quantity: 10 } + } + } + } + end + + subject { post :create, params: params.merge(format: :turbo_stream) } + + it "does not display an error" do + subject + + expect(flash[:alert]).to be_nil + end + + context "when distribution causes inventory to fall below minimum quantity for a storage location" do + let(:params) do + { + organization_name: organization.id, + distribution: { + partner_id: partner.id, + storage_location_id: second_storage_location.id, + line_items_attributes: + { + "0": { item_id: second_storage_location.items.first.id, quantity: 18 } + } + } + } + end + it "does not display an error" do + subject + expect(flash[:notice]).to eq("Distribution created!") + expect(flash[:error]).to be_nil + end + end + end + + context "when distribution causes inventory quantity to be below minimum quantity for an organization" do + let(:first_item) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5) } + let(:storage_location) { create(:storage_location, :with_items, item: first_item, item_quantity: 20, organization: organization) } let(:params) do { organization_name: organization.id, @@ -31,11 +79,44 @@ it "redirects with a flash notice and a flash error" do expect(subject).to have_http_status(:redirect) expect(flash[:notice]).to eq("Distribution created!") - expect(flash[:error]).to eq("The following items have fallen below the minimum on hand quantity: Item 1") + expect(flash[:alert]).to eq("The following items have fallen below the minimum on hand quantity, bank-wide: Item 1") + end + + context "when distribution causes inventory quantity to be below recommended quantity for an organization" do + let(:second_item) { create(:item, name: "Item 2", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let(:storage_location) { create(:storage_location, organization: organization) } + let(:params) do + { + organization_name: organization.id, + distribution: { + partner_id: partner.id, + storage_location_id: storage_location.id, + line_items_attributes: + { + "0": { item_id: storage_location.items.first.id, quantity: 18 }, + "1": { item_id: storage_location.items.second.id, quantity: 15 } + } + } + } + end + before do + TestInventory.create_inventory(organization, { + storage_location.id => { + first_item.id => 20, + second_item.id => 20 + } + }) + end + it "displays an error for both minimum and recommended quantity for an organization" do + expect(subject).to have_http_status(:redirect) + expect(flash[:notice]).to eq("Distribution created!") + expect(flash[:alert]).to include("The following items have fallen below the recommended on hand quantity, bank-wide: Item 2") + expect(flash[:alert]).to include("The following items have fallen below the minimum on hand quantity, bank-wide: Item 1") + end end end - context "multiple line_items that have inventory quantity below minimum quantity" do + context "multiple line_items that have inventory quantity below minimum quantity for an organization" do let(:item1) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } let(:item2) { create(:item, name: "Item 2", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } let(:storage_location) { create(:storage_location, organization: organization) } @@ -67,14 +148,13 @@ it "redirects with a flash notice and a flash error" do expect(subject).to have_http_status(:redirect) expect(flash[:notice]).to eq("Distribution created!") - expect(flash[:error]).to include("The following items have fallen below the minimum on hand quantity") - expect(flash[:error]).to include("Item 1") - expect(flash[:error]).to include("Item 2") - expect(flash[:alert]).to be_nil + expect(flash[:alert]).to include("The following items have fallen below the minimum on hand quantity, bank-wide") + expect(flash[:alert]).to include("Item 1") + expect(flash[:alert]).to include("Item 2") end end - context "multiple line_items that have inventory quantity below recommended quantity" do + context "multiple line_items that have inventory quantity below recommended quantity for an organization" do let(:item1) { create(:item, name: "Item 1", organization: organization, on_hand_recommended_quantity: 5) } let(:item2) { create(:item, name: "Item 2", organization: organization, on_hand_recommended_quantity: 5) } let(:storage_location) { create(:storage_location, organization: organization) } @@ -106,7 +186,7 @@ it "redirects with a flash notice and a flash alert" do expect(subject).to have_http_status(:redirect) expect(flash[:notice]).to eq("Distribution created!") - expect(flash[:alert]).to eq("The following items have fallen below the recommended on hand quantity: Item 1, Item 2") + expect(flash[:alert]).to eq("The following items have fallen below the recommended on hand quantity, bank-wide: Item 1, Item 2") end end @@ -136,7 +216,7 @@ end describe "PUT #update" do - context "when distribution causes inventory quantity to be below recommended quantity" do + context "when distribution causes inventory quantity to be below recommended quantity for an organization" do let(:item1) { create(:item, name: "Item 1", organization: organization, on_hand_recommended_quantity: 5) } let(:item2) { create(:item, name: "Item 2", organization: organization, on_hand_recommended_quantity: 5) } let(:storage_location) { create(:storage_location, organization: organization) } @@ -169,11 +249,11 @@ it "redirects with a flash notice and a flash error" do expect(subject).to have_http_status(:redirect) expect(flash[:notice]).to eq("Distribution updated!") - expect(flash[:alert]).to eq("The following items have fallen below the recommended on hand quantity: Item 1, Item 2") + expect(flash[:alert]).to eq("The following items have fallen below the recommended on hand quantity, bank-wide: Item 1, Item 2") end end - context "when distribution causes inventory quantity to be below minimum quantity" do + context "when distribution causes inventory quantity to be below minimum quantity for an organization" do let(:item1) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5) } let(:item2) { create(:item, name: "Item 2", organization: organization, on_hand_minimum_quantity: 5) } let(:storage_location) { create(:storage_location) } @@ -206,8 +286,7 @@ it "redirects with a flash notice and a flash error" do expect(subject).to have_http_status(:redirect) expect(flash[:notice]).to eq("Distribution updated!") - expect(flash[:error]).to eq("The following items have fallen below the minimum on hand quantity: Item 1, Item 2") - expect(flash[:alert]).to be_nil + expect(flash[:alert]).to eq("The following items have fallen below the minimum on hand quantity, bank-wide: Item 1, Item 2") end end diff --git a/spec/services/inventory_check_service_spec.rb b/spec/services/inventory_check_service_spec.rb index 7f6aa9efa6..9182d68eb8 100644 --- a/spec/services/inventory_check_service_spec.rb +++ b/spec/services/inventory_check_service_spec.rb @@ -3,56 +3,143 @@ subject { InventoryCheckService } describe "call" do - let(:item1) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } - let(:item2) { create(:item, name: "Item 2", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } - - context "error" do + context "when on hand quantity is below the minimum for the organization" do + let(:first_item) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let(:second_item) { create(:item, name: "Item 2", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } let(:storage_location) do storage_location = create(:storage_location, organization: organization) TestInventory.create_inventory(storage_location.organization, { storage_location.id => { - item1.id => 4, - item2.id => 4 + first_item.id => 4, + second_item.id => 4 } }) storage_location end + let(:distribution) { create(:distribution, storage_location_id: storage_location.id) } + before do + create(:line_item, item: first_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) + create(:line_item, item: second_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) + end it "should set the error" do - distribution = create(:distribution, storage_location_id: storage_location.id) - create(:line_item, item: item1, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) - create(:line_item, item: item2, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) - result = subject.new(distribution.reload).call - expect(result.error).to eq("The following items have fallen below the minimum on hand quantity: Item 1, Item 2") - expect(result.alert).to be_nil + expect(result.minimum_alert).to eq("The following items have fallen below the minimum on hand quantity, bank-wide: Item 1, Item 2") + expect(result.recommended_alert).to be_nil end end - context "alert" do - let(:storage_location) do + context "when on hand quantity is above the minimum for the organization" do + let(:available_item) { create(:item, name: "Available Item", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let!(:first_storage_location) do + first_storage_location = create(:storage_location, organization: organization) + TestInventory.create_inventory(first_storage_location.organization, { + first_storage_location.id => { + available_item.id => 9 + } + }) + + first_storage_location + end + let!(:second_storage_location) do + second_storage_location = create(:storage_location, organization: organization) + TestInventory.create_inventory(second_storage_location.organization, { + second_storage_location.id => { + available_item.id => 4 + } + }) + + second_storage_location + end + + context "when on hand quantity is below the minimum for one storage location" do + let(:distribution) { create(:distribution, storage_location_id: second_storage_location.id) } + before do + create(:line_item, item: available_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 2) + end + it "should not set the error" do + result = subject.new(distribution.reload).call + + expect(result.minimum_alert).to be_nil + end + end + end + + context "when on hand quantity is above the recommended amount for the organization" do + let(:first_item) { create(:item, name: "Item 1", organization: organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let!(:storage_location) do storage_location = create(:storage_location, organization: organization) TestInventory.create_inventory(storage_location.organization, { storage_location.id => { - item1.id => 9, - item2.id => 9 + first_item.id => 20 + } + }) + + storage_location + end + let(:distribution) { create(:distribution, storage_location_id: storage_location.id) } + before do + create(:line_item, item: first_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 2) + end + it "should not set the alert" do + result = subject.new(distribution.reload).call + + expect(result.recommended_alert).to be_nil + expect(result.minimum_alert).to be_nil + end + + context "when on hand quantity is below the recommended amount for one storage location" do + let!(:second_storage_location) do + second_storage_location = create(:storage_location, organization: organization) + TestInventory.create_inventory(second_storage_location.organization, { + second_storage_location.id => { + first_item.id => 8 + } + }) + + second_storage_location + end + let(:distribution) { create(:distribution, storage_location_id: second_storage_location.id) } + before do + create(:line_item, item: first_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 2) + end + it "should not set the alert" do + result = subject.new(distribution.reload).call + + expect(result.recommended_alert).to be_nil + expect(result.minimum_alert).to be_nil + end + end + end + + context "when on hand quantity is below the recommended amount for the organization" do + let(:somewhat_stocked_organization) { create(:organization) } + let(:first_item) { create(:item, name: "Item 1", organization: somewhat_stocked_organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let(:second_item) { create(:item, name: "Item 2", organization: somewhat_stocked_organization, on_hand_minimum_quantity: 5, on_hand_recommended_quantity: 10) } + let!(:storage_location) do + storage_location = create(:storage_location, organization: somewhat_stocked_organization) + TestInventory.create_inventory(storage_location.organization, { + storage_location.id => { + first_item.id => 9, + second_item.id => 9 } }) storage_location end + let(:distribution) { create(:distribution, storage_location_id: storage_location.id) } + before do + create(:line_item, item: first_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) + create(:line_item, item: second_item, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) + end it "should set the alert" do - distribution = create(:distribution, storage_location_id: storage_location.id) - create(:line_item, item: item1, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) - create(:line_item, item: item2, itemizable_type: "Distribution", itemizable_id: distribution.id, quantity: 16) - result = subject.new(distribution.reload).call - expect(result.alert).to eq("The following items have fallen below the recommended on hand quantity: Item 1, Item 2") - expect(result.error).to be_nil + expect(result.recommended_alert).to eq("The following items have fallen below the recommended on hand quantity, bank-wide: Item 1, Item 2") + expect(result.minimum_alert).to be_nil end end end diff --git a/spec/system/distribution_system_spec.rb b/spec/system/distribution_system_spec.rb index 923abd90ee..5a98ee1ba0 100644 --- a/spec/system/distribution_system_spec.rb +++ b/spec/system/distribution_system_spec.rb @@ -233,7 +233,7 @@ end expect(page).not_to have_content('New Distribution') - expect(page).to have_content("The following items have fallen below the minimum on hand quantity: #{item.name}") + expect(page).to have_content("The following items have fallen below the minimum on hand quantity, bank-wide: #{item.name}") end end @@ -268,7 +268,7 @@ click_button "Yes, it's correct" end - expect(page).to have_content("The following items have fallen below the recommended on hand quantity: #{item.name}") + expect(page).to have_content("The following items have fallen below the recommended on hand quantity, bank-wide: #{item.name}") end end