Good practices mail #372
Open
Good practices mail #372
+42
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe changes enhance the notification system for pull request best practice detection. The system now tracks and reports both good and bad practices. Email notifications and templates are updated to display detected good practices alongside bad practices, and a new HTML template for listing good practices is added. Method signatures and email content are adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Detector as BadPracticeDetectorTask
participant MailService
participant User
Detector->Detector: Identify bad and good practices in PR
Detector->MailService: sendBadPracticesDetectedInPullRequestEmail(user, pullRequest, badPractices, goodPractices)
MailService->MailService: Prepare email content (bad + good practices)
MailService->User: Send notification email with both practice lists
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces functionality to notify users about good practices detected in their pull requests, expanding the existing bad practice notification system. The implementation adds email notifications for good practices while refactoring the existing code to handle both types of practices more generically.
Key changes:
- Added email notifications for detected good practices in pull requests
- Refactored the mail service to handle both good and bad practices with a unified method
- Created a new HTML email template for good practice notifications
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
good-practices-detected.html |
New email template for notifying users about detected good practices |
MailService.java |
Added good practice email method and refactored practice string generation to be generic |
BadPracticeDetectorTask.java |
Added logic to filter and send notifications for good practices |
Comments suppressed due to low confidence (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java:77
- The parameter name
badPracticesis misleading since this method handles good practices. Consider renaming it topracticesorgoodPracticesfor clarity.
List<PullRequestBadPractice> badPractices
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java:111
- The parameter name
badPracticesis misleading since this method now handles both good and bad practices. Consider renaming it topracticesfor clarity.
private String getGoodOrBadPracticeString(List<PullRequestBadPractice> badPractices, String goodOrBad) {
| } | ||
| } else if (sendBadPracticeDetectionEmail && !goodPractices.isEmpty()) { | ||
| for (User user : pullRequest.getAssignees()) { | ||
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, unResolvedBadPractices); |
There was a problem hiding this comment.
The method is being called with unResolvedBadPractices instead of goodPractices. This will send an email about good practices but include the bad practices list, which is incorrect.
Suggested change
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, unResolvedBadPractices); | |
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, goodPractices); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/application-server/src/main/resources/mail-templates/good-practices-detected.html (1)
7-11: Consider the data structure inconsistency.The template iterates over
goodPracticesbut accessesgoodPractice.state.getValue(). Since good practices are filtered from the samePullRequestBadPracticeobjects, this works but creates semantic confusion.Consider renaming the variable in the template context to better reflect that these are practices marked as good:
- <li th:each="goodPractice : ${goodPractices}"> - <strong th:text="${goodPractice.title}">Good Practice Title</strong>: [[${goodPractice.state.getValue()}]]<br /> - <span th:text="${goodPractice.description}">Good Practice Description</span> + <li th:each="practice : ${goodPractices}"> + <strong th:text="${practice.title}">Good Practice Title</strong>: [[${practice.state.getValue()}]]<br /> + <span th:text="${practice.description}">Good Practice Description</span>server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java (1)
111-119: Consider refactoring to reduce code duplication.Both email methods share significant duplicate code. Consider extracting the common Keycloak user lookup and email sending logic.
Extract common functionality:
+private String getUserEmail(User user) throws Exception { + UserRepresentation keyCloakUser = keycloak + .realm(realm) + .users() + .searchByUsername(user.getLogin(), true) + .getFirst(); + return keyCloakUser.getEmail(); +} + +private void sendPracticeEmail(User user, PullRequest pullRequest, + List<PullRequestBadPractice> practices, + String practiceType, String templateName) { + logger.info("Sending {} practice detected email to user: {}", practiceType, user.getLogin()); + + try { + String email = getUserEmail(user); + String subject = "Hephaestus: " + + getGoodOrBadPracticeString(practices, practiceType) + + " detected in your pull request #" + pullRequest.getNumber(); + + MailBuilder mailBuilder = new MailBuilder(mailConfig, user, email, subject, templateName); + mailBuilder + .fillPlaceholder(user, "user") + .fillPlaceholder(pullRequest, "pullRequest") + .fillPlaceholder(practices, practiceType + "Practices") + .fillPlaceholder(getGoodOrBadPracticeString(practices, practiceType), practiceType + "PracticeString") + .fillPlaceholder(pullRequest.getRepository().getName(), "repository") + .send(javaMailSender); + } catch (Exception e) { + logger.error("Failed to find user in Keycloak: {}", user.getLogin(), e); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/notification/MailService.java(2 hunks)server/application-server/src/main/resources/mail-templates/good-practices-detected.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: security-scan / dependencies
- GitHub Check: quality-gates / application-server-quality
- GitHub Check: docker-build / webapp-react-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/webapp-react
- GitHub Check: quality-gates / database-documentation-validation
- GitHub Check: quality-gates / database-schema-validation
- GitHub Check: quality-gates / openapi-validation
- GitHub Check: quality-gates / intelligence-service-quality
- GitHub Check: security-scan / sast
- GitHub Check: test-suite / application-server-integration
🔇 Additional comments (4)
server/application-server/src/main/resources/mail-templates/good-practices-detected.html (2)
4-4: Verify the goodPracticeString variable is correctly passed.The template expects a
goodPracticeStringvariable, but based on the MailService implementation, this should contain the formatted count of good practices.
13-13: /best-practices endpoint verified – no action required.Found in both frontends:
- Angular (webapp):
webapp/src/app/app.routes.tsdefines{ path: 'best-practices', component: ActivityDashboardComponent, canActivate: [AuthGuard] }- React (webapp-react):
src/routes/_authenticated/best-practices.tsxandsrc/routes/_authenticated/user/$username/best-practices.tsxviacreateFileRoute("/_authenticated/best-practices")server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java (1)
45-48: LGTM! Good practice filtering logic is correct.The filtering correctly identifies practices marked with
GOOD_PRACTICEstate.server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java (1)
60-60: LGTM! Refactoring to use the generalized string method.The refactoring correctly maintains the existing functionality while using the new generalized method.
Also applies to: 69-69
Comment on lines
54
to
58
| } else if (sendBadPracticeDetectionEmail && !goodPractices.isEmpty()) { | ||
| for (User user : pullRequest.getAssignees()) { | ||
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, unResolvedBadPractices); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Wrong parameter passed to good practice email method.
Line 56 passes unResolvedBadPractices to the good practice email method, but it should pass goodPractices since that's what the email template expects.
Apply this fix:
- mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, unResolvedBadPractices);
+ mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, goodPractices);📝 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.
Suggested change
| } else if (sendBadPracticeDetectionEmail && !goodPractices.isEmpty()) { | |
| for (User user : pullRequest.getAssignees()) { | |
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, unResolvedBadPractices); | |
| } | |
| } | |
| } else if (sendBadPracticeDetectionEmail && !goodPractices.isEmpty()) { | |
| for (User user : pullRequest.getAssignees()) { | |
| mailService.sendGoodPracticesDetectedInPullRequestEmail(user, pullRequest, goodPractices); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java
between lines 54 and 58, the method
mailService.sendGoodPracticesDetectedInPullRequestEmail is incorrectly called
with unResolvedBadPractices instead of goodPractices. To fix this, replace the
unResolvedBadPractices argument with goodPractices in the method call to match
the expected email template parameter.
Comment on lines
74
to
109
| public void sendGoodPracticesDetectedInPullRequestEmail( | ||
| User user, | ||
| PullRequest pullRequest, | ||
| List<PullRequestBadPractice> badPractices | ||
| ) { | ||
| logger.info("Sending good practice detected email to user: {}", user.getLogin()); | ||
| String email; | ||
|
|
||
| try { | ||
| UserRepresentation keyCloakUser = keycloak | ||
| .realm(realm) | ||
| .users() | ||
| .searchByUsername(user.getLogin(), true) | ||
| .getFirst(); | ||
|
|
||
| email = keyCloakUser.getEmail(); | ||
| } catch (Exception e) { | ||
| logger.error("Failed to find user in Keycloak: {}", user.getLogin(), e); | ||
| return; | ||
| } | ||
|
|
||
| String subject = | ||
| "Hephaestus: " + | ||
| getGoodOrBadPracticeString(badPractices,"good") + | ||
| " detected in your pull request #" + | ||
| pullRequest.getNumber(); | ||
|
|
||
| MailBuilder mailBuilder = new MailBuilder(mailConfig, user, email, subject, "good-practices-detected"); | ||
| mailBuilder | ||
| .fillPlaceholder(user, "user") | ||
| .fillPlaceholder(pullRequest, "pullRequest") | ||
| .fillPlaceholder(badPractices, "goodPractices") | ||
| .fillPlaceholder(getGoodOrBadPracticeString(badPractices,"good"), "goodPracticeString") | ||
| .fillPlaceholder(pullRequest.getRepository().getName(), "repository") | ||
| .send(javaMailSender); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix parameter naming and method signature.
The method parameter is named badPractices but is used for good practices, creating confusion. Additionally, the method logic assumes it receives good practices but the parameter suggests otherwise.
Apply this fix to improve clarity:
public void sendGoodPracticesDetectedInPullRequestEmail(
User user,
PullRequest pullRequest,
- List<PullRequestBadPractice> badPractices
+ List<PullRequestBadPractice> goodPractices
) {
logger.info("Sending good practice detected email to user: {}", user.getLogin());
String email;
try {
UserRepresentation keyCloakUser = keycloak
.realm(realm)
.users()
.searchByUsername(user.getLogin(), true)
.getFirst();
email = keyCloakUser.getEmail();
} catch (Exception e) {
logger.error("Failed to find user in Keycloak: {}", user.getLogin(), e);
return;
}
String subject =
"Hephaestus: " +
- getGoodOrBadPracticeString(badPractices,"good") +
+ getGoodOrBadPracticeString(goodPractices,"good") +
" detected in your pull request #" +
pullRequest.getNumber();
MailBuilder mailBuilder = new MailBuilder(mailConfig, user, email, subject, "good-practices-detected");
mailBuilder
.fillPlaceholder(user, "user")
.fillPlaceholder(pullRequest, "pullRequest")
- .fillPlaceholder(badPractices, "goodPractices")
- .fillPlaceholder(getGoodOrBadPracticeString(badPractices,"good"), "goodPracticeString")
+ .fillPlaceholder(goodPractices, "goodPractices")
+ .fillPlaceholder(getGoodOrBadPracticeString(goodPractices,"good"), "goodPracticeString")
.fillPlaceholder(pullRequest.getRepository().getName(), "repository")
.send(javaMailSender);
}📝 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.
Suggested change
| public void sendGoodPracticesDetectedInPullRequestEmail( | |
| User user, | |
| PullRequest pullRequest, | |
| List<PullRequestBadPractice> badPractices | |
| ) { | |
| logger.info("Sending good practice detected email to user: {}", user.getLogin()); | |
| String email; | |
| try { | |
| UserRepresentation keyCloakUser = keycloak | |
| .realm(realm) | |
| .users() | |
| .searchByUsername(user.getLogin(), true) | |
| .getFirst(); | |
| email = keyCloakUser.getEmail(); | |
| } catch (Exception e) { | |
| logger.error("Failed to find user in Keycloak: {}", user.getLogin(), e); | |
| return; | |
| } | |
| String subject = | |
| "Hephaestus: " + | |
| getGoodOrBadPracticeString(badPractices,"good") + | |
| " detected in your pull request #" + | |
| pullRequest.getNumber(); | |
| MailBuilder mailBuilder = new MailBuilder(mailConfig, user, email, subject, "good-practices-detected"); | |
| mailBuilder | |
| .fillPlaceholder(user, "user") | |
| .fillPlaceholder(pullRequest, "pullRequest") | |
| .fillPlaceholder(badPractices, "goodPractices") | |
| .fillPlaceholder(getGoodOrBadPracticeString(badPractices,"good"), "goodPracticeString") | |
| .fillPlaceholder(pullRequest.getRepository().getName(), "repository") | |
| .send(javaMailSender); | |
| } | |
| public void sendGoodPracticesDetectedInPullRequestEmail( | |
| User user, | |
| PullRequest pullRequest, | |
| List<PullRequestBadPractice> goodPractices | |
| ) { | |
| logger.info("Sending good practice detected email to user: {}", user.getLogin()); | |
| String email; | |
| try { | |
| UserRepresentation keyCloakUser = keycloak | |
| .realm(realm) | |
| .users() | |
| .searchByUsername(user.getLogin(), true) | |
| .getFirst(); | |
| email = keyCloakUser.getEmail(); | |
| } catch (Exception e) { | |
| logger.error("Failed to find user in Keycloak: {}", user.getLogin(), e); | |
| return; | |
| } | |
| String subject = | |
| "Hephaestus: " + | |
| getGoodOrBadPracticeString(goodPractices, "good") + | |
| " detected in your pull request #" + | |
| pullRequest.getNumber(); | |
| MailBuilder mailBuilder = new MailBuilder(mailConfig, user, email, subject, "good-practices-detected"); | |
| mailBuilder | |
| .fillPlaceholder(user, "user") | |
| .fillPlaceholder(pullRequest, "pullRequest") | |
| .fillPlaceholder(goodPractices, "goodPractices") | |
| .fillPlaceholder(getGoodOrBadPracticeString(goodPractices, "good"), "goodPracticeString") | |
| .fillPlaceholder(pullRequest.getRepository().getName(), "repository") | |
| .send(javaMailSender); | |
| } |
🤖 Prompt for AI Agents
In
server/application-server/src/main/java/de/tum/in/www1/hephaestus/notification/MailService.java
between lines 74 and 109, the method parameter named 'badPractices' is
misleading because the method handles good practices. Rename the parameter to
'goodPractices' and update all references within the method accordingly to
reflect that it deals with good practices, ensuring the method signature and
internal usage are consistent and clear.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test