Merge pull request #4179 from rubyforgood/invalid-fixes

Disable inactivating item with inventory and don't show inactive item…

Author: awwaiid

app/controllers/items_controller.rb

@@ -2,7 +2,12 @@
 # they like with their own Items.
 class ItemsController < ApplicationController
   def index
-    @items = current_organization.items.includes(:base_item, :kit).alphabetized.class_filter(filter_params)
+    @items = current_organization
+      .items
+      .includes(:base_item, :kit, :line_items)
+      .alphabetized
+      .class_filter(filter_params)
+      .group('items.id')
     @items = @items.active unless params[:include_inactive_items]
 
     @item_categories = current_organization.item_categories.includes(:items).order('name ASC')
@@ -94,10 +99,27 @@ def update
     end
   end
 
+  def deactivate
+    item = current_organization.items.find(params[:id])
+    begin
+      item.deactivate!
+    rescue => e
+      flash[:error] = e.message
+      redirect_back(fallback_location: items_path)
+      return
+    end
+
+    flash[:notice] = "#{item.name} has been deactivated."
+    redirect_to items_path
+  end
+
   def destroy
     item = current_organization.items.find(params[:id])
-    ActiveRecord::Base.transaction do
-      item.destroy
+    item.destroy
+    if item.errors.any?
+      flash[:error] = item.errors.full_messages.join("\n")
+      redirect_back(fallback_location: items_path)
+      return
     end
 
     flash[:notice] = "#{item.name} has been removed."

app/models/item.rb

@@ -94,6 +94,8 @@ class Item < ApplicationRecord
       .or(where("base_items.category = 'Miscellaneous'"))
   }
 
+  before_destroy :validate_destroy, prepend: true
+
   def self.barcoded_items
     joins(:barcode_items).order(:name).group(:id)
   end
@@ -111,20 +113,49 @@ def self.reactivate(item_ids)
     Item.where(id: item_ids).find_each { |item| item.update(active: true) }
   end
 
+  def has_inventory?(inventory = nil)
+    if inventory
+      inventory.quantity_for(item_id: id).positive?
+    else
+      inventory_items.where("quantity > 0").any?
+    end
+  end
+
+  def is_in_kit?
+    organization.kits
+      .active
+      .joins(:line_items)
+      .where(line_items: { item_id: id}).any?
+  end
+
+  def can_delete?(inventory = nil)
+    can_deactivate_or_delete?(inventory) && line_items.none? && !barcode_count&.positive?
+  end
+
   # @return [Boolean]
-  def can_deactivate?
+  def can_deactivate_or_delete?(inventory = nil)
+    if inventory.nil? && Event.read_events?(organization)
+      inventory = View::Inventory.new(organization_id)
+    end
     # 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?
+    # 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.
+
+    !has_inventory?(inventory) && !is_in_kit?
+  end
+
+  def validate_destroy
+    unless can_delete?
+      errors.add(:base, "Cannot delete item - it has already been used!")
+      throw(:abort)
+    end
   end
 
-  def deactivate
+  def deactivate!
+    unless can_deactivate_or_delete?
+      raise "Cannot deactivate item - it is in a storage location or kit!"
+    end
     if kit
       kit.deactivate
     else
@@ -136,27 +167,6 @@ def other?
     partner_key == "other"
   end
 
-  # Override `destroy` to ensure Item isn't accidentally destroyed
-  # without first being disassociated with its historical presence
-  def destroy
-    if has_history?
-      deactivate
-    else
-      super
-    end
-  end
-
-  def has_history?
-    return true if line_items.any? || barcode_items.any?
-
-    if Event.read_events?(organization)
-      inventory = View::Inventory.new(organization_id)
-      inventory.quantity_for(item_id: id).positive?
-    else
-      inventory_items.any?
-    end
-  end
-
   def self.gather_items(current_organization, global = false)
     if global
       where(id: current_organization.barcode_items.all.pluck(:barcodeable_id))

app/models/view/inventory.rb

@@ -23,7 +23,7 @@ def initialize(organization_id, event_time: nil)
     # @param event_time [DateTime]
     def reload(event_time = nil)
       @inventory = InventoryAggregate.inventory_for(organization_id, event_time: event_time)
-      @items = Item.where(organization_id: organization_id)
+      @items = Item.where(organization_id: organization_id).active
       @db_storage_locations = StorageLocation.where(organization_id: organization_id).active_locations
       load_item_details
     end
@@ -38,9 +38,12 @@ def storage_location_name(id)
     # @param include_omitted [Boolean]
     # @return [Array<EventTypes::EventItem>]
     def items_for_location(storage_location_id, include_omitted: false)
-      items = @inventory.storage_locations[storage_location_id]&.items&.values || []
+      items = @inventory.storage_locations[storage_location_id]
+        &.items
+        &.values
+        &.select { |i| i.quantity.positive? } || []
       if include_omitted
-        db_items = Item.where(organization_id: @inventory.organization_id).where.not(id: items.map(&:item_id))
+        db_items = Item.active.where(organization_id: @inventory.organization_id).where.not(id: items.map(&:item_id))
         zero_items = db_items.map do |item|
           ViewInventoryItem.new(
             item_id: item.id,
@@ -113,14 +116,17 @@ def all_items
 
     def load_item_details
       @inventory.storage_locations.values.each do |loc|
-        loc.items.values.each do |item|
+        loc.items.delete_if do |_, item|
           db_item = @items.find { |i| i.id == item.item_id }
+          next true if db_item.nil?
+
           loc.items[item.item_id] = ViewInventoryItem.new(
             item_id: item.item_id,
             storage_location_id: loc.id,
             quantity: item.quantity,
             db_item: db_item
           )
+          false
         end
       end
     end

app/queries/items_by_storage_collection_and_quantity_query.rb

@@ -4,7 +4,7 @@
 class ItemsByStorageCollectionAndQuantityQuery
   def self.call(organization:, filter_params:, inventory: nil)
     if inventory
-      items = organization.items.order(name: :asc).class_filter(filter_params)
+      items = organization.items.active.order(name: :asc).class_filter(filter_params)
       return items.to_h do |item|
         locations = inventory.storage_locations_for_item(item.id).map do |sl|
           {

app/views/items/_form.html.erb

@@ -43,10 +43,6 @@
                 <%= f.input_field :package_size, class: "form-control" %>
               <% end %>
 
-              <%= f.input :active, label: "Item is Active?", wrapper: :input_group do %>
-                <%= f.check_box :active, {class: "input-group-text", id: "active"}, "true", "false" %>
-              <% end %>
-
               <%= f.input :visible, label: "Item is Visible to Partners?", wrapper: :input_group do %>
                 <%= f.check_box :visible_to_partners, {class: "input-group-text", id: "visible_to_partners"}, "true", "false" %>
               <% end %>

app/views/items/_item_list.html.erb

@@ -14,7 +14,7 @@
     </tr>
     </thead>
     <tbody>
-      <%= render partial: "items/item_row", collection: items, as: :item_row %>
+      <%= render partial: "items/item_row", collection: items, as: :item_row, locals: { inventory: inventory } %>
     </tbody>
   </table>
 </div><!-- /.box-body.table-responsive -->

app/views/items/_item_row.html.erb

@@ -6,9 +6,19 @@
   <td class="text-right">
     <%= view_button_to item_path(item_row) %>
     <%= edit_button_to edit_item_path(item_row) %>
-    <% if item_row.kit.blank? %>
-      <%= delete_button_to item_path(item_row), { confirm: confirm_delete_msg(item_row.name) } if item_row.active %>
+    <% if item_row.active? %>
+      <% if item_row.can_delete?(inventory) %>
+        <%= delete_button_to item_path(item_row),
+                             text: 'Delete',
+                             confirm: confirm_delete_msg(item_row.name) %>
+      <% else %>
+        <% can_deactivate =  item_row.can_deactivate_or_delete?(inventory) %>
+        <%= delete_button_to deactivate_item_path(item_row),
+                             text: 'Deactivate',
+                             enabled: can_deactivate,
+                             confirm: confirm_deactivate_msg(item_row.name) %>
      <% end %>
+    <% end %>
     <%= restore_button_to restore_item_path(item_row), { confirm: confirm_restore_msg(item_row.name) } unless item_row.active %>
   </td>
 </tr>

app/views/items/index.html.erb

@@ -93,7 +93,7 @@
           </div>
           <div class="card-body">
             <div class="tab-content" id="custom-tabs-three-tabContent">
-              <%= render partial: 'item_list', locals: { items: @items } %>
+              <%= render partial: 'item_list', locals: { items: @items, inventory: @inventory } %>
               <%= render partial: 'item_categories', locals: { item_categories: @item_categories } %>
               <%= render partial: 'items_quantity_and_location' %>
               <%= render partial: 'items_inventory' %>

app/views/storage_locations/show.html.erb

@@ -96,15 +96,15 @@
                   </thead>
                   <tbody>
                   <% if @inventory %>
-                    <% @inventory.items_for_location(@storage_location.id).each do |item| %>
+                    <% @inventory.items_for_location(@storage_location.id, include_omitted: true).each do |item| %>
                       <tr>
                         <td><%= link_to item.name, item_path(item.item_id) %></td>
                         <td><%= number_with_delimiter(item.quantity) %></td>
                       </tr>
                     <% end %>
                   <% else %>
                     <%= render partial: "inventory_item_row",
-                               collection: @storage_location.inventory_items,
+                               collection: @storage_location.inventory_items.joins(:item).where(items: { active: true }),
                                locals: { version_date: params[:version_date] } %>
                   <% end %>
                   </tbody>

config/routes.rb

@@ -166,6 +166,7 @@ def set_up_flipper
 
     resources :profiles, only: %i(edit update)
     resources :items do
+      delete :deactivate, on: :member
       patch :restore, on: :member
       patch :remove_category, on: :member
     end