Banner validation (Backend) #497
Conversation
WalkthroughThe pull request introduces significant changes to the validation and error handling mechanisms across various banner-related components in both the backend and frontend. Key modifications include the removal of validation checks from controller methods and model definitions, the addition of new validation middleware in routes, and enhancements to error reporting in frontend services. These changes streamline the handling of banner data while maintaining core functionalities. Changes
Suggested Reviewers
Possibly Related PRs
Finishing Touches
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: 5
🧹 Nitpick comments (7)
frontend/src/utils/guideHelper.js (1)
5-7: Yo dawg, let's make this error handling more robust!The iteration through errors is straight fire, but we can make it even better with modern JavaScript. Plus, we need to check if that errors array exists, or we might catch a TypeError!
Here's a cleaner way to do it:
- for (let i = 0; i < error.response.data.errors.length; i++){ - toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred: ' + error.response.data.errors[i]) - } + const errors = error.response.data.errors || []; + errors.forEach(errorMsg => + toastEmitter.emit(TOAST_EMITTER_KEY, `An error occurred: ${errorMsg}`) + );backend/src/models/Banner.js (1)
5-40: Yo, we need some database-level validation up in here!While moving validations to the route level is dope, we should consider adding some database-level constraints for the following fields to prevent invalid data from sneaking in:
closeButtonAction: Add CHECK constraint for valid actionsposition: Add CHECK constraint for valid positionsfontColor,backgroundColor: Add CHECK constraints for hex color formatConsider adding migrations to implement these constraints. Want me to help draft those SQL constraints?
backend/src/routes/banner.routes.js (1)
12-46: Let's organize these routes like mom organizes her kitchen!The routes could be grouped better for maintainability. Consider organizing them by authentication requirements.
Group them like this:
- Authenticated routes with banner permissions
- Authenticated routes without special permissions
- Public routes
Want me to show you how to restructure this?
backend/src/service/banner.service.js (2)
45-53: Yo dawg, let's make that error handling more lit! 🔥The error handling in
getBannerByIdcould drop more knowledge about what went wrong. Consider passing the original error message to help with debugging.} catch { - throw new Error('Error retrieving banner by ID'); + throw new Error(`Error retrieving banner by ID: ${error.message}`); }
55-74: These error messages be looking like twins, fam! 👯♂️The error messages in
getBannerByUrlandgetIncompleteBannersByUrlare identical despite performing different operations. Let's make them unique and include the URL for better debugging.} catch { - throw new Error('Error retrieving banner by URL'); + throw new Error(`Error retrieving banner for URL: ${url}`); }} catch { - throw new Error('Error retrieving banner by URL'); + throw new Error(`Error retrieving incomplete banners for URL: ${url}`); }backend/src/utils/banner.helper.js (1)
2-2: These hex colors need some swagger check! 🎨The hex color validation could be improved:
- Export the pattern for reuse
- Consider enforcing 6-digit format for consistency
-const hexColorPattern = /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/; +const HEX_COLOR_PATTERN = /^#[A-Fa-f0-9]{6}$/; module.exports = { + HEX_COLOR_PATTERN, // ... other exports };Also applies to: 45-49
backend/src/controllers/banner.controller.js (1)
14-14: That console log be looking kinda sus! 📝The informal error logging message "here is the error yar" should be standardized for better log searching and professionalism.
- console.log('here is the error yar', err.message); + console.error('Error creating banner:', err.message);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/src/controllers/banner.controller.js(1 hunks)backend/src/models/Banner.js(1 hunks)backend/src/routes/banner.routes.js(1 hunks)backend/src/service/banner.service.js(1 hunks)backend/src/test/e2e/auth.test.mjs(2 hunks)backend/src/test/e2e/banner.test.mjs(1 hunks)backend/src/test/mocks/banner.mock.js(3 hunks)backend/src/test/unit/controllers/banner.test.js(2 hunks)backend/src/utils/auth.helper.js(1 hunks)backend/src/utils/banner.helper.js(2 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(0 hunks)frontend/src/services/bannerServices.js(1 hunks)frontend/src/utils/guideHelper.js(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/scenes/banner/CreateBannerPage.jsx
✅ Files skipped from review due to trivial changes (4)
- backend/src/test/e2e/auth.test.mjs
- backend/src/test/unit/controllers/banner.test.js
- backend/src/test/mocks/banner.mock.js
- backend/src/test/e2e/banner.test.mjs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/routes/banner.routes.js (1)
1-51: Consider grouping these routes like mom's recipe categories!For better maintainability, consider grouping the routes by access level or functionality:
- Public routes
- Authenticated routes
- Admin routes
Example structure:
// Public routes router.get('/get_banner_by_url', ...); // Authenticated routes router.get('/banners', ...); router.get('/all_banners', ...); // Admin routes (with accessGuard) router.post('/add_banner', ...); router.delete('/delete_banner/:id', ...); router.put('/edit_banner/:id', ...);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/routes/banner.routes.js(1 hunks)backend/src/utils/auth.helper.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/utils/auth.helper.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (4)
backend/src/routes/banner.routes.js (4)
1-15: Yo, these imports are cleaner than mom's kitchen!The organization of dependencies is on point, with clear separation of core Express components, controllers, middleware, and validation utilities.
16-39: These protected routes are more secure than mom's secret recipe vault!Solid implementation with:
- JWT authentication
- Access control guards
- Validation middleware in the correct order
49-49: Validation's looking fresh like mom's homemade bread!Nice job adding the
getBannerByUrlValidationandhandleValidationErrorsmiddleware to the/get_banner_by_urlendpoint. This addresses the security concern raised in the previous review.
40-41:⚠️ Potential issueYo! These routes are missing validation like mom's spaghetti missing sauce!
The
/all_bannersand/bannersendpoints lack validation middleware, which could make them vulnerable to potential attacks.Add validation middleware:
-router.get('/all_banners', authenticateJWT, bannerController.getAllBanners); -router.get('/banners', authenticateJWT, bannerController.getBanners); +router.get( + '/all_banners', + authenticateJWT, + handleValidationErrors, + bannerController.getAllBanners +); +router.get( + '/banners', + authenticateJWT, + handleValidationErrors, + bannerController.getBanners +);Likely invalid or redundant comment.
|
@erenfn Let me know if i need to make any changes here |
There was a problem hiding this comment.
Missing repetition type returns:
{
"errors": [
"Invalid value",
"Invalid repetition"
]
}
However, missing position returns
{
"errors": [
"Position is required"
]
}
In repetitionType, can we remove "Invalid value", and change Invalid repetition to "Repetition type is required"?
|
@erenfn fixed the error message as requested |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/src/utils/banner.helper.js (4)
2-2: Yo dawg, let's make that regex more self-explanatory! 🎨Consider renaming
hexColorPatternto something more descriptive likeVALID_HEX_COLOR_REGEXto better communicate its purpose and validation rules.-const hexColorPattern = /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/; +const VALID_HEX_COLOR_REGEX = /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/;
11-16: Mom's spaghetti time! Let's make these actions more maintainable! 🍝Extract the allowed actions into a constant to make updates easier and prevent typos.
+const VALID_CLOSE_BUTTON_ACTIONS = ['no action', 'open url', 'open url in a new tab']; + body('closeButtonAction') .notEmpty() .withMessage('Close Button Action is required') .bail() - .isIn(['no action', 'open url', 'open url in a new tab']) + .isIn(VALID_CLOSE_BUTTON_ACTIONS) .withMessage('Invalid close button action'),
17-36: Knees weak, arms heavy... this URL validation needs some cleanup! 💪The nested custom validations make the code harder to follow. Consider splitting this into separate middleware functions.
+const validateUrlWithAction = (value, { req }) => { + if (!value && ['open url', 'open url in a new tab'].includes(req.body.closeButtonAction)) { + throw new Error('URL is required when close button action is set to open URL'); + } + return true; +}; + +const validateUrlProtocol = (value) => { + if (!value) return true; + try { + const url = new URL(value); + if (!['http:', 'https:'].includes(url.protocol)) { + throw new Error('URL must use HTTP or HTTPS protocol'); + } + return true; + } catch { + throw new Error('URL must use HTTP or HTTPS protocol'); + } +}; + body('url') .optional() - .custom((value, { req }) => { - if (!value && ['open url', 'open url in a new tab'].includes(req.body.closeButtonAction)) { - return false; - } - return true; - }) - .withMessage('URL is required when close button action is set to open URL') + .custom(validateUrlWithAction) .bail() - .custom((value) => { - if (!value) return true; - try { - const url = new URL(value); - return ['http:', 'https:'].includes(url.protocol); - } catch { - return false; - } - }) - .withMessage('URL must use HTTP or HTTPS protocol'), + .custom(validateUrlProtocol),
52-53: These validations looking slim like mom's spaghetti! 🍜Consider adding more robust checks:
- For ID: Add a minimum value check
- For URL: Add length limits and character restrictions
-const deleteOrGetBannerByIdValidation = [param('id').notEmpty().trim().isInt().withMessage('Invalid id')]; +const deleteOrGetBannerByIdValidation = [ + param('id') + .notEmpty() + .trim() + .isInt({ min: 1 }) + .withMessage('ID must be a positive integer') +]; -const getBannerByUrlValidation = [body('url').notEmpty().isString().withMessage('URL is missing or invalid')]; +const getBannerByUrlValidation = [ + body('url') + .notEmpty() + .isString() + .isLength({ max: 2048 }) + .matches(/^[a-zA-Z0-9-_/.]+$/) + .withMessage('URL must be valid and not exceed 2048 characters') +];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/utils/banner.helper.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
backend/src/utils/banner.helper.js (1)
Line range hint
65-87: Let's verify these helper functions ain't collecting dust! 🕵️These URL validation helpers might be used elsewhere, but let's verify their usage to ensure we're not keeping dead code.
Also, consider adding JSDoc comments to document these utility functions:
+/** + * Validates a URL string to ensure it uses HTTP/HTTPS protocol + * @param {string} value - The URL to validate + * @param {string} fieldName - Name of the field for error messages + * @throws {Error} If URL is invalid or doesn't use HTTP/HTTPS + */ const validateUrl = (value, fieldName) => {✅ Verification successful
Yo, these validation functions are straight fire! 🔥
Both
validateUrlandvalidateRelativeUrlare actively used across frontend and backend components. They're not just collecting dust - they're doing the heavy lifting in popup controllers and models, eh!The JSDoc suggestion is still solid though - would make these utility functions even more legit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of validateUrl and validateRelativeUrl functions echo "Searching for validateUrl usage..." rg "validateUrl\(" --type js echo "Searching for validateRelativeUrl usage..." rg "validateRelativeUrl\(" --type jsLength of output: 829
Sure. That would be great. Also can you change the validation of repetitionType to this? |
Added the tests for both add_banner and edit_banner and also implemented the repetitionType validation as requested |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/src/utils/banner.helper.js (1)
Line range hint
74-95: URL validation functions looking like copy-pasta! 🍝The URL validation functions have similar patterns and could be consolidated into a single function with options:
const validateUrlWithOptions = (value, options = {}) => { const { fieldName = 'URL', allowRelative = false, requireProtocol = false } = options; if (!value) return; try { const url = new URL(value); if (requireProtocol && !['http:', 'https:'].includes(url.protocol)) { throw new Error('URL must use HTTP or HTTPS protocol'); } } catch (error) { if (allowRelative && value.startsWith('/')) { return; } throw new Error(`Invalid ${fieldName}: ${error.message}`); } };
🧹 Nitpick comments (5)
backend/src/utils/banner.helper.js (5)
1-2: Yo dawg, let's make that hex color regex more readable! 🎨Add a descriptive comment explaining the hex color pattern:
const { body, param } = require('express-validator'); +// Validates 6-digit (#RRGGBB) or 3-digit (#RGB) hex color codes const hexColorPattern = /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/;
19-36: Mom's spaghetti moment: URL validation's getting heavy! 🍝The URL validation logic is complex and could be simplified using express-validator's built-in URL validator:
- .custom((value) => { - if (!value) return true; - try { - const url = new URL(value); - return ['http:', 'https:'].includes(url.protocol); - } catch { - return false; - } - }) + .if(value => value) + .isURL({ protocols: ['http', 'https'] })
44-52: Repetition type validation needs some cleanup on aisle 3! 🧹Convert the custom validation to use express-validator's built-in validators for cleaner code:
- body('repetitionType').custom((value) => { - if (!value) { - throw new Error('Repetition type is required'); - } - if (!['show only once', 'show every visit'].includes(value)) { - throw new Error('Invalid repetition type'); - } - return true; - }), + body('repetitionType') + .notEmpty() + .withMessage('Repetition type is required') + .bail() + .isIn(['show only once', 'show every visit']) + .withMessage('Invalid repetition type'),
60-61: These error messages be looking kinda basic, yo! 📝Enhance error messages to be more specific and helpful:
-const deleteOrGetBannerByIdValidation = [param('id').notEmpty().trim().isInt().withMessage('Invalid id')]; +const deleteOrGetBannerByIdValidation = [ + param('id') + .notEmpty() + .withMessage('Banner ID is required') + .trim() + .isInt() + .withMessage('Banner ID must be a valid integer') +];
63-71: Performance check: toLowerCase() result getting ghosted! 👻The toLowerCase() result should be stored to avoid potential multiple calls:
const validatePosition = (value) => { const validPositions = ['top', 'bottom']; - return validPositions.includes(value.toLowerCase()); + const normalizedValue = value.toLowerCase(); + return validPositions.includes(normalizedValue); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (2)
backend/src/test/e2e/banner.test.mjs(1 hunks)backend/src/utils/banner.helper.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/e2e/banner.test.mjs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
Banner backend validations.
This PR addresses the issue #388
I've used express validator for the backend validation checks.
Added Changes:
Removed code:
I've left the validatePosition, validatePositionWrapper, etc as they might still be used by some other service.