[HOLD for payment 2022-12-20] [$500] Allow native app releases to take staging env variables into consideration

[mountiny]

Problem

In this issue #10163, we have implemented a setting toggle which will aim all the API commands to staging or production API (if in Staging App). However, this works only in desktop or web, but not in native mobile apps as the live logs shown.

The reason it does not work is that we always build the App only once for deploy. We just relabel staging release for the production one and so when building the staging App we use the production env variables causing this issue

Actual behaviour

1. On native mobile App (iOS and Android), the staging API toggle is turned on
2. The API requests are still made to production API endpoints

Expected behaviour

1. On native mobile App (iOS and Android), the staging API toggle is turned on
2. The API requests will be made to the staging endpoints same as on web app version

Why is it important

This will allow us to test staging changes with staging API at the same time.

Solution

Find a way how we can use the environment variables in native app so we can distinguish between staging and production to send the API requests to desired API endpoints.

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[mountiny]

I realized we have this Staging badge or dev badge even in the native apps so we must have some way to figure that out and the withEnvironment HOC

I am not sure if we can use those in the API toggle but would be worth exploring.

[melvin-bot]

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

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

[MitchExpensify]

Upwork Job

[parasharrajat]

No proposals yet

[smrutiparida]

We just relabel staging release for the production one and so when building the staging App we use the production env variables causing this issue

It looks difficult to me to imagine something while we continue to release the andorid/ios app by relabeling the staging release.

1. What I want to propose instead is to use https://github.com/luggit/react-native-config npm and issue the build commands like

   $ ENVFILE=.env.staging react-native run-ios //for staging and

   $ ENVFILE=.env.production react-native run-ios //for production

2. Another way can be reading variable from firebase remote config

// remote config with production version if same found in the build then 
// app points to PROD_URL for everything else STAGING_URL is set for testing. 
// We can improvise this logic.

const checkAppVersion = remoteConfig().getValue('app_production_version');

if (checkAppVersion.asString() === getBuildVersion()) {
  setEnvironmentAsProduction();
}
else 
{
  setEnvironmentAsStaging();
}

https://github.com/invertase/react-native-firebase/tree/main/packages/remote-config

I can try the same. Request you comment @mountiny @parasharrajat

[parasharrajat]

@smrutiparida We already do that. Please check the code and let us know if you feel something is off.

[parasharrajat]

@mountiny I think the issue details are not very clear. I don't understand the expected behavior. Can you please improve these?

[smrutiparida]

My second solution above can help the android code read a version no from a firebase remote config. Based on the version stored in remote, the API_URL can be configured dynamically within in the code. By doing so, the android build can be tested against staging URL for all versions higher than the remote version. It can also be released to PROD by only changing the label. @mountiny is that the expected thing ?

[mountiny]

@parasharrajat @smrutiparida I have added some lines to the issue body. The problem is that the native apps disregard the API endpoint settings, they always send requests to the production API even if the staging API toggle in settings in turned on.

I am unfortunately not that familiar with firebase remote config so I am not sure. Is there any way we could use this approach: #11561 (comment) to achieve this natively?

[parasharrajat]

Thanks for adding that details. It is a lot more clear to me now. Between staging and prod APIs, the only difference is the API URL. IMO, this should be easier to do. Just change the Url based on the setting irrespective of the ENV.

[smrutiparida]

@mountiny What I have understood so far is, we rename the label of a staging build to release to PROD. This is the reason API_URL present in the ENV values are as PROD_URL as there are no way presently for an iOS/Android android build to read the env values dynamically based on dev/taging/prod post the creation of a build.

@parasharrajat Which setting you mean ?

[mountiny]

This is not done yet, original PR has been reverted and the fix is being worked on.

[mountiny]

The PR has been updated and is ready for a review.

[parasharrajat]

I will take a look.

[trjExpensify]

@mountiny we're just waiting on a deploy here, right?

[parasharrajat]

Yeah, PR is merged.

[mountiny]

Yeah this is in staging and I have QAed and it worked at least from what I saw 🎉

[trjExpensify]

Noice! ❤️

[MitchExpensify]

Reassigning the Bug label as I'm heading ooo and this will need a new CM!

[melvin-bot]

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

[mountiny]

@kadiealexander Coming from this comment we should have a payment off:

• 500 to @smrutiparida for their efforts to fix this
• 1000 to @parasharrajat for reviewing and testing the PRs, internal and external

[kadiealexander]

Thanks @mountiny!!!! Is payment due now?

[mountiny]

I would say yes, this has been PR which was kind of untraditional in terms of the process. The internal PR should already be in production.

[melvin-bot]

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

• Enable native staging apps to use Staging API #13029

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

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:

• [@parasharrajat / @mountiny] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat / @mountiny] 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:
• [@parasharrajat / @mountiny] 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:
• [@kadiealexander] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[kadiealexander]

Thanks @mountiny! New job post is here, I've already sent hire contracts to @parasharrajat and @smrutiparida, please let me know once you've accepted and I'll issue payment.

ETA: @mountiny correct me if I'm wrong, but the problem behind this issue doesn't look like it can be added to TestFlight for regression testing? Just confirming before I take action on the checklist above.

[mountiny]

ETA: @mountiny correct me if I'm wrong, but the problem behind this issue doesn't look like it can be added to TestFlight for regression testing? Just confirming before I take action on the checklist above.

Yeah I dont think think Applause could test the native apps which is the main issue here

[mountiny]

Per the checklist, commented on this PR, it was not really a regression as much as not fully working new feature. We had to internally test for native apps but we did not do that at the time and we only tested web as that was easier. It worked there and assumed ti will work across the platforms

I dont think this requires any updates to the checklist.

[kadiealexander]

Awesome! Everyone is paid, so I'm closing this one out.