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

Detect for timezone changes and update the user #1542

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Feb 13, 2025

Description

One Line Summary

Send up timezone ID as a one-time migration, send up timezone ID when fetching the legacy player, and start detecting timezone changes and sending it up when it changes.

Details

  • Unless users switch via logging in and out, we never update timezone (as it is only sent in the Create User request).
  • Now, we cache timezone ID and detect for changes on cold starts and update the user when it changes.
  • We also fix a rare issue that comes from upgrading from a very very old version of player model to user model. When we have a legacy player ID, we fetch its identity rather than creating a user. In some rare cases, this old player is already missing a timezone (version used was before we added timezone collection to the player model SDK), and the SDK never calls Create User to set a timezone. As a result, this user will be missing timezone.
  • As a one-time migration, we send up the timezone in the next release for all users without considering if timezone is indeed missing or not. Note that this update will be combined in the payload of the session_count update. This update is driven by reading the timezone_id from cache, but before this PR, timezone was never cached, so it will return null, driving this one-time update.

Motivation

Reports by customers about missing timezones on very old players after migrating to v5.
Also requests by customers to detect timezone changes.

Scope

Sending timezone

Testing

Unit testing

None added

Manual testing

iPhone 13 on iOS 18.1
Scenario tested - player model upgrade:

  1. Upgrade from an existing player model installation
  2. SDK uncaches the legacy player, fetch the identity, and sends the timezone
  3. Next cold start, no timezone is sent when there is no change
  4. Change timezone on phone and make a cold start
  5. The new timezone is sent

Scenario tested - user model upgrade:

  1. New install on main
  2. Upgrade to this branch
  3. See that timezone is sent up in a one-time migration. There may have been old players who were missing timezone, upgraded to user model, and still missing timezone.
  4. Subsequently, no timezone is sent when there are no changes
  5. If timezone does change on phone, it is sent up.

Scenario tested - new install:

  1. New installs call Create User so timezone is sent, same as before.
  2. Now moving forward, when it changes on the device, it is sent.

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

* Unless users switch via logging in and out, we never update timezone (as it is only sent in the Create User request).
* Now, we cache timezone ID and detect for changes on cold starts and update the user when it changes.
* We also fix a rare issue that comes from upgrading from an old version of player model to user model. When we have a legacy player ID, we fetch its identity rather than creating a user. Thus, in some rare cases, this old player is already missing a timezone, and the SDK never calls Create User to set a timezone. As a result, this user will be missing timezone.
* As a one-time migration, we send up the timezone for all users without considering if timezone is indeed missing or not. Note that this update will be combined in the payload of the session_count update. This update is driven by reading the timezone_id from cache, but before this PR, timezone was never cached, so it will return null.
@nan-li
Copy link
Contributor Author

nan-li commented Feb 13, 2025

Test OneSignalNotificationsTests.testDontclearBadgesWhenAppBecomesActive() has always been flaky.

@nan-li nan-li merged commit 99cdabb into main Feb 14, 2025
3 of 4 checks passed
@nan-li nan-li deleted the fix/migrate_timezone_id branch February 14, 2025 03:44
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