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.49k stars 2.84k forks source link

[HOLD for payment 2023-09-27] [$1000] Opening report via deep link animates the page twice #23441

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!

Action Performed:

  1. Navigate to a report
  2. Copy the url to clipboard
  3. Return to LHN
  4. Minimise the app
  5. Paste the url in an external app like Notes
  6. Tap on the report url

    Expected Result:

    NewDot opens and navigates to the corresponding report

    Actual Result:

    App opens and the report page gets animated in twice

    Workaround:

    Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.44-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/23f9d604-d6e1-49c3-888c-3dda8d01e031

https://github.com/Expensify/App/assets/43996225/d72a7517-e866-4ff8-92a7-9e8635571c57

Expensify/Expensify Issue URL: Issue reported by: @aswin-s Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690060285079399

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019b9b5dcffbb53478
  • Upwork Job ID: 1684994291328241664
  • Last Price Increase: 2023-09-01
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 26486528
    • tienifr | Contributor | 26486529
    • aswin-s | Reporter | 26486531
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

kevinksullivan commented 1 year ago

Unable to produce this as the link takes me to mWeb. Closing unless someone can correct the steps or let me know what's wrong.

https://github.com/Expensify/App/assets/22508290/dc7a6860-3235-4461-a1ac-631326dd3ad1

aswin-s commented 1 year ago

@kevinksullivan I can still reproduce this bug. I'm on latest prod build 1.3.42-26. Also I noticed that the link gets opened in mWeb, which should never happen on staging or prod builds. Deep links should get opened in native app itself.

Steps followed.

  1. Navigate to a group chat
  2. Copy URL
  3. Navigate back to LHN. (Important)
  4. Minimise NewDot and paste the URL in notes.
  5. Tap on the URL to open the deep in in NewDot.

https://github.com/Expensify/App/assets/6880914/34c20b15-cbfd-4b1d-9dc6-cd60ccc930b2

aswin-s commented 1 year ago

Reproduced on TestFlight version 1.3.44-2 as well.

aswin-s commented 1 year ago

@kevinksullivan Shouldn't this be reopened? It's a regression from https://github.com/Expensify/App/issues/20624

kbecciv commented 1 year ago

Issue is reproduced on build 1.3.45.3

https://github.com/Expensify/App/assets/93399543/9fd9b3eb-ff92-4156-bedc-6afbb2148a1a

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

kevinksullivan commented 1 year ago

Moving forward

tienifr commented 1 year ago

Proposal

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

NewDot opens and navigates to the corresponding report.

What is the root cause of that problem?

The problem in this line: https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/libs/actions/Report.js#L1716

After the navigation refactor, the deep linking already works out of the box, but in here we still keep the legacy logic where we manually handle the navigation to a specific report. That manual handling is not necessary now. This leads to the double navigation and the visual issue.

The issue happens after this PR. It causes a forced replacement of the current report screen, leading to the double navigation:

  1. First navigation is the out-of-the-box navigation from react-navigation
  2. Second is the forced replacement

If we don't have the change in that PR (adds 'UP'), this issue will not happen because in 2, it detects that we're already on that screen, then the second navigate will do nothing.

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

We just need to remove this code block: https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/libs/actions/Report.js#L1715-L1717

There'll be no regression since the deep linking is already handled out-of-the-box by the navigation library. I've tested with all these flows on iOS, Android, web, desktop and all works well:

  1. Deeplinking to a report while logged in
  2. Deeplinking to a report while logged out
  3. Deeplinking to a report while logged in and another report is opened
  4. Deeplinking to a report while logged in and a random screen (Settings/Workspace/...) is opened

What alternative solutions did you explore? (Optional)

We can revert the change in this PR, I've tested and it doesn't cause that issue again. But this is still unnecessary code so I prefer to remove it completely and let react-navigation handle it.

abdulrahuman5196 commented 1 year ago

Reviewing today

abdulrahuman5196 commented 1 year ago

Sorry wasn't able review that time. Will give a review in my morning

tienifr commented 1 year ago

@abdulrahuman5196 just FYI there has been some discussions on my above proposal in here. A few contributors have tested it and seem to find no issue.

aswin-s commented 1 year ago

I can still reproduce this issue on latest TestFlight build 1.3.50

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @sophiepintoraetz (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

kevinksullivan commented 1 year ago

Hi @sophiepintoraetz I am going OOO for the week so tapping you in the interim. Thanks!

abdulrahuman5196 commented 1 year ago

On @tienifr 's solution here https://github.com/Expensify/App/issues/23441#issuecomment-1656725230 I tested the solution and it seems to be working fine.

But I need to confirm on this root cause

After the navigation refactor, the deep linking already works out of the box, but in here we still keep the legacy logic where we manually handle the navigation to a specific report. That manual handling is not necessary now. This leads to the double navigation and the visual issue.

Tagging @WoLewicki / @mountiny as well to see if this is true about navigation refractor. Even in the other PR which this is mentioned https://github.com/Expensify/App/issues/20624 also says we are seeing this issue recently.

Meanwhile @tienifr , could you also share more information if you have any on the root cause? And if we are saying manual handling is not required, then openReportFromDeepLink function itself is not required, is it?

mountiny commented 1 year ago

@abdulrahuman5196 I swear we have tested this before and it was fine so possible it might be a regression from something lately.

@WoLewicki @abdulrahuman5196 any ideas for the proposed fix or where this could be coming from?

melvin-bot[bot] commented 1 year ago

@kevinksullivan @sophiepintoraetz @abdulrahuman5196 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!

WoLewicki commented 1 year ago

And if we are saying manual handling is not required, then openReportFromDeepLink function itself is not required, is it?

Yeah, we could check if removing this flow destroys any behaviors. I haven't participated in implementing this part of refactor, so I won't say for sure that it will work correctly then.

tienifr commented 1 year ago

And if we are saying manual handling is not required, then openReportFromDeepLink function itself is not required, is it?

@abdulrahuman5196 no, it's still needed to go to the Concierge route correctly, since we have to translate the /concierge route to the /r/[Concierge report id] route, so it's not supported out of the box by the navigation (the navigation does not know that route translation)

tienifr commented 1 year ago

Meanwhile @tienifr , could you also share more information if you have any on the root cause?

@abdulrahuman5196 I see all other routes are working without any manual intervention (/settings, /settings/workspaces, ...).

@abdulrahuman5196 I swear we have tested this before and it was fine so possible it might be a regression from something lately.

It seems to happen after this PR, but there might be other changes occurring at the same time.

sophiepintoraetz commented 1 year ago

Not overdue - still discussing solutions.

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

I did some testing and have some information.

1) This issue is not reproducible if you are already on chat page and click on deeplink

https://github.com/Expensify/App/assets/46707890/9b44545a-54fc-4593-9fc9-4a77c94bd098

2) Issue is reproducible if you are already on the home page and click on deeplink

https://github.com/Expensify/App/assets/46707890/90b59e7b-918c-4f48-8e86-332378ad9195

3) It should be a regression from this PR - https://github.com/Expensify/App/issues/20624 I tested removing that change PR and I am unable to reproduce this issue. On investigating the videos of the PR, the videos where taking from chat page in which cases this animate twice bug is not seeing. And the video is stop as soon as the page is opened, but on some cases it takes a second for the second page to animate. So it could have been missed in the PR.

abdulrahuman5196 commented 1 year ago

And @tienifr could you kindly confirm if we will not bring this bug https://github.com/Expensify/App/issues/20624 back since we are actually remove the change from there itself.

And we don't have solid rootcause on why this is happening, if possible could you check on the above information and see if we can piece the root cause together?

tienifr commented 1 year ago

And @tienifr could you kindly confirm if we will not bring this bug https://github.com/Expensify/App/issues/20624 back since we are actually remove the change from there itself.

@abdulrahuman5196 yes I've tested and we will not bring up that bug again, and it's also confirmed by the C+ there.

tienifr commented 1 year ago

And we don't have solid rootcause on why this is happening, if possible could you check on the above information and see if we can piece the root cause together?

@abdulrahuman5196 I've clarified the root cause a bit more and also describe the context linked to the other PR. Let me know if anything else is needed, thanks!

mountiny commented 1 year ago

@tienifr @abdulrahuman5196 can you check if its repro on this branch build? https://github.com/Expensify/App/pull/22437

tienifr commented 1 year ago

@mountiny yes, it's still happening on that branch.

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

@tienifr @abdulrahuman5196 can you check if its repro on this branch build? #22437

Are we trying to fix this there? Or something related we should wait on? @mountiny

mountiny commented 1 year ago

@abdulrahuman5196 do but its changing a bit the navigators structure so I though it might have helped

melvin-bot[bot] commented 1 year ago

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

kevinksullivan commented 1 year ago

@abdulrahuman5196 can you provide an update here?

abdulrahuman5196 commented 1 year ago

I am planning to approve @tienifr 's proposal here https://github.com/Expensify/App/issues/23441#issuecomment-1656725230

If there is no plan to fix this bug part of separate issue or this https://github.com/Expensify/App/issues/20624

cc: @allroundexperts @mountiny

mountiny commented 1 year ago

If there is no plan to fix this bug part of separate issue or this https://github.com/Expensify/App/issues/20624

Sorry can you please re-phrase this, I am not sure if I understood.

So if we go with @tienifr proposal it seems like it will introduce:

Issue is reproducible if you are already on the home page and click on deeplink

this could be an issue when you get a loud notification while at the app and you click on it, right? then the deeplink will navigate you twice

Anything else found so far?

tienifr commented 1 year ago

So if we go with @tienifr proposal it seems like it will introduce: Issue is reproducible if you are already on the home page and click on deeplink

@mountiny just to clarify this is what's happening currently, not something introduced by my proposal. My proposal is meant to fix it 😄

mountiny commented 1 year ago

What did @abdulrahuman5196 meant by:

If there is no plan to fix this bug part of separate issue or this https://github.com/Expensify/App/issues/20624

I think we could go with @tienifr proposal then, do you agree @abdulrahuman5196 ?

abdulrahuman5196 commented 1 year ago

@mountiny Sorry for the confusion. Let me clarify.

There were 2 parallel things going on related to this issue

1) This issue is most likely a regression from here https://github.com/Expensify/App/issues/20624 2) Some navigation structure change you had previously mentioned might fix this https://github.com/Expensify/App/issues/23441#issuecomment-1679355113

As far as the current status, I don't see both the above path leading to fixing this issue.

So my question was, can I proceed by treating/reviewing @tienifr 's solution (https://github.com/Expensify/App/issues/23441#issuecomment-1656725230) to fix this issue, instead of waiting on external paths? I just have to re-confirm, the proposal doesn't bring back this bug - https://github.com/Expensify/App/issues/20624. If that's successful, I am good to approving his proposal from my end.

mountiny commented 1 year ago

Perfect, thanks for the write up

melvin-bot[bot] commented 1 year ago

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

DanutGavrus commented 1 year ago

@abdulrahuman5196

Some navigation structure change you had previously mentioned might fix this

It might, because if I remove my PR change, no bug happens anymore(the one I fixed 1 month ago https://github.com/Expensify/App/issues/20624 & this new one https://github.com/Expensify/App/issues/23441). I have not been able to find the changes which fixed my original Issue as I've explained here, but we concluded to treat that as a regression as I could not find changes which could have broken my fix by adding this new issue either, thus it was most likely a regression.

If there is no plan to fix this bug part of separate issue or this https://github.com/Expensify/App/issues/20624

I've provided a fix for this new issue there. I've tried all 3 approaches: implementing that fix, removing my PR change & completely removing manual report navigation as it's now handled by the navigation itself(@tienifr's Proposal). I confirm that neither the old or this new bug happens with either of them. My fix just adds complexity, I prefer @tienifr's solution as it's much more elegant. Sibtain(C+ from my PR) also prefers @tienifr's approach as stated here. So I think there's no plan to fix that there.

melvin-bot[bot] commented 1 year ago

@kevinksullivan, @mountiny, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

kevinksullivan commented 1 year ago

Hi @abdulrahuman5196 , just checking in. Will you be able to check out the response above this week?