Merge pull request #3982 from rubyforgood/invalid-item-inventory

Invalid item inventory

app/controllers/items_controller.rb

@@ -4,6 +4,7 @@ class ItemsController < ApplicationController
   def index
     @items = current_organization.items.includes(:base_item, :kit).alphabetized.class_filter(filter_params)
     @items = @items.active unless params[:include_inactive_items]
+
     @item_categories = current_organization.item_categories.includes(:items).order('name ASC')
     @kits = current_organization.kits.includes(line_items: :item, inventory_items: :storage_location)
     @storages = current_organization.storage_locations.active_locations.order(id: :asc)
@@ -74,7 +75,17 @@ def show
 
   def update
     @item = current_organization.items.find(params[:id])
-    if @item.update(item_params)
+    @item.attributes = item_params
+
+    deactivated = @item.active_changed? && !@item.active
+    if deactivated && !@item.can_deactivate?
+      @base_items = BaseItem.without_kit.alphabetized
+      flash[:error] = "Can't deactivate this item - it is currently assigned to either an active kit or a storage location!"
+      render action: :edit
+      return
+    end
+
+    if @item.save
       redirect_to items_path, notice: "#{@item.name} updated!"
     else
       @base_items = BaseItem.without_kit.alphabetized

app/controllers/kits_controller.rb

@@ -47,8 +47,12 @@ def deactivate
 
   def reactivate
     @kit = Kit.find(params[:id])
-    @kit.reactivate
-    redirect_back(fallback_location: dashboard_path, notice: "Kit has been reactivated!")
+    if @kit.can_reactivate?
+      @kit.reactivate
+      redirect_back(fallback_location: dashboard_path, notice: "Kit has been reactivated!")
+    else
+      redirect_back(fallback_location: dashboard_path, alert: "Cannot reactivate kit - it has inactive items! Please reactivate the items first.")
+    end
   end
 
   def allocations

app/controllers/storage_locations_controller.rb

@@ -154,8 +154,8 @@ def inventory
                                              .includes(inventory_items: :item)
                                              .find(params[:id])
                                              .inventory_items
+                                           .active
 
-      @inventory_items = @inventory_items.active unless params[:include_inactive_items] == "true"
       @inventory_items += include_omitted_items(@inventory_items.collect(&:item_id)) if params[:include_omitted_items] == "true"
       respond_to :json
     end

app/models/item.rb

@@ -111,6 +111,19 @@ def self.reactivate(item_ids)
     Item.where(id: item_ids).find_each { |item| item.update(active: true) }
   end
 
+  # @return [Boolean]
+  def can_deactivate?
+    # Cannot deactivate if it's currently in inventory in a storage location. It doesn't make sense
+    # to have physical inventory of something we're now saying isn't valid.
+    inventory_items.where("quantity > 0").none? &&
+      # If an active kit includes this item, then changing kit allocations would change inventory
+      # for an inactive item - which we said above we don't want to allow.
+      organization.kits
+        .active
+        .joins(:line_items)
+        .where(line_items: { item_id: id}).none?
+  end
+
   def deactivate
     if kit
       kit.deactivate

app/models/kit.rb

@@ -46,6 +46,13 @@ def deactivate
     item.update!(active: false)
   end
 
+  # Kits can't reactivate if they have any inactive items, because now whenever they are allocated
+  # or deallocated, we are changing inventory for inactive items (which we don't allow).
+  # @return [Boolean]
+  def can_reactivate?
+    line_items.joins(:item).where(items: { active: false }).none?
+  end
+
   def reactivate
     update!(active: true)
     item.update!(active: true)

app/models/storage_location.rb

@@ -142,7 +142,6 @@ def increase_inventory(itemizable_array)
     # This is, at least for now, how we log changes to the inventory made in this call
     log = {}
     # Iterate through each of the line-items in the moving box
-    Item.reactivate(itemizable_array.map { |item_hash| item_hash[:item_id] })
     itemizable_array.each do |item_hash|
       # Locate the storage box for the item, or create a new storage box for it
       inventory_item = inventory_items.find_or_create_by!(item_id: item_hash[:item_id])

app/queries/items_by_storage_collection_and_quantity_query.rb

@@ -5,7 +5,6 @@ class ItemsByStorageCollectionAndQuantityQuery
   def self.call(organization:, filter_params:, inventory: nil)
     if inventory
       items = organization.items.order(name: :asc).class_filter(filter_params)
-      items = items.active if filter_params[:include_inactive_items]
       return items.to_h do |item|
         locations = inventory.storage_locations_for_item(item.id).map do |sl|
           {
@@ -31,9 +30,6 @@ def self.call(organization:, filter_params:, inventory: nil)
     end
 
     items_by_storage_collection = ItemsByStorageCollectionQuery.new(organization: organization, filter_params: filter_params).call
-    unless filter_params[:include_inactive_items]
-      items_by_storage_collection = items_by_storage_collection.active
-    end
     items_by_storage_collection_and_quantity = Hash.new
     items_by_storage_collection.each do |row|
       unless items_by_storage_collection_and_quantity.key?(row.id)

app/queries/items_by_storage_collection_query.rb

@@ -14,6 +14,7 @@ def initialize(organization:, filter_params:)
   def call
     @items ||=  organization
                 .items
+                .active
                 .joins(' LEFT OUTER JOIN "inventory_items" ON "inventory_items"."item_id" = "items"."id"')
                 .joins(' LEFT OUTER JOIN "storage_locations" ON "storage_locations"."id" = "inventory_items"."storage_location_id"')
                 .select('

app/services/itemizable_update_service.rb

@@ -7,7 +7,10 @@ module ItemizableUpdateService
   def self.call(itemizable:, type: :increase, params: {}, event_class: nil)
     StorageLocation.transaction do
       item_ids = params[:line_items_attributes]&.values&.map { |i| i[:item_id].to_i } || []
-      Item.reactivate(item_ids)
+      inactive_item_names = Item.where(id: item_ids, active: false).pluck(:name)
+      if inactive_item_names.any?
+        raise "Update failed: The following items are currently inactive: #{inactive_item_names.join(", ")}. Please reactivate them before continuing."
+      end
 
       from_location = to_location = itemizable.storage_location
       to_location = StorageLocation.find(params[:storage_location_id]) if params[:storage_location_id]

app/views/audits/_form.html.erb

@@ -14,7 +14,7 @@
               </div>
               <div class="box-body">
                 <%= simple_form_for @audit, html: {class: "storage-location-required"} do |f| %>
-                  <%= render partial: "storage_locations/source", object: f, locals: { label: "Storage location", error: "What storage location are you auditing?", include_inactive_items: true} %>
+                  <%= render partial: "storage_locations/source", object: f, locals: { label: "Storage location", error: "What storage location are you auditing?" } %>
                   <fieldset style="margin-bottom: 2rem;">
                     <legend>Items in this audit</legend>
                     <div id="audit_line_items" class="line-item-fields" data-capture-barcode="true">

app/views/distributions/_form.html.erb

@@ -24,7 +24,7 @@
       </div>
     </div>
 
-    <%= render partial: "storage_locations/source", object: f, locals: { include_inactive_items: false, include_omitted_items: true } %>
+    <%= render partial: "storage_locations/source", object: f, locals: { include_omitted_items: true } %>
 
     <%= f.input :comment, label: "Comment" %>
 

app/views/storage_locations/_source.html.erb

@@ -3,7 +3,6 @@
   label ||= "From storage location"
   error ||= "From what location are you moving inventory?"
   association_field ||= :storage_location
-  include_inactive_items ||= false
   include_omitted_items ||= false
 %>
 <%= source.association association_field,
@@ -16,7 +15,6 @@
     data: {
       storage_location_inventory_path: inventory_storage_location_path(
         organization_id: current_organization,
-        include_inactive_items: include_inactive_items,
         include_omitted_items: include_omitted_items,
         id: ":id",
         format: :json

db/migrate/20231222133931_activate_inactive_kit_items.rb

@@ -0,0 +1,16 @@
+class ActivateInactiveKitItems < ActiveRecord::Migration[7.0]
+  def up
+    Kit.all.each do |kit|
+      kit.line_items.each do |line_item|
+        unless line_item.item.active?
+          line_item.item.update!(active: true, visible_to_partners: false)
+        end
+      end
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+
+end

db/migrate/20231222140700_remove_invalid_inventory.rb

@@ -0,0 +1,23 @@
+class RemoveInvalidInventory < ActiveRecord::Migration[7.0]
+  def up
+    admin_role = Role.find_by(name: 'super_admin')
+    user = UsersRole.where(role_id: admin_role.id).first.user
+    StorageLocation.all.each do |loc|
+      adjustment = Adjustment.new(
+        organization_id: loc.organization_id,
+        storage_location_id: loc.id,
+        user_id: user.id,
+        comment: "Removing inactive items from inventory")
+      loc.inventory_items.joins(:item).where(items: { active: false }).where('quantity > 0').each do |ii|
+        adjustment.line_items.push(LineItem.new(item_id: ii.item_id, quantity: -ii.quantity))
+      end
+      if adjustment.line_items.any?
+        AdjustmentCreateService.new(adjustment).call
+      end
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/storage_location_spec.rb

@@ -87,18 +87,6 @@
           end.to change { subject.inventory_items.count }.by(1)
         end
       end
-
-      context "when increasing with an inactive item" do
-        let(:inactive_item) { create(:item, active: false, organization: @organization) }
-        let(:donation_with_inactive_item) { create(:donation, :with_items, organization: @organization, item_quantity: 10, item: inactive_item) }
-
-        it "re-activates the item as part of the creation process" do
-          expect do
-            subject.increase_inventory(donation_with_inactive_item.line_item_values)
-          end.to change { subject.inventory_items.count }.by(1)
-                                                         .and change { Item.count }.by(1)
-        end
-      end
     end
 
     describe "decrease_inventory" do

spec/requests/items_requests_spec.rb

@@ -32,5 +32,31 @@
         it { is_expected.to be_successful }
       end
     end
+
+    describe 'PUT #update' do
+      let(:item) { create(:item, organization: @organization, active: true) }
+      let(:storage_location) { create(:storage_location, organization: @organization) }
+      let(:kit) { create(:kit, organization: @organization) }
+      let(:inactive_params) { default_params.merge({id: item.id, item: { active: false } }) }
+
+      it 'should be able to deactivate an item' do
+        expect { put item_path(inactive_params) }.to change { item.reload.active }.from(true).to(false)
+        expect(response).to redirect_to(items_path)
+      end
+
+      it 'should not be able to deactivate an item in a storage location' do
+        create(:inventory_item, storage_location: storage_location, item_id: item.id)
+        put item_path(inactive_params)
+        expect(flash[:error]).to eq("Can't deactivate this item - it is currently assigned to either an active kit or a storage location!")
+        expect(item.reload.active).to eq(true)
+      end
+
+      it 'should not be able to deactivate an item in a kit' do
+        create(:line_item, itemizable: kit, item_id: item.id)
+        put item_path(inactive_params)
+        expect(flash[:error]).to eq("Can't deactivate this item - it is currently assigned to either an active kit or a storage location!")
+        expect(item.reload.active).to eq(true)
+      end
+    end
   end
 end

spec/requests/kit_requests_spec.rb

@@ -71,13 +71,26 @@
       expect(flash[:notice]).to eq("Kit has been deactivated!")
     end
 
-    specify "PUT #reactivate" do
-      kit.deactivate
-      expect(kit).not_to be_active
-      put reactivate_kit_url(kit, default_params)
-      expect(kit.reload).to be_active
-      expect(response).to redirect_to(dashboard_path)
-      expect(flash[:notice]).to eq("Kit has been reactivated!")
+    describe "PUT #reactivate" do
+      it "cannot reactivate if it has an inactive item" do
+        kit.deactivate
+        expect(kit).not_to be_active
+        kit.line_items.first.item.update!(active: false)
+
+        put reactivate_kit_url(kit, default_params)
+        expect(kit.reload).not_to be_active
+        expect(response).to redirect_to(dashboard_path)
+        expect(flash[:alert]).to eq("Cannot reactivate kit - it has inactive items! Please reactivate the items first.")
+      end
+
+      it "should successfully reactivate" do
+        kit.deactivate
+        expect(kit).not_to be_active
+        put reactivate_kit_url(kit, default_params)
+        expect(kit.reload).to be_active
+        expect(response).to redirect_to(dashboard_path)
+        expect(flash[:notice]).to eq("Kit has been reactivated!")
+      end
     end
   end
 end

spec/services/itemizable_update_service_spec.rb

@@ -1,8 +1,8 @@
 RSpec.describe ItemizableUpdateService do
   let(:storage_location) { create(:storage_location, organization: @organization, item_count: 0) }
   let(:new_storage_location) { create(:storage_location, organization: @organization, item_count: 0) }
-  let(:item1) { create(:item, organization: @organization) }
-  let(:item2) { create(:item, organization: @organization) }
+  let(:item1) { create(:item, organization: @organization, name: "My Item 1") }
+  let(:item2) { create(:item, organization: @organization, name: "My Item 2") }
   before(:each) do
     TestInventory.create_inventory(storage_location.organization, {
       storage_location.id => {
@@ -69,6 +69,12 @@
       expect(storage_location.size).to eq(10)
       expect(new_storage_location.size).to eq(24)
     end
+
+    it "should raise an error if any item is inactive" do
+      item1.update!(active: false)
+      msg = "Update failed: The following items are currently inactive: My Item 1. Please reactivate them before continuing."
+      expect { subject }.to raise_error(msg)
+    end
   end
 
   describe "decreases" do
@@ -114,5 +120,11 @@
       expect(storage_location.size).to eq(30)
       expect(new_storage_location.size).to eq(16)
     end
+
+    it "should raise an error if any item is inactive" do
+      item1.update!(active: false)
+      msg = "Update failed: The following items are currently inactive: My Item 1. Please reactivate them before continuing."
+      expect { subject }.to raise_error(msg)
+    end
   end
 end

spec/system/audit_system_spec.rb

@@ -39,20 +39,6 @@
       subject { url_prefix + "/audits/new" }
       let(:item) { Item.alphabetized.first }
 
-      it "*Does* include inactive items in the line item fields" do
-        visit subject
-
-        select storage_location.name, from: "Storage location"
-        expect(page).to have_content(item.name)
-        select item.name, from: "audit_line_items_attributes_0_item_id"
-
-        item.update(active: false)
-
-        page.refresh
-        select storage_location.name, from: "Storage location"
-        expect(page).to have_content(item.name)
-      end
-
       it "does not display quantities in line-item drop down selector" do
         create(:storage_location, :with_items, item: item, item_quantity: 10)
         visit subject

spec/system/distribution_system_spec.rb

@@ -268,15 +268,15 @@
     end
 
     context "when one of the items has been 'deleted'" do
-      it "the user can still reclaim it and it reactivates the item", js: true do
+      it "the user can still reclaim it", js: true do
         item = distribution.line_items.first.item
         item.destroy
         expect do
           accept_confirm do
             click_on "Reclaim"
           end
           page.find ".alert"
-        end.to change { Distribution.count }.by(-1).and change { Item.active.count }.by(1)
+        end.to change { Distribution.count }.by(-1)
         expect(page).to have_content "reclaimed"
       end
     end

spec/system/item_system_spec.rb

@@ -77,19 +77,6 @@
     end
   end
 
-  it "can include inactive items in the results" do
-    Item.delete_all
-    create(:item, :inactive, name: "Inactive Item")
-    create(:item, :active, name: "Active Item")
-    visit url_prefix + "/items"
-    expect(page).to have_text("Active Item")
-    expect(page).to have_no_text("Inactive Item")
-    page.check('include_inactive_items')
-    click_button "Filter"
-    expect(page).to have_text("Inactive Item")
-    expect(page).to have_text("Active Item")
-  end
-
   describe "destroying items" do
     subject { create(:item, name: "AAA DELETEME", organization: @user.organization) }
     context "when an item has history" do