[HOLD for payment 2024-11-01] [$250] Search - Selection persists after the expense is deleted and no expense is selected

[IuliiaHerets]

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.42-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Submit an expense to any user.
3. Go to Search.
4. Select the submitted expense via checkbox.
5. Go back to Inbox.
6. Delete the expense in Step 2.
7. Go to Search.

Expected Result:

The selection should reset because the expense is deleted.

Actual Result:

The dropdown button remains as "1 selected" when the expense is deleted and no expense is selected.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6621388_1727806755633.bandicam_2024-10-02_02-13-52-036.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021842230940180729212
• Upwork Job ID: 1842230940180729212
• Last Price Increase: 2024-10-11
• Automatic offers:
• Krishna2323 | Contributor | 104426385

Issue Owner

Current Issue Owner: @OfstadC

[IuliiaHerets]

We think that this bug might be related to #wave-control

[melvin-bot]

Triggered auto assignment to @OfstadC (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@OfstadC FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-10-11 22:25:51 UTC.

Proposal

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

The selection header is not updated when the search data is changed.

What is the root cause of that problem?

We do not update the selected header text when the search data changes.

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

• in the search page here, we should add a useEffect hook that recalculates the selected text whenever the data changes

const data = searchResults ? SearchUtils.getSections(type, status, searchResults?.data, searchResults?.search) : [];

useEffect(() => {
    // Check if there are any selected transactions before proceeding
    if (!Object.keys(selectedTransactions).length) {
        return; // Exit if no selected transactions
    }

    if (!data || data.length ===0) {
        // Clear selected transactions if there’s no data
        clearSelectedTransactions();
    } else {
        // Create a new object to store the updated transactions
        const updatedTransactions: SelectedTransactions = {};

        // Loop over selected transactions and validate if they exist in the current data
        Object.keys(selectedTransactions).forEach((key) => {
            const transactionExists = data.some((item) => {
                if (SearchUtils.isTransactionListItemType(item)) {
                    return item.keyForList === key;
                } else if (!SearchUtils.isReportActionListItemType(item)) {
                    return item?.transactions?.some((transaction) => transaction.keyForList === key);
                }
                return false;
            });

            // If the transaction exists, keep it in updatedTransactions
            if (transactionExists) {
                updatedTransactions[key] = selectedTransactions[key];
            }
        });

        // Update the context with the new selected transactions and reports
        setSelectedTransactions(updatedTransactions, data);
    }
}, [data]);

• Alternatively, instead of recalculating, we could reset the selected transactions whenever the data changes by using clearSelectedTransactions.
• Now handling the second part of the issue every time the user leaves the Search tab the selected items should be reset, to handle this issue we could clearSelectedTransactions when navigating to the search page here and here.
• However, to make it cleaner, we can create a util function buildQueryAndNavigate in the SearchUtils file that would build the query, clear the transactions, navigate to this query, and then use this function above.
• This way we covered the cases where the data change, in this case, we have to reset/recalculate the selected transaction because it will be outdated, and we also covered the cases where the user navigates to the search page from a page that is not related to the search, in those cases it will be cleared

What alternative solutions did you explore? (Optional)

[Krishna2323]

Proposal


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

Search - Selection persists after the expense is deleted and no expense is selected

What is the root cause of that problem?

In SearchPage we don't have a effect to clear the selected transactions and to turn off the mobile selection mode when the component unmounts.

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


• We should clear the selected transactions and turn off the mobile selection mode when the component unmounts.

    useEffect(
        () => () => {
            clearSelectedTransactions();
            turnOffMobileSelectionMode();
        },
        [isFocused, clearSelectedTransactions],
    );

Optional: We can call urnOffMobileSelectionMode(); after checking selectionMode?.isEnabled

What alternative solutions did you explore? (Optional)

• We can use navigation.addListener('beforeRemove').
• With or without the main solution we can also do the same when component mounts.

What alternative solutions did you explore? (Optional 2)

• If we want to keep the selection when user stays on the search page then we can add a isSearchTopmostCentralPane() check before removing the selected transactions.

    useEffect(
        () => () => {
            if (isSearchTopmostCentralPane()) {
                return;
            }
            clearSelectedTransactions();
            turnOffMobileSelectionMode();
        },
        [isFocused, clearSelectedTransactions],
    );

Result

Monosnap.screencast.2024-10-13.19-30-26.mp4

[huult]

Proposal

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

Selection persists after the expense is deleted and no expense is selected

What is the root cause of that problem?

We do not update selectedTransactionsData when deleting one or multiple expenses.

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

We should update selectedTransactionsData when deleting an expense to keep it up to date, something like this:

// add function to update `selectedTransactionsData`
// .src/components/Search/SearchContext.tsx#L56
+    const updateSelectedTransactions = useCallback((selectedTransactions: SelectedTransactions) => {
+        setSearchContextData((prevState) => ({
+            ...prevState,
+            selectedTransactions,
+        }));
+    }, []);

// .src/components/Search/SearchContext.tsx#L56
 const searchContext = useMemo<SearchContext>(
        () => ({
        ...
+     updateSelectedTransactions

// Update function delete expense
// .src/pages/ReportDetailsPage.tsx#L718
+  const {selectedTransactions, updateSelectedTransactions, clearSelectedTransactions} = useSearchContext();

// .src/pages/ReportDetailsPage.tsx#L734
const deleteTransaction = useCallback(() => {
...
+        const updateSelectedTransactionsData = Object.keys(selectedTransactions)
+                .filter((key) => String(key) !== String(iouTransactionID)) // Ensure both are strings for comparison
+                .reduce((obj, key) => {
+                    obj[key] = selectedTransactions[key];
+                    return obj;
+                }, {});

+            if (!Object.keys(updateSelectedTransactionsData).length) {
+                clearSelectedTransactions();
+            } else {
+               updateSelectedTransactions(updateSelectedTransactionsData);
+            }
       navigateBackToAfterDelete.current = IOU.deleteMoneyRequest(iouTransactionID, requestParentReportAction, 
       isSingleTransactionView);

We will create this logic to handle updating selectedTransactions in a common way and find where an expense is deleted to call it, so we can update other components like MoneyReportHeader.

Note: We can optimize updateSelectedTransactions by passing iouTransactionID as a parameter to handle the update logic, and the component can simply call it and send the parameters. For example:

    // ReportDetailsPage.tsx or MoneyReportHeader.tsx
    updateSelectedTransactions(iouTransactionID);
    // SearchContext.tsx
    const updateSelectedTransactions = useCallback((iouTransactionID: string) => {
        const updateSelectedTransactionsData = Object.keys(searchContextData.selectedTransactions)
            .filter((key) => String(key) !== String(iouTransactionID)) // Ensure both are strings for comparison
            .reduce((obj, key) => {
                obj[key] = searchContext.selectedTransactions[key];
                return obj;
            }, {});

        setSearchContextData((prevState) => ({
            ...prevState,
            selectedTransactions: updateSelectedTransactionsData,
        }));
    }, []);

POC

Screen.Recording.2024-10-02.at.17.40.26.mp4

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@Ollyws, @OfstadC Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Ollyws]

Will get to this one today/tomorrow

[Ollyws]

@abzokhattabs proposal LGTM, but I think the alternative proposal of using clearSelectedTransactions would be a bit cleaner.
🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @MarioExpensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Krishna2323]

@Ollyws, I believe we should just clear the selection as soon as we leave the search page. This is the behaviour accross the app. I'm not sure why do we need to persist the selection after leaving the page, am I missing something?

[MarioExpensify]

@OfstadC Can you give us your input regarding @Krishna2323's comment? I'm not sure what would be our preferred behavior regarding this issue.

[abzokhattab]

Here is one possible regression that would occur if we clear the selected transactions based on the isFocused state

i don't think we want this behavior

Screen.Recording.2024-10-08.at.11.22.14.PM.mov

[Krishna2323]

Here is one possible regression that would occur if we clear the selected transactions based on the isFocused state

That's not a bug, it's the behaviour we decided to use across the app.

selection_bug.mp4

cc: @Ollyws @OfstadC @MarioExpensify

[abzokhattab]

This was not decided to use "across the app" it was decided for the "workspace settings" taps only .. the search page is different.

a scenario that doesn't exist in the workspace settings:

Screen.Recording.2024-10-09.at.12.42.02.mov

I think making it dependent on the data would provide a better user experience, as the user wouldn’t have to reselect search items if the focus changes.

but yeah lets see what others think

[melvin-bot]

📣 @Krishna2323 🎉 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 📖

[MarioExpensify]

Let's move forward with @Krishna2323 proposal with the option to keep the selection while the user does not leave the page. Thank you @Ollyws.

[melvin-bot]

@Ollyws @OfstadC @Krishna2323 @MarioExpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[Ollyws]

Great, please assign them @MarioExpensify

[Krishna2323]

@Ollyws, I'm already assigned :), will raise the PR today.

[Krishna2323]

@Ollyws, PR ready for review ^

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.53-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Search - Selection persists after the expense is deleted and no expense is selected. #51105

If no regressions arise, payment will be issued on 2024-11-01. 🎊

For reference, here are some details about the assignees on this issue:

• @Ollyws requires payment through NewDot Manual Requests
• @Krishna2323 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
• [@Ollyws] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Ollyws] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Ollyws] Determine if we should create a regression test for this bug.
• [@Ollyws] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@OfstadC] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[OfstadC]

@Ollyws please complete the BZ checklist by Thursday EOD so I can issue payment on Friday 😃 Thanks!

[Ollyws]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR:

Seems like this has been the behaviour since the creation of the component.

• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

N/A

• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

N/A

• Determine if we should create a regression test for this bug.

Yes

• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Submit an expense to any user.
2. Go to Search.
3. Select the submitted expense via checkbox.
4. Go back to Inbox.
5. Delete the expense in Step 2.
6. Go to Search
7. Verify no transaction is selected
8. Select any other transaction
9. Open transaction details in the RHP > Go back
10. Verify the selection is persisted

Do we agree 👍 or 👎

[OfstadC]

Payment Summary

• @Krishna2323 paid $250 via Upwork
• @Ollyws due $250 via manual New Expensify request

Job posting Closed - QA Issue created

[melvin-bot]

@OfstadC @MarioExpensify Be sure to fill out the Contact List!

[Ollyws]

Requested in ND.

[garrettmknight]

$250 approved for @Ollyws