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

Category - Selected item is no longer selected after renaming the category #54486

Closed
2 of 8 tasks
lanitochka17 opened this issue Dec 23, 2024 · 11 comments · Fixed by #54583
Closed
2 of 8 tasks

Category - Selected item is no longer selected after renaming the category #54486

lanitochka17 opened this issue Dec 23, 2024 · 11 comments · Fixed by #54583
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 23, 2024

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: 9.0.78-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): applausetester+909608@applause.expensifail.com
Issue reported by: Applause - Internal Team
Component Workspace Settings

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Categories
  3. Click Select all checkbox
  4. Click on any selected category
  5. Click Name
  6. Edit the name and save it
  7. Note that the renamed category is now unselected
  8. Go to Taxes
  9. Click Select all checkbox
  10. Click on the selected rate
  11. Click Name
  12. Rename and save it
  13. Note that the renamed tag remains selected

Expected Result:

There should be consistency in the item selection after renaming category and tax rate while they are selected

Actual Result:

In Step 7, after renaming the selected category, it is unselected
In Step 9, after renaming the selected tag, it remains selected

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6701534_1734996446867.20241224_072205.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Dec 23, 2024
Copy link

melvin-bot bot commented Dec 23, 2024

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Dec 23, 2024

💬 A slack conversation has been started in #expensify-open-source

Copy link
Contributor

👋 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.

@BhuvaneshPatil
Copy link
Contributor

Proposal

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

When we select category, and edit the name. Category is not selected anymore.

What is the root cause of that problem?

We use name as key for list and to check if that item is selected using the name. So when we update the name of category, key changes, thus the category is un-selected.

return {
text: value.name,
keyForList: value.name,
isSelected: !!selectedCategories[value.name] && canSelectMultiple,
isDisabled,
pendingAction: value.pendingAction,
errors: value.errors ?? undefined,
rightElement: <ListItemRightCaretWithLabel labelText={value.enabled ? translate('workspace.common.enabled') : translate('workspace.common.disabled')} />,

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

We should have an id field for category that can be used for key. This field is non-changing and hence it will work.
Similar thing is done on tax rates page.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Daily KSv2 label Dec 24, 2024
@truph01
Copy link
Contributor

truph01 commented Dec 24, 2024

Edited by proposal-police: This proposal was edited at 2024-12-24 02:38:54 UTC.

Proposal

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

  • In Step 7, after renaming the selected category, it is unselected
  • In Step 9, after renaming the selected tag, it remains selected

What is the root cause of that problem?

  • Categories are identified solely by their names. When editing a category, we effectively delete the old category and create a new one:

[policyCategory.oldName]: null,

This results in a scenario where, when a user selects category A and subsequently updates it to B, the workspace A is no longer marked as selected.

  • This behavior is expected because selecting A indicates a desire to use A specifically. Updating it to B implies the removal of A and the creation of a new category, B. Consequently, marking B as selected is unnecessary in this case.

  • The only issue is that, the dropdown button still counting A as selected, which leads to an inaccurate display of the number of selected options. We need to address it.

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

  • We can create a hook which will auto-update the selected options once the options change:
import {useEffect} from 'react';
import type {PolicyCategories} from '@src/types/onyx';

const useAutoUpdateSelectedOptions = (
    listItem: PolicyCategories,
    selectedOptions: Record<string, boolean>,
    setSelectedOptions: React.Dispatch<React.SetStateAction<Record<string, boolean>>>,
) => {
    useEffect(() => {
        const availableOptions = Object.values(listItem ?? {})
            .filter((o) => o.pendingAction !== 'delete')
            .map((o) => o.name);
        const originalSelectedOptions = Object.keys(selectedOptions);
        const newSelectedOptions = originalSelectedOptions.filter((o) => availableOptions.includes(o));
        setSelectedOptions(
            newSelectedOptions.reduce((acc, key) => {
                acc[key] = true;
                return acc;
            }, {}),
        );
    }, [listItem]);
};
export default useAutoUpdateSelectedOptions;
  • And use it in here:
    useAutoUpdateSelectedOptions(policyCategories ?? {}, selectedCategories, setSelectedCategories);
  • We can use the hook above on other pages like workspace tax page, workspace tag page, etc.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • We can create a test function that simulate the update of policyCategories data and see whether the number of selected options is updated properly.

What alternative solutions did you explore? (Optional)

@truph01
Copy link
Contributor

truph01 commented Dec 24, 2024

@neil-marcellini The bug mentioned in OP is "when a user selects category A and subsequently updates it to B, the workspace A is no longer marked as selected."

IMO, this behavior is expected because selecting category A reflects the user's intent to use A specifically. When they update it to B, it signifies not just a change in category but the creation of a new category, B, and the removal of A. Marking B as selected would lead to confusion, as it misrepresents the user's intention.

What do you think?

@neil-marcellini
Copy link
Contributor

@truph01 I think the category should stay selected when its name changes. The category object/row is shown as selected in the UI, so it doesn't make sense to me that changing one field would remove the object and create a totally new one.

I like the proposal from @BhuvaneshPatil.

It looks like the issue is not reproducible on production because if you select all categories and then click on one, it clears the selection. After that was fixed with [this PR](fix: when selecting categories, the selected categories get reset)fix: when selecting categories, the selected categories get reset, it uncovered this bug that we didn't see before. I wouldn't say the PR I linked caused this regression and I don't think it should be reverted. Instead let's fix it quickly.

@BhuvaneshPatil
Copy link
Contributor

@neil-marcellini This is what we get in response for categories page
Screenshot 2024-12-25 at 7 58 01 AM
Is it possible to send id?

@neil-marcellini
Copy link
Contributor

Yeah, I think this is going to require a backend change. I tried making it work by adding an id field on the frontend and storing it on the component, but the problem is that when a category is modified, added, or deleted, there's no way to tell how the selectedCategories should be updated. I was thinking that ids could be added when the component mounts, and then if a category is added or deleted the necessary ID updates would get made, but that's problematic because adding and deleting categories happens in another component. There's no way to differentiate between adding a category and modifying one. To make this work on the frontend only it would probably have to be managed with a separate Onyx key, and then that's pretty messy and complicated.

It's also a fairly large amount of work to change this on the backend, so maybe the separate frontend Onyx key is a viable approach. I'll ask for some advice from the team internally.

@neil-marcellini
Copy link
Contributor

CP requested here. I'll close when that's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants