Merge pull request #5388 from rubyforgood/5358-donation-refactor

#5358: Refactor donations controller to use a view object

Author: awwaiid

app/controllers/donations_controller.rb

@@ -18,40 +18,14 @@ def print
   def index
     setup_date_range_picker
 
-    @donations = current_organization.donations
-                                     .includes(:storage_location, :donation_site, :product_drive, :product_drive_participant, :manufacturer, line_items: [:item])
-                                     .order(created_at: :desc)
-                                     .class_filter(filter_params)
-                                     .during(helpers.selected_range)
-    @item_categories = current_organization.item_categories.pluck(:name).uniq
-    @paginated_donations = @donations.page(params[:page])
-
-    @product_drives = current_organization.product_drives.alphabetized
-    @product_drive_participants = current_organization.product_drive_participants.alphabetized
-
-    # Are these going to be inefficient with large datasets?
-    # Using the @donations allows drilling down instead of always starting with the total dataset
-    @donations_quantity = @donations.collect(&:total_quantity).sum
-    @paginated_donations_quantity = @paginated_donations.collect(&:total_quantity).sum
-    @total_value_all_donations = total_value(@donations)
-    @paginated_in_kind_value = total_value(@paginated_donations)
-    @total_money_raised = total_money_raised(@donations)
-    @storage_locations = @donations.filter_map { |donation| donation.storage_location if !donation.storage_location.discarded_at }.compact.uniq.sort
-    @selected_storage_location = filter_params[:at_storage_location]
-    @sources = @donations.collect(&:source).uniq.sort
-    @selected_source = filter_params[:by_source]
-    @selected_item_category = filter_params[:by_category]
-    @donation_sites = @donations.collect(&:donation_site).compact.uniq.sort_by { |site| site.name.downcase }
-    @selected_donation_site = filter_params[:from_donation_site]
-    @selected_product_drive = filter_params[:by_product_drive]
-    @selected_product_drive_participant = filter_params[:by_product_drive_participant]
-    @manufacturers = @donations.collect(&:manufacturer).compact.uniq.sort
-    @selected_manufacturer = filter_params[:from_manufacturer]
+    @donation_info = View::Donations.from_params(params: params, organization: current_organization, helpers: helpers)
 
     respond_to do |format|
       format.html
       format.csv do
-        send_data Exports::ExportDonationsCSVService.new(donation_ids: @donations.map(&:id), organization: current_organization).generate_csv, filename: "Donations-#{Time.zone.today}.csv"
+        send_data Exports::ExportDonationsCSVService.new(donation_ids: @donation_info.donations.map(&:id),
+          organization: current_organization).generate_csv,
+          filename: "Donations-#{Time.zone.today}.csv"
       end
     end
   end
@@ -177,16 +151,4 @@ def compact_line_items
     params[:donation][:line_items_attributes].delete_if { |_row, data| data["quantity"].blank? && data["item_id"].blank? }
     params
   end
-
-  def total_value(donations)
-    total_value_all_donations = 0
-    donations.each do |donation|
-      total_value_all_donations += donation.value_per_itemizable
-    end
-    total_value_all_donations
-  end
-
-  def total_money_raised(donations)
-    donations.sum { |d| d.money_raised.to_i }
-  end
 end

app/models/view/donations.rb

@@ -0,0 +1,115 @@
+module View
+  Donations = Data.define(
+    :donations,
+    :filters,
+    :item_categories,
+    :paginated_donations,
+    :product_drives,
+    :product_drive_participants,
+    :storage_locations,
+    :donation_sites,
+    :manufacturers
+  ) do
+    include DateRangeHelper
+
+    class << self
+      def filter_params(params)
+        if params.key?(:filters)
+          params.require(:filters).permit(
+            :at_storage_location, :by_source, :from_donation_site,
+            :by_product_drive, :by_product_drive_participant,
+            :from_manufacturer, :by_category
+          )
+        else
+          {}
+        end
+      end
+
+      def from_params(params:, organization:, helpers:)
+        filters = filter_params(params)
+        donations = organization.donations
+          .includes(:storage_location,
+            :donation_site,
+            :product_drive,
+            :product_drive_participant,
+            :manufacturer,
+            line_items: [:item])
+          .order(created_at: :desc)
+          .class_filter(filters)
+          .during(helpers.selected_range)
+
+        paginated_donations = donations.page(params[:page])
+
+        storage_locations = donations.filter_map do |donation|
+          donation.storage_location unless donation.storage_location.discarded_at
+        end.compact.uniq.sort
+
+        manufacturers = donations.collect(&:manufacturer).compact.uniq.sort
+
+        new(
+          donations: donations,
+          filters: filters,
+          item_categories: organization.item_categories.pluck(:name).uniq,
+          paginated_donations: paginated_donations,
+          product_drives: organization.product_drives.alphabetized,
+          product_drive_participants: organization.product_drive_participants.alphabetized,
+          storage_locations: storage_locations,
+          donation_sites: donations.map(&:donation_site).compact.uniq.sort_by { |site| site.name.downcase },
+          manufacturers: manufacturers
+        )
+      end
+    end
+
+    def selected_storage_location
+      filters[:at_storage_location]
+    end
+
+    def selected_source
+      filters[:by_source]
+    end
+
+    def selected_item_category
+      filters[:by_category]
+    end
+
+    def sources
+      donations.map(&:source).uniq.sort
+    end
+
+    def donations_quantity
+      donations.map(&:total_quantity).sum
+    end
+
+    def selected_donation_site
+      filters[:from_donation_site]
+    end
+
+    def selected_product_drive
+      filters[:by_product_drive]
+    end
+
+    def selected_product_drive_participant
+      filters[:by_product_drive_participant]
+    end
+
+    def selected_manufacturer
+      filters[:from_manufacturer]
+    end
+
+    def paginated_donations_quantity
+      paginated_donations.map(&:total_quantity).sum
+    end
+
+    def paginated_in_kind_value
+      paginated_donations.sum { |donation| donation.value_per_itemizable }
+    end
+
+    def total_money_raised
+      donations.sum { |d| d.money_raised.to_i }
+    end
+
+    def total_value_all_donations
+      donations.sum { |donation| donation.value_per_itemizable }
+    end
+  end
+end

app/views/donations/index.html.erb

@@ -36,47 +36,63 @@
           <div class="card-body">
             <%= form_tag(donations_path, method: :get) do |f| %>
               <div class="row">
-                <% if @storage_locations.present? %>
+                <% if @donation_info.storage_locations.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
-                    <%= filter_select(label: "Filter by Storage Location", scope: :at_storage_location, collection: @storage_locations, selected: @selected_storage_location) %>
+                    <%= filter_select(label: "Filter by Storage Location",
+                                      scope: :at_storage_location,
+                                      collection: @donation_info.storage_locations,
+                                      selected: @donation_info.selected_storage_location) %>
                   </div>
                 <% end %>
-                <% if @sources.present? %>
+                <% if @donation_info.sources.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
                     <% id = "filter_#{SecureRandom.uuid}" %>
                     <%= label_tag id, "Filter by Source" %>
                     <%= select_tag "filters[by_source]",
-                    options_for_select(@sources, @selected_source),
+                    options_for_select(@donation_info.sources, @donation_info.selected_source),
                     { include_blank: true, class: "form-control", id: id } %>
                   </div>
                 <% end %>
-                <% if @product_drives.present? %>
+                <% if @donation_info.product_drives.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
-                    <%= filter_select(scope: :by_product_drive, collection: @product_drives, selected: @selected_product_drive) %>
+                    <%= filter_select(scope: :by_product_drive,
+                                      collection: @donation_info.product_drives,
+                                      selected: @donation_info.selected_product_drive) %>
 
                   </div>
                 <% end %>
-                <% if @product_drive_participants.present? %>
+                <% if @donation_info.product_drive_participants.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
-                    <%= filter_select(scope: :by_product_drive_participant, collection: @product_drive_participants, value: :business_name, selected: @selected_product_drive_participant) %>
+                    <%= filter_select(scope: :by_product_drive_participant,
+                                      collection: @donation_info.product_drive_participants,
+                                      value: :business_name,
+                                      selected: @donation_info.selected_product_drive_participant) %>
                   </div>
                 <% end %>
-                <% if @manufacturers.present? %>
+                <% if @donation_info.manufacturers.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
-                    <%= filter_select(label: "Filter by manufacturer", scope: :from_manufacturer, collection: @manufacturers, selected: @selected_manufacturer) %>
+                    <%= filter_select(label: "Filter by manufacturer",
+                                      scope: :from_manufacturer,
+                                      collection: @donation_info.manufacturers,
+                                      selected: @donation_info.selected_manufacturer) %>
                   </div>
                 <% end %>
-                <% if @donation_sites.present? %>
+                <% if @donation_info.donation_sites.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
-                    <%= filter_select(label: "Filter by Donation Site", scope: :from_donation_site, collection: @donation_sites, key: :id, value: :name, selected: @selected_donation_site) %>
+                    <%= filter_select(label: "Filter by Donation Site",
+                                      scope: :from_donation_site,
+                                      collection: @donation_info.donation_sites,
+                                      key: :id,
+                                      value: :name,
+                                      selected: @donation_info.selected_donation_site) %>
                   </div>
                 <% end %>
-                <% if @item_categories.present? %>
+                <% if @donation_info.item_categories.present? %>
                   <div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
                     <% id = "filter_#{SecureRandom.uuid}" %>
                     <%= label_tag id, "Filter by Category" %>
                     <%= select_tag "filters[by_category]",
-                    options_for_select(@item_categories, @selected_item_category),
+                    options_for_select(@donation_info.item_categories, @donation_info.selected_item_category),
                     { include_blank: true, class: "form-control", id: id } %>
                   </div>
                 <% end %>
@@ -89,7 +105,11 @@
                 <%= filter_button %>
                 <%= clear_filter_button %>
                 <span class="float-right">
-                    <%= download_button_to(donations_path(format: :csv, filters: filter_params.merge(date_range: date_range_params)), {text: "Export Donations", size: "md"}) if @donations.any? %>
+                  <% if @donation_info.donations.any? %>
+                    <%= download_button_to(donations_path(format: :csv,
+                                                          filters: filter_params.merge(date_range: date_range_params)),
+                                           {text: "Export Donations", size: "md"}) %>
+                  <% end %>
                   <%= new_button_to new_donation_path, {text: "New Donation"} %>
                     </span>
               </div>
@@ -124,7 +144,7 @@
                 </tr>
               </thead>
               <tbody>
-                <%= render partial: "donation_row", collection: @paginated_donations %>
+                <%= render partial: "donation_row", collection: @donation_info.paginated_donations %>
               </tbody>
               <tfoot>
               <tr>
@@ -133,24 +153,24 @@
                 <td></td>
                 <td></td>
                 <td class="numeric">
-                  <%= @paginated_donations_quantity %>
+                  <%= @donation_info.paginated_donations_quantity %>
                   <br>
                   (This page)
                   <br>
                   <strong id="donation_quantity">
-                    <%= @donations_quantity %>
+                    <%= @donation_info.donations_quantity %>
                     <br>
                     (Total)
                   </strong>
                 </td>
-                <td class="numeric"><strong><%= dollar_value(@total_money_raised) %></strong></td>
+                <td class="numeric"><strong><%= dollar_value(@donation_info.total_money_raised) %></strong></td>
                 <td class="numeric in-kind">
-                  <%= dollar_value(@paginated_in_kind_value) %>
+                  <%= dollar_value(@donation_info.paginated_in_kind_value) %>
                   <br>
                    (This page)
                   <br>
                   <strong>
-                    <%= dollar_value(@total_value_all_donations) %> (Total)
+                    <%= dollar_value(@donation_info.total_value_all_donations) %> (Total)
                   </strong>
                 </td>
               </tr>
@@ -159,7 +179,7 @@
           </div>
           <!-- /.card-body -->
           <div class="card-footer clearfix">
-            <%= paginate @paginated_donations %>
+            <%= paginate @donation_info.paginated_donations %>
           </div>
           <!-- /.card-footer-->
         </div>

spec/controllers/donations_controller_spec.rb

@@ -147,34 +147,4 @@
       end
     end
   end
-
-  context 'calculating total value of multiple donations' do
-    it 'works correctly for multiple line items per donation' do
-      donations = [
-        create(:donation, :with_items, item_quantity: 1),
-        create(:donation, :with_items, item_quantity: 2)
-      ]
-      value = subject.send(:total_value, donations) # private method, need to use `send`
-      expect(value).to eq(300)
-    end
-
-    it 'returns zero for an empty array of donations' do
-      expect(subject.send(:total_value, [])).to be_zero # private method, need to use `send`
-    end
-  end
-
-  context 'calculating total money raised for all donations' do
-    it 'correctly calculates the total' do
-      donations = [
-        create(:donation, money_raised: 2),
-        create(:donation, money_raised: 3)
-      ]
-      value = subject.send(:total_money_raised, donations) # private method, need to use `send`
-      expect(value).to eq(5)
-    end
-
-    it 'returns zero for an empty array of donations' do
-      expect(subject.send(:total_money_raised, [])).to be_zero # private method, need to use `send`
-    end
-  end
 end