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.55k stars 2.89k forks source link

[$250] iOS Hybrid - Lack of debug mode in the settings - Troubleshoot for IOS Hybrid app #50843

Closed lanitochka17 closed 1 day ago

lanitochka17 commented 4 weeks 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!


Version Number: 9.0.49-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: N/A Email or phone of affected tester (no customers): applausetester+en@applause.expensifail.com Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/50519

Action Performed:

  1. Open the Expenisfy Hybrid app
  2. Sign into a valid account and access the ND features within the app
  3. Go to Account section > Troubleshoot > Scroll down

Expected Result:

User expects to see the Debug option

Actual Result:

No Debug option is shown

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/84c7107a-885a-4e09-82e1-997569a3133e

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021846335027010160243
  • Upwork Job ID: 1846335027010160243
  • Last Price Increase: 2024-10-29
Issue OwnerCurrent Issue Owner: @situchan
melvin-bot[bot] commented 4 weeks ago

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

lanitochka17 commented 4 weeks ago

@greg-schroeder FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

layacat commented 3 weeks ago

Proposal

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

Lack of debug mode in the settings - Troubleshoot for IOS Hybrid app

What is the root cause of that problem?

We always return Production for HybridAppModule here: https://github.com/Expensify/App/blob/d77a4fde57783ffb8d2443ae4394aecb8e55c4d4/src/libs/Environment/getEnvironment/index.native.ts#L33-L36 That's the RCA.

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

We should remove this block of code:

 if (NativeModules.HybridAppModule) { 
     environment = CONST.ENVIRONMENT.PRODUCTION; 
     return; 
 } 

What alternative solutions did you explore? (Optional)

N/A Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

situchan commented 3 weeks ago

can we allow debug mode in hybrid app? tagging hybrid app experts: @AndrewGable @staszekscp @mateuuszzzzz

AndrewGable commented 3 weeks ago

I believe we can, but let's confirm with @staszekscp and @mateuuszzzzz

situchan commented 3 weeks ago

https://github.com/Expensify/App/blob/04214cdb3e35816fd28a640f1b3c1b23bd7407b0/src/libs/Environment/getEnvironment/index.native.ts#L32-L36 @staszekscp @AndrewGable are we fine to remove this block, which reverts this PR?

staszekscp commented 3 weeks ago

@situchan We'll have to remove this code in order to fix the issue, but I think we'll have to do one more thing on the OldDot side of things to make it work correctly, I'll come back to you with an update

staszekscp commented 3 weeks ago

So, I'm back with an update πŸ˜„ On the Mobile-Expensify side, we'll have to link a Native Module called EnvironmentChecker, which is an easy fix. Combining it with removal of the code mentioned here should fix iOS.

Nevertheless we'll have a problem with Android. Currently we base our beta check on this line: https://github.com/Expensify/App/blob/main/src/libs/Environment/betaChecker/index.android.ts#L27. We get the latest release tag, and compare it with the version in package.json. If the versions match, we're on production, if the one in package.json is higher, we're on a beta build.

The problem is that the HybridApp's production version is different than the release NewDot version. Eg. currently we're on 9.0.47 on HybridApp, and 9.0.51 on NewDot. We cannot simply swap CONST.GITHUB_RELEASE_URL value to the Mobile-Expensify url, because it's not a public repo, and AFAIK would require additional authentication. Maybe we could create some dummy public repo that would keep that information only for HybridApp version so the only thing we have to do on the NewDot side is to swap a url? Or maybe to have some API endpoint to check the release version?

cc: @Julesssss

Julesssss commented 3 weeks ago

Thanks for the detailed explanation. I wonder why the debug tool is restricted by version instead of email, internal beta, etc... It seems necessarily complex, and restricts us from debugging prod. Though I suppose with external contributors its better to keep access open rather than locked down...

What if we locked the beta tools behind a hidden menu? For example, 8 taps of the version code could set an Onyx key that unlocks the debug menu.

One bad alternative might be to query the Google Play Store API and provide the Mobile-Expensify version via a backend command, though I recall the API being pretty bad and undocumented...

staszekscp commented 3 weeks ago

Yep, as far as I know there is no reliable way to check if a specific build is an Android Play Store Beta build 😒 I suppose that's the reason why the whole version checker tool has been implemented. I'd like to keep using this mechanism in order to introduce less changes on the NewDot side - the hidden menu may expose some options to regular users, that we would like to keep unavailable πŸ˜„ I'm not familiar with the whole debugging tools code, but we would have to revise it thoroughly first

Maybe we could change the release tag logic on GitHub so it would match only the HybridApp version, since we're going to deprecate NewDot in the nearest future?

melvin-bot[bot] commented 3 weeks ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

AndrewGable commented 3 weeks ago

I think we are planning on having them be in sync, but we need to change some processes internally before we can do so.

staszekscp commented 2 weeks ago

In this case should we wait until it's changed? I suppose we could easily enable debug mode for iOS (since it's based on a TestFlight link, not the version in package.json), and have it disabled for Android until we have releases of both repos in sync

Julesssss commented 2 weeks ago

It could be a while though. I would vote for simplify access to the HybridApp debug tools in the meantime.

AndrewGable commented 2 weeks ago

@puneetlath - Do you think we could solve this in a different way than looking at the versioning for the time being? Maybe we can include the debug tools for certain domains using the beta manager?

Julesssss commented 2 weeks ago

One concern about a beta is that it'll be a barrier that might prevent users/qa from providing logs due to the extra step. Also we'll have to constantly be adding contributors to the beta.

AndrewGable commented 2 weeks ago

Do you have any ideas for alternative solutions? My thought was we could use Beta manager as an additional way to add SWM + Callstack + Expensify domains to the beta manager for those who use HybridApp. Not necessarily just use beta manager exclusively.

puneetlath commented 2 weeks ago

Perhaps we can just make it available via the four-finger-tap/cmd+d on production. That way there's still a way to get to it on Android. Or if we ever wanted someone to on production for whatever reason.

Julesssss commented 2 weeks ago

...For example, 8 taps of the version code could set an Onyx key that unlocks the debug menu.

I think 4 finger tap or unlocking the menu item after x taps are both good simple solutions. I don't think we should care about users accidentally discovering these tools (granted with the open repo, its pretty easy for users to discover if they really wanted to).

AndrewGable commented 2 weeks ago

Perhaps we can just make it available via the four-finger-tap/cmd+d on production

I like this πŸ‘

puneetlath commented 2 weeks ago

I agree with that @Julesssss

staszekscp commented 2 weeks ago

Ok, cool! In that case the implementation will be HybridApp agnostic, and it can be done entirely on standalone NewDot. I suppose this kind of work will require some proposal, and general agreement on the solution, so I'll try to find someone on our side to take care of it! πŸ˜„

Julesssss commented 2 weeks ago

Sounds good @staszekscp, let's open a new issue rather than modifying this one. Let me know when you find someone to work on the task and I'll create one for you.

blazejkustra commented 2 weeks ago

Hey @Julesssss! I'm going to work on it. Tag me in the issue, and I'll start tomorrow morning πŸ˜„

Julesssss commented 2 weeks ago

Hey @blazejkustra could you comment here and I'll assign you, thanks!

melvin-bot[bot] commented 2 weeks ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

greg-schroeder commented 2 weeks ago

Okay I'm a bit confused on this one - who is going to take this one on? Or are we still waiting for a valid proposal?

Julesssss commented 2 weeks ago

I think we can close this once this issue is completed (its awaiting testing on prod)

greg-schroeder commented 1 week ago

Held til 2024-11-07

melvin-bot[bot] commented 1 week ago

@Julesssss, @greg-schroeder, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

Julesssss commented 6 days ago

Still held

Julesssss commented 4 days ago

The linked issue is closed. We didn't end up needing C+ review here.