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

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

Open
8 tasks done
m-natarajan opened this issue Jan 23, 2025 · 54 comments
Open
8 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Jan 23, 2025

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

  • 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
Recording.920.mp4
web-bulk-delete-invoice.mov

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
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 23, 2025
Copy link

melvin-bot bot commented Jan 23, 2025

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
Copy link
Contributor

I think this can be external

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Jan 24, 2025
@melvin-bot melvin-bot bot changed the title Bulk Invoice Delete doesn't remove the deleted rows from the UI [$250] Bulk Invoice Delete doesn't remove the deleted rows from the UI Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

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

Copy link

melvin-bot bot commented Jan 25, 2025

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

Copy link

melvin-bot bot commented Jan 25, 2025

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@samh0lmes
Copy link

Contributor details
Your Expensify account email: [email protected]
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:

const {optimisticData, finallyData} = getOnyxLoadingData(hash);

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)

Copy link

melvin-bot bot commented Jan 25, 2025

📣 @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
Copy link

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01280e21e1cafb6114

Copy link

melvin-bot bot commented Jan 25, 2025

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@Christinadobrzyn
Copy link
Contributor

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

@DylanDylann
Copy link
Contributor

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

Screen.Recording.2025-01-28.at.00.30.18.mov

@melvin-bot melvin-bot bot removed the Overdue label Jan 27, 2025
@DylanDylann
Copy link
Contributor

@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
Copy link

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

@DylanDylann
Copy link
Contributor

Let's wait for @luacmartins to confirm that

@luacmartins
Copy link
Contributor

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.

@luacmartins luacmartins self-assigned this Jan 27, 2025
@DylanDylann
Copy link
Contributor

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
Copy link
Contributor

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

Copy link

melvin-bot bot commented Feb 3, 2025

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

Copy link

melvin-bot bot commented Feb 3, 2025

📣 @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
Copy link
Contributor

FitseTLT commented Feb 3, 2025

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
Copy link
Contributor

FitseTLT commented Feb 4, 2025

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@dubielzyk-expensify
Copy link
Contributor

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

@trjExpensify
Copy link
Contributor

Reduced opacity with strikethrough for optimistic delete actions! 🙇

@shawnborton
Copy link
Contributor

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

@trjExpensify
Copy link
Contributor

Yeah, looks like it:

Image

@shawnborton
Copy link
Contributor

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Feb 5, 2025

@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
Copy link
Contributor

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
Copy link
Contributor

I mean this one

Image

@luacmartins
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

FitseTLT commented Feb 5, 2025

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
Copy link
Contributor

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Feb 5, 2025

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
Copy link
Contributor

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
Copy link
Contributor

Here's a video of that offline download flow:

CleanShot.2025-02-05.at.10.43.18.mp4

@DylanDylann
Copy link
Contributor

This issue isn't related to offline mode.

406236629-fcec8183-1488-4d0a-8bd5-4a7ca45abd6d.mov

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

Copy link

melvin-bot bot commented Feb 6, 2025

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

@dannymcclain
Copy link
Contributor

Alright in that case I think we go with the reduced opacity strike-through style we use for pending delete offline. That feels like it still makes sense to me since it's a pending action. @Expensify/design how do you all feel about that?

Image

@dubielzyk-expensify
Copy link
Contributor

Yeah that makes sense to me 👍

@shawnborton
Copy link
Contributor

Looks great to me 👍

@DylanDylann
Copy link
Contributor

@FitseTLT Let's move forward this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: MEDIUM
Development

No branches or pull requests

10 participants