Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filtering on distribution -- specifically by item category -- the totals are not filtering properly (Judi, stakeholder 20240808) #4590

Closed
2 tasks
cielf opened this issue Aug 11, 2024 · 17 comments · Fixed by #4806
Assignees

Comments

@cielf
Copy link
Collaborator

cielf commented Aug 11, 2024

Summary

In the Distribution index, for the "Total in Category" column that appears when we filter by Item Category, the totals at the bottom of the page should show the total of the numbers in that column, and they don't.

Why fix?

It's confusing.

Details

Recreation

Sign in as org_admin@example.com. Choose "Distribution" from the left hand menu.. Then, filter the list by item category (fill in "Fiter by item category", then click "Filter")
You will see that the total of the "Total in Category" column does not match the total of the numbers in the column. It should.

Criteria for completion

  • totals in the "Total in Category Diapers" column match the total of the column. (page totals and grand totals)
  • tests to confirm this behaviour
@cielf cielf added Help Wanted Groomed + open to all! Difficulty—Beginner labels Aug 11, 2024
@rlew421
Copy link

rlew421 commented Aug 16, 2024

Hi @cielf, I'm new to this project and am looking for my first ticket. Can you assign this to me?

@github-actions github-actions bot removed the Help Wanted Groomed + open to all! label Aug 16, 2024
@cielf
Copy link
Collaborator Author

cielf commented Aug 16, 2024

Sure can! (Welcome aboard)

@rlew421
Copy link

rlew421 commented Aug 16, 2024

Thank you!

@rlew421
Copy link

rlew421 commented Aug 29, 2024

I was able to diagnose the source of the issue, but I have questions about the best way to change it.

After filtering based on an item category, the totals aren't calculating correctly at the bottom of the page because the method @selected_item = filter_params[:by_item_id].presence on line 54 of distributions_controller.rb is selecting based on item_id instead of item_category_id. When you select a category and check the filter_params in a pry session, item_id is an empty string while item_category_id contains the id we want to filter on. When you try to select a category and then select an item within that category (category - Category Diapers, item - Adult Cloth Diapers Small/Medium) and check filter_params again, you can see that it now contains the item_category_id and the item_id. I think @selected_item needs to select based on item_category_id instead of item_id, but I run into two issues when I do this.

  1. When I choose a category from the category dropdown, the item dropdown auto-populates to the second item on the list regardless of whether that item type actually belongs to the category selected. The item dropdown menu should not auto-populate to anything. Because this item dropdown auto-populates, this means the user needs to clear this dropdown box before filtering by category each time, and it's inconvenient to have to do that. This may necessitate writing another issue, but I think that once a category is selected, the available items in the item dropdown menu should be limited to items that are part of that category. Should I resolve the auto-populate on the item dropdown list in this ticket? Should I proceed to make the fix (set @selected_item based on item_category_id instead of item_id) and then write another issue to limit the items in the dropdown list?

  2. The totals still don't appear to be calculating correctly, but I think it's due to the seed data.

Before fix:
Screenshot 2024-08-28 at 10 57 19 PM

After fix:
Screenshot 2024-08-28 at 10 56 36 PM

It appears that the line query = query.where(item_id: item.to_i) if item in distributions_controller.rb isn't capturing all the distributions it should capture. When I looked at the item_id field for the distribution line_items in pry, many of them have an item_id that doesn't match any of the item_category_id's which can only be 1, 2, or 3. In order for the totals to calculate correctly, I think the line_items in the seed data need to be restricted to having item_id's that match up with the item_category_id's. Could I get another set of eyes to to check if my theory is accurate?

@cielf
Copy link
Collaborator Author

cielf commented Aug 30, 2024

Hey @rlew421 -- Thanks for reaching out! I think you might be tweaking the wrong place.

@selected_item is, specifically, the item that is selected in the drop-down. It gets fed back into the screen as such (see app/views/distributions/index.html, line 41)

So, putting the item_category in there instead of the item is going to have "unexpected results" g, such as what you've described.

I suspect the best place to try to address this would be moving @selected_item_category up to just below @selected_item, then reworking total_items so that it can know about item_categories as well as items.

Bonus:
Here's a rough sketch of the relationships in the db... (Note that line items are not always connected to distributions, which isn't shown in the sketch)

IMG_0599

Copy link
Contributor

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

@github-actions github-actions bot added the stale label Sep 30, 2024
Copy link
Contributor

github-actions bot commented Oct 7, 2024

Automatically unassigned after 7 days of inactivity.

@cielf cielf added Help Wanted Groomed + open to all! and removed stale labels Oct 7, 2024
@inane-pixel
Copy link
Contributor

Can I try this one?

@cielf
Copy link
Collaborator Author

cielf commented Oct 11, 2024

Give it a go! Though there 's an argument for waiting until we've got the other thing merged, depending on the state of your main.

@inane-pixel
Copy link
Contributor

4719

Copy link
Contributor

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

Copy link
Contributor

Automatically unassigned after 7 days of inactivity.

@cielf
Copy link
Collaborator Author

cielf commented Nov 20, 2024

@inane-pixel has indicated that they don't know when they can finish up the PR. It needs some work on tests, should someone want to pick it up.

@cielf cielf added Help Wanted Groomed + open to all! and removed stale labels Nov 20, 2024
@coalest
Copy link
Collaborator

coalest commented Nov 28, 2024

Can I take over on this?

@cielf
Copy link
Collaborator Author

cielf commented Nov 28, 2024

Please do.

@cielf
Copy link
Collaborator Author

cielf commented Nov 28, 2024

@coalest Also -- I think we sent you an invitation that, if accepted, would allow you to self-assign. Nov 17.

@coalest
Copy link
Collaborator

coalest commented Nov 28, 2024

@coalest Also -- I think we sent you an invitation that, if accepted, would allow you to self-assign. Nov 17.

@cielf Ah, I accepted that invitation, but I wasn't sure what those access privileges could be used for. Good to know!

dorner pushed a commit that referenced this issue Dec 10, 2024
…ce (#4590) (#4806)

* Improve distribution filtering and refactor for performance (#4590)

* Minor refactor

* Refactor with service object

* Return total distribion value not value of filtered items

* Fix default date range bug

* Fix inactive_items N+1

* Fix regression in item category filter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment