[$250] IOU - Category briefly reselects itself when removed

lanitochka17 commented 1 month ago

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.86-0 Reproducible in staging?: Y Reproducible in production?: Y 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): betlihemasfaw14@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

FAB > Submit Expense
Add amount and choose workspace
Add category and merchant
Go to IOU > Category > click to remove

Expected Result:

The category should not briefly reselect itself

Actual Result:

The category briefly reselects itself when clicked to remove

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/0e3ec17d-521c-4b0c-8911-cad120ef2785

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c03aa025a5c5f612
Upwork Job ID: 1807555103945888117
Last Price Increase: 2024-07-21

Issue Owner

Current Issue Owner: @fedirjh

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @jliexpensify (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.

lanitochka17 commented 1 month ago

@jliexpensify 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

jliexpensify commented 1 month ago

Yep, I can reproduce this one - after selecting and deselecting multiple times (e.g. 2 times) the Category reappears briefly again.

NOTE: Technically, IOU functionality is under #billpay, but I think this would affect all users so adding it under #collect

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

pankajsoftyoi commented 1 month ago

Proposal

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

Category briefly reselects itself when removed

What is the root cause of that problem?

There isn't any condition in place which prevents getting reselected itself in onSelectRow(item);. https://github.com/Expensify/App/blob/57622ffe1eb5e8396a0e60d8b9b06f9451985589/src/components/SelectionList/BaseListItem.tsx#L79-L82

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

Need to set the following condition for already selected item. With this change it will prevent to reselect itself.

 if (!canSelectMultiple && !item.isSelected && !rightHandSideComponent) {
      onSelectRow(item);
}

What alternative solutions did you explore? (Optional)

N/A

jliexpensify commented 1 month ago

Bump @fedirjh for a review!

fedirjh commented 1 month ago

Still awaiting more proposals.

fedirjh commented 1 month ago

@pankajsoftyoi Your proposal looks like a reverse solution statement. Can you elaborate more on the root cause of the problem? Can you explain why this bug occurs after multiple selecting and deselecting?

melvin-bot[bot] commented 3 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

fedirjh commented 3 weeks ago

Still awaiting more proposals.

wlegolas commented 3 weeks ago

Proposal

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

IOU - Category briefly reselects itself when removed

What is the root cause of that problem?

When the user selects and deselects multiple times (e.g. 2 times) the Category reappears briefly again because the previous request finished successfully and updates the Onyx state with a new transaction that has the category information before the request to update the transaction with deselected category has not yet been completed.

For example:

The user clicks on the "Category" label to open the modal to select the Category.
The user selects the category "Advertising"
App closes the modal
The request UpdateMoneyRequestCategory is started to update the transaction with the selected category "Advertising".
The user clicks on the "Category" label to open the modal to deselect the Category.
User deselects the category "Advertising"
App closes the modal
The request UpdateMoneyRequestCategory is started to update the transaction with an empty string in the category property.
The user clicks on the "Category" label to open the modal
The request started at step 4 finished and updates the Onyx state with a new transaction with the category "Advertising"
User sees the "Advertising" as selected category
The request started at step 8 finished and updates the Onyx state with a new transaction with the category with an empty string
User sees the "Advertising" being deselected

The issue is caused because when the user deselects the Category (CategoryPiker component) and opens the modal again (MoneyRequestView component) the previous request (to select the Category "Advertising") finished and updates the state with a new transaction with the category "Advertising" and the user can see the "Advertising" category as selected and after a while the last request finishes and updates the state with an empty category.

In the following video, you can see the steps above, in the Network tab you can see when each request finishes.

https://github.com/Expensify/App/assets/698363/8f83baf0-0196-4589-8e5b-f87f0b0068da

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

We should change the IOURequestStepCategory component to show the loading message meanwhile the request to update the category is not completed yet. With this solution when the user opens the modal they will see a loading indicator and when the last request finishes the user will be able to proceed.

The loading indicator here should be changed to use the isLoadingReportData information from Onyx state to show the loading indicator if the isLoadingReportData property is true. For example:

function IOURequestStepCategory({
    report: reportReal,
    reportDraft,
    route: {
        params: {transactionID, backTo, action, iouType, reportActionID},
    },
    transaction,
    splitDraftTransaction,
    policy: policyReal,
    policyDraft,
    policyTags,
    policyCategories: policyCategoriesReal,
    policyCategoriesDraft,
    reportActions,
    session,
    isLoadingReportData,
}: IOURequestStepCategoryProps) {
    ...
    const {isOffline} = useNetwork({onReconnect: fetchData});
    const isLoading = !isOffline && (policyCategories === undefined || isLoadingReportData); // Here we should change
}

...

const IOURequestStepCategoryWithOnyx = withOnyx<IOURequestStepCategoryProps, IOURequestStepCategoryOnyxProps>({
    ...
    session: {
        key: ONYXKEYS.SESSION,
    },
    isLoadingReportData: {
        key: ONYXKEYS.IS_LOADING_REPORT_DATA,
    },
})(IOURequestStepCategory);

What alternative solutions did you explore? (Optional)

N/A

POC

https://github.com/Expensify/App/assets/698363/9b5191b1-0439-44cb-b21e-b9d8415ffc68

fedirjh commented 3 weeks ago

@wlegolas Your proposal doesn’t look good to me :

Breaking the offline flow: I believe that implementing your proposal may disrupt the offline functionality. Users won't be able to update the category while in offline mode. The loading indicator will be displayed until they are connected again.
Loading indicator on form: Introducing a loading indicator slowly to this form would deviate from the standard behavior observed on other pages. It would make the behavior inconsistent, as only this view would have a loading indicator while others wouldn't.

wlegolas commented 3 weeks ago

Hi @fedirjh thank you for sharing your points.

About your point about "Breaking the offline flow" the loading indicator won't be shown because the first condition is to be online:

const isLoading = !isOffline && (policyCategories === undefined || isLoadingReportData);

I recorded the video below to show the offline functionality when the CategoryPicker is shown and the user is offline, you can see the behavior is the same that we have currently on Staging.

https://github.com/Expensify/App/assets/698363/9d4b3ea0-ba92-4562-a20c-74f4901f4677

Related to your point "Loading indicator on form", I suggested putting a loading indicator because the application state is not up to date and the CategoryPicker component needs to show feedback to the user about the previous process still occurring. I checked in other forms but didn't find a form with the same behavior that I found in the CategoryPicker, maybe other forms don't have this problem with the app state not being up to date.

If you know another form that had this same problem I can take a look at to find out another solution to solve this problem.

mvtglobally commented 3 weeks ago

Issue not reproducible during KI retests. (First week)

jliexpensify commented 3 weeks ago

Oh interesting, @fedirjh can you repro this? I wasn't able to.

wlegolas commented 3 weeks ago

I could see this problem when I changed my network connection to a slow one because the main problem was related to the time that the previous request didn't finish with the time the user opened the modal (CategoryPicker).

jliexpensify commented 3 weeks ago

Interesting - @fedirjh since selecting a Category is pretty integral to the product, it would be great to ensure that this is rock-solid. Are you able to confirm this and determine if we can fix it? Thanks!

melvin-bot[bot] commented 3 weeks ago

@jliexpensify @fedirjh 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!

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 2 weeks ago

Waiting on @fedirjh

fedirjh commented 2 weeks ago

@jliexpensify I am still able to reproduce the bug on staging.

fedirjh commented 2 weeks ago

@wlegolas I noticed something in the server response. For other fields, we have two properties: the original one and the modified one. However, for the category field (i.e we have amount and modifiedAmount), we don't have such property. This probably explains why other fields are working properly.

wlegolas commented 2 weeks ago

Hi @fedirjh I agree with your point about the category field does not have a related modified field.

I simulated locally adding the modifiedCategory field and looks like worked as expected.

I can change my proposal to have the implementation to fix it but in my opinion, we have two changes, one in the Frontend project (I can put the details in my current proposal) and another change on the Backend side because the transaction response when updating the Money Request Category needs to return this new field modifiedCategory to update the state with the correct information.

If you agree I can change my proposal to have the information to use the new field modifiedCategory to fix this problem.

fedirjh commented 2 weeks ago

we have two changes, one in the Frontend project (I can put the details in my current proposal)

@wlegolas Can you share details about front end changes?

wlegolas commented 2 weeks ago

we have two changes, one in the Frontend project (I can put the details in my current proposal)

@wlegolas Can you share details about front end changes?

Sure, below I'm putting a brief details about the front end changes that we should do to have the modifiedCategory information in the Transaction object and also solve this issue.

Add the new property modifiedCategory in the Transaction type
Change the getCategory method to retrieve the category value first using the modifiedCategory this looks like the implementation we have in the getMerchant
Improve the getUpdateMoneyRequestParams method to add a state in the successData and failureData to have the correct information before the request is finished. For example:

function getUpdateMoneyRequestParams(
    transactionID: string,
    transactionThreadReportID: string,
    transactionChanges: TransactionChanges,
    policy: OnyxTypes.OnyxInputOrEntry<OnyxTypes.Policy>,
    policyTagList: OnyxTypes.OnyxInputOrEntry<OnyxTypes.PolicyTagList>,
    policyCategories: OnyxTypes.OnyxInputOrEntry<OnyxTypes.PolicyCategories>,
    onlyIncludeChangedFields: boolean,
): UpdateMoneyRequestData {
    const optimisticData: OnyxUpdate[] = [];
    const successData: OnyxUpdate[] = [];
    const failureData: OnyxUpdate[] = [];
    ...

    // Update recently used categories if the category is changed
    if ('category' in transactionChanges) {
        const optimisticPolicyRecentlyUsedCategories = Category.buildOptimisticPolicyRecentlyUsedCategories(iouReport?.policyID, transactionChanges.category);
        if (optimisticPolicyRecentlyUsedCategories.length) {
            optimisticData.push({
                onyxMethod: Onyx.METHOD.SET,
                key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES}${iouReport?.policyID}`,
                value: optimisticPolicyRecentlyUsedCategories,
            });
        }

        // Update the state with the modified category
        successData.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
            value: {
                modifiedCategory: transactionChanges.category,
            },
        });

        // Revert the transaction's modified category to the original value on failure.
        if (transaction) {
            failureData.push({
                onyxMethod: Onyx.METHOD.MERGE,
                key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
                value: {
                    modifiedCategory: transaction.modifiedCategory,
                },
            });
        }
    }

    ...
}

Add some Unit Tests here to ensure that the getCategory method is returning the correct information across transaction scenarios.

This solution will work properly If the back end returns the modifiedCategory when the UpdateMoneyRequestCategory request is completed.

If you have any questions or suggestions, please let me know.

mvtglobally commented 2 weeks ago

Issue not reproducible during KI retests. (Second week)

jliexpensify commented 2 weeks ago

Waiting on @fedirjh

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

fedirjh commented 1 week ago

I am still able to replicate on staging.

fedirjh commented 1 week ago

@jliexpensify @wlegolas After reconsidering, I believe that the current behavior is expected. I can replicate this behavior with other fields that have a list, such as tags and taxes. To reliably reproduce the behavior:

Submit new expense
Go offline
Update the category field several times (select and deselect or select different categories)
Open category selector again
Go online

https://github.com/user-attachments/assets/c9be0de3-0fbb-4a0d-8e61-d008024e4ec5

fedirjh commented 1 week ago

@jliexpensify I think we can just close this for now.

jliexpensify commented 1 week ago

Ok, that's fair - thanks for doing some additional testing @fedirjh , closing!

Expensify / App