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.

Sign up for GitHub

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

Jump to bottom

fix: move push prompt logic to typescript, fix token registration bugs #8862

Merged
merged 53 commits into from
Jul 11, 2023
Merged

fix: move push prompt logic to typescript, fix token registration bugs #8862

merged 53 commits into from
Jul 11, 2023

Conversation

brainbicycle
Copy link
Contributor

@brainbicycle brainbicycle commented Jun 12, 2023 •
edited
Loading

This PR resolves MOPLAT-748

Description

Moves our push prompt logic to typescript and uses for Android as well.

Our intended push prompt logic:

  • Show a soft prompt dialog before the system dialog, user expresses interests (taps OK) show the system prompt
  • If a user rejects the soft prompt, do nothing for 2 weeks, after 2 weeks prompt again
  • Does not request more than once per session
  • If push permissions are denied it shows a link to settings only once so users can enable

Intended bug fixes:

  • Android 13 was showing push dialog immediately on app launch
  • We were not refreshing the push token on iOS if user changed accounts or environments, this clears token on log out and makes sure we refresh even if status is authorized

Follow-ups:

  • we need to migrate to a supported push notification library, maybe wix, callbacks not being called prevented using same logic across iOS and Android, if we do this we can in theory get rid of most native code around this
  • move push analytics events to cohesion and make consistent with cohesion schema

For Reviewers

  • I don't have an Android 13 device! If anyone with a device could test that the pre-prompt and push prompt are showing up correctly when landing on Home Screen it would be appreciated! 🙏

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • Move push prompt logic to typescript, fix bugs - Brian

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@brainbicycle brainbicycle self-assigned this Jun 12, 2023
@brainbicycle brainbicycle marked this pull request as draft June 12, 2023 21:30
@brainbicycle brainbicycle changed the title fix: move push logic to typescript, fix token registration bugs fix: move push prompt logic to typescript, fix token registration bugs Jun 15, 2023
brainbicycle
brainbicycle commented Jun 15, 2023
View reviewed changes
ios/Artsy.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
ios/Artsy.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
src/app/utils/PushNotification.ts Outdated Show resolved Hide resolved
src/app/utils/requestPushNotificationsPermissions.tests.ts Outdated Show resolved Hide resolved
@brainbicycle brainbicycle marked this pull request as draft June 22, 2023 13:50
@brainbicycle brainbicycle marked this pull request as ready for review July 3, 2023 18:43
gkartalis
gkartalis reviewed Jul 4, 2023
View reviewed changes
Copy link
Member

@gkartalis gkartalis left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing with @MrSltun:

The prompt looks to be working great.

One bug we noticed though 🦟:

  • when declining the notification permissions via the System Dialog (don't allow) on the Home tab, and moving to any other tab we get the Artsy Would like to send you notifications prompt once more
Record_2023-07-04-17-38-28.mp4

@gkartalis gkartalis added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Jul 11, 2023
@artsy-peril artsy-peril bot merged commit 0b45f9a into main Jul 11, 2023
@artsy-peril artsy-peril bot deleted the brian/move-push-to-ts branch July 11, 2023 14:25
MounirDhahri
MounirDhahri reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@MounirDhahri MounirDhahri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found out i didn't submit my review - i am very sorry.

android/build.gradle
compileSdkVersion = 31
targetSdkVersion = 31
compileSdkVersion = 33
targetSdkVersion = 33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: 👏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: It would be nice to ask our 3rd party QA party to test Android this time to make sure nothing broke as a side effect from this upgrade

package.json
@@ -52,7 +55,7 @@
"pod-install": "cd ios; bundle exec pod install; cd ..; ./scripts/post-pod-install.rb",
"pod-install-repo-update": "cd ios; bundle exec pod install --repo-update; cd ..; ./scripts/post-pod-install.rb",
"postinit-metaflags": "rimraf storybook.json",
"postinstall": "yarn init-metaflags; prettier --write package.json; ./scripts/update-echo",
"postinstall": "react-native setup-ios-permissions; yarn init-metaflags; prettier --write package.json; ./scripts/update-echo",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much stuff is going on with rn permissions to set permissions properly 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker though

gkartalis added a commit that referenced this pull request Jul 13, 2023
@gkartalis
Revert "fix: move push prompt logic to typescript, fix token registra…
356fc1c 
…tion bugs (#8862)"

This reverts commit 0b45f9a.
@gkartalis gkartalis mentioned this pull request Jul 13, 2023
Merged
gkartalis added a commit that referenced this pull request Jul 13, 2023
@gkartalis
chore(Revert): "fix: move push prompt logic to typescript, fix token …
7f7cb1e 
…registration bugs" (#8992)

Revert "fix: move push prompt logic to typescript, fix token registration bugs (#8862)"

This reverts commit 0b45f9a.
@gkartalis gkartalis restored the brian/move-push-to-ts branch July 13, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkartalis gkartalis gkartalis left review comments

@MounirDhahri MounirDhahri MounirDhahri left review comments

@damassi damassi Awaiting requested review from damassi

Assignees

@gkartalis gkartalis

Labels
Jira Synced Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brainbicycle @MounirDhahri @gkartalis