Fixed missing validation for title/description fields in offers settings#26696
Merged
Fixed missing validation for title/description fields in offers settings#26696
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds MAX_DISPLAY_TEXT_LENGTH (191) and enforces it on displayTitle and displayDescription inputs. Introduces normalizeDurationInMonths and isValidMonthDuration to coerce and validate month counts, and makes duration_in_months nullable when duration !== "repeating". Updates percent validation to require 1–100. Adjusts form data shaping, validation messages, and UI wiring in add/edit offer modals. Updates acceptance tests: renames suite, checks new validation error texts for month inputs, and verifies updated duration_in_months null/changed payload shapes. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0968e38645
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx (1)
33-44: Extract display-length UI helpers to a shared offers utility.
MAX_DISPLAY_TEXT_LENGTHandgetLengthHintare duplicated here and inapps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx(Line 37 and Line 169-178). Centralizing this will prevent future message/threshold drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx` around lines 33 - 44, Extract the duplicated MAX_DISPLAY_TEXT_LENGTH constant and getLengthHint function into a shared offers utility module and update both components to import them; specifically, create a new exported helper (e.g., MAX_DISPLAY_TEXT_LENGTH and getLengthHint(helperText?: string)) to preserve the existing signature/behavior (including lengthColor logic and JSX output), replace the local definitions in add-offer-modal and edit-retention-offer-modal with imports of those symbols, and remove the duplicate implementations so both components use the single shared source.apps/admin-x-settings/test/acceptance/membership/offers.test.ts (1)
173-180: Add an assertion for the new character hint UI contract.This currently verifies truncation, but not the hint behavior introduced in this PR. Add a direct hint assertion (for example
191 / 191) so the UX requirement is explicitly covered.💡 Suggested test addition
await sidebar.getByLabel('Display title').fill(tooLongDisplayTitle); await expect(sidebar.getByLabel('Display title')).toHaveValue('t'.repeat(191)); +await expect(sidebar).toContainText('191 / 191'); await sidebar.getByLabel('Display description').fill(tooLongDisplayDescription); await expect(sidebar.getByLabel('Display description')).toHaveValue('d'.repeat(191)); +await expect(sidebar).toContainText('191 / 191');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts` around lines 173 - 180, The test currently checks truncation but misses asserting the new character-count hint; after filling the inputs via sidebar.getByLabel('Display title') and sidebar.getByLabel('Display description') add assertions that the hint element updates (e.g. expect(sidebar.getByText('191 / 191')).toBeVisible() or use the specific hint selector if available) for both the title and description so the UI contract for the character counter is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`:
- Around line 133-134: durationInMonths currently forwards raw
formState.durationInMonths so invalid values can be sent when validation is
bypassed; coerce and guard it before sending: when duration === 'repeating'
compute durationInMonths = Math.max(1,
Math.floor(Number(formState.durationInMonths) || 0)) else 0, and use that
guarded value in the API payloads (also apply the same coercion wherever
durationInMonths is constructed for save/create-draft flows).
- Around line 571-577: The validation message text is inconsistent with the
isValidMonthDuration predicate (which allows 1), so update the user-facing copy
to match the validator: replace "greater than 1" messages with phrasing like "at
least 1 month" or "of at least 1" where used. Specifically, change the strings
assigned to newErrors.durationInMonths and newErrors.amount in the branches that
check formState.type and call isValidMonthDuration so they read e.g. "Enter a
duration of at least 1 month." and "Enter a number of free months of at least
1." to align with isValidMonthDuration and formState.freeMonths.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx`:
- Around line 33-44: Extract the duplicated MAX_DISPLAY_TEXT_LENGTH constant and
getLengthHint function into a shared offers utility module and update both
components to import them; specifically, create a new exported helper (e.g.,
MAX_DISPLAY_TEXT_LENGTH and getLengthHint(helperText?: string)) to preserve the
existing signature/behavior (including lengthColor logic and JSX output),
replace the local definitions in add-offer-modal and edit-retention-offer-modal
with imports of those symbols, and remove the duplicate implementations so both
components use the single shared source.
In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts`:
- Around line 173-180: The test currently checks truncation but misses asserting
the new character-count hint; after filling the inputs via
sidebar.getByLabel('Display title') and sidebar.getByLabel('Display
description') add assertions that the hint element updates (e.g.
expect(sidebar.getByText('191 / 191')).toBeVisible() or use the specific hint
selector if available) for both the title and description so the UI contract for
the character counter is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4599da97-4737-4024-b1f1-4fce617a5501
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsxapps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsxapps/admin-x-settings/test/acceptance/membership/offers.test.ts
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx
Outdated
Show resolved
Hide resolved
closes https://linear.app/ghost/issue/BER-3392 closes https://linear.app/ghost/issue/BER-3218 - added character hint and inline validation error for display title / description fields - added inline validation for floating numbers for free-months / duration-in-months fields
sagzy
force-pushed
the
inline-errors-for-offer-settings
branch
from
0968e38 to
748fb8e
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx (1)
443-465:⚠️ Potential issue | 🟠 MajorAdd integer validation for repeating month duration in Add Offer.
Decimal values (e.g.,
1.5) currently pass client validation for repeating duration and are only rejected downstream. This should be blocked inline like retention flow.🔧 Proposed fix
- if (formState.type !== 'trial' && formState.duration === 'repeating' && formState.durationInMonths < 1) { - newErrors.durationInMonths = 'Enter a duration greater than 0.'; + if ( + formState.type !== 'trial' && + formState.duration === 'repeating' && + (!Number.isInteger(formState.durationInMonths) || formState.durationInMonths < 1) + ) { + newErrors.durationInMonths = 'Enter a whole number of months (1 or more).'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx` around lines 443 - 465, The validation currently allows decimal values for repeating duration; update the validation in AddOfferModal (where formState is validated — look for the checks referencing formState.duration, formState.durationInMonths and newErrors.durationInMonths) to enforce an integer > 0 when formState.duration === 'repeating' and formState.type !== 'trial' (e.g., check Number.isInteger(formState.durationInMonths) and > 0) and set a clear error like "Enter a whole number greater than 0." Replace or augment the existing durationInMonths check so decimals are rejected client-side before submission.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx (1)
33-44: Extract shared display-length helper/constants to avoid drift.
MAX_DISPLAY_TEXT_LENGTHandgetLengthHintare duplicated here and inapps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx. Centralizing them in a shared offers helper keeps validation/UI copy aligned in both flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx` around lines 33 - 44, Move the duplicated constant and function into a single shared helper module and import it from both Add and Edit offer modals: create/export MAX_DISPLAY_TEXT_LENGTH and getLengthHint from a new offers helper (e.g., offersHelpers) and replace the local definitions in add-offer-modal.tsx and edit-retention-offer-modal.tsx with imports; ensure the exported getLengthHint returns the same JSX structure and keep the same prop signature (length, maxLength, helperText) so existing callers continue to work and remove the duplicate local declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx`:
- Around line 443-465: The validation currently allows decimal values for
repeating duration; update the validation in AddOfferModal (where formState is
validated — look for the checks referencing formState.duration,
formState.durationInMonths and newErrors.durationInMonths) to enforce an integer
> 0 when formState.duration === 'repeating' and formState.type !== 'trial'
(e.g., check Number.isInteger(formState.durationInMonths) and > 0) and set a
clear error like "Enter a whole number greater than 0." Replace or augment the
existing durationInMonths check so decimals are rejected client-side before
submission.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx`:
- Around line 33-44: Move the duplicated constant and function into a single
shared helper module and import it from both Add and Edit offer modals:
create/export MAX_DISPLAY_TEXT_LENGTH and getLengthHint from a new offers helper
(e.g., offersHelpers) and replace the local definitions in add-offer-modal.tsx
and edit-retention-offer-modal.tsx with imports; ensure the exported
getLengthHint returns the same JSX structure and keep the same prop signature
(length, maxLength, helperText) so existing callers continue to work and remove
the duplicate local declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9779d2b6-6437-4285-b8f1-c0200c3f6c69
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsxapps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsxapps/admin-x-settings/test/acceptance/membership/offers.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx (1)
37-37: ExtractMAX_DISPLAY_TEXT_LENGTHto a shared offers constant.Line 37 duplicates the same value used in the add-offer modal, which can drift. Prefer a shared
offers/constantsexport and import it in both files.♻️ Proposed refactor
-const MAX_DISPLAY_TEXT_LENGTH = 191; +import {MAX_DISPLAY_TEXT_LENGTH} from './offer-constants';// apps/admin-x-settings/src/components/settings/growth/offers/offer-constants.ts export const MAX_DISPLAY_TEXT_LENGTH = 191;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx` at line 37, Replace the duplicated const MAX_DISPLAY_TEXT_LENGTH in edit-retention-offer-modal.tsx with a single shared export: create a new module (e.g., offer-constants.ts) that exports const MAX_DISPLAY_TEXT_LENGTH = 191, then import that constant into edit-retention-offer-modal.tsx and the add-offer modal so both use the same symbol; update any local references to use the imported MAX_DISPLAY_TEXT_LENGTH and remove the local duplicate definition.apps/admin-x-settings/test/acceptance/membership/offers.test.ts (1)
144-173: Consolidate duplicate required-fields validation test.Line 144 duplicates the scenario/assertions already covered by Line 82. Keeping one canonical case will reduce suite noise and maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts` around lines 144 - 173, Remove the duplicate acceptance test block named "Shows validation errors" (the test(...) that opens offers modal, clicks "New offer", then "Publish" on the add-offer-modal and asserts validation messages in add-offer-sidebar) and keep the original canonical test that covers required-field validation to avoid duplicate assertions; if there are any unique assertions here not present in the canonical test, merge those specific expect(...) checks (e.g., checks for "Name is required", "Code is required", "Enter an amount between 1 and 100%.", "Display title is required") into the existing validation test instead of keeping both blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`:
- Line 37: Replace the duplicated const MAX_DISPLAY_TEXT_LENGTH in
edit-retention-offer-modal.tsx with a single shared export: create a new module
(e.g., offer-constants.ts) that exports const MAX_DISPLAY_TEXT_LENGTH = 191,
then import that constant into edit-retention-offer-modal.tsx and the add-offer
modal so both use the same symbol; update any local references to use the
imported MAX_DISPLAY_TEXT_LENGTH and remove the local duplicate definition.
In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts`:
- Around line 144-173: Remove the duplicate acceptance test block named "Shows
validation errors" (the test(...) that opens offers modal, clicks "New offer",
then "Publish" on the add-offer-modal and asserts validation messages in
add-offer-sidebar) and keep the original canonical test that covers
required-field validation to avoid duplicate assertions; if there are any unique
assertions here not present in the canonical test, merge those specific
expect(...) checks (e.g., checks for "Name is required", "Code is required",
"Enter an amount between 1 and 100%.", "Display title is required") into the
existing validation test instead of keeping both blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d4d7169-5063-4860-82e1-91557e208390
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsxapps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsxapps/admin-x-settings/test/acceptance/membership/offers.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-settings/src/components/settings/growth/offers/add-offer-modal.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/admin-x-settings/test/acceptance/membership/offers.test.ts (2)
732-735: This assertion is a bit too coupled to normalization internals.The test intent is disable/archive behavior, but asserting
0 -> 1normalization (duration_in_months: 1) makes it brittle to internal coercion changes. Consider using a valid duration input here and reserving normalization-specific expectations for a dedicated test.Also applies to: 755-755, 773-775
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts` around lines 732 - 735, The test is asserting normalization internals (0 -> 1) via duration_in_months which makes it brittle; for the disable/archive behavior tests in offers.test.ts (the object with display_title 'Before you go' and duration: 'repeating') use a valid duration value (e.g., set duration_in_months to a realistic number like 3) and remove any assertions that rely on coercion/normalization; instead keep this test focused on disable/archive behavior and move any normalization-specific expectations into a dedicated test that explicitly asserts the 0→1 normalization behavior.
143-171: Duplicate validation scenario can be consolidated.This test now duplicates
Errors if required fields are missing(Line 82-110) with the same setup and assertions. Consider removing one or consolidating into a shared helper to reduce maintenance overhead.♻️ Suggested simplification
- test('Shows validation errors', async ({page}) => { - await mockApi({page, requests: { - browseOffers: {method: 'GET', path: '/offers/', response: responseFixtures.offers}, - ...globalDataRequests, - browseSettings: {...globalDataRequests.browseSettings, response: settingsWithStripe}, - browseOffersById: {method: 'GET', path: `/offers/${responseFixtures.offers.offers![0].id}/`, response: responseFixtures.offers}, - browseTiers: {method: 'GET', path: '/tiers/', response: responseFixtures.tiers}, - addOffer: {method: 'POST', path: `/offers/`, response: { - offers: [{ - name: 'Coffee Tuesdays', - id: '6487ea6464fca78ec2fff5fe', - code: 'coffee-tuesdays', - amount: 5 - }] - }} - }}); - - await page.goto('/'); - const section = page.getByTestId('offers'); - await section.getByRole('button', {name: 'Manage offers'}).click(); - const modal = page.getByTestId('offers-modal'); - await modal.getByRole('button', {name: 'New offer'}).click(); - const addModal = page.getByTestId('add-offer-modal'); - await addModal.getByRole('button', {name: 'Publish'}).click(); - const sidebar = addModal.getByTestId('add-offer-sidebar'); - await expect(sidebar).toContainText(/Name is required/); - await expect(sidebar).toContainText(/Code is required/); - await expect(sidebar).toContainText(/Enter an amount between 1 and 100%./); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts` around lines 143 - 171, The test "Shows validation errors" duplicates the earlier "Errors if required fields are missing" case; remove the duplicate or consolidate by extracting a shared helper that performs the common setup (mockApi calls and navigation to the add-offer modal) and the common assertions (checking sidebar via getByTestId('add-offer-sidebar') contains the validation messages), then have both tests invoke that helper or keep only one test; update references in the file to use the helper or remove the extra test to avoid redundant assertions against the add-offer flow (selectors: getByTestId('offers'), getByTestId('offers-modal'), getByTestId('add-offer-modal'), getByTestId('add-offer-sidebar')).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin-x-settings/test/acceptance/membership/offers.test.ts`:
- Around line 732-735: The test is asserting normalization internals (0 -> 1)
via duration_in_months which makes it brittle; for the disable/archive behavior
tests in offers.test.ts (the object with display_title 'Before you go' and
duration: 'repeating') use a valid duration value (e.g., set duration_in_months
to a realistic number like 3) and remove any assertions that rely on
coercion/normalization; instead keep this test focused on disable/archive
behavior and move any normalization-specific expectations into a dedicated test
that explicitly asserts the 0→1 normalization behavior.
- Around line 143-171: The test "Shows validation errors" duplicates the earlier
"Errors if required fields are missing" case; remove the duplicate or
consolidate by extracting a shared helper that performs the common setup
(mockApi calls and navigation to the add-offer modal) and the common assertions
(checking sidebar via getByTestId('add-offer-sidebar') contains the validation
messages), then have both tests invoke that helper or keep only one test; update
references in the file to use the helper or remove the extra test to avoid
redundant assertions against the add-offer flow (selectors:
getByTestId('offers'), getByTestId('offers-modal'),
getByTestId('add-offer-modal'), getByTestId('add-offer-sidebar')).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c57c402-4b01-46a3-848d-7c4a77a38ee0
📒 Files selected for processing (1)
apps/admin-x-settings/test/acceptance/membership/offers.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes https://linear.app/ghost/issue/BER-3392
closes https://linear.app/ghost/issue/BER-3218
Max. length display title / description fields:
Inline validation for floating numbers for free-months / duration-in-months fields: