Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.02k stars 3.01k forks source link

[$250] Bulk Invoice Delete doesn't remove the deleted rows from the UI #55680

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks 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: 9.0.89-2 Reproducible in staging?: y Reproducible in production?: y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @mananjadhav Slack conversation (hyperlinked to channel name): expensify - bug

Action Performed:

  1. Go to Reports section with some outstanding invoices.
  2. Click on Invoices
  3. Click on the select all invoices or choose some of the invoices by clicking on the checkbox
  4. Click on the top right button and delete the invoice.
  5. You'll notice the selected invoices are still retained in the table.
  6. They're removed on refresh of the page.

    Expected Result:

    The deleted invoices should be removed from the UI as soon as they're deleted.

    Actual Result:

    The selected invoices are still retained in the table even after being deleted. They're removed on refreshing the page.

    Workaround:

    unknown

    Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/7b5c4d54-f6ed-4cab-9d00-f28e7ff2a81a

https://github.com/user-attachments/assets/fcec8183-1488-4d0a-8bd5-4a7ca45abd6d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021882909145340105934
  • Upwork Job ID: 1882909145340105934
  • Last Price Increase: 2025-01-31
  • Automatic offers:
    • DylanDylann | Reviewer | 105975282
    • FitseTLT | Contributor | 105975284
Issue OwnerCurrent Issue Owner: @DylanDylann
melvin-bot[bot] commented 2 weeks ago

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

Christinadobrzyn commented 1 week ago

I think this can be external

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

πŸ“£ @mac-ad! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 week ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

samh0lmes commented 1 week ago

Contributor details Your Expensify account email: sam.holmes1293+test@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01280e21e1cafb6114

Proposal

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

When a user selects 1 or more transactions to be deleted, they remain on the page until a refresh (sometimes with a delay)

What is the root cause of that problem?

The deletion is done asynchronously and the UI does not reflect that the items have been deleted until the API request is complete. The transactions are not removed from the UI until they are actually deleted.

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

Ensure the UI is optimistically updated with the selected transaction IDs to be deleted before the async job is complete by ensuring the optimisticData does not include the deleted transaction IDs: https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/libs/actions/Search.ts#L355

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

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Delete It should be immediately removed from the UI

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Cancel It should not be removed from the UI (even on refresh)

Go to reports -> invoices Add a few invoices Delete multiple invoice Click Delete They should be immediately removed from the UI

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Cancel They should not be removed from the UI (even on refresh)

melvin-bot[bot] commented 1 week ago

πŸ“£ @samh0lmes! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
samh0lmes commented 1 week ago

Contributor details Your Expensify account email: sam.holmes1293+test@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01280e21e1cafb6114

melvin-bot[bot] commented 1 week ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

Christinadobrzyn commented 1 week ago

Hi @DylanDylann can you please review these proposals when you have a moment? Thank you!

DylanDylann commented 1 week ago

When deleting money requests on the Search page. We don't remove it optimistically, instead of that we call Search APi again and then update UI. This bug is caused by the Search API taking a long time to respond (or failing).

https://github.com/user-attachments/assets/0836490a-5b8d-4ef4-842f-c3355bf74d6d

DylanDylann commented 1 week ago

@luacmartins Could you help to dig into this one to find out the best solution for this bug? (this is my investigation)

I have two solutions:

  1. On all actions on Search, let's also update optimistic data for the snapshot
  2. We can display a loading indicator while waiting for the action on the search and the search API is successful
samh0lmes commented 1 week ago

@DylanDylann oh gotcha yeah then a loading screen would be a simple solution there

DylanDylann commented 1 week ago

Let's wait for @luacmartins to confirm that

luacmartins commented 1 week ago

On all actions on Search, let's also update optimistic data for the snapshot

I don't think we should do this since all Search commands work online only.

@DylanDylann oh gotcha yeah then a loading screen would be a simple solution there

This sounds like the way to go here.

DylanDylann commented 1 week ago

Note for contributors about the expectation

We can display a loading indicator while waiting for the action on the search and the search API is successful

Christinadobrzyn commented 1 week ago

Just checking on this, are we waiting for new/revised proposals @DylanDylann?

DylanDylann commented 1 week ago

Looking for proposals

samh0lmes commented 1 week ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-30 15:25:30 UTC.

Do we need design for this? If not, here's a proposal:

Contributor details Your Expensify account email: sam.holmes1293+test@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01280e21e1cafb6114

Proposal

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

When a user selects and deletes 1 or more transactions, the transactions remain on the page until a refresh (sometimes with a delay)

What is the root cause of that problem?

The deletion API call is taking a long time and there is no loading indicator to the user. Additionally, the UI does not reflect that the items have been deleted until the API request is complete AND the page is refreshed.

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

Add a loading indicator when the API call is being made (https://github.com/Expensify/App/blob/main/src/components/Search/SearchPageHeader.tsx#L91) and remove the transactions + loading indicator when the API call is completed

Potentially following this pattern: https://github.com/Expensify/App/blob/main/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx#L72

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

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Delete A loading indicator should appear When the loading indicator is gone, the transaction should be immediately removed from the UI

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Cancel It should not be removed from the UI (even on refresh)

Go to reports -> invoices Add a few invoices Delete multiple invoice Click Delete A loading indicator should appear When the loading indicator is gone, the transactions should be immediately removed from the UI

Go to reports -> invoices Add a few invoices Delete 1 invoice Click Cancel They should not be removed from the UI (even on refresh)

FitseTLT commented 1 week ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-30 23:10:34 UTC.

Proposal

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

Bulk Invoice Delete doesn't remove the deleted rows from the UI

What is the root cause of that problem?

New Feature

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

We can set some isActionLoading prop optimistically on the transaction item similar to here then revert it only on failureData (we will not revert it on finallyData because in this case the API response will not remove the transition items) and then display a loading indicator until the Search is called and the transaction items are removed. https://github.com/Expensify/App/blob/6118a1ff12f673953000c1266f7fdc52c0caa39c/src/libs/actions/Search.ts#L355

  const createOnyxData = (update: Partial<SearchTransaction> | Partial<SearchReport>): OnyxUpdate[] => [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`,
            value: {
                data: Object.fromEntries(transactionIDList.map((transactionID) => [`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, update])) as Partial<SearchTransaction>,
            },
        },
    ];
    const {optimisticData, finallyData} = getOnyxLoadingData(hash);

    optimisticData.push(...createOnyxData({isActionLoading: true}));
    const failureData = createOnyxData({isActionLoading: false});

    API.write(WRITE_COMMANDS.DELETE_MONEY_REQUEST_ON_SEARCH, {hash, transactionIDList}, {optimisticData, finallyData, failureData});
}

https://github.com/Expensify/App/blob/6118a1ff12f673953000c1266f7fdc52c0caa39c/src/components/SelectionList/Search/ActionCell.tsx#L90-L91

                isLoading={isLoading}

The basic idea is this but we can discuss on the design perspective of how the loading should be displayed. It might also be important to make the item non-interactive until it is removed from search result.

Optionally we can also consider to show the loading indicator after the search delete API is successful in that case we will only set the loading prop on successData.

Result https://github.com/user-attachments/assets/10033df0-604f-4e9c-81be-c3641cc9191b

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

N / A

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 6 days ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

DylanDylann commented 4 days ago

It is straightforward. Let's go with @FitseTLT's proposal

πŸŽ€ πŸ‘€ πŸŽ€ C+ Reviewed

melvin-bot[bot] commented 4 days ago

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

melvin-bot[bot] commented 3 days ago

πŸ“£ @DylanDylann πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 days ago

πŸ“£ @FitseTLT πŸŽ‰ 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 πŸ“–

FitseTLT commented 3 days ago

I really doubt if showing loading indicator on delete is correct UI pattern. I think it is better to use our existing pattern of striking-through, greying-out/disabling the row.

So asking for confirmation @Expensify/design

When transactions are deleted from Search it takes a while before the search items are removed so here we are aiming to show some indicator that the items are deleted. Though showing loading indicator has been proposed as an option I didn't like it and it doesn't align with the UI we use for pending delete in other places. So guys what would you suggest us here?

FitseTLT commented 2 days ago

@dannymcclain I had a question for the design team above ^

And BTW I am not getting a response from the design team after mentioning @Expensify/design in my comment Is there anything I am missing? I see it works for others πŸ˜•

dannymcclain commented 2 days ago

And BTW I am not getting a response from the design team after mentioning @Expensify/design in my comment Is there anything I am missing? I see it works for others πŸ˜•

Hmm, not sure about this but it definitely does not seem to be working. We're asking our internal team about it.

dannymcclain commented 2 days ago

When transactions are deleted from Search it takes a while before the search items are removed so here we are aiming to show some indicator that the items are deleted.

As for this, I'd love to get @luacmartins in here since I feel like he will have some good insights. Any idea why these can't just animate out optimistically? (And by "animate" I mean the same type of animation we use when adding a row to the reports page)

cc @Expensify/design

luacmartins commented 2 days ago

The search commands only work online, so it might be a bit odd to animate them optimistically and have them rollback if the command fails. We could run the animation on the success of the command, but I don't think we should do it while the request is in flight

dannymcclain commented 2 days ago

Ah ok that makes sense. In that case, I'd probably lean into our reduced opacity pattern for this. I don't think we need to strike-through text like in a message since each of these are self-contained and much less free form than a chat message.

Here's what I'm thinking:

Image

cc @Expensify/design for viz / gut check

shawnborton commented 2 days ago

Makes sense to me, also let's get a gut check from our resident Offline expert @trjExpensify :)

dubielzyk-expensify commented 2 days ago

Definitely like the fade out, but keen to hear what Tom thinks too

trjExpensify commented 2 days ago

Reduced opacity with strikethrough for optimistic delete actions! πŸ™‡

shawnborton commented 2 days ago

Ah do we even do strikethrough if the element is contained within a card, like the expense rows are?

trjExpensify commented 2 days ago

Yeah, looks like it:

Image
shawnborton commented 2 days ago

Nice, precedent! Okay @dannymcclain guess you gotta update the mocks.

DylanDylann commented 2 days ago

@luacmartins I think we only should strike out or grey out when users are offline (following the offline pattern B). In case waiting for API, should we display a loading indicator for all pages?

cc @trjExpensify @Expensify/design

luacmartins commented 1 day ago

That's a good point. Because the Search commands only work online, they should follow Pattern C or D, but I don't know how I feel about a loading indicator here.

DylanDylann commented 1 day ago

I mean this one

Image
luacmartins commented 1 day ago

Right, I think we usually keep the view with the stale data but refresh it once the request is done. I can see how that'd be a bit unexpected after deleting something and not seeing it not reflect in the table though. I'd lean into the @Expensify/design team for this.

dubielzyk-expensify commented 1 day ago

Yeah I kinda think we should avoid the loading indicator if we can. Can we delete it optimistically? And only show an error dialog or something if the requests fails?

FitseTLT commented 1 day ago

The loading screen would prevent users from performing other actions until the delete succeeds I don't like it.

BTW the search APIs are a bit different from other APIs

  1. Search delete API is requested
  2. the delete API responds succesfully
  3. We sense change of transaction list change and re-execute search API for new search data
  4. New search list arrives.

@luacmartins I know what you meant by not use optimistic data in search as it is done online. But what if we make an exception in our optimistic approach and instead of setting optimisticData in (1) we remove the items from onyx snapshots_ list on successData i.e. on step (2) there is no way the transaction items would reappear after we are sure our search delete API has succeeded it all about time before it is removed in step (4) anyway. Let me know your opinion.

luacmartins commented 1 day ago

It feels a bit unnecessary since the data removing the transaction would come from the API anyways, but I think that's fine.

FitseTLT commented 1 day ago

It feels a bit unnecessary since the data removing the transaction would come from the API anyways, but I think that's fine.

The current issue is caused because Search API is sometimes not executed and the items are not removed unless refreshed this will solve it.

trjExpensify commented 1 day ago

I think we only should strike out or grey out when users are offline (following the offline pattern B). In case waiting for API, should we display a loading indicator for all pages? That's a good point. Because the Search commands only work online, they should follow Pattern C or D, but I don't know how I feel about a loading indicator here.

Oh, if we don't let them take the Delete action when offline, we should follow what we do for the Download action when offline then right?

dannymcclain commented 1 day ago

Here's a video of that offline download flow:

https://github.com/user-attachments/assets/f2610dac-23b9-46e0-b5b5-34d85227b6a7

DylanDylann commented 1 day ago

This issue isn't related to offline mode.

https://github.com/user-attachments/assets/9dfd8da6-6d4a-4f63-8d88-86fd72a94cae

When we delete rows, the removing action takes a long time to respond. Now we need to look for some ways to display UI while waiting for removing action to give a response

melvin-bot[bot] commented 1 day ago

@luacmartins @FitseTLT @Christinadobrzyn @DylanDylann 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!