[HOLD for payment 2023-09-21] [HOLD] [$1000] Chat - LHN page displayed instead Legal name page when using deeplink

[kbecciv]

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. Login in the same account in 2 platform (mWeb/Safari and iOS app)
2. Go to Settings -> Profile -> Personal details -> Legal name -> Copy the URL of page (mWeb/Safari)
3. Paste URL in another program
4. Log out in iOS app
5. Open the URL that was copied in step 2
6. Login with same account

Expected Result:

App navigates user to the Legal name page

Actual Result:

LHN displayed instead of the Legal name page

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.46-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

RPReplay_Final1690559564.MP4

Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal team
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01327557008996edfb
• Upwork Job ID: 1688707539214979072
• Last Price Increase: 2023-08-08
• Automatic offers:
• mollfpr | Reviewer | 26045352

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Talha345]

@kbecciv I think the task description does not conform with the video atatched. Can you please recheck and clarify?

[jliexpensify]

Agree with @Talha345 . @kbecciv please re-test and attach the correct video and re-open this GH.

[kbecciv]

Hey @jliexpensify Checking

[kbecciv]

@jliexpensify Sorry about confusion, steps and video have been updated. Thank you

[jliexpensify]

@kbecciv sorry I am still confused: you mentioned that this is for the IOS app but you've also checked Android. So is this an iOS issue or an Android one?

[jliexpensify]

Bumop @kbecciv - is this an iOS issue only or for both platforms? Thanks.

[jliexpensify]

Can't reproduce on Pixel 3a (Android) - takes me to the legal page on 1.3.48-0.

@kbecciv please tag me if you can reproduce on iOS and share a video.

[puneetlath]

I just tested this on iOS and the deep link does not seem to be working.

[jliexpensify]

Thank you Puneet, seems to be an iOS issue only then.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

as a Bug Zero member managing payments to follow the processes, is that we wouldn't pay you

@jliexpensify this will greatly discourage any contributor from investigating hard issues if there's no leeway. Imagine if you invested hours, or even days into a hard issue and very close to the solution, then someone from an agency just commented on it, then all your efforts go wasted.

You actually helped them come up with the proposal, then I'd be inclined to split the payment 50/50.

@jliexpensify yeah I think the fact that the final solution is using pretty much the same root cause and solution as I suggested earlier would qualify as "helping", and I agree only partial compensation is ok in that case.

[mollfpr]

Thanks @mollfpr. Do you agree that the root cause #23745 (comment) is correct and the approach that is ultimately implemented in @kubabutkiewicz 's #26073 is the same as the one in my proposal?

It's not the same, but I guess your solution may inspire @kubabutkiewicz PR. I thought your RCA mentioned something wrong with our navigation structure, so I hope to find a solution.

In the listener here https://github.com/Expensify/App/blob/77ac9d18f3bc122c10b1baa7f566f37e51b3de9a/src/Expensify.js#L169C1-L169C1, check if isAuthenticated is false, and the URL is of a protected route, then store it in a ref deferredDeeplink.

This may be similar to what @kubabutkiewicz did in the PR, where they decided to replace SidebarUtils.isSidebarLoadedReady for better timing and use the authToken as the promise.

If the external Contributor was struggling and you actually helped them come up with the proposal

I think @kubabutkiewicz is the one who can answer this.

@jliexpensify this will greatly discourage any contributor from investigating hard issues if there's no leeway. Imagine if you #23745 (comment), or even days into a hard issue and very close to the solution, then someone from an agency just commented on it, then all your efforts go wasted.

@tienifr I know this feeling. Hopefully, the team can consider some compensation for you.

[hayata-suenaga]

wow a lot of conversations happening here

@mollfpr if you could, could you summarize what is being discussed and what needs to be decided?

[tienifr]

I thought your RCA mentioned something wrong with our navigation structure, so I hope to find a solution.

@mollfpr FYI I also mentioned here that our navigation structure is correct. In my RCA and solution, we just need to handle the use case of deferred deep linking, which happens to be exactly what's implemented in the PR.

[kubabutkiewicz]

If the external Contributor was struggling and you actually helped them come up with the proposal
I think @kubabutkiewicz is the one who can answer this.

When I was looking at this issue I wasn't inspired by what @tienifr, I just looked at his proposal when @mollfpr asked me and gave my thoughts.
The reason which @tienifr suggested as a root cause was actually good but the solution which was suggested I thought there can be better, because of introduction of new state thats why I said this looks like a workaround.

[tienifr]

because of introduction of new state thats why I said this looks like a workaround.

@kubabutkiewicz To clarify it's not a new state, it's a ref, which doesn't have any performance downside.

@kubabutkiewicz do you agree the approach in the PR is pretty much the same as suggested in my proposal? which is:

• Wait until the user is logged in
• Then navigate to the deep link

Also, your initial implementation uses the SidebarUtils.isSidebarLoadedReady, which is slow as I highlighted here, then after that you/or @lukemorawski updated the implementation to wait for sign in instead (via this commit), which ends up being what I suggested initially.

If all of that are not any inspiration, I don't know what is 😄

cc @mollfpr @hayata-suenaga

[kubabutkiewicz]

@tienifr
What I meant is that I did not suggest by your proposal and just did my own.
So your investigation was good.
That's all.
I have finger crossed that you will be compensated 😄

[hayata-suenaga]

@mollfpr did the PR used most of @tienifr's suggestion?

@jliexpensify If this is the case, we can do partial compensation for @tienifr. I remember I read somewhere that there is an instruction on what to do when an internal engineer takes a contributor's proposal and implement it themselves. I think we can apply the same guideline here for the payment amount.

[mollfpr]

@hayata-suenaga The idea is the same, but the implementation is quite different.

[jliexpensify]

Hi @tienifr - I've spoken privately to @mollfpr and @hayata-suenaga to understand the situation presented here, and here's a summary of our joint decision:

• Firstly, we appreciate that you spent time on this issue, but it’s in the CONTRIBUTING.md that as soon as an agency comments, we’ll assign to them. This is the same for any issue out there.

• Similarly, we appreciate that you had a proposal first, but it wasn’t accepted. @mollfpr also confirmed with @kubabutkiewicz that their PR was not inspired by your solution. It has some overlap, but ultimately didn’t solve for the root cause.

• To close off this issue, we feel it's fair to offer a one-time payment of $250 to you - to recognise there is some overlap.

To be transparent, we all feel like this debate over compensation went on for too long and felt like we adequately explained our reasoning to you: we definitely appreciate you have worked on a proposal, but we're also bound by the processes set out in the .MD and by our wider team. It would be unfair to bend the rules for one contributor and goes against Rule #2 - Don't Ruin It For Everybody.

I will also take this opportunity to remind you that even though a proposal is posted, any issue can be ultimately made Internal or the proposal posted not be considered/used.

I'll go ahead and create an Upworks job and arrange for a payout to you.

[tienifr]

@jliexpensify thanks for your consideration 🙇 I'm happy with that.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix/deeplinks when app in background #26073

If no regressions arise, payment will be issued on 2023-09-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @kubabutkiewicz does not require payment (Contractor)
• @mollfpr requires payment offer (Reviewer)
• @lukemorawski does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@mollfpr] Determine if we should create a regression test for this bug.
• [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Bump @mollfpr to complete the checklist!

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#15788

[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/15788/files#r1330344843

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Make sure the app is in the background mode (only for iOS or Android)
2. Make sure you are logged out
3. Open the app from whatever deep link
4. Log in
5. You should be redirected to the screen that you provided in the deep link
6. 👍 or 👎

[mollfpr]

Friendly bump @jliexpensify

[jliexpensify]

Thanks for the bump - payment summary:

• Contributor: @lukemorawski $1000 (Invoicing), according to the PR. It doesn't look like @kubabutkiewicz worked on it.
• Contributor: @tienifr $250 (special payment, discussed here)
• C+: @mollfpr $1000

Upworks job

Am I missing anything?

[mollfpr]

@jliexpensify I already have the Upwork contract for this issue.

[jliexpensify]

Yep, I'm aware @mollfpr - I generally do a payment summary for every job now, to make sure that I'm paying the correct amounts. Will pay via Upworks now.

[jliexpensify]

All paid, job closed