Use event data #4000

dorner · 2023-12-27T21:16:28Z

Note: This PR is rebased off of #3994 which should be merged first.

This PR contains all the remaining code needed for us to run our app fully in event-sourced mode.

With this merged, the system will be in the following state:

All inventory writes are double writes (to both events and inventory items)
Reads are put behind a feature flag. By default it still reads from inventory items. With the flag on, it instead reads from events.
On write, with the flag on, it uses the event sourcing validations to determine if there are any invalid events (i.e. trying to remove more inventory than a location has). With the flag off, it uses the current approach of inspecting inventory items.

Broadly, the PR contains the following changes:

Adds the event-read feature flag, which can be set on an organization level to turn event-source reading on.
Adds a new class, View::Inventory, which acts as a repository for event-calculated inventory. Since we aren't inspecting database rows, but getting the full state of the inventory as calculated from events, we want to ensure we aren't recalculating this more than once per request.
In order to facilitate this, code has changed to calculate the inventory once (generally in a controller action) and pass it through the view and any other methods necessary.
This includes making changes to the Exportable CSV-related methods to take an inventory object - the relevant classes simply overwrite the Exportable methods to enable this.
In general, the feature flag is checked at the top level (e.g in controller) and an inventory is generated if necessary. Then lower-down code will check for the existence of the inventory object to determine whether to use it or the current methods of inspecting inventory items.

One subtle change is that there are several places in the current system that differentiate between an inventory item not existing at all, or existing with a quantity of zero. The new event-driven method doesn't differentiate between these (since semantically they are the same case).

The PR has been split up into multiple commits, where each commit accomplishes one set of changes. Hopefully this will make the review more palatable.

For specs, I've introduced a new create_inventory method that ensures that all relevant events and inventory items are double-written whenever inventory needs to be created. For specs that currently look at the state of inventory items, I've added additional checks to validate the event-sourced inventory.

Ultimately, once this has been merged, tested and fully rolled out, we will remove all reference to inventory items from the app.

Remove spec that was causing issues (diffing isn't really needed any more anyway)

# Conflicts: # app/controllers/distributions_controller.rb

# Conflicts: # spec/system/distribution_system_spec.rb # spec/system/request_system_spec.rb

dorner · 2024-01-22T00:57:00Z

@cielf as per discussion, pushed a commit that handles dealing with storage location change on update.

awwaiid · 2024-01-26T15:22:44Z

app/controllers/items_controller.rb

+      @inventory = View::Inventory.new(current_organization.id)
+    end
+    @items_by_storage_collection_and_quantity = ItemsByStorageCollectionAndQuantityQuery.call(organization: current_organization,
+      inventory: @inventory,


Note to self: I verified that ItemsByStorageCollectionAndQuantityQuery handles nil inventory

awwaiid

Looks great, but the tests don't pass. I created #4078 which pulls in the latest main to see if that would help, but ... not so much.

awwaiid · 2024-01-27T16:27:47Z

app/controllers/requests_controller.rb

+      inventory = View::Inventory.new(@request.organization_id)
+    end
+
+    @request.request_items.map { |json| RequestItem.from_json(json, @request, inventory) }


NTS: Verified RequestItem new param

awwaiid · 2024-01-27T16:29:03Z

app/controllers/partners/dashboards_controller.rb

@@ -24,6 +24,9 @@ def show

      @families = @partner.families
      @children = @partner.children
+      if Event.read_events?(@partner.organization)
+        @inventory = View::Inventory.new(@partner.organization_id)


NTS: The view does a RequestItem.from_json(json, request, @inventory) with this, which is OK

awwaiid · 2024-01-27T16:55:35Z

app/models/storage_location.rb

+        .sort_by(&:name)
+        .map { |i| OpenStruct.new(name: i.name, id: i.item_id) }
+    else
+      organization.items.joins(:storage_locations).select(:id, :name).group(:id, :name).order(name: :asc)


NTS: This one is items scoped to organization which might change the results

awwaiid · 2024-01-27T16:57:17Z

app/controllers/storage_locations_controller.rb

    @selected_item_category = filter_params[:containing]
-    @items = current_organization.storage_locations.items_inventoried
+    @items = StorageLocation.items_inventoried(current_organization, @inventory)


NTS: As long as these callers are already scoped to current organization storage locations this'll be fine (like here)

awwaiid · 2024-01-28T13:32:36Z

app/events/adjustment_event.rb

      organization_id: adjustment.organization_id,
-      event_time: adjustment.created_at,
+      event_time: Time.zone.now,


Seems reasonable

awwaiid · 2024-01-28T13:48:14Z

app/models/storage_location.rb

@@ -63,16 +63,18 @@ class StorageLocation < ApplicationRecord
  scope :for_csv_export, ->(organization, *) { where(organization: organization) }
  scope :active_locations, -> { where(discarded_at: nil) }

-  def self.item_total(item_id)


NTS: Dead code

awwaiid · 2024-01-28T13:50:58Z

app/models/view/inventory.rb

+  include Dry.Types()
+end
+
+module View


Calling this a view might be confusing vs app/views/*. No better name immediately leaps to mind.

awwaiid · 2024-01-28T14:11:51Z

app/services/deallocate_kit_inventory_service.rb

+    if error.nil?
+      ApplicationRecord.transaction do
+        deallocate_inventory_items
+        KitDeallocateEvent.publish(@kit, @storage_location, @decrease_by)


awwaiid · 2024-02-04T02:31:20Z

spec/models/kit_spec.rb

      end
    end

    context 'without inventory items' do
      it 'should return true' do
        kit = create(:kit, :with_item)
-        expect(kit.reload.can_deactivate?).to eq(true)
+        expect(kit.reload.can_deactivate?(nil)).to eq(true)


NTS: hmm. Seems like can_deactivate should work without an explicit param

awwaiid · 2024-02-04T02:31:57Z

spec/services/adjustment_create_service_spec.rb

    end

    it "gives an error if we attempt to adjust inventory below 0" do
-      quantity = -1 * (inventory_item_1.quantity + 1)
+      quantity = -101


I like the explicitness of these

Me too. The dynamism of our specs really rubs me the wrong way since it's so hard to tell what things are supposed to be. Half the time the specs just copy the code from the thing they're testing. -_-

# Conflicts: # app/models/storage_location.rb # app/services/adjustment_create_service.rb # app/services/donation_create_service.rb # app/services/purchase_create_service.rb # spec/factories/donations.rb # spec/factories/purchases.rb # spec/models/storage_location_spec.rb

awwaiid

I traced through, everything looks good!

cielf · 2024-02-09T18:46:55Z

I've done some light testing of this and I'm ok with merging it in. Sanity check -- @awwaiid -- did your technical review include the changes in #3994?

cielf · 2024-02-11T13:38:31Z

per @awwaiid, all the changes from #3994 are in #4000, so it can be closed and then we can merge this.

Conflicts: app/views/audits/show.html.erb app/views/items/show.html.erb app/views/partners/dashboards/_requests_in_progress.html.erb app/views/storage_locations/show.html.erb db/schema.rb spec/models/event_spec.rb

awwaiid · 2024-02-11T14:11:39Z

@dorner rspec (non-browser) failed and then I re-ran it without changes and it passed, so you should take a look -- https://github.com/rubyforgood/human-essentials/actions/runs/7862418075/job/21451870283?pr=4000

github-actions · 2024-02-18T15:44:36Z

@dorner: Your PR Use event data is part of today's Human Essentials production release: 2024.02.18.
Thank you very much for your contribution!

dorner force-pushed the use-event-data branch 2 times, most recently from 3b61a0b to ec47e19 Compare December 31, 2023 01:16

dorner added 3 commits January 1, 2024 09:34

Update logic for destroy events

6387014

Use group_id instead of eventable_type

eb810b0

dangit

66f9a8b

dorner force-pushed the use-event-data branch from ebe5430 to 96ac191 Compare January 1, 2024 14:35

dorner added 11 commits January 1, 2024 09:38

Add storage_location_id to event items

2f0195c

Add feature flag for reading events

1a57173

Add View::Inventory class and use it in specs

69b7085

Audit changes

4decf60

Items changes

0619200

Storage location changes

ad3058e

Distribution and request changes (with base itemizable changes)

7676665

Kits changes

e140b10

Donation changes

dc99faa

Transfer changes

6e4d9f4

Remaining changes

0c3c3b9

dorner force-pushed the use-event-data branch 4 times, most recently from fe3f540 to 16a3f6e Compare January 2, 2024 16:12

dorner marked this pull request as ready for review January 2, 2024 16:15

Validate inventory on event creation + fixes

7cf0391

Remove spec that was causing issues (diffing isn't really needed any more anyway)

dorner force-pushed the use-event-data branch from 16a3f6e to 7cf0391 Compare January 2, 2024 19:38

dorner added 6 commits January 7, 2024 20:59

Change events to all be NOW and use diffing logic

4bc26cd

Missed migration

2d3c674

Merge branch 'main' into use-event-data

9152c3e

spec fixes

0161baf

last fix?

0edbff1

Merge branch 'main' into use-event-data

e716b22

# Conflicts: # app/controllers/distributions_controller.rb

dorner added 4 commits January 19, 2024 15:51

Fix bug where previous item was not in the itemizable

229474c

Handle changing storage locations

5c1ccfa

Merge branch 'main' into use-event-data

96f15d1

# Conflicts: # spec/system/distribution_system_spec.rb # spec/system/request_system_spec.rb

Rubocop fixes

158710e

cielf requested a review from awwaiid January 26, 2024 14:36

awwaiid reviewed Jan 26, 2024

View reviewed changes

awwaiid requested changes Feb 4, 2024

View reviewed changes

dorner added 2 commits February 4, 2024 10:11

Fix specs

5da88dd

awwaiid approved these changes Feb 4, 2024

View reviewed changes

awwaiid mentioned this pull request Feb 11, 2024

Update logic for destroy events #3994

Closed

Merge branch 'main' into use-event-data

bbb65bd

Conflicts: app/views/audits/show.html.erb app/views/items/show.html.erb app/views/partners/dashboards/_requests_in_progress.html.erb app/views/storage_locations/show.html.erb db/schema.rb spec/models/event_spec.rb

Fix failing spec

76ad95d

cielf merged commit 07cb695 into main Feb 12, 2024
20 checks passed

cielf deleted the use-event-data branch February 12, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use event data #4000

Use event data #4000

dorner commented Dec 27, 2023 •

edited

dorner commented Jan 22, 2024

awwaiid Jan 26, 2024

awwaiid left a comment

awwaiid Jan 27, 2024

awwaiid Jan 27, 2024

awwaiid Jan 27, 2024

awwaiid Jan 27, 2024

awwaiid Jan 28, 2024

awwaiid Jan 28, 2024

awwaiid Jan 28, 2024

awwaiid Jan 28, 2024

awwaiid Feb 4, 2024

awwaiid Feb 4, 2024

dorner Feb 4, 2024

awwaiid left a comment

cielf commented Feb 9, 2024

cielf commented Feb 11, 2024

awwaiid commented Feb 11, 2024

github-actions bot commented Feb 18, 2024

Use event data #4000

Use event data #4000

Conversation

dorner commented Dec 27, 2023 • edited

dorner commented Jan 22, 2024

Choose a reason for hiding this comment

awwaiid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwaiid left a comment

Choose a reason for hiding this comment

cielf commented Feb 9, 2024

cielf commented Feb 11, 2024

awwaiid commented Feb 11, 2024

github-actions bot commented Feb 18, 2024

dorner commented Dec 27, 2023 •

edited