Skip to content

feat: [SDK-5473] send onesignal_id with confirmed receipts and notification opened - #2830

Draft
nan-li wants to merge 2 commits into
mainfrom
nan/sdk-5473
Draft

nan-li wants to merge 2 commits into
mainfrom
nan/sdk-5473

Conversation

@nan-li

@nan-li nan-li commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Send the user's onesignal_id with confirmed receipts and notification opened reports when the SDK has a backend-assigned id.

Details

Motivation

Both requests carry only the subscription id, so the server resolves the user through a Perseus lookup that scans every partition of the app's shard on a cache miss. Large sends turn that into tens of thousands of lookups a second and 500s on report_received, which loses confirmed-delivery stats (SDK-5473, UD-4748). With onesignal_id in the request the server can query one partition, confirm the subscription belongs to that user, and fall back to today's path if it does not.

Scope

  • PUT notifications/:id/report_received and PUT notifications/:id (opened) gain a top-level onesignal_id string.
  • The field is omitted entirely when the SDK has no backend id: a fresh install, or the window after login() or logout() until the create-user response hydrates the real id. Local placeholder ids are never sent.
  • For receipts the id is captured when the notification arrives, together with the subscription id, and carried to the delayed worker.
  • Outcomes already send onesignal_id and are untouched. Existing fields keep their names.

Deploy order

The server's report_received handler currently rejects unknown JSON fields, so the receipt change must not release until the server accepts onesignal_id. The opened endpoint ignores unknown fields. Neither handler reads the field yet; the server side is tracked by User Data.

Testing

Unit testing

New tests cover the backendOneSignalIdOrNull helper (backend id, local id, empty, missing), the receipt processor, both backend methods with and without the id, and the lifecycle service reporting the id or omitting it on open. The full core and notifications suites pass. Detekt baseline entries were updated for the changed constructor and loop signatures.

Manual testing

Not yet. Needs a build against a server that accepts the field.

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

Lets the server find the user in one partition instead of a Perseus
lookup by subscription that scans every partition on a cache miss. The
field is left out while the user only has a local placeholder id.
Same subscription-to-user lookup as the receipt, read at tap time from
the identity model. The opened endpoint ignores unknown fields today, so
this can ship before the server reads it.
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • IdentityModelStore.kt: 3/3 touched executable lines (100.0%) (11 touched lines in diff)
  • NotificationBackendService.kt: 4/4 touched executable lines (100.0%) (9 touched lines in diff)
  • NotificationLifecycleService.kt: 4/4 touched executable lines (100.0%) (7 touched lines in diff)
  • ReceiveReceiptProcessor.kt: 1/1 touched executable lines (100.0%) (2 touched lines in diff)
  • ReceiveReceiptWorkManager.kt: 0/5 touched executable lines (0.0%) (11 touched lines in diff)
    • 5 uncovered touched lines in this file

Overall (aggregate gate)

12/17 touched executable lines covered (70.6% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • ReceiveReceiptWorkManager.kt: 0.0% (5 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 70.6% (minimum 80%).

📥 View workflow run

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.

1 participant