[$500] IOU - The selected category moves to the top of the list when <8 categories active

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.43-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4327825
Issue reported by: Applause - Internal Team

Action Performed:

Preconditions:
In OldDot under admin, create a Collect group policy, enable categories and add 3 categories, add an employee to the policy. Setup OldDot Collect Policy:
https://sites.google.com/applausemail.com/applause-expensifyproject/wiki-guides/newdot-categories?authuser=0

1. Open https://staging.new.expensify.com/
2. Log in to the employee's account
3. Tap on the green plus button (FAB)
4. Select Request money
5. Select any currency and amount
6. Click next
7. Choose a group policy room
8. Click on "Show more"
9. In the category section, select the category at the bottom of the list
10. Navigate to the categories section again

Expected Result:

The selected category should remain in its place in alphabetical order if <8 categories are active

Actual Result:

The selected category moves to the top of the list when <8 categories active

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6385731_1708436028448.Recording__1359.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014ddec0e527151c87
• Upwork Job ID: 1759967177619148800
• Last Price Increase: 2024-02-20

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014ddec0e527151c87

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #wave6-collect-submitters
CC @greg-schroeder

[tgolen]

@lukemorawski It appears this could be related to your PR #35567. Can you confirm?

I'm going to demote this issue from being a deploy blocker to being a normal bug.

cc @cristipaval

[barros001]

Can't seem to reproduce it on latest master.

[lukemorawski]

@tgolen I can confirm that this is still reproducible on latest main but I don't see how my PR would cause that behaviour as getFilteredOptions params where untouched.

[barros001]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When category list has less than 8 items and you select a category, the selected category will move to the top of the list

What is the root cause of that problem?

It's a regression from #35922

const enabledAndSelectedCategories = [...selectedOptions, ...sortedCategories.filter((category) => category.enabled && !selectedOptionNames.includes(category.name))];

This will always move the selected options to the top of the list regardless of the site of the list.

What changes do you think we should make in order to solve the problem?

To solve this issue we need to go back to what we had before but we need to make sure to enable any disbaled categories so that they can be de-selected. To do that I propose we iterate through each selected option, find it's index in the enabledAndSelectedCategoriesIndex and then splice it to replace the category in that index with the object from selectedOptions, which will be enabled.

ALSO, and this is important, if we cannot find a match, we unshift that selected option into the array. That will allow us to still list DELETED categories so that they can be unselected. It would be at the top of the list though, which might not be ideal here.

With the original code any deleted category would not be on the list (but it would if more than 8 categories are present).

Here's the proposed code change:

    const enabledAndSelectedCategories = sortedCategories.filter((category) => category.enabled || selectedOptionNames.includes(category.name));
    selectedOptions.forEach((selectedOption) => {
        const enabledAndSelectedCategoriesIndex = enabledAndSelectedCategories.findIndex((category) => category.name === selectedOption.name);

        if (enabledAndSelectedCategoriesIndex === -1) {
            enabledAndSelectedCategories.unshift(selectedOption);
        } else {
            enabledAndSelectedCategories.splice(enabledAndSelectedCategoriesIndex, 1, selectedOption);
        }
    });

This could be extracted into a helper method for better readability.

What alternative solutions did you explore? (Optional)

Another options is to leave the code the way it currently is but then call sortCategories to re-sort the array. This would solve the problem and also keep deleted categories in the correct position. sortCategories does NOT take an array as input so we need to normalize that array back into an object before calling sortCategories.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[aimane-chnaif]

This should be handled by folks involved in offending PR
cc: @tienifr @DylanDylann @Julesssss

[tgolen]

@tienifr I'm going to assign to you for now.

[tgolen]

@tienifr would you mind commenting on this issue so I can then assign it to you?

[DylanDylann]

@tgolen This is intentional in here

Also a bonus is that the selected category now will come as the first item in the list, consistent with the a-lot-of-categories UX.

I think we need to confirm the expectations first. @Expensify/design Could you guys check this issue and verify if we should fix it? Note that: In many other places, the selected option is in the first position on the list

cc @tienifr

[dubielzyk-expensify]

In my mind the category list shouldn't reorder based on selection but keen to hear what @dannymcclain and @shawnborton thinks. I don't feel too strongly though

[shawnborton]

Hmm I could be wrong here, but I think if the list options are less than say 9, we never reoder the list even after one is selected. But if the list is super long, the current selected list item moves to the top and we get a UI that looks like this:

cc @trjExpensify - thoughts on that?

[tienifr]

Hmm I could be wrong here, but I think if the list options are less than say 9, we never reoder the list even after one is selected. But if the list is super long, the current selected list item moves to the top and we get a UI that looks like this:

@shawnborton @dubielzyk-expensify IMO we should have consistent behavior in both cases, otherwise the user might be confused when seeing the selected item jumps around when enabling/disabling categories. I don't see any downside with having the selected item on top consistently in both cases.

[aimane-chnaif]

We already discussed expected behavior in #34018
Also in slack

[dannymcclain]

Hmm I could be wrong here, but I think if the list options are less than say 9, we never reoder the list even after one is selected. But if the list is super long, the current selected list item moves to the top and we get a UI that looks like this:

This was how I thought it worked as well. But seeing this issue pop up made me question why we have a bunch of conditional logic (ie - added complexity) for lists at all. I mean you know me, I'm a set it and forget it kinda guy! 😂

IMO we should have consistent behavior in both cases... I don't see any downside with having the selected item on top consistently in both cases.

Now I think I'm trending toward agreeing with this perspective.

[dannymcclain]

Oh @aimane-chnaif thanks for posting that. Everyone feel free to ignore me and just do what the team has already decided on.

[trjExpensify]

@trjExpensify - thoughts on that?

Yep, agreed. With 8 or less there's no "recent" section or search input and therefore the selected category doesn't sit above that section in the list - it remains in place. For comparison with what you posted above:

[DylanDylann]

@Julesssss While implementing PR to fix this issue, we faced a new bug as mentioned here. It is caused by the position of the selected option change when we increase the number of categories from 7 to 8.
Note that, if we always keep the selected option at the first of the list, this bug won't happen.

cc @trjExpensify

[DylanDylann]

Currently, I see 2 options:

1. We will always display the selected option at the first of the list. Then we don't need to do anymore and this bug also doesn't happen
2. We still keep the current design and will create a new bug to fix new above bug

[Julesssss]

Note that, if we always keep the selected option at the first of the list, this bug won't happen

Choosing behavior to avoid bugs isn't a good reason. but I'm also a bit unsure where we aligned based on the above comments. @Expensify/design & @trjExpensify, did we align on this?

[trjExpensify]

The behaviour came from the categories doc in wave6. Latest ref here.

[Julesssss]

Thanks. So keeping the option in place aligns with 2 here:

1. We will always display the selected option at the first of the list. Then we don't need to do anymore and this bug also doesn't happen
2. We still keep the current design and will create a new bug to fix new above bug

[DylanDylann]

Yes, I am reviewing this PR

[tgolen]

FWIW I like always displaying the selected option at the first of the list (option 1).

• It's a consistent UX in all cases
• It makes the code less complex (which helps with technical debt, maintenance, and yes, preventing bugs)

I feel that having the UX change based on the number of items in the list just feels like we are getting more clever than we need to. Let's go with the most simple solution first, and if we get significant customer feedback that this isn't ideal, we can change it later.

[DylanDylann]

@Expensify/design Could you help to check this comment and help to give your viewpoint? (should go with 1 or 2 in here)

[dannymcclain]

Personally I tend to agree with Tim. But I think this behavior is something that has been talked about widely and decided upon by the larger team. I'm hesitant to let my opinion override a widely agreed upon and documented behavior. 🤷‍♂️

[shawnborton]

I don't feel too strongly. I think the idea behind not changing the order on the smaller amounts of list items (less than 9) was that you can see all of the items in your viewport, and thus it might feel weird if you have the list jumping when you can see the entire screen at once?

That being said, definitely down to go with consistency here.

[DylanDylann]

@tgolen @Julesssss What is the final confirmation?

[Julesssss]

Alright, thanks everyone. So we all agree on the consistent component behaviour for now... which is this option:

We will always display the selected option at the first of the list. Then we don't need to do anymore and #36912 (comment) also doesn't happen

@DylanDylann you may want hold for a few hours just in case of any final objections

[DylanDylann]

@Julesssss If there aren't any objections, let's close this issue. @tienifr Also close this PR

[Julesssss]

Closing based on this comment