add 'send per post' option to the appreciation dialog

[ivannissimrch]

Update 2026-08-26

The status field landed. sdk#210 published AppreciationStatusType in 0.0.147, and be#917 added the column with a migration that backfills existing rows. This PR consumes the real contract now: types.ts deleted, local widening gone.

Description

The frontend side is done, but it needs something from the backend, and I'm not sure what the right shape is, so I'm opening this to show what I have and ask.

The problem: an appreciation row only has dateDue and dateDelivery. There is no status, so the frontend figures out what to show by checking which date is set (Appreciation.tsx, entry.dateDelivery ? "received" : "pending"). With two options, that works. With three it breaks. A posted item and a pending item look identical in the database; both have a due date and no delivery date, so when you reopen the dialog, there is nothing to tell them apart and it shows pending.

What I tried: I added a status field on the appreciation on a local be branch, to see if it worked end to end.

// SDK, on ApiAppreciation
status: "received" | "pending" | "post";

That was the field on ApiAppreciation in the SDK, a column on the appreciation table, the same property in sdk-types.json so request validation lets it through, and one line in dtoAppreciation. No route logic changed, GET /volunteer/:id/appreciation, POST /volunteer/:id/appreciation and PATCH /appreciation/:id all pick it up as they are. It worked, and the frontend in this PR is written against it. Until the field is in the published SDK, types.ts widens the type locally, so this does not run against dev or prod.

@arturasmckwcz here is what I did and why. Is a status field the right approach, or would you do it differently?
Can you help me get it built on the backend?

One thing I am unsure about either way: the sent date currently goes into dateDue, because that is the only column that can hold it, which makes the column name a bit off.

Related Issues

Closes #640

Changes

• New Appreciation/types.ts with DeliveryStatus and AppreciationWithStatus, matching the sibling types.ts files in ActivityLog, OpportunityDetails and VolunteerAgents`
• Third delivery option in the dialog, with en/de strings
• Read-back reads status directly instead of an ordered chain of date checks
• getStatusLabel becomes a Record<DeliveryStatus, ...> lookup, so a fourth status will not compile until it is handled
• Third badge colour for the new status

Screenshots / Demos

Checklist

• WITHIN THE SCOPE OF AN ISSUE; No unnecessary files included
• Tests added/updated
• Documentation updated
• CI passes

[arturasmckwcz]

Review

This is effectively an RFC — the PR itself flags that it depends on an unpublished status field prototyped only on a local be branch. Reviewed against the real published need4deed-sdk@0.0.145 contract, and confirmed that merging as-is to develop would break things for real data:

1. Render crash — Appreciation.tsx:109, getStatusLabel does STATUS_LABEL[entry.status](entry) with no fallback. The published SDK's ApiAppreciationGet has no status field, so for any volunteer with existing appreciation entries this hits undefined(entry) → uncaught TypeError, crashing the whole Appreciation section (no error boundary around it) as soon as this merges — not gated on the backend shipping the field.

2. Contract violation — types.ts:4, AppreciationWithStatus intersects the real SDK type with a required status: DeliveryStatus that doesn't exist in the published contract. Per shared-rules.md: "Do not implement or 'stub' an API change in be or fe ahead of the contract... the work is not ready to land in be/fe." This is the root cause of add login to be #1.

3. Live API writes an undeclared field — Appreciation.tsx:83, every create/update now sends a status property the real backend schema doesn't define. Depending on the route's AJV strictness, saves either get rejected or the field is silently dropped, so it never round-trips — and the next read re-triggers the crash from add login to be #1.

4. Edit dialog breaks for existing entries — AppreciationDialog.tsx:121, the restore effect now sets deliveryStatus from initialData.status instead of deriving it from dateDue/dateDelivery presence (which always exist). For any real entry status is undefined, so no option is pre-selected and isFormValid (!!deliveryStatus) keeps Save disabled until the user manually reselects.

5. Minor — AppreciationDialog.tsx:194, the three DeliveryStatusOption blocks (received/pending/post) are hand copy-pasted; a {status, labelKey, dateLabelKey, allowFuture, testId}[] config array mapped over would make a future 4th status a one-line change instead of another copy-paste block.

On the actual question asked (status field vs. date-chain): a status field is the right direction — but per our SDK-first workflow, it needs to land in sdk and get published to npm first, then implemented in be, before this fe PR can safely target develop. Happy to help drive the sdk/be side once we agree on the shape (and worth deciding there whether the "sent by post" date belongs in dateDue or its own column, per your note).

[ivannissimrch]

All five addressed.

1, 3, and 4 are gone with the real field.
2: types.ts is deleted. ApiAppreciationGet, ApiAppreciationPost, and ApiAppreciationPatch straight from 0.0.147, no intersections.

5: done, the three blocks are a DELIVERY_STATUSES config array mapped over now.

[arturasmckwcz]

Automated review findings.

Three are attached as inline comments below (on lines touched by this diff). Two more are real bugs but sit on code this PR doesn't touch a diff hunk for, so GitHub won't let me anchor them inline — flagging here instead:

1. src/components/Dashboard/Profile/sections/Appreciation/Appreciation.tsx:137 — the "Received on" table cell only reads entry.dateDelivery, so POST-status (mailed) rows show an empty placeholder even though a mailed date exists in dateDue. A coordinator sets status POST with a mailed date; the status badge shows "Mailed on " but the adjacent "Received on" column shows a blank dash for the same row.
2. src/hooks/useAppreciationTracker.ts:37-41 — sortedAppreciations sorts by dateDelivery ?? dateDue with no regard to status, so it now mixes forward-looking PENDING due-dates with backward-looking POST mailed-dates in one ordering. This pre-existing comparator was never updated for the new status semantics this PR introduces, so list order becomes incorrect once POST entries exist alongside PENDING ones.

[arturasmckwcz]

correctness: handleDeliveryStatusSelect only sets a default date when none is selected; it never clears a stale date when switching between statuses with different valid date ranges.

Failure scenario: user selects PENDING and picks a future due date (allowed, allowFuture: true on that option), then switches to POST or RECEIVED (both disallow future dates). The date picker blocks new future date selections going forward, but doesn't clear the already-selected one — so a stale future date can be submitted as the mailed/received date unless the user manually changes it.

[arturasmckwcz]

correctness: STATUS_LABEL has no fallback for an unrecognized entry.status; STATUS_LABEL[entry.status](entry) will throw a TypeError instead of degrading gracefully like the old EMPTY_PLACEHOLDER_VALUE-based logic it replaced.

Failure scenario: a stale cached GET response (pre-dating the SDK 0.0.147 upgrade) or a future SDK enum value added before the FE catches up causes STATUS_LABEL[entry.status] to be undefined; calling it as a function throws while rendering that row, crashing the whole appreciation table instead of showing a placeholder.

[arturasmckwcz]

simplification: StatusBadge's background color is a hand-rolled ternary chain on AppreciationStatusType instead of extending the codebase's existing statusColorMap pattern (used by several other status badges in Dashboard).

A third, independent place now needs updating whenever status colors/tokens change; the established Record<StatusValue, string> + shared StatusBadge convention used elsewhere is bypassed here, increasing drift risk for future status/palette changes.

[arturasmckwcz]

Re-review

Correctness

1. Appreciation.tsx:95 — On create, dateDue: data.dateDue || new Date() overwrites an intentional null dateDue for a RECEIVED entry with today's date. The PATCH branch just above passes data.dateDue straight through, so create and update behave inconsistently for the same logical input (mark as "received" with a delivery date → create silently stamps a bogus due date).

2. AppreciationDialog.tsx:188 — dateDue is now overloaded with two meanings depending on status: a future deadline for PENDING, a past/present "mailed on" fact for POST. Any future code reading dateDue without also checking status (a reminder job, a report, a new UI surface) risks misclassifying an already-mailed item as overdue.

3. AppreciationDialog.tsx:165 — selectedDate is shared state across all three delivery statuses; handleDeliveryStatusSelect silently clears it when the newly selected status disallows the currently-held date. E.g. set a future due date under "need to give it", then switch to "mailed" — the date field empties with no explanation, which reads as data loss rather than intentional validation.

Test coverage

4. AppreciationDialog.tsx:111 — data-testid={\sub-option-${status}`}now interpolates the raw enum value (e.g.appr-received) instead of the previous short received/pending` strings, silently changing these test ids for anything selecting on the old format.

Simplification / reuse

5. styles.ts:10 — StatusBadge's background color is a nested ternary over the 3-member enum, duplicating the Record<StatusValue, string> pattern already used in Dashboard/common/statusMaps.ts and Dashboard/Profile/common/statusMaps.ts. A Record would make TS flag a missing key if a 4th status is added later, instead of silently falling into the else branch.

6. Appreciation.tsx:105 — STATUS_LABEL is a bespoke inline Record of closures in the component body instead of the codebase's createXxxLabelMap(t) factory convention (see createOpportunityStatusLabelMap, createEngagementStatusLabelMap). Rebuilt every render and inconsistent with where the rest of the app looks for status-label maps.

7. Appreciation.tsx:103 — getStatus is a trivial one-line passthrough (entry => entry.status) used at exactly one call site; inlining removes the indirection.

---

No contract-first violations — AppreciationStatusType/status is already published on npm (0.0.147) and consumed on be's develop.

[arturasmckwcz]

Re-review findings (automated).

[arturasmckwcz]

statusColorMap has no default/fallback entry (CONFIRMED)

If a row's entry.status is not one of RECEIVED/PENDING/POST (e.g. a pre-migration record served before the backend backfill completes), statusColorMap[props.$status] is undefined, producing background: undefined; — the badge silently loses its intended color instead of degrading to a defined default.

[arturasmckwcz]

Missing placeholder fallback for null dateDue (PLAUSIBLE)

The old getStatusLabel explicitly returned EMPTY_PLACEHOLDER_VALUE ('–') when an entry had neither dateDelivery nor dateDue. The new createAppreciationStatusLabelMap always concatenates the translated prefix with formatDate(entry.dateDue ?? undefined) for PENDING/POST, so an entry with status PENDING/POST but a null dateDue (e.g. legacy/back-filled rows) renders as "Due on –" / "Mailed on –" instead of the clean placeholder previously shown.

[arturasmckwcz]

Future-date invariant not enforced on dialog open (PLAUSIBLE)

handleDeliveryStatusSelect clears selectedDate when switching to a non-allowFuture status (RECEIVED/POST) with a future date already set, but this useEffect seeding state from initialData on dialog open does no such check. Editing an existing entry whose stored status is RECEIVED or POST but whose date is in the future (possible via legacy data or direct API writes) loads that future date unfiltered; saving without touching the date field resubmits the invalid future date.

[ivannissimrch]

@arturasmckwcz I need a dateMailed field from the BE so I can show the mailed date on the appreciation.Right now there's only dateDue and dateDelivery, and I need to display three: due on, mailed on, and received on. Can you help me get it in?

On the sort comparator you flagged. Agreed that it mixes forward-looking and backward-looking dates. I want to group by status first, then sort by date inside each group, so a due date is never compared against a mailed date. That grouping works with the fields we have today, so it is not blocked, but without a dateMailed column, I can't sort on the mailed date once a row moves on, because saving as received nulls dateDue and the mailed date is gone. I'll fix it after I get the dateMailed from the be.

Screencast.from.2026-08-29.08-55-39.mp4

[arturasmckwcz]

@ivannissimrch now we have the following structure:

❯ echo "\d appreciation"|aits-sql
Defaulted container "postgres" out of: postgres, assert-pgdata-layout (init)
                                          Table "public.appreciation"
     Column     |            Type             | Collation | Nullable |                 Default
----------------+-----------------------------+-----------+----------+------------------------------------------
 id             | integer                     |           | not null | nextval('appreciation_id_seq'::regclass)
 title          | appreciation_title_enum     |           | not null |
 date_due       | timestamp without time zone |           |          |
 date_delivery  | timestamp without time zone |           |          |
 created_at     | timestamp without time zone |           | not null | now()
 updated_at     | timestamp without time zone |           | not null | now()
 opportunity_id | integer                     |           |          |
 volunteer_id   | integer                     |           | not null |
 user_id        | integer                     |           |          |
 status         | appreciation_status_enum    |           | not null |
Indexes:
    "PK_d9824c8e198e82f7394c805eddf" PRIMARY KEY, btree (id)
Foreign-key constraints:
    "FK_29ae22414bad9bb74367b329b00" FOREIGN KEY (user_id) REFERENCES "user"(id) ON DELETE CASCADE
    "FK_2a91e0b949799349a3a87aa220b" FOREIGN KEY (volunteer_id) REFERENCES volunteer(id) ON DELETE CASCADE
    "FK_ce5266cf486c563f4e2c8babe4c" FOREIGN KEY (opportunity_id) REFERENCES opportunity(id) ON DELETE CASCADE

so you suggest adding date_sent, right?

 date_sent      | timestamp without time zone |           |          |

[ivannissimrch]

@arturasmckwcz Yes, exactly that. date_sent, nullable. Set when the status goes to POST, and kept when it moves to RECEIVED so the row still shows when it was mailed.