Communication: Fix announcement emails not rendering correctly
#9850
Conversation
github-actions
bot
added
tests
server
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes in this pull request involve enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (14)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java (1)
7-10: Consider using package-private access for the class.Following the principle of least privilege, if this factory is only used within the notifications package, consider removing the
publicmodifier to restrict access.-public class MarkdownImageBlockRendererFactory implements HtmlNodeRendererFactory { +class MarkdownImageBlockRendererFactory implements HtmlNodeRendererFactory {src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownRelativeToAbsolutePathAttributeProvider.java (2)
8-10: Consider adding input validation for baseUrl.While the implementation is clean, the
baseUrlfield should be validated to ensure it's not null or empty and ends with a trailing slash if required for proper URL concatenation.private final String baseUrl; + +private static String validateBaseUrl(String baseUrl) { + if (baseUrl == null || baseUrl.trim().isEmpty()) { + throw new IllegalArgumentException("baseUrl must not be null or empty"); + } + return baseUrl.endsWith("/") ? baseUrl : baseUrl + "/"; +}
12-14: Add validation in constructor.Update the constructor to use the suggested validation method.
public MarkdownRelativeToAbsolutePathAttributeProvider(String baseUrl) { - this.baseUrl = baseUrl; + this.baseUrl = validateBaseUrl(baseUrl); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRenderer.java (1)
13-18: Add class-level JavaDoc documentation.As per the PR objectives, Java code should be documented using JavaDoc style. Please add class-level documentation explaining the purpose and usage of this renderer.
Add the following documentation:
+/** + * A custom renderer for Markdown image nodes that converts them into HTML anchor elements. + * This renderer is specifically designed for email notifications where images need to be + * rendered as absolute URLs. + */ public class MarkdownImageBlockRenderer implements NodeRenderer {src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomReferenceRendererServiceTest.java (2)
10-18: Add JavaDoc to document the test class purpose.Consider adding class-level documentation to describe the purpose and scope of these tests, especially since this is part of the email rendering enhancement feature.
+/** + * Tests for {@link MarkdownCustomReferenceRendererService} which handles the rendering + * of user and channel references in announcement emails. + */ class MarkdownCustomReferenceRendererServiceTest {
19-59: Consider adding more edge cases and boundary tests.While the current tests cover the basic scenarios well, consider adding the following test cases:
- Handling of null input
- Malformed tags (e.g., unclosed tags, missing parentheses)
- Performance test with large input containing many references
- Nested tags
Example test additions:
@Test void shouldHandleNullInputGracefully() { assertThat(service.render(null)).isNull(); } @Test void shouldHandleMalformedTags() { String input = "[user]Max Muster(ga1234as)[/wrongtag]"; assertThat(service.render(input)).isEqualTo(input); } @Test void shouldHandleLargeInput() { StringBuilder input = new StringBuilder(); for (int i = 0; i < 1000; i++) { input.append("[user]User").append(i).append("(id").append(i).append(")[/user] "); } String result = service.render(input.toString()); assertThat(result).contains("@User0").contains("@User999"); }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java (2)
19-24: Consider improving the test setup approach.The current setup has a few potential improvements to consider:
- Using ReflectionTestUtils to set private fields might indicate that the
artemisServerUrlshould be injectable through constructor or setter.- The hard-coded URL could be moved to a constant or test configuration.
Consider this alternative approach:
- @BeforeEach - void setUp() throws MalformedURLException, URISyntaxException { - markdownCustomLinkRendererService = new MarkdownCustomLinkRendererService(); - - ReflectionTestUtils.setField(markdownCustomLinkRendererService, "artemisServerUrl", new URI("http://localhost:8080").toURL()); - } + private static final String TEST_SERVER_URL = "http://localhost:8080"; + + @BeforeEach + void setUp() { + markdownCustomLinkRendererService = new MarkdownCustomLinkRendererService(URI.create(TEST_SERVER_URL).toURL()); + }
26-70: Consider adding more edge cases to improve test coverage.While the current tests cover the basic scenarios well, consider adding tests for:
- Special characters in URLs and text
- XSS prevention (e.g.,
<script>tags in input)- Very long inputs
- Nested tags (e.g.,
[programming][quiz]text[/quiz][/programming])Here's an example of additional test cases:
@Test void shouldEscapeSpecialCharactersInUrls() { String input = "[programming]Test (/path with spaces/test?param=value&other=123)[/programming]"; String expected = "<a href=\"http://localhost:8080/path%20with%20spaces/test?param=value&other=123\">Test</a>"; String result = markdownCustomLinkRendererService.render(input); assertThat(result).isEqualTo(expected); } @Test void shouldPreventXssInLinks() { String input = "[programming]Test (/test?param=<script>alert('xss')</script>)[/programming]"; String result = markdownCustomLinkRendererService.render(input); assertThat(result).doesNotContain("<script>"); } @Test void shouldHandleNestedTags() { String input = "[programming][quiz]Nested (/quiz/1)[/quiz][/programming]"; String result = markdownCustomLinkRendererService.render(input); assertThat(result).contains("href=\"http://localhost:8080/quiz/1\""); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java (3)
17-20: Enhance class-level documentationWhile the documentation explains the basic purpose, it would be beneficial to add:
- Example input/output
- List of supported tags
- Format requirements for the markdown tags
/** * This service implements the rendering of markdown tags that represent a link. * It takes the tag, transforms it into an <a></a> tag, and sets the corresponding href. + * + * Example: + * Input: [programming]Course Overview(/courses/123)[/programming] + * Output: <a href="https://artemis.example.com/courses/123">Course Overview</a> + * + * Supported tags: programming, modeling, quiz, text, file-upload, lecture, + * attachment, lecture-unit, slide, faq + * + * Format: [tag]text(/relative/path)[/tag] */
27-30: Make fields final and privateFields should be marked as final when they're only set once.
- private final Set<String> supportedTags; + private static final Set<String> SUPPORTED_TAGS; @Value("${server.url}") - private URL artemisServerUrl; + private final URL artemisServerUrl;
43-70: Consider using StringBuilder for better performanceWhen performing multiple string modifications, StringBuilder would be more efficient.
public String render(String content) { Matcher matcher = MARKDOWN_PATTERN.matcher(content); - String parsedContent = content; + StringBuilder result = new StringBuilder(content); + int offset = 0; while (matcher.find()) { try { // ... existing validation code ... - parsedContent = parsedContent.substring(0, matcher.start()) + - "<a href=\"" + absoluteUrl + "\">" + text + "</a>" + - parsedContent.substring(matcher.end()); + String replacement = "<a href=\"" + absoluteUrl + "\">" + text + "</a>"; + result.replace(matcher.start() + offset, + matcher.end() + offset, + replacement); + offset += replacement.length() - (matcher.end() - matcher.start()); - matcher = pattern.matcher(parsedContent); } catch (Exception e) { // ... error handling ... } } - return parsedContent; + return result.toString(); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java (2)
15-21: Enhance class-level documentationWhile the current documentation provides a basic overview, consider enhancing it with:
- Examples of supported reference formats
- Description of the interface contract being implemented
- Expected input/output format examples
/** * This service implements the rendering of markdown tags that represent a reference (to e.g. a user). * These references cannot directly represent a link, so they are rendered as their text only. + * + * Supported formats: + * - User reference: [user]@username(id)[/user] -> @username + * - Channel reference: [channel]#channel-name(id)[/channel] -> #channel-name + * + * Implements MarkdownCustomRendererService to provide consistent rendering behavior + * across the application's markdown processing pipeline. */
25-34: Consider using EnumMap for type-safety and performanceThe current implementation using
SetandHashMapcould be improved for better type-safety and maintainability.Consider using an enum to represent the reference types:
public enum ReferenceType { USER("user", "@"), CHANNEL("channel", "#"); private final String tag; private final String prefix; ReferenceType(String tag, String prefix) { this.tag = tag; this.prefix = prefix; } public String getTag() { return tag; } public String getPrefix() { return prefix; } }Then modify the fields and constructor:
- private final Set<String> supportedTags; - private final HashMap<String, String> startingCharacters; + private final EnumMap<ReferenceType, String> startingCharacters; public MarkdownCustomReferenceRendererService() { - supportedTags = Set.of("user", "channel"); - startingCharacters = new HashMap<>(); - startingCharacters.put("user", "@"); - startingCharacters.put("channel", "#"); + startingCharacters = new EnumMap<>(ReferenceType.class); + for (ReferenceType type : ReferenceType.values()) { + startingCharacters.put(type, type.getPrefix()); + } }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (1)
Line range hint
142-170: Add test coverage for new rendering functionality.The current test suite focuses on email sending and exception handling but lacks coverage for the new rendering features. To ensure the PR objectives are met, consider adding the following test cases:
Attachment rendering tests:
- Test proper rendering of attachment links
- Verify conversion of relative to absolute URLs
- Check handling of invalid attachments
Reference rendering tests:
- Test proper rendering of user references
- Verify proper name display format
- Check handling of invalid references
Example test structure:
@Test void testAttachmentRendering() { // Given Post post = createPostWithAttachment(); when(linkRendererService.render(any())).thenReturn("rendered_attachment_link"); // When mailService.sendNotification(...); // Then verify(linkRendererService).render(any()); // Add assertions for proper rendering } @Test void testUserReferenceRendering() { // Given Post post = createPostWithUserReference(); when(referenceRendererService.render(any())).thenReturn("@username"); // When mailService.sendNotification(...); // Then verify(referenceRendererService).render(any()); // Add assertions for proper rendering }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomRendererService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRenderer.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownRelativeToAbsolutePathAttributeProvider.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomReferenceRendererServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomRendererService.java
🧰 Additional context used
📓 Path-based instructions (9)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRenderer.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownRelativeToAbsolutePathAttributeProvider.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomReferenceRendererServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (13)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java (2)
1-6: LGTM! Clean and minimal imports.
The package structure and imports are well-organized, using specific imports rather than wildcard imports.
17-17: Verify the MarkdownImageBlockRenderer implementation.
Let's ensure that the MarkdownImageBlockRenderer class exists and is properly implemented.
✅ Verification successful
Let me try a different approach to search for the implementation.
Let me check the complete implementation of MarkdownImageBlockRenderer to verify it properly implements NodeRenderer.
MarkdownImageBlockRenderer implementation is properly verified
The class correctly implements the NodeRenderer interface with both required methods:
getNodeTypes(): Returns a Set containing Image.classrender(Node): Implements image rendering logic by creating an HTML anchor tag with the image destination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence and implementation of MarkdownImageBlockRenderer
# Search for the class definition
echo "Searching for MarkdownImageBlockRenderer class definition:"
ast-grep --pattern 'class MarkdownImageBlockRenderer {
$$$
}'
# Search for NodeRenderer implementation
echo "Verifying NodeRenderer implementation:"
ast-grep --pattern 'class MarkdownImageBlockRenderer implements NodeRenderer {
$$$
}'
Length of output: 617
Script:
#!/bin/bash
# Search for MarkdownImageBlockRenderer using ripgrep
echo "Searching for MarkdownImageBlockRenderer class:"
rg -l "class MarkdownImageBlockRenderer"
echo -e "\nSearching for MarkdownImageBlockRenderer implementation details:"
rg "class MarkdownImageBlockRenderer" -A 10
Length of output: 3607
Script:
#!/bin/bash
# Get the complete implementation of MarkdownImageBlockRenderer
echo "Complete MarkdownImageBlockRenderer implementation:"
cat src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRenderer.java
Length of output: 1372
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownRelativeToAbsolutePathAttributeProvider.java (2)
1-7: LGTM! Clean and well-organized imports.
Package structure and imports follow best practices with specific imports and proper organization.
1-32: Verify coverage of all announcement email rendering requirements.
While the implementation handles relative to absolute URL conversion, let's verify if this covers all the requirements mentioned in the PR objectives regarding attachments and references.
✅ Verification successful
Let me gather more information about the email rendering implementation to verify the requirements.
Implementation correctly handles URL transformations for email rendering
The implementation is verified to be complete and working as intended:
- The
MarkdownRelativeToAbsolutePathAttributeProvideris properly integrated into theMailServicealong with other complementary components:- Works alongside
MarkdownImageBlockRendererfor image handling - Used in conjunction with
MarkdownCustomLinkRendererServicefor link transformations
- Works alongside
- The implementation handles both attachments and references through:
- Converting relative URLs to absolute ones for links (
hrefattributes) - Supporting image blocks through dedicated renderer
- Proper base URL injection from the server configuration
- Converting relative URLs to absolute ones for links (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files to ensure all scenarios are covered
fd -e java -e kt "Test.*\.java$|.*Test\.java$" | grep -i "announcement\|email\|notification"
# Search for attachment and reference related code
rg -t java "attachment|reference" --glob "src/main/**/notification/**"
Length of output: 6401
Script:
#!/bin/bash
# Check for email rendering related tests and implementations
rg -t java "MarkdownRelativeToAbsolutePathAttributeProvider" -B 2 -A 5
# Look for other URL or path transformation logic in notifications
rg -t java "baseUrl|setAttributes|href" --glob "src/main/**/notifications/**"
# Check for markdown rendering tests
ast-grep --pattern 'class $_ {
$$$
@Test
$_() {
$$$
MarkdownRelativeToAbsolutePathAttributeProvider $$$
$$$
}
}'
Length of output: 7239
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRenderer.java (3)
1-12: LGTM! Package and imports are well-organized.
The package naming and import statements follow Java best practices with specific imports.
24-27: LGTM! Clean and efficient implementation.
The method correctly specifies that this renderer handles Image nodes using an immutable set.
1-44: Verify integration with MailService.
Let's verify that this renderer is properly integrated with the email service and handles all image scenarios correctly.
✅ Verification successful
Let me check the integration details by examining the MailService implementation and the factory class.
Let me check how the image URLs are handled in the rest of the codebase to ensure this implementation aligns with the existing patterns.
Integration verified and implementation looks correct
The implementation is properly integrated and works as intended:
- MailService correctly uses the renderer through MarkdownImageBlockRendererFactory
- The renderer handles image URLs by prepending the artemisServerUrl
- The implementation gracefully handles both cases: when image has text (through firstChild) and fallback to destination
- The factory properly initializes the renderer with the required baseUrl
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of MarkdownImageBlockRenderer in MailService
echo "Checking MailService integration..."
rg -l "MarkdownImageBlockRenderer" "src/main/java/de/tum/cit/aet/artemis/communication/service/"
# Search for test coverage
echo "Checking test coverage..."
fd -e java -e kt "MarkdownImageBlockRenderer.*Test" "src/test/"
Length of output: 607
Script:
#!/bin/bash
# Check MailService implementation
echo "Checking MailService implementation..."
rg -A 10 "MarkdownImageBlockRenderer" "src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java"
# Check factory implementation
echo "Checking factory implementation..."
cat "src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java"
Length of output: 1657
Script:
#!/bin/bash
# Search for image URL handling patterns
echo "Checking image URL handling patterns..."
rg -g '*.java' -B 2 -A 2 'getDestination\(\)' --no-filename
# Check if there are any existing image-related tests
echo "Checking existing image-related tests..."
rg -g '*Test.java' -l 'Image|image.*attachment'
Length of output: 3431
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomReferenceRendererServiceTest.java (1)
1-9: LGTM! Clean and minimal imports.
The package structure and imports follow best practices with static imports for assertions and specific JUnit 5 annotations.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java (1)
1-71: Test implementation follows coding guidelines.
The test class demonstrates good compliance with the project's testing guidelines:
- Descriptive test names following behavior-driven style
- Small, focused test methods
- Proper use of JUnit 5 features
- AssertJ assertions with specific checks
- Fixed test data
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java (1)
43-70: Verify rendering behavior with integration tests
While the implementation looks correct, we should verify the rendering behavior with integration tests.
✅ Verification successful
Tests already cover the rendering functionality comprehensively
The implementation is already well-tested with unit tests that cover:
- Basic tag rendering with proper URL construction
- Multiple tag rendering in the same content
- Handling of unsupported tags
- Edge cases (missing links, empty input)
- Integration with MailService
The test cases align perfectly with the implementation's objectives and verify the proper handling of announcement email rendering issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for existing integration tests that cover this functionality
# Search for test files related to announcements and emails
echo "Searching for related test files..."
fd -e java -e groovy -e kt test announcement email
# Search for specific test patterns
echo "Searching for specific test patterns..."
rg -l "announcement.*email" -t java -t groovy -t kt
# Look for existing markdown rendering tests
echo "Searching for markdown rendering tests..."
rg -l "markdown.*render" -t java -t groovy -t kt
Length of output: 626
Script:
#!/bin/bash
# Let's try a different approach to find test files
# Search for test files related to MarkdownCustomLinkRendererService
echo "Searching for test files related to MarkdownCustomLinkRendererService..."
fd -e java "MarkdownCustomLinkRendererService.*Test"
# Search for test files containing "markdown" in their name
echo "Searching for markdown-related test files..."
fd -e java ".*[Mm]arkdown.*Test"
# Look for test files that might test this functionality
echo "Searching for test content..."
rg -l --type java "class.*Test.*Markdown|class.*Test.*Notification|class.*Test.*Email"
# Search for usage of the renderer service in tests
echo "Searching for renderer service usage in tests..."
rg -l --type java "MarkdownCustomLinkRendererService" "src/test"
Length of output: 1331
Script:
#!/bin/bash
# Let's examine the content of the test files we found
# Check the MarkdownCustomLinkRendererServiceTest content
echo "Content of MarkdownCustomLinkRendererServiceTest.java:"
cat "src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java"
# Check how it's used in MailServiceTest
echo -e "\nRelevant parts of MailServiceTest.java:"
rg -A 5 -B 5 "MarkdownCustomLinkRendererService" "src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java"
Length of output: 4452
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java (1)
1-72: Verify complete implementation of PR objectives
While this class handles reference rendering, the PR objectives mention fixing both references and attachments in announcement emails. Please verify:
- Where is the attachment rendering implementation?
- How does this service integrate with the email rendering pipeline?
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (2)
34-35: LGTM: Import statements are properly organized.
The new imports align with the PR's objective of improving email rendering capabilities.
Line range hint 1-170: Verify integration with related components.
To ensure proper integration of the new rendering services, let's verify the related components:
✅ Verification successful
Integration is properly implemented and tested
The verification shows that:
- Both renderer services are properly integrated into
MailServicethrough constructor injection and stored inmarkdownCustomRendererServiceslist - Both services have comprehensive test coverage with dedicated test classes
MarkdownCustomLinkRendererServiceTestcovers URL rendering scenariosMarkdownCustomReferenceRendererServiceTestcovers user and channel reference rendering- Error cases and edge conditions are properly tested in both services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper integration of renderer services
# Check for test coverage in related test files
echo "Checking for related test files..."
fd -e java -p "MarkdownCustom.*RendererServiceTest"
# Check for proper usage of renderer services in MailService
echo "Checking MailService implementation..."
rg -A 5 "class MailService" src/main/java
Length of output: 1237
Script:
#!/bin/bash
# Let's check the implementation details of MailService and its integration with renderer services
# Check how MailService uses the renderer services
echo "Checking MailService usage of renderer services..."
rg -A 10 "MarkdownCustom.*RendererService" src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
# Check if renderer services are properly tested
echo "Checking renderer service test implementations..."
cat src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomLinkRendererServiceTest.java
echo "---"
cat src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MarkdownCustomReferenceRendererServiceTest.java
Length of output: 6196
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
...m/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java
Show resolved
Hide resolved
...m/cit/aet/artemis/communication/service/notifications/MarkdownImageBlockRendererFactory.java
Show resolved
Hide resolved
...m/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java
Show resolved
Hide resolved
...m/cit/aet/artemis/communication/service/notifications/MarkdownCustomLinkRendererService.java
Show resolved
Hide resolved
.../aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java
Show resolved
Hide resolved
.../aet/artemis/communication/service/notifications/MarkdownCustomReferenceRendererService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
alekspetrov9e
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
…n-announcement-e-mails
JohannesStoehr
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
…n-announcement-e-mails
krusche
changed the title
Communication: Fix announcement e-mails not rendering attachments and references properlyCommunication: Fix announcement emails not rendering correctly
…n-announcement-e-mails
…n-announcement-e-mails
krusche
deleted the
bugfix/communication/render-attachments-in-announcement-e-mails
branch
Checklist
General
Server
Motivation and Context
Currently, attachments and references are not properly displayed in announcement e-mails. They are shown as text with their text (e.g. [programming]Test(/link/to/exercise)[/programming]).
Description
I added a custom parser that matches the tags and replaces them accordingly. Additionally, for images and attachments I added absolute links instead of relative ones to make them display properly. User references should just display their names.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests