Banner validations #450
Banner validations #450
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the banner management system across backend and frontend components. The changes focus on centralizing error handling, implementing robust validation middleware, and enhancing user feedback mechanisms for banner-related operations. The modifications span multiple files, introducing more consistent error reporting, validation strategies, and improved component interactions for banner creation and management. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Routes
participant Validation
participant Controller
participant Service
participant Database
Client->>Routes: Send Banner Request
Routes->>Validation: Validate Request
Validation-->>Routes: Validation Result
alt Validation Passed
Routes->>Controller: Forward Request
Controller->>Service: Process Request
Service->>Database: Perform Database Operation
Database-->>Service: Return Result
Service-->>Controller: Return Data
Controller-->>Client: Send Response
else Validation Failed
Validation-->>Client: Return Validation Errors
end
Possibly Related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (9)
backend/src/controllers/banner.controller.js (2)
26-45: Consider concurrency edge cases for banner deletion.Right now, if multiple deletion attempts occur close together, only one request can truly succeed, and the rest will yield 404. This is typically fine, but you might consider logs or notifications to highlight these collisions. Other than that, the synchronous flow is easy to follow and quite robust, so keep rocking on.
72-82: Paginate if large results can boggle memory.
getAllBannersmight return a massive list if your user base or banner collection grows. For better performance going forward, consider pagination or filtering. Because your arms won't be heavy carrying too big a payload, but your system might suffer latency under heavy load.backend/src/utils/banner.helper.js (4)
8-30: Remove or clarify the commented-out validation code.Leaving large chunks of commented-out code could cause confusion for future maintainers. If it’s no longer needed, consider deleting it. Otherwise, add clear explanations on why you’re keeping it for reference.
55-59: Refine id checks.The
validateIdfunction correctly identifies numeric misfires. One potential improvement would be adding more context in the response or allowing a more descriptive error message. Otherwise, this logic is simple and effective.
74-84: Strict URL protocol validation.Restricting to HTTP/HTTPS is a good security measure, though keep in mind some legitimate protocols (e.g., mailto, ftp) might occasionally arise. Evaluate whether your application will require these in the future.
189-205: Consider flatteningErrorHandlerinto simpler functions.Your code is all static, and static analysis warns about potential overhead or complexity. Replacing the class with straightforward async utility functions might reduce code overhead. You’d keep the same functionality with less complexity.
-class ErrorHandler { - static async handleAsync(res, operation, errorType) { +async function handleAsync(res, operation, errorType) { try { const result = await operation(); return result; } catch (err) { console.error(`${errorType}: `, err); const { statusCode, payload } = internalServerError( errorType, err.message ); res.status(statusCode).json(payload); return null; } } -}backend/src/utils/guide.helper.js (1)
8-10: Straightforward error throw for invalid hex.Using exceptions for invalid input can be beneficial, just be sure each call site gracefully handles it. But overall, your approach is consistent and aligns with your error-handling conventions.
frontend/src/templates/DefaultPageTemplate/DefaultPageTemplate.jsx (1)
30-45: Give those arms a rest: streamline delete error handling.
Your delete logic is solid. Consider ensuring you gracefully handle partial deletes (like concurrency or stale data) by verifying the item’s current existence before removal, so your nerves stay intact.frontend/src/components/ColorTextField/ColorTextField.module.scss (1)
31-36: Yo dawg, let's make these error styles more robust! 🍝The error styling implementation could use some improvements to better align with the design system and Material-UI patterns.
Here's a spicier way to handle those error states:
&.error { - border-color: red; + border-color: var(--error-color); + transition: border-color 0.2s ease; } &.error input { - border-color: red; + border-color: var(--error-color); + transition: border-color 0.2s ease; } + /* Handle Material-UI's error state */ + :global { + .Mui-error fieldset { + border-color: var(--error-color) !important; + } + }Mom's spaghetti suggests:
- Use design system variables instead of hardcoded colors
- Add smooth transitions for better UX
- Handle Material-UI's native error states
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/src/controllers/banner.controller.js(1 hunks)backend/src/routes/banner.routes.js(1 hunks)backend/src/server.js(1 hunks)backend/src/service/banner.service.js(1 hunks)backend/src/utils/banner.helper.js(1 hunks)backend/src/utils/guide.helper.js(2 hunks)frontend/src/components/ColorTextField/ColorTextField.module.scss(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx(2 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.module.scss(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(1 hunks)frontend/src/templates/DefaultPageTemplate/DefaultPageTemplate.jsx(1 hunks)frontend/src/utils/constants.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/src/service/banner.service.js
- frontend/src/utils/constants.js
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/utils/banner.helper.js
[error] 188-204: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (28)
backend/src/controllers/banner.controller.js (5)
2-3: Good introduction of dependencies.
Solid move bringing in db and ErrorHandler. Centralizing your error handling approach is a tidy way to streamline asynchronous operations, ensuring uniform and consistent responses across endpoints. No concerns spotted here, knees are steady!
8-22: Validate req.user to avoid potential runtime misfires.
While the approach to create a banner is straightforward, ensure req.user always exists (e.g., if authentication fails or user data is incomplete, this could cause an unhandled exception). Verifying that req.user is set before trying to access req.user.id can further bolster reliability.
49-68: Clear separation of logic for edits.
You properly handle the scenario where the banner doesn't exist by returning 404. This is consistent with typical resource-based patterns. The method is organized, with no obvious logic or security concerns, so let's keep it that way.
86-97: Ensure req.user is consistently validated.
Retrieving banners by user ID depends on req.user.id. If the user is not authenticated properly, we might encounter unexpected behaviour. If you're certain authenticateJWT always sets req.user, then you’re good here. Otherwise, verify that the user indeed exists.
101-120: Neat response structure aligns well with your approach.
Your 404 response stands out as consistent with the rest of the code. The usage of ErrorHandler.handleAsync unifies error handling and helps keep your logic tidy. Keep those palms dry, the code is smooth!
backend/src/utils/banner.helper.js (8)
1-7: Leveraging shared helpers.
Importing internalServerError and the validation helpers from guide.helper fosters code reusability. Great step in ensuring that colour validations and error handling remain consistent across modules.
50-53: Align return type with your error array approach.
Returning [ { msg: "Invalid position" } ] fits your overall validation convention. This pattern is easy to parse and forward to your client. No issues here; it’s as comfy as a fresh set of lululemon pants.
67-68: Relative URL validations: watch out for edge cases.
By only skipping the check if the value starts with '/', you may inadvertently pass partial paths like 'foo/bar'. If that’s not intended, consider adding more specificity so that we only accept properly formed relative paths.
Also applies to: 70-70, 72-72
86-130: Robust new banner validations.
The addBannerValidation function comprehensively checks position, closeButtonAction, colour fields, and URLs. Returning a 400 with a list of errors is a consistent approach, so you’re holding it down firmly. Make sure you keep the queue of validations updated as new features roll in.
132-142: ID validation ensures proper resource targeting.
deleteBannerValidation is straightforward. The ID check wards off malformed inputs, returning a structured error array. This matches the rest of your validation approach, which is quite consistent.
143-176: All-around checks for edits.
editBannerValidation integrates ID checks, position checks, closeButtonAction validations, and colour validations. Handling everything here helps keep the controller method clean. Nicely done.
178-187: Uniform ID check for getBannerById.
Again, the consistent approach to verifying id before hitting the controller prevents undue confusion. Your consistent approach across validations is exemplary. Mom’s spaghetti indeed.
207-207: Export structure is consistent.
All required validation and error handling utilities are neatly exported, making them straightforward to import and use across the codebase. Keep it neat and tidy.
Also applies to: 210-213
backend/src/utils/guide.helper.js (3)
2-4: Hex colour validation looks accurate.
Your regex effectively supports multiple hex formats. Keep an eye on performance if you find yourself validating extremely high volumes of colours, but that’s generally not an issue.
14-14: Aggregate errors rather than prematurely responding.
Collecting all invalid colour fields before responding to the client is a user-friendly approach. This ensures the client sees the entire set of problems in one pass, rather than discovering them one by one. Smooth and efficient.
Also applies to: 17-17, 20-20
30-30: Clearer messaging for invalid close button action.
Changing the quotes is minor, but consistency in your messages fosters clarity. No sweaty palms here—just neat formatting changes that keep the code tidy.
backend/src/routes/banner.routes.js (2)
6-6: Nice addition of validation middleware import.
Importing your new validations keeps the routes succinct while enforcing input correctness. This is a boon to maintainability and helps unify your approach.
11-13: Validation-first routing.
Chaining authenticateJWT, accessGuard, and your relevant validation function ensures each request is screened prior to hitting the controller. This pattern reduces duplication and fosters a clean, layered architecture. Favourable technique, eh?
Also applies to: 16-16
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (3)
5-5: Extended props for refined user feedback.
Adding isBackgroundColorValid, isFontColorValid, isBackgroundColorTouched, and isFontColorTouched offers more precise control over when to display error messages. This fosters a more robust user experience.
14-14: Show immediate feedback for invalid background colour.
Displaying "Invalid hex colour" right away guides the user to correct their input. This real-time error handling strategy reduces friction and confusion. Well played.
Also applies to: 16-16
23-23: Similar pattern for font colour validations.
Consistently applying the same approach to fontColor ensures uniform behaviour across all colour inputs. Looks great and keeps the pacing steady.
Also applies to: 25-25
backend/src/server.js (1)
42-42: Keep your spaghetti off the floor, forced sync can drop data!
Forcing a sync might recreate tables and cause data loss. Use this cautiously in environments containing production or valuable data, so your knees don’t buckle under the pressure.
frontend/src/scenes/banner/CreateBannerPage.jsx (1)
36-37: Mom’s spaghetti moment: ensure response.data remains consistent.
Assigning bannerData from response.data is correct, but make sure to handle unexpected response structures or empty payloads to avoid unpleasant surprises.
frontend/src/templates/DefaultPageTemplate/DefaultPageTemplate.jsx (3)
36-39: Drop a beat: custom toast message is on point.
The toast emitter usage is great for user feedback. Just ensure your messages remain consistent across the app for a cohesive experience.
58-69: Steady on your feet: handle data fetch failures.
By logging the error and falling back to an empty array, you’re saving users from confusion if the server or network is down. This balanced approach keeps your application robust.
64-65: Don’t let sweat drip onto the data: check for null or undefined.
response.data is assigned directly to items. Make sure response has data to avoid a runtime error if that property is missing in certain responses.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.module.scss (2)
14-16: Easing the tension: vertical layout is tidy.
Switching .color to column layout with spacing improves clarity and usability. No sweaty knees here—looks good!
18-23: Wrap your spaghetti: .error class adds clear feedback.
The bright red text and spacing effectively highlight validation issues to users and help them correct mistakes swiftly.
| &.error { | ||
| border-color: red; | ||
| } | ||
| &.error input { | ||
| border-color: red; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Yo, there's inconsistency in your error styles already! 🍝
Looks like we've got some spaghetti code in our error styling! The codebase has defined error colour variables that aren't being used consistently:
- In
variables.css, we've got--border-error-solid: #D92D20and--error-color: #b00020 - Your component is using plain
redinstead of these variables - Other components like
ButtonStyles.cssandCustomTextFieldare properly usingvar(--border-error-solid)
To clean up this mess:
- Replace
border-color: redwithborder-color: var(--border-error-solid) - Consider using
--error-colorfor text-based errors
🔗 Analysis chain
Vomit on his sweater already... let's verify these styles! 🤢
Let's ensure consistent error styling patterns across the banner components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error styling patterns
echo "Checking for error color usage..."
rg -g "*.scss" -g "*.css" "red" --context 2
echo "Checking for error class implementations..."
rg -g "*.scss" -g "*.css" "\.error\s*{" --context 2
echo "Checking for Material-UI error state handling..."
rg -g "*.scss" -g "*.css" "Mui-error" --context 2
Length of output: 2495
Script:
#!/bin/bash
# Let's check for CSS variables related to error states
echo "Checking for CSS error variables..."
rg -g "*.scss" -g "*.css" -g "*.ts" -g "*.tsx" "(--border-error|--color-error|--error)" --context 2
# Let's find more error-related styles with a better regex
echo "Checking for error class patterns..."
rg -g "*.scss" -g "*.css" "error" --context 2
# Let's check for theme configurations
echo "Checking for theme error color definitions..."
fd -e ts -e js "theme" --exec rg -l "error"
Length of output: 8983
…nboarding into banner-validations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/test/unit/services/banner.test.js (1)
113-124: An extra check could amplify your performance, but it's still good.The test successfully filters out untouched banners by using
[Op.notIn]. Adding a test case for an empty exclusion list might strengthen coverage, though not mandatory. Rock on.backend/src/controllers/banner.controller.js (2)
26-45: Smoother than a freestyle.
deleteBannertransitions to usingErrorHandler, returning 404 for not found. It's tidy, but you might consider logging the error for better production diagnostics. Think about it.
86-97: Nothing shaky in these lines.
getBannersby user ID is consistent with the new central scheme. Though you might add a quick note for logs if user ID is invalid. This is more of a friendly jab than a must-do.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/controllers/banner.controller.js(1 hunks)backend/src/test/unit/controllers/banner.test.js(1 hunks)backend/src/test/unit/services/banner.test.js(1 hunks)frontend/src/components/ColorTextField/ColorTextField.module.scss(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(1 hunks)frontend/src/templates/DefaultPageTemplate/DefaultPageTemplate.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/ColorTextField/ColorTextField.module.scss
- frontend/src/scenes/banner/CreateBannerPage.jsx
- frontend/src/templates/DefaultPageTemplate/DefaultPageTemplate.jsx
🔇 Additional comments (10)
backend/src/test/unit/services/banner.test.js (1)
105-111: This test is straight up legit, ain't no sweaty palms needed.
These lines verify the method's ability to fetch banners by URL. It's properly relying on stubbing Banner.findAll and validating the where condition. Solid approach, yo.
backend/src/test/unit/controllers/banner.test.js (3)
402-410: Rhymes aside, these validations are dope.
This test properly checks for a missing or invalid URL scenario, returning 400 with helpful feedback. Keep serving that lyrical correctness.
412-420: Smooth like a good flow, the success path is tested well.
Returning 200 on fetch success is verified, along with the structure of the banner response. The coverage is on point, homie.
422-435: Error path tested, so there's no meltdown under pressure.
When the service rejects, your code properly returns 500 with a descriptive error. The integration of ErrorHandler is rock-solid.
backend/src/controllers/banner.controller.js (6)
2-3: Import synergy.
ErrorHandler and db are both crucial for this centralized logic. The synergy looks tight, no issues with the references, champion.
8-22: The new approach is hot, less clutter, more central smarts.
The addBanner method neatly delegates errors through ErrorHandler.handleAsync, trimming local validations. Code is concise, so keep spitting that fire.
49-68: Central error handling is shining bright.
editBanner method looks fresh, returns 404 if no banner. Flow is consistent, we can see how one design pattern ties everything together. You’re nailing it.
72-82: All banners in one shot, all day.
Retrieving all banners with ErrorHandler is straightforward. It's good to keep success path consistent with the others, so the user gets a uniform API response.
101-120: Where’s the banner? Not found means 404, that’s a rap.
Error transitions are consistent, returning 404 if not found. This standardization makes your code easy to read. Keep it blazing.
123-138: URL-based retrieval is champion.
The need for a properly formatted url is validated first, then processed. Inner synergy with ErrorHandler ensures consistent responses. You’re good to go!
|
Hi eulerbutcooler, this branch is not in our project. |
Fix for issue #388
The backend validations for banners are working properly.
I've created a centralized error handler which will handle all the backend errors that may arise.