Merge pull request #5273 from rubyforgood/5262-editing-old-inventory

5262. Disallow editing of inventory past a snapshot

Author: awwaiid

app/controllers/distributions_controller.rb

@@ -27,12 +27,13 @@ def print
   end
 
   def destroy
-    result = DistributionDestroyService.new(params[:id]).call
+    service = DistributionDestroyService.new(params[:id])
+    result = service.call
 
     if result.success?
       flash[:notice] = "Distribution #{params[:id]} has been reclaimed!"
     else
-      flash[:error] = "Could not destroy distribution #{params[:id]}. Please contact technical support."
+      flash[:error] = result.error.message
     end
 
     redirect_to distributions_path
@@ -185,9 +186,10 @@ def edit
       @request = @distribution.request
       @items = current_organization.items.active.alphabetized
       @partner_list = current_organization.partners.alphabetized
+      @changes_disallowed = SnapshotEvent.intervening(@distribution).present?
       @audit_warning = current_organization.audits
         .where(storage_location_id: @distribution.storage_location_id)
-        .where("updated_at > ?", @distribution.created_at).any?
+        .where("updated_at > ?", @distribution.created_at).any? && !@changes_disallowed
       inventory = View::Inventory.new(@distribution.organization_id)
       @storage_locations = current_organization.storage_locations.active.alphabetized.select do |storage_loc|
         !inventory.quantity_for(storage_location: storage_loc.id).negative?

app/controllers/donations_controller.rb

@@ -55,7 +55,9 @@ def new
   def edit
     @donation = Donation.find(params[:id])
     @donation.line_items.build
-    @audit_performed_and_finalized = Audit.finalized_since?(@donation, @donation.storage_location_id)
+    @changes_disallowed = SnapshotEvent.intervening(@donation).present?
+    @audit_performed_and_finalized = Audit.finalized_since?(@donation, @donation.storage_location_id) &&
+      !@changes_disallowed
 
     load_form_collections
   end
@@ -91,7 +93,7 @@ def destroy
     if service.success?
       flash[:notice] = "Donation #{params[:id]} has been removed!"
     else
-      flash[:error] = "Donation #{params[:id]} failed to be removed because #{service.error}"
+      flash[:error] = service.error.message
     end
 
     redirect_to donations_path

app/controllers/purchases_controller.rb

@@ -63,7 +63,9 @@ def new
   def edit
     @purchase = current_organization.purchases.find(params[:id])
     @purchase.line_items.build
-    @audit_performed_and_finalized = Audit.finalized_since?(@purchase, @purchase.storage_location_id)
+    @changes_disallowed = SnapshotEvent.intervening(@purchase).present?
+    @audit_performed_and_finalized = Audit.finalized_since?(@purchase, @purchase.storage_location_id) &&
+      !@changes_disallowed
 
     load_form_collections(@purchase)
   end

app/events/snapshot_event.rb

@@ -1,6 +1,14 @@
 class SnapshotEvent < Event
   serialize :data, coder: EventTypes::StructCoder.new(EventTypes::Inventory)
 
+  # @param record [#organization_id, #created_at]
+  # @return [SnapshotEvent, nil] the intervening snapshot event or null if there aren't any
+  def self.intervening(record)
+    where(organization_id: record.organization_id, event_time: record.created_at..)
+      .order("created_at desc")
+      .first
+  end
+
   # @param organization [Organization]
   def self.publish(organization)
     inventory = InventoryAggregate.inventory_for(organization.id)

app/models/distribution.rb

@@ -41,6 +41,7 @@ class Distribution < ApplicationRecord
   validates :delivery_method, presence: true
   validate :line_items_quantity_is_positive
   validates :shipping_cost, numericality: { greater_than_or_equal_to: 0 }, allow_blank: true, if: :shipped?
+  before_destroy :check_no_intervening_snapshot
 
   before_save :combine_distribution, :reset_shipping_cost
 
@@ -173,6 +174,13 @@ def line_items_quantity_is_positive
     line_items_quantity_is_at_least(1)
   end
 
+  def check_no_intervening_snapshot
+    intervening = SnapshotEvent.intervening(self)
+    if intervening
+      raise "We can't delete distributions entered before #{intervening.event_time.to_date}."
+    end
+  end
+
   def reset_shipping_cost
     self.shipping_cost = nil unless delivery_method == "shipped"
   end

app/models/donation.rb

@@ -67,6 +67,7 @@ class Donation < ApplicationRecord
   validates :source, presence: true, inclusion: { in: SOURCES.values, message: "Must be a valid source." }
   validates :money_raised, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true
   validate :line_items_quantity_is_positive
+  before_destroy :check_no_intervening_snapshot
 
   # TODO: move this to Organization.donations as an extension
   scope :during, ->(range) { where(donations: { issued_at: range }) }
@@ -135,4 +136,11 @@ def combine_duplicates
   def line_items_quantity_is_positive
     line_items_quantity_is_at_least(1)
   end
+
+  def check_no_intervening_snapshot
+    intervening = SnapshotEvent.intervening(self)
+    if intervening
+      raise "We can't delete donations entered before #{intervening.event_time.to_date}."
+    end
+  end
 end

app/models/event.rb

@@ -40,6 +40,18 @@ class Event < ApplicationRecord
     self.user_id = PaperTrail.request&.whodunnit
   end
   after_create :validate_inventory
+  validate :no_intervening_snapshot, on: :create
+
+  def no_intervening_snapshot
+    return if is_a?(SnapshotEvent)
+    return unless eventable.respond_to?(:organization)
+
+    intervening = SnapshotEvent.intervening(eventable)
+    if intervening.present?
+      errors.add(:base,
+        "Cannot change inventory for an #{eventable.class.name.downcase} created before #{intervening.event_time.to_date}.")
+    end
+  end
 
   # @return [Array<Option>]
   def self.types_for_select

app/models/purchase.rb

@@ -59,6 +59,7 @@ class Purchase < ApplicationRecord
 
   validates :amount_spent_in_cents, numericality: { greater_than: 0 }
   validate :total_equal_to_all_categories
+  before_destroy :check_no_intervening_snapshot
 
   validates :amount_spent_on_diapers_cents, numericality: { greater_than_or_equal_to: 0 }
   validates :amount_spent_on_adult_incontinence_cents, numericality: { greater_than_or_equal_to: 0 }
@@ -132,4 +133,11 @@ def total_equal_to_all_categories
         "does not equal all categories - categories add to #{cat_total} but given total is #{total}")
     end
   end
+
+  def check_no_intervening_snapshot
+    intervening = SnapshotEvent.intervening(self)
+    if intervening
+      raise "We can't delete purchases entered before #{intervening.event_time.to_date}."
+    end
+  end
 end

app/services/distribution_service.rb

@@ -19,6 +19,15 @@ def perform_distribution_service(&block)
     return self
   end
 
+  def distribution
+    # Return distribution if it has already been defined
+    return @distribution if instance_variable_defined? :@distribution
+
+    # Otherwise try to get this value with possibly
+    # provided distribution_id from initialize
+    @distribution = @distribution_id.present? ? Distribution.find(@distribution_id) : nil
+  end
+
   def success?
     error.nil?
   end
@@ -35,15 +44,6 @@ def set_error(error)
     @error = error
   end
 
-  def distribution
-    # Return distribution if it has already been defined
-    return @distribution if instance_variable_defined? :@distribution
-
-    # Otherwise try to get this value with possibly
-    # provided distribution_id from initialize
-    @distribution = @distribution_id.present? ? Distribution.find(@distribution_id) : nil
-  end
-
   def distribution_id
     return @distribution_id if instance_variable_defined? :@distribution_id
 

app/services/itemizable_update_service.rb

@@ -1,63 +1,84 @@
 module ItemizableUpdateService
-  # @param itemizable [Itemizable]
-  # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`.
-  # @param event_class [Class<Event>] the event class to publish the itemizable to.
-  def self.call(itemizable:, params: {}, event_class: nil)
-    original_storage_location = itemizable.storage_location
-    StorageLocation.transaction do
-      item_ids = params[:line_items_attributes]&.values&.map { |i| i[:item_id].to_i } || []
-      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."
+  class << self
+    # @param itemizable [Itemizable]
+    # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`.
+    # @param event_class [Class<Event>] the event class to publish the itemizable to.
+    def call(itemizable:, params: {}, event_class: nil)
+      # we're updating just attributes on the itemizable, not line items
+      if params[:line_items_attributes].blank? && SnapshotEvent.intervening(itemizable).present?
+        itemizable.update!(params)
+        itemizable.reload
+        return
       end
 
-      from_location = to_location = itemizable.storage_location
-      to_location = StorageLocation.find(params[:storage_location_id]) if params[:storage_location_id]
+      original_storage_location = itemizable.storage_location
+      StorageLocation.transaction do
+        item_ids = params[:line_items_attributes]&.values&.map { |i| i[:item_id].to_i } || []
+        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
 
-      verify_intervening_audit_on_storage_location_items(itemizable: itemizable, from_location_id: from_location.id, to_location_id: to_location.id)
+        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)
 
-      previous = nil
-      # TODO once event sourcing has been out for long enough, we can safely remove this
-      if Event.where(eventable: itemizable).none? || UpdateExistingEvent.where(eventable: itemizable).any?
         previous = itemizable.line_items.map(&:dup)
-      end
+        if inventory_changes?(previous, params[:line_items_attributes]) && SnapshotEvent.intervening(itemizable).present?
+          raise "Cannot update #{itemizable.class.name.downcase} because there has been an intervening snapshot of the inventory."
+        end
 
-      line_item_attrs = Array.wrap(params[:line_items_attributes]&.values)
-      line_item_attrs.each { |attr| attr.delete(:id) }
+        line_item_attrs = Array.wrap(params[:line_items_attributes]&.values)
+        line_item_attrs.each { |attr| attr.delete(:id) }
 
-      update_storage_location(itemizable: itemizable, params: params)
-      if previous
-        UpdateExistingEvent.publish(itemizable, previous, original_storage_location)
-      else
-        event_class&.publish(itemizable)
+        update_storage_location(itemizable: itemizable, params: params)
+
+        # TODO once event sourcing has been out for long enough, we can safely remove this
+        if Event.where(eventable: itemizable).none? || UpdateExistingEvent.where(eventable: itemizable).any?
+          UpdateExistingEvent.publish(itemizable, previous, original_storage_location)
+        elsif inventory_changes?(previous, params[:line_items_attributes])
+          event_class&.publish(itemizable)
+        end
       end
     end
-  end
 
-  # @param itemizable [Itemizable]
-  # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`.
-  def self.update_storage_location(itemizable:, params:)
-    # Delete the line items -- they'll be replaced later
-    itemizable.line_items.delete_all
-    # Update the current model with the new parameters
-    itemizable.update!(params)
-    itemizable.reload
-  end
+    # @param previous [Array<LineItem>] Previous line items before the update.
+    # @param line_item_attributes [Hash] The new line item attributes from the params.
+    # @return [Boolean] Returns true if the inventory has changed, false otherwise.
+    def inventory_changes?(previous, line_item_attributes)
+      previous_attrs = previous.to_h { |li| [li.item_id, li.quantity] }
+      new_attrs = line_item_attributes.to_h do |_, li|
+        [li[:item_id].to_i, li[:quantity].to_i]
+      end
+      previous_attrs != new_attrs
+    end
+
+    # @param itemizable [Itemizable]
+    # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`.
+    def update_storage_location(itemizable:, params:)
+      # Delete the line items -- they'll be replaced later
+      itemizable.line_items.delete_all
+      # Update the current model with the new parameters
+      itemizable.update!(params)
+      itemizable.reload
+    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 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
-    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."
+    # @param itemizable [Itemizable]
+    # @param from_location [StorageLocation]
+    # @param to_location [StorageLocation]
+    def verify_intervening_audit_on_storage_location_items(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
+      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
 end