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
3.56k stars 2.9k forks source link

[$250] Expense - Not here page shows up briefly when deleting the expense #45576

Open izarutskaya opened 4 months ago

izarutskaya commented 4 months 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.8-1 Reproducible in staging?: Y Reproducible in production?: N Found when executing PR : https://github.com/Expensify/App/pull/44537 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to DM.
  3. Submit two expenses to the user.
  4. Open any of the transaction thread.
  5. Tap on the report header.
  6. Tap Delete expense..
  7. Delete the expense.

Expected Result:

Not here page will not show up when deleting the expense.

Actual Result:

Not here page shows up briefly when deleting the expense.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/819b601e-65bc-4aab-82ea-69cfa7faa418

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c66dfe570a289fac
  • Upwork Job ID: 1813534350182069816
  • Last Price Increase: 2024-08-20
  • Automatic offers:
    • alitoshmatov | Reviewer | 103693223
    • wildan-m | Contributor | 104145017
Issue OwnerCurrent Issue Owner: @alitoshmatov
melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

luacmartins commented 2 months ago

@daledah @alitoshmatov we reverted the PR associated with this issue because it caused the following regressions. Please take them into consideration when working on v2.

alitoshmatov commented 2 months ago

So looks like chosen solution was not a good one. We need to start over and choose a better proposal

Can we reapply Help wanted label to get some eyes cc: @luacmartins @sonialiap

wildan-m commented 2 months ago

@alitoshmatov Would you be able to take another look at my proposal? https://github.com/Expensify/App/issues/45576#issuecomment-2309556187 The branch link has been updated with the latest main, and my quick test doesn't show any regressions. (https://github.com/Expensify/App/issues/45576#issuecomment-2346844390)

melvin-bot[bot] commented 2 months ago

@iwiznia, @sonialiap, @luacmartins, @alitoshmatov, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

alitoshmatov commented 2 months ago

@alitoshmatov Would you be able to take another look at my proposal? https://github.com/Expensify/App/issues/45576#issuecomment-2309556187 The branch link has been updated with the latest main, and my quick test doesn't show any regressions. (https://github.com/Expensify/App/issues/45576#issuecomment-2346844390)

Looks promising, let me do some more testing

melvin-bot[bot] commented 1 month ago

@iwiznia, @sonialiap, @luacmartins, @alitoshmatov, @daledah Huh... This is 4 days overdue. Who can take care of this?

sonialiap commented 1 month ago

@alitoshmatov how's the testing going?

iwiznia commented 1 month ago

Is there any progress here? It's been a week without update, should we re-assign?

allgandalf commented 1 month ago

Commenting on https://github.com/Expensify/App/issues/45576#issuecomment-2282063749,

@alitoshmatov then i think on small screens we can navigate first and then delete, would you please consider my proposal again here, thanks

melvin-bot[bot] commented 1 month ago

@iwiznia, @sonialiap, @luacmartins, @alitoshmatov, @daledah Still overdue 6 days?! Let's take care of this!

alitoshmatov commented 1 month ago

I think we can go with @wildan-m's proposal which soft deletes transaction

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 1 month ago

Current assignees @iwiznia and @luacmartins are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

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

wildan-m commented 1 month ago

working on the PR, perform more tests

wildan-m commented 1 month ago

@alitoshmatov @iwiznia there is an open issue that will block the result of the test of this issue in iOS, I can experience it after implement the solution, should we keep creating the PR or hold this one?

iwiznia commented 1 month ago

If you think that your code will work once that other issue is done, then let's send the PR but keep this issue on hold till we can actually test the results in all platforms.

melvin-bot[bot] commented 1 month ago

@iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah Eep! 4 days overdue now. Issues have feelings too...

wildan-m commented 1 month ago

@alitoshmatov @iwiznia after further testing, my solution doesn't solve all cases, the briefly [deleted] text also shown in the report header.

https://github.com/user-attachments/assets/08ca609f-2186-4ba4-8552-29467e65148c

While I search for another solution, you should consider reapplying the "help wanted" label to attract better proposals.

allgandalf commented 1 month ago

If so, then i guess we can work on my proposal to improve it and apply further

alitoshmatov commented 1 month ago

Thank you @wildan-m . Then we are back on choosing proposals

alitoshmatov commented 1 month ago

FYI: Looks like we have the same issue when deleting tasks

https://github.com/user-attachments/assets/b5f19406-1b5e-4ca7-b163-de43f9fd6a68

wildan-m commented 1 month ago

Proposal Updated

wildan-m commented 1 month ago

@alitoshmatov Any feedback about my updated proposal? I've noticed that I'm still assigned, if we are ok with the solution I'll create the PR.

melvin-bot[bot] commented 1 month ago

@iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 month ago

@iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah Huh... This is 4 days overdue. Who can take care of this?

sonialiap commented 1 month ago

@alitoshmatov bumping the request to review proposal changes

melvin-bot[bot] commented 1 month ago

@iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 1 month ago

@iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah 10 days overdue. I'm getting more depressed than Marvin.

luacmartins commented 4 weeks ago

@alitoshmatov what's the latest here? Did you get a chance to look into this proposal?

melvin-bot[bot] commented 4 weeks ago

This issue has not been updated in over 14 days. @iwiznia, @wildan-m, @sonialiap, @luacmartins, @alitoshmatov, @daledah eroding to Weekly issue.

alitoshmatov commented 4 weeks ago

Sorry for late response. @wildan-m Your proposal looks promising, can you update your solution with main branch, I think there are some changes there and it looks like effecting your solution

wildan-m commented 3 weeks ago

Resolving conflict with the latest main branch & re-testing...

wildan-m commented 3 weeks ago

Proposal Updated

I can confirm that this iOS issue https://github.com/Expensify/App/issues/45576#issuecomment-2384466032 has resolved after https://github.com/Expensify/App/issues/47584 closed.

@alitoshmatov

sonialiap commented 1 week ago

@alitoshmatov bumping for proposal review

alitoshmatov commented 6 days ago

Okay I think we can go with @wildan-m 's proposal

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 6 days ago

Current assignees @iwiznia and @luacmartins are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

iwiznia commented 6 days ago

In mWeb, even when we navigate to destination report earlier, not found might still appear,

Why would mWeb be different than mobile?

navigateBackAfterDeleteTransaction

Nitpick but let's rename this then since the order of operations is different now, maybe navigateBackOnDeleteTransaction

wildan-m commented 6 days ago

Why would mWeb be different than mobile?

@iwiznia Using Onyx.set may be too quick for certain platforms or if React Navigation transition is slower than Onyx changes using set in Android mWeb. In such cases, we can opt for Onyx.merge instead. This approach is already utilized in leaveRoom and leaveGroupChat and it can effectively address similar issues.

wildan-m commented 6 days ago

PR creation blocked by this issue

Cannot read properties of undefined (reading 'ownerAccountID')

https://github.com/user-attachments/assets/1595b10c-32ab-4f40-8abf-9ba19f6e0423

Reported here

https://expensify.slack.com/archives/C01GTK53T8Q/p1731569593270779

iwiznia commented 5 days ago

That should be fixed already @wildan-m

wildan-m commented 5 days ago

@iwiznia @alitoshmatov @sonialiap I can't reproduce the issue in the latest main, can you?

wildan-m commented 1 day ago

@alitoshmatov I can reproduce it again, especially in ios native. The PR is ready to review https://github.com/Expensify/App/pull/52740. Thanks!