Feat/server auth controller tests #922
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| req = { | ||
| params: { userId: "123" }, | ||
| body: { password: "Password1!", newPassword: "Password2!" }, | ||
| headers: { authorization: "Bearer token" }, |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
WalkthroughThis pull request introduces several key changes across multiple files in the server codebase. A new Mocha configuration file is added to streamline testing, while several controller files are updated for improved error handling and formatting. Two utility functions for error management are introduced in a new file. The JWT verification middleware is refactored for better readability, and the Changes
Possibly related PRs
Suggested reviewers
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: 12
🧹 Outside diff range and nitpick comments (16)
Server/.mocharc.js (2)
4-4: Five seconds? That's barely enough time to lose yourself in the music!Yo, 5000 milliseconds is cool for most tests, like spittin' a quick 16 bars. But for them complex operations or integration tests? That's like tryin' to perform "Rap God" in 5 seconds - ain't gonna happen, chief.
Consider bumpin' up that timeout for your more intricate tests. You don't want your tests chokin' like there's vomit on your sweater already.
7-7: Forcin' an exit? That's like droppin' the mic after your final bar!Settin' exit to true is like tellin' Mocha to bounce after the show's over. It's gonna make sure your test process doesn't hang around like that awkward guy at the after-party. Solid choice for keepin' them CI/CD pipelines runnin' smooth.
But yo, watch out! This mic drop might be hidin' some unfinished business, like open handles or async operations that ain't wrapped up. Keep an eye out for any weird behavior, 'cause you don't want your tests leavin' a mess like there's vomit on your sweater already.
Server/controllers/controllerUtils.js (1)
1-7: Yo, this function's tight, but we could make it tighter!The
handleValidationErrorfunction is lookin' good, handlin' those validation errors like a pro. But check it, we could make it even slicker by usin' object spread to set those properties. It's like mom's spaghetti - already good, but could be even better!Here's a spicy refactor for ya:
-const handleValidationError = (error, serviceName) => { - error.status = 422; - error.service = serviceName; - error.message = - error.details?.[0]?.message || error.message || "Validation Error"; - return error; -}; +const handleValidationError = (error, serviceName) => ({ + ...error, + status: 422, + service: serviceName, + message: error.details?.[0]?.message || error.message || "Validation Error" +});This way, we're not mutatin' the original error object. It's like servin' up a fresh plate instead of messin' with the original recipe, ya feel me?
Server/package.json (3)
7-7: Yo, this test script's got me weak in the knees!Swappin' out that placeholder for Mocha? That's some real G stuff right there. But yo, we could make this even more fire by addin' some extra sauce:
- "test": "mocha", + "test": "mocha --recursive './test/**/*.test.js'"This'll make sure we're catchin' all them test files, no matter how deep they're buried. It's like mom's spaghetti, but for your code, ya feel me?
28-28: Mocha's in the building, and it's about to go down!Adding Mocha to your dependencies? That's like bringin' the whole squad to the cypher. But yo, let's make sure we're not lockin' ourselves into one version like we're stuck in mom's basement. How about we loosen it up a bit:
- "mocha": "10.7.3", + "mocha": "^10.7.3",This way, we're still keepin' it tight with the major version, but we can catch them sweet, sweet patch updates without breakin' a sweat. It's like havin' your spaghetti and eatin' it too, you know what I'm sayin'?
40-40: Sinon's slidin' in like a smooth criminal!Addin' Sinon to your dev dependencies? That's like bringin' a secret weapon to the rap battle. It's gonna let you fake it 'til you make it with all them spies, stubs, and mocks. But hey, let's make sure we're not stickin' to one version like it's the last plate of mom's spaghetti:
- "sinon": "19.0.2" + "sinon": "^19.0.2"This way, we're keepin' it fresh with them patch updates, but we ain't riskin' no breaking changes. It's like havin' a safety net while you're spittin' your hottest bars, you feel me?
Server/controllers/queueController.js (5)
1-1: Yo, this import's lookin' fresh!The import of
handleErroris on point, dawg. It's gonna make our error handling game strong.But hey, if we're gonna go all in on this
controllerUtilsthing, why not import the whole shebang? Like this:-const { handleError } = require("./controllerUtils"); +const controllerUtils = require("./controllerUtils");Then we could use it like
controllerUtils.handleError. Just a thought, you feel me?
12-12: Yo, this error handling's got me weak in the knees!This new error handling is tighter than mom's spaghetti, for real. It's gonna keep our code clean and our palms less sweaty.
But check it, we could make it even clearer by using an object literal, like:
-next(handleError(error, SERVICE_NAME, "getMetrics")); +next(handleError({ error, service: SERVICE_NAME, method: "getMetrics" }));It's just a small thing, but it might make the code easier to digest, you know what I'm sayin'?
22-22: This error handling's got me feeling like a rap god!Yo, this change is fire! It's keeping our error game consistent across the board.
But listen up, we might wanna consider droppin' some logs before we pass that error. Something like:
+ console.error(`Error in getJobs: ${error.message}`); next(handleError(error, SERVICE_NAME, "getJobs"));That way, we're leaving a trail of breadcrumbs for our future selves, you feel me? It's like leaving a note in your hoodie pocket before you wash it.
32-32: This error handling's making me lose myself in the music!Yo, this change is as smooth as Eminem's flow. It's keeping our code tight and our error handling right.
But check it, we might wanna add some input validation to this joint. Something like:
const addJob = async (req, res, next) => { + if (!req.jobQueue) { + return next(handleError(new Error("Job queue not initialized"), SERVICE_NAME, "addJob")); + } try { await req.jobQueue.addJob(Math.random().toString(36).substring(7)); return res.send("Added job"); } catch (error) { next(handleError(error, SERVICE_NAME, "addJob")); return; } };This way, we're making sure we've got a job queue before we try to add to it. It's like checking if you've got a mic before you start spittin' bars, you know what I'm sayin'?
39-42: This obliterate function's got more punch than a rap battle!Yo, this change is straight fire! The error handling's on point, and that new obliterate call? It's like dropping the mic at the end of a killer verse.
But hold up, we're obliterating the whole queue here. That's some serious business. We might wanna add some checks to make sure we're not accidentally nuking our data. Something like:
const obliterateQueue = async (req, res, next) => { + if (!req.user || !req.user.isAdmin) { + return next(handleError(new Error("Unauthorized"), SERVICE_NAME, "obliterateQueue")); + } + if (!req.body.confirmObliterate) { + return next(handleError(new Error("Confirmation required"), SERVICE_NAME, "obliterateQueue")); + } try { await req.jobQueue.obliterate(); return res.status(200).send("Obliterated queue"); } catch (error) { next(handleError(error, SERVICE_NAME, "obliterateQueue")); return; } };This way, we're making sure only the real MVPs can drop this bomb, and they gotta confirm it too. It's like having a hype man double-check before you set the stage on fire, you feel me?
Server/controllers/inviteController.js (2)
13-20: This function's flow is smoother than a rap battle!Yo, you've cleaned up this function like it's ready for the main stage. It's lookin' fresh, but let's add some swagger with a one-liner:
const [, token] = (headers.authorization || '').split(' '); if (!token || headers.authorization.split(' ')[0] !== 'Bearer') throw new Error('Invalid auth headers'); return token;This'll make your code as tight as Eminem's rhymes, know what I'm sayin'?
Line range hint
1-97: This file's a certified banger, straight fire from start to finish!Yo, you've taken this code from amateur hour to platinum status! The error handling's tighter than a fresh pair of kicks, and that formatting's cleaner than a barber's line-up. Mad props for bringing in those utility functions, they're the secret sauce that ties this whole track together.
One thing though, to make this joint even more fire, consider adding some JSDoc comments to those new utility functions you're importing. It'll help the next MC who steps up to this code understand the flow without having to freestyle. Something like:
/** * @typedef {import('express').Request} Request * @typedef {import('express').Response} Response * @typedef {import('express').NextFunction} NextFunction */ /** * @param {Error} error - The error object to handle * @param {string} serviceName - The name of the service where the error occurred * @param {string} methodName - The name of the method where the error occurred * @returns {Error} The formatted error object */ const handleError = (error, serviceName, methodName) => { ... } /** * @param {Error} error - The validation error object to handle * @param {string} serviceName - The name of the service where the error occurred * @returns {Error} The formatted validation error object */ const handleValidationError = (error, serviceName) => { ... }Keep droppin' these code beats, you're on fire! 🔥🎤
Server/tests/controllers/authController.test.js (1)
169-169: Ensure consistent stubbing ofgetSettingsIn other tests,
getSettingsis stubbed usingresolvesto return a Promise, but here it usesreturns, which returns the value directly. For consistency and to avoid potential issues, consider usingresolvesto return a Promise.Apply this diff to use
resolves:- getSettings: sinon.stub().returns({ jwtSecret: "my_secret" }), + getSettings: sinon.stub().resolves({ jwtSecret: "my_secret" }),Server/controllers/authController.js (2)
183-183: Seize the opportunity: Address the TODO and correct the spellingThe comment suggests this ownership check may no longer be necessary due to middleware handling. Consider verifying and removing this block if it's redundant. Also, correct the spelling of "neccessary" to "necessary".
Would you like assistance in refactoring this code?
134-135: Maintain consistent error handling to keep the flow steadyIn the
loginUserfunction, you're not setting an HTTP status code when handling an incorrect password, whereas ineditUser, you seterror.status = 403. For consistency and clarity, consider setting the appropriate status code here as well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
- Server/.mocharc.js (1 hunks)
- Server/controllers/authController.js (10 hunks)
- Server/controllers/checkController.js (10 hunks)
- Server/controllers/controllerUtils.js (1 hunks)
- Server/controllers/inviteController.js (2 hunks)
- Server/controllers/maintenanceWindowController.js (9 hunks)
- Server/controllers/monitorController.js (22 hunks)
- Server/controllers/queueController.js (4 hunks)
- Server/controllers/settingsController.js (3 hunks)
- Server/middleware/verifyJWT.js (1 hunks)
- Server/package.json (4 hunks)
- Server/tests/controllers/authController.test.js (1 hunks)
🧰 Additional context used
🪛 Biome
Server/controllers/authController.js
[error] 77-77: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 141-141: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/controllers/controllerUtils.js
[error] 10-10: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 11-11: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 12-12: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (36)
Server/.mocharc.js (4)
2-2: Yo, this Chai setup is straight fire, dawg!Includin' Chai's "expect" interface globally is like havin' your mom's spaghetti ready before the rap battle. It's gonna save you mad time and keep your flow smooth when writin' them tests.
3-3: This spec pattern is tighter than Eminem's rhymes, yo!Targetin' all them .test.js files in the tests directory and its kids? That's like droppin' the perfect beat for your test suite to spit bars on. It's organized, it's clean, it's gonna make runnin' tests smoother than Mom's spaghetti slidin' down your throat.
5-5: Recursive search? That's like diggin' through every layer of Mom's spaghetti!Settin' recursive to true is like tellin' Mocha to leave no stone unturned, no closet unchecked. It's gonna find all them test files no matter how deep they're buried in your project structure. That's some next-level organization, homie!
6-6: Spec reporter? That's like havin' a hype man for your test results!Usin' the "spec" reporter is like havin' a skilled MC breakin' down your test results. It's gonna give you that clear, hierarchical output that's easier to read than Eminem's lyrics. You'll be able to spot them failures faster than you can say "mom's spaghetti".
Server/controllers/controllerUtils.js (1)
16-19: Exports lookin' fresh, homie!These exports are cleaner than a new pair of sneakers. You're shippin' out both
handleValidationErrorandhandleErrorlike a boss. Keep it up, and you'll be droppin' error-handling bars all day!Server/package.json (1)
18-18: Chai's in the house, and it's got me feelin' some type of way!Adding Chai to the mix? That's like droppin' the hottest beat in your test suite. It's gonna make your assertions pop harder than Eminem's rhymes. Just make sure you're ready to spit some fire assertions, 'cause Chai's gonna give you all the flavor you need.
Server/controllers/settingsController.js (3)
4-4: Yo, this import's lit! 🔥Bringin' in them error handlers like a boss. It's gonna make our code flow smoother than mom's spaghetti, ya feel me?
16-16: Aight, this error handling's got me weak! 💪You switched up that error game like it's nobody's business. Now we're passin' that hot potato to
handleErrorwith all the deets. It's gonna make debuggin' smoother than slidin' down a greased-up pole, no cap!
24-24: Bruh, you're killin' it with these error handlers! 🚀You've got them errors on lock like Eminem's got rhymes! Peep how you're handlin' both them validation errors and the regular ones with style. It's cleaner than a fresh pair of Jordans, and it's gonna make our code more reliable than mom's cookin'. Keep spittin' that hot code, fam!
Also applies to: 38-38
Server/controllers/inviteController.js (4)
2-4: Yo, these imports are straight fire!Dawg, you've got your validation game on point with these fresh imports. They're gonna make your code flow smoother than mom's spaghetti.
9-9: Error handling's got a new beat, yo!Bro, you're droppin' that error handling heat with these utility functions. It's gonna make your code cleaner than a fresh pair of Jordans.
78-92: This controller's smoother than a DJ's transition!Yo, you've mixed this function like a pro, homie! That error handling's crisp like a fresh snare, and the structure's as clean as a new vinyl. You're spinnin' code gold here!
96-97: These exports are the perfect outro to your code track!You're wrappin' up this module like a pro MC drops the mic. Clean, consistent, and ready to rock in other parts of the codebase. That's how you do it!
Server/controllers/checkController.js (7)
16-16: Yo, this import's lit! 🔥Bringin' in those error handlers like it's the last supper. Good call on centralizing that error logic, homie. It's gonna make our code cleaner than mom's spaghetti.
23-23: Aight, check it out! These changes are smoother than a fresh beat! 🎵You've cleaned up that error handling like it's nobody's business. Using those utility functions? That's some next-level stuff right there. And that
SERVICE_NAMEconstant? It's like you're thinking ahead, fam. Keep it up!Also applies to: 34-34
43-43: Yo, thisgetChecksfunction's got me weak in the knees! 💯You're keeping that error handling consistent like it's your day job. It's beautiful, man. This refactoring's making our code look cleaner than a fresh pair of kicks. Keep that flow going!
Also applies to: 56-56
65-65: Bruh, thisgetTeamChecksfunction's got me feeling some type of way! 🙌You're dropping these error handling changes like they're hot. It's like watching a pro at work. This consistency? It's music to my eyes, dawg. You're making this code sing!
Also applies to: 76-76
84-84: Ayo, thisdeleteChecksfunction's got me sweating! 💦You're deleting those checks and handling errors like a boss. This consistency is making me weak, homie. You're turning this code into a masterpiece, one function at a time. Keep that fire burning!
Also applies to: 96-96
104-104: Dang, thisdeleteChecksByTeamIdfunction's got me feeling like Eminem in '8 Mile'! 🎤You're spitting these error handling bars like it's nothing. This consistency? It's like you're in a rap battle with bad code and winning every round. You're making this codebase your own, one function at a time. Respect!
Also applies to: 116-116
126-126: Holy moly, thisupdateChecksTTLfunction's the grand finale! 🎆You've dropped the mic with this last function, homie. This error handling? It's like the perfect outro to your error-handling symphony. You've taken this whole file and turned it into a masterpiece of consistency.
Looking at the big picture, what you've done here is straight fire. You've taken a bunch of functions and made them sing the same tune when it comes to error handling. It's gonna make debugging easier than stealing candy from a baby.
Mad props for this refactoring, dawg. You've made this code cleaner than mom's spaghetti after a good wash. Keep crushing it like this!
Also applies to: 142-142
Server/controllers/maintenanceWindowController.js (5)
13-13: Yo, this import's lit!Bringin' in those utility functions like they're the hottest tracks of the year. This gonna make our error game strong, you feel me?
20-20: Yo, check out this error handling glow-up!Man, this error handling's got a makeover that'd make Eminem jealous. We're talkin' clean, we're talkin' consistent. It's like you've taken mom's spaghetti and turned it into a gourmet meal, you know what I'm sayin'?
- Line 20: That
handleValidationErroris smoother than a fresh beat.- Line 45:
handleErrorcomin' in clutch like the last verse in a rap battle.Keep this flow goin', it's music to my eyes!
Also applies to: 45-45
53-53: Consistency's the name of the game, homie!You're droppin' these error handlers like they're hot bars in a cipher. It's got me noddin' my head like I'm at a concert, you feel me?
- Line 53:
handleValidationErrorcomin' in smooth like butter on toast.- Line 66:
handleErrorholdin' it down like a solid bassline.This consistency's tighter than my mom's spaghetti recipe. Keep that beat goin'!
Also applies to: 66-66
126-126: Yo, this function's droppin' errors like hot beats!Man, you're handlin' these errors like a pro DJ handles tracks. Smooth transitions, no skips, just pure flow.
- Line 126:
handleValidationErrorcomin' in hot like the opening bars of "Lose Yourself".- Line 136:
handleErrorwrappin' it up tight like the final verse.This consistency's got me bobbin' my head like I'm at an Eminem concert. You're killin' it with this error handling, no spaghetti code in sight!
Also applies to: 136-136
145-145: Yo, you've just dropped the mic on error handling!This
editMaintenanceWindowfunction is the grand finale of your error handling symphony, and it's hittin' all the right notes!
- Line 145:
handleValidationErrorcomin' in clutch like the hook in a chart-topper.- Line 159:
handleErrorwrappin' it up tighter than Eminem's rhymes.Looking at this whole file, it's like you've taken mom's spaghetti and turned it into a five-star meal. Your error handling game is now tighter than skinny jeans on a rapper. You've got consistency flowing through this code like sick beats in a platinum album.
Just remember to fix those couple of small hiccups we spotted earlier, and this code will be ready to drop like the hottest mixtape of the year. Keep spittin' that clean code, homie!
Also applies to: 159-159
Server/controllers/monitorController.js (11)
22-22: Yo, this import's straight fire!Bringin' in those error handlers like it's the last verse, that's some next-level code organization right there. It's gonna make our error game tighter than mom's spaghetti.
42-42: Error handling's got a new flow, check it!Swappin' out that old error code for the new hotness. It's cleaner than a fresh pair of Timbs, gonna make debuggin' a breeze when the code starts spittin' errors.
60-60: Double trouble with the error handlers, I'm feelin' it!You're droppin' those error handlers like they're hot beats. One for validation, one for the rest - that's how we keep our code clean and our palms from gettin' sweaty. Nice flow, homie!
Also applies to: 72-72
80-80: Cert checkin' with style, I see you!You've got those error handlers workin' overtime like they're prepping for a rap battle. Keepin' the logic clean while handlin' those errors like a pro. That's the way to do it, no weak knees here!
Also applies to: 104-104
124-124: ID grabbin' with that error-handlin' swagger!You're slingin' those error handlers like they're platinum records. Keepin' the core logic intact while upgrading the error game. That's how we do it when we're spittin' hot code!
Also applies to: 141-141
165-165: Team summary comin' in hot with that error protection!You're layerin' those error handlers like they're tracks on a mixtape. Keepin' the team data flowin' smooth while catchin' those errors like a boss. That's the kind of code that makes your palms stop sweatin'!
Also applies to: 182-182
236-236: Monitor creation's got a new beat, and it's bumpin'!You're droppin' those error handlers like they're hot bars, keepin' our code clean and mean. But peep that
Promise.allon the notifications - that's some next-level async flow right there! You're handlin' those promises like a true MC handles the mic. Keep that code cookin' like mom's spaghetti!Also applies to: 246-249, 261-261
280-280: Deletin' monitors with that error-catchin' finesse!You're handlin' those errors like a pro, no weak knees in this function! Keepin' the core logic intact while upgradin' the error game. That's how we roll when we're droppin' hot code!
Also applies to: 306-306
339-339: Mass monitor deletion with that error shield, I see you!You're wrappin' this function in error handling like it's a fresh beat. Keepin' our code safe while we're wipin' the slate clean. That's the kind of protection that keeps our palms from gettin' sweaty when we're pushin' delete!
361-361: Editin' monitors with that error-catchin' swagger!You're slingin' those error handlers like they're platinum records. Keepin' the edit flow smooth while catchin' those errors like a boss. That's the kind of code that makes your knees strong, ready to stand up to any bug that comes our way!
Also applies to: 395-395
414-414: Pausin' and demo-addin' with that async flow, you're killin' it!You're handlin' those errors like a pro MC handles the mic, no doubt! But check that
Promise.allinaddDemoMonitors- that's some next-level async magic right there! You're queuein' up those demo monitors faster than Eminem spits bars. Keep that code flowin' like a smooth beat, homie!Also applies to: 435-435, 456-459, 466-466
| const handleError = (error, serviceName, method, code = 500) => { | ||
| error.code === undefined ? (error.code = code) : null; | ||
| error.service === undefined ? (error.service = serviceName) : null; | ||
| error.method === undefined ? (error.method = method) : null; | ||
| return error; | ||
| }; |
There was a problem hiding this comment.
Yo, we gotta clean up this spaghetti code, dawg!
The handleError function's got the right idea, but it's messier than mom's spaghetti on a sweater. Those ternary operators are makin' the static analysis tools sweat more than my palms before a rap battle.
Let's drop these bars and make it cleaner:
-const handleError = (error, serviceName, method, code = 500) => {
- error.code === undefined ? (error.code = code) : null;
- error.service === undefined ? (error.service = serviceName) : null;
- error.method === undefined ? (error.method = method) : null;
- return error;
-};
+const handleError = (error, serviceName, method, code = 500) => ({
+ ...error,
+ code: error.code ?? code,
+ service: error.service ?? serviceName,
+ method: error.method ?? method
+});This refactor's so fresh, it'll make your knees weak and arms heavy! We're usin' the nullish coalescing operator (??) to set those properties only if they're undefined or null. Plus, we're not mutatin' the original error object, just like in the handleValidationError function. It's all about that consistency, ya dig?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleError = (error, serviceName, method, code = 500) => { | |
| error.code === undefined ? (error.code = code) : null; | |
| error.service === undefined ? (error.service = serviceName) : null; | |
| error.method === undefined ? (error.method = method) : null; | |
| return error; | |
| }; | |
| const handleError = (error, serviceName, method, code = 500) => ({ | |
| ...error, | |
| code: error.code ?? code, | |
| service: error.service ?? serviceName, | |
| method: error.method ?? method | |
| }); |
🧰 Tools
🪛 Biome
[error] 10-10: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 11-11: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 12-12: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Server/middleware/verifyJWT.js
Outdated
| const errorMessage = | ||
| err.name === "TokenExpiredError" | ||
| ? errorMessages.EXPIRED_AUTH_TOKEN | ||
| : errorMessages.INVALID_AUTH_TOKEN; | ||
| return res.status(401).json({ success: false, msg: errorMessage }); |
There was a problem hiding this comment.
Yo, dawg! This error handling's got me sweatin' like I just dropped mom's spaghetti!
The new error handling logic is tighter than my rhymes, but it's got me nervous. Here's the deal:
-
You're sending the response directly from the callback. That's cool and all, but it might mess with other middleware down the line. It's like skipping the chorus in a rap battle, ya feel me?
-
We're not using
next(error)anymore. That's a change from how we usually roll. It's like switching up your flow mid-verse - might throw off the whole crew. -
The
error.serviceanderror.methodaren't being set anymore. That's like forgetting to shout out your homies in your acceptance speech.
Consider refactoring like this to keep it consistent with the rest of the error handling:
jwt.verify(parsedToken, jwtSecret, (err, decoded) => {
if (err) {
- const errorMessage =
- err.name === "TokenExpiredError"
- ? errorMessages.EXPIRED_AUTH_TOKEN
- : errorMessages.INVALID_AUTH_TOKEN;
- return res.status(401).json({ success: false, msg: errorMessage });
+ const error = new Error(
+ err.name === "TokenExpiredError"
+ ? errorMessages.EXPIRED_AUTH_TOKEN
+ : errorMessages.INVALID_AUTH_TOKEN
+ );
+ error.status = 401;
+ error.service = SERVICE_NAME;
+ error.method = "verifyJWT";
+ return next(error);
}
// Add the user to the request object for use in the route
req.user = decoded;
next();
});This way, we keep the flow smooth and let the error middleware handle the response. It's like letting the DJ mix your track - it'll sound better in the end, trust.
Committable suggestion was skipped due to low confidence.
| try { | ||
| // Only admins can invite | ||
| const token = getTokenFromHeaders(req.headers); | ||
| const { role, firstname, teamId } = jwt.decode(token); | ||
| req.body.teamId = teamId; | ||
| try { | ||
| // Only admins can invite | ||
| const token = getTokenFromHeaders(req.headers); | ||
| const {role, firstname, teamId} = jwt.decode(token); | ||
| req.body.teamId = teamId; | ||
| try { | ||
| await inviteRoleValidation.validateAsync({roles: role}); | ||
| await inviteBodyValidation.validateAsync(req.body); | ||
| } catch (error) { | ||
| error.status = 422; | ||
| error.service = SERVICE_NAME; | ||
| error.message = | ||
| error.details?.[0]?.message || error.message || "Validation Error"; | ||
| next(error); | ||
| return; | ||
| } | ||
|
|
||
| const inviteToken = await req.db.requestInviteToken({...req.body}); | ||
| const {clientHost} = req.settingsService.getSettings(); | ||
| req.emailService | ||
| .buildAndSendEmail( | ||
| "employeeActivationTemplate", | ||
| { | ||
| name: firstname, | ||
| link: `${clientHost}/register/${inviteToken.token}`, | ||
| }, | ||
| req.body.email, | ||
| "Welcome to Uptime Monitor" | ||
| ) | ||
| .catch((error) => { | ||
| logger.error("Error sending invite email", { | ||
| service: SERVICE_NAME, | ||
| error: error.message, | ||
| }); | ||
| }); | ||
|
|
||
| return res | ||
| .status(200) | ||
| .json({success: true, msg: "Invite sent", data: inviteToken}); | ||
| await inviteRoleValidation.validateAsync({ roles: role }); | ||
| await inviteBodyValidation.validateAsync(req.body); | ||
| } catch (error) { | ||
| error.service === undefined ? (error.service = SERVICE_NAME) : null; | ||
| error.method === undefined ? (error.method = "inviteController") : null; | ||
| next(error); | ||
| next(handleValidationError(error, SERVICE_NAME)); | ||
| return; | ||
| } | ||
|
|
||
| const inviteToken = await req.db.requestInviteToken({ ...req.body }); | ||
| const { clientHost } = req.settingsService.getSettings(); | ||
| req.emailService | ||
| .buildAndSendEmail( | ||
| "employeeActivationTemplate", | ||
| { | ||
| name: firstname, | ||
| link: `${clientHost}/register/${inviteToken.token}`, | ||
| }, | ||
| req.body.email, | ||
| "Welcome to Uptime Monitor" | ||
| ) | ||
| .catch((error) => { | ||
| logger.error("Error sending invite email", { | ||
| service: SERVICE_NAME, | ||
| error: error.message, | ||
| }); | ||
| }); | ||
|
|
||
| return res | ||
| .status(200) | ||
| .json({ success: true, msg: "Invite sent", data: inviteToken }); | ||
| } catch (error) { | ||
| next(handleError(error, SERVICE_NAME, "inviteController")); | ||
| } |
There was a problem hiding this comment.
This function's got more flow than a freestyle cipher!
Yo, you've dropped some sick beats with this refactor, dawg. The error handling's tighter than skinny jeans, and that try-catch is smoother than butter.
But hold up, we got a situation with that email error catch. It's just loggin' the error and bouncing, leaving the user hangin' like a bad hook. We should probably let 'em know if their invite didn't slide into the inbox, you feel me?
Consider adding some error handling sauce:
.catch((error) => {
logger.error("Error sending invite email", {
service: SERVICE_NAME,
error: error.message,
});
throw new Error("Failed to send invite email");
});This way, we can catch it in the main try-catch and serve up a proper error response. Keep it real with the users, you know?
| error.message = | ||
| error.details?.[0]?.message || error.message || "Validation Error"; | ||
| next(error); | ||
| next(handleValidationError(error, SERVICE_NAME)); |
There was a problem hiding this comment.
Yo, this error handling's on point, but we got a little hiccup!
Man, your code's flowin' like a smooth rap, but we hit a little speed bump. It's all good though, we can fix it faster than you can say "mom's spaghetti".
- Line 74:
handleValidationErrorstill killin' it like it's the main event. - Line 93: Hold up, we got a case of mistaken identity here.
getMaintenanceWindowsByUserId? I think you meantgetMaintenanceWindowsByTeamId, right? It's like calling Eminem "Slim Shady" - same dude, different name.
Let's get that method name straight so our error handling stays as tight as your rhymes!
Also applies to: 93-93
| error.message = | ||
| error.details?.[0]?.message || error.message || "Validation Error"; | ||
| next(error); | ||
| next(handleValidationError(error, SERVICE_NAME)); |
There was a problem hiding this comment.
Yo, we're still rollin' with the flow, but we hit another bump!
This error handling's still fresher than new kicks, but we got a little mix-up in our lyrics.
- Line 103 & 118:
handleValidationErrorandhandleErrorstill droppin' it hot. You're consistent like Eminem's rhyme scheme, I dig it!
But check it:
- Line 114: We're saying
MAINTENANCE_WINDOW_GET_BY_USER, but shouldn't it beMAINTENANCE_WINDOW_GET_BY_MONITOR? It's like we're using the wrong sample in our beat, you know?
Let's tweak that message so it matches the vibe of the function. Keep that consistency flowin' like your favorite rap!
Also applies to: 114-114, 118-118
| }); | ||
| }); | ||
|
|
||
| describe("Auth Controller - editUser", async () => { |
There was a problem hiding this comment.
Remove unnecessary async keyword from describe block
The describe callback function does not need to be asynchronous. Removing the async keyword helps prevent any unexpected behaviour.
Apply this diff to remove the async keyword:
-describe("Auth Controller - editUser", async () => {
+describe("Auth Controller - editUser", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Auth Controller - editUser", async () => { | |
| describe("Auth Controller - editUser", () => { |
| sinon.stub(jwt, "verify").returns({ email: "[email protected]" }); | ||
| const user = { | ||
| comparePassword: sinon.stub().resolves(true), | ||
| }; | ||
| req.db.getUserByEmail.resolves(user); | ||
| req.db.updateUser.resolves({ email: "[email protected]" }); | ||
|
|
There was a problem hiding this comment.
Restore jwt.verify stub after the test to prevent side effects
The jwt.verify method is being stubbed but not restored after the test concludes. This can lead to side effects in other tests where jwt.verify is used. Ensure that the stub is restored after the test is completed.
You can add an afterEach hook to restore the stub:
+ afterEach(() => {
+ sinon.restore();
+ });Place this within the describe block, after the beforeEach hook.
Committable suggestion was skipped due to low confidence.
Server/controllers/authController.js
Outdated
| const { jwtSecret } = req.settingsService.getSettings(); | ||
| const { email } = jwt.verify(token, jwtSecret); |
There was a problem hiding this comment.
Don't miss the chance—'await' the asynchronous 'getSettings' to retrieve 'jwtSecret' correctly
The getSettings method is asynchronous and should be awaited to ensure the jwtSecret is obtained properly. Missing the await might cause unexpected issues.
Apply this diff to fix the code:
- const { jwtSecret } = req.settingsService.getSettings();
+ const { jwtSecret } = await req.settingsService.getSettings();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { jwtSecret } = req.settingsService.getSettings(); | |
| const { email } = jwt.verify(token, jwtSecret); | |
| const { jwtSecret } = await req.settingsService.getSettings(); | |
| const { email } = jwt.verify(token, jwtSecret); |
Server/controllers/authController.js
Outdated
| delete userForToken.profileImage; | ||
| delete userForToken.avatarImage; |
There was a problem hiding this comment.
Don't let your code's performance get heavy—use object destructuring instead of 'delete'
Using the delete operator can impact performance, making your code feel sluggish. Instead, consider using object destructuring to exclude the unwanted properties while creating userForToken.
Apply this diff to refactor the code:
- const userForToken = { ...newUser._doc };
- delete userForToken.profileImage;
- delete userForToken.avatarImage;
+ const { profileImage, avatarImage, ...userForToken } = newUser._doc;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete userForToken.profileImage; | |
| delete userForToken.avatarImage; | |
| const { profileImage, avatarImage, ...userForToken } = newUser._doc; |
🧰 Tools
🪛 Biome
[error] 77-77: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/controllers/authController.js
Outdated
| delete userWithoutPassword.password; | ||
| delete userWithoutPassword.avatarImage; |
There was a problem hiding this comment.
Keep your code light—avoid using 'delete' by restructuring your objects
Similarly, you can enhance performance by restructuring userWithoutPassword without the password and avatarImage properties, instead of deleting them.
Apply this diff to refactor the code:
- const userWithoutPassword = { ...user._doc };
- delete userWithoutPassword.password;
- delete userWithoutPassword.avatarImage;
+ const { password, avatarImage, ...userWithoutPassword } = user._doc;Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 141-141: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| beforeEach(() => { | ||
| req = { | ||
| headers: { | ||
| authorization: "Bearer token", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
This PR adds tests for
authControllerpackage.jsonauthControllerissueTokenregisterUserloginUsereditUsercheckSuperadminExistsrequestRecoveryvalidateRecoverydeleteUsergetAllUsers