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.

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

Fix legacy player ID was not cached properly when upgrading from v3 #1345

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Dec 1, 2023

Description

One Line Summary

Correctly cache push subscription ID when upgrading from v3 for use in receive receipts and notification opened requests.

Details

Correctly cache push subscription ID when upgrading from v3
We were not saving the OSUD_PUSH_SUBSCRIPTION_ID to user defaults when we detected a legacy player ID and proceed with upgrading.
It was only saved from create user responses that hydrate the subscription model, which does not happen on v3 -> v5 upgrade.
Receive receipts and notification opened requests use this value from the cache.

Save push subscription ID on app start
Because of the bug that this PR aims to fix, players that upgraded from v3 -> v5 were missing the player ID in cache for sending to notification opened and notification received endpoints.
Now, to remedy, every time app cold starts, as part of the push subscription model checking itself for updates, we will cache its subscription ID if non-empty.

Future Work
Consider some sort of migration in Core that the extension and user manager can utilize.
Also, this will not fix the issue for users that updated their app and did not open it yet.

Motivation

It was brought up by other team members that many receive receipts and notification opened requests are missing the player ID.

Scope

Sending non-null player ID to receive receipts and notification opened endpoints.

Testing

Unit testing

None, should consider adding

Manual testing

iPhone 13 on iOS 17.1

Scenario 1 Tested mimics v3 -> v5 -> this PR

  1. Install and open app on v3
    a. see player created in dashboard
  2. Update app to main which is v5.0.4, prior to this PR
    a. see legacy player get migrated in SDK
    b. player in dashboard remains the same but with sdk version updated to 5.0.4
    c. Receive receipts and notification opened will be missing player ID
  3. Update app to this PR and open app
    a. Now receive receipts and notification opened will have player ID

Scenario 2 Tested mimics v3 -> this PR

  1. Legacy player is migrated in SDK and requests have player ID

Scenario 3 Tested using new app install

  1. New user created and requests have player ID

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

We were not saving the OSUD_PUSH_SUBSCRIPTION_ID to user defaults when we detected a legacy player ID and proceed with upgrading.
It was only saved from create user responses that hydrate the subscription model, which does not happen on v3 -> v5 upgrade.
Because of a bug prior to v5.0.5, players that upgraded from v3 -> v5 were missing the player ID in cache for sending to notification opened and notification received endpoints.
Now, every time app cold starts, as part of the push subscription model checking itself for updates, we will cache its subscription ID if non-empty.
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

This fixes the bug for new v3 -> v5 migrations for all cases, but it doesn't fix confirmed deliveries for those who already upgrade. Also it probably doesn't fix the not notification open REST API call if someone opens the app for the first time after the v3->v5 upgrade via push, due to the update() code running to late

@nan-li nan-li merged commit 72c2e17 into main Dec 1, 2023
3 of 4 checks passed
@nan-li nan-li deleted the fix_send_player_id_to_notifications_requests branch December 1, 2023 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants