feat(data-access): add feedback review constants + verdict/signal helpers (SITES-43974)

[tathagat2241]

What
Shared, drift-proof constants + helpers for the feedback loop: REVIEW_SOURCES, REVIEW_VERDICTS, REVIEW_SIGNALS, REJECTION_CATEGORIES, FEEDBACK_TIERS, EXPORT_EXCLUDED_REJECTION_CATEGORIES, OPT_OUT_STRIPPED_FIELDS, and verdictToSignal / signalToVerdict / toReviewView.

Why
feedback_event is intentionally not a data-access entity (client-supplied PK + append-only don't fit the framework's auto-id assumption). Consumers use the raw postgrestClient; these constants keep the enum values + the JSONL contract from drifting across api-service and jobs-dispatcher.

Notes

Additive: one barrel line in models/index.js; verified no export name collisions.
⚠️ This must publish before spacecat-api-service and spacecat-jobs-dispatcher merge (they import these names).
Testing

Unit test added; logic verified. (100% coverage gate runs in CI.)
Please ensure your pull request adheres to the following guidelines:

• make sure to link the related issues in this description
• when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

https://jira.corp.adobe.com/browse/SITES-39002
Thanks for contributing!

[absarasw]

Tight, well-shaped PR. Frozen enums + freeze-asserting tests, JSDoc on every export, matching .d.ts typings, and the conscious decision NOT to model feedback_event as a data-access entity (client-supplied PK + append-only don't fit the framework's auto-id assumption) is the right call — name-stamped in the file header so future contributors know why.

One Important finding inline about the cross-repo JSONL contract — both previousFix/editedFix/detailMarkdown naming and the absence of a shared JSONL-row formatter. Both are about the same underlying concern: the PR description promises "these constants keep the enum values + the JSONL contract from drifting across api-service and jobs-dispatcher," but the JSONL contract piece is currently incomplete — the camelCase opt-out list doesn't directly help the snake-case JSONL exporter, and there's no toJsonlRow helper to codify the JSONL row shape itself.

A few minor observations (no constants for state_transition values, no null-guard on toReviewView, the toReviewView JSDoc could explicitly say "for API responses, NOT for JSONL export") aren't worth PR comments — happy to mention on Slack if useful.

Assessment

Ready to merge? With one revision suggested — not a blocker for this PR specifically, but ideally addressed here OR coordinated with PR #743 (jobs-dispatcher exporter) so the JSONL contract doesn't drift across the two repos.

Reasoning: the PR as-shipped is internally consistent and correct. The cross-repo concern is that the exporter (PR #743) will either reinvent the snake_case opt-out list locally (drift risk) or have to do its own camelCase↔snake_case translation when stripping. Closing this gap here keeps the JSONL contract single-sourced.

Next steps

1. Address the inline finding either here (rename to snake_case + add toJsonlRow helper) or coordinate the snake_case strip list in PR #743 with a comment-link back.
2. Note: this PR must publish before #2652 (api-service) and #743 (jobs-dispatcher) merge — PR description already flags that. ✓

[github-actions]

This PR will trigger a minor release when merged.

[absarasw]

Thanks for the quick turnaround — Important finding fully addressed in 63de6fbe. The JSONL contract is now single-sourced in shared (toJsonlRow + shouldExport + buildJsonl + SCHEMA_VERSION); OPT_OUT_STRIPPED_FIELDS is snake_case matching the JSONL/DB columns; PR #743's local copy is deleted and the handler now imports from here.

Bonus on the toReviewView null-guard + JSDoc disambiguation ("NOT for JSONL export; use toJsonlRow") — clean separation between "API view shape" and "JSONL export shape" with both contracts owned in one place.

Test coverage on the new helpers is solid: shouldExport (product_bug / bad_recommendation / NULL-category), toJsonlRow (opt-out strip, missing-optional, schema_version stamp), buildJsonl (filter + empty-when-all-filtered), toReviewView null-guard. LGTM, approving.

[ramboz]

Verdict: APPROVE with minor non-blocking concerns

Clean, well-documented, purely additive (+646 / -0) constants-and-helpers module. The verdict/signal translation logic is correct, exhaustive over the two-value enums, fails closed on invalid input, and is fully reachable from the package root for api-service to import. All 14 exports load and behave as documented (verified by running the helper logic directly through Node — all branches pass, including case-sensitivity and null/missing-field edges). JS/TS security pass clean (no dynamic code, deserialization, injection, or prototype-pollution vectors). No export-name collisions with the other ~40 model barrels.

One thing the PR description undersells: it advertises "constants + verdict/signal helpers," but the module also ships the entire JSONL export contract (toJsonlRow, shouldExport, buildJsonl, SCHEMA_VERSION, OPT_OUT_STRIPPED_FIELDS) consumed by spacecat-jobs-dispatcher. That's the higher-risk surface.

Concerns

C1 — toJsonlRow / toReviewView throw on a row with missing/invalid signal. feedback-event.constants.js:159, 215. Both call signalToVerdict(row.signal) unconditionally → throws for any row whose signal is null/absent. In the exporter (buildJsonl → .map(toJsonlRow)), a single malformed row aborts the entire day's export rather than skipping it. Probably fine since feedback_event.signal is NOT NULL, but it's a sharp edge for a daily batch — document the precondition or decide deliberately between fail-fast (current) vs skip-and-log.

C2 — OPT_OUT_STRIPPED_FIELDS and the toJsonlRow field list can silently drift. feedback-event.constants.js:88, 204-223. Opt-out stripping is single-sourced via the forEach, but the set of fields in the JSONL row is a separate hardcoded object literal. No test asserts every entry in OPT_OUT_STRIPPED_FIELDS is actually produced by toJsonlRow. Add a one-line guard test to lock the privacy boundary against drift.

C3 — Contract values are asserted here but nothing cross-checks them against the DB enums. This module is the contract source, but alignment only holds if the DB migration's CHECK values byte-match these strings. Inherently cross-repo; flagging so reviewers of 718/2652 diff against the manifest.

C4 — toReviewView d.ts types tier as required string, but runtime passes row.tier bare (no ?? null). feedback-event.constants.js:156-166. Same for source, eventId, eventTime — presumably always-present NOT NULL columns, so consistent with intent, but the asymmetry (some fields ?? null, identity/required fields bare) deserves a one-line comment so nobody "fixes" it and weakens the type.

Nits

• N1 — SCHEMA_VERSION, toJsonlRow, shouldExport, buildJsonl not mentioned in the PR description's "What" list. Update so the dependent jobs-dispatcher PR reviewers know the export contract lives here.
• N2 — shouldExport uses Array.includes on EXPORT_EXCLUDED_REJECTION_CATEGORIES (1 element). Fine; a Set only if it grows. No action.
• N3 — Barrel comment in index.js duplicates the @fileoverview. Harmless.
• N4 — previousFix/editedFix typed unknown in d.ts while toJsonlRow uses Record<string, unknown>. Consistent enough.

Strengths

• Single-source-of-truth discipline executed well: enums Object.freezed, opt-out stripping data-driven from one list, JSONL shape one function, schema version one constant.
• Helpers fail closed: verdictToSignal/signalToVerdict throw on unrecognized input rather than defaulting — correct for a training-data pipeline. Case-sensitivity intentional and tested.
• Test coverage genuinely branch-complete (re-ran logic independently; matches).
• Clean additive integration: one barrel line, reachable via models/index.js → service/index.js:51 → src/index.js:15. No collisions. .d.ts ships alongside.
• Correctly chose NOT to model this as a data-access entity (client-supplied PK + append-only) and documented why.