search

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

[HOLD for payment 2023-12-20] [DISTANCE] LOW: PHASE 1: Wave 5 CLEANUP Refactor navigation among screens related to money request features #26538

Closed hayata-suenaga closed 10 months ago

hayata-suenaga commented 1 year ago

Remove deep-linking support for screens related to money request

In the current code, deep linking to the screen for each step of money request flow is allowed.

There are logics to handle deep linking to a screen that corresponds to a step in the middle of the money request flow. These logics are not scalable and adds additional complexity.

Let's remove the support for deep linking. All screens should be under a single URL and all steps should be completed under that URL.

Relevant Slack discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1692998235169129

Issue OwnerCurrent Issue Owner: @anmurali
tgolen commented 1 year ago

We had some more discussion about this in https://expensify.slack.com/archives/C05DWUDHVK7/p1694542449982949 and I believe we should actually double-down on the URL based navigation. It has lots of benefits with the navigation flow (back buttons, screen animations, etc.).

Yes, it is pretty flawed and difficult to understand in its current state. I wouldn't mind taking this issue and having a go at cleaning this up so that it's more predictable and easier to use.

tgolen commented 1 year ago

Two big pieces for this were merged today. I'll keep working on cleanup.

Note to self: Try to remove ONYXKEYS.IOU

tgolen commented 1 year ago

I've been slowing chipping away at this over the last two days and have begun to implement a new way of routing and organizing the feature.

tgolen commented 1 year ago

I'm having to temporarily drop this down to weekly as I have had some higher priority things crop up that I needed to address urgently. My general update on this issue is that I'm about 50% done. I have refactored about 90% of the creation flow, and then I need to do a pass on the edit flow (but it should be easier than the creation flow because it was already following better patterns).

tgolen commented 1 year ago

I got back to working on this today. I made some good progress for trying to get the confirmation step cleaned up but I'm running into a low-level Onyx error that is causing participants to not be stored in Onyx. I posted about it here in Slack, but need to keep digging into that.

tgolen commented 1 year ago

I made really good progress on this today. I've got all the screens built for the create request flow. The next part of this is completing the confirmation step (the part that actually creates the requests). Once that is done, then I'll need to go through and find some scattered TODO: items and get them taken care of. Lastly, I'll need to go through the old components to ensure nothing has been added to them during development.

ETA is end of next week to have a PR ready for this.

tgolen commented 1 year ago

I approached this refactoring in a way to reduce conflicts. My idea was to essentially make as few changes to existing code as possible while building the new flow from scratch (copying code from the original components). This results in essentially a 100% duplicated flow. Once the two existing flow exist side-by-side, the original flow and files will be removed and they will be audited to ensure any changes since they were originally copied are incorporated into the new files.

I am creating a second issue for this final cleanup: https://github.com/Expensify/App/issues/29107

tgolen commented 1 year ago

Tasks remaining to be done:

tgolen commented 1 year ago

Today I:

Tasks remaining to be done:

tgolen commented 1 year ago

Today I:

Final things left to do:

tgolen commented 1 year ago

Today I was trying to get the PR mostly ready for review and I got all the components updated with what's in main. I then ran into issues with npm i failing and pod install failing, so I was unable to move into the testing phase. I'll dig into those tomorrow.

tgolen commented 1 year ago

OK, I finished my local testing today on all platforms 🎉 and I've moved the PR out of draft so it can begin to be reviewed.

tgolen commented 1 year ago

I had a discussion with @mountiny and @luacmartins and I'm taking a bit of a different approach to getting this PR merged. It seems that it's going to cause a lot of headaches with Money 2020 next week, so we agreed to hold on the majority of the refactor pieces. In the meantime, I am splitting off smaller PRs from it that will be easier to review and merge in isolation without as big of a risk of causing regressions. I created the first PR for that today: https://github.com/Expensify/App/pull/29696

tgolen commented 1 year ago

Split out these PRs yesterday:

Split out these PRs today:

tgolen commented 1 year ago

I'm adding an official HOLD to the title. There was at least one more PR that I could split out from this refactoring, and I should hopefully be able to get to it next week.

neil-marcellini commented 1 year ago

Money2020 is over tomorrow, removing the HOLD

tgolen commented 1 year ago

Pushing this back to daily now that the HOLD is removed.

tgolen commented 1 year ago

I have been working on updating the PR to get it ready for review. I began testing it today and it went OK-ish. I had to fix quite a few things, but I'm getting there slowly. The PR should be ready for review by EOD tomorrow.

melvin-bot[bot] commented 1 year ago

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

tgolen commented 1 year ago

I'm planning to work on more testing today. I found that the participants screen wasn't working with splits properly. It should be ready by the EOD today if all goes well.

tgolen commented 1 year ago

The PR is ready for review now and I'm taking it out of draft and added some tips for reviewing it.

tgolen commented 1 year ago

The PR is actively being reviewed and testing. I have one bug that I was not able to look into today. That's where I will pick back up on tomorrow.

tgolen commented 1 year ago

I'm moving this back to daily to reflect my current priority.

tgolen commented 1 year ago

Today I am getting the PR ready to create an ad-hoc build for Applause to start regression testing.

tgolen commented 12 months ago

The testing is held up on a Web-E PR that adds an additional API call necessary for testing. As soon as https://github.com/Expensify/Web-Expensify/pull/39758 is on staging, then the testing can commence on the App PR.

dylanexpensify commented 12 months ago

Looks like we got Web-E merged and on prod!!

tgolen commented 11 months ago

Lots of testing happened on this last week and 22 bugs have been reported so far. Applause hasn't been able to test on desktop yet with the builds failing. I'll start tackling these today and also solving all the conflicts.

tgolen commented 11 months ago
tgolen commented 11 months ago

I was able to make it through about half of these today. Gonna keep the gas pedal down tomorrow and keep working through them.

tgolen commented 11 months ago

I made some good progress on this. Only three bugs left to go!

tgolen commented 11 months ago

All the reported bugs have been fixed and I need to check today to see if there were any new ones reported with the last round of testing. If there were no new bugs, then I will push to have this merged today.

kbecciv commented 11 months ago

@tgolen Re-testing Summary:

https://github.com/Expensify/App/pull/28618#issuecomment-1826084751 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1816801720 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1816805031 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824765990 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824768199 is NOT fixed, still reproducible https://github.com/Expensify/App/pull/28618#issuecomment-1824797444 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824770628 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824773907 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824776162 is NOT fixed, still reproducible https://github.com/Expensify/App/pull/28618#issuecomment-1824777860 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824783919 is NOT fixed, still reproducible https://github.com/Expensify/App/pull/28618#issuecomment-1824785908 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824784736 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824790520 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824788253 is fixed https://github.com/Expensify/App/pull/28618#issuecomment-1824823713 is NOT fixed, still reproducible https://github.com/Expensify/App/pull/28618#issuecomment-1826082091, app is crashing after selecting WS - should we create an issue for this? https://github.com/Expensify/App/pull/28618#issuecomment-1826190134 is NOT fixed, still reproducible https://github.com/Expensify/App/pull/28618#issuecomment-1826388214 is fixed

Edit from Tim: https://github.com/Expensify/App/pull/28618#issuecomment-1816702786 is NOT fixed

kbecciv commented 11 months ago

@tgolen Also, seeing RHP tab button background colour is wrong, should we create an issue for this? Note: not reproduced on staging and production image

tgolen commented 11 months ago

Thank you for that update. I will look into those today.

Also, seeing RHP tab button background colour is wrong, should we create an issue for this?

No, that's probably related to this PR so I will look into it and fix it in this PR.

tgolen commented 11 months ago

@kbecciv I had a look through the remaining bugs and here are my results. I still had some issues reproducing some of them, but I think we are in really good shape. I've triggered another ad-hoc build on the PR to ensure there is a build with the most recent changes.

Resolved

Unresolved

image

https://github.com/Expensify/App/assets/1228807/39dd9056-6541-44fe-86a5-113d659804be

tgolen commented 11 months ago

bump @jjcoffee and @kbecciv

kbecciv commented 11 months ago

@tgolen Could you please share the link for latest build? I will work on the above issues.

kbecciv commented 11 months ago

Is this correct build to re-test https://github.com/Expensify/App/pull/28618#issuecomment-1841478938? @tgolen

tgolen commented 11 months ago

Yep, that's the right one. 👍

kbecciv commented 11 months ago

https://github.com/Expensify/App/pull/28618#issuecomment-1824776162 is still reproducible, try selecting a user you have never chatted with before and request money.

https://github.com/Expensify/App/assets/93399543/591074b2-331b-4ece-b210-126e6b0c7b7b

https://github.com/Expensify/App/pull/28618#issuecomment-1824823713 is still reproducible. I'm able to reproduce the issue by simply opening any IOU and refreshing the page.

https://github.com/Expensify/App/assets/93399543/0e45ae33-8ba6-4c43-950f-8d724d43aacd

https://github.com/Expensify/App/assets/93399543/6c8ec914-6d25-422d-82dc-f31a1703d85d

https://github.com/Expensify/App/pull/28618#issuecomment-1826082091 is reproducible, added 2 steps in OP

https://github.com/Expensify/App/assets/93399543/ec66bb4f-bd03-4c96-aad1-cb4cc155b830

https://github.com/Expensify/App/issues/26538#issuecomment-1840865247 is fixed!

image

tgolen commented 11 months ago

Thank you! Those are helpful tips. I'll look into them.

On Thu, Dec 7, 2023 at 12:31 PM Kateryna Becciv @.***> wrote:

#28618 (comment) https://github.com/Expensify/App/pull/28618#issuecomment-1824776162 is still reproducible, try selecting a user you have never chatted with before and request money.

https://github.com/Expensify/App/assets/93399543/591074b2-331b-4ece-b210-126e6b0c7b7b

#28618 (comment) https://github.com/Expensify/App/pull/28618#issuecomment-1824823713 is still reproducible. I'm able to reproduce the issue by simply opening any IOU and refreshing the page.

https://github.com/Expensify/App/assets/93399543/0e45ae33-8ba6-4c43-950f-8d724d43aacd

https://github.com/Expensify/App/assets/93399543/6c8ec914-6d25-422d-82dc-f31a1703d85d

#28618 (comment) https://github.com/Expensify/App/pull/28618#issuecomment-1826082091 is Fixed!

https://github.com/Expensify/App/assets/93399543/14e5a57d-f8b0-4e85-a685-4a94d673a191

#26538 (comment) https://github.com/Expensify/App/issues/26538#issuecomment-1840865247 is fixed!

image.png (view on web) https://github.com/Expensify/App/assets/93399543/9b430d4e-d8f9-48cd-bd30-0c04cd828fbf

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/26538#issuecomment-1845978364, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAB7WFSLWFC3ZKWSESEDYIIKPJAVCNFSM6AAAAAA4IEQ4Z6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBVHE3TQMZWGQ . You are receiving this because you were mentioned.Message ID: @.***>

melvin-bot[bot] commented 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 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.