5262. Disallow editing of inventory past a snapshot

Author: dorner

app/events/snapshot_event.rb

@@ -1,6 +1,12 @@
 class SnapshotEvent < Event
   serialize :data, coder: EventTypes::StructCoder.new(EventTypes::Inventory)
 
+  # @param record [#organization_id, #created_at]
+  # @return [Boolean] true if there is an intervening snapshot event
+  def self.intervening?(record)
+    where(organization_id: record.organization_id, event_time: record.created_at..).any?
+  end
+
   # @param organization [Organization]
   def self.publish(organization)
     inventory = InventoryAggregate.inventory_for(organization.id)

app/models/event.rb

@@ -39,6 +39,17 @@ 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)
+
+    if SnapshotEvent.intervening?(eventable)
+      errors.add(:base,
+        "Cannot change inventory for an old action that has an intervening snapshot.")
+    end
+  end
 
   # @return [Array<Option>]
   def self.types_for_select

app/services/itemizable_update_service.rb

@@ -1,63 +1,77 @@
 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."
-      end
+  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)
+      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
 
-      from_location = to_location = itemizable.storage_location
-      to_location = StorageLocation.find(params[:storage_location_id]) if params[:storage_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)
+        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)
+          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

spec/models/event_spec.rb

@@ -79,4 +79,27 @@
       end
     end
   end
+
+  describe "#no_intervening_snapshot" do
+    let(:eventable) {
+      FactoryBot.create(:distribution,
+        organization_id: organization.id,
+        created_at: 1.week.ago)
+    }
+    let(:data) { EventTypes::Inventory.new(storage_locations: {}, organization_id: organization.id) }
+    let(:payload) { EventTypes::InventoryPayload.new(items: []) }
+
+    it "prevents creation if an intervening snapshot exists" do
+      travel(-1.day) do
+        SnapshotEvent.create!(organization_id: organization.id,
+          eventable: organization,
+          data: data,
+          event_time: Time.zone.now)
+      end
+
+      expect {
+        DistributionEvent.create!(eventable: eventable, organization_id: organization.id, data: payload)
+      }.to raise_error(ActiveRecord::RecordInvalid, /Cannot change inventory for an old action that has an intervening snapshot/)
+    end
+  end
 end

spec/services/itemizable_update_service_spec.rb

@@ -262,6 +262,35 @@
         expect(UpdateExistingEvent.count).to eq(1)
         expect(View::Inventory.total_inventory(organization.id)).to eq(52) # 50 + 3 (item1) + 5 (item2) +- 6 (item3)
       end
+
+      it "should raise an error if there is an intervening snapshot" do
+        itemizable.save!
+        travel(-1.week)
+        SnapshotEvent.publish(organization)
+        travel 1.week
+        itemizable.update!(created_at: 2.weeks.ago)
+        expect do
+          described_class.call(itemizable: itemizable, params: attributes, event_class: DistributionEvent)
+        end.to raise_error("Cannot update distribution because there has been an intervening snapshot of the inventory.")
+      end
+
+      it "should not raise an error if no inventory was changed" do
+        no_change_attrs = {
+          issued_at: 2.days.ago,
+          line_items_attributes: {
+            "0": {item_id: item1.id, quantity: 10},
+            "1": {item_id: item2.id, quantity: 10}
+          }
+        }
+        itemizable.save!
+        travel(-1.week)
+        SnapshotEvent.publish(organization)
+        travel 1.week
+        itemizable.update!(created_at: 2.weeks.ago)
+        expect do
+          described_class.call(itemizable: itemizable, params: no_change_attrs, event_class: DistributionEvent)
+        end.to raise_error("Cannot update distribution because there has been an intervening snapshot of the inventory.")
+      end
     end
   end
 end