- Fix CRUD issue in policy manager #2289
Conversation
… server expectations - Add missing "Deprecated" status option to dropdown - Set default status for new policies to "Under Review" - Add comprehensive client-side validation for title, content length, and initial status - Implement server error handling and mapping to display validation errors in UI
WalkthroughAdds policy content field and validation to PolicyDetailsModal, updates default status to "Under Review", enhances server-side error extraction/mapping (including content and tags), and displays inline error messages. PolicyForm updates status options ("Under Review", adds "Deprecated") and shows inline tags error. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as PolicyDetailsModal
participant API as Policies API
U->>M: Click Save
M->>API: POST/PUT policy {title, status, tags, content, ...}
alt Success
API-->>M: 200 OK {policy}
M-->>U: Close modal / refresh
else Validation Error
API-->>M: 400 {errors: {title|status|tags|content|...}}
M->>M: Map server fields to FormErrors
M-->>U: Show inline errors (incl. content & tags)
else Other Error
API-->>M: 5xx / network error
M->>M: Log extended error info
M-->>U: Show generic failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Clients/src/presentation/components/Policies/PolicyDetailsModal.tsx (1)
90-131: Consider adding client-side content validation.The
validateFormfunction validates title, status, tags, nextReviewDate, and assignedReviewers, but does not validate thecontentfield despiteFormErrors.contentbeing defined. While server-side validation exists (lines 246-247), adding client-side validation would provide immediate feedback to users.Consider adding content validation:
if (policyAssignedReviewers.length === 0) { newErrors.assignedReviewers = "At least one reviewer is required"; } + + // Content validation + const contentHtml = await serializeHtml(editor); + const contentText = contentHtml.replace(/<[^>]*>/g, '').trim(); + if (contentText.length === 0) { + newErrors.content = "Content is required"; + } else if (contentText.length > 10000) { + newErrors.content = "Content exceeds maximum length of 10,000 characters"; + } setErrors(newErrors); return Object.keys(newErrors).length === 0;Note: This would require making
validateFormasync or moving content validation to thesavefunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Clients/src/presentation/components/Policies/PolicyDetailsModal.tsx(7 hunks)Clients/src/presentation/components/Policies/PolicyForm.tsx(2 hunks)
🔇 Additional comments (5)
Clients/src/presentation/components/Policies/PolicyForm.tsx (2)
36-43: LGTM! Status options updated correctly.The status list now includes "Under Review" (replacing "In review") and the new "Deprecated" option, aligning with the PR objectives and maintaining consistency with PolicyDetailsModal.tsx.
276-288: LGTM! Tags error display implemented correctly.The inline error message for tags follows consistent styling patterns and uses theme colors appropriately. The implementation aligns well with the content error display in PolicyDetailsModal.tsx.
Clients/src/presentation/components/Policies/PolicyDetailsModal.tsx (3)
44-51: LGTM! FormErrors interface appropriately extended.The addition of the optional
contentfield enables validation error tracking for the policy content, consistent with other form fields.
133-140: LGTM! Default status correctly set to "Under Review".Both the initial formData state (line 135) and the reset state for new policies (line 161) now default to "Under Review", fulfilling the PR objective and maintaining consistency with the updated status options.
Also applies to: 159-166
492-504: LGTM! Content error display implemented correctly.The inline error message for content follows consistent styling patterns, uses theme colors with an appropriate fallback, and maintains visual consistency with the tags error display in PolicyForm.tsx.
| } catch (err: any) { | ||
| // setIsSubmitting(false); | ||
| console.error(err); | ||
| console.error("Full error object:", err); | ||
| console.error("Original error:", err?.originalError); | ||
| console.error("Original error response:", err?.originalError?.response); | ||
|
|
||
| // Handle server validation errors - the CustomException is in originalError | ||
| const errorData = err?.originalError?.response || err?.response?.data || err?.response; | ||
| console.error("Error data:", errorData); | ||
|
|
||
| if (errorData?.errors) { | ||
| console.error("Processing server errors:", errorData.errors); | ||
| const serverErrors: FormErrors = {}; | ||
| errorData.errors.forEach((error: any) => { | ||
| console.error("Processing error:", error); | ||
| if (error.field === 'title') { | ||
| serverErrors.title = error.message; | ||
| } else if (error.field === 'status') { | ||
| serverErrors.status = error.message; | ||
| } else if (error.field === 'tags') { | ||
| serverErrors.tags = error.message; | ||
| } else if (error.field === 'content_html') { | ||
| serverErrors.content = error.message; | ||
| } else if (error.field === 'next_review_date') { | ||
| serverErrors.nextReviewDate = error.message; | ||
| } else if (error.field === 'assigned_reviewer_ids') { | ||
| serverErrors.assignedReviewers = error.message; | ||
| } | ||
| }); | ||
| console.error("Setting server errors:", serverErrors); | ||
| setErrors(serverErrors); | ||
| } else { | ||
| console.error("No errors found in response"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove excessive console.error statements before production.
The error handling logic correctly extracts and maps server validation errors to the form state. However, there are 8 console.error statements (lines 227-230, 233, 236, 239, 254, 257) that appear to be debugging artifacts.
Apply this diff to remove debugging console statements:
} catch (err: any) {
// setIsSubmitting(false);
- console.error("Full error object:", err);
- console.error("Original error:", err?.originalError);
- console.error("Original error response:", err?.originalError?.response);
-
// Handle server validation errors - the CustomException is in originalError
const errorData = err?.originalError?.response || err?.response?.data || err?.response;
- console.error("Error data:", errorData);
if (errorData?.errors) {
- console.error("Processing server errors:", errorData.errors);
const serverErrors: FormErrors = {};
errorData.errors.forEach((error: any) => {
- console.error("Processing error:", error);
if (error.field === 'title') {
serverErrors.title = error.message;
} else if (error.field === 'status') {
serverErrors.status = error.message;
} else if (error.field === 'tags') {
serverErrors.tags = error.message;
} else if (error.field === 'content_html') {
serverErrors.content = error.message;
} else if (error.field === 'next_review_date') {
serverErrors.nextReviewDate = error.message;
} else if (error.field === 'assigned_reviewer_ids') {
serverErrors.assignedReviewers = error.message;
}
});
- console.error("Setting server errors:", serverErrors);
setErrors(serverErrors);
- } else {
- console.error("No errors found in response");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err: any) { | |
| // setIsSubmitting(false); | |
| console.error(err); | |
| console.error("Full error object:", err); | |
| console.error("Original error:", err?.originalError); | |
| console.error("Original error response:", err?.originalError?.response); | |
| // Handle server validation errors - the CustomException is in originalError | |
| const errorData = err?.originalError?.response || err?.response?.data || err?.response; | |
| console.error("Error data:", errorData); | |
| if (errorData?.errors) { | |
| console.error("Processing server errors:", errorData.errors); | |
| const serverErrors: FormErrors = {}; | |
| errorData.errors.forEach((error: any) => { | |
| console.error("Processing error:", error); | |
| if (error.field === 'title') { | |
| serverErrors.title = error.message; | |
| } else if (error.field === 'status') { | |
| serverErrors.status = error.message; | |
| } else if (error.field === 'tags') { | |
| serverErrors.tags = error.message; | |
| } else if (error.field === 'content_html') { | |
| serverErrors.content = error.message; | |
| } else if (error.field === 'next_review_date') { | |
| serverErrors.nextReviewDate = error.message; | |
| } else if (error.field === 'assigned_reviewer_ids') { | |
| serverErrors.assignedReviewers = error.message; | |
| } | |
| }); | |
| console.error("Setting server errors:", serverErrors); | |
| setErrors(serverErrors); | |
| } else { | |
| console.error("No errors found in response"); | |
| } | |
| } | |
| } catch (err: any) { | |
| // setIsSubmitting(false); | |
| // Handle server validation errors - the CustomException is in originalError | |
| const errorData = err?.originalError?.response || err?.response?.data || err?.response; | |
| if (errorData?.errors) { | |
| const serverErrors: FormErrors = {}; | |
| errorData.errors.forEach((error: any) => { | |
| if (error.field === 'title') { | |
| serverErrors.title = error.message; | |
| } else if (error.field === 'status') { | |
| serverErrors.status = error.message; | |
| } else if (error.field === 'tags') { | |
| serverErrors.tags = error.message; | |
| } else if (error.field === 'content_html') { | |
| serverErrors.content = error.message; | |
| } else if (error.field === 'next_review_date') { | |
| serverErrors.nextReviewDate = error.message; | |
| } else if (error.field === 'assigned_reviewer_ids') { | |
| serverErrors.assignedReviewers = error.message; | |
| } | |
| }); | |
| setErrors(serverErrors); | |
| } | |
| } |
🤖 Prompt for AI Agents
In Clients/src/presentation/components/Policies/PolicyDetailsModal.tsx around
lines 225 to 259, remove the debugging console.error statements (the multiple
logs printing the full error object, originalError, response, intermediate
"Processing..." logs and "Setting server errors"/"No errors found" logs) and
leave only the functional error-handling logic that extracts errorData, maps
server validation errors into the serverErrors object and calls
setErrors(serverErrors); ensure no other behavior is changed.
|
LGTM! |
Provide a concise description of the changes made and their intended purpose.
Write your issue number after "Fixes "
Fixes #2279
Please ensure all items are checked off before requesting a review: