-
Notifications
You must be signed in to change notification settings - Fork 376
fix(subs): Fix parsing subscriptions from server, and improve handling of deleted push subscriptions #2341
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
Conversation
* The transfer subscription operation only contains the subscription ID. It is possible for a push subscription to have been deleted on the server. In this case, only using transfer subscription will lose accurate push subscription data when the server creates a new push subscription. Missing data can include no push token, and inaccurate opted in or permission status. * Instead, we will use Create Subscription instead which will include accurate push subscription data so the server can create a new push subscription accurately.
* When logging into new users, generate a create-subscription operation for the push subscription instead of enqueueing a transfer subscription operation. The Create Subscription operation has the added benefit of including the whole push subscription's data.
* The subscriptions in the create user response are not returned in the order that they are sent. We cannot rely on ordering to match sent and returned subscriptions. So, we do our best to use data to match them. * Fixes a bug where a push subscription and email/sms subscription are in the same request and the SDK extracts the wrong subscription ID for push.
333ab3f to
e266904
Compare
jkasten2
left a comment
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.
The code changes themselves look good, but can we add some tests?
jinliu9508
left a comment
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.
I also verified various scenarios by performing identity and subscription operations after the subscription was deleted. All behaviors aligned with expectations.
|
@jkasten2 I have added two test cases for this change, verifying the mapping of subscriptions is correct and can handle the situation when push sub is deleted. |
Description
One Line Summary
When multiple subscriptions are in the create user payload, parse the subscriptions correctly, and this also corrects the handling of deleted push subscriptions.
Details
The SDK was relying on subscription order in the sent request and response to match subscriptions, but the order is not maintained. We fix this by deprecating
transfer-subscriptionin favor of always usingcreate-subscription. Create-subscription includes more properties that we can use to parse and match the subscriptions. By usingcreate-subscription, this also fixes problems related to push subscriptions deleted and losing the subscription data, now that we always send all the push data. See #2327 for an example.Motivation
Scope
Testing
Unit testing
None added
Manual testing
Android emulator on API 35
New install, add email immediately, delete the anon user, login
Current user is anon and has an email sub, delete the push sub, call login to existing user
Current user is anon and has an email sub, delete the push sub, call login to new user
Current user is identified, delete the push sub, then login + add email
Current user is identified, delete the push sub, then logout
Delete the push subscription, call optIn/optOut to trigger sub update
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is