Skip to content

feat(user): add createSupportTicket capability with prerequisite hint#3772

Merged
TaprootFreak merged 2 commits into
developfrom
feat/createSupportTicket-capability
May 26, 2026
Merged

feat(user): add createSupportTicket capability with prerequisite hint#3772
TaprootFreak merged 2 commits into
developfrom
feat/createSupportTicket-capability

Conversation

@TaprootFreak
Copy link
Copy Markdown
Collaborator

Narrow follow-up after #3767 was closed. That PR proposed a structured ActionCapability tree (170 LOC, 4 new DTOs, HttpMethod enum); @davidleomay correctly pointed out the endpoint paths are static and belong in Swagger docs, not on every `/v2/user` response. The pure-error pattern (his alternative suggestion) doesn't fit the actual UX requirement: the realunit-app needs to gate the support tile's tap before opening the ticket form, so the user is routed to the email-capture step first — a post-submit 400 means the user fills the form for nothing.

The compromise

  • `UserCapabilitiesDto.createSupportTicket: { available, missingPrerequisite? }` — per-user runtime info only. The `available` flag and the `missingPrerequisite` discriminator are derived server-side from `userData.mail`. No endpoint paths in the response — those stay in Swagger.
  • `MissingPrerequisite` is a closed enum (`Email`, `Phone`). The app maps the value to the matching capture flow; the backend owns which prerequisite blocks the action, the app owns how the prerequisite is rendered. That's where API-as-Decision-Authority draws the line.
  • Mapper returns a discriminated union `{ available: true } | { available: false; missingPrerequisite }` so the compiler pins the invariant `!available implies missingPrerequisite defined`. The DTO class stays for Swagger schema generation (NestJS decorators don't model TS unions cleanly).
  • `@ApiBadRequestResponse` decorator added on `POST /v1/support/issue` per @davidleomay's review — Swagger docs now name the email-prerequisite remediation path. Description deliberately not narrowed to the email case so other 400 causes (DTO validation, etc.) remain covered.

Size comparison vs #3767

#3767 (closed) this PR
Files 4 4
LOC +177 +92
New classes / enums 4 DTOs + 1 enum 1 DTO + 1 enum + 1 TS union type
Endpoint hints in response Yes (4-field nested structure) No (Swagger only)

Backwards compatibility

  • `createSupportTicket?` is optional on the schema (`@ApiPropertyOptional`) so client SDKs pinned to older API versions ignore the new field.
  • Other `canEdit*` capability flags untouched.
  • Existing service gate (`support-issue.service.ts:123` — `throw new BadRequestException('Mail is missing')`) is unchanged; this PR only adds the proactive Pre-Tap signal that mirrors the gate.

Tests

`user-dto.mapper.spec.ts` gains 4 fixtures:

  • user with mail → `{ available: true }`
  • user with mail = `null` → `{ available: false, missingPrerequisite: 'Email' }`
  • user with mail = `''` → same (pins JS-truthy semantics against future gate drift)
  • user with mail = `undefined` → same

Local verification

  • `npm run type-check` — clean
  • `npm run lint` — clean
  • `npm run format:check` — clean
  • `npm test` — 944 / 944 passing (940 baseline + 4 new tests)

Review history

Reviewed internally via subagent loop. Two iterations — first pass flagged 3 SHOULD-FIX (discriminated union for type-safety, JS-path-leak in Swagger description, controller decorator scope), all addressed. Final verdict "Ready".

Pair-PR

Closes V9 in `DFXswiss/realunit-app:docs/api-authority-audit.md`. Companion app PR coming on `DFXswiss/realunit-app` to consume the new capability and replace the local mail-gating logic.

cc @davidleomay — interested in your take on the compromise. The Swagger decorator is in (your suggestion); the per-user discriminator is the smallest additional surface I could find that lets the app pre-check before the user starts filling out the form.

Narrow follow-up after #3767 was closed. That PR proposed a structured
ActionCapability tree (170 LOC, 4 new DTOs, HttpMethod enum); @davidleomay
correctly pointed out the endpoint paths are static and belong in Swagger
docs, not on every /v2/user response. Pure-error pattern (his alternative
suggestion) doesn't fit the actual UX requirement: the realunit-app needs
to gate the support tile's tap *before* opening the ticket form, so the
user is routed to the email capture step first — a post-submit 400 means
the user fills the form for nothing.

The middle ground here:

- `UserCapabilitiesDto.createSupportTicket: { available, missingPrerequisite? }`
  — per-user runtime info only. The available flag and the missing-
  prerequisite discriminator are derived server-side from `userData.mail`.
  No endpoint paths in the response — those stay in Swagger.
- `MissingPrerequisite` is a closed enum (`Email`, `Phone`). The app maps
  the value to the matching capture flow; the backend owns *which*
  prerequisite blocks the action, the app owns *how* the prerequisite is
  rendered. That's where API-as-Decision-Authority draws the line.
- Mapper returns a discriminated union (`{ available: true } | { available:
  false; missingPrerequisite }`) so the compiler pins `!available implies
  missingPrerequisite defined`. The DTO class stays for Swagger schema
  generation (NestJS decorators don't model TS unions cleanly).
- `@ApiBadRequestResponse` decorator added on `POST /v1/support/issue`
  per @davidleomay's review — Swagger docs now name the email-prerequisite
  remediation path. Description deliberately not narrowed to the email
  case so other 400 causes (DTO validation, etc.) remain covered.

Backwards-compatible: `createSupportTicket?` is optional on the schema
(`@ApiPropertyOptional`) so client SDKs pinned to older API versions
ignore the new field. Other UserCapabilities flags untouched.

## Tests

`user-dto.mapper.spec.ts` gains 4 fixtures: mail present, mail null, mail
empty string, mail undefined. The empty-string and undefined cases pin
the JS-truthy semantics so a future drift in `support-issue.service`'s
gate is caught.

- type-check: clean
- lint: clean
- prettier: clean
- jest: 944 / 944 passing (940 baseline + 4 new tests)
Comment on lines +22 to +25
export enum MissingPrerequisite {
EMAIL = 'Email',
PHONE = 'Phone',
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MissingPrerequisite.PHONE is dead code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — YAGNI. Dropped in 3e26e1f. If a Phone-prerequisite gate gets added on the backend later, the enum extension is additive and backwards-compatible.

@TaprootFreak
Copy link
Copy Markdown
Collaborator Author

@davidleomay — explicit reasoning for why the PR landed where it did, including where we diverged from your suggestion:

What we adopted from your review (closed PR #3767)

  1. Endpoint paths are static — they belong in Swagger, not in every /v2/user payload. Agreed. No { method, path } objects ship in this response. The realunit-app already knows where POST /v1/realunit/register/email lives (it calls it elsewhere already).
  2. @ApiBadRequestResponse on POST /v1/support/issue. Added in this PR — Swagger docs now name the email-prerequisite remediation path so any integrator reading the docs sees the full flow.
  3. Reduce complexity. From 170 LOC + 4 new DTOs + a new HttpMethod enum down to 92 LOC + 1 DTO + 1 enum (MissingPrerequisite) + 1 TS union type. ~50% smaller.

Where we diverged

Your stronger implicit position is "the app should attempt POST /support/issue, handle the 400 reactively, and use the Swagger-documented remediation". This PR keeps a small per-user runtime field (createSupportTicket.available + optional missingPrerequisite discriminator) on /v2/user. Reasoning, in order of weight:

1. The UX requirement is pre-tap gating, not post-submit recovery

Cyrill's explicit product requirement: when the support tile is tapped, if mail is missing, the user is routed into the email-capture step first, then to the ticket form. The tile itself stays unconditionally visible (per the recently-merged #588 — discoverable support for pre-signin onboarding). This requirement comes from the realunit-app being a pre-signin onboarding surface where many users land before registering an email — they need to discover support, and the path to enable it must be a single forward navigation, not "open form → fill it out → bounce off 400 → scroll to a settings page somewhere".

2. Reactive-error pattern means lost work and confusing UX

If the app attempts POST /support/issue first and reacts to the 400:

  • The user has already opened the ticket form, picked a type/reason, typed a message
  • On 400, the app would have to interrupt the flow — modal asking to add email, OR back-navigate and lose the typed content, OR push the email page on top of the create-ticket page and pop the stack on success (fragile state management)
  • The 400 response is, deliberately, indistinguishable from other 400 causes (DTO validation, etc.) at the HTTP layer. The app would have to introspect the error body to decide whether to retry or surrender.

Pre-tap check makes all of that disappear: at the moment of intent (tile tap), the app already knows whether to push the form or the prerequisite step. Zero retries, zero lost form data, zero error-body parsing for control flow.

3. The runtime cost is bounded and small

The per-user payload added by this PR: 2 fields (available: boolean, missingPrerequisite?: 'Email' | 'Phone'). No nested arrays, no endpoint hints, no labels. Encoded JSON: ~50 bytes worst case. /v2/user is the canonical user-state endpoint; one more boolean-with-discriminator there is in the same order of magnitude as the existing four canEdit* flags this PR sits alongside.

What's not shipped:

  • Endpoint paths (your point — they're static, they're in Swagger)
  • Method verbs (same reason)
  • Display labels (the app owns its i18n)
  • Generic capability hierarchy (the original ActionCapability tree was abandoned)

4. Alternatives we considered and rejected

  • Dedicated GET /v1/support/issue/preconditions endpoint. Extra round-trip per tap, more endpoints to version, more surface area. The /v2/user response is already fetched on app foreground and cached — riding on it is essentially free.
  • Header-driven capabilities (Accept-Capabilities: support). More machinery than the simple optional field, and old clients still pay the dispatcher cost.
  • Just a bool supportAvailable: bool. Was the original Wave-3 attempt (feat(user/realunit): expose user capabilities + structured ALREADY_REGISTERED status #3733refactor(user/realunit): drop UserCapabilitiesDto.supportAvailable #3761 rollback). A bool doesn't tell the app what's missing, so the app would have to encode "if !supportAvailable then mail must be missing" — which is a backend rule replicated in the frontend, the exact anti-pattern the audit V9 captures.

The missingPrerequisite discriminator is the smallest field that:

  • (a) lets the app pre-check before opening the form,
  • (b) carries no backend rule into the frontend (the app maps a typed enum value to a UI step, not "interpret why support is unavailable"),
  • (c) extends cleanly when a future prerequisite type is added (e.g. Phone) without an app release for the "what does that mean" mapping — only for the UI of the new step.

5. How this aligns with the API authority plan

realunit-app/docs/api-authority-plan.md Wave 3 prescribed exactly this kind of capability surfacing (with the original bool variant). #3761 rolled that back because the UX requirement made the bool insufficient. This PR is the second attempt at Wave 3 with a discriminator that's expressive enough for the UX without re-creating the structural overhead of #3767.


Open question for you

Is the marginal per-user payload (the createSupportTicket field) tolerable given the UX rationale above? If yes — happy to mark the PR ready for review. If you'd push back further — happy to revisit by extracting the capability to an opt-in endpoint, but I'd want to be sure the per-tap round-trip is actually a worse trade than the /v2/user field before going that direction.

@davidleomay
Copy link
Copy Markdown
Member

Makes sense, approved.

YAGNI: the mapper only emits 'Email'. Add 'Phone' (or any other
prerequisite) when a backend gate actually requires it — the enum
extension would be additive and backwards-compatible.

Addresses @davidleomay's review comment on PR #3772.
@TaprootFreak TaprootFreak marked this pull request as ready for review May 26, 2026 21:39
@TaprootFreak TaprootFreak merged commit 37f2059 into develop May 26, 2026
7 checks passed
@TaprootFreak TaprootFreak deleted the feat/createSupportTicket-capability branch May 26, 2026 21:40
TaprootFreak added a commit that referenced this pull request May 27, 2026
Synthesised from the #3733#3761#3767#3772 review sequence with
@davidleomay. New section under "API Design" makes the eight rules
binding for every future capability flag:

1. Heterogeneous capabilities — bool for hide-able, struct for discoverable
2. Static info belongs in Swagger, not in /user response
3. YAGNI for enum members and optional fields
4. Discriminated union for compiler-enforced invariants
5. Pre-tap signal for discoverable actions
6. Pair-PR with a documented trade-off
7. Backend owns business rules; client only maps types to UI
8. Reduction before extension — test the UX requirement first

Cross-references the consumer-side mirror in DFXswiss/realunit-app's
CONTRIBUTING.md and the api-authority-plan.md roll-out document.

Pure documentation — no code changes.
TaprootFreak added a commit to DFXswiss/realunit-app that referenced this pull request May 28, 2026
…lessons (#593)

Companion to
[DFXswiss/api#3773](DFXswiss/api#3773). The
David-review back-and-forth on the Wave-3 reset produced a concrete set
of rules that should bind every future capability consumption in this
repo — this PR writes them down so the next contributor doesn't have to
rediscover them.

## Three docs updated

### 1. \`CONTRIBUTING.md\` — new sub-section "Consuming API capabilities
— eight rules"

Lives inside the existing "API as Decision Authority — CRITICAL"
section. Covers:

1. Read the capability shape, don't reconstruct it
2. Tile/button visibility for discoverable actions is unconditional
3. Map prerequisite types to UI components, not to business rules
4. Legacy backend tolerance — capability optional, sane fallback
5. No reactive 400-handling for what a capability could pre-tell
6. Pair-PR discipline
7. Tests pin the contract, not the implementation
8. Push back on capability shape that's over-engineered

Cross-references the API-side mirror in
[\`DFXswiss/api:CONTRIBUTING.md\`](https://github.com/DFXswiss/api/blob/develop/CONTRIBUTING.md).

### 2. \`docs/api-authority-plan.md\` — Wave-3 lessons-learned block

Documents the **six-PR** history of V9 (a single capability flag):

| PR | Direction | Outcome |
|---|---|---|
| [api#3733](DFXswiss/api#3733) | API:
\`+supportAvailable: bool\` | merged |
| [app#497](#497) | App:
consume \`supportAvailable\` bool | merged |
| [app#588](#588) | App:
unconditional Support tile | merged |
| [api#3761](DFXswiss/api#3761) | API:
\`-supportAvailable: bool\` | merged |
| [api#3767](DFXswiss/api#3767) | API:
ActionCapability tree (4 DTOs, 170 LOC) | **closed without merge** |
| [api#3772](DFXswiss/api#3772) | API:
\`createSupportTicket: { available, missingPrerequisite? }\` (91 LOC) |
merged |

Plus three "what we'd do differently" points and forward guidance for
Waves 4 and 5.

### 3. \`docs/api-authority-audit.md\` — V9 closed

Marked as closed by api#3772 with a link back to the lessons-learned
section (because the linear "closed by W3" mapping in the audit table
doesn't capture the actual non-linear history).

## Why now

Cyrill asked for these rules to be prominently documented in both repos
and in cross-repo working notes so the pattern doesn't drift on the next
capability. The API side gets the mirror PR
([DFXswiss/api#3773](DFXswiss/api#3773)).

## Verification

- Pure documentation — no code changes
- Markdown renders cleanly (verified)
- Cross-references resolve

## Companion PRs

- API rules:
[DFXswiss/api#3773](DFXswiss/api#3773)
- App pair-PR consuming the first capability following this pattern
(\`createSupportTicket\`): opening shortly on
\`refactor/consume-support-capability\`.
TaprootFreak added a commit to DFXswiss/realunit-app that referenced this pull request May 28, 2026
)

Companion app PR to
[DFXswiss/api#3772](DFXswiss/api#3772) (merged
2026-05-26). **Closes V9** in
[\`docs/api-authority-audit.md\`](docs/api-authority-audit.md).

The Support tile now reads the new
\`user.capabilities.createSupportTicket\` field for its tap decision. No
more local \`mail != null\` reconstruction; the rule lives on the
backend, the app maps a typed enum value to a UI step.

## Architecture

Per the eight consumer rules in [\`CONTRIBUTING.md\`](CONTRIBUTING.md) →
"Consuming API capabilities — eight rules" (documented in
[#593](#593)):

| State | Tap action |
|---|---|
| \`capability == null\` (legacy backend pre-#3772) | Direct push to
Support — API is the authority |
| \`capability.available == true\` | Direct push to Support |
| \`capability.available == false, missingPrerequisite == email\` | Push
email capture page; on \`pop(true)\` re-init the cubit and push Support
if the refreshed capability is now available (or null — symmetric to
branch 1) |
| \`missingPrerequisite == unknown\` or \`null\` | Defensive direct push
— let the API render the error |

\`MissingPrerequisite\` is an **open enum** with \`email\` +
\`unknown\`. Additive backend values degrade to \`unknown\` so a future
prerequisite type never breaks \`/v2/user\` parsing for unrelated
callers (KYC, settings, etc.).

## What changes

### \`lib/\`
- \`packages/service/dfx/models/user/dto/user_dto.dart\` —
\`UserCapabilitiesDto.createSupportTicket\` optional field +
\`CreateSupportTicketCapabilityDto\` + open enum
\`MissingPrerequisite\`.
- \`screens/settings_contact/settings_contact_page.dart\` —
BlocProvider-wrapped; Support tile \`onTap\` dispatches through
\`_onSupportTap\` (4 branches above). Tile layout unchanged.
- \`screens/settings_contact/cubit/...\` — new \`SettingsContactCubit\`
+ state (\`part of\` pattern, States extend Equatable).
- \`screens/support/cubits/support_email_capture/...\` — new cubit +
state for the standalone email capture flow.
- \`screens/support/subpages/support_email_capture_page.dart\` —
standalone page (no KYC coupling) calling
\`RealUnitRegistrationService.registerEmail\`. \`mergeRequested\` status
surfaces a dedicated message — the multi-step verification flow is
deliberately not dragged into this minimal page.
- \`setup/routing/routes/support_routes.dart\` +
\`setup/routing/router_config.dart\` — new
\`SupportRoutes.emailCapture\` under \`/support/email\`.
- \`assets/languages/strings_{en,de}.arb\` — 4 new keys (alphabetically
sorted, both languages).
- \`lib/generated/i18n.dart\` — regenerated via \`dart run
tool/generate_localization.dart\`.

### \`test/\`

| File | Tests |
|---|---|
| \`packages/service/dfx/models/user/dto/user_dto_test.dart\` | +14
cases for createSupportTicket parsing, incl. \`unknown\` degradation and
JSON-null handling |
| \`screens/settings_contact/cubit/settings_contact_cubit_test.dart\` |
6 cases |
| \`screens/settings_contact/cubit/settings_contact_state_test.dart\` |
8 cases |
| \`screens/settings_contact/settings_contact_page_test.dart\` | 15
widget tests covering tile visibility + 11 routing branches incl.
\`unknown\` and pop(null\|false) |
|
\`screens/support/cubits/support_email_capture/support_email_capture_cubit_test.dart\`
| 5 cases (success, mergeRequested, ApiException, generic throw) |
|
\`screens/support/cubits/support_email_capture/support_email_capture_state_test.dart\`
| 7 Equatable cases |
| \`screens/support/subpages/support_email_capture_page_test.dart\` | 9
widget tests |
| \`goldens/screens/support/support_email_capture_golden_test.dart\` | 2
goldens (default + submitting) |
| \`goldens/screens/settings_contact/settings_contact_golden_test.dart\`
| Re-baselined for BlocProvider wrap; visual surface unchanged |

## Local verification

- \`dart format\` clean on all touched files
- \`dart analyze lib/ test/\` — no issues found
- \`flutter test\` on touched scope → **140/140 green**

## Review history

Implemented + reviewed via internal subagent loop, three iterations:
1. First implementation off \`develop\` → had to rebase onto
\`chore/post-580-followups\` (PR #588 base mismatch).
2. Reviewer found 2 SHOULD-FIX (Branch-1 asymmetry with \`?? false\`
violating the no-fallback rule; \`MissingPrerequisite.fromString\` throw
breaking unrelated \`/v2/user\` callers on additive backend changes).
Both addressed: explicit null-check symmetry, open-enum \`unknown\`
degradation.
3. Final reviewer pass found a \`dart format\` issue on the enum block —
fixed; added an explicit \`unknown\`-routing widget test as a
NICE-TO-HAVE.

## Targeting \`chore/post-580-followups\`

Per request — this is a post-#580 follow-up that consumes a new API
capability rather than introducing one in isolation. PR base set
accordingly.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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