Conversation
…d of root message
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds client-side validation/error-mapping utilities, new translation keys, and integrates them across Bulletins, Events, and date-field components. Introduces date and URL-or-NA validators, server-error mapping, and form-validation flows. Updates components to accept validation rules and wires save actions to validate-before-save with surfacing of server-side field errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Admin User
participant UI as Bulletin Dialog (Vue)
participant V as validationRules / form
participant API as Server API
U->>UI: Click "Save"
UI->>V: validateFormAndSave()
V->>V: form.validate() + field rules (date/urlOrNA)
alt Validation fails
V-->>UI: invalid
UI->>U: Show snack/scroll to errors
else Validation passes
UI->>API: save(create/update)
alt API success
API-->>UI: 200 OK
UI->>U: Close dialog / success snack
else API returns field errors
API-->>UI: 4xx + errors
UI->>UI: errorsMap = mapResponseErrors(err)
UI->>V: validateForm() to surface externalError(...)
UI->>U: Highlight fields with server messages
end
end
sequenceDiagram
autonumber
participant U as Editor
participant ES as EventsSection
participant DF as PopDateField (from/to)
participant V as validationRules
U->>ES: Submit Event
ES->>ES: validateFormAndSave()
ES->>DF: from_date rules [date(), dateBeforeOtherDate(to)]
ES->>DF: to_date rules [date(), dateAfterOtherDate(from)]
DF->>V: evaluate rules with dayjs
alt Rules or business checks fail
ES-->>U: Snack + prevent save
else
ES->>API: Save event
API-->>ES: OK
ES-->>U: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
enferno/static/js/common/config.js (3)
17-22: Date validator is fine; consider trimming inputs.Minor: Users may paste values with leading/trailing spaces. Trimming prevents false negatives.
- return v => { - if (!v) return true; // allow empty values - return dayjs(v).isValid() || message; - }; + return v => { + const s = typeof v === 'string' ? v.trim() : v; + if (!s) return true; // allow empty values + return dayjs(s).isValid() || message; + };
123-129: mapResponseErrors: handle array and non-plain error shapes to avoid dropping useful messages.If API returns an array or nested lists, the function currently returns undefined. Fall back to joining arrays and mapping to root when no field is provided.
function mapResponseErrors(error) { - const errors = error?.response?.data?.errors - if (!errors) return - if (isPlainObject(errors)) { - return Object.fromEntries(Object.entries(errors).map(([key, value]) => [key.replace('item.', ''), value])) - } + const errors = error?.response?.data?.errors; + if (!errors) return; + if (isPlainObject(errors)) { + return Object.fromEntries( + Object.entries(errors).map(([key, value]) => [key.replace(/^item\./, ''), Array.isArray(value) ? value.join(', ') : value]) + ); + } + if (Array.isArray(errors)) { + return { '__root__': errors.join(', ') }; + } }
359-376: HTML-response detection: make content-type check case-insensitive and resilient to charset params.Minor hardening to avoid false negatives.
- const ct = response?.headers?.['content-type'] || ''; - if (ct.includes('text/html') and response?.status) { + const ct = (response?.headers?.['content-type'] || '').toLowerCase(); + if (ct.startsWith('text/html') && response?.status) { return getInfraMessage(response.status); }enferno/static/js/components/PopDateField.js (1)
2-2: Defaulting to validationRules.date() is good; verify global availability on all pages.If PopDateField is used on routes where config.js isn’t loaded, validationRules will be undefined at runtime.
- data: () => ({ - validationRules: validationRules - }), + data: () => ({ + validationRules: (typeof validationRules !== 'undefined' ? validationRules : { date: () => () => true }) + }),Optionally, import or attach validationRules via provide/inject to make the dependency explicit.
Also applies to: 4-6, 22-22
enferno/admin/templates/admin/bulletins.html (1)
721-721: Reset errorsMap on dialog open/close and after successful save — v-form verifiedFound at enferno/admin/templates/admin/partials/bulletin_dialog.html:18; this.$refs.form.validate() is safe. Set this.errorsMap = null when opening/closing the dialog (e.g., editItem(), close() when resetting editedItem) and after successful api.put / api.post responses to avoid stale externalError() results. Optionally move validation into save() and return early on invalid.
enferno/admin/templates/admin/partials/bulletin_dialog.html (2)
236-236: Avoid full-form revalidation on every keystrokeCalling validateForm() on each input can be chatty and janky on slower devices. Prefer validating on blur/change, or debounce the call.
Example:
- @input="resetErrorFromMap('source_link', errorsMap); validateForm()" + @input="resetErrorFromMap('source_link', errorsMap)" + @blur="validateForm()"
239-240: Wire the checkbox to the model to reflect NA intentMake the checkbox set/clear the field so urlOrNA() and required rules behave predictably.
- <v-checkbox class="mx-2" label="{{ _('Not Available') }}" v-model="source_alt" @change="resetErrorFromMap('source_link', errorsMap); validateForm()"> + <v-checkbox + class="mx-2" + label="{{ _('Not Available') }}" + v-model="source_alt" + @update:modelValue="val => { resetErrorFromMap('source_link', errorsMap); editedItem.source_link = val ? 'NA' : (editedItem.source_link || ''); validateForm(); }">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
enferno/admin/templates/admin/bulletins.html(4 hunks)enferno/admin/templates/admin/jsapi.jinja2(1 hunks)enferno/admin/templates/admin/partials/bulletin_dialog.html(2 hunks)enferno/static/js/common/config.js(3 hunks)enferno/static/js/components/EventsSection.js(4 hunks)enferno/static/js/components/PopDateField.js(2 hunks)enferno/static/js/components/PopDateTimeField.js(2 hunks)
🔇 Additional comments (10)
enferno/static/js/common/config.js (3)
131-135: LGTM: small utility is clear and side-effect free beyond the passed object.
355-358: LGTM: honoring server-provided message as a fallback is appropriate.
35-40: ```shell
#!/bin/bash
set -euo pipefailecho "== enferno/static/js/common/config.js (head) =="
if [ -f "enferno/static/js/common/config.js" ]; then
sed -n '1,240p' "enferno/static/js/common/config.js"
else
echo "MISSING: enferno/static/js/common/config.js"
fiecho
echo "== ripgrep: files mentioning 'dayjs' (include ignored) =="
rg -n --hidden -uu -S "dayjs" -C2 || echo "NO_MATCH"echo
echo "== ripgrep: exact 'dayjs.extend(' occurrences =="
rg -n --hidden -uu -S "dayjs.extend(" -C2 || echo "NO_MATCH"echo
echo "== ripgrep: literal 'dayjs/plugin' path occurrences =="
rg -n --hidden -uu -S "dayjs/plugin" -C2 || echo "NO_MATCH"echo
echo "== ripgrep: plugin filenames (isSameOrBefore|isSameOrAfter|customParseFormat) =="
rg -n --hidden -uu -S "isSameOrBefore|isSameOrAfter|customParseFormat" -C2 || echo "NO_MATCH"echo
echo "== ripgrep: import/require of dayjs plugins =="
rg -n --hidden -uu -S "import .*dayjs/plugin|require\(.*dayjs/plugin" -C2 || echo "NO_MATCH"echo
echo "== package.json: check for dayjs dependency =="
if [ -f package.json ]; then
jq -r '.dependencies.dayjs // .devDependencies.dayjs // "NOT_FOUND"' package.json || true
else
echo "NO package.json"
fi</blockquote></details> <details> <summary>enferno/admin/templates/admin/jsapi.jinja2 (1)</summary><blockquote> `26-29`: **Add missing i18n keys to messages.pot and update .po files.** Keys present in enferno/admin/templates/admin/jsapi.jinja2 but not found in messages.pot (repo root): invalidDate_, fromDateMustBeBeforeTheToDate_, toDateMustBeAfterTheFromDate_, mustBeAValidUrlOrNa_. Run the project's extraction (e.g. babel/pybabel or project i18n script) and update all locale .po files before merging. </blockquote></details> <details> <summary>enferno/static/js/components/PopDateTimeField.js (1)</summary><blockquote> `14-17`: **Propagating rules to inner date field is correct; no behavior regressions observed.** Also applies to: 78-78 </blockquote></details> <details> <summary>enferno/static/js/components/EventsSection.js (5)</summary><blockquote> `41-57`: **Renamed validator reads well and preserves behavior** checkEventFormRules is clear and keeps the snack-on-missing pattern. LGTM. --- `164-166`: **Save button now uses validateFormAndSave — good consistency** Matches the new flow. LGTM. --- `169-169`: **Submitting the form triggers validate + rules — good** Enter key path is covered here for Events. LGTM. --- `227-232`: **Date rules: confirm optional behavior and cross-field robustness** - Ensure validationRules.date() treats empty values as valid so dates remain optional when location/title satisfy business rules. - Verify dateBeforeOtherDate/dateAfterOtherDate handle null/invalid peers gracefully to avoid over-restricting. If needed, wrap your validators to short-circuit on falsy value: ```diff - :rules="[validationRules.date(), validationRules.dateBeforeOtherDate(editedEvent.to_date)]" + :rules="[(v) => !v || validationRules.date()(v), validationRules.dateBeforeOtherDate(editedEvent.to_date)]"Also applies to: 231-232
58-69: Use component-scoped translations and verify scrollToFirstError availability
- Replace global translations with component-scoped this.translations in enferno/static/js/components/EventsSection.js (lines 58–69).
- Confirm scrollToFirstError is exported/imported or attached globally at runtime; repo search returned no matches — add an import or define the helper.
- this.$root.showSnack(translations.pleaseReviewFormForErrors_); + this.$root.showSnack(this.translations.pleaseReviewFormForErrors_);
level09
left a comment
There was a problem hiding this comment.
This is great , but I was thinking we could maybe enhance our overall validation appraoch and simplify it signficantly if we use Vuetify3 natvie validation features:
Can you review the plan below and tell me what you think?
Idea Summary
Vuetify 3 can provide the exact same user experience while eliminating ~60% of custom validation code.
Current vs Vuetify 3 Approach
What You Have Now
// Custom error mapping, manual validation triggers, HTML in errors
this.errorsMap = mapResponseErrors(err);
this.validateForm();
@input="resetErrorFromMap('source_link', errorsMap); validateForm()"With Vuetify 3 Native Features
// Direct error binding, automatic validation, no custom functions needed
<v-text-field
v-model="item.source_link"
:rules="[rules.required(), rules.urlOrNA()]"
:error-messages="serverErrors?.source_link"
@update:model-value="serverErrors.source_link = null"
/>Feature Parity Check ✅
| Current Feature | Vuetify 3 Support | How |
|---|---|---|
| Show server errors on fields | ✅ | :error-messages prop |
| Clear errors on input | ✅ | @update:model-value event |
| Scroll to first error | ✅ | Built-in with scrollToError |
| Custom validation rules | ✅ | Same rule functions work |
| Date comparison validation | ✅ | Rules can access other field values |
| Show validation on save | ✅ | validate-on="submit" |
| Red field highlighting | ✅ | Automatic with errors |
| Generic error toasts | ✅ | Keep current toast system |
Real Implementation Example
Before (Current PR - 40+ lines)
data: () => ({
errorsMap: null,
}),
methods: {
resetErrorFromMap: resetErrorFromMap,
validateForm() {
this.$refs.form.validate().then(({ valid, errors }) => {
if (!valid) {
this.showSnack("Please review the form for errors.")
scrollToFirstError(errors)
}
});
},
save() {
// ... save logic
.catch(err => {
this.errorsMap = mapResponseErrors(err);
this.validateForm()
});
}
}After (Vuetify 3 - 15 lines)
data: () => ({
serverErrors: {},
}),
methods: {
async save() {
const { valid } = await this.$refs.form.validate()
if (!valid) {
this.showSnack("Please review the form for errors.")
return
}
// ... save logic
.catch(err => {
this.serverErrors = err.response?.data?.errors || {}
this.$refs.form.validate() // Re-validate to show errors
});
}
}What Changes for Users?
Nothing. The UX remains identical:
- Same error messages
- Same red highlighting
- Same scroll behavior
- Same validation timing
What You Can Remove
mapResponseErrors()functionresetErrorFromMap()functionerrorsMapdata property- Manual
@inputhandlers for error clearing - Custom
scrollToFirstError()(use Vuetify's)
Migration Path (2 Steps)
Step 1: Update Template
<!-- From -->
<v-text-field
:rules="[rules.required(), rules.urlOrNA(), rules.externalError(errorsMap?.source_link)]"
@input="resetErrorFromMap('source_link', errorsMap); validateForm()"
/>
<!-- To -->
<v-text-field
:rules="[rules.required(), rules.urlOrNA()]"
:error-messages="serverErrors?.source_link"
@update:model-value="serverErrors.source_link = null"
/>Step 2: Simplify Methods
Replace complex error handling with direct assignment:
catch (err) {
// Simply assign errors, Vuetify handles display
this.serverErrors = err.response?.data?.errors || {}
}Benefits You Get
- Less code - Remove ~100 lines of custom validation logic
- Better performance - Vuetify optimizes validation cycles
- Accessibility - Proper ARIA announcements built-in
- Future-proof - Leverage framework improvements automatically
|
@level09 i made an update to use your suggestion on a/b/i dialogs, it should now be simpler to display server errors |
level09
left a comment
There was a problem hiding this comment.
@apodacaduron This is perfect. only one small typo to fix before we can merge.
…463-Validation-Error-Messages-Lack-Detail
|
Summary: Updated snackbar error handling and improved form validations.
|
… line on snackbar
level09
left a comment
There was a problem hiding this comment.
Perfect. All comments are addressed correctly. ready to merge 🚀
Jira Issue
Description
When a field contains an invalid or unacceptable value, the system prevents saving the item. In some cases, the field is highlighted in red and the window scrolls to it, clearly indicating the problem.
In many cases, however, the system only shows a generic “Validation failed” message without explaining which field or value caused the problem. Examples include:
In events, if the “To date” field is earlier than the “From date” field.
In the source link field, when the value is unacceptable.
In some actor profiles, when a field contains an invalid value.
Note 1: These actor profiles were updated and saved previously, suggesting (as I think) that either validations were not in place before, or the field type has changed.
Note 2: Regarding the actor profile issue (as shown in the attached video), after removing the problematic value and saving, the value can be re-entered manually without causing an error.
Impact:
Users cannot easily identify or correct errors.
Leads to confusion, wasted time, and repeated failed attempts to save items.
Checklist
API Changes (if applicable)
Additional Notes
[Any other relevant information]
Summary by CodeRabbit
New Features
Refactor