Fix flaky ItemizableUpdateService. Logic was order dependent and have removed the global Timecop freeze to be able to order Event entries.

Author: Sergio-Mira

spec/services/itemizable_update_service_spec.rb

@@ -5,6 +5,7 @@
   let(:item1) { create(:item, organization: organization, name: "My Item 1") }
   let(:item2) { create(:item, organization: organization, name: "My Item 2") }
   let(:item3) { create(:item, organization: organization, name: "My Item 3") }
+  let(:two_days_ago) { 2.days.ago } # Store exact timestamp to be able to assert later
   before(:each) do
     TestInventory.create_inventory(storage_location.organization, {
       storage_location.id => {
@@ -18,12 +19,6 @@
     })
   end
 
-  around(:each) do |ex|
-    freeze_time do
-      ex.run
-    end
-  end
-
   describe "increases" do
     let(:itemizable) do
       line_items = [
@@ -39,12 +34,12 @@
 
     let(:attributes) do
       {
-        issued_at: 2.days.ago,
+        issued_at: two_days_ago,
         line_items_attributes: {"0": {item_id: item1.id, quantity: 2}, "1": {item_id: item2.id, quantity: 2}}
       }
     end
 
-    subject do
+    def call_update_service
       described_class.call(itemizable: itemizable,
         params: attributes,
         event_class: DonationEvent)
@@ -53,19 +48,19 @@
     it "should update quantity in same storage location" do
       expect(storage_location.size).to eq(20)
       expect(new_storage_location.size).to eq(20)
-      subject
+      call_update_service
       expect(itemizable.reload.line_items.count).to eq(2)
       expect(itemizable.line_items.sum(&:quantity)).to eq(4)
       expect(storage_location.size).to eq(14)
       expect(new_storage_location.size).to eq(20)
-      expect(itemizable.issued_at).to eq(2.days.ago)
+      expect(itemizable.issued_at.to_fs(:db)).to eq(two_days_ago.to_fs(:db)) # Use to_fs to ignore nanosecond differences
       expect(UpdateExistingEvent.count).to eq(1)
     end
 
     it "fails with a validation message for donation events with invalid issued_at" do
       attributes[:issued_at] = ""
 
-      expect { subject }.to raise_error do |e|
+      expect { call_update_service }.to raise_error do |e|
         expect(e).to be_a(ActiveRecord::RecordInvalid)
         expect(e.message).to eq("Validation failed: Issue date can't be blank")
       end
@@ -75,7 +70,7 @@
       context "when there is no intervening audit" do
         it "should update quantity in different locations" do
           attributes[:storage_location_id] = new_storage_location.id
-          subject
+          call_update_service
           expect(itemizable.reload.line_items.count).to eq(2)
           expect(itemizable.line_items.sum(&:quantity)).to eq(4)
           expect(storage_location.size).to eq(10)
@@ -89,15 +84,15 @@
                 "If you need to change the storage location, please delete this donation and create a new donation with the new storage location."
           create(:audit, :with_items, item: itemizable.items.first, organization: organization, storage_location: storage_location, status: "finalized")
           attributes[:storage_location_id] = new_storage_location.id
-          expect { subject }.to raise_error(msg)
+          expect { call_update_service }.to raise_error(msg)
         end
       end
     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)
+      expect { call_update_service }.to raise_error(msg)
     end
   end
 
@@ -116,30 +111,30 @@
 
     let(:attributes) do
       {
-        issued_at: 2.days.ago,
+        issued_at: two_days_ago,
         line_items_attributes: {"0": {item_id: item1.id, quantity: 2}, "1": {item_id: item2.id, quantity: 2}}
       }
     end
 
-    subject do
+    def call_update_service
       described_class.call(itemizable: itemizable, params: attributes, event_class: DistributionEvent)
     end
 
     it "should update quantity in same storage location" do
       expect(storage_location.size).to eq(20)
       expect(new_storage_location.size).to eq(20)
-      subject
+      call_update_service
       expect(itemizable.reload.line_items.count).to eq(2)
       expect(itemizable.line_items.sum(&:quantity)).to eq(4)
       expect(storage_location.size).to eq(26)
       expect(new_storage_location.size).to eq(20)
-      expect(itemizable.issued_at).to eq(2.days.ago)
+      expect(itemizable.issued_at.to_fs(:db)).to eq(two_days_ago.to_fs(:db)) # Use to_fs to ignore nanosecond differences
     end
 
     it "fails with a validation message for distribution events with invalid issued_at" do
       attributes[:issued_at] = ""
 
-      expect { subject }.to raise_error do |e|
+      expect { call_update_service }.to raise_error do |e|
         expect(e).to be_a(ActiveRecord::RecordInvalid)
         expect(e.message).to eq("Validation failed: Distribution date and time can't be blank")
       end
@@ -149,7 +144,7 @@
       context "when there is no intervening audit" do
         it "should update quantity in different locations" do
           attributes[:storage_location_id] = new_storage_location.id
-          subject
+          call_update_service
           expect(itemizable.reload.line_items.count).to eq(2)
           expect(itemizable.line_items.sum(&:quantity)).to eq(4)
           expect(storage_location.size).to eq(30)
@@ -163,15 +158,15 @@
                 "If you need to change the storage location, please reclaim this distribution and create a new distribution from the new storage location."
           create(:audit, :with_items, item: itemizable.items.first, organization: organization, storage_location: storage_location, status: "finalized")
           attributes[:storage_location_id] = new_storage_location.id
-          expect { subject }.to raise_error(msg)
+          expect { call_update_service }.to raise_error(msg)
         end
       end
     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)
+      expect { call_update_service }.to raise_error(msg)
     end
   end
 
@@ -188,12 +183,14 @@
           line_items: line_items,
           issued_at: 1.day.ago)
       end
+
       let(:attributes) do
         {
-          issued_at: 2.days.ago,
+          issued_at: two_days_ago,
           line_items_attributes: {"0": {item_id: item1.id, quantity: 5}, "1": {item_id: item3.id, quantity: 50}}
         }
       end
+
       it "should send an itemizable event if it already exists" do
         DonationEvent.publish(itemizable)
         expect(DonationEvent.count).to eq(1)
@@ -216,14 +213,8 @@
         expect(View::Inventory.total_inventory(organization.id)).to eq(75) # 40 - 5 (item1) - 10 (item2) + 50 (item3)
       end
     end
+
     describe "with distributions" do
-      before(:each) do
-        TestInventory.create_inventory(storage_location.organization, {
-          storage_location.id => {
-            item3.id => 10
-          }
-        })
-      end
       let(:itemizable) do
         line_items = [
           create(:line_item, item_id: item1.id, quantity: 5),
@@ -235,12 +226,22 @@
           line_items: line_items,
           issued_at: 1.day.ago)
       end
+
       let(:attributes) do
         {
-          issued_at: 2.days.ago,
+          issued_at: two_days_ago,
           line_items_attributes: {"0": {item_id: item1.id, quantity: 2}, "1": {item_id: item3.id, quantity: 6}}
         }
       end
+
+      before(:each) do
+        TestInventory.create_inventory(storage_location.organization, {
+          storage_location.id => {
+            item3.id => 10
+          }
+        })
+      end
+
       it "should send an itemizable event if it already exists" do
         DistributionEvent.publish(itemizable)
         expect(DistributionEvent.count).to eq(1)
@@ -264,10 +265,9 @@
       end
 
       it "should raise an error if there is an intervening snapshot" do
-        itemizable.save!
-        travel(-1.week)
-        SnapshotEvent.publish(organization)
-        travel 1.week
+        travel(-1.week) do
+          SnapshotEvent.publish(organization)
+        end
         itemizable.update!(created_at: 2.weeks.ago)
         expect do
           described_class.call(itemizable: itemizable, params: attributes, event_class: DistributionEvent)
@@ -276,16 +276,15 @@
 
       it "should not raise an error if no inventory was changed" do
         no_change_attrs = {
-          issued_at: 2.days.ago,
+          issued_at: two_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
+        travel(-1.week) do
+          SnapshotEvent.publish(organization)
+        end
         itemizable.update!(created_at: 2.weeks.ago)
         expect do
           described_class.call(itemizable: itemizable, params: no_change_attrs, event_class: DistributionEvent)