feat: key DeviceToken on (AppId, InstallationId) — register upsert + adoption, AppId send filter - #1693
Merged
Merged
Conversation
Pins the (AppId, InstallationId) identity: rotated token updates in place, soft-deleted rows revive, a migration-backfilled legacy:<id> row is adopted rather than duplicated, adoption never crosses AppId, and the send path only sees this app's tokens. Also pins the SENDER_ID_MISMATCH prune rule: prune a mismatching token only when other tokens in the same send succeeded, since a wholesale mismatch is a wrong-credential fault. Expected red until the register/send paths and the 10.0.60 base bump land.
There was a problem hiding this comment.
🟡 Changes recommended
The updated tests reference new service APIs/properties (e.g., RegisterAsync overload, TimePlanningAppId, FcmToken, PruneSenderIdMismatchesAsync) that are not present in the current production code, so the solution will not compile as-is.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the TimePlanning.Pn test suite to validate a new “device-install identity” model for push notification tokens (AppId/InstallationId keyed identity, sender filtering, and pruning behavior).
Changes:
- Refactors
DeviceTokenServiceTeststo exercise registration/upsert/adoption scenarios keyed on(AppId, InstallationId)and to assert workflow-state revival after soft-delete. - Extends
PushNotificationServiceTeststo assert sender token selection excludes soft-deleted and foreign-app tokens, and adds tests forSENDER_ID_MISMATCHpruning behavior. - Updates token property expectations from
TokentoFcmTokenthroughout the affected tests.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PushNotificationServiceTests.cs | Adds/updates tests for token selection filtering and sender-id-mismatch pruning behavior; switches to FcmToken. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs | Reworks registration tests around (AppId, InstallationId) identity, revival/adoption cases, and updated token field naming. |
Review details
Suppressed comments (2)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs:125
- This assertion references DeviceTokenService.TimePlanningAppId, but that constant/member does not exist in the current DeviceTokenService implementation, so the test will not compile unless the production service is updated accordingly.
var stored = await TimePlanningPnDbContext!.DeviceTokens.SingleAsync();
Assert.That(stored.AppId, Is.EqualTo(DeviceTokenService.TimePlanningAppId),
"this service only ever mints tokens for the time app");
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PushNotificationServiceTests.cs:147
- These tests call PushNotificationService.PruneSenderIdMismatchesAsync, but that method does not exist in the current PushNotificationService implementation. The PR needs to add the method (and its logic) or remove/adjust these tests so the suite compiles.
await CreateService().PruneSenderIdMismatchesAsync(
new List<DeviceToken> { mismatching }, targetedCount: 2, targetSdkSiteId: 500);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+43
to
45
| var result = await _service.RegisterAsync( | ||
| 42, "fcm-token-abc", "android", 0, "time", "inst-new"); | ||
|
|
Comment on lines
85
to
87
| var tokens = await service.ResolveTargetTokensAsync(7, minBuild: 31221); | ||
| var picked = tokens.Select(t => t.Token).ToList(); | ||
| var picked = tokens.Select(t => t.FcmToken).ToList(); | ||
|
|
Register upserts on the install identity instead of the FCM token, including soft-deleted rows, and explicitly resets WorkflowState to created - Update() left it alone, so a device pruned after an FCM permanent failure stayed dark forever. When no (AppId, InstallationId) row matches, a row carrying the same (AppId, FcmToken) is adopted and its InstallationId rewritten. The base migration backfills pre-existing rows with 'legacy:<Id>', so without adoption every pre-existing device would end up with two live rows and doubled pushes indefinitely; FCM never prunes either, since the token is still valid. The send path filters on AppId (the leading column of IX_DeviceTokens_AppId_SdkSiteId_WorkflowState, which replaced the dropped site-only index) and reads FcmToken. SenderIdMismatch now prunes, but only when other tokens in the same send succeeded: a send where every token mismatches means this sender holds the wrong Firebase credential, and pruning would wipe the tenant's whole token set. Bumps Microting.TimePlanningBase to 10.0.60.
The sender's AppId and the registrar's AppId are the same value by definition; PushNotificationService now derives it from DeviceTokenService.TimePlanningAppId rather than repeating the literal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task 8 of the device-token identity model (plan
2026-08-30-device-token-identity-model.md, spec2026-08-30-device-token-identity-model-design.md).Identity moves from the FCM token to the app install:
device_token.protogainsapp_id = 4andinstallation_id = 5(field 3 staysbuild_number).(AppId, InstallationId)including soft-deleted rows and explicitly revives them (today a pruned row isUpdate()d without touchingWorkflowState, so a pruned device goes permanently dark).(AppId, InstallationId)row matches, a row matching(AppId, FcmToken)is adopted — itsInstallationIdis overwritten with the real one. Without this, every device that existed before the base migration would end up with both its backfilledlegacy:<id>row and a new row, both live, both selected by the sender: doubled pushes forever.AppId == "time"and readsFcmToken.SenderIdMismatchjoins the prune branch, but only when some token in the same send succeeded. A send where every targeted token mismatches is a wrong-credential fault (TimePlanningBaseSettings:FirebaseServiceAccountJsonpointing at the wrong Firebase project); pruning there would silently wipe the tenant's whole token set. Logged at warn with a Sentry capture.Microting.TimePlanningBase10.0.59 → 10.0.60.Tests run in the existing
DeviceTokenServiceTests(shard c) andPushNotificationServiceTests(shard f) classes; no new class, so no shard-filter change is needed.🤖 Generated with Claude Code
https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk
Deploy ordering — read before merging
app_idandinstallation_idare required: an empty one is rejected with an unsuccessfulOperationResponse. Proto3 gives an old client binary empty strings, not absent fields, so once this is deployed the currently-shipped flutter-time app (whose proto still has only 3 fields) cannot register until Task 10 ships. Already-registered rows keep receiving pushes — the send path does not need the new fields — but a token rotation or a fresh install during the window goes dark.Synthesizing a server-side installation id instead was considered and rejected: any token- or site-derived value re-creates the collision the base migration's
legacy:<Id>backfill exists to avoid, and it would hand a device a permanent identity it can never reconcile with its real UUID. Deploy this plugin together with, or after, the flutter-time release that adds the two proto fields.