Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

390 refactoring validations in link and helper link routes with express validator #448

Conversation

DeboraSerra
Copy link
Contributor

@DeboraSerra DeboraSerra commented Dec 30, 2024

Describe your changes

Briefly describe the changes you made and their purpose.

Issue number

Mention the issue number(s) this PR addresses (e.g., #123).

Please ensure all items are checked off before requesting a review:

  • I deployed the code locally.
  • I have performed a self-review of my code.
  • I have included the issue # in the PR.
  • I have labelled the PR correctly.
  • The issue I am working on is assigned to me.
  • I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
  • I made sure font sizes, color choices etc are all referenced from the theme.
  • My PR is granular and targeted to one specific feature.
  • I took a screenshot or a video and attached to this PR if there is a UI change.

Copy link
Contributor

coderabbitai bot commented Dec 30, 2024

Walkthrough

The pull request introduces comprehensive updates to the backend development environment, focusing on code formatting, linting, and API endpoint validation. The changes standardize code style, add new validation middleware, and refactor API routes and controllers. The modifications enhance code quality by implementing consistent formatting rules, adding input validation, and simplifying error handling across various components of the backend application.

Changes

File Change Summary
.prettierrc New Prettier configuration file added with formatting rules
eslint.config.mjs New ESLint configuration with linting rules and global variables
package.json Added development dependencies for ESLint and related tools
postman/*.postman_collection.json Updated API endpoint URLs and request structures
src/controllers/*.controller.js Simplified error handling and validation logic
src/routes/*.routes.js Added global middleware and enhanced route validation
src/utils/*.helper.js Introduced new validation helpers using express-validator

Suggested Reviewers

  • swoopertr
  • gorkem-bwl
  • erenfn

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (3)
backend/src/utils/helperLink.helper.js (1)

5-32: Detailed Validation Enforces Stronger Data Integrity

Your validations for title, colour fields, and links array show you’re not trembling under the pressure. The use of isValidHexColor and validateUrl helps avoid messy data – like vomit on your brand-new sweater. This sets a higher standard for incoming requests, ensuring safer and more robust services. Don’t forget to ensure comprehensive testing on these validations, so you stay calm, cool, and collected!

backend/src/controllers/link.controller.js (2)

10-12: Validate order more thoroughly
Your logic checks if order is too large for the array size. That’s rocking, but consider verifying it’s also a positive integer so we don’t trip on spaghetti code.


57-59: Ensure order is validated
Similar to earlier checks, verifying positivity might help. Don’t let negativity weigh you down.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dcb99e0 and f074815.

⛔ Files ignored due to path filters (1)
  • backend/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (16)
  • backend/.prettierrc (1 hunks)
  • backend/eslint.config.mjs (1 hunks)
  • backend/package.json (1 hunks)
  • backend/postman/HelperLinks.postman_collection.json (15 hunks)
  • backend/postman/Links.postman_collection.json (13 hunks)
  • backend/src/controllers/helperLink.controller.js (9 hunks)
  • backend/src/controllers/link.controller.js (8 hunks)
  • backend/src/routes/helperLink.routes.js (1 hunks)
  • backend/src/routes/link.routes.js (1 hunks)
  • backend/src/test/e2e/helperLink.test.mjs (10 hunks)
  • backend/src/test/e2e/link.test.mjs (8 hunks)
  • backend/src/test/mocks/helperLink.mock.js (1 hunks)
  • backend/src/test/unit/controllers/helperLink.test.js (0 hunks)
  • backend/src/test/unit/controllers/link.test.js (0 hunks)
  • backend/src/utils/helperLink.helper.js (1 hunks)
  • backend/src/utils/link.helper.js (2 hunks)
💤 Files with no reviewable changes (2)
  • backend/src/test/unit/controllers/helperLink.test.js
  • backend/src/test/unit/controllers/link.test.js
✅ Files skipped from review due to trivial changes (2)
  • backend/.prettierrc
  • backend/src/test/e2e/helperLink.test.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/test/e2e/link.test.mjs

[error] 34-34: 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)

backend/src/controllers/helperLink.controller.js

[error] 26-26: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 Gitleaks (8.21.2)
backend/postman/HelperLinks.postman_collection.json

17-17: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


102-102: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


111-111: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


187-187: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


272-272: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


372-372: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


458-458: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


499-499: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

backend/postman/Links.postman_collection.json

102-102: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


193-193: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


293-293: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


379-379: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


420-420: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


503-503: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

🔇 Additional comments (45)
backend/eslint.config.mjs (3)

1-3: Looks good adding imports for ESLint plugins and globals.
Knees might feel weak, but rest assured that using these libraries sets a stable foundation for linting.


4-4: Great usage of JSDoc for clarity.
This explicit type reference is as comforting as a warm bowl of mom’s spaghetti—it ensures maintainers know exactly what’s being exported.


5-13: Solid ESLint configuration.
The rules and language options provide consistent enforcement of best practices. No sweaty palms needed—this configuration is straightforward.

backend/package.json (1)

35-39: New devDependencies are well-aligned with linting needs.
These additions keep the codebase clean and consistent, preventing any lyrical slip-ups that might cause your arms to feel heavy.

backend/src/routes/link.routes.js (1)

1-2: Watch Your Step With Middleware Placement

His palms are sweaty, but so is the structure of these routes – it's good that you've re-ordered them to apply authenticateJWT globally on line 14. This ensures no route is invoked without checking authentication. Also, applying the respective validators with handleValidationErrors helps your code stand firm like knees that aren’t weak. Overall, these changes look solid from a security standpoint and will help weed out invalid data. Keep it up, and your spaghetti need not be spilled!

Also applies to: 6-7, 12-12, 14-24

backend/src/routes/helperLink.routes.js (1)

1-2: Well-Structured Middleware for Validation

Arms are heavy when juggling multiple middleware, but your approach here is neat. Using router.use(authenticateJWT) at line 14 keeps your routes protected. Then, applying idParamValidator and helperValidator with handleValidationErrors ensures your code holds firm even under pressure – no mom’s spaghetti all over the console. Great job keeping your validations consistent!

Also applies to: 6-8, 14-25

backend/src/utils/link.helper.js (1)

17-28: Solid Validation Arrays for Maintainable Routes

Knees weak? Not with these validators! The linkValidator, idParamValidator, queryValidator, and bodyUrlValidator set a strong baseline for request validation. Checking for integer IDs and valid URLs is crucial to avoid chaos. Keep verifying these changes thoroughly so you don’t end up cleaning spaghetti from your production logs. Great job championing best practices!

Also applies to: 30-36, 38-44, 46-48

backend/src/test/mocks/helperLink.mock.js (1)

22-22: Use a string for title to prevent confusion
Setting title to a number can raise eyebrows in places expecting a string. My knees are wobbly just thinking about debugging that.

backend/src/controllers/link.controller.js (11)

1-2: Good call keeping imports lean
No concerns here, everything seems in place. Palms sweaty but code steady.


27-27: Robust error handling
Catching and logging errors is on point. My arms are heavy with relief at this thorough approach.


40-40: Meaningful 404 message
Telling the user the link doesn’t exist is helpful. No sweaty confusion there.


44-44: Clear success message
You confirm deletion with the link ID. That’s clarity. Mom’s spaghetti would be proud.


46-46: Consistent error response
Keeping error messages concise and consistently styled helps. Great job.


74-74: Internal server error logging
Consistent error handling is music to my ears. Keep it up for fewer sweaty debugging sessions.


84-84: Sleek error handling for GET calls
Serving a trimmed error message is great. My arms are less heavy thanks to clarity.


95-95: Same pattern, consistent
Confidence soars when error approaches are uniform. Palms remain calm.


107-107: Concise 404
A short-and-sweet message helps clients handle missing resources. Good job.


112-112: Uniform error pattern
Yes, keep naming these error codes systematically. The synergy is real.


126-126: Dead code or leftover bracket
This trailing line appears idle. Ensure your function closes properly and an appropriate response is sent.

backend/src/controllers/helperLink.controller.js (16)

19-19: ‘title and url are required’
Mandatory fields are spelled out. Keep spitting that truth so we’re not weighed down by confusion.


23-23: Invalid URL caution
Good that you clarify if a URL is not valid. Knees remain stable thanks to precise messages.


38-38: Extracting links
Neat approach to destructure from req.body. Reduces confusion from heavy arms.


49-49: Internal error catch
Crisp error naming keeps us calm under pressure. Good hustle.


62-62: Meaningful 404
Specifying the helper doesn’t exist is direct and user-friendly. Palms remain unsweaty.


66-66: Clear success message
You confirm deletion with an ID. Crisp communication, no vomit on the details.


68-68: Error naming
Using DELETE_HELPER_ERROR is consistent and easy to track.


76-76: Again destructuring
{ links } keeps it neat. Keep up that style, it’s your top-of-the-charts jam.


78-78: Helper update logic
Making the service call here is tidy. Don’t let arms get heavy from scattered code.


82-82: Graceful 404
Honest error message helps. Mom’s spaghetti thanks you for clarity.


88-88: Clear error naming
No guesswork with EDIT_HELPER_ERROR.


98-98: Deriving clarity
GET_ALL_HELPERS_ERROR is short and sweet. Thanks for not letting confusion linger.


119-119: Same pattern
Error name consistency tames sweaty confusion. Good on you.


131-131: 404 check
Explicitly telling them the helper wasn’t found is nice. Palms are less sweaty with clarity.


136-136: Dedicated error code
Yes, consistent naming is pure gold. Bravo.


27-27: 🛠️ Refactor suggestion

Use Number.isNaN
If (order && isNaN(order)), swap with Number.isNaN to avoid type coercion fiascos. Mom’s spaghetti code begs for mercy.

-if ((order && isNaN(order)) || order < 1) {
+if (order && (Number.isNaN(order) || order < 1)) {
   ...
}

Likely invalid or redundant comment.

backend/src/test/e2e/link.test.mjs (7)

35-35: Consolidated describe
Grouping tests neatly is helpful. Less code spaghetti = calmer knees.


49-49: Robust DB check
Logging DB readiness issues can save you from messy debugging. Good.


60-60: Unauthorized request
Well-defined 401 scenario. A stable hallmark of your test coverage.


63-63: Invalid URL error
Ensuring we test the invalid URL path is crucial. Strong coverage, no sweaty regrets.


68-68: Missing URL test
Again, thorough coverage is the name of the game. Arms heavy but test coverage is heavier.


74-74: Positive integer order
Your test insists on positivity. That’s good. Others might trip over negative values.


122-122: Order must be positive
Clear error message tested. We’re safe from code heartbreak.

backend/postman/HelperLinks.postman_collection.json (3)

17-17: Same JWT exposure issue as before! Let's keep it consistent 🔐

Please refer to the JWT security comment in the Links collection. The same solution applies here - use Postman environment variables instead of hardcoded tokens.

Also applies to: 102-102, 187-187, 272-272, 372-372, 458-458, 499-499

🧰 Tools
🪛 Gitleaks (8.21.2)

17-17: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


94-178: New endpoint looks solid! But let's double-check that request structure 🔍

The new get_helpers_with_links endpoint follows the established pattern. The request structure is clean and consistent with other endpoints.

Let's verify the route handler exists and handles the response correctly:

✅ Verification successful

Yo, the endpoint is legit! Implementation's clean like mom's spaghetti! 🍝

The route handler and implementation are properly set up with:

  • Route defined in helperLink.routes.js
  • Controller method getAllHelpersWithLinks properly handles errors and responses
  • Service layer fetches data with correct Sequelize associations
  • Proper error handling with standardized error responses

The request structure in the Postman collection matches the implementation perfectly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify route handler implementation

# Search for the route handler
ast-grep --pattern 'get_helpers_with_links = async (req, res) {
  $$$
}'

Length of output: 74


Script:

#!/bin/bash
# Let's try a different pattern and search for route definitions
rg -l "get_helpers_with_links" --type ts --type js

# Also search for router.get definitions
rg "router\.get.*helper.*link" --type ts --type js -A 2

Length of output: 513


Script:

#!/bin/bash
# Let's check the controller implementation
ast-grep --pattern 'getAllHelpersWithLinks = async (req, res) {
  $$$
}'

# Also search for the controller file content
rg -l "getAllHelpersWithLinks" --type ts --type js | xargs cat

Length of output: 11996

🧰 Tools
🪛 Gitleaks (8.21.2)

102-102: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


111-111: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)


341-341: Request body's got that fresh structure! But we need validation 🎨

The request bodies now include:

  • UI customization (colors)
  • Links array structure
  • User association

Let's verify the validation middleware handles these new fields:

Also applies to: 467-467

✅ Verification successful

Yo, the validation's got your back! 🍝

Knees weak with relief 'cause the validation is tight! Found comprehensive checks in both middleware and model:

  • helperValidator middleware's droppin' bars with:

    • Title validation ✓
    • Color hex validation for all UI elements ✓
    • Links array structure validation ✓
    • URL validation for each link ✓
  • Model's got that extra layer of validation sauce:

    • DB-level constraints for all fields ✓
    • Default colors set up ✓
    • Hex color validation on model level ✓

Mom's spaghetti never looked this organized! The request body structure is fully covered by validation, no need to lose yourself in worry.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check validation middleware for new fields

# Search for validation rules
ast-grep --pattern 'check([
  $$$
])'

Length of output: 39


Script:

#!/bin/bash
# Let's try a different approach to find validation logic
# Search for validation-related files and middleware
fd -e js -e ts validation

# Also search for express validation imports or requires
rg -l "express-validator|body\(|check\(|checkSchema|validate" 

Length of output: 1875


Script:

#!/bin/bash
# Search for route handlers that might contain validation
rg -l "helperLinks|helper.*links" --type js --type ts

# Look for schema definitions
rg -l "schema.*helper|helper.*schema" --type js --type ts

Length of output: 503


Script:

#!/bin/bash
# Search for any validation or middleware related to helper links
rg -B 2 -A 2 "helperLinks.*middleware|validate.*helper|helper.*validate"

Length of output: 4284


Script:

#!/bin/bash
# Let's check the validation implementation in helperLink.helper.js
cat backend/src/utils/helperLink.helper.js

Length of output: 1381


Script:

#!/bin/bash
# Check the model validation
cat backend/src/models/HelperLink.js

Length of output: 1777

backend/src/controllers/link.controller.js Outdated Show resolved Hide resolved
backend/src/controllers/helperLink.controller.js Outdated Show resolved Hide resolved
backend/src/test/e2e/link.test.mjs Outdated Show resolved Hide resolved
backend/postman/Links.postman_collection.json Show resolved Hide resolved
backend/postman/Links.postman_collection.json Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
backend/src/controllers/link.controller.js (1)

57-61: ⚠️ Potential issue

Mom's spaghetti alert! 🍝 Same order validation weakness as above!

This validation has the same issues as in addLink. Let's keep it consistent across the codebase.

Apply the same fix as suggested for the addLink method to maintain consistency.

🧹 Nitpick comments (4)
backend/src/controllers/helperLink.controller.js (2)

48-51: Enhance error logging with request context

Mom's spaghetti! The error logging could use some sauce! Consider adding request context to help with debugging.

Here's a tastier error handling approach:

-      console.log(err);
+      console.error('Failed to create helper', {
+        userId,
+        error: err.message,
+        stack: err.stack
+      });
       const { statusCode, payload } = internalServerError('CREATE_HELPER_ERROR', err.message);

62-62: Extract error messages to constants

There's vomit on his sweater already... I mean, these error messages are repeated! Let's extract them to constants at the top of the file for better maintainability.

Add this at the top of the file:

const ERROR_MESSAGES = {
  HELPER_NOT_FOUND: 'Helper not found',
  HELPER_ID_NOT_EXIST: 'Helper with the specified id does not exist',
};

Then use them in your error responses:

-          errors: [{ msg: 'Helper with the specified id does not exist' }],
+          errors: [{ msg: ERROR_MESSAGES.HELPER_ID_NOT_EXIST }],

Also applies to: 82-82, 131-131

backend/src/controllers/link.controller.js (2)

44-48: Clean success message, but let's make that error handling bulletproof! 🔥

The success response looks good, but we could enhance the error handling to be more specific about what went wrong.

Consider adding error type differentiation:

     } catch (err) {
-      const { statusCode, payload } = internalServerError('DELETE_LINK_ERROR', err.message);
+      const { statusCode, payload } = internalServerError(
+        'DELETE_LINK_ERROR',
+        `Failed to delete link ${id}: ${err.message}`
+      );
       res.status(statusCode).json(payload);
     }

27-29: Level up that error handling game! 💪

While the error handling is consistent, we could make it more informative for debugging.

Consider creating specific error types:

 const { internalServerError } = require('../utils/errors.helper');
+const { LinkError } = require('../utils/custom-errors');
 
 // In catch blocks:
-      const { statusCode, payload } = internalServerError('CREATE_LINK_ERROR', err.message);
+      const { statusCode, payload } = err instanceof LinkError
+        ? { statusCode: 400, payload: { errors: [{ msg: err.message }] } }
+        : internalServerError('CREATE_LINK_ERROR', err.message);

Also applies to: 46-48, 74-76, 84-86, 95-97, 112-114, 124-125

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f074815 and 26dd257.

📒 Files selected for processing (3)
  • backend/src/controllers/helperLink.controller.js (9 hunks)
  • backend/src/controllers/link.controller.js (8 hunks)
  • backend/src/test/e2e/link.test.mjs (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/src/test/e2e/link.test.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/controllers/helperLink.controller.js

[error] 26-26: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🔇 Additional comments (4)
backend/src/controllers/helperLink.controller.js (2)

108-108: Error code typo fixed correctly

The error code GET_ALL_HELPERS_WITH_LINKS_ERROR has been properly fixed with the underscore, as suggested in the previous review. Nice work!


Line range hint 141-145: LGTM! Clean exports structure

The exports are well-organized, exposing both the controller instance and validation utilities. This structure supports the validation refactoring objectives nicely.

backend/src/controllers/link.controller.js (2)

124-125: Palms ain't sweaty no more! Error handling is fixed! 👊

The previous issue with missing response has been properly addressed. Nice job implementing the fix!


Line range hint 1-127: Verify validation middleware implementation! 🎯

The removal of validation from the controller suggests it's been moved to middleware, but we need to verify this is properly implemented.

Let's check for the validation middleware:

✅ Verification successful

Yo, the validation game is strong! 💪

Looks like we've got a solid validation setup that's even better than mom's spaghetti! The validation has been properly moved to middleware and helper files with:

  • validation.middleware.js handling the express-validator results
  • link.helper.js containing all the validation rules:
    • URL validation with regex patterns
    • Title validation
    • Order validation (positive integers)
    • Helper ID validation
    • Target boolean validation

The routes in link.routes.js are properly using these validators before hitting the controller:

router.post('/add_link', linkValidator, handleValidationErrors, linkController.addLink);
router.put('/edit_link/:id', idParamValidator, linkValidator, handleValidationErrors, linkController.editLink);
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for validation middleware implementation
echo "Searching for validation middleware..."
rg -l "express-validator|body|param|query" src/middleware
rg -l "linkValidator|validateUrl" src/utils

# Check routes for validator usage
echo "Checking routes for validator usage..."
rg -A 3 "router\.(get|post|put|delete)" src/routes/link.routes.js

Length of output: 518


Script:

#!/bin/bash
# Let's search from the repository root with correct paths
echo "Searching for validation middleware..."
fd -e js -e ts middleware
fd -e js -e ts routes
fd -e js -e ts utils

echo "Checking for validation patterns..."
rg -l "express-validator|body|param|query" backend/
rg -l "linkValidator|validateUrl" backend/

echo "Checking routes implementation..."
fd "link.routes" backend/ --exec cat {}

Length of output: 8405


Script:

#!/bin/bash
# Let's check the validation implementation
echo "Checking validation middleware implementation..."
cat backend/src/middleware/validation.middleware.js

echo "Checking link helper validators..."
cat backend/src/utils/link.helper.js

Length of output: 2306

backend/src/controllers/helperLink.controller.js Outdated Show resolved Hide resolved
backend/src/controllers/link.controller.js Show resolved Hide resolved
Copy link
Collaborator

@erenfn erenfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I enter an invalid URL to a helper link, the error message is not shown correctly:

image

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
backend/src/controllers/helperLink.controller.js (3)

Line range hint 62-80: Arms are heavy! These endpoints might struggle with large datasets! 🍝

Consider implementing pagination to handle large numbers of helpers efficiently. This would improve performance and reduce server load.

Example implementation:

async getAllHelpers(req, res) {
  try {
    const { page = 1, limit = 10 } = req.query;
    const links = await helperService.getAllHelpers({ page, limit });
    res.status(200).json(links);
  } catch (err) {
    const { statusCode, payload } = internalServerError('GET_ALL_HELPERS_ERROR', err.message);
    res.status(statusCode).json(payload);
  }
}

Line range hint 93-108: Validate that ID before my knees get weak! 🍝

Consider adding ID format validation before making the service call to fail fast and save resources.

    try {
      const { id } = req.params;
+     if (!id.match(/^[0-9a-fA-F]{24}$/)) {  // Assuming MongoDB ObjectId
+       return res.status(400).json({ errors: [{ msg: 'Invalid helper ID format' }] });
+     }
      const helper = await helperService.getHelperById(id);

Line range hint 1-113: Documentation's ready - mom's spaghetti! 🍝

Consider adding JSDoc comments to document the class and its methods. This would improve maintainability and IDE support.

Example for the class:

/**
 * Controller handling helper link operations
 * @class LinkController
 */
class LinkController {
  /**
   * Creates a new helper with associated links
   * @param {Request} req - Express request object
   * @param {Response} res - Express response object
   * @returns {Promise<void>}
   */
  async addHelper(req, res) {
    // ... existing implementation
  }
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26dd257 and a9496b0.

📒 Files selected for processing (2)
  • backend/src/controllers/helperLink.controller.js (7 hunks)
  • frontend/src/scenes/links/NewLinksPopup.jsx (3 hunks)
🔇 Additional comments (4)
backend/src/controllers/helperLink.controller.js (2)

Line range hint 22-40: LGTM! This deletion logic is clean like mom's spaghetti! 🍝

The error handling and status codes are on point. Good job with the proper ID validation and clear error messages.


Line range hint 82-91: Clean implementation! No spaghetti code here! 🍝

Good use of authenticated user ID and proper error handling.

frontend/src/scenes/links/NewLinksPopup.jsx (2)

1-1: Good addition of AxiosError import.

My knees might be weak, but I'm still standing strong behind the decision to explicitly import AxiosError. This is valuable for narrowing down error handling logic and ensuring more specific error-handling branches.


87-93: Consider verifying the shape of the error before returning.

His palms might be sweaty, but we should still ensure that msg.response and subsequent properties (msg.response.data.errors) are reliably structured. A missing or malformed response could lead to runtime errors or inaccurate toast messages.

erenfn
erenfn previously approved these changes Jan 2, 2025
Copy link
Collaborator

@erenfn erenfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge after resolving the conflict

…g into 390-refactoring-validations-in-link-and-helper-link-routes-with-express-validator
@erenfn erenfn merged commit 296a4fd into develop Jan 2, 2025
2 checks passed
@erenfn erenfn deleted the 390-refactoring-validations-in-link-and-helper-link-routes-with-express-validator branch January 16, 2025 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring Validations in link and helper link routes with express-validator
2 participants