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 requiring privacy consent blocks confirmed deliveries indefinitely #1541

Merged
merged 5 commits into from
Feb 14, 2025

Conversation

nan-li
Copy link
Contributor

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

Description

One Line Summary

Setting consent required and then providing consent still blocked confirmed deliveries from being sent - this PR fixes that.

Details

The consent required value was being written to / read from the shared user defaults but the consent granted value was written to / read from standard user defaults (which is not accessible to the NSE).

  • Looking at the history, the consent granted logic was added in Gdpr privacy #361 in 2018 when few things were shared with the NSE.
  • In 2020, the requires consent value started to get cached from remote params in Add new remote params #707, which saved this to shared user defaults.

This PR also fixes a migration bug (and creates more migrations to fix that and fix the bug in this PR) that arose after modularizing the SDK.

Motivation

Reported by customers, and we should send confirmed deliveries once consent is granted.

Scope

  • where consent is set and read
  • migrations changes including fixing a migration bug after modularization

Testing

Unit testing

None added, little infrastructure

Manual testing

iPhone 13 with iOS 18.1
Fix the bug in this PR

  1. On main, set consent required, and then provide consent
  2. Confirmed deliveries fail due to not detecting consent in the extension
  3. Upgrade to this PR, and confirmed deliveries succeed, and other requests, such as for the user, succeed too (the migration was necessary to maintain behavior in the app).

Fix the existing migration bug

  1. Mock an SDK version upgrade but don't open the app.
  2. Let's say the previous version was 2.0.0 and the new version is 5.2.10.
  3. Send a notification
  4. The NSE receives notification and calls a migration in OSOutcomes
  5. The old SDK version of 2.0.0 is read from cache to determine if migration is needed, and the new SDK version is cached. Outcomes is migrated now.
  6. Next time the app is launched, the Migration Controller reads the new version from cache 5.2.10 and does not do a migration, even if it is necessary. The migration controller invokes in app messages to migrate.

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

* Keeping this method is misleading, it is no longer used by anything
* Instead, logic is using the `requiresUserPrivacyConsent` method instead that considers the plist value.
The migration controller's `migrateToVersion_02_14_00_AndGreater` method is the exact method as `[OSOutcomes migrate]`. This was duplicated unnecessarily during modularization of the SDK.
* Each module will own its migration and a separate cached SDK version to determine if it should do a migration.
* This change is made because it is possible for one module to migrate while another has not yet, and caching the same SDK version will affect the second module's future migration when it reads the version from cache.
* For example, this is currently a bug as of 5.2.9:
- The extension can receive a notification during the time an app is updated without the app being initialized/launched before. This path calls a migration in the outcomes module. When the outcomes module is done migrating, it caches the new SDK version, overriding the previous cached version.
- The next time the app starts up, in app messages may need to migrate but it is using the new SDK version that is cached by outcomes above, and skip a necessary migration.
@nan-li nan-li force-pushed the fix_privacy_consent_blocks_confirmed_deliveries branch from d045e22 to 959cf10 Compare February 13, 2025 05:38
* Previously this was saved only to the standard user defaults so it could not be read by the NSE, which blocked confirmed deliveries from being sent, even after consent has been granted.
* Additionally, migrate this from the standard user defaults to the shared user defaults.
@nan-li nan-li force-pushed the fix_privacy_consent_blocks_confirmed_deliveries branch from 959cf10 to 6054ff7 Compare February 13, 2025 05:52
@nan-li nan-li merged commit 94f471e into main Feb 14, 2025
4 checks passed
@nan-li nan-li deleted the fix_privacy_consent_blocks_confirmed_deliveries branch February 14, 2025 03:43
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