Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On internal test builds, the staging icon can be present #13993

Closed
1 task
kbecciv opened this issue Jan 4, 2023 · 31 comments
Closed
1 task

On internal test builds, the staging icon can be present #13993

kbecciv opened this issue Jan 4, 2023 · 31 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jan 4, 2023

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. Open the App
  2. Login with any account

Expected Result:

"stg" icon is present in Android app

Actual Result:

No "stg" icon in Android app

Workaround:

Unknown

Platforms:

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

  • Android / Chrome

Version Number: 1.2.47.0

Reproducible in staging?: Yes

Reproducible in production?: n/a

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

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/~017bf40c081569d3b2
  • Upwork Job ID: 1610756792600559616
  • Last Price Increase: 2023-01-04
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 4, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 4, 2023
@kbecciv
Copy link
Author

kbecciv commented Jan 4, 2023

Issue found when executing special request #13967 (comment)

@mvtglobally
Copy link

@AndrewGable tagging you as well as requested

@jliexpensify
Copy link
Contributor

jliexpensify commented Jan 4, 2023

@mvtglobally I logged into staging on Android and am seeing this:

Screenshot_20230105-083725

My version is 1.2.48-2

@mvtglobally
Copy link

@jliexpensify the issue is ONLY happening with the build #13967 (comment). This is a separate build requests

@AndrewGable
Copy link
Contributor

I can confirm this is a bug, we need to think of how to handle ad hoc requests versioning and badges. cc @roryabraham

@AndrewGable AndrewGable added the Internal Requires API changes or must be handled by Expensify staff label Jan 4, 2023
@jliexpensify
Copy link
Contributor

Sorry, I misread! Going to unassign myself then, as it's Internal, not External.

@jliexpensify jliexpensify removed their assignment Jan 4, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 9, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

@AndrewGable Huh... This is 4 days overdue. Who can take care of this?

@AndrewGable AndrewGable added Weekly KSv2 and removed Daily KSv2 labels Jan 10, 2023
@melvin-bot melvin-bot bot removed the Overdue label Jan 10, 2023
@AndrewGable AndrewGable changed the title Android - No "stg" icon in Android app On internal test builds, the staging icon can be present Jan 10, 2023
@roryabraham
Copy link
Contributor

roryabraham commented Jan 10, 2023

Some background... The "environment badge" refers to a few different things:

  • On web and desktop, the correct logo is injected into the splash screen via webpack here. This logo is consumed here and the svg is inlined in the splash screen DOM directly here. The source of truth for which logo to use comes from the --envFile cli argument passed to webpack, and if that's missing we presume that the environment is dev.
  • On the sign in page, we render the no-longer-aptly-named ExpensifyCashLogo component right here. The source of truth for the correct environment of that logo comes from the withEnvironment HOC
  • On all platforms, the Header component can render the EnvironmentBadge component, which also uses withEnvironment as it's source of truth.
  • withEnvironment in turn uses the Environment lib, which:
    • On web, relies on the react-native-config and the dotenv file as a source of truth
    • However, on native, the same build that was once a staging build becomes a production build later on. So .env.staging isn't used for native platforms at all AFAIK. Instead, we use the betaChecker library to determine whether the app is a staging or production build at the time.
      • On iOS, betaChecker uses a custom native module to determine whether it's a TestFlight build. On the native side, the source of truth is the appStoreReceiptURL (source code)
      • On Android, betaChecker just fetches the latest GitHub release and determines that the app is a beta app if it's newer than the latest production release. An interesting side-effect of this is that if you're using a beta version of the android app and we do a GitHub release and you don't update, then that "staging app" would become a "production app".

So I think it's fair to say that this "environment checking" and the associated badges and such are a little all-over-the-place. There are at least three sources of truth:

  • The --envFile CLI param passed to webpack (react-native-config later just consumes this source of truth in the main JS bundle)
  • The appStoreReceiptURL on iOS can tell us if the app was downloaded via TestFlight or the App store
  • We couldn't identify an Android equivalent of appStoreReceiptURL, so we use GitHub releases as a third source of truth

@roryabraham
Copy link
Contributor

So evaluating these sources of truth:

  • On web, we could create a separate environment file for ad-hoc builds. The downside is that we'd be introducing a new environment file that we have to maintain, and that environment file would be different from the staging environment. In general I think we want these ad-hoc builds to be as similar to staging builds as possible.

  • Another solution on web would be to just look at document.URL like so:

    /**
     * Returns a promise that resolves with the current environment string value
     *
     * @returns {Promise}
     */
    function getEnvironment() {
        return new Promise((resolve) => {
            if (document.URL.startsWith('https://new.expensify.com')) {
                return resolve(CONST.ENVIRIONMENT.PRODUCTION);
            }
            
            if (document.URL.startsWith('https://staging.new.expensify.com')) {
                return resolve(CONST.ENVIRONMENT.STAGING);
            }
            
            if (document.URL.includes('pr-testing.expensify.com')) {
                return resolve(CONST.ENVIRONMENT.AD_HOC);
            }
            
            return resolve(CONST.ENVIRONMENT.DEV);
        });
    }

    The PRO is that it works without introducing another env file. The CON is that it won't work during the webpack build phase for the splash screen icon, so we'd be introducing another source of truth without eliminating the old one.

  • I assume on iOS that the appStoreReceiptURL would be different or undefined when the app wasn't installed via the App Store OR TestFlight, but need to test.

  • On Android it looks like we can detect programmatically whether the app was installed via the Play Store or not. So that seems like it would work to distinguish between ad-hoc and staging/production builds.

@AndrewGable
Copy link
Contributor

BTW I was thinking we probably should handle this with the versioning too, it's really hard to tell on a mobile device what ad-hoc build you are testing.

So we could:

  • Set version to the PR you are testing, e.g. 0.0.0.PR-Number
  • Add a local check for version 0.0.0 if so, display the ad-hoc badge

Thoughts?

@roryabraham
Copy link
Contributor

That sounds like a good solution for ad-hoc builds. IDK if we should do 0.0.0 or just the version number from package.json though 🤷🏼

@AndrewGable
Copy link
Contributor

I think the only reason I was suggesting 0.0.0-PR so we can easily determine locally if it's an ad-hoc or not. Otherwise I think we are going to have to rely on some external API

@roryabraham
Copy link
Contributor

Yeah, but we could also use 1.2.52-PR.13755 or whatever. That way the about page can still use the version that the PR is based on 🤷🏼

@AndrewGable
Copy link
Contributor

AndrewGable commented Jan 11, 2023

Ah so you are suggesting to use "PR", can a version be non-numeric (I'm not sure)? My suggestion was #13967 would generate 0.0.0-13967 (or 0.0.0.13967)

@roryabraham
Copy link
Contributor

Ah so you are suggesting to use "PR"

Yeah, I thought that's what you were suggesting 🙂

can a version be non-numeric (I'm not sure)?

I think so, since in our React Native fork we use 0.71.0-alpha.2 and such

@AndrewGable
Copy link
Contributor

AndrewGable commented Jan 11, 2023

@situchan
Copy link
Contributor

Is this still reproducible in latest staging android build?
I think this has the same root cause as #14047.
The logic of checking staging/production is different per platform.
In android, Github release version is checked.

These are possible cases:

  • Github release version is same or newer than app version
  • network offline or not stable when open the app so it fails to fetch github release version info

@AndrewGable
Copy link
Contributor

Not quite the same. These are released outside of the app store and will require different logic.

@melvin-bot melvin-bot bot added the Overdue label Feb 24, 2023
@AndrewGable
Copy link
Contributor

Still in holding pattern.

@melvin-bot melvin-bot bot removed the Overdue label Mar 2, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 10, 2023
@AndrewGable
Copy link
Contributor

Still an issue, if I do not get help here I will fix myself!

@melvin-bot melvin-bot bot removed the Overdue label Mar 20, 2023
@staszekscp
Copy link
Contributor

I can take it over :)

@staszekscp
Copy link
Contributor

Hey!
After giving the task some thought I decided to go for a simple solution, that wouldn't require adding a separate environment to the project, and internal builds would be easily detectable.

During the testBuild workflow a PULL_REQUEST_NUMBER will be added to the .env.staging file, and later adapted by react-native-config to use it in the app and display correct badge near Chats. The builds are run in the staging environment, and the PULL_REQUEST_NUMBER env will be an indicator, that the app was built from a PR.

This way we won't risk playing with environmental config of the app lowering the risk, and the main problem will be solved :)

@Julesssss Julesssss self-assigned this Apr 11, 2023
@Julesssss
Copy link
Contributor

Going to help out with this one as it's linked to the additional build flavours issue: #17265

@staszekscp
Copy link
Contributor

Hey! I'm leaving short instruction how to build the app from correct workflow in order to test this PR

  1. Checkout to my branch, and push your own version of it to the repo
  2. Go to Actions and than navigate to Build and deploy apps for testing
  3. Run workflow - use workflow from your newly pushed branch, and as input choose my PR’s number (16735)

@Julesssss
Copy link
Contributor

Following the test steps here

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 21, 2023
@melvin-bot melvin-bot bot changed the title On internal test builds, the staging icon can be present [HOLD for payment 2023-04-28] On internal test builds, the staging icon can be present Apr 21, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 21, 2023
@MelvinBot
Copy link

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

@Expensify Expensify deleted a comment from MelvinBot Apr 25, 2023
@Julesssss
Copy link
Contributor

Deleting bugZero checklist.

@Julesssss Julesssss changed the title [HOLD for payment 2023-04-28] On internal test builds, the staging icon can be present On internal test builds, the staging icon can be present Apr 25, 2023
@Julesssss
Copy link
Contributor

Closing this out. PR was merged successfully 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants