Fix problem with item name dropdown selection #4797

coalest · 2024-11-21T14:11:33Z

Resolves #4547

Description

This issue occurs when an ajax call to populate the item names in the dropdown options occurs while the dropdown has been opened. Selecting an option doesn't work because that option no longer exists, so nothing happens (so item selected in the field will be the first item from the list of new options).

To fix this I made two changes:

Added an event listener so that whenever a item name dropdown option is selected, if the id of that item matches in the list of new options, select that option. (Note: This is enough to fix the bug)
But during investigation I found that when adding a new item, two ajax calls are fired off (to /inventory.json) every time you click "Add Another Item". And an ajax call is made to fetch the inventory whenever the storage location changes. I'm assuming it's not needed to fetch the inventory so often. So I changed it to only fetch the inventory item names/quantities on storage location field change, then we store those dropdown options and reuse them whenever a new line item is added.

This will make adding new line items more responsive, and with less jank (from the reduced number of ajax calls) (ie. the item name changing from "XL Diapers" to "XL Diapers (1353)").

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested manually in Chrome on my local machine, using the reproduction steps provided here.

Testing has been tough. I see a similar issue to the one I'm having where the inputevent isn't being triggered by cuprite. In my case the select2:select event isn't being triggered for some reason.

dorner · 2024-11-22T01:41:17Z

Looks great on the technical side - @cielf did you want to test this out manually?

coalest · 2024-11-22T10:51:04Z

Actually, I might I have a better way to resolve this issue. Instead of using an the select2:select event listener to detect when an option has been selected and match it to the new option, I think we can might be able change the options in the open dropdown as well. Need to test it out though.

cielf · 2024-11-22T13:45:02Z

I would - but now I'll wait.

…e load if storage location selected

coalest · 2024-11-25T10:20:17Z

So I tried to update the opened dropdown options, but I couldn't find a way to do so using select2's api. I tested out a hacky way (closing and opening the dropdown to refresh), and it kinda works but is a little glitchy if the user is currently scrolling or typing something in dropdown.

In summary, we can fix the bug by matching the options (option 1), or by rerendering the options with some jankiness (option 2).

Here are two videos showing the difference from a user standpoint. I used Chrome and changed the network setting to have 3 seconds of latency.

Video of what option 1 looks like (commit 71698f4):
I click "Kids (Size 6)" and it becomes "Kids (Size 6) (1986)".

Screencast.from.2024-11-24.19-35-28.webm

Video of what option 2 looks like (commit bdd3a6a):
Quantities are added dynamically to the dropdown with a bit of jank.

Screencast.from.2024-11-24.19-37-16.webm

@dorner @cielf Do y'all have a preference from a functional standpoint? (Keep in mind I do think this issue will be less common going forward with the change I made to only fetch inventory quantities on storage location change not when adding a new item. So either way, this should occur less often.)

P.S. Sorry for the force pushing, I had an annoying issue with Rails serving stale JS assets and while debugging it I made some weird commits that I cleaned up.

cielf · 2024-11-25T14:03:37Z

I think it's more smooth for the users to see the number of items available as they are picking them, so option # 2, please.

coalest · 2024-11-26T10:06:19Z

Updated to use the workaround for option 2. This is now ready for functional and technical review

cielf · 2024-11-26T12:51:57Z

LGTM from functional pov

dorner · 2024-11-27T01:40:07Z

@cielf FYI - the changes in this PR basically completely rewrite the distributions/transfers dropdown logic. Might be worth some extra cycles to regression test it before merging.

cielf · 2024-11-27T13:52:27Z

Agreed. Thanks for pointing that out.

coalest · 2024-11-28T13:22:37Z

@dorner If we want to play it safe for production, I can split this into two PRs (one to refresh dropdowns and fix the bug and one with the refactoring).

cielf

Mostly working like a charm -checked transfers, distributions, and inventory adjustments but found one thing that needs to be addressed on the sequence for distributions made from requests, if you have an error.

Here's the sequence:
sign in as [email protected]
click on Requests
click on view
click Fulfill request
Pick a storage location
Make one of the items have a bigger quantity than is allowed (i.e. we are forcing an error)
Save
Yes, it's correct

The quantities for the items disappear.

coalest · 2024-11-29T22:16:43Z

@cielf Good catch. I didn't think about the pages with turbo streams, those aren't triggering the Javascript that is supposed to execute on page load.

The same thing happens when saving a new distribution with errors. (Distributions index => New distribution => Select storage location => Save with an error => Quantities disappear)

Does this work currently in production? I feel like this issue should be occurring on the main branch as well.

cielf · 2024-11-29T23:14:24Z

@coalest Yup -- confirmed It's happening on staging. Do you think you can fix it within the bounds of this PR, or do we need to split it up?

coalest · 2024-11-30T19:00:19Z

@cielf I just pushed a commit on this branch that should fix the issue.

Ideally, I would like to refactor things a little bit. I don't like the current flow on some pages. Currently (like when we get a request to show the page to edit a distribution), we have access to what the dropdown options should be on the backend if there is a storage location selected (item names + quantities). But instead of providing the HTML with those dropdown options included, we send back default options (with missing quantities) and an ajax request gets triggered via javascript to update labels.

Not a big deal, but I think it could be improved. But I think that could be a future PR.

cielf · 2024-11-30T23:00:44Z

@dorner I'm taking this out for a spin, but could you do another technical pass in parallel? Thx.

cielf

LGTM. Thanks!

dorner · 2024-12-01T01:08:14Z

@coalest if we can separate out the behavior changes from the refactor, I'd much rather do that.

dorner · 2024-11-27T01:32:38Z

app/javascript/utils/distributions_and_transfers.js

-  content += "</option>\n";
-  return content;
+function newOption(item, selectedValue, includeQuantity) {
+  const text = includeQuantity ? item.item_name + ` (${item.quantity})` : item.item_name;


The middle clause looks a bit weird - I'd expect to see `${item.item_name} (${item.quantity})`

Very true, not sure what happened there. Will fix.

coalest · 2024-12-01T12:24:54Z

@dorner @cielf Reverted all refactoring out of this PR, leaving only the bug fix. Ready for review 🙏

coalest · 2024-12-01T12:33:16Z

spec/system/distribution_system_spec.rb

@@ -699,6 +699,34 @@
    end
  end

+  context "with dropdown options changed while open", skip: "select2 not working correctly with cuprite" do


Really don't understand why I can't get this system spec to work. These steps work in a browser locally. I notice some weird behavior with select2 though. It seems like those events aren't being triggered in cuprite.

I should probably just delete this?

coalest · 2024-12-01T12:57:53Z

Tests aren't passing due to a flaky test. Here is a PR to fix that: #4816

cielf

Huh. I could have sworn this was working earlier -- but now it isn't, with the case of fulfilling a request, picking a storage location, putting a riduculously high number in a quantity, saving, confirming, Comes back with an error as expected, but the inventory values do not appear.

Same with the case of an error on a new distribution.

coalest · 2024-12-02T19:40:51Z

@cielf Yea, I took that fix out because I thought we were trying to keep each PR small. so this is now just a PR to fix #4547. Not anything else.

I will add that back in.

coalest · 2024-12-02T19:46:25Z

@cielf I put that change back in. So it's ready for review again. Sorry for the confusion.

cielf · 2024-12-02T20:02:54Z

(nods) it's still part of fixing the item dropdown selection, imo. I can see where it might not have been clear where to draw the line, though.

cielf

LGTM from a functional pov

cielf requested a review from dorner November 21, 2024 17:25

coalest changed the title ~~4547: Fix problem with item name dropdown selection~~ WIP: 4547: Fix problem with item name dropdown selection Nov 22, 2024

coalest force-pushed the 4547-item-dropdown-problem-fix branch from e5203c1 to e913553 Compare November 24, 2024 14:24

coalest added 3 commits November 24, 2024 20:22

4547: Fix problem with item dropdown selection

042c4f4

Refresh stale open dropdown options and fetch dropdown options on pag…

bdd3a6a

…e load if storage location selected

Refactor and switch back to previous solution

71698f4

coalest force-pushed the 4547-item-dropdown-problem-fix branch from e858028 to 71698f4 Compare November 24, 2024 19:42

Move guard clause to populateDropdowns function

9456925

coalest changed the title ~~WIP: 4547: Fix problem with item name dropdown selection~~ Fix problem with item name dropdown selection Nov 25, 2024

Rerender open dropdowns during item population

20fee2f

Merge branch 'main' into 4547-item-dropdown-problem-fix

4f5f239

cielf closed this Nov 29, 2024

cielf reopened this Nov 29, 2024

cielf self-requested a review November 29, 2024 14:01

cielf requested changes Nov 29, 2024

View reviewed changes

Fix missing item dropdown options on turbo stream distribution pages

006a687

coalest requested a review from cielf November 30, 2024 20:57

cielf reviewed Nov 30, 2024

View reviewed changes

dorner reviewed Dec 1, 2024

View reviewed changes

Revert dropdown options refactor except rubyforgood#4547 bug fix

94e9931

coalest commented Dec 1, 2024

View reviewed changes

coalest mentioned this pull request Dec 1, 2024

Fix flaky distributions by county spec #4816

Merged

coalest requested review from cielf and dorner December 1, 2024 12:57

Merge branch 'main' into 4547-item-dropdown-problem-fix

ecfa898

cielf requested changes Dec 2, 2024

View reviewed changes

Fix missing item dropdown options on turbo stream distribution pages

21d6ad6

coalest requested a review from cielf December 2, 2024 19:46

cielf reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problem with item name dropdown selection #4797

Fix problem with item name dropdown selection #4797

coalest commented Nov 21, 2024 •

edited

Loading

dorner commented Nov 22, 2024

coalest commented Nov 22, 2024

cielf commented Nov 22, 2024

coalest commented Nov 25, 2024 •

edited

Loading

cielf commented Nov 25, 2024

coalest commented Nov 26, 2024

cielf commented Nov 26, 2024 •

edited

Loading

dorner commented Nov 27, 2024

cielf commented Nov 27, 2024

coalest commented Nov 28, 2024

cielf left a comment •

edited

Loading

coalest commented Nov 29, 2024

cielf commented Nov 29, 2024

coalest commented Nov 30, 2024

cielf commented Nov 30, 2024

cielf left a comment

dorner commented Dec 1, 2024

dorner Nov 27, 2024

coalest Dec 1, 2024

coalest commented Dec 1, 2024

coalest Dec 1, 2024

coalest Dec 1, 2024

coalest commented Dec 1, 2024

cielf left a comment

coalest commented Dec 2, 2024

coalest commented Dec 2, 2024

cielf commented Dec 2, 2024

cielf left a comment

Fix problem with item name dropdown selection #4797

Are you sure you want to change the base?

Fix problem with item name dropdown selection #4797

Conversation

coalest commented Nov 21, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

dorner commented Nov 22, 2024

coalest commented Nov 22, 2024

cielf commented Nov 22, 2024

coalest commented Nov 25, 2024 • edited Loading

cielf commented Nov 25, 2024

coalest commented Nov 26, 2024

cielf commented Nov 26, 2024 • edited Loading

dorner commented Nov 27, 2024

cielf commented Nov 27, 2024

coalest commented Nov 28, 2024

cielf left a comment • edited Loading

Choose a reason for hiding this comment

coalest commented Nov 29, 2024

cielf commented Nov 29, 2024

coalest commented Nov 30, 2024

cielf commented Nov 30, 2024

cielf left a comment

Choose a reason for hiding this comment

dorner commented Dec 1, 2024

dorner Nov 27, 2024

Choose a reason for hiding this comment

coalest Dec 1, 2024

Choose a reason for hiding this comment

coalest commented Dec 1, 2024

coalest Dec 1, 2024

Choose a reason for hiding this comment

coalest Dec 1, 2024

Choose a reason for hiding this comment

coalest commented Dec 1, 2024

cielf left a comment

Choose a reason for hiding this comment

coalest commented Dec 2, 2024

coalest commented Dec 2, 2024

cielf commented Dec 2, 2024

cielf left a comment

Choose a reason for hiding this comment

coalest commented Nov 21, 2024 •

edited

Loading

coalest commented Nov 25, 2024 •

edited

Loading

cielf commented Nov 26, 2024 •

edited

Loading

cielf left a comment •

edited

Loading