From 206a1658216ebdd47c537628c065ff4e1264f0f8 Mon Sep 17 00:00:00 2001 From: guswhitten Date: Tue, 30 Jul 2024 07:39:00 -0400 Subject: [PATCH 1/5] Resolves #4544. filter manufacturer donations summary report by selected range --- app/controllers/reports_controller.rb | 7 ++- app/models/manufacturer.rb | 13 ++--- app/views/reports/_manufacturer.html.erb | 10 ++-- .../manufacturer_donations_summary_spec.rb | 48 ++++++++++++++++++- 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 7c177789f9..db87324bb3 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -7,10 +7,9 @@ def donations_summary end def manufacturer_donations_summary - @recent_donations_from_manufacturers = current_organization.donations.during(helpers.selected_range).by_source(:manufacturer) - @top_manufacturers = current_organization.manufacturers.by_donation_count - @donations = current_organization.donations.during(helpers.selected_range) - @recent_donations = @donations.recent + @date_range = helpers.selected_range + @recent_donations_from_manufacturers = current_organization.donations.during(@date_range).by_source(:manufacturer) + @top_manufacturers = current_organization.manufacturers.by_donation_count(10, @date_range) end def purchases_summary diff --git a/app/models/manufacturer.rb b/app/models/manufacturer.rb index 3a274fefa8..4836f2603d 100644 --- a/app/models/manufacturer.rb +++ b/app/models/manufacturer.rb @@ -21,15 +21,16 @@ class Manufacturer < ApplicationRecord scope :alphabetized, -> { order(:name) } - def volume - # returns 0 instead of nil when Manufacturer exists without any donations - donations.joins(:line_items).sum(:quantity) + def volume(date_range = nil) + scope = donations.joins(:line_items) + scope = scope.where(issued_at: date_range) if date_range + scope.sum(:quantity) end - def self.by_donation_count(count = 10) - # selects manufacturers that have donation qty > 0 + def self.by_donation_count(count = 10, date_range = nil) + # selects manufacturers that have donation qty > 0 in the timeframe # and sorts them by highest volume of donation - select { |m| m.volume.positive? }.sort.reverse.first(count) + select { |m| m.volume(date_range).positive? }.sort.first(count) end private diff --git a/app/views/reports/_manufacturer.html.erb b/app/views/reports/_manufacturer.html.erb index 67fe3ad341..bf1ca19f9e 100644 --- a/app/views/reports/_manufacturer.html.erb +++ b/app/views/reports/_manufacturer.html.erb @@ -1,5 +1,5 @@ -
- <%= link_to manufacturer do %> - <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.volume) %>) - <% end %> -
+
+ <%= link_to manufacturer do %> + <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.volume(@date_range)) %>) + <% end %> +
diff --git a/spec/requests/reports/manufacturer_donations_summary_spec.rb b/spec/requests/reports/manufacturer_donations_summary_spec.rb index bd3285c751..0123830f67 100644 --- a/spec/requests/reports/manufacturer_donations_summary_spec.rb +++ b/spec/requests/reports/manufacturer_donations_summary_spec.rb @@ -1,6 +1,8 @@ RSpec.describe "Reports::ManufacturerDonationsSummary", type: :request do let(:organization) { create(:organization) } let(:user) { create(:user, organization: organization) } + let(:manufacturer1) { create(:manufacturer, organization: organization, name: "Manufacturer 1") } + let(:manufacturer2) { create(:manufacturer, organization: organization, name: "Manufacturer 2") } describe "while signed in" do before do @@ -9,13 +11,55 @@ describe "GET #index" do subject do - get reports_manufacturer_donations_summary_path(format: response_format) + get reports_manufacturer_donations_summary_path(format: "html") response end - let(:response_format) { "html" } it { is_expected.to have_http_status(:success) } end + + context "when visiting the summary page" do + it "has a link to create a new donation" do + get reports_manufacturer_donations_summary_path + + expect(response.body).to include("New Donation") + expect(response.body).to include("#{@url_prefix}/donations/new") + end + + context "with manufacturer donations in the last year" do + let(:formatted_date_range) { date_range.map { _1.to_formatted_s(:date_picker) }.join(" - ") } + let(:date_range) { [1.year.ago, 0.days.ago] } + let!(:donations) do + [ + create(:donation, :with_items, item_quantity: 2, issued_at: 5.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), + create(:donation, :with_items, item_quantity: 3, issued_at: 3.months.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), + create(:donation, :with_items, item_quantity: 7, issued_at: 2.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), + create(:donation, :with_items, item_quantity: 11, issued_at: 0.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), + create(:donation, :with_items, item_quantity: 13, issued_at: 20.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), + create(:donation, :with_items, item_quantity: 17, issued_at: 5.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2) + ] + end + + it "shows correct total donations" do + get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} + + expect(response.body).to match(%r{\s*29\s*}) + end + + it "shows correct individual donations for each manufacturer" do + get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} + + expect(response.body).to match(%r{Manufacturer 1 \(5\)}) + expect(response.body).to match(%r{Manufacturer 2 \(24\)}) + end + + it "shows top manufacturers in desc. order" do + get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} + + expect(response.body).to match(%r{Manufacturer 2 .*Manufacturer 1}m) + end + end + end end describe "while not signed in" do From 0410f61c5c2676b0e6e238ad8e5e808f20a44bdf Mon Sep 17 00:00:00 2001 From: guswhitten Date: Tue, 30 Jul 2024 08:00:46 -0400 Subject: [PATCH 2/5] initiate workflow? --- spec/requests/reports/manufacturer_donations_summary_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/reports/manufacturer_donations_summary_spec.rb b/spec/requests/reports/manufacturer_donations_summary_spec.rb index 0123830f67..7804c97174 100644 --- a/spec/requests/reports/manufacturer_donations_summary_spec.rb +++ b/spec/requests/reports/manufacturer_donations_summary_spec.rb @@ -40,7 +40,7 @@ ] end - it "shows correct total donations" do + it "shows correct total received donations" do get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} expect(response.body).to match(%r{\s*29\s*}) From 7cac9dca7eff2c8c91ca5c52b9ebae5d457d99e1 Mon Sep 17 00:00:00 2001 From: guswhitten Date: Fri, 2 Aug 2024 08:27:55 -0400 Subject: [PATCH 3/5] add instance variable to each manufacturer to hold number of donations in current timeframe --- app/controllers/reports_controller.rb | 5 ++--- app/models/manufacturer.rb | 18 ++++++++++-------- app/views/reports/_manufacturer.html.erb | 2 +- .../manufacturer_donations_summary_spec.rb | 16 +++++++++------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index db87324bb3..cbd87cb409 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -7,9 +7,8 @@ def donations_summary end def manufacturer_donations_summary - @date_range = helpers.selected_range - @recent_donations_from_manufacturers = current_organization.donations.during(@date_range).by_source(:manufacturer) - @top_manufacturers = current_organization.manufacturers.by_donation_count(10, @date_range) + @recent_donations_from_manufacturers = current_organization.donations.during(helpers.selected_range).by_source(:manufacturer) + @top_manufacturers = current_organization.manufacturers.by_donation_count(10, helpers.selected_range) end def purchases_summary diff --git a/app/models/manufacturer.rb b/app/models/manufacturer.rb index 4836f2603d..6c56a740b7 100644 --- a/app/models/manufacturer.rb +++ b/app/models/manufacturer.rb @@ -21,16 +21,18 @@ class Manufacturer < ApplicationRecord scope :alphabetized, -> { order(:name) } - def volume(date_range = nil) - scope = donations.joins(:line_items) - scope = scope.where(issued_at: date_range) if date_range - scope.sum(:quantity) - end - def self.by_donation_count(count = 10, date_range = nil) - # selects manufacturers that have donation qty > 0 in the timeframe + # selects manufacturers that have donation qty > 0 in the provided date range # and sorts them by highest volume of donation - select { |m| m.volume(date_range).positive? }.sort.first(count) + manufacturers = select do |m| + donation_volume = m.donations.joins(:line_items).where(issued_at: date_range).sum(:quantity) + if donation_volume.positive? + m.instance_variable_set(:@num_of_donations, donation_volume) + m + end + end + + manufacturers.sort_by { |m| -m.instance_variable_get(:@num_of_donations) }.first(count) end private diff --git a/app/views/reports/_manufacturer.html.erb b/app/views/reports/_manufacturer.html.erb index bf1ca19f9e..abe6a3d799 100644 --- a/app/views/reports/_manufacturer.html.erb +++ b/app/views/reports/_manufacturer.html.erb @@ -1,5 +1,5 @@
<%= link_to manufacturer do %> - <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.volume(@date_range)) %>) + <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.instance_variable_get(:@num_of_donations)) %>) <% end %>
diff --git a/spec/requests/reports/manufacturer_donations_summary_spec.rb b/spec/requests/reports/manufacturer_donations_summary_spec.rb index 7804c97174..e9172e1e9e 100644 --- a/spec/requests/reports/manufacturer_donations_summary_spec.rb +++ b/spec/requests/reports/manufacturer_donations_summary_spec.rb @@ -3,6 +3,7 @@ let(:user) { create(:user, organization: organization) } let(:manufacturer1) { create(:manufacturer, organization: organization, name: "Manufacturer 1") } let(:manufacturer2) { create(:manufacturer, organization: organization, name: "Manufacturer 2") } + let(:manufacturer3) { create(:manufacturer, organization: organization, name: "Manufacturer 3") } describe "while signed in" do before do @@ -33,30 +34,31 @@ [ create(:donation, :with_items, item_quantity: 2, issued_at: 5.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), create(:donation, :with_items, item_quantity: 3, issued_at: 3.months.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), - create(:donation, :with_items, item_quantity: 7, issued_at: 2.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer1), - create(:donation, :with_items, item_quantity: 11, issued_at: 0.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), - create(:donation, :with_items, item_quantity: 13, issued_at: 20.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), - create(:donation, :with_items, item_quantity: 17, issued_at: 5.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2) + create(:donation, :with_items, item_quantity: 7, issued_at: 2.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), + create(:donation, :with_items, item_quantity: 1, issued_at: 0.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer2), + create(:donation, :with_items, item_quantity: 13, issued_at: 20.days.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer3), + create(:donation, :with_items, item_quantity: 17, issued_at: 5.years.ago, organization: organization, source: "Manufacturer", manufacturer: manufacturer3) ] end it "shows correct total received donations" do get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} - expect(response.body).to match(%r{\s*29\s*}) + expect(response.body).to match(%r{\s*19\s*}) end it "shows correct individual donations for each manufacturer" do get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} expect(response.body).to match(%r{Manufacturer 1 \(5\)}) - expect(response.body).to match(%r{Manufacturer 2 \(24\)}) + expect(response.body).to match(%r{Manufacturer 2 \(1\)}) + expect(response.body).to match(%r{Manufacturer 3 \(13\)}) end it "shows top manufacturers in desc. order" do get reports_manufacturer_donations_summary_path(user.organization), params: {filters: {date_range: formatted_date_range}} - expect(response.body).to match(%r{Manufacturer 2 .*Manufacturer 1}m) + expect(response.body).to match(%r{Manufacturer 3 .* Manufacturer 1 .*Manufacturer 2}m) end end end From 999fe442d0d8d67e60091604d97f17c3a8dad7f7 Mon Sep 17 00:00:00 2001 From: guswhitten Date: Sun, 4 Aug 2024 12:47:17 -0400 Subject: [PATCH 4/5] set donation count variable and sort using active record query --- app/models/manufacturer.rb | 15 ++++++--------- app/views/reports/_manufacturer.html.erb | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/app/models/manufacturer.rb b/app/models/manufacturer.rb index 6c56a740b7..770288af7f 100644 --- a/app/models/manufacturer.rb +++ b/app/models/manufacturer.rb @@ -24,15 +24,12 @@ class Manufacturer < ApplicationRecord def self.by_donation_count(count = 10, date_range = nil) # selects manufacturers that have donation qty > 0 in the provided date range # and sorts them by highest volume of donation - manufacturers = select do |m| - donation_volume = m.donations.joins(:line_items).where(issued_at: date_range).sum(:quantity) - if donation_volume.positive? - m.instance_variable_set(:@num_of_donations, donation_volume) - m - end - end - - manufacturers.sort_by { |m| -m.instance_variable_get(:@num_of_donations) }.first(count) + joins(donations: :line_items).where(donations: { issued_at: date_range }) + .select('manufacturers.*, sum(line_items.quantity) as donation_count') + .group('manufacturers.id') + .having('sum(line_items.quantity) > 0') + .order('donation_count DESC') + .limit(count) end private diff --git a/app/views/reports/_manufacturer.html.erb b/app/views/reports/_manufacturer.html.erb index abe6a3d799..ea42675571 100644 --- a/app/views/reports/_manufacturer.html.erb +++ b/app/views/reports/_manufacturer.html.erb @@ -1,5 +1,5 @@
<%= link_to manufacturer do %> - <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.instance_variable_get(:@num_of_donations)) %>) + <%= manufacturer.name %> (<%= number_with_delimiter(manufacturer.donation_count) %>) <% end %>
From 5bbbb2f5ff86d8d813893d4cb63f21adad9f72fa Mon Sep 17 00:00:00 2001 From: guswhitten Date: Sun, 25 Aug 2024 09:04:54 -0400 Subject: [PATCH 5/5] add back def volume method in manufacturer model --- app/models/manufacturer.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/manufacturer.rb b/app/models/manufacturer.rb index 770288af7f..21034cc1eb 100644 --- a/app/models/manufacturer.rb +++ b/app/models/manufacturer.rb @@ -21,6 +21,11 @@ class Manufacturer < ApplicationRecord scope :alphabetized, -> { order(:name) } + def volume + # returns 0 instead of nil when Manufacturer exists without any donations + donations.joins(:line_items).sum(:quantity) + end + def self.by_donation_count(count = 10, date_range = nil) # selects manufacturers that have donation qty > 0 in the provided date range # and sorts them by highest volume of donation