248 notify user by email #258
Conversation
WalkthroughThis pull request introduces significant updates, including the removal of the Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
github-actions
bot
added
application-server
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
server/application-server/src/main/resources/mail-templates/bad-practices-detected.html (1)
1-5: Enhance the email template with more detailed information.While the template includes the basic information, it could be more helpful by including:
- A link to the specific pull request
- More details about the detected bad practice
- Suggestions for fixing the issue
- A call-to-action button/link
Apply this diff to enhance the template:
<p th:inline="text"> Hi [[${user.name}]], <br> - We have detected the bad practice [[${badPractice}]] in your pull request. Please review the issue on Hephaestus. + We have detected a bad practice in your pull request #[[${pullRequest.number}]]: + <br><br> + <strong>[[${badPractice}]]</strong> + <br><br> + To help you address this issue: + <br> + [[${suggestion}]] + <br><br> + <a href="[[${pullRequestUrl}]]" style="background-color: #4CAF50; color: white; padding: 10px 20px; text-decoration: none; border-radius: 5px;">Review on Hephaestus</a> </p>server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java (1)
102-104: Add documentation for the notifications feature.While the implementation is correct, consider adding Javadoc to document the purpose and usage of this feature.
Apply this diff to add documentation:
@NonNull + /** + * Indicates whether the user has opted in to receive email notifications. + * This setting affects various notification types including bad practice alerts. + */ private boolean notificationsEnabled = false;server/application-server/compose.yaml (1)
35-44: Consider improving service reliability.A few suggestions to enhance the postfix service configuration:
- Use a specific version tag instead of
latestto ensure reproducible builds- Add a health check to ensure the service is running correctly
Apply this diff to implement the suggestions:
postfix: - image: ghcr.io/ls1admin/postfix:latest + image: ghcr.io/ls1admin/postfix:1.0.0 # Replace with actual version container_name: hephaestus-postfix restart: unless-stopped volumes: - ./postfix-config:/config hostname: hephaestus.aet.cit.tum.de + healthcheck: + test: ["CMD", "postfix", "status"] + interval: 30s + timeout: 10s + retries: 3 networks: - app-network🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
server/application-server/src/main/resources/application.yml (1)
36-47: Consider adding additional mail security configurations.While the basic SMTP configuration is correct, consider adding these security enhancements:
- Enable SSL/TLS by default
- Configure connection timeout
- Set authentication settings explicitly
mail: host: ${POSTFIX_HOST:localhost} port: ${POSTFIX_PORT:25} username: ${POSTFIX_USERNAME:} password: ${POSTFIX_PASSWORD:} properties: mail: transport: protocol: smtp smtp: starttls: enable: true + auth: true + connectiontimeout: 5000 + timeout: 5000 + writetimeout: 5000 + ssl: + enable: trueserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java (2)
91-94: Improve logging and add null check.The method should use parameterized logging and validate the user parameter.
public UserSettingsDTO getUserSettings(User user) { - logger.info("Getting user settings with userId: " + user); + if (user == null) { + throw new IllegalArgumentException("User cannot be null"); + } + logger.info("Getting user settings for user: {}", user.getLogin()); return new UserSettingsDTO(user.isNotificationsEnabled()); }
96-101: Add validation and improve logging.The method should validate both parameters and use parameterized logging.
public UserSettingsDTO updateUserSettings(User user, UserSettingsDTO userSettings) { - logger.info("Updating user settings with userId: " + user); + if (user == null || userSettings == null) { + throw new IllegalArgumentException("User and userSettings cannot be null"); + } + logger.info("Updating notification settings to {} for user: {}", + userSettings.receiveNotifications(), user.getLogin()); user.setNotificationsEnabled(userSettings.receiveNotifications()); userRepository.save(user); + logger.debug("Successfully updated notification settings for user: {}", user.getLogin()); return new UserSettingsDTO(user.isNotificationsEnabled()); }server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java (1)
16-16: Consider makingenabledfield final and adding @Getter.The
enabledfield is only accessed through theisEnabled()method. For consistency with other fields and to follow the class's immutable design pattern:- private final Boolean enabled; + @Getter + private final Boolean enabled; - public boolean isEnabled() { - return enabled; - }Also applies to: 40-42
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java (1)
110-111: Consider using a consistent method for setting the sender address.The code uses both
setFromandsetSenderwhich might be redundant:- message.setFrom("Hephaestus <" + config.getSender().getAddress() + ">"); - message.setSender(config.getSender()); + message.setFrom(config.getSender());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
server/application-server/compose.yaml(1 hunks)server/application-server/pom.xml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java(1 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml(1 hunks)server/application-server/src/main/resources/db/master.xml(1 hunks)server/application-server/src/main/resources/mail-templates/bad-practices-detected.html(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java
- server/application-server/src/main/resources/db/master.xml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
server/application-server/compose.yaml
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Mend Security Check
🔇 Additional comments (5)
server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml (1)
1-10: LGTM! The changelog is well-structured.The Liquibase changelog correctly defines the new
notifications_enabledcolumn with appropriate constraints and default value.server/application-server/pom.xml (1)
95-102: LGTM!The required dependencies for email functionality and template rendering have been correctly added.
server/application-server/src/main/resources/application.yml (2)
32-34: LGTM!The Thymeleaf configuration is correctly set up with a configurable template folder and HTML suffix.
71-74: LGTM!The mail settings under the hephaestus section are correctly configured with environment variables.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java (1)
13-26: LGTM!The service class is correctly structured with proper dependency injection and logging setup.
| public void sendBadPracticesDetectedEmail(User user, String badPractice) { | ||
| logger.info("Sending bad practice detected email to user: " + user.getLogin()); | ||
| if (!user.getLogin().equals("iam-flo")) | ||
| return; | ||
|
|
||
| MailBuilder mailBuilder = new MailBuilder(mailConfig, "Bad Practices Detected", "bad-practices-detected"); | ||
| mailBuilder | ||
| .addPrimaryRecipient(user) | ||
| .fillUserPlaceholders(user, "user") | ||
| .fillBadPracticePlaceholders(badPractice, "badPractice") | ||
| .send(javaMailSender); | ||
| } |
There was a problem hiding this comment.
Remove hardcoded user check and improve error handling.
The method has several issues:
- The hardcoded user check
user.getLogin().equals("iam-flo")should be removed. - Missing null checks for user and badPractice parameters.
- No error handling for mail sending failures.
Apply this diff to fix the issues:
public void sendBadPracticesDetectedEmail(User user, String badPractice) {
+ if (user == null || badPractice == null) {
+ logger.error("Cannot send email: user or badPractice is null");
+ return;
+ }
+
logger.info("Sending bad practice detected email to user: {}", user.getLogin());
- if (!user.getLogin().equals("iam-flo"))
+ if (!user.isNotificationsEnabled()) {
+ logger.debug("Notifications disabled for user: {}", user.getLogin());
return;
+ }
try {
MailBuilder mailBuilder = new MailBuilder(mailConfig, "Bad Practices Detected", "bad-practices-detected");
mailBuilder
.addPrimaryRecipient(user)
.fillUserPlaceholders(user, "user")
.fillBadPracticePlaceholders(badPractice, "badPractice")
.send(javaMailSender);
+ logger.debug("Email sent successfully to user: {}", user.getLogin());
+ } catch (Exception e) {
+ logger.error("Failed to send email to user: {}", user.getLogin(), e);
+ }
}📝 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.
| public void sendBadPracticesDetectedEmail(User user, String badPractice) { | |
| logger.info("Sending bad practice detected email to user: " + user.getLogin()); | |
| if (!user.getLogin().equals("iam-flo")) | |
| return; | |
| MailBuilder mailBuilder = new MailBuilder(mailConfig, "Bad Practices Detected", "bad-practices-detected"); | |
| mailBuilder | |
| .addPrimaryRecipient(user) | |
| .fillUserPlaceholders(user, "user") | |
| .fillBadPracticePlaceholders(badPractice, "badPractice") | |
| .send(javaMailSender); | |
| } | |
| public void sendBadPracticesDetectedEmail(User user, String badPractice) { | |
| if (user == null || badPractice == null) { | |
| logger.error("Cannot send email: user or badPractice is null"); | |
| return; | |
| } | |
| logger.info("Sending bad practice detected email to user: {}", user.getLogin()); | |
| if (!user.isNotificationsEnabled()) { | |
| logger.debug("Notifications disabled for user: {}", user.getLogin()); | |
| return; | |
| } | |
| try { | |
| MailBuilder mailBuilder = new MailBuilder(mailConfig, "Bad Practices Detected", "bad-practices-detected"); | |
| mailBuilder | |
| .addPrimaryRecipient(user) | |
| .fillUserPlaceholders(user, "user") | |
| .fillBadPracticePlaceholders(badPractice, "badPractice") | |
| .send(javaMailSender); | |
| logger.debug("Email sent successfully to user: {}", user.getLogin()); | |
| } catch (Exception e) { | |
| logger.error("Failed to send email to user: {}", user.getLogin(), e); | |
| } | |
| } |
| @Value("${hephaestus.mail.sender}") InternetAddress sender, | ||
| @Value("${hephaestus.mail.enabled}") boolean enabled, | ||
| @Value("${hephaestus.mail.signature}") String mailSignature, | ||
| TemplateEngine templateEngine |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate mail configuration parameters.
The constructor accepts critical configuration parameters but doesn't validate them. Consider adding validation to ensure the configuration is valid:
@Autowired
public MailConfig(
@Value("${hephaestus.mail.sender}") InternetAddress sender,
@Value("${hephaestus.mail.enabled}") boolean enabled,
@Value("${hephaestus.mail.signature}") String mailSignature,
TemplateEngine templateEngine
) {
+ if (sender == null) {
+ throw new IllegalArgumentException("Sender address must not be null");
+ }
+ if (templateEngine == null) {
+ throw new IllegalArgumentException("Template engine must not be null");
+ }
this.enabled = enabled;
this.sender = sender;
this.signature = mailSignature;
this.templateEngine = templateEngine;
}📝 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.
| @Value("${hephaestus.mail.sender}") InternetAddress sender, | |
| @Value("${hephaestus.mail.enabled}") boolean enabled, | |
| @Value("${hephaestus.mail.signature}") String mailSignature, | |
| TemplateEngine templateEngine | |
| @Autowired | |
| public MailConfig( | |
| @Value("${hephaestus.mail.sender}") InternetAddress sender, | |
| @Value("${hephaestus.mail.enabled}") boolean enabled, | |
| @Value("${hephaestus.mail.signature}") String mailSignature, | |
| TemplateEngine templateEngine | |
| ) { | |
| if (sender == null) { | |
| throw new IllegalArgumentException("Sender address must not be null"); | |
| } | |
| if (templateEngine == null) { | |
| throw new IllegalArgumentException("Template engine must not be null"); | |
| } | |
| this.enabled = enabled; | |
| this.sender = sender; | |
| this.signature = mailSignature; | |
| this.templateEngine = templateEngine; | |
| } |
...lication-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java
Show resolved
Hide resolved
...lication-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java
Show resolved
Hide resolved
| List<User> toRecipients = new ArrayList<>(); | ||
|
|
||
| for (User recipient : primaryRecipients) { | ||
| if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || !recipient.getEmail().contains("@")) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve email validation.
The current email validation is too basic. Consider using a more robust validation method:
- if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || !recipient.getEmail().contains("@")) {
+ if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null ||
+ !recipient.getEmail().matches("^[A-Za-z0-9+_.-]+@(.+)$")) {📝 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.
| if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || !recipient.getEmail().contains("@")) { | |
| if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || | |
| !recipient.getEmail().matches("^[A-Za-z0-9+_.-]+@(.+)$")) { |
|
|
||
| public void send(JavaMailSender mailSender) { | ||
| List<User> toRecipients = new ArrayList<>(); | ||
|
|
||
| for (User recipient : primaryRecipients) { | ||
| if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || !recipient.getEmail().contains("@")) { | ||
| continue; | ||
| } | ||
| toRecipients.add(recipient); | ||
| } | ||
|
|
||
| for (User recipient : toRecipients) { | ||
| try { | ||
| MimeMessage message = mailSender.createMimeMessage(); | ||
|
|
||
| message.setFrom("Hephaestus <" + config.getSender().getAddress() + ">"); | ||
| message.setSender(config.getSender()); | ||
|
|
||
| message.addRecipient(Message.RecipientType.TO, new InternetAddress(recipient.getEmail())); | ||
|
|
||
| Context templateContext = new Context(); | ||
| templateContext.setVariables(this.variables); | ||
| templateContext.setVariable("recipient", UserInfoDTO.fromUser(recipient)); | ||
|
|
||
| message.setSubject(subject); | ||
|
|
||
| Multipart messageContent = new MimeMultipart(); | ||
|
|
||
| BodyPart messageBody = new MimeBodyPart(); | ||
| messageBody.setContent(config.getTemplateEngine().process(template, templateContext), "text/html; charset=utf-8"); | ||
| messageContent.addBodyPart(messageBody); | ||
|
|
||
| message.setContent(messageContent); | ||
|
|
||
| if (config.isEnabled()) { | ||
| log.info("Sending Mail\n{}", messageBody.getContent()); | ||
| mailSender.send(message); | ||
| } else { | ||
| log.info("Sending Mail (postfix disabled)\n{}", messageBody.getContent()); | ||
| } | ||
| } catch (Exception exception) { | ||
| log.warn("Failed to send email", exception); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add retry mechanism.
The send method has several areas for improvement:
- Catch specific exceptions instead of generic Exception
- Add retry mechanism for transient failures
- Add metrics for monitoring
public void send(JavaMailSender mailSender) {
+ if (mailSender == null) {
+ throw new IllegalArgumentException("mailSender must not be null");
+ }
List<User> toRecipients = new ArrayList<>();
for (User recipient : primaryRecipients) {
if (!recipient.isNotificationsEnabled() || recipient.getEmail() == null || !recipient.getEmail().contains("@")) {
continue;
}
toRecipients.add(recipient);
}
for (User recipient : toRecipients) {
+ int retryCount = 0;
+ while (retryCount < 3) {
try {
MimeMessage message = mailSender.createMimeMessage();
// ... existing code ...
if (config.isEnabled()) {
log.info("Sending Mail\n{}", messageBody.getContent());
mailSender.send(message);
+ break;
} else {
log.info("Sending Mail (postfix disabled)\n{}", messageBody.getContent());
+ break;
}
- } catch (Exception exception) {
- log.warn("Failed to send email", exception);
+ } catch (MessagingException e) {
+ log.warn("Failed to send email to {}, attempt {}/3", recipient.getEmail(), retryCount + 1, e);
+ retryCount++;
+ if (retryCount == 3) {
+ log.error("Failed to send email to {} after 3 attempts", recipient.getEmail(), e);
+ }
+ Thread.sleep(1000 * retryCount);
+ } catch (Exception e) {
+ log.error("Unexpected error while sending email to {}", recipient.getEmail(), e);
+ break;
}
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
iam-flo
force-pushed
the
248-notify-user-by-email
branch
from
7d984d8 to
d57d93e
Compare
github-actions
bot
added
client
size:XL
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
server/application-server/src/main/resources/application.yml (1)
36-74:⚠️ Potential issueReview mail configuration for production security.
The mail configuration has potential security concerns:
- Port 25 (default) is unencrypted SMTP. Consider using port 587 (STARTTLS) or 465 (SMTPS) by default.
- The default sender email (
[email protected]) should not be used in production.Apply this diff to enhance security:
mail: - host: ${POSTFIX_HOST:localhost} - port: ${POSTFIX_PORT:25} + host: ${POSTFIX_HOST:localhost} + port: ${POSTFIX_PORT:587} username: ${POSTFIX_USERNAME:} password: ${POSTFIX_PASSWORD:} properties: mail: transport: protocol: smtp smtp: starttls: enable: true + required: true + auth: true hephaestus: mail: enabled: ${MAIL_ENABLED:true} - sender: ${MAIL_SENDER:[email protected]} + sender: ${MAIL_SENDER:} signature: ${MAIL_SIGNATURE:}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 62-62: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (4)
webapp/src/app/core/modules/openapi/api/user.serviceInterface.ts (1)
41-52: Add proper documentation for the new methods.The new methods lack proper documentation. Please add descriptions explaining:
- The purpose of each method
- Expected behavior
- Any side effects
- Parameter descriptions for
userSettingsApply this diff to add documentation:
/** - * - * + * Retrieves the current user's notification settings. + * + * @returns An Observable that emits the user's settings */ getUserSettings(extraHttpRequestParams?: any): Observable<UserSettings>; /** - * - * - * @param userSettings + * Updates the current user's notification settings. + * + * @param userSettings The new settings to apply + * @returns An Observable that emits the updated settings */ updateUserSettings(userSettings: UserSettings, extraHttpRequestParams?: any): Observable<UserSettings>;webapp/src/app/core/modules/openapi/api/user.service.ts (3)
23-24: Remove unnecessary@ts-ignore.If there is no actual type error here, consider removing the
@ts-ignorefor better type safety and clarity:- // @ts-ignore import { UserSettings } from '../model/user-settings';If removing
@ts-ignoreraises a type error, please clarify the underlying issue so we can address it properly.
221-278: Use optional chaining to simplify checks onoptions.Multiple lines in this block (232, 244, 249) use the
options && options.propertypattern. You can adopt optional chaining to simplify your code:let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; ... let localVarHttpContext: HttpContext | undefined = options?.context; ... let localVarTransferCache: boolean | undefined = options?.transferCache;🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 249-249: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
280-351: Adopt consistent REST practices and simplify code with optional chaining.
- Consider using a PUT or PATCH request for resource updates. This is a stylistic convention, but you might prefer consistency with standard REST patterns.
- Apply optional chaining in lines 295, 307, and 312, mirroring the refactor suggestion for
getUserSettings:let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; ... let localVarHttpContext: HttpContext | undefined = options?.context; ... let localVarTransferCache: boolean | undefined = options?.transferCache;Ensure these methods have proper test coverage (unit or integration tests) confirming that user settings are correctly retrieved and updated.
🧰 Tools
🪛 Biome (1.9.4)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 312-312: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
server/application-server/compose.yaml(1 hunks)server/application-server/openapi.yaml(2 hunks)server/application-server/pom.xml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java(1 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml(1 hunks)server/application-server/src/main/resources/db/master.xml(1 hunks)server/application-server/src/main/resources/mail-templates/bad-practices-detected.html(1 hunks)webapp/src/app/core/modules/openapi/.openapi-generator/FILES(1 hunks)webapp/src/app/core/modules/openapi/api/user.service.ts(2 hunks)webapp/src/app/core/modules/openapi/api/user.serviceInterface.ts(2 hunks)webapp/src/app/core/modules/openapi/model/models.ts(1 hunks)webapp/src/app/core/modules/openapi/model/user-settings.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- webapp/src/app/core/modules/openapi/model/user-settings.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java
- server/application-server/src/main/resources/db/master.xml
- server/application-server/src/main/resources/mail-templates/bad-practices-detected.html
- server/application-server/pom.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java
- server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
🧰 Additional context used
🪛 Biome (1.9.4)
webapp/src/app/core/modules/openapi/api/user.service.ts
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 249-249: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 312-312: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 YAMLlint (1.35.1)
server/application-server/compose.yaml
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
🔇 Additional comments (5)
webapp/src/app/core/modules/openapi/model/models.ts (1)
13-13: LGTM!The new export statement for
user-settingsis correctly placed and aligns with the module's export pattern.webapp/src/app/core/modules/openapi/.openapi-generator/FILES (1)
37-37: LGTM!The new entry for
user-settings.tsis correctly placed and follows the established pattern for model files.server/application-server/compose.yaml (2)
35-41: LGTM! Please verify the hostname configuration.The postfix service configuration looks good, but please verify that the hostname
hephaestus.aet.cit.tum.deis the correct domain for your environment.
42-43: Fix indentation in networks section.The indentation is incorrect (8 spaces instead of 6).
Apply this diff to fix the indentation:
networks: - - app-network + - app-network🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
server/application-server/src/main/resources/application.yml (1)
32-34: LGTM!The Thymeleaf configuration is well-structured, allowing for customization of the template folder via environment variables while providing sensible defaults.
...application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java
Show resolved
Hide resolved
...application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
webapp/src/app/settings/settings.component.html (3)
1-13: The notification settings UI looks good, consider adding feedback after save.The notification settings section is well structured with clear labeling and a logical layout. The toggle switch for email notifications is properly bound to the form control.
Consider adding a success message or visual feedback after the settings are saved successfully to improve user experience.
12-12: Add loading state to the save button.The save button doesn't appear to have a loading state while the settings are being saved. This could lead to user confusion if the save operation takes time.
Consider adding a loading state to the button:
- <button hlmBtn variant="outline" class="w-full" (click)="saveSettings()">Save settings</button> + <button hlmBtn variant="outline" class="w-full" (click)="saveSettings()" [disabled]="settingsMutation.isPending"> + {{ settingsMutation.isPending ? 'Saving...' : 'Save settings' }} + </button>
15-15: Incorrect id attribute for delete account button.The delete account button has an id "edit-profile" which doesn't match its function.
- <button id="edit-profile" variant="outline" brnAlertDialogTrigger hlmBtn>Delete Account</button> + <button id="delete-account" variant="outline" brnAlertDialogTrigger hlmBtn>Delete Account</button>webapp/src/app/settings/settings.component.ts (2)
47-47: Consider adding a default value comment for form control.The form control initializes with a default value of
falsebut it's not clear if this is intentional or just a placeholder until the real value is loaded.- _newNotificationEnabled = new FormControl(false); + // Default to disabled until user settings are loaded + _newNotificationEnabled = new FormControl(false);
70-72: Add feedback handling in the saveSettings method.The current implementation doesn't provide any user feedback after saving settings.
Consider modifying the saveSettings method to handle success and error states:
saveSettings() { - this.settingsMutation.mutate({ receiveNotifications: this._newNotificationEnabled.value ?? false }); + this.settingsMutation.mutate( + { receiveNotifications: this._newNotificationEnabled.value ?? false }, + { + onSuccess: () => { + // Show success notification to user + console.log('Settings saved successfully'); + }, + onError: (error) => { + // Show error message to user + console.error('Failed to save settings:', error); + } + } + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webapp/src/app/settings/settings.component.html(1 hunks)webapp/src/app/settings/settings.component.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Run Chromatic
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
webapp/src/app/settings/settings.component.html (1)
14-26: Account deletion dialog implementation looks good.The account deletion confirmation dialog follows best practices:
- Clear warning about the permanent nature of the action
- Explicit confirmation required
- Easy way to cancel the action
- Clear labeling of actions
webapp/src/app/settings/settings.component.ts (1)
43-73: Initialize settings when component loads.The component doesn't explicitly trigger the loading of settings when it initializes. While injectQuery may handle this automatically, it would be clearer to explicitly trigger or ensure the query runs on initialization.
It appears the component relies on the automatic execution of the query. Please verify that settings are being loaded correctly when the component is initialized. If a more explicit approach is preferred, consider adding an ngOnInit lifecycle hook that triggers the query.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/application-server/src/main/resources/application-prod.yml (2)
16-28: New SMTP Mail Configuration in the Spring SectionThe new mail configuration under
springis clear and well-organized. It correctly defines properties such ashost,port,username,password, and includes the necessary nested properties for SMTP transport and STARTTLS.Suggestions:
- Ensure that the environment variables (
POSTFIX_HOST,POSTFIX_PORT,POSTFIX_USERNAME, andPOSTFIX_PASSWORD) are properly secured and set in production.- Consider adding inline comments to describe each setting briefly for easier maintenance and clarity in the future.
46-50: New Hephaestus Mail Configuration SectionThe addition of the
hephaestusprovides a neat way to control mail functionality specific to Hephaestus. Using environment variables to manageenabled,sender, andsignatureis a good approach.Suggestions:
- The default empty values for
senderandsignaturemay lead to unintended behavior if not explicitly configured. Consider either providing sensible defaults or adding clear documentation/comments indicating that these values must be set in a production environment when mail is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/application-server/compose.yaml(1 hunks)server/application-server/src/main/resources/application-prod.yml(2 hunks)server/application-server/src/main/resources/application.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/application-server/src/main/resources/application.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
server/application-server/compose.yaml
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
🔇 Additional comments (2)
server/application-server/compose.yaml (2)
35-44: New Postfix Service AdditionThe addition of the
postfixservice is well integrated. This service is configured with an appropriate image, container name, restart policy, volume binding, and hostname substitution, which aligns with the email notification functionality. Please ensure that the./postfix-configdirectory contains the required configuration files for Postfix.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
43-43: Fix Indentation in Networks SectionYAMLlint reports that the list item under the
networkskey is indented with 8 spaces instead of the expected 6. This indentation error can lead to parsing issues.Apply the following diff to correct the indentation:
- - app-network + - app-network🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 43-43: wrong indentation: expected 6 but found 8
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/application-server/src/main/resources/application-prod.yml (1)
46-50: Review of Hephaestus Mail ConfigurationThe introduction of the
hephaestussection clearly enables configurable email functionality within the application. The default sender email is provided, and the feature is gated by theMAIL_ENABLEDvariable. Consider adding inline comments to document the purpose of each parameter (especially the empty default forMAIL_SIGNATURE) to improve clarity and maintainability for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/application-server/compose.yaml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/resources/application-prod.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/application-server/compose.yaml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
🔇 Additional comments (1)
server/application-server/src/main/resources/application-prod.yml (1)
16-28: Review of Spring Mail ConfigurationThe new
springsection is well structured and leverages environment variables with sensible defaults. Please verify that the empty defaults forPOSTFIX_USERNAMEandPOSTFIX_PASSWORDare acceptable for your SMTP server—especially in cases where authentication might be required. If authentication is needed, you may need to enforce stricter defaults or additional validations.
iam-flo
force-pushed
the
248-notify-user-by-email
branch
from
d2e3720 to
058956b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
server/application-server/src/main/resources/application.yml (2)
36-49: Mail Settings ConfigurationThis block introduces comprehensive SMTP settings for Gmail. The configuration is generally correct. A few points to consider:
- Debug Mode: The
debug: truesetting is useful for development, but ensure that it’s disabled in production environments (e.g., via profile-specific configurations).- Credential Management: The use of
${POSTFIX_USERNAME:}and${POSTFIX_PASSWORD:}is appropriate for injecting values securely. Make sure these environment variables are properly managed in production.
76-79: Application-Level Mail SettingsThe new
hephaestusintroduces application-specific mail settings, such asenabled,sender, andsignature. This is a good separation from the SMTP configurations underspring.mail. Consider:
- Validation: If notifications are enabled, ensure that
senderis not left empty—either handle it in the application logic or provide a sensible default.- Documentation: Clearly document the distinction between
spring.mail(for SMTP connection details) andhephaestus.mail(for application email behaviors) to aid future maintenance.webapp/src/app/core/modules/openapi/api/user.serviceInterface.ts (1)
41-53: Add documentation for the new settings methods.The new methods lack proper JSDoc descriptions to explain their purpose and parameters. This makes it harder for developers to understand their usage.
Improve the JSDoc documentation:
/** - * - * + * Retrieves the current user's notification settings + * + * @returns Observable containing the user's settings */ getUserSettings(extraHttpRequestParams?: any): Observable<UserSettings>; /** - * - * + * Updates the current user's notification settings + * + * @param userSettings The settings to be updated + * @returns Observable containing the updated user settings * @param userSettings */ updateUserSettings(userSettings: UserSettings, extraHttpRequestParams?: any): Observable<UserSettings>;webapp/src/app/settings/settings.component.ts (2)
47-47: Consider using a more descriptive name for the form control.The underscore prefix typically indicates a private property, but the naming doesn't clearly communicate what this control represents.
- _newNotificationEnabled = new FormControl(false); + notificationEnabledControl = new FormControl(false);
70-72: Add feedback to user after saving settings.The
saveSettingsmethod should provide feedback to the user after successful update or on error.saveSettings() { - this.settingsMutation.mutate({ receiveNotifications: this._newNotificationEnabled.value ?? false }); + this.settingsMutation.mutate( + { receiveNotifications: this._newNotificationEnabled.value ?? false }, + { + onSuccess: () => { + // Show success message to user + console.log('Settings saved successfully'); + // You could use a toast notification service here + }, + onError: (error) => { + console.error('Failed to save settings:', error); + // Show error message to user + } + } + ); }webapp/src/app/core/modules/openapi/api/user.service.ts (4)
23-24: Import looks good but consider removing the @ts-ignore comment.The import of the
UserSettingsmodel is necessary for the new functionality, but the@ts-ignorecomment suggests there might be TypeScript errors being suppressed. Consider addressing the underlying issue instead of ignoring it.-// @ts-ignore +// Import the UserSettings model for user notification preferences import { UserSettings } from '../model/user-settings';
221-278: Implement optional chaining for cleaner code in getUserSettings method.The method looks functionally correct, but you can modernize the property access pattern using optional chaining.
- let localVarHttpHeaderAcceptSelected: string | undefined = options && options.httpHeaderAccept; + let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; - let localVarHttpContext: HttpContext | undefined = options && options.context; + let localVarHttpContext: HttpContext | undefined = options?.context; - let localVarTransferCache: boolean | undefined = options && options.transferCache; + let localVarTransferCache: boolean | undefined = options?.transferCache;🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 249-249: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
280-351: Implement optional chaining for cleaner code in updateUserSettings method.The method correctly handles input validation and properly implements the update functionality, but the same modernization opportunity exists here as well.
- let localVarHttpHeaderAcceptSelected: string | undefined = options && options.httpHeaderAccept; + let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; - let localVarHttpContext: HttpContext | undefined = options && options.context; + let localVarHttpContext: HttpContext | undefined = options?.context; - let localVarTransferCache: boolean | undefined = options && options.transferCache; + let localVarTransferCache: boolean | undefined = options?.transferCache;🧰 Tools
🪛 Biome (1.9.4)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 312-312: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
289-291: Consider adding type validation beyond null/undefined checks.While checking for null/undefined is good, consider adding additional validation for the
userSettingsobject to ensure it contains the expected properties before sending to the server.if (userSettings === null || userSettings === undefined) { throw new Error('Required parameter userSettings was null or undefined when calling updateUserSettings.'); } +// Validate that userSettings has the expected properties +if (userSettings.notificationsEnabled === undefined) { + throw new Error('Required property notificationsEnabled was missing in userSettings parameter.'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
server/application-server/compose.yaml(1 hunks)server/application-server/openapi.yaml(2 hunks)server/application-server/pom.xml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java(1 hunks)server/application-server/src/main/resources/application-prod.yml(2 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml(1 hunks)server/application-server/src/main/resources/db/master.xml(1 hunks)server/application-server/src/main/resources/mail-templates/bad-practices-detected.html(1 hunks)webapp/src/app/core/modules/openapi/.openapi-generator/FILES(1 hunks)webapp/src/app/core/modules/openapi/api/user.service.ts(2 hunks)webapp/src/app/core/modules/openapi/api/user.serviceInterface.ts(2 hunks)webapp/src/app/core/modules/openapi/model/models.ts(1 hunks)webapp/src/app/core/modules/openapi/model/user-settings.ts(1 hunks)webapp/src/app/settings/settings.component.html(1 hunks)webapp/src/app/settings/settings.component.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserSettingsDTO.java
- webapp/src/app/core/modules/openapi/model/user-settings.ts
- server/application-server/src/main/resources/db/master.xml
- server/application-server/src/main/resources/mail-templates/bad-practices-detected.html
- webapp/src/app/core/modules/openapi/model/models.ts
- server/application-server/src/main/resources/db/changelog/1740410752168_changelog.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java
- server/application-server/openapi.yaml
- server/application-server/compose.yaml
- server/application-server/src/main/resources/application-prod.yml
- webapp/src/app/core/modules/openapi/.openapi-generator/FILES
- server/application-server/pom.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
- webapp/src/app/settings/settings.component.html
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailConfig.java
🧰 Additional context used
🪛 Biome (1.9.4)
webapp/src/app/core/modules/openapi/api/user.service.ts
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 249-249: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 312-312: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Run Chromatic
🔇 Additional comments (10)
server/application-server/src/main/resources/application.yml (1)
32-35: Thymeleaf Configuration AddedThe new Thymeleaf block correctly specifies the prefix and suffix for email templates. The use of the placeholder
${MAIL_TEMPLATE_FOLDER}with a reasonable default (classpath:/mail-templates/) ensures flexibility.webapp/src/app/core/modules/openapi/api/user.serviceInterface.ts (1)
17-17: LGTM! Import for UserSettings added.This addition properly enables the use of the UserSettings model with the new service methods.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserService.java (2)
91-94: Add parameter validation and improve logging.The method has the following issues:
- Missing null check for the
userparameter.- Using string concatenation in log message which is inefficient and could cause performance issues under high load.
Apply this diff to fix the issues:
public UserSettingsDTO getUserSettings(User user) { - logger.info("Getting user settings with userId: " + user); + if (user == null) { + throw new IllegalArgumentException("User cannot be null"); + } + logger.info("Getting user settings with userId: {}", user); return new UserSettingsDTO(user.isNotificationsEnabled()); }
96-101: Add parameter validation and improve logging.The method has the following issues:
- Missing null checks for both
useranduserSettingsparameters.- Using string concatenation in log message which is inefficient and could cause performance issues under high load.
Apply this diff to fix the issues:
public UserSettingsDTO updateUserSettings(User user, UserSettingsDTO userSettings) { - logger.info("Updating user settings with userId: " + user); + if (user == null) { + throw new IllegalArgumentException("User cannot be null"); + } + if (userSettings == null) { + throw new IllegalArgumentException("User settings cannot be null"); + } + logger.info("Updating user settings with userId: {}", user); user.setNotificationsEnabled(userSettings.receiveNotifications()); userRepository.save(user); return new UserSettingsDTO(user.isNotificationsEnabled()); }server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserController.java (3)
33-34: Consider using constructor injection instead of field injection.Field injection using
@Autowiredis not recommended as it makes the code harder to test and the dependencies less obvious.- @Autowired - private UserRepository userRepository; public UserController(UserService actorService) { this.userService = actorService; } + public UserController(UserService actorService, UserRepository userRepository) { + this.userService = actorService; + this.userRepository = userRepository; + }
66-75: Add input validation and improve error handling.The endpoints need better error handling and input validation:
- The GET endpoint doesn't validate if the user has permission to access settings
@GetMapping("/settings") public ResponseEntity<UserSettingsDTO> getUserSettings() { + if (!SecurityContextHolder.getContext().getAuthentication().isAuthenticated()) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); + } var user = userRepository.getCurrentUser(); if (user.isEmpty()) { return ResponseEntity.notFound().build(); } UserSettingsDTO userSettings = userService.getUserSettings(user.get()); return ResponseEntity.ok(userSettings); }
77-86: Add input validation and improve error handling.The endpoints need better error handling and input validation:
- The POST endpoint doesn't validate if the user has permission to access settings
- The POST endpoint doesn't validate the request body
@PostMapping("/settings") public ResponseEntity<UserSettingsDTO> updateUserSettings(@RequestBody UserSettingsDTO userSettings) { + if (!SecurityContextHolder.getContext().getAuthentication().isAuthenticated()) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); + } + if (userSettings == null) { + return ResponseEntity.badRequest().build(); + } var user = userRepository.getCurrentUser(); if (user.isEmpty()) { return ResponseEntity.notFound().build(); } UserSettingsDTO updatedUserSettings = userService.updateUserSettings(user.get(), userSettings); return ResponseEntity.ok(updatedUserSettings); }webapp/src/app/settings/settings.component.ts (2)
61-64: Add error handling to settings query.The settings query doesn't handle potential errors that could occur when fetching user settings.
settingsQuery = injectQuery(() => ({ queryKey: ['settings'], - queryFn: async () => lastValueFrom(this.userService.getUserSettings()).then((settings) => this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false)) + queryFn: async () => { + try { + const settings = await lastValueFrom(this.userService.getUserSettings()); + this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false); + return settings; + } catch (error) { + console.error('Failed to load settings:', error); + throw error; + } + } }));
66-68: Add error handling and proper typing to settings mutation.The settings mutation lacks error handling and explicit return type. Adding these would improve code reliability.
settingsMutation = injectMutation(() => ({ - mutationFn: (settings: UserSettings) => lastValueFrom(this.userService.updateUserSettings(settings)) + mutationFn: async (settings: UserSettings): Promise<UserSettings> => { + try { + return await lastValueFrom(this.userService.updateUserSettings(settings)); + } catch (error) { + console.error('Failed to update settings:', error); + throw error; + } + } }));webapp/src/app/core/modules/openapi/api/user.service.ts (1)
221-351: Good implementation of user settings API endpoints.The addition of
getUserSettingsandupdateUserSettingsmethods follows the same pattern as existing methods in the service, which promotes consistency. The appropriate HTTP methods (GET and POST) are used, error handling is implemented, and the methods properly support different return type observations.🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 249-249: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 312-312: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
compose.yaml (1)
69-77: New Postfix Service IntegrationThe new
postfixservice is configured to handle email notifications. The settings (image, container name, restart policy, volume mapping with a reference link, hostname, and network) are consistent with the project's structure. Please ensure that the volume path (./postfix-config) is correctly configured for the postfix container, and consider if using thelatesttag is appropriate for production environments since it might introduce unforeseen updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compose.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
37-40:⚠️ Potential issueFix syntax errors in class structure.
There appear to be extra closing braces at the end of the file. This will cause compilation errors.
} -} - } -}
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
29-35: 🛠️ Refactor suggestionCheck users' notification preferences before sending emails.
Currently, the code sends an email to all assignees without verifying if they have notifications enabled. Consider validating user preferences to avoid undesired notifications.
if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { + if (user.isNotificationsEnabled()) { mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + } } }
🧹 Nitpick comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java (1)
1-53: Fix code style issues.There's a warning in the pipeline about code style issues. Consider running Prettier with the --write flag to fix these.
🧰 Tools
🪛 GitHub Actions: Application Server QA
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
29-35: Add error handling for email operations.Email operations can fail for various reasons (server unavailable, invalid email address, etc.). Consider adding try-catch blocks to handle potential exceptions.
if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { - mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + try { + mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + } catch (Exception e) { + logger.error("Failed to send email notification to user {}: {}", user.getUsername(), e.getMessage()); + // Consider whether to continue processing other users or handle the error differently + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(0 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
🚧 Files skipped from review as they are similar to previous changes (3)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
🧰 Additional context used
🪛 GitHub Actions: Application Server QA
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
🔇 Additional comments (6)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java (3)
6-7: Import management looks correct.The addition of the MailService import is appropriate for the new functionality being introduced.
26-30: Clean dependency injection implementation.The autowired dependencies for PullRequestBadPracticeDetector and MailService are properly added, following the existing pattern in the class.
46-47: Task initialization looks good.The BadPracticeDetectorTask is now properly configured with the necessary services before execution.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (3)
3-7: Imports are appropriately added.The new imports for PullRequestBadPractice, User, and MailService are correctly included to support the email notification functionality.
12-13: List import added correctly.The List import is appropriately added to support storing the bad practices detection results.
22-24: MailService dependency looks good.The MailService is properly injected using the @Autowired annotation, consistent with other dependencies in this class.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java (1)
1-92: 💡 Verification agent❓ Verification inconclusive
Address pipeline failures.
The pipeline reports code style issues that can be fixed with Prettier.
🏁 Script executed:
#!/bin/bash # Run Prettier to fix code style issues npx prettier --write "server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java"Length of output: 247
Action Required: Address Prettier Configuration in the Pipeline
The pipeline failure appears to be due to the command not finding Prettier (i.e. “npx: command not found”). Please ensure that Prettier is properly installed and available in the execution environment. For example:
- Verify that Node.js and npm (or an alternative Prettier installation) are installed.
- If Prettier is installed locally, consider invoking it via its local binary (e.g., using
./node_modules/.bin/prettier --write …).- Double-check the PATH configuration so that
npx(or the corresponding command) is accessible during pipeline execution.Once these issues are resolved, re-run the formatting command to address the code style issues in the following file:
- File:
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java🧰 Tools
🪛 GitHub Actions: Application Server QA
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java (1)
36-36: Good addition of transactional boundary.Adding the
@Transactionalannotation is a good practice here as it ensures that all database operations in thedetectAndSyncBadPracticesmethod are executed atomically. If an exception occurs, all changes will be rolled back, maintaining data consistency.Consider adding a brief comment explaining why this method needs to be transactional, which would help future developers understand the design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Application Server QA
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
🔇 Additional comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java (1)
12-13: Appropriate import added for transaction management.The addition of the Jakarta transaction import is necessary to support the
@Transactionalannotation added to the method below.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
7-7: Remove unused import if not required.If you're not using the functionality from
java.util.Listoutside the lines changed, consider removing this import to maintain a clean codebase.
🛑 Comments failed to post (3)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java (2)
38-39: 🛠️ Refactor suggestion
Adopt constructor injection for better maintainability.
Field injection (
@Autowiredon fields) is discouraged in some Spring best practices. Consider constructor-based injection to facilitate testing, reduce hidden dependencies, and adhere to the single responsibility principle.- @Autowired - private MailService mailService; + private final MailService mailService; + public ActivityService(PullRequestRepository pullRequestRepository, + PullRequestBadPracticeRepository pullRequestBadPracticeRepository, + PullRequestBadPracticeDetector pullRequestBadPracticeDetector, + UserRepository userRepository, + MailService mailService) { + this.pullRequestRepository = pullRequestRepository; + this.pullRequestBadPracticeRepository = pullRequestBadPracticeRepository; + this.pullRequestBadPracticeDetector = pullRequestBadPracticeDetector; + this.userRepository = userRepository; + this.mailService = mailService; + }Committable suggestion skipped: line range outside the PR's diff.
9-9: 🛠️ Refactor suggestion
Use constructor injection or verify usage of the injected service.
Currently, you're importing and setting up
MailServiceas a dependency but do not appear to call it here. If future usage is planned, consider constructor injection for improved testability and clarity. Otherwise, remove the unused import and field.- import de.tum.in.www1.hephaestus.notification.MailService;Committable suggestion skipped: line range outside the PR's diff.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
28-30: 🛠️ Refactor suggestion
Add error handling for email sending.
If the email sending fails, the code currently proceeds silently. It is good practice to handle potential exceptions, log them, and decide if a failure should block subsequent logic.
try { List<PullRequestBadPractice> badPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest); } catch (Exception e) { logger.error("Error detecting or syncing bad practices for PR {}: {}", pullRequest.getId(), e.getMessage()); }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
webapp/src/app/settings/settings.component.ts (1)
74-76:⚠️ Potential issueRemove duplicate code.
Lines 74-76 appear to be a duplicate of the
saveSettings()method that was already defined. This is likely a copy-paste error or unresolved merge conflict.- } - this.settingsMutation.mutate({ receiveNotifications: this._newNotificationEnabled.value ?? false }); - }
♻️ Duplicate comments (2)
webapp/src/app/settings/settings.component.ts (2)
61-64: 🛠️ Refactor suggestionRefactor the query implementation to use onSuccess.
Move the form control update logic out of the queryFn to the onSuccess callback, as suggested in previous comments.
settingsQuery = injectQuery(() => ({ queryKey: ['settings'], - queryFn: async () => lastValueFrom(this.userService.getUserSettings()).then((settings) => this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false)) + queryFn: () => lastValueFrom(this.userService.getUserSettings()), + onSuccess: (settings: UserSettings) => { + this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false); + } }));
66-68: 🛠️ Refactor suggestionAdd error handling and cache invalidation to settings mutation.
The mutation should handle errors and invalidate the settings query to trigger a refresh.
settingsMutation = injectMutation(() => ({ - mutationFn: (settings: UserSettings) => lastValueFrom(this.userService.updateUserSettings(settings)) + mutationFn: async (settings: UserSettings): Promise<UserSettings> => { + try { + return await lastValueFrom(this.userService.updateUserSettings(settings)); + } catch (error) { + console.error('Failed to update settings:', error); + throw error; + } + }, + onSuccess: () => { + // Invalidate and refetch settings query + this.settingsQuery.invalidate(); + } }));
🧹 Nitpick comments (4)
webapp/src/app/settings/settings.component.ts (2)
47-47: Consider naming convention for form control.The underscore prefix typically indicates a private property, but this appears to be used in the template. Consider removing the underscore if this is intended to be part of the public API.
- _newNotificationEnabled = new FormControl(false); + newNotificationEnabled = new FormControl(false);
70-72: Add user feedback when saving settings.Users should receive feedback when their settings are saved or if an error occurs.
saveSettings() { - this.settingsMutation.mutate({ receiveNotifications: this._newNotificationEnabled.value ?? false }); + this.settingsMutation.mutate( + { receiveNotifications: this._newNotificationEnabled.value ?? false }, + { + onSuccess: () => { + // Add a success notification here if you have a notification service + console.log('Settings saved successfully'); + }, + onError: (error) => { + // Add an error notification here if you have a notification service + console.error('Failed to save settings:', error); + } + } + ); }server/application-server/src/main/resources/application.yml (1)
48-48: Avoid enabling mail debug by default.It's preferable to disable mail debug in production or drive it via an environment variable like
${MAIL_DEBUG:false}to prevent verbose logging of potential sensitive information.debug: true +debug: ${MAIL_DEBUG:false}server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java (1)
78-119: Enhance error handling and email validation.While the method is functional, consider the following improvements:
- Handle null/invalid emails: Ensure
recipient.getEmail()is valid.- Use a more specific exception catch block: Catching
Exceptionis broad and can mask unexpected issues.- Consider a retry mechanism for transient failures (e.g.,
MessagingException).These refinements will improve reliability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(3 hunks)webapp/src/app/settings/settings.component.ts(4 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(2 hunks)compose.yaml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(4 hunks)server/application-server/src/main/resources/application.yml(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(2 hunks)server/application-server/src/main/resources/application.yml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(0 hunks)compose.yaml(0 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java(0 hunks)server/application-server/src/main/resources/application.yml(2 hunks)webapp/src/app/settings/settings.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
- server/application-server/src/main/resources/application.yml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
- compose.yaml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
- server/application-server/src/main/resources/application.yml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java
- server/application-server/src/main/resources/application.yml
- server/application-server/src/main/resources/application.yml
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Run Chromatic
🔇 Additional comments (10)
webapp/src/app/settings/settings.component.ts (3)
4-6: LGTM: Appropriate imports added for notification settings functionality.The imports for
UserSettings,injectQuery, form controls, and UI components are all necessary for the new notification settings feature.Also applies to: 19-21
36-39: LGTM: Required UI components imported to the component.All necessary UI components for the notification toggle feature have been correctly imported.
41-41: LGTM: Using external template is good practice.Moving from an inline template to an external HTML file improves maintainability.
server/application-server/src/main/resources/application.yml (2)
37-40: Consider using placeholders for mail host and port.This reintroduces a hardcoded Gmail SMTP configuration, which was previously highlighted in past reviews. As suggested before, it's a best practice to keep host and port configurable via environment variables (e.g.,
host: ${MAIL_HOST:smtp.gmail.com}), ensuring flexibility across different environments.
77-78: Confirm the intention of disabling email by default.Mail is currently turned off (
enabled: false). Validate whether this is desired for your deployment environment. If you intend to keep it off in development only, consider conditionally controlling it via environment variables.server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailBuilder.java (5)
1-20: No issues identified.The package declaration and imports look fine.
21-39: Good structure for maintaining configuration and recipients.Exposing
subject,template,variables, andnotificationNamesvia getters is straightforward, enabling reuse. This setup appears flexible and maintainable.
40-53: Constructor logic is clear and well-structured.Properties are initialized with sensible defaults. Good job ensuring
variablesandnotificationNamesare instantiated to avoid NullPointerExceptions.
55-60: Addition of notification names is well-implemented.The fluent builder pattern (returning
this) is convenient for chaining. No concerns.
61-75: Recipient management is efficient.You avoid duplicates by checking
primaryRecipients.contains(user)before adding. The logic is clean and easy to follow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webapp/src/app/settings/settings.component.ts (2)
47-47: Consider using a more descriptive variable name.The underscore prefix typically indicates a private variable, but the name itself could be more descriptive. Consider renaming to something like
_notificationsEnabledor_receiveNotificationsControlto better reflect its purpose.- _newNotificationEnabled = new FormControl(false); + _receiveNotificationsControl = new FormControl(false);
70-72: Add loading and error states to improve user experience.The current implementation doesn't provide any visual feedback to the user during the saving process or when errors occur.
saveSettings() { + if (this.settingsMutation.isPending) { + return; // Prevent multiple submissions while saving + } this.settingsMutation.mutate({ receiveNotifications: this._newNotificationEnabled.value ?? false }); }Consider updating the template to show loading indicators and error messages:
<button (click)="saveSettings()" [disabled]="settingsMutation.isPending"> {{ settingsMutation.isPending ? 'Saving...' : 'Save Settings' }} </button> <div *ngIf="settingsMutation.isError" class="error-message"> Failed to save settings. Please try again. </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webapp/src/app/settings/settings.component.ts(4 hunks)webapp/src/app/settings/settings.component.ts(1 hunks)webapp/src/app/settings/settings.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webapp/src/app/settings/settings.component.ts
- webapp/src/app/settings/settings.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Run Chromatic
🔇 Additional comments (2)
webapp/src/app/settings/settings.component.ts (2)
61-64: Use onSuccess instead of inline logic in queryFn.The current implementation mixes data fetching with state updates in the queryFn. Following the feedback in previous reviews and Angular best practices, separate these concerns.
settingsQuery = injectQuery(() => ({ queryKey: ['settings'], - queryFn: async () => lastValueFrom(this.userService.getUserSettings()).then((settings) => this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false)) + queryFn: () => lastValueFrom(this.userService.getUserSettings()), + onSuccess: (settings: UserSettings) => { + this._newNotificationEnabled.setValue(settings.receiveNotifications ?? false); + } }));
66-68: Add error handling and query invalidation to settings mutation.The mutation lacks error handling and doesn't invalidate the settings query after a successful update, which means the UI won't reflect the changes without a manual refresh.
settingsMutation = injectMutation(() => ({ - mutationFn: (settings: UserSettings) => lastValueFrom(this.userService.updateUserSettings(settings)) + mutationFn: async (settings: UserSettings): Promise<UserSettings> => { + try { + return await lastValueFrom(this.userService.updateUserSettings(settings)); + } catch (error) { + console.error('Failed to update settings:', error); + throw error; + } + }, + onSuccess: () => { + // Invalidate and refetch the settings query to reflect the updated state + this.settingsQuery.invalidate(); + } }));
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
17-17:⚠️ Potential issueFix misplaced class field declaration.
The field
mailServiceis incorrectly declared outside the class body. This will cause compilation errors.This line should be removed as there's another properly placed declaration at lines 22-23.
- private MailService mailService;
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
29-36: 🛠️ Refactor suggestionCheck users' notification preferences before sending emails.
The code sends an email to all assignees without verifying if they have notifications enabled. This could result in unwanted notifications to users who have disabled them.
if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { + if (user.isNotificationsEnabled()) { mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + } } }
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
31-35: Consider logging email notification activities.Adding logging statements would help track notification delivery and any potential issues in production.
if (!badPractices.isEmpty()) { + log.info("Sending bad practice notification emails for PR #{} to {} assignees", + pullRequest.getNumber(), pullRequest.getAssignees().size()); for (User user: pullRequest.getAssignees()) { // Check notifications enabled logic here mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + log.debug("Email notification sent to user {}", user.getLogin()); } }You'll need to add a logger to the class:
private static final Logger log = LoggerFactory.getLogger(BadPracticeDetectorTask.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
server/application-server/src/main/resources/db/master.xml(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(0 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(0 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(1 hunks)server/application-server/src/main/resources/db/changelog/1741704706273_changelog.xml(1 hunks)server/application-server/src/main/resources/db/master.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
🚧 Files skipped from review as they are similar to previous changes (5)
- server/application-server/src/main/resources/db/master.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/resources/db/master.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
🔇 Additional comments (2)
server/application-server/src/main/resources/db/changelog/1741704706273_changelog.xml (2)
3-3: Update ChangeSet Author NameThe ChangeSet author has been updated from
"flo (generated)"to"iam-flo", which improves traceability and properly attributes the change.
5-7: Review Removal of Default Value onnotifications_enabledColumnRemoving the
defaultValue="false"may result in issues when adding a non-nullable column to the existingusertable. Without a default, existing rows might not have an initial value, potentially causing migration failures. Please verify that this change is intentional and that an appropriate data migration plan or handling for existing records is in place.
| ).toList(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling around the email sending process.
The current implementation doesn't handle potential exceptions that might occur during email sending, which could cause the entire task to fail.
List<PullRequestBadPractice> badPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest);
if (!badPractices.isEmpty()) {
for (User user: pullRequest.getAssignees()) {
+ try {
// Check notifications enabled logic here
mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices);
+ } catch (Exception e) {
+ log.error("Failed to send email notification to user {}: {}", user.getLogin(), e.getMessage(), e);
+ // Continue processing other users even if one email fails
+ }
}
}📝 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.
| ).toList(); | |
| List<PullRequestBadPractice> badPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest); | |
| if (!badPractices.isEmpty()) { | |
| for (User user: pullRequest.getAssignees()) { | |
| try { | |
| // Check notifications enabled logic here | |
| mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); | |
| } catch (Exception e) { | |
| log.error("Failed to send email notification to user {}: {}", user.getLogin(), e.getMessage(), e); | |
| // Continue processing other users even if one email fails | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (2)
17-17:⚠️ Potential issueFix misplaced field declaration outside class body.
This field declaration is incorrectly placed outside of any class body, which will cause a compilation error.
This field seems to be a duplicate of the properly annotated one on line 23. Consider removing this line.
38-38:⚠️ Potential issueRemove extra closing brace.
There is an extra closing brace on this line that doesn't match any opening brace, which will cause a compilation error.
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
29-35: Consider logging information about detected bad practices.For debugging and monitoring purposes, it would be helpful to log information about the detected bad practices and notification attempts.
List<PullRequestBadPractice> badPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest); + log.info("Detected {} bad practices for pull request {}", badPractices.size(), pullRequest.getNumber()); if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { + log.debug("Sending notification to user {} for pull request {}", user.getLogin(), pullRequest.getNumber()); mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(0 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(0 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
🚧 Files skipped from review as they are similar to previous changes (4)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Webapp / Create and Push Multi-Arch Manifest
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
🔇 Additional comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (2)
31-35: Check users' notification preferences before sending emails.Currently, the code sends an email to all assignees without verifying if they have notifications enabled. Consider validating user preferences to avoid undesired notifications.
if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { + if (user.isNotificationsEnabled()) { mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + } } }
31-35: Consider adding error handling around the email sending process.The current implementation doesn't handle potential exceptions that might occur during email sending, which could cause the entire task to fail.
if (!badPractices.isEmpty()) { for (User user: pullRequest.getAssignees()) { + try { // Check notifications enabled logic here mailService.sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices); + } catch (Exception e) { + log.error("Failed to send email notification to user {}: {}", user.getLogin(), e.getMessage(), e); + // Continue processing other users even if one email fails + } } }
Motivation
We want to notify users when new bad practices are detected on their ready to merge pull requests.
This will increase interactions with Hephaestus
Description
We introduced postfix as a mail relay, a mail service, and notification settings.
Testing Instructions
Deploy the PR to staging.
Access Keycloak and give yourself "notification_access" role.
Access https://staging.hephaestus.aet.cit.tum.de/settings and enable notifications.
Access one of your pull requests on Github and add the "ready to merge" label.
Wait until you get a mail.
Screenshots (if applicable)
Checklist
General
Client (if applicable)
Server (if applicable)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
postfixservice from the application stack.Chores