-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Ensure Staging environment badge is shown for beta builds on iOS and Android #2647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Objective-C is pretty foreign to me, but overall looks good!
Going to put this on WIP to include the improvements mentioned in #2597 (comment) |
Updated this to add an Android check and move the environment value into Onyx, this is ready for another review! |
Looks like there's a conflict in |
fetch(CONST.PLAY_STORE_URL) | ||
.then(res => res.text()) | ||
.then((text) => { | ||
const match = text.match(/<span[^>]+class="htlgb"[^>]*>([-\d.]+)<\/span>/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could easily break if Google decides to change the name of the class. Can't think of anything better though, I did a quick search and it doesn't seem like they have an API to query an app's version number.
Maybe it's overkill but could we store the version number somewhere (tbd where) when we make a new release, and then make a new API command on expensify.com to retrieve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was something I thought of doing as well since it is one of the only other options I found. I ultimately decided against it since that HTML hasn't been updated in >5 years, and even if it was updated it'd be easy to update the code. Also, having the version somewhere means we have to update expensify.com (or wherever we store that value) every time we deploy the app to production, which seemed like overkill since Google is already giving us that data on the play store page. If we think that's a better solution though, I can have a look at implementing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, good to know it hasn't changed in over 5 years. And yes, it should be easy to update if they change the structure of the play store page.
Updated to remove the unnecessary catch and boolean |
Updated to DRY the code a bit and address the review comments, added one question but otherwise this is good for another review! |
Thanks for the review @tgolen, updated again to resolve the last outstanding comment |
@tgolen @roryabraham Updated to add the ref forwarding, please let me know if there's anything I missed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thanks for that!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@roryabraham will you please review this?
Details
Since we use the same builds for staging and production, both staging and production builds on iOS and Android use the production
.env
, so we need another way to differentiate whether the app is staging or production. iOS allows us to check if the app is a TestFlight build, so this PR adds a nativeModule to check that on startup. For the Android check, there isn't an easy way to do this, so we have to query the play store and grab the current version from the html.Fixed Issues
Fixes #2597
Tests
dev
badge on the sign-in screen logo. On desktop, confirm you see adev
badge for the doc icon.dev
badge on the main app screen next toChats
npm run desktop-build-staging
, then open the Expensify.cash app insidedist/mac
. Confirm the icon has thestg
badge.stg
badge. Sign in and confirm thestg
badge shows up next toChats
.env
file to includeENVIRONMENT=PROD
and run the app on web/ios, confirm you see no badges on the sign-in screen and in the appEnvironmentChecker.m
toreturn [NSNumber numberWithBool:isRunningTestFlightBeta];
and setENVIRONMENT=PROD
in your.env
file. Confirm that when running the app, you see theSTG
badge (we'll test this live once its merged to confirm it shows up properly on the actual TestFlight)CONST.PLAY_STORE_URL
constant inside setEnvironment/index.android.js to be a nonsense URL. Run the app again, confirm no badge is shownCONST.PLAY_STORE_URL
value so it is correct again, then update theversion
value insidepackage.json
andpackage-lock.json
to theCurrent Version
from https://play.google.com/store/apps/details?id=com.expensify.chat&hl=enQA Steps
STG
badge appears on the sign-in screen icon, and in the main app next toChats
(after login) on Desktop, iOS, and Web2. Confirm that on desktop, the desktop icon shows with the
STG
badgeTested On