389 refactoring validations in hint routes with express validator #490
Conversation
…g into 389-refactoring-validations-in-hint-routes-with-express-validator
…g into 389-refactoring-validations-in-hint-routes-with-express-validator
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to hint-related functionality across the backend and frontend. The modifications primarily focus on restructuring validation mechanisms, removing strict input checks, and updating error handling approaches. The changes span multiple files including controllers, routes, validators, tests, and frontend components, with a consistent theme of simplifying validation and error management for hint-related operations. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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: 3
🧹 Nitpick comments (3)
backend/src/utils/hint.helper.js (2)
4-4: Let's make those valid actions more maintainable!Consider moving the
validActionsarray to a separate configuration file to make it easier to maintain and reuse.Create a new file
config/constants.js:exports.HINT_ACTIONS = ['no action', 'open url', 'open url in a new tab'];Then import and use it:
- const validActions = ['no action', 'open url', 'open url in a new tab']; + const { HINT_ACTIONS } = require('../config/constants');
15-31: These color validations got me losing myself in the repetition!The color validation logic is repeated for multiple fields. Let's DRY this up!
Create a reusable color validator:
+ const colorValidator = (fieldName) => + body(fieldName) + .optional() + .isString() + .custom(isValidHexColor) + .withMessage(`Invalid value for ${fieldName}`); const hintValidator = [ body('action') .isString() .notEmpty() .withMessage('action is required') .custom((value) => { - return validActions.includes(value); + return HINT_ACTIONS.includes(value); }) .withMessage('Invalid value for action'), - body('headerBackgroundColor') - .optional() - .isString() - .custom(isValidHexColor) - .withMessage('Invalid value for headerBackgroundColor'), + colorValidator('headerBackgroundColor'), - body('headerColor').optional().isString().custom(isValidHexColor).withMessage('Invalid value for headerColor'), + colorValidator('headerColor'), - body('textColor').optional().isString().custom(isValidHexColor).withMessage('Invalid value for textColor'), + colorValidator('textColor'), - body('buttonBackgroundColor') - .optional() - .isString() - .custom(isValidHexColor) - .withMessage('Invalid value for buttonBackgroundColor'), + colorValidator('buttonBackgroundColor'), - body('buttonTextColor') - .optional() - .isString() - .custom(isValidHexColor) - .withMessage('Invalid value for buttonTextColor'), + colorValidator('buttonTextColor'), ];frontend/src/scenes/hints/CreateHintPage.jsx (1)
28-32: These states got my palms sweaty! Let's clean them up! 💦Multiple color-related states with hardcoded default values could be more maintainable.
Consider using a configuration object:
const DEFAULT_COLORS = { headerBackground: '#FFFFFF', header: '#101828', text: '#344054', buttonBackground: '#7F56D9', buttonText: '#FFFFFF' }; // Then use it in your component: const [colors, setColors] = useState(DEFAULT_COLORS); // Update individual colors: setColors(prev => ({ ...prev, headerBackground: newColor }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/controllers/hint.controller.js(10 hunks)backend/src/routes/hint.routes.js(1 hunks)backend/src/test/e2e/hint.test.mjs(8 hunks)backend/src/test/unit/controllers/hint.test.js(0 hunks)backend/src/utils/hint.helper.js(1 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(4 hunks)
💤 Files with no reviewable changes (1)
- backend/src/test/unit/controllers/hint.test.js
✅ Files skipped from review due to trivial changes (1)
- backend/src/test/e2e/hint.test.mjs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
backend/src/routes/hint.routes.js (1)
18-24: Validation middleware looking clean, mom's spaghetti! 🍝Solid implementation of validation middleware chains. Each route has appropriate validators for its parameters and request body.
backend/src/controllers/hint.controller.js (1)
17-19: Error handling's looking clean, better than mom's spaghetti! 🍝Consistent error handling pattern across all controller methods using the error helper.
Also applies to: 33-35, 47-49, 70-72, 95-97, 120-122
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/src/utils/hint.helper.js (2)
1-5: Yo dawg, let's make these valid actions more maintainable!Consider moving
validActionsto a separate constants/config file. This would make it easier to maintain and reuse across the codebase.- const validActions = ['no action', 'open url', 'open url in a new tab']; + const { HINT_ACTIONS } = require('../constants/hint.constants');
47-49: Knees weak, arms heavy, this ID validation needs to be more steady!Consider adding bounds checking for the hintId to prevent potential integer overflow issues.
param('hintId') .notEmpty() .withMessage('hintId is required') .isInt() + .isInt({ min: 1 }) .withMessage('Invalid hint ID'),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/utils/hint.helper.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
backend/src/utils/hint.helper.js (1)
53-53: Clean exports, just like mom's spaghetti! 🍝The export statement is clean and follows best practices.
backend/src/utils/hint.helper.js
Outdated
| body('headerColor').optional().isString().custom(isValidHexColor).withMessage('Invalid value for headerColor'), | ||
| body('textColor').optional().isString().custom(isValidHexColor).withMessage('Invalid value for textColor'), | ||
| body('buttonBackgroundColor') | ||
| .optional() |
There was a problem hiding this comment.
The original code was set as optional... I believe they have a default value...
There was a problem hiding this comment.
onboarding_backend-1 | CREATE_HINT_ERROR: Error creating hint: Validation error: Validation isIn on tooltipPlacement failed
onboarding_backend-1 | CREATE_HINT_ERROR: Error creating hint: notNull Violation: Hint.tooltipPlacement cannot be null
Not null condition is not checked
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/src/utils/hint.helper.js (3)
5-5: Yo! Let's freeze that array like mom's spaghetti!Consider freezing the
validActionsarray to prevent accidental modifications:-const validActions = ['no action', 'open url', 'open url in a new tab']; +const validActions = Object.freeze(['no action', 'open url', 'open url in a new tab']);
11-18: Yo! These error messages are all over the place like spilled spaghetti!Let's make the error messages consistent with the format "Invalid value for...":
body('action') .isString() .notEmpty() - .withMessage('action is required') + .withMessage('Invalid value: action is required') .custom((value) => { return validActions.includes(value); }) .withMessage('Invalid value for action'),
48-50: Let's convert that ID like we convert raw spaghetti to cooked!Consider adding type conversion to ensure the hint ID is always a number:
param('hintId') .notEmpty() .withMessage('hintId is required') .isInt() + .toInt() .withMessage('Invalid hint ID'),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/test/e2e/auth.test.mjs(3 hunks)backend/src/test/e2e/hint.test.mjs(8 hunks)backend/src/test/mocks/hint.mock.js(1 hunks)backend/src/utils/hint.helper.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/e2e/hint.test.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/test/e2e/auth.test.mjs
[error] 11-11: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (3)
backend/src/utils/hint.helper.js (2)
7-8: Neat DRY implementation, better than mom's dry spaghetti!The color validator helper function effectively reduces code duplication and maintains consistency.
24-35: Still got inconsistent URL validation - mom's spaghetti would not approve!The URL validation approach is inconsistent between the hint validator and body validator.
backend/src/test/mocks/hint.mock.js (1)
6-8: These mock updates are as fresh as mom's spaghetti sauce!The default values added to the hint builder align well with the validation requirements.
Also applies to: 12-12
…g into 389-refactoring-validations-in-hint-routes-with-express-validator
…s-with-express-validator
erenfn
deleted the
389-refactoring-validations-in-hint-routes-with-express-validator
branch
Describe your changes
Update the code to use the validation middleware instead of foing the validations in the controller for the Hint.
Issue number
#389
Please ensure all items are checked off before requesting a review: