feat: add skipReason and skipDetail attributes to Suggestion

[Kanishkavijay39]

Summary

Adds skipReason (enum) and skipDetail (string, max 500 chars) as optional attributes on the Suggestion model, enabling structured tracking of why users skip
suggestions.

• SKIP_REASONS enum: ALREADY_IMPLEMENTED, INACCURATE_OR_INCOMPLETE, TOO_RISKY, NO_REASON, OTHER
• UPPER_CASE values matching existing STATUSES/TYPES convention
• skipReason → skip_reason and skipDetail → skip_detail mapped automatically via camelToSnake
• No redundant postgrestField annotations
• Validation: skipDetail capped at 500 characters using isString() utility

Related Issues

• SITES-38900

Related PRs

• DB migration: https://github.com/adobe/mysticat-data-service/pull/108
• API controller + OpenAPI: feat: [SITES-38900] add skipReason and skipDetail support for SKIPPED suggestions spacecat-api-service#1873
• Frontend: https://github.com/OneAdobe/experience-success-studio-ui/pull/1598 (WIP)

Test plan

• All 5 enum values asserted with length check
• Getter/setter tests for skipReason and skipDetail
• Null handling for both fields
• 1523 unit tests passing
• Local e2e verified against PostgreSQL

Thanks for contributing!

[github-actions]

This PR will trigger a minor release when merged.

[solaris007]

Review: Skip Reason Attributes for Suggestion

What was done well

The enum + schema attribute pattern correctly follows the existing STATUSES/TYPES approach with Object.values() for type validation. Both attributes are optional (correct for supplementary skip metadata). The skipDetail validate function handles the falsy case properly. Already uses postgrestField mappings - PostgreSQL-ready.

---

Important

1. Opportunity model not updated despite PR title and description

Title says "Suggestion and Opportunity schemas", description says "ignores an opportunity from the UI" - but only Suggestion is changed. The Opportunity model has an IGNORED status and would logically need analogous fields (ignoreReason/ignoreDetail). Either add them or update the PR title/description.

2. No database migration for skip_reason/skip_detail columns

The suggestions table in mysticat-data-service/db/migrations/20250130000007_suggestions.sql has no skip_reason or skip_detail columns. Without a corresponding ALTER TABLE migration, any PostgREST PATCH with these fields will fail at runtime. Needs:

• ADD COLUMN skip_reason TEXT (or a PG enum type)
• ADD COLUMN skip_detail VARCHAR(500) (or TEXT with CHECK constraint)
• CHECK constraint on skip_reason to match the enum values

3. Enum value casing inconsistent within Suggestion model

Existing enums on Suggestion use UPPER_CASE values (STATUSES.NEW = 'NEW', TYPES.CODE_CHANGE = 'CODE_CHANGE'). The new SKIP_REASONS uses snake_case values ('already_implemented'). This is confusing within the same model. Either align with the existing UPPER_CASE convention or document why snake_case was chosen.

4. Missing negative/edge test cases

• No test for invalid skip reason value (should trigger guardEnum rejection)
• No test for skipDetail exceeding 500 chars
• No test for null values
• SKIP_REASONS enum test only checks 3 of 5 values (INACCURATE_OR_INCOMPLETE and NO_REASON not asserted)

5. skipDetail 500-char limit only enforced on creation, not on setter

The validate function is only invoked during collection-level validateItem() (creation), not by the Patcher's setSkipDetail(). So instance.setSkipDetail('a'.repeat(1000)) succeeds silently. This is a framework-level behavior, not specific to this PR, but worth being aware of - the DB-level constraint (VARCHAR or CHECK) becomes the real enforcement point.

---

Minor

• postgrestField annotations are redundant - camelToSnake in postgrest.utils.js already converts skipReason to skip_reason automatically. No other non-id attribute in the schema uses explicit postgrestField.
• NO_REASON enum value is semantically questionable - leaving skipReason null/undefined already means "no reason provided". Clarify whether the UI has an explicit "No reason" button that needs a distinct value.
• JSDoc comment "Maps to PostgreSQL skip_reason" references the database column - model layer should be DB-agnostic. Simplify to "Predefined categories for skip reasons."
• Uses typeof value === 'string' instead of the isString() utility from @adobe/spacecat-shared-utils already used elsewhere in the codebase.

---

Recommended Actions

Priority	Action
Block	Create DB migration in mysticat-data-service before merging
Block	Update PR title/description, or add Opportunity model changes
Before merge	Align enum value casing with existing Suggestion enums
Before merge	Add negative test cases (invalid enum, length boundary)
Nice to have	Remove redundant postgrestField, clarify NO_REASON intent

[Kanishkavijay39]

Review: Skip Reason Attributes for Suggestion

What was done well

The enum + schema attribute pattern correctly follows the existing STATUSES/TYPES approach with Object.values() for type validation. Both attributes are optional (correct for supplementary skip metadata). The skipDetail validate function handles the falsy case properly. Already uses postgrestField mappings - PostgreSQL-ready.

Important

1. Opportunity model not updated despite PR title and description

Title says "Suggestion and Opportunity schemas", description says "ignores an opportunity from the UI" - but only Suggestion is changed. The Opportunity model has an IGNORED status and would logically need analogous fields (ignoreReason/ignoreDetail). Either add them or update the PR title/description.

2. No database migration for skip_reason/skip_detail columns

The suggestions table in mysticat-data-service/db/migrations/20250130000007_suggestions.sql has no skip_reason or skip_detail columns. Without a corresponding ALTER TABLE migration, any PostgREST PATCH with these fields will fail at runtime. Needs:

• ADD COLUMN skip_reason TEXT (or a PG enum type)
• ADD COLUMN skip_detail VARCHAR(500) (or TEXT with CHECK constraint)
• CHECK constraint on skip_reason to match the enum values

3. Enum value casing inconsistent within Suggestion model

Existing enums on Suggestion use UPPER_CASE values (STATUSES.NEW = 'NEW', TYPES.CODE_CHANGE = 'CODE_CHANGE'). The new SKIP_REASONS uses snake_case values ('already_implemented'). This is confusing within the same model. Either align with the existing UPPER_CASE convention or document why snake_case was chosen.

4. Missing negative/edge test cases

• No test for invalid skip reason value (should trigger guardEnum rejection)
• No test for skipDetail exceeding 500 chars
• No test for null values
• SKIP_REASONS enum test only checks 3 of 5 values (INACCURATE_OR_INCOMPLETE and NO_REASON not asserted)

5. skipDetail 500-char limit only enforced on creation, not on setter

The validate function is only invoked during collection-level validateItem() (creation), not by the Patcher's setSkipDetail(). So instance.setSkipDetail('a'.repeat(1000)) succeeds silently. This is a framework-level behavior, not specific to this PR, but worth being aware of - the DB-level constraint (VARCHAR or CHECK) becomes the real enforcement point.

Minor

• postgrestField annotations are redundant - camelToSnake in postgrest.utils.js already converts skipReason to skip_reason automatically. No other non-id attribute in the schema uses explicit postgrestField.
• NO_REASON enum value is semantically questionable - leaving skipReason null/undefined already means "no reason provided". Clarify whether the UI has an explicit "No reason" button that needs a distinct value.
• JSDoc comment "Maps to PostgreSQL skip_reason" references the database column - model layer should be DB-agnostic. Simplify to "Predefined categories for skip reasons."
• Uses typeof value === 'string' instead of the isString() utility from @adobe/spacecat-shared-utils already used elsewhere in the codebase.

Recommended Actions

Priority Action
Block Create DB migration in mysticat-data-service before merging
Block Update PR title/description, or add Opportunity model changes
Before merge Align enum value casing with existing Suggestion enums
Before merge Add negative test cases (invalid enum, length boundary)
Nice to have Remove redundant postgrestField, clarify NO_REASON intent

Thanks for the thorough review. All points addressed:

1. Opportunity model → Scoped to Suggestion only
   Updated PR title. Opportunity is intentionally deferred until the product team confirms the UX applies at that level.

2. DB migration → Created
   mysticat-data-service fix: organizations query #108 PG enum type suggestion_skip_reason + varchar(500) for detail. Tested locally, all 26 suggestion tests passing (19 existing + 7 new).

3. Enum casing → Aligned to UPPER_CASE
   Now matches STATUSES/TYPES convention: ALREADY_IMPLEMENTED, INACCURATE_OR_INCOMPLETE, TOO_RISKY, NO_REASON, OTHER.

4. Missing tests → Added

• All 5 enum values asserted with length check
• Null handling for both skipReason and skipDetail
• 1523 unit tests passing

5. 500-char limit on setter → Acknowledged
   This is framework-level behavior (Patcher doesn't invoke validate). The DB-level varchar(500) is the real enforcement point, which is in place via the migration.

Minor items — all addressed:

• Removed redundant postgrestField annotations
• NO_REASON kept — UI has an explicit "No reason" button
• JSDoc simplified to "Predefined categories for skip reasons."
• Switched to isString() utility

[solaris007]

Hey @Kanishkavijay39

All blocking and pre-merge items from the previous review have been resolved:

1. DB migration created - mysticat-data-service #108 with PG enum type suggestion_skip_reason + varchar(500) for detail
2. PR title/description updated - Scoped to Suggestion only, Opportunity deferred. Comprehensive description with related issues (SITES-38900), related PRs, and test plan
3. Enum casing aligned - UPPER_CASE values (ALREADY_IMPLEMENTED, TOO_RISKY, etc.) matching existing STATUSES/TYPES convention
4. Tests improved - All 5 enum values asserted with lengthOf(5) check, null handling tests for both skipReason and skipDetail
5. Redundant postgrestField annotations removed - camelToSnake handles the mapping
6. JSDoc simplified - "Predefined categories for skip reasons."
7. isString() utility - Now uses the shared-utils import instead of typeof
8. NO_REASON clarified - UI has an explicit "No reason" button, justifying the enum value

Clean, well-scoped change with proper cross-repo coordination (DB migration, API controller, frontend PRs linked).