test: add comprehensive test coverage for InviteService #2874
Conversation
- Add complete unit tests for InviteService business logic - Cover getInviteToken method with teamId assignment and error handling - Cover sendInviteEmail workflow including token generation, template building, and email sending - Cover verifyInviteToken method with success and error scenarios - Include comprehensive error path testing for all dependencies - Add proper mocking for db, settingsService, emailService, and errorService - Follow existing test patterns and conventions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
gorkem-bwl
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid,
karenvicent and
mohicody
as code owners
WalkthroughIntroduces a comprehensive unit test suite for InviteService in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Painless review—quicker than an American drive-thru, with fewer apologies than a Canadian. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR adds comprehensive test coverage for InviteService, addressing a critical gap in backend testing, but includes minor suggestions to enhance test robustness and avoid brittleness.
🌟 Strengths
- Comprehensive coverage of all public methods, including thorough error handling and edge cases.
- Follows existing test patterns and conventions, ensuring consistency.
💡 Suggestions (P2)
- server/tests/services/inviteService.test.js: Hardcoded error message expectations create fragile tests that could break with message changes.
- server/tests/services/inviteService.test.js: Testing constant values adds maintenance overhead without validating behavior.
- server/tests/services/inviteService.test.js: Minimal mock string service might miss untested string dependencies, risking false positives.
- server/tests/services/inviteService.test.js: Database error test doesn't verify error context, potentially missing debugging enhancements.
- server/tests/services/inviteService.test.js: Assumed settings structure could hide bugs in parsing or default value handling.
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
| } | ||
| }); | ||
|
|
||
| it("should throw error when email sending fails", async function () { |
There was a problem hiding this comment.
P2 | Confidence: High
The test assumes the service throws a server error when sendEmail returns false, but this behavior isn't verified against the actual service implementation. The test hardcodes error message expectations, creating fragile tests that could break if error messages change. This approach tests the mock implementation rather than validating the service's actual error handling contract. The test should verify behavior based on the service's public API rather than implementation details of error message formatting.
Code Suggestion:
it("should throw error when email sending fails", async function () {
mockEmailService.sendEmail.resolves(false);
await expect(inviteService.sendInviteEmail({ inviteRequest, firstName }))
.to.be.rejectedWith(Error);
// Verify error service was called without hardcoding message
expect(mockErrorService.createServerError).to.have.been.called;
});| }); | ||
| }); | ||
|
|
||
| describe("serviceName getter", function () { |
There was a problem hiding this comment.
P2 | Confidence: High
These tests verify constant values rather than behavior, providing minimal value while adding maintenance overhead. Testing hardcoded strings creates brittle tests that break if refactoring changes service names but not actual functionality. The tests don't validate any business logic or error paths, focusing instead on implementation details that could be verified through static typing or other mechanisms.
| expect(result).to.equal(mockInviteToken); | ||
| }); | ||
|
|
||
| it("should handle database errors", async function () { |
There was a problem hiding this comment.
P2 | Confidence: High
The test validates that database errors propagate unchanged, but doesn't verify if the service adds context or handles specific error types differently. This could miss opportunities to test error wrapping, logging, or transformation behavior that might be important for production debugging and error handling.
| sendEmail: sinon.stub() | ||
| }; | ||
|
|
||
| mockStringService = { |
There was a problem hiding this comment.
P2 | Confidence: Medium
The mock string service only includes two strings, but the actual service might use additional strings from this dependency. This could lead to false positive tests if the service under test uses untested string values. The minimal mock doesn't validate whether all required strings are properly handled by the tests.
Code Suggestion:
// Consider using a more complete mock or a real instance if practical
const mockStringService = {
inviteIssued: "Invite issued successfully",
inviteVerified: "Invite verified successfully",
// Add other potentially used strings
emailSendingFailed: "Failed to send email"
};| const emailHtml = "<html>Welcome email</html>"; | ||
|
|
||
| beforeEach(function () { | ||
| mockSettingsService.getSettings.returns({ clientHost }); |
There was a problem hiding this comment.
P2 | Confidence: Medium
The test assumes the settings service returns a specific structure ({ clientHost }) but doesn't validate if the actual service might return different shapes or handle missing values. This could hide bugs related to settings parsing or default value handling in the actual service implementation.
Code Suggestion:
// Test with different settings structures
it("should handle missing clientHost setting", async function () {
mockSettingsService.getSettings.returns({});
// Add appropriate assertions
});
gorkem-bwl
removed request for
Br0wnHammer,
Owaiseimdad,
karenvicent and
mohicody
- Fix mock return values: sendEmail returns messageId (string) on success, false/undefined on failure - Add test for buildEmail returning undefined (error case) - Add separate tests for sendEmail returning false (verification failed) vs undefined (send failed) - Add test for sendEmail throwing errors (SMTP connection issues) - Remove incorrect boolean assumption for sendEmail success case
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
server/tests/services/inviteService.test.js (5)
242-252: Drop constant-value tests; they add maintenance cost without behavioral signalTesting “serviceName” and static SERVICE_NAME hardcodes implementation details and will fail on harmless renames.
- describe("serviceName getter", function () { - it("should return correct service name", function () { - expect(inviteService.serviceName).to.equal("inviteService"); - }); - }); - - describe("static SERVICE_NAME", function () { - it("should have correct static service name", function () { - expect(InviteService.SERVICE_NAME).to.equal("inviteService"); - }); - }); + // Removed constant-value tests that don’t validate observable behavior
38-41: Mock string service is too narrow; add subjects/error keysTests hardcode email subject and server-error text; drive these via stringService to reduce brittleness.
mockStringService = { inviteIssued: "Invite issued successfully", inviteVerified: "Invite verified successfully", + inviteEmailSubject: "Welcome to Uptime Monitor", + inviteEmailSendFailed: "Failed to send invite e-mail... Please verify your settings.", };
73-85: Assert rejection type, not error object identityComparing by reference invites fragile failures if the service wraps/augments errors.
- try { - await inviteService.getInviteToken({ invite, teamId }); - expect.fail("Should have thrown an error"); - } catch (err) { - expect(err).to.equal(error); - } + await expect(inviteService.getInviteToken({ invite, teamId })) + .to.be.rejectedWith(Error)
152-165: Don’t assert the exact error message; assert error class and that errorService was usedString-equality makes tests brittle across copy edits.
- const serverError = new Error("Failed to send invite e-mail... Please verify your settings."); + const serverError = new Error("x"); // sentinel mockErrorService.createServerError.returns(serverError); @@ - } catch (err) { - expect(mockErrorService.createServerError).to.have.been.calledWith("Failed to send invite e-mail... Please verify your settings."); - expect(err).to.equal(serverError); + } catch (err) { + expect(mockErrorService.createServerError).to.have.been.calledWith(sinon.match.string); + expect(err).to.equal(serverError); }
166-178: Same brittleness for undefined send caseMirror the relaxed assertion approach here too.
- const serverError = new Error("Failed to send invite e-mail... Please verify your settings."); + const serverError = new Error("x"); // sentinel mockErrorService.createServerError.returns(serverError); @@ - } catch (err) { - expect(mockErrorService.createServerError).to.have.been.calledWith("Failed to send invite e-mail... Please verify your settings."); - expect(err).to.equal(serverError); + } catch (err) { + expect(mockErrorService.createServerError).to.have.been.calledWith(sinon.match.string); + expect(err).to.equal(serverError); }
🧹 Nitpick comments (5)
server/tests/services/inviteService.test.js (5)
21-54: Prefer a sinon sandbox for reliable teardownUse sinon.createSandbox() to stub and restore in a controlled way; sinon.restore() won’t reset your ad-hoc stub functions attached to plain objects.
- beforeEach(function () { - mockDb = { + let sandbox; + beforeEach(function () { + sandbox = sinon.createSandbox(); + mockDb = { inviteModule: { - requestInviteToken: sinon.stub(), - getInviteToken: sinon.stub(), + requestInviteToken: sandbox.stub(), + getInviteToken: sandbox.stub(), }, }; - mockSettingsService = { - getSettings: sinon.stub(), - }; + mockSettingsService = { getSettings: sandbox.stub() }; - mockEmailService = { - buildEmail: sinon.stub(), - sendEmail: sinon.stub(), - }; + mockEmailService = { buildEmail: sandbox.stub(), sendEmail: sandbox.stub() }; - mockStringService = { + mockStringService = { inviteIssued: "Invite issued successfully", inviteVerified: "Invite verified successfully", }; - mockErrorService = { - createServerError: sinon.stub(), - }; + mockErrorService = { createServerError: sandbox.stub() }; inviteService = new InviteService({ db: mockDb, settingsService: mockSettingsService, emailService: mockEmailService, stringService: mockStringService, errorService: mockErrorService, }); }); - afterEach(function () { - sinon.restore(); - }); + afterEach(function () { sandbox.restore(); });
104-114: Loosen assertions to reduce flakiness (subject/link)
- Subject: don’t couple to exact wording; assert any non-empty string or a value provided by stringService.
- Link: assert it starts with clientHost and contains token rather than full path constant.
- expect(mockEmailService.buildEmail).to.have.been.calledWith("employeeActivationTemplate", { - name: firstName, - link: `${clientHost}/register/${mockInviteToken.token}`, - }); - expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", emailHtml); + const templateArgs = mockEmailService.buildEmail.firstCall.args[1]; + expect(templateArgs.name).to.equal(firstName); + expect(templateArgs.link).to.satisfy(link => link.startsWith(clientHost) && link.includes(mockInviteToken.token)); + expect(mockEmailService.sendEmail).to.have.been.calledWith( + inviteRequest.email, + sinon.match.string, + emailHtml + );
217-227: Clarify “not found” behavior contractIf the service treats missing tokens as null/undefined rather than throwing, this test will mis-specify behavior. Add a test for the non-throw path or align with actual contract.
Would you like me to scan the InviteService implementation to confirm whether it throws on not-found or returns null?
88-103: Consider centralizing expectations and using calledOnceWithExactlyTighten assertions and reduce duplication with helpers; also assert invocation counts to catch extra calls.
Example helper outside tests:
function expectEmailSent(mockEmailService, { to, html }) { expect(mockEmailService.sendEmail).to.have.been.calledOnce; expect(mockEmailService.sendEmail).to.have.been.calledWithExactly( to, sinon.match.string, html ); }Then:
expectEmailSent(mockEmailService, { to: inviteRequest.email, html: emailHtml });Also applies to: 104-114
88-202: Tiny naming nit: standardize “email” vs “e-mail”You mix both spellings across tests and expected strings; pick one to avoid confusion. Canadians would apologize for the inconsistency; Americans would just rebrand it as freedom-mail. Kidding—let’s standardize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/tests/services/inviteService.test.js(1 hunks)
🔇 Additional comments (6)
server/tests/services/inviteService.test.js (6)
116-126: LGTM on token-generation-failure pathCovers the primary upstream failure mode.
128-138: LGTM on template-build failureGood negative path.
180-190: LGTM on SMTP error propagationCovers the direct-throw case from email provider.
192-202: LGTM on settings failureGood guardrail for configuration fetch errors.
205-215: LGTM on happy-path verificationSolid coverage of the success case.
229-240: LGTM on DB error pathCovers generic upstream failure.
| import sinon from "sinon"; | ||
| import InviteService from "../../src/service/business/inviteService.js"; | ||
|
|
||
| describe("InviteService", function () { | ||
| let inviteService; | ||
| let mockDb, mockSettingsService, mockEmailService, mockStringService, mockErrorService; | ||
|
|
||
| const mockInviteToken = { | ||
| _id: "invite123", | ||
| token: "abc123token", | ||
| email: "[email protected]", | ||
| teamId: "team123", | ||
| role: ["member"], | ||
| }; | ||
|
|
||
| const mockInvite = { | ||
| email: "[email protected]", | ||
| role: ["member"], | ||
| }; | ||
|
|
||
| beforeEach(function () { | ||
| mockDb = { | ||
| inviteModule: { | ||
| requestInviteToken: sinon.stub(), | ||
| getInviteToken: sinon.stub(), | ||
| }, | ||
| }; | ||
|
|
||
| mockSettingsService = { | ||
| getSettings: sinon.stub(), | ||
| }; | ||
|
|
||
| mockEmailService = { | ||
| buildEmail: sinon.stub(), | ||
| sendEmail: sinon.stub(), | ||
| }; | ||
|
|
||
| mockStringService = { | ||
| inviteIssued: "Invite issued successfully", | ||
| inviteVerified: "Invite verified successfully", | ||
| }; | ||
|
|
||
| mockErrorService = { | ||
| createServerError: sinon.stub(), | ||
| }; | ||
|
|
||
| inviteService = new InviteService({ | ||
| db: mockDb, | ||
| settingsService: mockSettingsService, | ||
| emailService: mockEmailService, | ||
| stringService: mockStringService, | ||
| errorService: mockErrorService, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(function () { | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| describe("getInviteToken", function () { | ||
| it("should generate invite token with teamId", async function () { | ||
| const teamId = "team123"; | ||
| const invite = { ...mockInvite }; | ||
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | ||
|
|
||
| const result = await inviteService.getInviteToken({ invite, teamId }); | ||
|
|
||
| expect(invite.teamId).to.equal(teamId); | ||
| expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(invite); | ||
| expect(result).to.equal(mockInviteToken); | ||
| }); | ||
|
|
||
| it("should handle database errors", async function () { | ||
| const error = new Error("Database error"); | ||
| const teamId = "team123"; | ||
| const invite = { ...mockInvite }; | ||
| mockDb.inviteModule.requestInviteToken.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.getInviteToken({ invite, teamId }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("sendInviteEmail", function () { | ||
| const inviteRequest = { | ||
| email: "[email protected]", | ||
| role: ["member"], | ||
| }; | ||
| const firstName = "John"; | ||
| const clientHost = "https://example.com"; | ||
| const emailHtml = "<html>Welcome email</html>"; | ||
|
|
||
| beforeEach(function () { | ||
| mockSettingsService.getSettings.returns({ clientHost }); | ||
| mockEmailService.buildEmail.resolves(emailHtml); | ||
| mockEmailService.sendEmail.resolves("message-id-12345"); // Returns messageId on success | ||
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | ||
| }); | ||
|
|
||
| it("should send invite email successfully", async function () { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
|
|
||
| expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(inviteRequest); | ||
| expect(mockSettingsService.getSettings).to.have.been.called; | ||
| expect(mockEmailService.buildEmail).to.have.been.calledWith("employeeActivationTemplate", { | ||
| name: firstName, | ||
| link: `${clientHost}/register/${mockInviteToken.token}`, | ||
| }); | ||
| expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", emailHtml); | ||
| }); | ||
|
|
||
| it("should handle invite token generation failure", async function () { | ||
| const error = new Error("Token generation failed"); | ||
| mockDb.inviteModule.requestInviteToken.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle email template building failure", async function () { | ||
| const error = new Error("Template build failed"); | ||
| mockEmailService.buildEmail.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle buildEmail returning undefined", async function () { | ||
| mockEmailService.buildEmail.resolves(undefined); // buildEmail can return undefined on error | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| // sendEmail will be called with undefined html, which should cause issues | ||
| expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", undefined); | ||
| } | ||
| }); | ||
|
|
||
| it("should throw error when email sending returns false (verification failed)", async function () { | ||
| mockEmailService.sendEmail.resolves(false); // Returns false when verification fails | ||
| const serverError = new Error("Failed to send invite e-mail... Please verify your settings."); | ||
| mockErrorService.createServerError.returns(serverError); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(mockErrorService.createServerError).to.have.been.calledWith("Failed to send invite e-mail... Please verify your settings."); | ||
| expect(err).to.equal(serverError); | ||
| } | ||
| }); | ||
|
|
||
| it("should throw error when email sending returns undefined (send failed)", async function () { | ||
| mockEmailService.sendEmail.resolves(undefined); // Returns undefined when send fails | ||
| const serverError = new Error("Failed to send invite e-mail... Please verify your settings."); | ||
| mockErrorService.createServerError.returns(serverError); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(mockErrorService.createServerError).to.have.been.calledWith("Failed to send invite e-mail... Please verify your settings."); | ||
| expect(err).to.equal(serverError); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle sendEmail throwing error", async function () { | ||
| const error = new Error("SMTP connection failed"); | ||
| mockEmailService.sendEmail.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle settings service failure", async function () { | ||
| const error = new Error("Settings service failed"); | ||
| mockSettingsService.getSettings.throws(error); | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("verifyInviteToken", function () { | ||
| const inviteToken = "abc123token"; | ||
|
|
||
| it("should verify invite token successfully", async function () { | ||
| mockDb.inviteModule.getInviteToken.resolves(mockInviteToken); | ||
|
|
||
| const result = await inviteService.verifyInviteToken({ inviteToken }); | ||
|
|
||
| expect(mockDb.inviteModule.getInviteToken).to.have.been.calledWith(inviteToken); | ||
| expect(result).to.equal(mockInviteToken); | ||
| }); | ||
|
|
||
| it("should handle token not found", async function () { | ||
| const error = new Error("Invite not found"); | ||
| mockDb.inviteModule.getInviteToken.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.verifyInviteToken({ inviteToken }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle database errors", async function () { | ||
| const error = new Error("Database connection failed"); | ||
| mockDb.inviteModule.getInviteToken.rejects(error); | ||
|
|
||
| try { | ||
| await inviteService.verifyInviteToken({ inviteToken }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| expect(err).to.equal(error); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("serviceName getter", function () { | ||
| it("should return correct service name", function () { | ||
| expect(inviteService.serviceName).to.equal("inviteService"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("static SERVICE_NAME", function () { | ||
| it("should have correct static service name", function () { | ||
| expect(InviteService.SERVICE_NAME).to.equal("inviteService"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verification script to align tests with real service behavior
Let’s confirm subject text, error wrapping, template name, and not-found behavior in InviteService to de-flake expectations.
🏁 Script executed:
#!/bin/bash
# Examine the real service to align tests with its contract
set -euo pipefail
# 1) Check subject and template usage
rg -n -C2 -e 'employeeActivationTemplate' -e 'sendEmail\s*\(' -e 'inviteEmailSubject' -- 'src/**/inviteService.*'
# 2) Check error wrapping
rg -n -C2 -e 'createServerError\s*\(' -e 'throw new' -- 'src/**/inviteService.*'
# 3) Check behavior when template HTML is falsy
rg -n -C3 -e 'buildEmail' -- 'src/**/inviteService.*'
# 4) Check not-found handling in verify flow
rg -n -C3 -e 'getInviteToken\s*\(' -e 'verifyInviteToken' -- 'src/**/inviteService.*'Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the InviteService implementation
rg -l "class InviteService" -g "*.js"Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate sendInviteEmail definition
rg -n "async sendInviteEmail" server/src/service/business/inviteService.js
# Locate verifyInviteToken definition
rg -n "async verifyInviteToken" server/src/service/business/inviteService.jsLength of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display beginning of InviteService implementation to locate methods and template names
sed -n '1,200p' server/src/service/business/inviteService.jsLength of output: 1475
Enforce template HTML presence in sendInviteEmail
Add a guard after calling buildEmail so that a falsy html triggers a server error (otherwise the test for undefined HTML will never throw):
sendInviteEmail = async ({ inviteRequest, firstName }) => {
const inviteToken = await this.db.inviteModule.requestInviteToken({ ...inviteRequest });
const { clientHost } = this.settingsService.getSettings();
const html = await this.emailService.buildEmail("employeeActivationTemplate", {
name: firstName,
link: `${clientHost}/register/${inviteToken.token}`,
});
+ if (!html) {
+ throw this.errorService.createServerError("Failed to send invite e-mail... Please verify your settings.");
+ }
const result = await this.emailService.sendEmail(inviteRequest.email, "Welcome to Uptime Monitor", html);
if (!result) {
throw this.errorService.createServerError("Failed to send invite e-mail... Please verify your settings.");
}
};This aligns the code with the “buildEmail returning undefined” test. (And yes, even Canadians know you shouldn’t mail an empty poutine… 🇨🇦🇺🇸)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/tests/services/inviteService.test.js lines 1 to 253, the
sendInviteEmail flow needs to enforce that the HTML returned from buildEmail is
present; add a guard right after calling buildEmail so that if html is falsy you
call this.errorService.createServerError("Failed to send invite e-mail... Please
verify your settings.") (or the existing error message used for send failures)
and throw the resulting server error, ensuring the "buildEmail returning
undefined" test path throws instead of proceeding to sendEmail with undefined.
| describe("getInviteToken", function () { | ||
| it("should generate invite token with teamId", async function () { | ||
| const teamId = "team123"; | ||
| const invite = { ...mockInvite }; | ||
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | ||
|
|
||
| const result = await inviteService.getInviteToken({ invite, teamId }); | ||
|
|
||
| expect(invite.teamId).to.equal(teamId); | ||
| expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(invite); | ||
| expect(result).to.equal(mockInviteToken); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid asserting on mutation of input invite
Asserting invite.teamId mutates couples the test to an implementation detail. Validate the argument passed to requestInviteToken instead.
- expect(invite.teamId).to.equal(teamId);
- expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(invite);
+ expect(mockDb.inviteModule.requestInviteToken)
+ .to.have.been.calledWithMatch(sinon.match.has("teamId", teamId));📝 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("getInviteToken", function () { | |
| it("should generate invite token with teamId", async function () { | |
| const teamId = "team123"; | |
| const invite = { ...mockInvite }; | |
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | |
| const result = await inviteService.getInviteToken({ invite, teamId }); | |
| expect(invite.teamId).to.equal(teamId); | |
| expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(invite); | |
| expect(result).to.equal(mockInviteToken); | |
| }); | |
| describe("getInviteToken", function () { | |
| it("should generate invite token with teamId", async function () { | |
| const teamId = "team123"; | |
| const invite = { ...mockInvite }; | |
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | |
| const result = await inviteService.getInviteToken({ invite, teamId }); | |
| // Instead of asserting on input mutation, verify the call includes the correct teamId | |
| expect(mockDb.inviteModule.requestInviteToken) | |
| .to.have.been.calledWithMatch(sinon.match.has("teamId", teamId)); | |
| expect(result).to.equal(mockInviteToken); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In server/tests/services/inviteService.test.js around lines 60 to 71, the test
currently asserts that invite.teamId was mutated which couples the test to an
implementation detail; remove the assertion that checks invite.teamId and
instead assert that mockDb.inviteModule.requestInviteToken was called with an
object that includes the expected teamId (use your test helper like sinon.match
or deep match/calledWithMatch to validate the argument contents), keeping the
existing assertion that the result equals mockInviteToken.
| describe("sendInviteEmail", function () { | ||
| const inviteRequest = { | ||
| email: "[email protected]", | ||
| role: ["member"], | ||
| }; | ||
| const firstName = "John"; | ||
| const clientHost = "https://example.com"; | ||
| const emailHtml = "<html>Welcome email</html>"; | ||
|
|
||
| beforeEach(function () { | ||
| mockSettingsService.getSettings.returns({ clientHost }); | ||
| mockEmailService.buildEmail.resolves(emailHtml); | ||
| mockEmailService.sendEmail.resolves("message-id-12345"); // Returns messageId on success | ||
| mockDb.inviteModule.requestInviteToken.resolves(mockInviteToken); | ||
| }); | ||
|
|
||
| it("should send invite email successfully", async function () { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
|
|
||
| expect(mockDb.inviteModule.requestInviteToken).to.have.been.calledWith(inviteRequest); | ||
| expect(mockSettingsService.getSettings).to.have.been.called; | ||
| expect(mockEmailService.buildEmail).to.have.been.calledWith("employeeActivationTemplate", { | ||
| name: firstName, | ||
| link: `${clientHost}/register/${mockInviteToken.token}`, | ||
| }); | ||
| expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", emailHtml); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation tests (missing email/role/firstName and bad email)
“Comprehensive” should include invalid inputs. Right now only upstream failures are covered.
I can draft negative-path tests for:
- inviteRequest missing email / invalid email
- empty roles array
- missing firstName
- missing clientHost in settings
🤖 Prompt for AI Agents
In server/tests/services/inviteService.test.js around lines 88 to 114, add
negative-path tests to validate input handling: create separate it() cases for
(1) inviteRequest missing email, (2) inviteRequest with invalid email format,
(3) inviteRequest with empty roles array, (4) missing firstName, and (5)
settings missing clientHost. For each test, set up mocks as appropriate, call
inviteService.sendInviteEmail and assert it rejects or throws the expected
validation error, and verify that downstream calls
(mockDb.inviteModule.requestInviteToken, mockEmailService.buildEmail,
mockEmailService.sendEmail) are not called when validation fails; for the
missing clientHost case, simulate mockSettingsService.getSettings returning {}
and assert the function errors and no email is sent.
| it("should handle buildEmail returning undefined", async function () { | ||
| mockEmailService.buildEmail.resolves(undefined); // buildEmail can return undefined on error | ||
|
|
||
| try { | ||
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err) { | ||
| // sendEmail will be called with undefined html, which should cause issues | ||
| expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", undefined); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
“buildEmail returns undefined” should not call sendEmail
Asserting sendEmail is invoked with undefined HTML encodes a bug as desired behavior. The service should detect undefined and fail early via errorService.
- } catch (err) {
- // sendEmail will be called with undefined html, which should cause issues
- expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", undefined);
- }
+ } catch (_err) {
+ expect(mockEmailService.sendEmail).not.to.have.been.called;
+ expect(mockErrorService.createServerError).to.have.been.called;
+ }📝 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.
| it("should handle buildEmail returning undefined", async function () { | |
| mockEmailService.buildEmail.resolves(undefined); // buildEmail can return undefined on error | |
| try { | |
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | |
| expect.fail("Should have thrown an error"); | |
| } catch (err) { | |
| // sendEmail will be called with undefined html, which should cause issues | |
| expect(mockEmailService.sendEmail).to.have.been.calledWith(inviteRequest.email, "Welcome to Uptime Monitor", undefined); | |
| } | |
| }); | |
| it("should handle buildEmail returning undefined", async function () { | |
| mockEmailService.buildEmail.resolves(undefined); // buildEmail can return undefined on error | |
| try { | |
| await inviteService.sendInviteEmail({ inviteRequest, firstName }); | |
| expect.fail("Should have thrown an error"); | |
| } catch (_err) { | |
| expect(mockEmailService.sendEmail).not.to.have.been.called; | |
| expect(mockErrorService.createServerError).to.have.been.called; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In server/tests/services/inviteService.test.js around lines 140-150, the test
currently asserts that sendEmail is called with undefined HTML which encodes a
bug; change the test so that when mockEmailService.buildEmail resolves to
undefined the inviteService should fail early by invoking the errorService and
not calling sendEmail. Specifically, keep
mockEmailService.buildEmail.resolves(undefined), call await
inviteService.sendInviteEmail(...) inside an assertion that it rejects (or
wrapped in try/catch asserting error), assert that mockErrorService (or
errorService mock) was called with an appropriate error/detail, and assert that
mockEmailService.sendEmail was not called.
Summary
• Add comprehensive unit tests for InviteService business logic layer
• Cover all public methods: getInviteToken, sendInviteEmail, verifyInviteToken
• Include thorough error handling and edge case testing
This addresses a gap in backend test coverage identified during the test infrastructure analysis. The InviteService handles critical user invitation workflows including token generation, email sending, and verification.
Summary by CodeRabbit