[HybridApp][MEDIUM] Improvement to HybridApp TTI / NewDot authentication page

[Julesssss]

P/S authored by @staszekscp here

Problem

The TTI on HybridApp is longer than on a standalone NewDot application. In order to determine if a specific user should go to NewDot, HybridApp has to boot OldDot code first. The business logic for OldDot was written in JS in a framework called YAPL, and it runs on JSC.

Potentially also resolves:

• [HOLD #49844][$250] Hybrid - Android - LHN - Name changes to Email on expense information on LHN, when returning from OD #56685

Solution

Let’s replace JSC with Hermes! Hermes is known for its advantage over JSC in initial boot time, and it became a standard in the React Native world. Since YAPL is written in JS, theoretically should be able to do that, and get rid of JSC from the project. As bonus the app size would become smaller, too. 💪:skin-tone-2: 🚀

However, I’d like to emphasise that, since YAPL has been written in legacy JavaScript (mostly pre ES2015) there is a chance that the migration may become very time-consuming. Nevertheless I think it is worth to do at least a research, and see how far we can get, because it will speed up the both, OldDot, and HybridApp!

Mateusz' updated Solution

Adjust the current authentication flow on HybridApp to replace the OldDot sign-in page with the NewDot version.

1. Currently, we perform authentication on the OldDot side and pass all necessary credentials to enable the New Experience and handle authentication/re-authentication. We could reverse this by using the NewDot sign-in page to generate all required credentials. These credentials could then be used to authenticate on the OldDot side. To trigger authentication on YAPL side, native methods can be used.
2. We also need to decide at the start of sign-in whether the user should be redirected to OldDot or NewDot. The SignInUser command should return an additional field with nvp_tryNewDot, allowing us to determine based on the value of the dismissed key whether to stay on NewDot or transition to OldDot.
3. Another important aspect from the user’s perspective is error handling. OldDot initialisation might fail, so we should be able to display errors on the SignIn page, just as we do when NewDot authentication fails. We can track OldDot initialisation through native events and store the current state in Onyx. Errors returned by OldDot’s API can then be passed via these events and displayed on the NewDot SignIn page

[staszekscp]

Hey! With @WoLewicki we'll be working on this issue!

[Julesssss]

Work is ongoing.

[Julesssss]

In progress

[melvin-bot]

This issue has not been updated in over 15 days. @Julesssss, @staszekscp eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[Julesssss]

Making progress on the backend changes here.

[Julesssss]

Backend is now providing tryNewDot onyxData and in the response data in some cases which should unblock work here.

[Julesssss]

I'm fixing the incorrect NVP key here -- I'll share here when it hits staging/prod.

[Julesssss]

I'm fixing the incorrect NVP key here -- I'll share here when it hits staging/prod.

It hit production today

[Julesssss]

Hey @war-in, just checking in to see if this is still in progress?

[war-in]

Hey, yes! We're actively hunting bugs
@289Adam289 could you elaborate a bit? 🙏

[289Adam289]

Hi, the main sign-in flow works pretty good I am currently working on Onboarding. For some new accounts the onboarding flow doesn't start. Unfortunately, this behavior seems quite random.

Once we fix that I believe we can move on to other sign-in flows: SAML, Apple and Google.

[289Adam289]

Hi,
@Julesssss I've managed to solve most of the main flow problems, but still there are some bugs and questions that would require your attention.

1. I believe that backend returns wrong nvp_tryNewDot value for new accounts both created on ND and classic expensify:
   When creating new account BeginSignIn request returns:

{
        "onyxMethod": "merge",
        "key": "nvp_tryNewDot",
        "value": []
}

Same thing happens when trying to log in with account created on expansify.com. Because it's an empty array following (correct) onyx merge updates do not overwrite it.

2. In scenario:

• create account on classic web Expensify
• open hybrid app
• log in to the account
• We are immediately moved to OD

Should the Try New Expensify button be visible? I assume that it should because we want users to try the new app.

3. I am moving my focus to rest of the sign in flows. Is there any way to test Google and Apple flows without adhoc builds? Or do we have to relay on them.

cc @mateuuszzzzz @war-in

[Julesssss]

I believe that backend returns wrong nvp_tryNewDot value for new accounts both created on ND and classic expensify:

Ah so that is the correct value of the NVP, it is empty during beginSignIn and set when account is opened depending on the email domain and the site you are using to authenticate. It might remain empty or be set during signUpUser. This would be problematic to change.

2. We are immediately moved to OD

You shouldn't be redirected in this case, you will only be redirected to New Expensify from OldExpensify. However, if you manually navigated to OldDot the Try New Expensify button will only be displayed if the user has previously be set to the new experience. So if classicRedirect.dismissed is either true or false they will see the button. If the NVP is an empty array (as you saw above), then the user won't see the button (we only want some users to try new dot currently).

3. I think AdHoc build is the main way to test. Google SSO should work on AdHoc builds, but I'm not sure that Apple SSO is testable. I can look into this though. Presumably there is a method.

[289Adam289]

Ah so that is the correct value of the NVP, it is empty during beginSignIn and set when account is ...

Oh ok, I understand, but would it be possible to an change empty array to an empty object. I don't think it is possible to overwrite an empty array with an object in Onyx using merge. This causes the value of nvp_tryNewDot locally to be always equal to [], or at least until Onyx is cleared.

You shouldn't be redirected in this case, you will only be redirected to New Expensify from OldExpensify.

Do I understand correctly that new accounts (even created on expensify.com) should always go to ND?

[Julesssss]

Oh ok, I understand, but would it be possible to an change empty array to an empty object. I don't think it is possible to overwrite an empty array with an object in Onyx using merge.

Ah gotcha, yeah that should be fine. I'll change that soon.

Do I understand correctly that new accounts (even created on expensify.com) should always go to ND?

So this is changing frequently as we push more and more users to NewDot. Here is the most up to date logic:

All users signing up via NewDot will have the redirect NVP applied
Users signing up via OldDot on public domains will have the redirect NVP applied

[trjExpensify]

Users signing up via OldDot on public domains will have the redirect NVP applied

Mhm, is that accurate?

Anyone with any email that signs up on expensify.com and chooses individual or 1-10 is redirected into NewDot. Very soon though, that will be anyone who chooses 10+ as well I believe (CC: @chiragsalian). So all new sign-ups will go to NewDot, and then anyone identifying as 50+ in the onboarding modal will be sent back to Classic. I think this is the App issue to get it over the line: #60161

On HybridApp right now, all sign-ups go to the NewDot experience. Those that choose a 11+ option are being redirected back to Classic, but I think this will become 50+ as well soon per the above.

[chiragsalian]

I think this is the App issue to get it over the line: #60161

The issue to get it over the line is https://github.com/Expensify/Expensify/issues/490399. I'm just waiting for some other parts to get live before moving https://github.com/Expensify/Expensify/issues/490399 to live.

[trjExpensify]

Yeah cool, I'm not sure if this issue is talking about the mobile app only, and specifically. In which case, all mobile sign-ups are going into NewDot already -- we're just sending anyone that chooses one of the 10+ options in the companyStep back to Classic -- which I believe we'll stop doing when the accountingStep upgrade comes in?

[Julesssss]

This issue is regarding the HybridApp specific authentication page refactor. But good point Tom yeah, for mobile you are taken to NewDot for new accounts, BUT if you self select as a team with more than 10 users you'll be switched back to OldDot. I'll update this.

[Julesssss]

Progress ongoing.

[289Adam289]

Update:

After the recent changes, all known bugs have been fixed, and most of the sign-in flows are working. The sign-in page is in a very usable state. Currently, the fully working flows are:

• Standard flow (via the magic code)
• 2FA
• SAML (both single sign-in and magic code)

Everything seems to be working smoothly.

There are only two blocking issues left:

• Backend changes (changing [] to {} when nvp_tryNewDot is empty). I believe @Julesssss will be working on it.
• Testing and fixing the Apple and Google sign-in flows. From what I know, we have to rely on AdHoc builds for tests. I would appreciate any tips on how to improve the development process. I've read the contributing guides, unfortunately, there is not much information about Android and iOS builds.

Once we take care of that, we can move on to review and hopefully merge it soon.

cc @war-in @mateuuszzzzz @Julesssss

[289Adam289]

Hi,
On more update on our side.

I've worked on Google and Apple flow configuration with @mateuuszzzzz. We have reused OD configuration which should work properly. At least on release builds. Unfortunately, not every flow works with AdHoc builds even on main, which makes it very hard to verify that the configuration provided by us is correct. In current state there are no errors returned by Google or Apple flows. The only error we get is I suspect returned from BE. The message is as follows:

Cannot get account details, please try again or contact [email protected]

It's problematic to tell whether it's caused by wrong token format or some BE issue.

Is it possible for internal engineers to maybe test the changes on local signed release build and verify the configuration is correct? A lot of the changes in this pr are in native code that runs before we even get information from API. That makes it impossible to hide them behind the beta.

Testing both flows and BE issue with [] are the main blokers right now. Apart from that PR is really close to be ready for review.

[289Adam289]

This PR is very complex and changes code in multiple flows. It would be great to test this pr before we merge it. It would minimize the number of deploy blokers and bugs found later in development.

@Julesssss Looking forward to hearing your thoughts.

[Julesssss]

Hey, I just got back from holiday. Nice progress 🎉

It's problematic to tell whether it's caused by wrong token format or some BE issue.

I'm certain that Google SSO used to work on Android AdHoc builds and I can see they do not with the current AdHoc build on your PR, so hopefully it is a new BE issue I can help with. Apple will be trickier, I'm not sure we ever had a real method of testing this.

Backend changes (changing [] to {} when nvp_tryNewDot is empty)

I'll work on this next. I just need to catch up with some things first.

[289Adam289]

@Julesssss I've installed Android AdHoc build from a different PR and I wasn't able to login using Google flow.

Let me know if theres anything more I can do to move this pr forward.

[Julesssss]

I've installed Android AdHoc build from a different PR and I wasn't able to login using Google flow.

Alright thanks, I think it broke recently so I'll take a look at this next. Today I submitted the PR which make the NVP default to an empty object. It should be live in a few days.