Resolves #4544. filter manufacturer donations summary report by date #4558
Conversation
…rt by selected range
There was a problem hiding this comment.
Hey @guswhitten -- This is just a functional, not a technical review. It looks like the filtration works -- but the ranking isn't working properly as yet.
I added a third manufacturer with a 5000 item donation on December 31 2023, and did an "All Time" filtration and got the result below. The manufacturers should be listed in order of number of items from most to least.
…s in current timeframe
|
@ dorner - bit late in the day, but if you have the cycles still, could you take a technical look at this? (I'll do the last functional confirm either later today or in the morning) |
app/models/manufacturer.rb
Outdated
| 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) |
There was a problem hiding this comment.
We really don't want to set instance variables from outside the class. The way I'd do this is to create a method on Manufacturer called donation_count which you can memoize if necessary by setting the result to an instance variable if it doesn't exist, e.g. @donation_count ||= ... .
Having said that, this is something you can solve in straight SQL (or ActiveRecord). It should be something along the lines of:
self.joins(donations: :line_items).where(donations: {issued_at: date_range}).
select('sum(quantity) as quantity_count').
group('manufacturers.id').
having('sum(quantity) > 0').
order('quantity_count desc').to_ahttps://chatgpt.com/g/g-m5lMeGifF-sql-expert is a good resource to talk through this if necessary.
There was a problem hiding this comment.
That looks like a significant enough adjustment that I'm going to hold off on the final functional check.
There was a problem hiding this comment.
Hey @guswhitten - unfortunately removing the volume method is breaking a bunch of specs (and a bunch of pages). Can this be fixed?
get latest from main
merge master
|
@dorner Bringing this one back to your attention. |
|
Functionality looks good. |
dorner
dismissed
cielf’s stale review
CL onfirmed functionality works now per above comment.
|
@guswhitten: Your PR |
…rt by date (rubyforgood#4558) * Resolves rubyforgood#4544. filter manufacturer donations summary report by selected range * initiate workflow? * add instance variable to each manufacturer to hold number of donations in current timeframe * set donation count variable and sort using active record query * add back def volume method in manufacturer model
Checklist:
Resolves #4544
Description
Manufacturer Donations Summary Report individual manufacturers donations count were not being filtered by the selected date range (past year).
Type of change
How Has This Been Tested?
Tests added in
spec/requests/reports/manufacturer_donations_summary_spec.rbScreenshots
Before:
After:
