[$250] Taxes - "1 selected" dropdown is not dismissed after the selected rate is deleted

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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): [email protected]
Issue reported by: Applause - Internal Team
Component Workspace Settings

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace settings > Taxes
3. Click on the select all checkbox
4. Click on the tax rate which is selected
5. Click Delete
6. Delete the rate
7. Note that "1 selected" dropdown is not dismissed after all the selected tag is deleted
8. Click on the dropdown > Delete
9. Click Delete

Expected Result:

In Step 7, "1 selected" dropdown should be dismissed because there is no more selected rate

Actual Result:

In Step 7, "1 selected" dropdown remains when there is no more selected rate
In Step 9, console error shows up when trying to delete no rate

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

Bug6701530_1734995738401.20241224_064741.mp4

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021871350917212703788
• Upwork Job ID: 1871350917212703788
• Last Price Increase: 2024-12-24
• Automatic offers:
• truph01 | Contributor | 105456786

Issue Owner

Current Issue Owner: @mollfpr

[melvin-bot]

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

[melvin-bot]

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

[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]

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

[melvin-bot]

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

[daledah]

Edited by proposal-police: This proposal was edited at 2024-12-24 00:36:36 UTC.

Proposal

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

In Step 7, "1 selected" dropdown remains when there is no more selected rate
In Step 9, console error shows up when trying to delete no rate

What is the root cause of that problem?

We are not updating selectedTaxesIDs when deleting taxes in taxes detail page

App/src/pages/workspace/taxes/WorkspaceTaxesPage.tsx

Lines 49 to 54 in a088878

	function WorkspaceTaxesPage({
	policy,
	route: {
	params: {policyID},
	},
	}: WorkspaceTaxesPageProps) {

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

We should clear seletedTaxesIDs when policy?.taxRates?.taxes is changed in this component

    useEffect(() => {
        if(!policy?.taxRates?.taxes){
            return;
        }
        setSelectedTaxesIDs([]);
    }, [policy?.taxRates?.taxes])

Notes: this bug also occurs in other pages, we should fix this too in this issue

OR: We can check that seletedTaxesIDs does not have any tax existing in policy?.taxRates?.taxes then set seletedTaxesIDs to empty

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

What alternative solutions did you explore? (Optional)

Or we can use the function turnOnSelectionMode here to turn off selection mode

App/src/components/SelectionListWithModal/index.tsx

Lines 94 to 101 in a088878

	const turnOnSelectionMode = () => {
	turnOnMobileSelectionMode();
	setIsModalVisible(false);
	if (onTurnOnSelectionMode) {
	onTurnOnSelectionMode(longPressedItem);
	}
	};

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.

[github-actions]

denkoy Your proposal will be dismissed because you did not follow the proposal template.

[Krishna2323]

Proposal

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

Taxes - "1 selected" dropdown is not dismissed after the selected rate is deleted

What is the root cause of that problem?

• WorkspaceTaxesPage along with few other components uses useCleanupSelectedOptions hook to clear the selected options when the app is navigated away from RightModal. But we haven't configured the hook to also clear the selection when the data changes.
  

  App/src/pages/workspace/taxes/WorkspaceTaxesPage.tsx

  Lines 90 to 91 in 8b7096f

  	const cleanupSelectedOption = useCallback(() => setSelectedTaxesIDs([]), []);
  	useCleanupSelectedOptions(cleanupSelectedOption);

  
  

  App/src/hooks/useCleanupSelectedOptions/index.ts

  Lines 1 to 21 in 8b7096f

  	import {NavigationContainerRefContext, useIsFocused} from '@react-navigation/native';
  	import {useContext, useEffect} from 'react';
  	import NAVIGATORS from '@src/NAVIGATORS';
  	const useCleanupSelectedOptions = (cleanupFunction?: () => void) => {
  	const navigationContainerRef = useContext(NavigationContainerRefContext);
  	const state = navigationContainerRef?.getState();
  	const lastRoute = state?.routes.at(-1);
  	const isRightModalOpening = lastRoute?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;
  	const isFocused = useIsFocused();
  	useEffect(() => {
  	if (isFocused || isRightModalOpening) {
  	return;
  	}
  	cleanupFunction?.();
  	}, [isFocused, cleanupFunction, isRightModalOpening]);
  	};
  	export default useCleanupSelectedOptions;

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

• We should add an additional param originalListLength to useCleanupSelectedOptions and inside the hook we should save the previous length value using usePrevious and when the new list length is different from prev list length, we will call the cleanupFunction.

const useCleanupSelectedOptions = (cleanupFunction?: () => void, originalListLength?: number) => {
    const navigationContainerRef = useContext(NavigationContainerRefContext);
    const state = navigationContainerRef?.getState();
    const lastRoute = state?.routes.at(-1);
    const isRightModalOpening = lastRoute?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;
    const prevListLength = usePrevious(originalListLength);
    const isFocused = useIsFocused();

    useEffect(() => {
        if ((isFocused || isRightModalOpening) && prevListLength === originalListLength) {
            return;
        }
        cleanupFunction?.();
    }, [isFocused, cleanupFunction, isRightModalOpening, originalListLength, prevListLength]);
};

• From all components which uses this hook, we will simply pass the list length, like in WorkspaceTaxesPage we will pass Object.keys(policy?.taxRates?.taxes ?? {}).length.

    const cleanupSelectedOption = useCallback(() => setSelectedTaxesIDs([]), []);
    useCleanupSelectedOptions(cleanupSelectedOption, Object.keys(policy?.taxRates?.taxes ?? {}).length);

• Additionally, we can apply the new hook to all other pages which does not use that yet, like WorkspaceMemberPage, PolicyDistanceRatesPage, and few others.

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


What alternative solutions did you explore? (Optional)

[github-actions]

lopezjorge Your proposal will be dismissed because you did not follow the proposal template.

[ikevin127]

Proposal

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

"1 selected" dropdown is not dismissed after the selected rate is deleted from RHP.

What is the root cause of that problem?

In WorkspaceTaxesPage we have logic that clears the select state on deletion using setSelectedTaxesIDs([]) here and we also have the useCleanupSelectedOptions hook implementation which also clears select state in specific case (see issue #49322).

The issue is that when we delete individually selected tax rate by clicking on one rate, this opens RHP WorkspaceEditTaxPage component which has it's own deleteTaxRate function here which has no logic to reset the select state in the WorkspaceTaxesPage component when deletion is performed.

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

PR #53519 introduced a new hook useCleanupSelectedOptions which is used to reset select state in specific scenarios (see issue #49322 for more details), we can adjust the hook's logic to fix our issue's case.

In WorkspaceTaxesPage pass the length of the current tax rate options to the useCleanupSelectedOptions hook:

useCleanupSelectedOptions(cleanupSelectedOption, Object.keys(policy?.taxRates?.taxes ?? {}).length);

modify the useCleanupSelectedOptions logic as follows:

...
import usePrevious from '@hooks/usePrevious'; // <- add import

const useCleanupSelectedOptions = (cleanupFunction?: () => void, initialSelectedLength?: number) => { // <- pass 2nd argument / type
   // ...
    const previousSelectedLength = usePrevious(initialSelectedLength);
    const hasDeletedOption = !!initialSelectedLength && previousSelectedLength !== initialSelectedLength;

   // ...

    useEffect(() => {
        if ((isFocused || isRightModalOpening) && !hasDeletedOption) { // <- add the new variable check, wrapping the first two
            return;
        }
        cleanupFunction?.();
    }, [isFocused, cleanupFunction, isRightModalOpening, hasDeletedOption]);
};

Explanation

Change (1.) passes the argument needed to implement the fix logic.

Changes (2.) are using the new passed argument (if existent) and ensures the cleanupFunction() is called in the case where one of the options was deleted (including from RHP WorkspaceEditTaxPage). This change also ensures that if the 2nd argument is not passed, we're maintaining original useCleanupSelectedOptions behaviour.

Result video (before / after)

MacOS: Chrome

Before	After
before.mov	after.mov

[bernhardoj]

Proposal

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

The "selected" button doesn't disappear when the selected item is deleted.

What is the root cause of that problem?

This happens after #53519. Before the PR, when we select a tax (or category, etc.) and then press the item to open the RHP settings, the selection will be removed. But this was changed on the PR to make sure they are persisted. However, currently, the pages (tax, category, etc.) don't have a logic to update the selected item when the item is removed outside from the page list itself, in this case from the individual tax RHP settings. This issue could also happen if we delete the item from another client. So, this isn't a regression but an existing lack of functionality on those pages.

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

We can have a new effect that updates the selected item whenever the list is updated.

For example, on the categories page, we can do it like this:

useEffect(() => {
    if (isEmptyObject(selectedCategories)) {
        return;
    }

    setSelectedCategories((prevSelectedCategories) => {
        const keys = Object.keys(prevSelectedCategories);
        const newSelectedCategories: Record<string, boolean> = {};

        for (const key of keys) {
            if (policyCategories?.[key] && policyCategories[key].pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
                newSelectedCategories[key] = prevSelectedCategories[key];
            }
        }

        return newSelectedCategories;
    });
}, [policyCategories]);

It iterates over the selected item, and checks if the item still exists in the policy categories. If it still exists, then we keep it on the selected items.

We need to apply this to tax, tag, and category pages.

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

We can render each of the pages, perform a press event to select one item, and then manually merge the onyx to remove the item from the onyx. Then, verify if the "n selected" button still shows or not.

[truph01]

This issue is unrelated to my PR. It is a known problem that I previously reported here.

I’ve already provided a general solution for it in the same thread here and later shared a more detailed proposal here.

@mollfpr, could you please review my proposal? Thanks! You can check the timestamps of my proposal to confirm the chronological order of my proposal.

cc @MonilBhavsar Since you are internal of the original issue.

[mollfpr]

Reviewing 👀

@truph01 Please post your proposal here and I'll review it like everyone else.

[truph01]

Re-post the proposal

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:

App/src/libs/actions/Policy/Category.ts

Line 589 in 8b7096f

[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)

[mollfpr]

Thank you guys for the proposal!

@daledah @Krishna2323 @ikevin127 Three of you reset the selected item on every change to the list items, while it should be only to remove the deleted item from the selection and keep the remaining selection.

@bernhardoj @truph01 Your solution has the correct result, but thinking the effect is the same for other components and the use case is pretty much the same, I think it's better we create a hook for it.

So I will go with the proposal from @truph01.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

Current assignee @robertjchen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[robertjchen]

Thanks for the proposals and review- agreed with moving forward with @truph01 's proposal 👍

[melvin-bot]

📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[truph01]

@mollfpr PR is ready