fix: page templates date field required#992
Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR threads an ChangesGlobal page template and save flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PageTemplateListPage
participant PageTemplatePopup
participant PageModules
participant MediaRequestModule
participant savePageTemplate
PageTemplateListPage->>PageTemplatePopup: pass isGlobal
PageTemplatePopup->>PageModules: pass isGlobal
PageModules->>MediaRequestModule: pass showUploadDeadline={!isGlobal}
PageTemplatePopup->>savePageTemplate: save entity
savePageTemplate-->>PageTemplatePopup: resolve or reject save
PageTemplateListPage->>PageTemplateListPage: refresh page templates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses cases where upload_deadline was being treated as a required field for page template MEDIA modules (notably for global templates), by omitting it from the payload when unset and updating the UI/validation to allow it to be absent.
Changes:
- Update module normalization to remove
upload_deadlinefrom the outgoing payload when it is null/undefined. - Relax form validation for
upload_deadlineand hide the deadline field for global templates in the MEDIA module UI. - Add/adjust constants and unit tests to cover the new normalization behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/page-template.js | Omits upload_deadline from normalized MEDIA modules when unset. |
| src/utils/constants.js | Adds grid column constants used by the updated MEDIA module layout. |
| src/utils/tests/page-template.test.js | Adds coverage ensuring upload_deadline is omitted when null/undefined. |
| src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js | Threads isGlobal into MEDIA module rendering. |
| src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.js | Hides the deadline field for global templates and adjusts layout sizing. |
| src/pages/sponsors-global/page-templates/page-template-popup/index.js | Makes upload_deadline nullable in schema and passes isGlobal down. |
| src/pages/sponsors-global/page-templates/page-template-list-page.js | Marks the popup usage as isGlobal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d3d139 to
26b5622
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/sponsors-global/page-templates/page-template-popup/index.js (1)
125-127: 🎯 Functional Correctness | 🔴 Critical
validationContextis not a Formik option; global template submission is blocked.
useFormik(v2.4.6) ignoresvalidationContext, so the Yup schema resolves$isGlobaltoundefined. This forcesupload_deadlineto remain.required()even when the field is hidden for global templates, preventing form submission.Replace
validationSchemawith a customvalidatefunction to inject the context:🛠️ Fix: Inject context via custom validate
const formik = useFormik({ initialValues: pageTemplate, - validationContext: { isGlobal }, - validationSchema: yup.object().shape({ - code: yup.string().required(T.translate("validation.required")), - name: yup.string().required(T.translate("validation.required")), - ...(showSponsorships && { - sponsorship_types: yup - .array() - .min(1, T.translate("validation.required")) - }), - modules: yup.array().of(moduleSchema) - }), + validate: (values) => + validationSchema + .validate(values, { abortEarly: false, context: { isGlobal } }) + .then(() => ({})) + .catch((err) => yupToFormErrors(err)), });Define
validationSchemacontaining themoduleSchemalogic before this hook.Ensure
yupToFormErrorsis imported.🤖 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-global/page-templates/page-template-popup/index.js` around lines 125 - 127, useFormik is currently passing validationContext, which Formik ignores, so the Yup schema in this popup cannot receive isGlobal and global template submission stays blocked. Update the page-template-popup form hook to replace validationSchema usage with a custom validate function that runs the existing moduleSchema validation with context, and convert Yup errors with yupToFormErrors so hidden global-only fields like upload_deadline do not remain required. Keep the fix localized to the useFormik setup in the page template popup component and preserve the current moduleSchema logic.
🤖 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.
Duplicate comments:
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js`:
- Around line 125-127: useFormik is currently passing validationContext, which
Formik ignores, so the Yup schema in this popup cannot receive isGlobal and
global template submission stays blocked. Update the page-template-popup form
hook to replace validationSchema usage with a custom validate function that runs
the existing moduleSchema validation with context, and convert Yup errors with
yupToFormErrors so hidden global-only fields like upload_deadline do not remain
required. Keep the fix localized to the useFormik setup in the page template
popup component and preserve the current moduleSchema logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32cffde2-ea83-41d3-9605-c42134967c97
📒 Files selected for processing (7)
src/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors-global/page-templates/page-template-popup/index.jssrc/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.jssrc/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.jssrc/utils/__tests__/page-template.test.jssrc/utils/constants.jssrc/utils/page-template.js
| }), | ||
| modules: yup.array().of(moduleSchema) | ||
| }), | ||
| validate: (values) => { |
There was a problem hiding this comment.
@tomrndom Switching from the declarative validationSchema prop to this manual validate callback was necessary to pass context: { isGlobal } into Yup, but it also drops Formik's built-in prepareDataForValidation step (which normalizes '' to undefined before validation runs) for every field in the form, not just upload_deadline. I checked this repo's yup (1.7.1): .required() still rejects '' directly, so required-field enforcement isn't broken. But non-required numeric fields cleared to '' (e.g. max_file_size) now surface as a Yup type-cast error ("must be a number type... cast from the value """) instead of Formik's normalized "required" message - a behavior change to every field's validation UX as a side effect of this fix, and it isn't called out or tested.
Suggested fix: coerce '' to undefined on values before calling validationSchema.validateSync(...), mirroring Formik's prepareDataForValidation, so cleared fields keep producing the same "required" error message they did before this refactor.
There was a problem hiding this comment.
@tomrndom why don't we just add a hidden field with the isGlobal value ? then we can just have normal conditional validation . I think thats more straight forward
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
aefd24b to
49f0b0a
Compare
| }), | ||
| modules: yup.array().of(moduleSchema) | ||
| }), | ||
| validate: (values) => { |
There was a problem hiding this comment.
@tomrndom why don't we just add a hidden field with the isGlobal value ? then we can just have normal conditional validation . I think thats more straight forward
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/pages/sponsors-global/page-templates/page-template-list-page.js`:
- Line 117: Update both save branches in the page-template save action to
rethrow API errors or remove their terminal catch, ensuring the promise returned
through handleSavePageTemplate remains rejected when saving fails. Preserve
successful-save behavior while preventing the popup from closing or resetting
after an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fd626ec-b13d-4ed8-b146-0ebe6eda56a0
📒 Files selected for processing (15)
src/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors-global/page-templates/page-template-popup/__tests__/page-template-module-form.test.jssrc/pages/sponsors-global/page-templates/page-template-popup/__tests__/page-template-popup.test.jssrc/pages/sponsors-global/page-templates/page-template-popup/index.jssrc/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.jssrc/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-info-module.jssrc/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.jssrc/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.jssrc/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.jssrc/utils/__tests__/page-template.test.jssrc/utils/constants.jssrc/utils/page-template.js
🚧 Files skipped from review as they are similar to previous changes (13)
- src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/tests/sponsor-pages-tab.test.js
- src/pages/sponsors/show-pages-list-page/tests/show-pages-list-page.test.js
- src/pages/sponsors/show-pages-list-page/index.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.js
- src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.js
- src/utils/tests/page-template.test.js
- src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-info-module.js
- src/pages/sponsors-global/page-templates/page-template-popup/tests/page-template-popup.test.js
- src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js
- src/utils/page-template.js
- src/pages/sponsors-global/page-templates/page-template-popup/tests/page-template-module-form.test.js
- src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.js
- src/pages/sponsors-global/page-templates/page-template-popup/index.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| }) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| throw err; |
There was a problem hiding this comment.
you are already returning the promise so this throw is redundant
| }) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| throw err; |
There was a problem hiding this comment.
you are already returning the promise so this throw is redundant
ref: https://app.clickup.com/t/9014802374/86b9c2nd4
Summary by CodeRabbit
upload_deadlinevalues are cleared and omitted when the field is hidden.