Skip to content

fix: make managed forms dates optional, adjust payload#976

Merged
smarcet merged 1 commit into
masterfrom
fix/sponsor-managed-popup-dates-required
Jun 11, 2026
Merged

fix: make managed forms dates optional, adjust payload#976
smarcet merged 1 commit into
masterfrom
fix/sponsor-managed-popup-dates-required

Conversation

@tomrndom

@tomrndom tomrndom commented Jun 11, 2026

Copy link
Copy Markdown

ref: https://app.clickup.com/t/9014802374/86badfw5a

Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation logic for sponsor form date fields to properly handle cases where opening or expiration dates are optional or absent.
    • Enhanced timestamp handling to only convert dates when they contain values.

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR makes sponsor customized form date fields optional by adding conditional validation rules and corresponding normalization guards. The form validation in CustomizedForm shifts from required fixed dates to nullable fields with cross-field dependencies using yup.when, while the normalization layer conditionally converts timestamps only when present.

Changes

Optional Date Fields

Layer / File(s) Summary
Conditional date field validation
src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/customized-form/customized-form.js
opens_at and expires_at become nullable fields with conditional requirements: opens_at is required only if expires_at is present, and vice versa; when both exist, expires_at must satisfy isAfterDateField relative to opens_at. The opensAtValidation import helper is removed in favor of inline yup.when rules.
Timestamp normalization guards
src/actions/sponsor-forms-actions.js
normalizeSponsorCustomizedForm now guards Moment-timezone conversions for opens_at and expires_at with truthy checks, skipping conversion when either field is absent.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • smarcet
  • martinquiroga-exo

Poem

🐰 A rabbit hops through sponsor dates,
Where forms once locked at both the gates—
Now flexible with conditional cheer,
One date or two, the fields are clear!
Timestamps guard themselves with care,
When present, converted in the air. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: make managed forms dates optional, adjust payload' directly and clearly describes the main changes: making form dates optional and adjusting the payload accordingly, which aligns with the changes in both modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sponsor-managed-popup-dates-required

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed due to a network error.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/customized-form/customized-form.js (1)

154-171: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove or conditionally set required prop on date pickers.

The required prop at lines 160 and 169 shows visual indicators (typically asterisks) that these fields are mandatory. However, the validation schema now allows both fields to be empty. This creates a UX inconsistency where users see required markers but can submit without filling them.

Either remove the required prop from both datepickers, or make it conditional based on whether the other field has a value.

🛠️ Proposed fix to remove required prop
             <Grid2 size={4}>
               <MuiFormikDatepicker
                 name="opens_at"
                 label={T.translate(
                   "edit_sponsor.forms_tab.customized_form.opens_at"
                 )}
-                required
               />
             </Grid2>
             <Grid2 size={4}>
               <MuiFormikDatepicker
                 name="expires_at"
                 label={T.translate(
                   "edit_sponsor.forms_tab.customized_form.expires_at"
                 )}
-                required
               />
             </Grid2>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/customized-form/customized-form.js`
around lines 154 - 171, The MuiFormikDatepicker components for fields "opens_at"
and "expires_at" are marked required but the validation schema permits empties;
either remove the required prop from both MuiFormikDatepicker instances (the
ones with name="opens_at" and name="expires_at") or make the required prop
conditional by reading Formik state (e.g., via useFormikContext or formik props)
so required is true only when the counterpart field has a value; update the two
occurrences in customized-form.js accordingly to keep the UI consistent with
validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/customized-form/customized-form.js`:
- Around line 154-171: The MuiFormikDatepicker components for fields "opens_at"
and "expires_at" are marked required but the validation schema permits empties;
either remove the required prop from both MuiFormikDatepicker instances (the
ones with name="opens_at" and name="expires_at") or make the required prop
conditional by reading Formik state (e.g., via useFormikContext or formik props)
so required is true only when the counterpart field has a value; update the two
occurrences in customized-form.js accordingly to keep the UI consistent with
validation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ffbb5540-1677-4262-98db-1b012426c504

📥 Commits

Reviewing files that changed from the base of the PR and between 2c21d1d and 25d6eb2.

📒 Files selected for processing (2)
  • src/actions/sponsor-forms-actions.js
  • src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/customized-form/customized-form.js

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@smarcet smarcet merged commit 2f1f3b3 into master Jun 11, 2026
9 checks passed
smarcet pushed a commit that referenced this pull request Jun 12, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.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