Make TimePlanning device-token register backward-compatible with shipped clients

[renemadsen]

Why — this decouples the backend deploy from the app release

RegisterDeviceToken recently made app_id and installation_id mandatory. proto3 decodes an absent string as "", so every currently-shipped flutter-time build sends empty values for both and is rejected as InvalidArgument. Without this change, the window between deploying the backend and the app reaching both stores looks like:

• new installs never register, so they get no push at all;
• a rotated FCM token can never re-register — that device goes dark permanently until the user updates the app;
• flutter-time does not swallow the rejection. personal_view_widget.dart:214 and main.dart:105 both Sentry.captureMessage(..., SentryLevel.warning) on a failed register. _registerDeviceTokenIfNeeded runs on every personal-view mount and _registerRotatedToken on every app start, so across the fleet on old builds that is a continuous Sentry flood.

Already-registered devices keep receiving push (the migration backfilled AppId and left WorkflowState = 'created'), so this is attrition and noise rather than an outage — but it should not happen at all. With this merged the new backend can ship today and old clients keep working; the app release is no longer a prerequisite.

What changed

1. Empty app_id defaults to the existing TimePlanningAppId constant ("time"). This service backs exactly one app, so there is nothing to disambiguate.
2. Empty installation_id falls back to match-by-token, which is the pre-change behaviour: an existing row for (AppId, FcmToken) is updated in place (owner, platform, build number, WorkflowState revived to Created) and keeps its own InstallationId, so a legacy register can never downgrade a real install id. When no row matches, one is inserted with a deterministic synthetic id — the column is NOT NULL, so an insert has to put something there.
3. Adoption still reclaims that row when the client upgrades. Once the app ships a real installation_id with the same token, the existing (AppId, FcmToken) adoption path rewrites the synthetic id to the real one — same row id, no second row, no doubled pushes as the fleet upgrades.
4. The new-client path is unchanged when both fields are supplied.
5. fcm_token stays hard-required, and is now explicitly validated. It was previously unchecked; without a guard the legacy path would derive an installation id from the empty string and store a junk row.

Both transports are covered: the gRPC service and the REST DeviceTokenController.Register funnel through the same RegisterAsync, and RegisterDeviceTokenModel's string.Empty defaults are exactly what an old JSON body binds to.

The synthetic installation id, and why it cannot collide

legacy-token: + sha256(token) as lowercase hex — 77 characters, comfortably inside InstallationId's varchar(128).

• Cannot collide with a real client id. Clients send a canonical v4 UUID: 36 characters over [0-9a-f-]. : is outside that alphabet and the length differs, so no client-generated value can ever equal one of these. It is equally distinct from the migration's legacy:<Id> backfill.
• Cannot collide with another device. It is derived from the token, and this table's old unique index was on Token alone, so tokens are unique here and two distinct devices cannot hash to the same id.
• Deterministic, which is the point: repeated legacy registers from one device resolve to the SAME row instead of inserting a duplicate on every call.

Hashing the token is sound in this table specifically. The identical formula was rejected for the BackendConfiguration migration because its old key was (WorkerId, FcmToken), which permits duplicate tokens — that rejection does not carry over here, and the code comment says so, so a future reader does not re-derive the wrong conclusion.

Not touched: eform-backendconfiguration-plugin

Its register stays hard-required. Two apps (adhoc and eform) share that table, so an empty app_id there is genuinely ambiguous. This relaxation is only sound because TimePlanning serves exactly one app.

Tests

DeviceTokenServiceTests (CI shard c) — all assert observable DB state, and the cycle is visible in the commits: the two test: commits are red, the fix: commit turns them green.

• empty app_id + real installation_id -> row stored with AppId == "time"
• empty installation_id, no existing row -> row created with the golden synthetic id (a hard-coded sha256 digest, so a random or row-id-derived scheme cannot pass), correct owner/platform/build
• the synthetic id is not parseable as a UUID
• empty installation_id twice with the same token -> exactly ONE row, updated not duplicated
• empty installation_id where a row already exists for that token -> same row id, real InstallationId not downgraded
• legacy row then re-registered by a NEW client sending a real installation_id -> SAME row id, InstallationId now the real one, still exactly one row (RegisterAsync_LegacyRow_IsAdoptedWhenTheClientUpgrades)
• a soft-deleted legacy row re-registered -> revived to Created
• a legacy register never takes another app's row
• a legacy-created row is still selected by the send path (PushNotificationService.ResolveTargetTokensAsync, whose query is keyed on AppId) — a row written with an empty AppId would be invisible to every push and would defeat the whole change
• empty fcm_token -> rejected without storing
• both fields supplied -> stored verbatim (this one passes before and after, by design)
• REST RegisterDeviceTokenModel defaults take the legacy path

Red run: 11 behavioural failures on the tests-only commit, build clean — no compile errors, no test passing by accident.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk

[Copilot]

🟡 Changes recommended

The new tests assert legacy acceptance and empty-token rejection, but the current DeviceTokenService.RegisterAsync implementation still rejects empty app_id/installation_id and does not validate fcm_token, so the PR’s intended behavior won’t pass as-is.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds regression coverage to ensure TimePlanning’s device-token registration remains compatible with already-shipped clients that send empty app_id / installation_id, while still enforcing fcm_token as required.

Changes:

• Introduces a backward-compatibility test suite for legacy clients (empty app_id, empty installation_id) including deterministic synthetic installation IDs.
• Adds tests for upgrade/adoption behavior to ensure legacy rows are reclaimed (no duplicate rows / doubled pushes).
• Adds REST model-default coverage to ensure omitted JSON fields take the legacy path.

File summaries

File	Description
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs	Adds comprehensive tests that pin expected backward-compatible registration behavior and required-field validation.

Review details

Suppressed comments (1)

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs:515

• This test asserts empty fcm_token is rejected without storing, but DeviceTokenService.RegisterAsync currently has no guard for token being empty/whitespace, so it would proceed to insert/update a row with an empty FcmToken. Add an explicit validation for token before any upsert/legacy-fallback logic so the behavior matches this test and prevents storing junk rows.

    // fcm_token stays hard-required: there is no sensible fallback for an
    // absent token, and it is not one of the fields old clients omit.
    [Test]
    public async Task RegisterAsync_EmptyToken_IsRejectedWithoutStoring()
    {
        var result = await _service.RegisterAsync(420, "", "android", 0, "time", "inst-notoken");

        Assert.That(result.Success, Is.False);
        Assert.That(await TimePlanningPnDbContext!.DeviceTokens.CountAsync(), Is.EqualTo(0));
    }

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.