[Feat/user device token] - 알림을 위한 디바이스 토큰수집#104
Conversation
|
Warning Rate limit exceeded@ekgns33 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis update introduces device token and platform support during user registration. It adds new fields and validation to the signup request, creates a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant SignUpUsecaseImpl
participant UserRegisterService
participant UserCreator
participant UserDeviceTokenRepository
Client->>AuthController: POST /signup (registerToken, nickname, deviceToken, devicePlatform)
AuthController->>SignUpUsecaseImpl: register(AuthSignupRequest)
SignUpUsecaseImpl->>UserRegisterService: registerUser(UserRegisterCommand)
UserRegisterService->>UserCreator: createUser(...)
UserRegisterService->>UserCreator: createUserDeviceToken(user, DeviceTokenDto)
UserCreator->>UserDeviceTokenRepository: save(UserDeviceToken)
UserRegisterService-->>SignUpUsecaseImpl: User created
SignUpUsecaseImpl-->>AuthController: SignupUserResponse
AuthController-->>Client: HTTP 201 Created
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 comments (1)
src/test/resources/sql/schema.sql (1)
217-218: Remove duplicate foreign key constraint.The foreign key constraint for
user_token.user_idis already defined in the table creation (line 51). This duplicate constraint will cause a SQL error.Remove the duplicate constraint:
-ALTER TABLE `user_token` - ADD FOREIGN KEY (`user_id`) REFERENCES `users` (`id`); -
🧹 Nitpick comments (4)
src/main/java/org/runimo/runimo/user/repository/UserDeviceTokenRepository.java (1)
8-10: Consider adding custom query methods for device token management.The repository interface is correctly implemented but could benefit from common query methods for device token operations.
Consider adding these useful query methods:
@Repository public interface UserDeviceTokenRepository extends JpaRepository<UserDeviceToken, Long> { - + List<UserDeviceToken> findByUserId(Long userId); + List<UserDeviceToken> findByUserIdAndPlatform(Long userId, DevicePlatform platform); + Optional<UserDeviceToken> findByDeviceToken(String deviceToken); + List<UserDeviceToken> findByUserIdAndNotificationAllowedTrue(Long userId); }src/main/java/org/runimo/runimo/user/service/UserCreator.java (1)
56-68: Consider making notification preference configurable.The implementation is correct and follows good practices with the early return and transaction annotation. However, the hardcoded
notificationAllowed(true)might not be flexible enough for future requirements.Consider making the notification preference configurable:
- public void createUserDeviceToken(User user, DeviceTokenDto deviceTokenDto) { + public void createUserDeviceToken(User user, DeviceTokenDto deviceTokenDto, boolean notificationAllowed) { if (deviceTokenDto.isEmpty()) { return; } UserDeviceToken userDeviceToken = UserDeviceToken.builder() .user(user) .deviceToken(deviceTokenDto.token()) .platform(deviceTokenDto.platform()) - .notificationAllowed(true) + .notificationAllowed(notificationAllowed) .build(); userDeviceTokenRepository.save(userDeviceToken); }Alternatively, add the notification preference to
DeviceTokenDtoif it should come from the client.src/main/java/org/runimo/runimo/auth/controller/request/AuthSignupRequest.java (2)
47-49: Consider using a more specific exception type.While the validation logic is correct, consider using a more specific exception type like
ValidationExceptionor creating a custom exception for better error handling and API consistency.- if (hasDeviceToken() && (devicePlatform == null || devicePlatform.trim().isEmpty())) { - throw new IllegalArgumentException("디바이스 토큰이 있으면 플랫폼도 필수입니다."); - } + if (hasDeviceToken() && (devicePlatform == null || devicePlatform.trim().isEmpty())) { + throw new ValidationException("디바이스 토큰이 있으면 플랫폼도 필수입니다."); + }
60-62: Simplify the helper method for better readability.The
hasDeviceToken()method can be simplified while maintaining the same functionality.- private boolean hasDeviceToken() { - return deviceToken != null && !deviceToken.trim().isEmpty(); - } + private boolean hasDeviceToken() { + return deviceToken != null && !deviceToken.isBlank(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/org/runimo/runimo/auth/controller/request/AuthSignupRequest.java(1 hunks)src/main/java/org/runimo/runimo/auth/service/SignUpUsecaseImpl.java(3 hunks)src/main/java/org/runimo/runimo/auth/service/dto/UserSignupCommand.java(1 hunks)src/main/java/org/runimo/runimo/user/domain/DevicePlatform.java(1 hunks)src/main/java/org/runimo/runimo/user/domain/UserDeviceToken.java(1 hunks)src/main/java/org/runimo/runimo/user/repository/UserDeviceTokenRepository.java(1 hunks)src/main/java/org/runimo/runimo/user/service/UserCreator.java(3 hunks)src/main/java/org/runimo/runimo/user/service/UserRegisterService.java(1 hunks)src/main/java/org/runimo/runimo/user/service/dto/command/DeviceTokenDto.java(1 hunks)src/main/java/org/runimo/runimo/user/service/dto/command/UserRegisterCommand.java(1 hunks)src/main/resources/sql/schema.sql(1 hunks)src/test/java/org/runimo/runimo/auth/controller/AuthAcceptanceTest.java(2 hunks)src/test/java/org/runimo/runimo/auth/controller/AuthControllerTest.java(3 hunks)src/test/resources/sql/schema.sql(1 hunks)
🔇 Additional comments (21)
src/main/java/org/runimo/runimo/user/domain/UserDeviceToken.java (2)
51-58: Missing status column mentioned in PR objectives.The PR objectives mention adding a status column to the
user_tokentable, but this entity doesn't include a status field. Please verify if this is intentional or if the status column should be added.#!/bin/bash # Check if status column exists in the database schema rg -A 10 -B 5 "user_token" src/main/resources/sql/schema.sql src/test/resources/sql/schema.sql
32-34: Let’s confirm whether astatusfield is defined in this entity:#!/bin/bash grep -n "status" src/main/java/org/runimo/runimo/user/domain/UserDeviceToken.java || echo "No status field found"src/main/java/org/runimo/runimo/user/service/UserRegisterService.java (1)
29-29: LGTM! Device token creation is properly integrated.The call to
createUserDeviceTokenis correctly placed after user creation and uses the appropriate parameters from the registration command.src/main/java/org/runimo/runimo/user/service/dto/command/UserRegisterCommand.java (1)
13-13: LGTM! Device token field properly added for backward compatibility.The
DeviceTokenDto deviceTokenfield is correctly added as optional (no@NotNullannotation), which maintains backward compatibility with existing registration flows.src/main/java/org/runimo/runimo/auth/service/dto/UserSignupCommand.java (2)
4-4: LGTM!The import for
DevicePlatformis correctly added to support the new field.
12-14: LGTM!The device token fields are appropriately added to the record. The field types and positioning are consistent with the existing pattern.
src/main/java/org/runimo/runimo/user/service/UserCreator.java (1)
8-8: LGTM!The new imports and repository dependency are correctly added to support device token functionality.
Also applies to: 11-11, 13-13, 25-25
src/main/java/org/runimo/runimo/auth/service/SignUpUsecaseImpl.java (3)
22-22: LGTM!The import for
DeviceTokenDtois correctly added to support the new device token mapping functionality.
47-48: LGTM!Good refactoring to extract the command mapping logic into a separate method, improving code readability and maintainability.
78-88: LGTM!The extracted mapping method is well-implemented and follows good naming conventions. The use of
DeviceTokenDto.of()suggests a clean factory method pattern.src/test/resources/sql/schema.sql (2)
39-52: LGTM! Well-designed table structure for device token management.The updated
user_tokentable structure is comprehensive and well-designed:
- Primary key
idwith auto-increment is appropriateplatformENUM with FCM/APNS options covers major mobile platformsnotification_allowedboolean with TRUE default enables opt-out functionalitylast_used_attimestamp supports token lifecycle management- Soft deletion support with
deleted_atcolumn
44-44: Verify the default platform value choice.The default platform is set to 'APNS' (Apple Push Notification Service). Verify that this default makes sense for your user base, or consider if it should be 'FCM' (Firebase Cloud Messaging) which supports both Android and iOS.
Consider the platform distribution of your users to determine the most appropriate default value.
src/main/resources/sql/schema.sql (1)
40-51: Well-designed schema update for device token management.The
user_tokentable restructuring looks solid with these improvements:
- Primary key addition enables proper JPA entity mapping
- Platform ENUM with FCM/APNS values provides clear device type distinction
notification_allowedflag gives users control over push notificationslast_used_attimestamp enables token lifecycle management and cleanupThe default values are appropriate (APNS default, notifications enabled by default).
src/test/java/org/runimo/runimo/auth/controller/AuthControllerTest.java (3)
150-170: Good test coverage for backward compatibility.This test appropriately verifies that signup works without device tokens, ensuring backward compatibility with older app versions.
172-184: Effective validation test for incomplete device token data.The test correctly verifies that providing a device token without specifying the platform results in a 400 Bad Request response, which aligns with the validation logic in
AuthSignupRequest.toUserSignupCommand().
186-206: Comprehensive test for complete device token functionality.This test validates the full happy path with both device token and platform provided, correctly expecting a 201 Created response.
src/main/java/org/runimo/runimo/auth/controller/request/AuthSignupRequest.java (2)
16-21: Good choice using Lombok to reduce boilerplate code.The conversion from record to class with Lombok annotations maintains readability while providing the flexibility needed for the new optional fields.
34-44: Well-designed backward compatibility approach.The addition of optional
deviceTokenanddevicePlatformfields with a backward-compatible constructor ensures existing clients continue to work while enabling new functionality.src/test/java/org/runimo/runimo/auth/controller/AuthAcceptanceTest.java (3)
164-179: Comprehensive acceptance test for device token functionality.This test provides good end-to-end validation of the device token signup flow, verifying the complete request-response cycle.
181-193: Effective validation test for incomplete device token data.This acceptance test correctly validates the server's response when device token is provided without a platform, ensuring proper API validation behavior.
209-211: Good use of helper method to reduce code duplication.The
buildSignupRequesthelper method follows the DRY principle and makes the tests more maintainable by centralizing the request creation logic.
| public static DevicePlatform fromString(String value) { | ||
| return DevicePlatform.valueOf(value.toUpperCase()); | ||
| } |
There was a problem hiding this comment.
Add null safety to the fromString method.
The method will throw a NullPointerException if a null string is passed, since it calls toUpperCase() on the input without null checking.
public static DevicePlatform fromString(String value) {
+ if (value == null) {
+ throw new IllegalArgumentException("Device platform value cannot be null");
+ }
return DevicePlatform.valueOf(value.toUpperCase());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static DevicePlatform fromString(String value) { | |
| return DevicePlatform.valueOf(value.toUpperCase()); | |
| } | |
| public static DevicePlatform fromString(String value) { | |
| if (value == null) { | |
| throw new IllegalArgumentException("Device platform value cannot be null"); | |
| } | |
| return DevicePlatform.valueOf(value.toUpperCase()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/runimo/runimo/user/domain/DevicePlatform.java around lines
8 to 10, the fromString method lacks null safety and will throw a
NullPointerException if a null value is passed. Add a null check at the start of
the method to handle null inputs gracefully, either by returning null, throwing
a custom exception, or handling it according to the application's requirements
before calling toUpperCase().
src/main/java/org/runimo/runimo/user/service/dto/command/DeviceTokenDto.java
Outdated
Show resolved
Hide resolved
src/main/java/org/runimo/runimo/user/service/dto/command/DeviceTokenDto.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/runimo/runimo/user/service/usecases/UserRegisterServiceTest.java (1)
43-43: Update test method name to reflect expanded functionality.The test method name "회원가입_알_지급_테스트" (user registration egg distribution test) should be updated to reflect that it now also tests device token functionality.
Consider renaming to something like:
-void 회원가입_알_지급_테스트() { +void 회원가입_디바이스토큰_알_지급_테스트() {or
-void 회원가입_알_지급_테스트() { +void 디바이스토큰_포함_회원가입_테스트() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/runimo/runimo/user/service/usecases/UserRegisterServiceTest.java(2 hunks)
🔇 Additional comments (1)
src/test/java/org/runimo/runimo/user/service/usecases/UserRegisterServiceTest.java (1)
16-16: Import statements correctly added for new device token functionality.The imports for
DevicePlatformandDeviceTokenDtoare properly added to support the updated test.Also applies to: 23-23
src/test/java/org/runimo/runimo/user/service/usecases/UserRegisterServiceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/runimo/runimo/auth/service/SignUpUsecaseTest.java (1)
85-86: Consider adding device token handling verification.The parameter updates are correct and consistent across both test methods. However, these tests don't verify that the device token is being processed correctly by the service. Consider adding assertions to validate that:
- The device token is passed to the underlying service correctly
- Different device platforms are handled appropriately
- Null device token scenarios are tested
For example, you could add verification that the
userRegisterService.registerUser()is called with the correct device token information:when(userRegisterService.registerUser(any())).thenReturn(UserFixtures.getUserWithId(1L)); + +// Verify device token is passed correctly +verify(userRegisterService).registerUser(argThat(command -> + "device_token".equals(command.deviceToken()) && + DevicePlatform.APNS.equals(command.devicePlatform()) +));Would you like me to help create additional test methods to cover different device token scenarios?
Also applies to: 115-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/runimo/runimo/user/domain/DevicePlatform.java(1 hunks)src/main/java/org/runimo/runimo/user/service/dto/command/DeviceTokenDto.java(1 hunks)src/test/java/org/runimo/runimo/auth/service/SignUpUsecaseTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/runimo/runimo/user/domain/DevicePlatform.java
- src/main/java/org/runimo/runimo/user/service/dto/command/DeviceTokenDto.java
🔇 Additional comments (1)
src/test/java/org/runimo/runimo/auth/service/SignUpUsecaseTest.java (1)
28-28: LGTM: Import addition is correct.The import for
DevicePlatformenum is necessary to support the updated test method signatures.
ekgns33
force-pushed
the
feat/user-device-token
branch
from
26b421f to
6b20ce0
Compare
|
|
jeeheaG
left a comment
jeeheaG
left a comment
There was a problem hiding this comment.
고생하셨습니다!
알림에 대해 정리하면
알림 약관 동의가 필수이고
알림 약관 동의 시 토큰 발급(따라서 모든 회원의 토큰 존재),
알림 수신 on/off 에 따라 토큰 테이블에 notificationAllowed 값이 바뀌는 거 맞을까요?
| private Boolean notificationAllowed; | ||
|
|
||
| @Column(name = "last_used_at") | ||
| private LocalDateTime lastUsedAt; |
There was a problem hiding this comment.
최근 알림 지표로 삼을 것 같아서 추가했어요! 알림 대상 테이블은 빠르게 수가 증가할 것 같아서 디바이스토큰에 넣었습니다.
|
2 participants
작업 내역
디바이스 토큰 엔티티 추가
UserDeviceToken엔티티 추가 : 2acf56csrc/resources/sql/schema.sql수정하위 호환성
리뷰해주세요
user_token테이블에 상태 컬럼을 두었어요! 확인부탁드립니다Summary by CodeRabbit