App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Clicking Take me there doesn't automatically sign user in to New Expensify

Open m-natarajan opened this issue 1 year ago β€’ 27 comments

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.66-0 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: @quinthar Slack conversation (hyperlinked to channel name): Expensify-migrate

Action Performed:

  1. Submit a report to a workspace approver
  2. In OD open the report
  3. Click on "take me there"

Expected Result:

User logged in to ND with automatically signed in

Actual Result:

User is taken to login screen

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
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/d0833a5e-f68f-49ae-aaac-b0a591300fa4

https://github.com/user-attachments/assets/5637799d-c58a-442c-98b5-4f752965d860

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021861182667807693472
  • Upwork Job ID: 1861182667807693472
  • Last Price Increase: 2024-12-09
Issue OwnerCurrent Issue Owner: @rojiphil

m-natarajan avatar Nov 23 '24 19:11 m-natarajan

Triggered auto assignment to @joekaufmanexpensify (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.

melvin-bot[bot] avatar Nov 23 '24 19:11 melvin-bot[bot]

Reproduced.

joekaufmanexpensify avatar Nov 25 '24 22:11 joekaufmanexpensify

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

melvin-bot[bot] avatar Nov 25 '24 22:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 25 '24 22:11 melvin-bot[bot]

This is an issue on the Old Dot. Take me there button is pointing to the wrong environment url, it should point to staging.expensify.com instead it's point to new.expensify.com/expenses

klajdipaja avatar Nov 26 '24 09:11 klajdipaja

Reproduced.

@joekaufmanexpensify I too can reproduce this but only if the user has not already signed-in in ND. And if the user has already signed-in to ND, the ND will take us to the concerned report. So, this seems to be working as per the existing design.

Or are we talking about implementing a new feature here i.e. auto sign-in even when the user is not signed in?

rojiphil avatar Nov 26 '24 12:11 rojiphil

@rojiphil Are you seeing somewhere that it's expected with the current design that we don't support the case of transferring you from OldDot > NewDot without explicitly signing into NewDot?

We definitely support the reverse now, so I'd think we'd support this too.

joekaufmanexpensify avatar Nov 26 '24 20:11 joekaufmanexpensify

@rojiphil Are you seeing somewhere that it's expected with the current design that we don't support the case of transferring you from OldDot > NewDot without explicitly signing into NewDot?

We definitely support the reverse now, so I'd think we'd support this too.

Yeah. I too agree that the transferring from OD > ND should be supported. I was just confirming if this was previously implemented or is a new feature. Anyway I think we need BE support as OD must invoke the URL with auth credentials as params following which we can set the session data locally to automatically sign in. Trigger the involvement of an internal engineer here to take this forward. πŸŽ€πŸ‘€πŸŽ€

rojiphil avatar Nov 27 '24 17:11 rojiphil

Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Nov 27 '24 17:11 melvin-bot[bot]

But I think we need BE support as OD must invoke the URL with auth credentials as params following which we can set the session data locally to automatically signin.

@marcaaron But there are also references to signing in from OD here. So, curious to know if this feature is supposed to work with Hybrid Apps. What do you think?

rojiphil avatar Nov 27 '24 17:11 rojiphil

I was just confirming if this was previously implemented or is a new feature.

I think this is a good distinction. @joekaufmanexpensify maybe you can help figure out who has the most context around this new feature (or is it a bug)? It will be some time before I can look into this. I see it's labeled as a "HIGH" in the #migrate project (maybe someone has bandwidth).

marcaaron avatar Nov 28 '24 01:11 marcaaron

I was just confirming if this was previously implemented or is a new feature.

I think this is a good distinction. @joekaufmanexpensify maybe you can help figure out who has the most context around this new feature (or is it a bug)?

Thanks @marcaaron for your input. Digging a little deeper, I think it may be as simple as launching the URL from OD with the auth credentials to solve this issue. @joekaufmanexpensify Once we involve a context-aware BE engineer, I also think we will have a solid way forward to resolve the issue.

rojiphil avatar Nov 28 '24 05:11 rojiphil

πŸ“£ 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] avatar Dec 02 '24 16:12 melvin-bot[bot]

Sure, happy to figure out whether this is a bug or new feature. Discussing.

joekaufmanexpensify avatar Dec 02 '24 16:12 joekaufmanexpensify

Sounds like the case got missed in the original implementation. We agreed in Slack we def want to handle this though.

joekaufmanexpensify avatar Dec 02 '24 16:12 joekaufmanexpensify

Sounds like the case got missed in the original implementation. We agreed in Slack we def want to handle this though.

@joekaufmanexpensify Thanks for the clarification. Can we also involve an internal engineer who has context to this feature? More specifically, I am curious to know the feedback on the following comment:

Digging a little deeper, I think it may be as simple as launching the URL from OD with the auth credentials to solve this issue.

rojiphil avatar Dec 02 '24 16:12 rojiphil

It was @puneetlath who originally implemented it. He'll have more context if you have any questions. Though, as far as who will work on it, if Marc ends up having the bandwidth that is great, otherwise, I'll try and source another volunteer.

Just to confirm @rojiphil, this one will be an entirely backend issue?

joekaufmanexpensify avatar Dec 03 '24 16:12 joekaufmanexpensify

Just to confirm @rojiphil, this one will be an entirely backend issue?

Looks like a BE issue to me but it depends on the feedback for the comment here. @puneetlath What do you think?

rojiphil avatar Dec 04 '24 08:12 rojiphil

@rojiphil @marcaaron @joekaufmanexpensify 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!

melvin-bot[bot] avatar Dec 07 '24 10:12 melvin-bot[bot]

@rojiphil, @marcaaron, @joekaufmanexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Dec 09 '24 09:12 melvin-bot[bot]

πŸ“£ 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] avatar Dec 09 '24 16:12 melvin-bot[bot]

Looks like a BE issue to me but it depends on the feedback for the comment https://github.com/Expensify/App/issues/53034#issuecomment-2512138592.

Got it. I think any engineer can probably confirm this. Curious if @marcaaron has thoughts on whether Roji's solution is on the right track? I also asked Puneet if he has time to take a quick look in Slack too.

joekaufmanexpensify avatar Dec 09 '24 16:12 joekaufmanexpensify

I agree, I think we need to have an internal engineer update the link to use the auth credentials.

puneetlath avatar Dec 09 '24 16:12 puneetlath

Cool. Now that we have a consensus on the way ahead, I think we can make this issue as internal. Meanwhile, I am unassigning myself here but will stay subscribed if any FE work arises. Thanks. cc @joekaufmanexpensify

rojiphil avatar Dec 09 '24 17:12 rojiphil

Yeah ok I see we did this: https://github.com/Expensify/Web-Expensify/pull/41884

So we just need to ... make that a short lived auth token link instead I think and this should work. Seems kind of half implemented.

marcaaron avatar Dec 10 '24 01:12 marcaaron

Ok repro'd and made a PR for this. Was a one line change πŸ₯²

marcaaron avatar Dec 10 '24 02:12 marcaaron

Sounds good. Thanks Marc!

joekaufmanexpensify avatar Dec 10 '24 13:12 joekaufmanexpensify

Fix is out on prod. All set here. Thanks again Marc!

joekaufmanexpensify avatar Dec 16 '24 15:12 joekaufmanexpensify