Communication: Improve push notifications for the iOS mobile app
#9787
Conversation
github-actions
bot
added
server
github-actions
bot
added
tests
core
PaRangger
changed the title
Development: Add changes for new iOS notification systemNotifications: Add changes for new iOS notification system
…ion/add-new-ios-api-to-notifications
…ion/add-new-ios-api-to-notifications
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces modifications across several classes related to notification handling in the application. Key changes include the addition of new parameters for image URLs and identifiers in notification creation methods, updates to method signatures, and the introduction of a new enum for push notification API types. The changes enhance the detail and contextual information in notifications without altering the fundamental control flow or error handling. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Outside diff range and nitpick comments (9)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java (1)
3-3: Consider usingPushNotificationApiTypeinstead ofshortforapiTypeUsing
shortfor theapiTypereduces type safety and may lead to errors. Consider using thePushNotificationApiTypeenum directly to enhance clarity and type safety.Apply this diff to update the record definition:
-public record RelayNotificationRequest(String initializationVector, String payloadCipherText, String token, short apiType) { +public record RelayNotificationRequest(String initializationVector, String payloadCipherText, String token, PushNotificationApiType apiType) {src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java (3)
Line range hint
121-134: Update equals method to include apiType fieldThe equals method should be updated to include the new
apiTypefield for proper entity comparison.Apply this diff:
PushNotificationDeviceConfiguration that = (PushNotificationDeviceConfiguration) object; // Use compareTo rather than equals for dates to ensure timestamps and dates with the same time are considered equal // This is caused by Java internal design having different classes for Date (java.util) and Timestamp (java.sql) - return token.equals(that.token) && deviceType == that.deviceType && expirationDate.compareTo(that.expirationDate) == 0 && Arrays.equals(secretKey, that.secretKey) - && owner.equals(that.owner); + return token.equals(that.token) && deviceType == that.deviceType && expirationDate.compareTo(that.expirationDate) == 0 + && Arrays.equals(secretKey, that.secretKey) && owner.equals(that.owner) + && apiType == that.apiType;
Line range hint
136-140: Update hashCode method to include apiType fieldThe hashCode method should be updated to maintain consistency with equals.
Apply this diff:
@Override public int hashCode() { - int result = Objects.hash(token, deviceType, expirationDate, owner); + int result = Objects.hash(token, deviceType, expirationDate, owner, apiType); result = 31 * result + Arrays.hashCode(secretKey); return result; }
Line range hint
142-145: Update toString to include apiType fieldThe toString method should include the new field for better debugging.
Apply this diff:
@Override public String toString() { return "PushNotificationDeviceConfiguration{" + "token='" + token + '\'' + ", deviceType=" + deviceType + ", expirationDate=" + expirationDate + ", secretKey=" - + Arrays.toString(secretKey) + ", owner=" + owner + '}'; + + Arrays.toString(secretKey) + ", owner=" + owner + ", apiType=" + apiType + '}'; }src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.java (1)
108-110: Add logging for apiType registrationConsider adding debug logging for the apiType being registered to aid in troubleshooting.
Add this before saving the configuration:
PushNotificationDeviceConfiguration deviceConfiguration = new PushNotificationDeviceConfiguration(pushNotificationRegisterBody.token(), pushNotificationRegisterBody.deviceType(), expirationDate, newKey.getEncoded(), user, apiType); + log.debug("Registering device with apiType: {}", apiType); pushNotificationDeviceConfigurationRepository.save(deviceConfiguration);src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (1)
87-90: Consider null-safe image URL handling using OptionalThe current implementation uses a ternary operator for null checking. Consider using Optional for more idiomatic null handling.
-var imageUrl = createdMessage.getAuthor().getImageUrl() == null ? "" : createdMessage.getAuthor().getImageUrl(); +var imageUrl = Optional.ofNullable(createdMessage.getAuthor().getImageUrl()).orElse("");src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java (1)
160-162: Consider extracting notification data creation to a separate methodThe creation of notification data could be extracted to improve readability and testability.
+private PushNotificationData createNotificationData(Notification notification, String date) { + return new PushNotificationData( + notification.getTransientPlaceholderValuesAsArray(), + notification.getTarget(), + NotificationType.findCorrespondingNotificationType(notification.getTitle()).name(), + date, + Constants.PUSH_NOTIFICATION_VERSION + ); +}src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
379-383: LGTM! Consider enhancing version documentation.The addition of
PUSH_NOTIFICATION_MINOR_VERSIONfollows semantic versioning principles and aligns with the goal of supporting multiple iOS versions.Consider adding more detailed documentation about version compatibility:
/** - * The value of the version field we send with each push notification to the native clients (Android & iOS). + * The value of the minor version field we send with each push notification to the native clients. + * Minor version changes indicate backward-compatible enhancements to the notification payload. + * Current version mapping: + * - 2: Added support for iOS 18 notification payload + * - 1: Initial version supporting iOS 17 and earlier */src/test/resources/placeholder-signatures.json (1)
114-125: Consider consolidating duplicate field structures.Multiple notification types (CONVERSATION_NEW_REPLY_MESSAGE, CONVERSATION_USER_MENTIONED, NEW_REPLY_FOR_*_POST) share identical field structures. This repetition could be simplified using a shared schema or type.
Consider creating a common reply notification structure that can be reused:
{ "replyNotificationFields": { "imageUrl": "java.lang.String", "userId": "java.lang.String", "postingId": "java.lang.String", "parentPostId": "java.lang.String" } }Also applies to: 174-185, 402-413, 441-452, 480-491, 519-530
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241114122713_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationApiType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/PushNotificationRegisterBody.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/GroupNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/GroupNotificationFactoryTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/service/PushNotificationDeviceConfigurationCleanupServiceTest.java(2 hunks)src/test/resources/placeholder-signatures.json(8 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/GroupNotificationService.java
- src/test/java/de/tum/cit/aet/artemis/communication/notification/GroupNotificationFactoryTest.java
- src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
🧰 Additional context used
📓 Path-based instructions (16)
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.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/service/PushNotificationDeviceConfigurationCleanupServiceTest.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/main/java/de/tum/cit/aet/artemis/core/config/Constants.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/notification/NotificationPlaceholderSignatureTest.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/push_notifications/AppleFirebasePushNotificationServiceTest.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/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.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/ConversationNotificationService.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/notification/GroupNotificationFactoryTest.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/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.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/domain/notification/GroupNotificationFactory.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/GroupNotificationService.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/dto/PushNotificationRegisterBody.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/push_notifications/PushNotificationService.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/domain/push_notification/PushNotificationDeviceConfiguration.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/domain/push_notification/PushNotificationApiType.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/domain/notification/SingleUserNotificationFactory.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
🔇 Additional comments (15)
src/main/java/de/tum/cit/aet/artemis/communication/dto/PushNotificationRegisterBody.java (1)
6-10: Verify that JSON serialization and deserialization handle the new apiType field correctly
The addition of the apiType field and overloaded constructors may affect JSON serialization/deserialization. Ensure that all serialization frameworks handle the new field appropriately.
Run the following script to check for usages of PushNotificationRegisterBody in serialization contexts:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
JSON serialization of PushNotificationRegisterBody is handled correctly by Spring's default mechanisms
The code is safe because:
- The class is a Java record, which automatically provides proper serialization/deserialization behavior
- Integration tests in
PushNotificationResourceTestverify the serialization works correctly through REST endpoints - The
apiTypefield has a default value through the secondary constructor, ensuring backward compatibility - Spring Boot's default ObjectMapper configuration handles enum serialization automatically
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of PushNotificationRegisterBody in REST endpoints and check for serialization annotations.
# Search in REST controllers for methods using PushNotificationRegisterBody
rg '@RestController' -A 10 | rg 'PushNotificationRegisterBody' -A 5 | rg '@RequestBody'
# Check for JsonCreator and JsonProperty annotations in PushNotificationRegisterBody
rg 'PushNotificationRegisterBody' -A 5 | rg '@JsonCreator|@JsonProperty'
Length of output: 156
Script:
#!/bin/bash
# Let's try a different approach to find usages and serialization handling
# Find all files containing PushNotificationRegisterBody
echo "=== Files containing PushNotificationRegisterBody ==="
rg -l "PushNotificationRegisterBody"
# Look for REST endpoints or controller methods using this class
echo -e "\n=== REST endpoint usages ==="
rg -B 5 -A 5 "PushNotificationRegisterBody.*@RequestBody|@RequestBody.*PushNotificationRegisterBody"
# Check the record class implementation and any annotations
echo -e "\n=== Record class implementation ==="
rg -B 3 -A 10 "record PushNotificationRegisterBody"
# Look for test cases that might involve serialization
echo -e "\n=== Test cases ==="
rg -B 3 -A 5 "PushNotificationRegisterBody" "*Test.java"
Length of output: 3648
Script:
#!/bin/bash
# Let's check the test file specifically and look for serialization tests
echo "=== Test file content ==="
cat src/test/java/de/tum/cit/aet/artemis/communication/notification/PushNotificationResourceTest.java
echo -e "\n=== Check for ObjectMapper or similar JSON handling ==="
rg -B 3 -A 3 "ObjectMapper|@Test.*PushNotificationRegisterBody" src/test/java/de/tum/cit/aet/artemis/communication/notification/PushNotificationResourceTest.java
# Let's also check if there are any custom serializers/deserializers
echo -e "\n=== Check for custom serializers/deserializers ==="
rg -l "JsonSerializer|JsonDeserializer"
Length of output: 4915
Script:
#!/bin/bash
# Let's check the test infrastructure and Spring configuration for JSON handling
# Check the base test class for ObjectMapper configuration
echo "=== Base Test Class Configuration ==="
cat src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java
# Check for Spring test configuration files
echo -e "\n=== Spring Test Configuration ==="
fd -e yml -e yaml -e properties . src/test/resources/
# Check if there are any custom configurations for ObjectMapper in main application
echo -e "\n=== Main Application Configuration ==="
rg -l "ObjectMapper|@JsonTest|@SpringBootTest" src/main/java/
Length of output: 21156
src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationApiType.java (1)
1-23: LGTM!
The implementation of the PushNotificationApiType enum is clean and follows best practices.
src/test/java/de/tum/cit/aet/artemis/communication/service/PushNotificationDeviceConfigurationCleanupServiceTest.java (1)
45-48: LGTM!
The test updates correctly reflect the changes in the PushNotificationDeviceConfiguration constructor.
src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java (3)
38-40: LGTM: Field annotations are correctly applied
The apiType field is properly annotated with @Enumerated and @Column for JPA persistence.
61-69: LGTM: Constructor maintains backward compatibility
The new constructor overload properly handles the apiType parameter while maintaining backward compatibility through the existing constructor.
115-117: LGTM: Getter method follows Java conventions
The getApiType method follows Java getter naming conventions and provides proper access to the field.
src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java (1)
33-38: LGTM: Documentation improvements enhance clarity
The updated documentation clearly distinguishes between breaking and non-breaking changes, providing better guidance for version management.
src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.java (1)
104-105: LGTM: Proper null handling with default value
The code correctly handles null apiType by defaulting to PushNotificationApiType.DEFAULT.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java (2)
31-31: LGTM: Import added for new enum type
The import statement for PushNotificationApiType is correctly added to support the new API type functionality.
74-76: LGTM: Appropriate API types assigned in test setup
The test setup correctly initializes the device configurations with appropriate API types:
PushNotificationApiType.IOS_V2for Apple devicesPushNotificationApiType.DEFAULTfor Firebase devices
This aligns with the PR objective of supporting the new iOS notification system while maintaining compatibility.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (1)
98-99: Verify parameter ordering consistency
The parameter ordering in the method signature has been updated to include new fields. Ensure that the order of parameters in the placeholders array matches the order expected by the mobile client.
✅ Verification successful
Let me search for more context about how these placeholders are used in notifications and mobile clients.
Let me search for the notification text templates to see how these placeholders are used.
Let me try one more search to find the i18n files with a different approach.
Let me check the content of these notification translation files to understand the placeholder usage.
Based on the i18n files, I can now generate the final response as I have found the template that uses these placeholders.
Parameter ordering is correct and consistent
The placeholders in the array match the expected order in the notification template:
courseTitle→{{ placeholderValues.0 }}messageContent→{{ placeholderValues.1 }}messageCreationDate→{{ placeholderValues.2 }}conversationName→{{ placeholderValues.3 }}authorName→{{ placeholderValues.4 }}conversationType→{{ placeholderValues.5 }}imageUrl→{{ placeholderValues.6 }}userId→{{ placeholderValues.7 }}postId→{{ placeholderValues.8 }}
The order in the array matches how they are used in the notification templates for both English and German translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of these placeholders in the mobile client code
rg -A 5 "createPlaceholdersNewMessageChannelText"
Length of output: 2578
Script:
#!/bin/bash
# Search for notification text templates or constants that might use these placeholders
rg -A 5 "NEW_MESSAGE_CHANNEL_TEXT"
# Search for any mobile-related notification handling code
rg -A 5 "MobileNotification"
# Look for any notification template or message pattern definitions
ast-grep --pattern 'String[] NOTIFICATION_PLACEHOLDERS = $_'
Length of output: 5037
Script:
#!/bin/bash
# Search for the notification text template in i18n files
fd -e json -e properties . | xargs rg -A 5 "artemisApp.conversationNotification.text.newMessageChannel"
# Look for any mobile notification handling code in Java
ast-grep --pattern 'class $_ implements MobileNotification { $$$ }'
Length of output: 767
Script:
#!/bin/bash
# Search for i18n files in src directory
fd i18n src
# If we find i18n files, let's look inside them for the notification text
rg -l "conversationNotification" $(fd i18n src)
# Also search for any mobile-specific notification classes
rg -A 5 "class.*Notification.*Mobile"
Length of output: 413
Script:
#!/bin/bash
# Check the content of notification files
cat src/main/webapp/i18n/en/notification.json
cat src/main/webapp/i18n/de/notification.json
# Also look for any notification template definitions
rg "artemisApp.conversationNotification.text" src/main/webapp/i18n/
Length of output: 29943
src/test/resources/placeholder-signatures.json (2)
78-86: LGTM! New fields added to CONVERSATION_NEW_MESSAGE.
The addition of imageUrl, userId, and postId fields enhances the notification payload with necessary metadata.
333-341: LGTM! New fields added to NEW_ANNOUNCEMENT_POST.
The addition of imageUrl, authorId, and postId fields is consistent with the notification enhancement pattern.
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.java (2)
245-247: LGTM! Proper null handling for imageUrl.
The code correctly handles null imageUrl by defaulting to an empty string.
254-256: LGTM! Updated method signature with new parameters.
The method signature update aligns with the changes in placeholder-signatures.json.
.../de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java
Show resolved
Hide resolved
.../artemis/communication/service/notifications/push_notifications/PushNotificationService.java
Show resolved
Hide resolved
WalkthroughThe pull request introduces multiple modifications across several classes related to notification handling within the application. Key changes include the enhancement of notification data structures by adding new parameters such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationApiType.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
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
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java (1)
218-218: Consider using a test utility for ID generationThe pattern of manually setting IDs for test objects could be improved by using a test utility method. This would make the tests more maintainable and reduce duplication.
Consider creating a utility method like:
- post.setId(1L); - answerPostPost.setId(2L); - answerPost.setId(1L); + TestUtil.setTestEntityId(post, 1L); + TestUtil.setTestEntityId(answerPostPost, 2L); + TestUtil.setTestEntityId(answerPost, 1L);Add this utility method to your test utilities:
public class TestUtil { public static void setTestEntityId(DomainObject entity, Long id) { entity.setId(id); } }Also applies to: 227-227, 229-229
src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java (1)
33-38: Documentation improvements look good, but could be even more helpful with examples.The split between breaking and non-breaking changes is well documented. This distinction is particularly important given the PR's focus on iOS notification system changes.
Consider adding examples of what constitutes breaking vs non-breaking changes to make the documentation more actionable. For example:
* 2a. In case of breaking changes, increment the {{@link Constants#PUSH_NOTIFICATION_VERSION}}. This ensures that old versions of the native apps discard your new - * notifications. + * notifications. Example: Removing or renaming existing fields, changing field types. * 2b. In case of non-breaking changes, increment the {{@link Constants#PUSH_NOTIFICATION_MINOR_VERSION}} for better traceability + * Example: Adding new optional fields while maintaining backward compatibility.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java (2)
3-3: Add Javadoc documentation for the record and its fields.Please add documentation explaining:
- The purpose of this record
- The meaning and format of each field
- Valid values for apiType
- Any validation rules or constraints
+/** + * Represents a request to relay a push notification to a device. + * + * @param initializationVector The initialization vector used for encryption + * @param payloadCipherText The encrypted notification payload + * @param token The device token for notification delivery + * @param apiType The notification API version to use (e.g., DEFAULT or IOS_V2) + */ public record RelayNotificationRequest(String initializationVector, String payloadCipherText, String token, short apiType) {
3-3: Add validation for required fields.Consider adding validation to ensure that required fields are not null or empty. This can be done using a compact constructor.
public record RelayNotificationRequest(String initializationVector, String payloadCipherText, String token, short apiType) { + public RelayNotificationRequest { + if (initializationVector == null || initializationVector.isEmpty()) { + throw new IllegalArgumentException("initializationVector must not be null or empty"); + } + if (payloadCipherText == null || payloadCipherText.isEmpty()) { + throw new IllegalArgumentException("payloadCipherText must not be null or empty"); + } + if (token == null || token.isEmpty()) { + throw new IllegalArgumentException("token must not be null or empty"); + } + } }src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationApiType.java (1)
1-23: Add JavaDoc comments for enum and methodsConsider adding JavaDoc comments to the
PushNotificationApiTypeenum and its methods (getDatabaseKey(),fromDatabaseKey()) to enhance code readability and provide clear documentation for other developers.src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java (1)
38-41: AnnotateapiTypewith@Enumerated(EnumType.STRING)Currently,
apiTypeis annotated with@Enumeratedwithout specifying theEnumType. It's recommended to explicitly define theEnumTypefor clarity and to avoid potential issues with the database representation.Apply this diff to specify the enum type:
@Enumerated +@Enumerated(EnumType.STRING) @Column(name = "api_type") private PushNotificationApiType apiType;Alternatively, if you prefer to store the enum ordinal:
@Enumerated +@Enumerated(EnumType.ORDINAL) @Column(name = "api_type") private PushNotificationApiType apiType;Ensure that the chosen
EnumTypealigns with your database schema and data requirements.src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java (1)
74-74: LGTM! Consider adding edge cases.The test setup correctly differentiates between Apple and Firebase configurations. Consider adding test cases for:
- Handling null apiType
- Migration scenarios from DEFAULT to IOS_V2
Also applies to: 76-76
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (1)
87-87: LGTM! Consider extracting default value.The null check with empty string default is appropriate. Consider extracting the empty string to a constant for consistency.
+private static final String DEFAULT_IMAGE_URL = ""; -var imageUrl = createdMessage.getAuthor().getImageUrl() == null ? "" : createdMessage.getAuthor().getImageUrl(); +var imageUrl = createdMessage.getAuthor().getImageUrl() == null ? DEFAULT_IMAGE_URL : createdMessage.getAuthor().getImageUrl();src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
379-383: Add documentation for version compatibility.The new
PUSH_NOTIFICATION_MINOR_VERSIONconstant would benefit from documentation explaining the version compatibility matrix between iOS versions and notification payload versions.Add Javadoc explaining version compatibility:
/** * The value of the version field we send with each push notification to the native clients (Android & iOS). + * Version compatibility: + * - Major version 1: + * - Minor version 1: iOS 17 and earlier + * - Minor version 2: iOS 18+ */ public static final int PUSH_NOTIFICATION_MINOR_VERSION = 2;src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java (1)
160-162: Add null checks for new notification data fields.The new fields added to
PushNotificationDatashould be validated to handle potential null values gracefully.Consider adding null checks or using Optional for the new fields:
- var notificationData = new PushNotificationData(notification.getTransientPlaceholderValuesAsArray(), notification.getTarget(), type.name(), date, - Constants.PUSH_NOTIFICATION_VERSION); + var placeholders = notification.getTransientPlaceholderValuesAsArray(); + var target = Objects.requireNonNull(notification.getTarget(), "Notification target cannot be null"); + var typeName = type.name(); + var notificationData = new PushNotificationData(placeholders, target, typeName, date, + Constants.PUSH_NOTIFICATION_VERSION);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241114122713_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationApiType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/PushNotificationRegisterBody.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/GroupNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/GroupNotificationFactoryTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/service/PushNotificationDeviceConfigurationCleanupServiceTest.java(2 hunks)src/test/resources/placeholder-signatures.json(8 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/GroupNotificationService.java
- src/test/java/de/tum/cit/aet/artemis/communication/notification/GroupNotificationFactoryTest.java
🧰 Additional context used
📓 Path-based instructions (16)
src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.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/main/java/de/tum/cit/aet/artemis/core/config/Constants.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/notification/SingleUserNotificationServiceTest.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/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.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/push_notifications/AppleFirebasePushNotificationServiceTest.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/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.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/dto/PushNotificationRegisterBody.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/service/PushNotificationDeviceConfigurationCleanupServiceTest.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/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.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/domain/push_notification/PushNotificationDeviceConfiguration.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/GroupNotificationService.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/notification/GroupNotificationFactoryTest.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/main/java/de/tum/cit/aet/artemis/communication/domain/notification/GroupNotificationFactory.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/domain/push_notification/PushNotificationApiType.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/domain/notification/SingleUserNotificationFactory.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/ConversationNotificationService.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
🔇 Additional comments (7)
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java (1)
Line range hint 1-624: Verify test coverage for notification ID handling
The addition of explicit IDs suggests that notification handling might depend on these IDs. However, there are no explicit tests verifying this behavior.
Let's verify if there are any ID-dependent notification handling tests:
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java (1)
3-3: Verify the impact of this breaking change.
The addition of the apiType parameter is a breaking change that requires updates to all clients. Please ensure that:
- All constructor calls are updated
- Any serialization/deserialization code handles the new field
- Database migrations preserve existing data
✅ Verification successful
Let me gather more context about the changes and serialization handling.
Let me check one more aspect to understand if this is a new file and how the ApiType is defined.
Based on the gathered information, I can now provide the final response:
The apiType parameter addition is properly handled and doesn't require manual verification.
The change is safe because:
- The
apiTypeparameter is properly integrated with a default value (PushNotificationApiType.DEFAULT) in the registration flow - The only usage of
RelayNotificationRequestconstructor is inPushNotificationServicewhere it correctly passes theapiTypefrom the device configuration - The
PushNotificationApiTypeenum is properly persisted in the database with an@Enumeratedcolumn - The registration flow handles backward compatibility by making the
apiTypeparameter optional inPushNotificationRegisterBody, defaulting toPushNotificationApiType.DEFAULT
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing constructor calls that need to be updated
rg "new RelayNotificationRequest\(" --type java
# Search for any JSON serialization/deserialization of this class
rg "RelayNotificationRequest" -A 5 -B 5 --type java | rg -i "json|objectmapper|serialize|deserialize"
Length of output: 773
Script:
#!/bin/bash
# Check git history for this file to understand if this is a new file or modification
git log --oneline -- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java
# Look for any database related code for this class
rg "RelayNotificationRequest" -A 5 -B 5 --type java | rg -i "repository|entity|database|jpa"
# Check the implementation of getApiType() to understand the conversion
ast-grep --pattern 'getApiType()'
Length of output: 733
Script:
#!/bin/bash
# Find the ApiType enum or class definition
rg "ApiType" -A 10 -B 2 --type java
# Check if RelayNotificationRequest is a new file by looking for deleted lines
git diff HEAD~1 -- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java
Length of output: 27629
src/main/java/de/tum/cit/aet/artemis/communication/dto/PushNotificationRegisterBody.java (1)
6-10: Ensure default apiType maintains backward compatibility
Adding the apiType parameter to the record and providing a constructor that defaults it to PushNotificationApiType.DEFAULT ensures backward compatibility with existing code that doesn't specify apiType. This is a good practice to prevent breaking changes.
src/test/java/de/tum/cit/aet/artemis/communication/service/PushNotificationDeviceConfigurationCleanupServiceTest.java (1)
45-48: Update test constructors with new apiType parameter
The test cases have been appropriately updated to include the new apiType parameter in the PushNotificationDeviceConfiguration constructor. This ensures that the tests reflect the updated class structure and continue to verify the intended functionality.
src/main/java/de/tum/cit/aet/artemis/communication/web/PushNotificationResource.java (2)
27-27: LGTM! Good handling of backward compatibility.
The addition of PushNotificationApiType with a default fallback ensures smooth transition and maintains compatibility with existing clients.
Also applies to: 104-105
109-109: Verify constructor parameter ordering.
The constructor parameter ordering looks correct, but let's verify all usages to ensure consistency.
✅ Verification successful
Let me verify the constructor definition to ensure the parameter ordering matches all usages.
Let me try a different approach to find the constructor definition.
Constructor parameter ordering is correct
All constructor calls in the codebase match the parameter order defined in the class: token, deviceType, expirationDate, secretKey, owner, apiType. The implementation in PushNotificationResource.java correctly follows this order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all PushNotificationDeviceConfiguration constructor calls
ast-grep --pattern 'new PushNotificationDeviceConfiguration($$$)'
Length of output: 2710
Script:
#!/bin/bash
# Search for PushNotificationDeviceConfiguration class definition
ast-grep --pattern 'class PushNotificationDeviceConfiguration {
$$$
PushNotificationDeviceConfiguration($$$) {
$$$
}
$$$
}'
Length of output: 261
Script:
#!/bin/bash
# Search for PushNotificationDeviceConfiguration class file and show its content
fd PushNotificationDeviceConfiguration.java --exec cat {}
Length of output: 4447
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java (1)
173-174: Consider version compatibility in payload construction.
The code should handle different payload versions based on the device's iOS version.
Consider implementing a version-specific payload builder:
- Create a PayloadBuilder interface
- Implement version-specific builders (iOS17PayloadBuilder, iOS18PayloadBuilder)
- Use the appropriate builder based on device configuration
...ava/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java
Show resolved
Hide resolved
...artemis/communication/service/notifications/push_notifications/RelayNotificationRequest.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java
Show resolved
Hide resolved
.../aet/artemis/communication/domain/push_notification/PushNotificationDeviceConfiguration.java
Show resolved
Hide resolved
...tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java
Show resolved
Hide resolved
|
After more intense testing with iOS versions 1.5.1 and 1.4.1, everything seems to work as expected – but "Exercise released" notifications are sent twice (they even show up twice in the notification sheet inside the app, so this is stored like that on the server). Seems like this is unrelated to this PR as I was able to reproduce the same thing on another Test Server |
PaRangger
added
deploy:artemis-test3
deploy:artemis-test2
and removed
deploy:artemis-test3
deploy:artemis-test2
labels
…ion/add-new-ios-api-to-notifications
…s-api-to-notifications' into feature/communication/add-new-ios-api-to-notifications
PaRangger
dismissed stale reviews from coderabbitai[bot] and anian03
via
e6792b8
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Notifications: Add changes for new iOS notification systemCommunication: Improve push notifications for the iOS mobile app
krusche
deleted the
feature/communication/add-new-ios-api-to-notifications
branch
Checklist
General
Server
Motivation and Context
Currently, the notifications in iOS are running as background notifications. To change this, we need to send different payloads to iOS18 devices, while keeping the iOS17 and lower devices the same.
Description
I added a column to the push_notification_device_configuration table, which tells the system what version of payload to send.
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
Performance Review
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores