human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

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

Open cielf opened this issue 1 year ago • 10 comments

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 [email protected]. 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 avatar Aug 11 '24 14:08 cielf

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

rlew421 avatar Aug 16 '24 20:08 rlew421

Sure can! (Welcome aboard)

cielf avatar Aug 16 '24 22:08 cielf

Thank you!

rlew421 avatar Aug 16 '24 23:08 rlew421

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?

rlew421 avatar Aug 29 '24 17:08 rlew421

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

cielf avatar Aug 30 '24 14:08 cielf

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[bot] avatar Sep 30 '24 00:09 github-actions[bot]

Automatically unassigned after 7 days of inactivity.

github-actions[bot] avatar Oct 07 '24 00:10 github-actions[bot]

Can I try this one?

inane-pixel avatar Oct 11 '24 22:10 inane-pixel

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.

cielf avatar Oct 11 '24 23:10 cielf

4719

inane-pixel avatar Oct 13 '24 14:10 inane-pixel

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[bot] avatar Nov 13 '24 00:11 github-actions[bot]

Automatically unassigned after 7 days of inactivity.

github-actions[bot] avatar Nov 20 '24 00:11 github-actions[bot]

@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 avatar Nov 20 '24 12:11 cielf

Can I take over on this?

coalest avatar Nov 28 '24 09:11 coalest

Please do.

cielf avatar Nov 28 '24 13:11 cielf

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

cielf avatar Nov 28 '24 13:11 cielf

@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!

coalest avatar Nov 28 '24 14:11 coalest