-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/recovery email #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Check the documentation preview: https://6807bab7ab7865223fe5bdfa--niaefeup-backend-docs.netlify.app |
|
Check the documentation preview: https://6807de46053c504e8c4ad324--niaefeup-backend-docs.netlify.app |
|
Check the documentation preview: https://685edb0395873da364f3ba65--niaefeup-backend-docs.netlify.app |
…niaefeup-backend into feature/recovery-email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR integrates the email service into the password recovery flow by adding Mustache templates, CSS styling, and wiring a RecoveryEmailBuilder into the AuthController.
- Introduces
recover-account.mustacheand a shared HTML layout with inlined styles. - Adds
TemplateEmailBuilder,RecoveryEmailBuilder, and CSS for email formatting. - Updates
AuthControllerto send recovery emails and adapts tests.
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/kotlin/.../AuthControllerTest.kt | Adjusted test imports for JUnit, removed unused import |
| src/main/resources/templates/email/recover-account.mustache | New Mustache template for recovery emails |
| src/main/resources/templates/email/layout.html.mustache | Updated HTML layout, inlines styles and adds header/footer |
| src/main/resources/email/style.css | Added CSS for email styling |
| src/main/resources/application.properties | Added blank line (minor) and email.from properties |
| src/main/kotlin/.../TemplateEmailBuilder.kt | Injected baseServeUrl, templating logic |
| src/main/kotlin/.../RecoveryEmailBuilder.kt | New builder for recovery email context |
| src/main/kotlin/.../AuthController.kt | Injected EmailService, sends recovery email |
Comments suppressed due to low confidence (2)
src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt:64
- This response format differs from the prior
recovery_urlJSON—this is a breaking change for API clients. Consider updating clients or restoring the original field name.
return mapOf("message" to "Sent email to account recovery")
| import java.util.* | ||
| import java.util.Calendar | ||
| import org.hamcrest.Matchers.startsWith | ||
| import org.junit.Test |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing JUnit4’s @Test import with JUnit Jupiter in the same file may cause confusion—use org.junit.jupiter.api.Test consistently.
| import org.junit.Test | |
| import org.junit.jupiter.api.Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree unless this is intentional
| <img src="cid:image" alt="niaefeup-logo" width="100"/> | ||
| <h2>{{{ subject }}}</h2> | ||
| </div> | ||
| {{{ content }}} |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the {{{ content }}} placeholder in a <div class="content"> to match the CSS selector .content and ensure consistent styling.
| {{{ content }}} | |
| <div class="content"> | |
| {{{ content }}} | |
| </div> |
| private val commonmarkTextRenderer = ApplicationContextUtils.getBean(TextContentRenderer::class.java) | ||
| private val mustache = ApplicationContextUtils.getBean(Mustache.Compiler::class.java) | ||
|
|
||
| @field:Value("\${upload.static-serve}") |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The injected baseServeUrl field is not used in this class—consider removing it or using it for templating image URLs.
| /* | ||
| TODO | ||
| */ | ||
| body { |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The email CSS file is added but never referenced in the templates or passed to the builder’s styles front matter—ensure it’s loaded and inlined.
| "$baseServeUrl/niaefeup.png" | ||
| ) | ||
| ) | ||
| .to(recoveryRequestDto.email) // change to dev's email for testing |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inline comment indicates a temporary testing change—remove or replace it before merging to avoid sending test emails in production.
| .to(recoveryRequestDto.email) // change to dev's email for testing | |
| .to(if (isTestingEnabled) developerEmail else recoveryRequestDto.email) |
|
Check the documentation preview: https://686427162de5e5f922f67d2e--niaefeup-backend-docs.netlify.app |
MRita443
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job here! Left mostly some questions.
Also is there not a way in the tests to confirm an email was sent? Maybe by mocking some of the email functions? But if you think that's pedantic lmk!
|
|
||
| # Email config | ||
| spring.mail.host= | ||
| spring.mail.host= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| spring.mail.host= | |
| spring.mail.host= |
| import java.util.* | ||
| import java.util.Calendar | ||
| import org.hamcrest.Matchers.startsWith | ||
| import org.junit.Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree unless this is intentional
| @field:Value("\${upload.static-serve}") | ||
| private var baseServeUrl: String = "" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed here?
| exclude(group = "org.mockito2", module = "mockito-core") | ||
| } | ||
| testImplementation("org.mockito:mockito-inline:5.2.0") | ||
| testImplementation(kotlin("test")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can safely remove this
| .expiry { | ||
| font-size: 13px; | ||
| color: #777; | ||
| margin-top: 30px; | ||
| } | ||
| .help { | ||
| margin-top: 30px; | ||
| font-size: 14px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these and others not currently used in the recovery template working well and may be used in the future or is it just leftover code?
| ) | ||
| .to(recoveryRequestDto.email) // change to dev's email for testing | ||
| ) | ||
| return mapOf("message" to "Sent email to account recovery") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return mapOf("message" to "Sent email to account recovery") | |
| return mapOf("message" to "Sent email for account recovery") |
|
Check the documentation preview: https://687e519f76a8848f7127441b--niaefeup-backend-docs.netlify.app |
Closes #171
Closes #140
Integrates the email service with the recover password functionality.
The email message looks like this:
Review checklist