4033 product drives filter by category #4094

PhantomSean · 2024-02-11T01:54:28Z

Resolves #4033

Description

Added new filter to the product drive page. This allows for filtration by item category. Made adjustments to the functions that display donation quantities and distinct item counts to return correct figures for when item category is chosen.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Visual Test:

Login as organization admin.
Go to the product drive page.
Select an item category from the drop down menu.
Apply the filter.
Confirm product drive are filtered correctly and that donation quantities and distinct item counts are correct.

Rspec Tests:
Added testing to spec/models/product_drive_spec.rb to confirm that

All donated organization item quantities by category and date are correctly calculated and returned.
All distinct donated organization items belonging to a given category are correctly counted and returned.
Filter by_item_category_id works as expected.

Screenshots

Before:

After:

…tegory

dorner · 2024-02-13T00:37:07Z

app/controllers/product_drives_controller.rb

@@ -6,10 +6,13 @@ def index
    setup_date_range_picker
    @product_drives = current_organization
                     .product_drives
+                     .includes(donations: [line_items: [:item]])


This is a weird syntax... I'd assume it should look like donations: {line_items: :item}. Not even sure how this parses 😮

Functionally they create the same SQL query but describe the associations using arrays vs using a hash as you have.

I've changed it as you suggested as it probably is better to have it described as a hash and it a bit more readable.

dorner · 2024-02-13T00:37:35Z

app/models/product_drive.rb

@@ -18,6 +18,7 @@ class ProductDrive < ApplicationRecord
  include Filterable

  scope :by_name, ->(name_filter) { where(name: name_filter) }
+  scope :by_item_category_id, ->(item_category_id) { joins(donations: [line_items: [:item]]).where(item: { item_category_id: item_category_id }) }


Can this line be split? Kinda long.

dorner · 2024-02-13T00:37:56Z

app/models/product_drive.rb

-      .during(date_range)
-      .sum('line_items.quantity')
+  def donation_quantity_by_date(date_range, item_category_id = nil)
+    if [nil, ''].include? item_category_id


More Rails-y: if item_category_id.blank? Better yet, use present? and switch the conditional so the positive condition comes first.

dorner · 2024-02-13T00:39:51Z

app/models/product_drive.rb

+        .where(item: {item_category_id: item_category_id})
+        .during(date_range)
+        .sum('line_items.quantity')
+    end


I usually try to build the query rather than copy the lines. So I'd write this thusly:

query = donations.during(date_range) if item_category_id.present? query = query.joins(line_items: :item).where(item: {item_category_id: item_category_id}) else query = query.joins(:line_items) end query.sum('line_items.quantity')

Thank you, that's a lot cleaner

dorner · 2024-02-13T00:40:24Z

app/models/product_drive.rb

+      donations.joins(line_items: [:item])
+        .where(item: {item_category_id: item_category_id})
+        .during(date_range)
+        .select('line_items.item_id').distinct.count


Same as above.

See above comment, can this be built reusing the parts that are common to both conditions?

dorner · 2024-02-13T00:41:42Z

spec/models/product_drive_spec.rb

+  it "calculates and returns all donated organization item quantities by category and date" do
+    create(:donation, :with_items, item_quantity: 2, product_drive: product_drive, issued_at: '26-01-2023')
+    result = product_drive.donation_quantity_by_date(Time.zone.parse('23/01/2023')..Time.zone.parse('26/01/2023'), 1)
+    expect(result).to eq product_drive.organization.items.where(item_category_id: 1).count


This is a bit too finicky IMO. I'd actually create an item with the desired item category and verify that it comes back in the query. Otherwise we could just not have any data in there and it comes back empty... but we wouldn't know if it was empty because there was no data, or because there's an error in our query. In general for things like this you'll want at least one result that should be included and one that shouldn't, and verify that only the one you want comes back.

Good feedback, I've created two items with unique category ids and have the test verifying the one we want comes back

…tegory

dorner

One last nitpick and then I think this is good to go!

dorner · 2024-02-25T14:54:11Z

app/models/product_drive.rb

+      donations.joins(line_items: [:item])
+        .where(item: {item_category_id: item_category_id})
+        .during(date_range)
+        .select('line_items.item_id').distinct.count


See above comment, can this be built reusing the parts that are common to both conditions?

PhantomSean · 2024-02-27T03:15:49Z

One last nitpick and then I think this is good to go!

Sorry missed that one! Should be sorted now 😄

PhantomSean · 2024-02-27T03:44:07Z

Seems as though there appears to be a flaky test case in spec/system/manufacturer_system_spec.rb:51 that caused the following timeout failure

Failures:

  1) Manufacturer allows a user to update a manufacturer with empty attributes
     Failure/Error: raise TimeoutError unless data

     Ferrum::TimeoutError:
       Timed out waiting for response. It's possible that this happened because something took a very long time (for example a page load was slow). If so, setting the :timeout option to a higher value might help.

Not sure if this a known issue.
see https://github.com/rubyforgood/human-essentials/actions/runs/8059102112/job/22012957655 for full log

And another flaky test in ./spec/system/distribution_system_spec.rb:111 but I believe this was confirmed to be a known issue based on the conversation here and that there is a fix on another branch

dorner · 2024-02-27T21:59:13Z

Yep, does look flaky. Re-ran it and it passed. The other one should be fixed on the main branch.

dorner

Looks good - thank you!

PhantomSean · 2024-02-28T05:18:42Z

Looks good - thank you!

No problem, thanks for all the good feedback!

github-actions · 2024-03-03T16:21:02Z

@PhantomSean: Your PR 4033 product drives filter by category is part of today's Human Essentials production release: 2024.03.03.
Thank you very much for your contribution!

PhantomSean and others added 12 commits February 1, 2024 21:25

Filter by item category

62c26b2

Merge branch 'rubyforgood:main' into 4033-Product-Drives-Filter-By-Ca…

b6c8626

…tegory

Adjust totals based on category filter

ef210e2

Merge branch 'rubyforgood:main' into 4033-Product-Drives-Filter-By-Ca…

af3dbb7

…tegory

Fixing logic for updating quantities when filtering

7f990d6

updating function params

643632c

tests

f6ea0d2

cleaning up

7871743

cleaning up

7555f93

Fixing test

17612bc

Fixing test

6d8b04c

Fixing test

9e9e597

dorner requested changes Feb 13, 2024

View reviewed changes

PhantomSean and others added 4 commits February 22, 2024 16:56

Merge branch 'main' into 4033-Product-Drives-Filter-By-Category

d0ed3d3

Merge branch 'rubyforgood:main' into 4033-Product-Drives-Filter-By-Ca…

8386759

…tegory

Implementing changes suggested in review

3b80d7f

Lint errors

1970570

dorner requested changes Feb 25, 2024

View reviewed changes

Cleaning up query

01e0e93

Merge branch 'main' into 4033-Product-Drives-Filter-By-Category

0441b85

dorner approved these changes Feb 27, 2024

View reviewed changes

dorner merged commit 03dc9cc into rubyforgood:main Feb 27, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4033 product drives filter by category #4094

4033 product drives filter by category #4094

PhantomSean commented Feb 11, 2024

dorner Feb 13, 2024

PhantomSean Feb 25, 2024

dorner Feb 13, 2024

PhantomSean Feb 25, 2024

dorner Feb 13, 2024

dorner Feb 13, 2024

PhantomSean Feb 25, 2024

dorner Feb 13, 2024

dorner Feb 25, 2024

dorner Feb 13, 2024

PhantomSean Feb 25, 2024

dorner left a comment

dorner Feb 25, 2024

PhantomSean commented Feb 27, 2024

PhantomSean commented Feb 27, 2024

dorner commented Feb 27, 2024

dorner left a comment

PhantomSean commented Feb 28, 2024

github-actions bot commented Mar 3, 2024

4033 product drives filter by category #4094

4033 product drives filter by category #4094

Conversation

PhantomSean commented Feb 11, 2024

Description

Type of change

How Has This Been Tested?

Screenshots

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

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhantomSean commented Feb 27, 2024

PhantomSean commented Feb 27, 2024

dorner commented Feb 27, 2024

dorner left a comment

Choose a reason for hiding this comment

PhantomSean commented Feb 28, 2024

github-actions bot commented Mar 3, 2024