Refactor Popup Validations #549
Conversation
Develop -> master
Develop -> Master
WalkthroughThis pull request updates documentation and refactors backend validations for popup functionalities. The README now includes a demo link, clarified installation instructions, optional steps for symbolic link creation and Certbot, a JavaScript snippet version update, and updated project names. In the backend, inline validations and error handling have been removed from popup controller methods, replaced by validation middleware in the routes. Centralized validation rules are introduced in a helper module using the express-validator library. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Route Handler
participant V as Validation Middleware
participant P as Popup Controller
C->>R: Send HTTP Request (e.g., POST /add_popup)
R->>V: Apply addOrUpdatePopupValidation & handleValidationErrors
V-->>R: Return validation result
R->>P: Forward request to addPopup
P-->>R: Process and return response
R-->>C: Deliver final response
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (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: 1
🧹 Nitpick comments (4)
backend/src/routes/popup.routes.js (1)
15-16: Yo dawg, we need some validation here too!Consider adding validation middleware to
all_popupsandpopupsroutes for consistency.-router.get("/all_popups", authenticateJWT, popupController.getAllPopups); -router.get("/popups", authenticateJWT, popupController.getPopups); +router.get("/all_popups", authenticateJWT, handleValidationErrors, popupController.getAllPopups); +router.get("/popups", authenticateJWT, handleValidationErrors, popupController.getPopups);backend/src/utils/popup.helper.js (1)
47-66: These color validations be looking clean! But let's make them clearer! 🎨Consider providing more specific error messages that include the allowed formats.
body('headerBackgroundColor') .optional() .matches(hexColorPattern) - .withMessage('Header background color must be a valid hex color code'), + .withMessage('Header background color must be a valid hex color code (e.g., #FFF, #FFFF, #FFFFFF, or #FFFFFFFF)'),README.md (2)
242-252: Yo, let's fix that code block formatting! 📝Add a language identifier to the fenced code block for better syntax highlighting.
-``` +```javascript window.bwApiBaseUrl = 'https://guidefox-demo.bluewavelabs.ca/api/';🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
242-242: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
275-275: Grammar check, fam! 📚Fix the article usage before "secure".
-DataHall, an secure file sharing application, aka dataroom. +DataHall, a secure file sharing application, aka dataroom.🧰 Tools
🪛 LanguageTool
[misspelling] ~275-~275: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...s://github.com/bluewave-labs/datahall), an secure file sharing application, aka da...(EN_A_VS_AN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)backend/src/controllers/popup.controller.js(0 hunks)backend/src/routes/popup.routes.js(1 hunks)backend/src/utils/popup.helper.js(2 hunks)
💤 Files with no reviewable changes (1)
- backend/src/controllers/popup.controller.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
242-242: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
README.md
[misspelling] ~275-~275: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...s://github.com/bluewave-labs/datahall), an secure file sharing application, aka da...
(EN_A_VS_AN)
🔇 Additional comments (2)
backend/src/routes/popup.routes.js (2)
6-7: Yo, these imports are fire! 🔥Clean separation of validation middleware and helpers enhances code maintainability.
12-14: These routes are straight fire with that validation! 🎯Solid implementation of authentication, access control, and validation middleware.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/utils/popup.helper.js (3)
41-50: Yo, the actionUrl validation needs some cleanup! 🧹The error handling in actionUrl validation isn't consistent with the url validation above it.
Let's make it match the style:
body('actionUrl') .custom((value) => { if(!value) return true; try { const url = new URL(value); - return ['http:', 'https:'].includes(url.protocol) - }catch { - return false + return ['http:', 'https:'].includes(url.protocol); + } catch (error) { + return false; } })
73-75: Mom's spaghetti says we need more validation! 🍝The ID and URL validations could use some additional safeguards.
Consider these enhancements:
-const deleteOrGetPopupByIdValidation = [param('id').notEmpty().trim().isInt().withMessage("Invalid popup id")] +const deleteOrGetPopupByIdValidation = [ + param('id') + .notEmpty() + .trim() + .isInt({ min: 1 }) + .withMessage("Invalid popup id - must be a positive integer") +] -const getPopupByUrlValidation = [body('url').notEmpty().isString().withMessage('URL is missing or invalid')] +const getPopupByUrlValidation = [ + body('url') + .notEmpty() + .isString() + .trim() + .isURL({ protocols: ['http', 'https'] }) + .withMessage('URL must be a valid HTTP/HTTPS URL') +]
77-110: These helper functions need some documentation love! 📝The existing validation functions are solid but could use some JSDoc comments for better maintainability.
Add documentation like this:
+/** + * Validates if the given popup size is one of the allowed values + * @param {string} value - The popup size to validate + * @returns {boolean} - True if size is valid, false otherwise + */ const validatePopupSize = (value) => { const validSizes = settings.popup.size; return validSizes.includes(value); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/utils/popup.helper.js(2 hunks)
🔇 Additional comments (2)
backend/src/utils/popup.helper.js (2)
1-4: Yo dawg, that hex color regex is straight fire! 🔥The hex color pattern supports all valid formats (3,4,6,8 digits). Clean imports and solid regex definition.
112-120: Clean exports, we're done here! ✨All validation functions and arrays are properly exported. Nice organization!
|
@erenfn - PR raised and added the suggestion from the code rabbit review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/src/utils/popup.helper.js (1)
78-111: 🛠️ Refactor suggestionTime to drop some legacy code like it's hot! 🔥
These helper functions are now redundant since we've moved to express-validator. Consider deprecating them with a migration plan:
- Add deprecation warnings
- Update any existing usage to use the new validation rules
- Remove in next major version
const validatePopupSize = (value) => { + console.warn('Deprecated: Use express-validator rules instead'); const validSizes = settings.popup.size; return validSizes.includes(value); };
🧹 Nitpick comments (3)
backend/src/utils/popup.helper.js (3)
1-4: Yo dawg, let's make those protocols a constant! 🎵The setup looks clean, but we're seeing those protocols repeated throughout. Let's drop this at the top:
const { body, param } = require('express-validator'); const settings = require('../../config/settings'); const hexColorPattern = /^#([A-Fa-f0-9]{3}|[A-Fa-f0-9]{4}|[A-Fa-f0-9]{6}|[A-Fa-f0-9]{8})$/; +const ALLOWED_PROTOCOLS = ['http:', 'https:'];
74-76: Break it down like a rap verse! 🎤Let's make these validations more readable by breaking them into multiple lines:
-const deleteOrGetPopupByIdValidation = [param('id').notEmpty().trim().isInt({ min: 1 }).withMessage("Invalid popup id")] +const deleteOrGetPopupByIdValidation = [ + param('id') + .notEmpty() + .trim() + .isInt({ min: 1 }) + .withMessage("Invalid popup id") +]; -const getPopupByUrlValidation = [body('url').notEmpty().isString().withMessage('URL is missing or invalid')] +const getPopupByUrlValidation = [ + body('url') + .notEmpty() + .isString() + .withMessage('URL is missing or invalid') +];
113-121: Drop some documentation beats! 📝Let's add some JSDoc swagger to these exports:
+/** + * @typedef {import('express-validator').ValidationChain[]} ValidationRules + */ module.exports = { + /** @type {ValidationRules} Rules for adding or updating popups */ addOrUpdatePopupValidation, + /** @type {ValidationRules} Rules for validating popup ID in params */ deleteOrGetPopupByIdValidation, + /** @type {ValidationRules} Rules for validating popup URL in body */ getPopupByUrlValidation, + /** @deprecated Use express-validator rules instead */ validatePopupSize, + /** @deprecated Use express-validator rules instead */ validatePopupSizeWrapper, + /** @deprecated Use express-validator rules instead */ validateUrl, + /** @deprecated Use express-validator rules instead */ validateRelativeUrl, };
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/src/utils/popup.helper.js (1)
71-76: 🛠️ Refactor suggestionThis URL validation's got one shot, one opportunity! 🎯
Let's make it more robust by reusing our URL validation logic:
const getPopupByUrlValidation = [ body('url') .notEmpty() - .isString() - .withMessage('URL is missing or invalid') + .isString() + .withMessage('URL must be a string') + .bail() + .custom((value) => { + if (value.startsWith('/')) { + return /^\/[\w-]+(\/[\w-]+)*(\?.*)?$/.test(value); + } + try { + const url = new URL(value); + return ['http:', 'https:'].includes(url.protocol); + } catch { + return false; + } + }) + .withMessage('Invalid URL format. Must be a valid relative or absolute URL') ]
🧹 Nitpick comments (3)
backend/src/utils/popup.helper.js (3)
3-7: Yo dawg, let's make this color validation more lit! 🎨The color validation is solid, but we can make it even better:
-const hexColorPattern = /^#([A-Fa-f0-9]{3}|[A-Fa-f0-9]{4}|[A-Fa-f0-9]{6}|[A-Fa-f0-9]{8})$/; +// Supports 3-digit (RGB), 4-digit (RGBA), 6-digit (RRGGBB), and 8-digit (RRGGBBAA) hex colors +const hexColorPattern = /^#([A-Fa-f0-9]{3,4}|[A-Fa-f0-9]{6}|[A-Fa-f0-9]{8})$/i;
63-69: These ID validations straight fire, but the message could be more precise! 🎯.notEmpty() .trim() .isInt({ min: 1 }) - .withMessage("Invalid popup id") + .withMessage("Popup ID must be a positive integer")
78-111: These old helpers looking kinda sus! 🤔Consider adding deprecation notices to these functions since we've moved to express-validator:
+/** + * @deprecated Use express-validator middleware instead. + * See addOrUpdatePopupValidation for the new implementation. + */ const validatePopupSize = (value) => { const validSizes = settings.popup.size; return validSizes.includes(value); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/utils/popup.helper.js(2 hunks)
🔇 Additional comments (1)
backend/src/utils/popup.helper.js (1)
113-121: Exports looking clean, no spaghetti code here! 🍝The module exports are well-organized, clearly separating the new validation middleware from the existing helper functions.
There was a problem hiding this comment.
The endpoints are working good!
There is just one more issue left. At the bottom of the PR you can see that some tests for popups fail. Can you change the tests accordingly? You can follow what @DeboraSerra did at #490
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
backend/src/test/unit/controllers/popup.test.js (2)
418-418:⚠️ Potential issueFix duplicate error message in array.
The error message array contains a duplicate entry for textColor validation.
Apply this diff to fix the duplicate error message:
- errors: ['textColor must be a valid hex color code']['textColor must be a valid hex color code'], + errors: ['textColor must be a valid hex color code'],
180-181: 🛠️ Refactor suggestionStandardize error message format.
Some error messages still use the old object format with
msgproperty while others use the new string array format. This inconsistency should be fixed.Apply this diff to standardize the error message format:
- errors: [{ msg: "Popup with the specified id does not exist" }], + errors: ["Popup with the specified id does not exist"], - errors: [{ msg: "Popup not found" }], + errors: ["Popup not found"],Also applies to: 453-454, 621-625
🧹 Nitpick comments (2)
backend/src/test/e2e/popup.test.mjs (2)
640-644: Remove commented-out test assertion.There's a commented-out assertion that should either be implemented or removed.
Apply this diff to clean up the code:
const { ...rest } = res2.body; expect(res2).to.have.status(200); expect(rest).to.be.deep.equal(popup().build()); - // expect(creator.id).to.be.equal(user().build().id);
486-497: Improve test data setup readability.The test data setup using
Promise.allwith map could be more readable using a more descriptive approach.Consider refactoring to improve readability:
- await Promise.all( - popupList.map((popup) => { - return chai.request - .execute(app) - .post("/api/popup/add_popup") - .set( - "Authorization", - `Bearer ${popup.createdBy === 1 ? token : newToken}` - ) - .send(popup); - }) - ); + const createPopup = async (popup) => { + const authToken = popup.createdBy === 1 ? token : newToken; + return chai.request + .execute(app) + .post("/api/popup/add_popup") + .set("Authorization", `Bearer ${authToken}`) + .send(popup); + }; + + await Promise.all(popupList.map(createPopup));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/test/e2e/popup.test.mjs(18 hunks)backend/src/test/unit/controllers/popup.test.js(22 hunks)
Refactored Popup Validations in Backend
Updated the existing popup validations in popup.controller.js file using express validator and placed them in popup.helper.js file.
This PR addresses the issue #387
Below is a summary of all the changes:
add_popup, delete_popup/:id, edit_popup/:id, get_popup/:id, get_popup_by_url
Existing functions in popup.helper.js file such as validatePopupSize, validatePopupSizeWrapper, validateRelativeUrl, validateUrl have been left as is since they might be useful in another implementations.