From 6e6adfabe1b986e3c14b5ed3d1378de08bdb7140 Mon Sep 17 00:00:00 2001
From: Cory Streiff <90390502+coalest@users.noreply.github.com>
Date: Fri, 10 Jan 2025 01:46:08 +0100
Subject: [PATCH] Remove OpenStructs (#4882)

---
 app/controllers/items_controller.rb           |  2 +-
 app/models/event.rb                           |  8 ++++----
 app/models/option.rb                          |  3 +++
 app/models/purchase.rb                        | 12 ++++++++++-
 app/models/result.rb                          |  9 +++++++++
 app/models/storage_location.rb                |  3 ++-
 app/queries/low_inventory_query.rb            |  6 ++++--
 app/services/item_create_service.rb           |  4 ++--
 app/services/transfer_destroy_service.rb      |  4 ++--
 .../dashboard/_low_inventory_report.html.erb  | 12 +++++------
 app/views/events/index.html.erb               |  2 +-
 spec/models/event_spec.rb                     | 19 ++++++++++++++++++
 spec/requests/transfers_requests_spec.rb      |  8 ++++----
 spec/services/item_create_service_spec.rb     | 14 ++++++-------
 spec/services/kit_create_service_spec.rb      |  4 ++--
 .../services/transfer_destroy_service_spec.rb | 20 +++++++++----------
 spec/system/dashboard_system_spec.rb          |  2 --
 17 files changed, 87 insertions(+), 45 deletions(-)
 create mode 100644 app/models/option.rb
 create mode 100644 app/models/result.rb

diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb
index e1d7e20dce..447e2647d0 100644
--- a/app/controllers/items_controller.rb
+++ b/app/controllers/items_controller.rb
@@ -39,7 +39,7 @@ def create
     result = create.call
 
     if result.success?
-      redirect_to items_path, notice: "#{result.item.name} added!"
+      redirect_to items_path, notice: "#{result.value.name} added!"
     else
       @base_items = BaseItem.without_kit.alphabetized
       # Define a @item to be used in the `new` action to be rendered with
diff --git a/app/models/event.rb b/app/models/event.rb
index a67f972666..477e4e3509 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -40,11 +40,11 @@ class Event < ApplicationRecord
   end
   after_create :validate_inventory
 
-  # @return [Array<OpenStruct>]
+  # @return [Array<Option>]
   def self.types_for_select
-    descendants.map { |klass|
-      OpenStruct.new(name: klass.name.sub("Event", "").titleize, value: klass.name)
-    }.sort_by(&:name)
+    descendants.map do |klass|
+      Option.new(name: klass.name.sub("Event", "").titleize, id: klass.name)
+    end.sort_by(&:name)
   end
 
   # Returns the most recent "usable" snapshot. A snapshot is unusable if there is another event
diff --git a/app/models/option.rb b/app/models/option.rb
new file mode 100644
index 0000000000..cb6d421cfd
--- /dev/null
+++ b/app/models/option.rb
@@ -0,0 +1,3 @@
+# PORO used to create select options in views
+class Option < Data.define(:id, :name)
+end
diff --git a/app/models/purchase.rb b/app/models/purchase.rb
index 734d36fff7..418fec5465 100644
--- a/app/models/purchase.rb
+++ b/app/models/purchase.rb
@@ -61,6 +61,16 @@ class Purchase < ApplicationRecord
   validates :amount_spent_in_cents, numericality: { greater_than: 0 }
   validate :total_equal_to_all_categories
 
+  SummaryByDates = Data.define(
+    :amount_spent,
+    :recent_purchases,
+    :period_supplies,
+    :diapers,
+    :adult_incontinence,
+    :other,
+    :total_items
+  )
+
   def storage_view
     storage_location.nil? ? "N/A" : storage_location.name
   end
@@ -84,7 +94,7 @@ def remove(item)
   def self.organization_summary_by_dates(organization, date_range)
     purchases = where(organization: organization).during(date_range)
 
-    OpenStruct.new(
+    SummaryByDates.new(
       amount_spent: purchases.sum(:amount_spent_in_cents),
       recent_purchases: purchases.recent.includes(:vendor),
       period_supplies: purchases.sum(:amount_spent_on_period_supplies_cents),
diff --git a/app/models/result.rb b/app/models/result.rb
new file mode 100644
index 0000000000..f0fc033a5f
--- /dev/null
+++ b/app/models/result.rb
@@ -0,0 +1,9 @@
+# Immutable result object for use in services
+class Result < Data.define(:value, :error)
+  # Set default attribute values
+  def initialize(value: nil, error: nil)
+    super
+  end
+
+  def success? = error.nil?
+end
diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb
index 4b2f655c5b..96ebc93158 100644
--- a/app/models/storage_location.rb
+++ b/app/models/storage_location.rb
@@ -64,13 +64,14 @@ class StorageLocation < ApplicationRecord
 
   # @param organization [Organization]
   # @param inventory [View::Inventory]
+  # @return [Array<Option>]
   def self.items_inventoried(organization, inventory = nil)
     inventory ||= View::Inventory.new(organization.id)
     inventory
       .all_items
       .uniq(&:item_id)
       .sort_by(&:name)
-      .map { |i| OpenStruct.new(name: i.name, id: i.item_id) }
+      .map { |i| Option.new(name: i.name, id: i.item_id) }
   end
 
   # @return [Array<Item>]
diff --git a/app/queries/low_inventory_query.rb b/app/queries/low_inventory_query.rb
index 5deee91d7a..f5ebe083cc 100644
--- a/app/queries/low_inventory_query.rb
+++ b/app/queries/low_inventory_query.rb
@@ -1,4 +1,6 @@
 class LowInventoryQuery
+  LowInventoryItem = Data.define(:id, :name, :on_hand_minimum_quantity, :on_hand_recommended_quantity, :total_quantity)
+
   def self.call(organization)
     inventory = View::Inventory.new(organization.id)
     items = inventory.all_items.uniq(&:item_id)
@@ -7,7 +9,7 @@ def self.call(organization)
     items.each do |item|
       quantity = inventory.quantity_for(item_id: item.id)
       if quantity < item.on_hand_minimum_quantity.to_i || quantity < item.on_hand_recommended_quantity.to_i
-        low_inventory_items.push(OpenStruct.new(
+        low_inventory_items.push(LowInventoryItem.new(
           id: item.id,
           name: item.name,
           on_hand_minimum_quantity: item.on_hand_minimum_quantity,
@@ -17,6 +19,6 @@ def self.call(organization)
       end
     end
 
-    low_inventory_items.sort_by { |item| item[:name] }
+    low_inventory_items.sort_by { |item| item.name }
   end
 end
diff --git a/app/services/item_create_service.rb b/app/services/item_create_service.rb
index 60126ad803..01159d33e4 100644
--- a/app/services/item_create_service.rb
+++ b/app/services/item_create_service.rb
@@ -12,9 +12,9 @@ def call
       new_item.sync_request_units!(@request_unit_ids)
     end
 
-    OpenStruct.new(success?: true, item: new_item)
+    Result.new(value: new_item)
   rescue StandardError => e
-    OpenStruct.new(success?: false, error: e)
+    Result.new(error: e)
   end
 
   private
diff --git a/app/services/transfer_destroy_service.rb b/app/services/transfer_destroy_service.rb
index cbeaaa4def..f47794cd10 100644
--- a/app/services/transfer_destroy_service.rb
+++ b/app/services/transfer_destroy_service.rb
@@ -13,9 +13,9 @@ def call
       transfer.destroy!
     end
 
-    OpenStruct.new(success?: true)
+    Result.new
   rescue StandardError => e
-    OpenStruct.new(success?: false, error: e)
+    Result.new(error: e)
   end
 
   private
diff --git a/app/views/dashboard/_low_inventory_report.html.erb b/app/views/dashboard/_low_inventory_report.html.erb
index 9563eaed98..3beaecf124 100644
--- a/app/views/dashboard/_low_inventory_report.html.erb
+++ b/app/views/dashboard/_low_inventory_report.html.erb
@@ -19,16 +19,16 @@
       <tbody>
         <% @low_inventory_report.each do |inventory_item| %>
           <tr>
-            <td><%= inventory_item["name"] %></td>
+            <td><%= inventory_item.name %></td>
             <td>
-              <% if inventory_item["total_quantity"] < inventory_item["on_hand_minimum_quantity"] %>
-                <strong class="text-red" title="Below minimum quantity"><%= inventory_item["total_quantity"] %></strong>
+              <% if inventory_item.total_quantity < inventory_item.on_hand_minimum_quantity %>
+                <strong class="text-red" title="Below minimum quantity"><%= inventory_item.total_quantity %></strong>
               <% else %>
-                <%= inventory_item["total_quantity"] %>
+                <%= inventory_item.total_quantity %>
               <% end %>
             </td>
-            <td><%= inventory_item["on_hand_minimum_quantity"] %></td>
-            <td><%= inventory_item["on_hand_recommended_quantity"] %></td>
+            <td><%= inventory_item.on_hand_minimum_quantity %></td>
+            <td><%= inventory_item.on_hand_recommended_quantity %></td>
           </tr>
         <% end %>
       </tbody>
diff --git a/app/views/events/index.html.erb b/app/views/events/index.html.erb
index ea24a1f682..b9e5cb3b6d 100644
--- a/app/views/events/index.html.erb
+++ b/app/views/events/index.html.erb
@@ -42,7 +42,7 @@
                   <%= render partial: "shared/date_range_picker", locals: {css_class: "form-control"} %>
                 </div>
                 <div class="form-group col-lg-3 col-md-3 col-sm-6 col-xs-12">
-                  <%= filter_select(scope: :by_type, collection: Event.types_for_select, key: :value, value: :name,
+                  <%= filter_select(scope: :by_type, collection: Event.types_for_select, key: :id, value: :name,
                                     selected: params.dig(:filters, :by_type)) %>
                 </div>
                 <div class="form-group col-lg-3 col-md-3 col-sm-6 col-xs-12">
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index a10991754f..98299f7a8c 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -17,6 +17,25 @@
 RSpec.describe Event, type: :model do
   let(:organization) { create(:organization) }
 
+  describe "::types_for_select" do
+    let(:a_event) { Class.new { def self.name = "AEvent" } }
+    let(:z_event) { Class.new { def self.name = "ZEvent" } }
+
+    before do
+      allow(described_class).to receive(:descendants).and_return([z_event, a_event])
+    end
+
+    it "returns an array of ids and names" do
+      results = described_class.types_for_select
+      ids = results.map(&:id)
+      names = results.map(&:name)
+
+      expect(results.size).to eq(2)
+      expect(ids).to eq(["AEvent", "ZEvent"])
+      expect(names).to eq(["A", "Z"])
+    end
+  end
+
   describe "#most_recent_snapshot" do
     let(:eventable) { FactoryBot.create(:distribution, organization_id: organization.id) }
     let(:data) { EventTypes::Inventory.new(storage_locations: {}, organization_id: organization.id) }
diff --git a/spec/requests/transfers_requests_spec.rb b/spec/requests/transfers_requests_spec.rb
index 65e2441150..49a4d5d6e1 100644
--- a/spec/requests/transfers_requests_spec.rb
+++ b/spec/requests/transfers_requests_spec.rb
@@ -101,10 +101,10 @@
       end
 
       context 'when the transfer destroy service was successful' do
-        let(:fake_success_struct) { OpenStruct.new(success?: true) }
+        let(:successful_result) { Result.new }
 
         before do
-          allow(fake_destroy_service).to receive(:call).and_return(fake_success_struct)
+          allow(fake_destroy_service).to receive(:call).and_return(successful_result)
           subject
         end
 
@@ -115,11 +115,11 @@
       end
 
       context 'when the transfer destroy service was not successful' do
-        let(:fake_error_struct) { OpenStruct.new(success?: false, error: fake_error) }
+        let(:failing_result) { Result.new(error: fake_error) }
         let(:fake_error) { StandardError.new('fake-error-msg') }
 
         before do
-          allow(fake_destroy_service).to receive(:call).and_return(fake_error_struct)
+          allow(fake_destroy_service).to receive(:call).and_return(failing_result)
           subject
         end
 
diff --git a/spec/services/item_create_service_spec.rb b/spec/services/item_create_service_spec.rb
index 54e005ea44..8c8d3eb555 100644
--- a/spec/services/item_create_service_spec.rb
+++ b/spec/services/item_create_service_spec.rb
@@ -27,10 +27,10 @@
     end
 
     context 'when there are no issues' do
-      it 'should return an OpenStruct with success? set to true and the item' do
-        expect(subject).to be_a_kind_of(OpenStruct)
+      it 'should return a result object with success? returning true and the item' do
+        expect(subject).to be_a_kind_of(Result)
         expect(subject.success?).to eq(true)
-        expect(subject.item).to eq(fake_organization_item)
+        expect(subject.value).to eq(fake_organization_item)
       end
 
       it 'should execute the expected methods' do
@@ -48,8 +48,8 @@
           allow(Organization).to receive(:find).with(organization_id).and_raise(ActiveRecord::RecordNotFound)
         end
 
-        it 'should return a OpenStruct with an ActiveRecord::RecordNotFound error' do
-          expect(subject).to be_a_kind_of(OpenStruct)
+        it 'should return a result object with an ActiveRecord::RecordNotFound error' do
+          expect(subject).to be_a_kind_of(Result)
           expect(subject.success?).to eq(false)
           expect(subject.error).to be_a_kind_of(ActiveRecord::RecordNotFound)
         end
@@ -62,8 +62,8 @@
           allow(fake_organization_item).to receive(:save!).and_raise(fake_error)
         end
 
-        it 'should return a OpenStruct with the raised error' do
-          expect(subject).to be_a_kind_of(OpenStruct)
+        it 'should return a result object with the raised error' do
+          expect(subject).to be_a_kind_of(Result)
           expect(subject.success?).to eq(false)
           expect(subject.error).to eq(fake_error)
         end
diff --git a/spec/services/kit_create_service_spec.rb b/spec/services/kit_create_service_spec.rb
index 9a2aaaadb6..afe6c837bb 100644
--- a/spec/services/kit_create_service_spec.rb
+++ b/spec/services/kit_create_service_spec.rb
@@ -62,11 +62,11 @@
       end
 
       context 'but the ItemCreationService is unsuccesful' do
-        let(:fake_error_struct) { OpenStruct.new(success?: false, error: error) }
+        let(:failing_result) { Result.new(error: error) }
         let(:error) { Faker::Name.name }
 
         before do
-          allow_any_instance_of(ItemCreateService).to receive(:call).and_return(fake_error_struct)
+          allow_any_instance_of(ItemCreateService).to receive(:call).and_return(failing_result)
         end
 
         it 'should not create the Kit' do
diff --git a/spec/services/transfer_destroy_service_spec.rb b/spec/services/transfer_destroy_service_spec.rb
index 0e75af925a..269a4c8a5b 100644
--- a/spec/services/transfer_destroy_service_spec.rb
+++ b/spec/services/transfer_destroy_service_spec.rb
@@ -33,9 +33,9 @@
     end
 
     context 'when there are no issues' do
-      it 'should return an OpenStruct with success? set to true' do
+      it 'should return a result object with success? returning true' do
         expect { subject }.to change { TransferDestroyEvent.count }.by(1)
-        expect(subject).to be_a_kind_of(OpenStruct)
+        expect(subject).to be_a_kind_of(Result)
         expect(subject.success?).to eq(true)
       end
 
@@ -54,8 +54,8 @@
           allow(Transfer).to receive(:find).with(transfer_id).and_raise(ActiveRecord::RecordNotFound)
         end
 
-        it 'should return a OpenStruct with an ActiveRecord::RecordNotFound error' do
-          expect(subject).to be_a_kind_of(OpenStruct)
+        it 'should return a result object with an ActiveRecord::RecordNotFound error' do
+          expect(subject).to be_a_kind_of(Result)
           expect(subject.success?).to eq(false)
           expect(subject.error).to be_a_kind_of(ActiveRecord::RecordNotFound)
         end
@@ -66,8 +66,8 @@
           allow(TransferDestroyEvent).to receive(:publish).and_raise('OH NOES')
         end
 
-        it 'should return a OpenStruct with the raised error' do
-          expect(subject).to be_a_kind_of(OpenStruct)
+        it 'should return a result object with the raised error' do
+          expect(subject).to be_a_kind_of(Result)
           expect(subject.success?).to eq(false)
           expect(subject.error.message).to eq('OH NOES')
         end
@@ -79,8 +79,8 @@
           allow(transfer).to receive(:destroy!).and_raise(fake_error)
         end
 
-        it 'should return a OpenStruct with the raised error' do
-          expect(subject).to be_a_kind_of(OpenStruct)
+        it 'should return a result object with the raised error' do
+          expect(subject).to be_a_kind_of(Result)
           expect(subject.success?).to eq(false)
           expect(subject.error).to eq(fake_error)
         end
@@ -92,8 +92,8 @@
         allow(Audit).to receive(:finalized_since?).and_return(true)
       end
 
-      it 'should return an OpenStruct with the raised error' do
-        expect(subject).to be_a_kind_of(OpenStruct)
+      it 'should return an result object with the raised error' do
+        expect(subject).to be_a_kind_of(Result)
         expect(subject.success?).to eq(false)
         expect(subject.error).to be_a_kind_of(StandardError)
       end
diff --git a/spec/system/dashboard_system_spec.rb b/spec/system/dashboard_system_spec.rb
index fc9359afdb..3ac040a5c0 100644
--- a/spec/system/dashboard_system_spec.rb
+++ b/spec/system/dashboard_system_spec.rb
@@ -1,5 +1,3 @@
-require 'ostruct'
-
 RSpec.describe "Dashboard", type: :system, js: true do
   let(:organization) { create(:organization) }
   let(:user) { create(:user, organization: organization) }