-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Migrate to Airship v15.2.0 #16360
Migrate to Airship v15.2.0 #16360
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.
Thanks for separating the changes. This seems good, and we can ask Applause to thoroughly test camera/notitifications/permissions/attachments once it's on staging.
This comment was marked as outdated.
This comment was marked as outdated.
Alright on further investigation, permission checks for |
@Santhosh-Sellavel @AndrewGable One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
// On Android API Level 33 and above, these permissions do nothing and always return 'never_ask_again' | ||
// More info here: https://stackoverflow.com/a/74296799 |
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.
NAB: I think your explanation is enough here and you might not need the S.O link
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.
Changes look good, and this tested well for me on a physical Android device.
@AndrewGable would you mind testing iOS when you review?
Building an AdHoc version of the app now to test. |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
Can you review @Santhosh-Sellavel? Thanks! |
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.
Worked great on my iPhone!
Yes, will do it in some time. |
Reviewer Checklist
Screenshots/VideosiOSSimulator.Screen.Recording.-.iPhone.14.-.2023-03-29.at.23.51.58.mp4AndroidAndroid_notifications.mp4android_downloads.mp4 |
Install iOS from this link shows integrity could not be verified error. The Android link works and tests well! |
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.
LGTM tests well for me on android!
🎯 @Santhosh-Sellavel, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #16690. |
Thanks for reviewing @Santhosh-Sellavel. Just confirming that we handled the iOS testing internally, which explains the missing video. |
Lets make sure to test this one thoroughly once on staging. |
✋ 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/Julesssss in version: 1.2.93-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.93-4 🚀
|
This PR caused a regression, the |
Details
Fixed Issues
$ Part of https://github.com/Expensify/Expensify/issues/259220
$ https://github.com/Expensify/Expensify/issues/270305
Tests
Only iOS and Android need to be tested
Regression test notifications
Regression file downloads on Android and iOS
Devs: you need to point your .env to staging to be able to successfully download attachments.
Offline tests
None
QA Steps
@Expensify/applauseleads we'd like to do a regression test for notifications and attachements (and their permissions) on iOS and Android when testing this PR.
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
N/A
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
N/A
iOS
Android