Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Going to send this one! Users won't actually be able to see this yet as the triggers are still disabled in the Auth backend. The flow can only be triggered manually as described in the so there is minimal risk. |
|
@marcaaron looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
The test that was not passing was prettier and unclear why it kept failing as there was no output provided. I do not think it is a blocker. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.2.75-0 🚀
|
|
Tested Android on staging and the first modal looks good and the Concierge message appears. The in-app review part did not show. I'm not sure if that's because it's a beta version. I'll test again on production and if it does not work then we'll have to figure out why. This should not be a blocker for rolling this PR to production as the feature won't be officially enabled for users until we merge this Auth PR. I'm waiting for the iOS staging build at the moment. So, if everything looks good there I will check this off the staging deploy list. We will need to test the Android flow again after this PR is deployed. If Android's in app modal still doesn't show on production then we will need to investigate that before merging the Auth PR. Edit: iOS in app review is not showing up either on staging. If it doesn't show on production then we will know something is wrong. It worked fine on dev. Not sure what else to check. Perhaps @Julesssss can help debug if they've any time to spare. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.2.75-0 🚀
|
Sorry I'm about to go OOO. I recall us only getting one chance on an Android use account that has never reviewed the app before. But that iOS was a bit more reliable. Maybe QA could help with this? |
|
No worries. I'm also going OOO so if this needs to be on HOLD for a bit that seems ok. I will try to test on production with a fresh google account. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|
|
Just following up to report that this works great on iOS, but still unconfirmed on Android. I asked QA to help here. AFAICT from the logs when testing on my Android phone there is a review available - but the modal does not show. I also pulled device logs and there's no information really other than it looks like it should be working. Did some digging and found out that Android can fail silently if Google's algorithm determines that the user should not be able to review. Going to try with a different account to see if that works better. |
|
App review not displayed in Samsung A52 / Android 14, v9.2.83-0 Prod app.review.mp4 |
|
Moving this conversation to Slack as I'd prefer to ship the Auth changes only after confirming that the in-app review modal works as expected on Android. |
|
Coming from this issue #79635 , we should have used localized strings in |
Hold On Deploys:
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/561663
Tests
NVP.set('appReview', {trigger: 'test'})NVP.set('appReview', null)and thenNVP.set('appReview', {trigger: 'test'})againOffline tests
❌
QA Steps
Internal QA - Come get me and I will test this.
Test on iOS and Android mainly and verify that these screens show up:
iOS
ANDROID
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari