General: Add more details to login email
#10934
Conversation
github-actions
bot
added
tests
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
github-actions
bot
added
the
exam
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
florian-glombik
dismissed stale reviews from jfr2102 and coderabbitai[bot]
via
38402bd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/java/de/tum/cit/aet/artemis/core/service/util/HttpRequestUtilsTest.java (2)
107-126: Well-structured browser test data provider.The comment explaining why @CsvSource isn't used is helpful, and the test data covers a comprehensive range of browsers with realistic User-Agent and Sec-Ch-Ua header combinations.
178-183: Comprehensive OS detection test data.Good coverage of major operating systems with realistic user agent strings.
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/core/service/util/HttpRequestUtilsTest.java (2)
147-149: Consider expanding Artemis app test coverage.While the basic iOS and Android app detection is covered, consider adding more test cases for edge scenarios like different app versions or malformed app user agents.
private static Stream<Arguments> provideAppTestData() { - return Stream.of(Arguments.of("Artemis/20250524013147 CFNetwork/3826.500.131 Darwin/24.5.0", ArtemisApp.IOS), Arguments.of("ktor-client", ArtemisApp.ANDROID)); + return Stream.of( + Arguments.of("Artemis/20250524013147 CFNetwork/3826.500.131 Darwin/24.5.0", ArtemisApp.IOS), + Arguments.of("Artemis/20240101000000 CFNetwork/1234.567.890 Darwin/23.0.0", ArtemisApp.IOS), + Arguments.of("ktor-client", ArtemisApp.ANDROID), + Arguments.of("ktor-client/1.0.0", ArtemisApp.ANDROID)); }
185-197: Clarify the purpose of Sec-Ch-Ua header in OS detection tests.The test sets a Sec-Ch-Ua header but doesn't explain why this is necessary for OS detection. Consider adding a comment or removing it if not needed.
@ParameterizedTest @MethodSource("provideOperatingSystemTestData") void shouldDetectOperatingSystem(String userAgent, OperatingSystem expectedOperatingSystem) { HttpServletRequest request = mock(HttpServletRequest.class); + // Sec-Ch-Ua header is set to ensure browser detection logic doesn't interfere with OS detection when(request.getHeader("Sec-Ch-Ua")).thenReturn("\"Chromium\";v=\"136\", \"Google Chrome\";v=\"136\", \"Not.A/Brand\";v=\"99\""); when(request.getHeader(HttpHeaders.USER_AGENT)).thenReturn(userAgent);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/AuthenticationMethod.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ArtemisSuccessfulLoginService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/util/ClientEnvironment.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/util/HttpRequestUtils.java(2 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/main/resources/i18n/messages_de.properties(1 hunks)src/main/resources/i18n/messages_en.properties(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/util/HttpRequestUtilsTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/resources/i18n/messages.properties
- src/main/resources/i18n/messages_en.properties
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/AuthenticationMethod.java
- src/main/resources/i18n/messages_de.properties
- src/main/java/de/tum/cit/aet/artemis/core/util/HttpRequestUtils.java
- src/main/java/de/tum/cit/aet/artemis/core/util/ClientEnvironment.java
- src/main/java/de/tum/cit/aet/artemis/core/service/ArtemisSuccessfulLoginService.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/core/service/util/HttpRequestUtilsTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (5)
src/test/java/de/tum/cit/aet/artemis/core/service/util/HttpRequestUtilsTest.java (5)
8-23: LGTM! Clean imports and good organization.The new imports are well-organized and include all necessary testing utilities for the comprehensive client environment testing functionality.
27-97: Excellent reorganization of IP address tests.The existing IP address parsing tests have been properly organized into a nested
IpAddressTestsclass with good test coverage including IPv4, IPv6, loopback addresses, and edge cases for invalid inputs.
128-140: Excellent parameterized browser detection test.This addresses the past review comment about writing parameterized tests for browser detection. The test properly validates browser detection while ensuring Artemis app detection returns null for browser requests.
199-205: Good edge case coverage for unknown user agents.The test properly handles the edge case of unknown user agents returning null, which is important for robustness.
165-171: Excellent null user agent handling test.This test ensures the method gracefully handles null user agent headers without throwing exceptions.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
florian-glombik
dismissed stale reviews from jfr2102, tobias-lippert, and coderabbitai[bot]
via
b3c9f43
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
florian-glombik
moved this from Ready For Review
to Developer Approved
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
Checklist
General
Server
Motivation and Context
Add more information to login emails to ensure users can recognize more easily if a suspicious action has taken place.
Description
HttpHeadervariableSteps for Testing
Prerequisites:
Note: ts4 does not seem to have a working mail server, you might want to user the other test servers
Please state the Browser + OS that you have tested it with and add a screenshot of the mail that you have received.
You can also test it with the Android app https://github.com/ls1intum/artemis-android or the ios app https://apps.apple.com/de/app/artemis-learning/id6478965616
You can try to break it with Browser + OS combinations (and using german and english language settings)
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests