Merge tenant branding instead of replacing it (PMS-758) #512

Merged

longjacksonle merged 1 commit from fix/PMS-758-logo-rendering into main

2026-08-10 20:25:59 +02:00

longjacksonle commented

2026-08-10 20:19:43 +02:00

Owner

PMS-758. One of the three reasons the org logo never rendered, and the only one that also breaks the client's email. The SPA half is mokosh-apps #511.

The symptom

GET /api/v1/public/tenants/4a870d0c-.../logo returns 404 right now, on the correct URL, for a tenant that has just uploaded a logo. branding.logo_url still points at it, so the request-form email renders a broken image rather than none, which is exactly the failure the compose-time check was written to avoid.

The cause

That route answers 404 when branding.logo_mime is missing, and it was missing because saving the organisation settings page deleted it.

branding is one JSONB document with two writers: the settings page owns the contact keys, the logo upload owns logo_url and logo_mime. update_tenant wrote branding = $n, replacing the whole document, so whichever caller saved last erased the other's half.

Typing the field as TenantBranding made this worse rather than catching it. serde filled in every field the caller did not send as an explicit null, so even a merge at the SQL level would have written those nulls over the top. That is why the fix is in two parts: the update now carries a raw object, so only the keys a caller actually sent are bound, and the service merges with branding || $n::jsonb.

A key sent as an explicit null still clears, which is how the settings page empties a contact field, and is why the SPA now sends nulls instead of omitting empty values.

Non-object branding is rejected rather than stored: || concatenates two non-objects instead of merging, and a string or an array there would leave a document no reader can destructure.

The logo handlers also stop reading the document to write it back, which removes a read-modify-write that could clobber a concurrent edit from the settings page.

Testing

cargo fmt --all --check, cargo clippy --all-targets (clean), tenants suite green (20 tests). The new one walks the exact sequence from the report: write the logo's keys, then write the settings page's keys, then assert the logo's content type survived; then clear a contact with an explicit null and assert the logo is still untouched.

This is a behaviour change to PUT /tenants/{id} and /tenants/current: branding is now a patch document rather than a replacement. Nothing else in the codebase writes it.

PMS-758. One of the three reasons the org logo never rendered, and the only one that also breaks the client's email. The SPA half is mokosh-apps #511. ## The symptom `GET /api/v1/public/tenants/4a870d0c-.../logo` returns **404** right now, on the correct URL, for a tenant that has just uploaded a logo. `branding.logo_url` still points at it, so the request-form email renders a broken image rather than none, which is exactly the failure the compose-time check was written to avoid. ## The cause That route answers 404 when `branding.logo_mime` is missing, and it was missing because saving the organisation settings page deleted it. `branding` is one JSONB document with two writers: the settings page owns the contact keys, the logo upload owns `logo_url` and `logo_mime`. `update_tenant` wrote `branding = $n`, replacing the whole document, so whichever caller saved last erased the other's half. Typing the field as `TenantBranding` made this worse rather than catching it. serde filled in every field the caller did not send as an explicit null, so even a merge at the SQL level would have written those nulls over the top. That is why the fix is in two parts: the update now carries a raw object, so only the keys a caller actually sent are bound, and the service merges with `branding || $n::jsonb`. A key sent as an explicit null still clears, which is how the settings page empties a contact field, and is why the SPA now sends nulls instead of omitting empty values. Non-object branding is rejected rather than stored: `||` concatenates two non-objects instead of merging, and a string or an array there would leave a document no reader can destructure. The logo handlers also stop reading the document to write it back, which removes a read-modify-write that could clobber a concurrent edit from the settings page. ## Testing `cargo fmt --all --check`, `cargo clippy --all-targets` (clean), tenants suite green (20 tests). The new one walks the exact sequence from the report: write the logo's keys, then write the settings page's keys, then assert the logo's content type survived; then clear a contact with an explicit null and assert the logo is still untouched. This is a behaviour change to `PUT /tenants/{id}` and `/tenants/current`: `branding` is now a patch document rather than a replacement. Nothing else in the codebase writes it.

longjacksonle added 1 commit

2026-08-10 20:19:43 +02:00

fix(tenants): merge branding instead of replacing it wholesale

E2E / Playwright against staging (pull_request) Successful in 1m44s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 2m1s

Details

Integration / integration tests (pull_request) Successful in 5m19s

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

ad0f0c1921

PMS-758. `GET /api/v1/public/tenants/{id}/logo` answered 404 on a tenant that had just uploaded a logo, so the client's request-form email rendered a broken image rather than none, which is the failure the compose-time check exists to avoid.

The route answers 404 when `branding.logo_mime` is missing, and it was missing because saving the organisation settings page deleted it. `branding` is one JSONB document with two writers: the settings page owns the contact keys, the logo upload owns `logo_url` and `logo_mime`. `update_tenant` wrote `branding = $n`, replacing the document, so whichever caller saved last erased the other's keys.

Typing the field as `TenantBranding` made it worse rather than caught it: serde filled every field the caller did not send as an explicit null, so even a merge at the SQL level would have written those nulls over the top. The update now carries a raw object, so only the keys a caller actually sent are bound, and the service merges with `branding || $n::jsonb`. A key sent as an explicit null still clears, which is how the settings page empties a contact field.

Non-object branding is rejected rather than stored: `||` concatenates two non-objects instead of merging, and a string or array there would leave a document no reader can destructure.

The logo handlers now send only the two keys they own rather than reading the document to write it back, which also removes a read-modify-write that could clobber a concurrent edit from the settings page.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E48b8YHWYwmaKjFBy5esXp