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] iOS double welcome notification #1051

Merged
merged 3 commits into from
Jul 7, 2023
Merged

[Fix] iOS double welcome notification #1051

merged 3 commits into from
Jul 7, 2023

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Jul 7, 2023 •
edited by jwilm
Loading

Description

1 Line Summary

Ensures welcome notifications are only sent once, was only reproducible on iOS WebApps, but only somtimes.

Details

PR #1047 addressed this issue but it introduced some side-effects:

  • tags not always being set from the category slide down prompt
  • notificationPermisionChange not firing when it should sometimes

Instead of patching at the subscription change event we are just patching at the welcome notification send function. (changes in PR #1047 were reverted in this PR)

  • Ideally it would be better to fix the root issue of fixing the subscription event firing more than once but we are looking for simpler and safer fix for v15. A longer term fix can be done in v16.

Validation

Tests

Tested on iOS 16.5, ensuring only one welcome notification shows.

Info

Checklist

  • All the automated tests pass or I explained why that is not possible
    • CI tests no longer run due to Python 2 missing from the image.
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed
    • No existing test coverage.

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed
    • No visual changes, other than sometimes seeing a duplicate welcome notification.

Related Tickets

PR #1047

This change is Reviewable

jkasten2 added 3 commits July 7, 2023 01:39
@jkasten2
use early returns on showWelcomeNotification
137c998 
Consistently early return in
onSubscriptionChanged_showWelcomeNotification. This is to prepare for an
additional early return condition in the next commit.
@jkasten2
ensure welcome notification is only sent once
11d6b0b 
In some rare cases iOS WebApps send two welcome notifications. Adding a
simple boolean state to prevent this as a simple workaround for this v15
branch.
@jkasten2
Revert "Add mutex protection to checkAndTriggerSubscriptionChanged"
5c0edcd 
This reverts commit 9620ffc.

A simpler fix without side-effects was done in commit
11d6b0b.
@jkasten2 jkasten2 mentioned this pull request Jul 7, 2023
Merged
@jkasten2 jkasten2 merged commit 954f461 into main Jul 7, 2023
@jkasten2 jkasten2 deleted the fix/double_welcome_notification branch July 7, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgomezp rgomezp rgomezp approved these changes

@nan-li nan-li Awaiting requested review from nan-li

@emawby emawby Awaiting requested review from emawby

@jennantilla jennantilla Awaiting requested review from jennantilla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jkasten2 @rgomezp