fix invite and reset passwd email service #482
Conversation
WalkthroughThis pull request introduces significant changes to email configuration and functionality across the application. Key modifications include updates to email credentials in environment files, enhancements to email service capabilities, and refactoring of password reset and invitation email workflows. The changes enable email sending functionality, update email templates, and modify how password reset tokens are handled in both frontend and backend components. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant EmailService
User->>Frontend: Request Password Reset
Frontend->>Backend: Send Reset Token Request
Backend->>EmailService: Generate Reset Token
EmailService->>User: Send Reset Email
User->>Frontend: Click Reset Link
Frontend->>Backend: Validate Reset Token
Backend-->>Frontend: Token Validation Result
Frontend->>User: Allow Password Reset
Possibly Related PRs
Suggested Reviewers
Finishing Touches
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: 9
🔭 Outside diff range comments (1)
.env (1)
Line range hint
15-20: Mom's spaghetti moment! Email config needs fixing! 📧Found some issues in the email configuration:
EMAIL_PASSWORDappears to be truncated (ends with "passwor")EMAIL_ENABLEis set to false while other email configs are presentPlease verify:
- The complete email password
- Set
EMAIL_ENABLE=trueif email functionality should be active🧰 Tools
🪛 Gitleaks (8.21.2)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (4)
backend/src/templates/resetPassword.hbs (1)
1-6: Yo dawg, this email template is straight fire! 🔥The template's structure is clean and professional, with all the essential elements:
- Clear greeting
- Security warning about unrequested resets
- Straightforward instructions
- Professional sign-off
Just one tiny suggestion to make it even more lit:
-<p>To reset your password, click the link below:</p> +<p>To reset your password, click the secure link below:</p>backend/src/controllers/auth.controller.js (1)
161-162: Arms are heavy! Let's clean up those expired tokens! 🧹While destroying existing reset tokens is good, we should also clean up any other expired tokens for this user.
- await Token.destroy({ where: { userId: user.id, type: 'reset' } }); + await Token.destroy({ + where: { + userId: user.id, + [sequelize.Op.or]: [ + { type: 'reset' }, + { expiresAt: { [sequelize.Op.lt]: new Date() } } + ] + } + });frontend/src/scenes/login/SetNewPassword.jsx (1)
52-53: Cleanup on aisle 53! Double logging ain't cool! 🧼Remove the redundant error logging to keep the code clean.
- console.error("Password Reset failed:", error); - console.log("error", error); + console.error("Password Reset failed:", error);backend/.env (1)
Line range hint
1-35: Vomit on his sweater already: Let's organize this config file!The configuration entries are scattered throughout the file like loose spaghetti. Let's group related configs together for better maintainability.
Here's a suggested organization:
# Node environment NODE_ENV=development + +# Email Configuration +EMAIL_ENABLE=true +EMAIL=${SMTP_EMAIL} +EMAIL_PASSWORD=${SMTP_PASSWORD} +EMAIL_HOST=${SMTP_HOST} +EMAIL_PORT=${SMTP_PORT} +APP_PASSWORD=${SMTP_APP_PASSWORD} # Development environment DEV_DB_USERNAME=user123
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.env(1 hunks)backend/.env(2 hunks)backend/src/controllers/auth.controller.js(2 hunks)backend/src/service/email.service.js(2 hunks)backend/src/service/invite.service.js(2 hunks)backend/src/templates/invite.hbs(1 hunks)backend/src/templates/resetPassword.hbs(1 hunks)backend/src/utils/constants.helper.js(2 hunks)frontend/src/scenes/login/SetNewPassword.jsx(3 hunks)frontend/src/services/settingServices.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/templates/invite.hbs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/scenes/login/SetNewPassword.jsx (1)
17-18:⚠️ Potential issueThere's vomit on his sweater already! We need token validation! 🎯
The code doesn't validate if the token is present in the URL. Users could end up in an invalid state.
🧹 Nitpick comments (3)
frontend/src/tests/scenes/login/SetNewPassword.test.jsx (1)
50-51: Mom's spaghetti alert! 🍝 These comments need more sauce!The inline comments could be more descriptive:
- token: "mock-token", // Ensure token is passed - newPassword: "Test123!@", // Ensure the new password is passed + token: "mock-token", // Token from URL query parameter + newPassword: "Test123!@", // Password meeting all requirements (8+ chars, special char)frontend/src/scenes/login/SetNewPassword.jsx (2)
Line range hint
42-61: Knees weak, arms are heavy! Error handling needs more spaghetti! 🍝The error handling could be more robust:
- No timeout handling for the API call
- Generic error message might not be helpful
Try this enhancement:
onSubmit={async (values, { setSubmitting, resetForm }) => { setServerErrors([]); try { - const response = await resetPassword({ - token: token, - newPassword: values.password, - }); + const response = await Promise.race([ + resetPassword({ + token: token, + newPassword: values.password, + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Request timeout')), 5000) + ) + ]); resetForm(); navigate("/reset-password"); } catch (error) { console.error("Password Reset failed:", error); + const errorMessage = error.message === 'Request timeout' + ? "Request timed out. Please try again." + : error.response?.data?.errors + ? error.response.data.errors + : error.response?.data?.error + ? [error.response.data.error] + : ["An unexpected error occurred. Please try again later."]; - if (error.response?.data?.errors) { - setServerErrors(error.response?.data?.errors); - } else if (error.response?.data?.error) { - setServerErrors([error.response.data.error]); - } else { - setServerErrors(["An error occurred. Please try again."]); - } + setServerErrors(errorMessage); }
74-77: The UI text needs more sauce! 🍝The password requirements message could be more specific:
- Your new password must be different from previously used passwords. + Your new password must be at least 8 characters long, contain at least one special character, and be different from previously used passwords.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/.env(2 hunks)backend/src/server.js(2 hunks)backend/src/utils/constants.helper.js(1 hunks)frontend/src/scenes/login/SetNewPassword.jsx(3 hunks)frontend/src/tests/scenes/login/SetNewPassword.test.jsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/server.js
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/utils/constants.helper.js
- backend/.env
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
frontend/src/tests/scenes/login/SetNewPassword.test.jsx (1)
39-40: Yo! These test cases are making me nervous! We need more edge cases! 🍝The test suite only covers the happy path with matching passwords. We should add test cases for:
- Password mismatch
- Password without special characters
- Password less than 8 characters
Want me to generate these additional test cases for you?
frontend/src/scenes/login/SetNewPassword.jsx (1)
130-154: Password constraints looking fresh like mom's spaghetti! 👌Good use of visual feedback with CheckCircleIcon for password requirements. The implementation is clean and user-friendly.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
backend/src/test/unit/services/email.test.js (1)
Line range hint
1-98: Yo! We're missing tests for the new features, dawg! 🍝The PR introduces new functionality that needs test coverage:
- The
sendInviteEmailfunction- The logic for destroying existing reset tokens
Let's make sure we don't let these slip through untested!
Here's a template to get you started:
it("sendInviteEmail - should send invitation email with correct content", async () => { process.env.EMAIL_ENABLE = "true"; await readFile.resolves("html"); await service.sendInviteEmail(user().build().email, user().build().name); expect(readFile.called).to.be.true; const params = transporterMock.getCall(0).args[0]; expect(transporterMock.called).to.be.true; expect(params).to.deep.equal({ from: process.env.EMAIL, to: user().build().email, subject: "You're invited to join Guidefox", html: "html", }); }); it("should destroy existing reset tokens before generating new one", async () => { // Test implementation here });frontend/src/tests/components/customLinkComponent/CustomLink.test.jsx (1)
Line range hint
1-39: Knees weak, arms are heavy - we're missing some tests!The test suite needs additional coverage for the new
onClickfunctionality. Consider adding these test cases:
- Verify onClick handler is called when clicked
- Verify event prevention works correctly
- Verify behavior when onClick is not provided
Here's a suggested test case to add:
it('handles click events correctly', () => { const mockOnClick = vi.fn(); render(<CustomLink text="Click Me" onClick={mockOnClick} />); const linkElement = screen.getByText(/Click Me/i); linkElement.click(); expect(mockOnClick).toHaveBeenCalledTimes(1); });
🧹 Nitpick comments (5)
backend/src/test/unit/services/email.test.js (1)
Line range hint
14-98: Let's organize these tests like mom's recipe book! 🍝The test structure is solid but could be more organized. Consider grouping related tests using nested
describeblocks:describe("Test email service", () => { // Setup and teardown... describe("User lookup", () => { it("findUserByEmail - should return the user if it is found"); it("findUserByEmail - should return null if it isn't found"); }); describe("Signup emails", () => { it("sendSignupEmail - if email is not enabled, should return undefined"); it("sendSignupEmail - should send the email with the sing up content"); }); describe("Password reset", () => { it("should destroy existing reset tokens before generating new one"); it("sendPasswordResetEmail - if email is not enabled, should return undefined"); it("sendPasswordResetEmail - should send the email with the reset password content"); }); describe("User invitations", () => { it("sendInviteEmail - should send invitation email with correct content"); }); });frontend/src/scenes/login/ForgotPasswordPage.jsx (1)
36-36: Mom's spaghetti approves this change! 🍝Removing the unused response variable makes the code cleaner. The error handling is solid, but we could make it even better.
Consider enhancing the error message display with a more user-friendly fallback:
- setServerErrors(error.response?.data?.error); + setServerErrors(error.response?.data?.error || "Unable to process your request. Please try again later.");frontend/src/components/CustomLink/CustomLink.jsx (2)
8-8: Yo dawg, that empty URL default's making me nervousAn empty URL might cause accessibility issues. Consider using '#' or 'javascript:void(0)' as the default to maintain better compatibility with screen readers.
- url = '', + url = 'javascript:void(0)',
13-18: Mom's spaghetti moment with that event handlingThe event handling could be more explicit about propagation. Consider adding
event.stopPropagation()to prevent any unexpected event bubbling.const handleClick = (event) => { if (onClick) { event.preventDefault(); + event.stopPropagation(); onClick(event); } };frontend/src/scenes/login/CheckYourEmailPage.jsx (1)
36-36: Button's heavy, UI's weakThe resend link should be disabled during the API call to prevent multiple submissions.
- Didn't receive the email? <CustomLink text="Click to resend" onClick={handleResendClick} /> + Didn't receive the email? <CustomLink + text={isResending ? "Sending..." : "Click to resend"} + onClick={!isResending ? handleResendClick : undefined} + />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/src/test/unit/services/email.test.js(1 hunks)docker-compose.yml(1 hunks)frontend/src/assets/theme.jsx(2 hunks)frontend/src/components/CustomLink/CustomLink.jsx(2 hunks)frontend/src/scenes/login/CheckYourEmailPage.jsx(1 hunks)frontend/src/scenes/login/ForgotPasswordPage.jsx(2 hunks)frontend/src/scenes/login/SetNewPassword.jsx(3 hunks)frontend/src/tests/components/customLinkComponent/CustomLink.test.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/scenes/login/SetNewPassword.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (3)
frontend/src/assets/theme.jsx (2)
144-146: Mom's spaghetti... I mean, nice consistency! 🍝The same box-sizing rule is properly applied to the dark theme, maintaining styling consistency across themes.
61-63: Yo, this box-sizing change is straight fire! 🔥Adding
box-sizing: border-boxto input elements is a solid move for consistent sizing calculations. Just make sure existing layouts aren't relying on the old content-box behavior.Let's check if any components might be affected:
✅ Verification successful
Yo dawg, this box-sizing change is clean as mom's spaghetti! 🍝
The test file shows we're already handling input heights explicitly through props, and there's no sketchy width calculations that could break. This change is straight fire and won't cause any layout drama.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for width/height calculations on MuiOutlinedInput rg -l "MuiOutlinedInput.*width|MuiOutlinedInput.*height" frontend/src/Length of output: 146
Script:
#!/bin/bash # Check the content of the test file cat frontend/src/tests/components/textFieldComponent/CustomTextField.test.jsxLength of output: 3106
frontend/src/scenes/login/ForgotPasswordPage.jsx (1)
4-4: Yo, let's verify this service implementation!The import looks clean now, but we should make sure the service is rock-solid. Let's verify the implementation of
forgotPasswordin the login services.✅ Verification successful
Yo dawg, this service is straight fire! 🔥
The
forgotPasswordservice inloginServices.jsis implemented properly with all the right moves - async/await, error handling, and clean data flow. The import's legit, no need to sweat it!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the forgotPassword implementation rg -t js -t jsx "export.*forgotPassword.*=" src/services/Length of output: 87
Script:
#!/bin/bash # Try multiple approaches to find the forgotPassword implementation echo "=== Searching with ast-grep ===" ast-grep --pattern 'export const forgotPassword = $_' echo -e "\n=== Searching with basic grep ===" find src/services -type f -name "*.js*" -exec grep -l "export.*forgotPassword" {} \; echo -e "\n=== Showing implementation if found ===" find src/services -type f -name "*.js*" -exec grep -A 5 "export.*forgotPassword" {} \;Length of output: 1201
| subject: "Reset your password for Guidefox", | ||
| html: "html", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mom's spaghetti moment: Let's beef up this reset password test! 🍝
The test verifies the basic email parameters but misses some crucial checks:
- The reset link construction using
FRONTEND_URL - The template variables passed to handlebars
Here's how we can enhance it:
it("sendPasswordResetEmail - should send the email with the reset password content", async () => {
process.env.EMAIL_ENABLE = "true";
process.env.FRONTEND_URL = "https://guidefox.com";
const token = "test-token";
const compileMock = sinon.stub().returns("compiled-html");
handlebars.compile.returns(compileMock);
await service.sendPasswordResetEmail(
user().build().email,
user().build().name,
token
);
expect(compileMock.calledWith({
name: user().build().name,
resetLink: `${process.env.FRONTEND_URL}/reset-password?token=${token}`
})).to.be.true;
const params = transporterMock.getCall(0).args[0];
expect(params).to.deep.equal({
from: process.env.EMAIL,
to: user().build().email,
subject: "Reset your password for Guidefox",
html: "compiled-html",
});
});| const handleResendClick = () => { | ||
| if (emailFromState) { | ||
| forgotPassword(values) | ||
| .then(response => { | ||
| console.log("Password reset link resent successfully:", response); | ||
| }) | ||
| .catch(error => { | ||
| console.error("Error resending password reset link:", error); | ||
| }); | ||
| } else { | ||
| console.warn("No email provided to resend the password reset link."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Lose yourself in better error handling
The current implementation has several issues:
- Console logs shouldn't be in production code
- No user feedback for success/error states
- Missing loading state during API call
Consider using a toast notification system and adding loading state:
const [isResending, setIsResending] = useState(false);
const handleResendClick = async () => {
if (!emailFromState) {
toast.warn("No email provided");
return;
}
setIsResending(true);
try {
await forgotPassword(values);
toast.success("Password reset link sent successfully");
} catch (error) {
toast.error("Failed to send reset link. Please try again.");
} finally {
setIsResending(false);
}
};
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/scenes/login/CheckYourEmailPage.jsx (1)
14-26: 🛠️ Refactor suggestionMom's spaghetti moment - we need better error handling!
The current implementation still relies on console logs and lacks proper user feedback.
Here's an updated version with toast notifications and loading state:
+ import { toast } from 'react-toastify'; + const [isResending, setIsResending] = useState(false); - const handleResendClick = () => { + const handleResendClick = async () => { if (emailFromState) { + setIsResending(true); try { - forgotPassword(values) - .then(response => { - console.log("Password reset link resent successfully:", response); - }) - .catch(error => { - console.error("Error resending password reset link:", error); - }); + await forgotPassword(values); + toast.success("Password reset link sent successfully!"); + } catch (error) { + toast.error("Failed to send reset link. Please try again."); } else { - console.warn("No email provided to resend the password reset link."); + toast.warn("No email provided to resend the password reset link."); + } finally { + setIsResending(false); } } };frontend/src/scenes/login/SetNewPassword.jsx (1)
20-24: 🛠️ Refactor suggestionKnees weak, arms are heavy! We need better error handling! 🎯
The token validation silently redirects users without explaining why. Let's make it more user-friendly.
useEffect(() => { if (!token) { - navigate("/login"); + navigate("/login", { + state: { error: "Invalid or expired password reset link. Please request a new one." } + }); } }, [token]);
🧹 Nitpick comments (2)
frontend/src/scenes/login/CheckYourEmailPage.jsx (1)
31-45: Clean up these sweaty styles, yo!The inline styles could be moved to the CSS module for better maintainability.
Consider moving these styles to your CSS module:
// Login.module.css + .heading-no-margin { + margin: 0px; + } + .subheading { + margin-top: 0.25rem; + font-weight: bold; + margin-bottom: 1rem; + } + .back-button { + margin-top: 20px; + } + .back-icon { + font-size: 18px; + margin-right: 5px; + } // JSX - <h3 style={{ margin: "0px" }}> + <h3 className={styles["heading-no-margin"]}> - <h3 style={{ marginTop: "0.25rem", fontWeight: "bold", marginBottom: "1rem" }}> + <h3 className={styles["subheading"]}> - style={{ marginTop: "20px" }} + className={styles["back-button"]} - style={{ fontSize: "18px", marginRight: "5px" }} + className={styles["back-icon"]}frontend/src/scenes/login/SetNewPassword.jsx (1)
Line range hint
48-67: There's vomit on his sweater already! Let's clean up this error handling! 🧹While the error handling is comprehensive, we've got some cleanup to do:
- Console.error shouldn't be in production code
- Error messages could be more specific for different failure scenarios
} catch (error) { - console.error("Password Reset failed:", error); if (error.response?.data?.errors) { setServerErrors(error.response?.data?.errors); } else if (error.response?.data?.error) { setServerErrors([error.response.data.error]); } else { - setServerErrors(["An error occurred. Please try again."]); + setServerErrors([ + "Unable to reset password. Please ensure your reset link hasn't expired." + ]); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/scenes/login/CheckYourEmailPage.jsx(1 hunks)frontend/src/scenes/login/SetNewPassword.jsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (3)
frontend/src/scenes/login/CheckYourEmailPage.jsx (1)
6-6: Yo, the null checks are on point now!Previous concerns about null safety have been addressed with proper null checks on
location.state. Keep up the good work!Also applies to: 11-12
frontend/src/scenes/login/SetNewPassword.jsx (2)
1-12: Yo, these imports are clean like mom's spaghetti! 🍝The addition of
useLocationand removal of the email prop align perfectly with the token-based approach.
79-182: This UI is fresher than mom's spaghetti! 🔥The form implementation is solid with:
- Clear password requirements ✅
- Visual feedback with CheckCircleIcon ✅
- Proper validation and error handling ✅
- Accessible error messages ✅
Issue number
467