[BE] 매칭 완료 알람 기능 구현(#761) #768
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Test Results 65 files 65 suites 7s ⏱️ Results for commit f4eec6b. ♻️ This comment has been updated with latest results. |
github-actions
bot
requested review from
hjk0761,
jcoding-play and
youngsu5582
hjk0761
approved these changes
Nov 18, 2024
| } | ||
|
|
||
| public static AlarmResponse of(ServerToUserAlarm alarm, Room room) { | ||
| return new AlarmResponse(alarm.getId(), alarm.getActionType(), null, |
Comment on lines
24
to
31
| if (alarm instanceof UserToUserAlarm userAlarm) { | ||
| return AlarmResponse.of(userAlarm, members.get(userAlarm.getActorId()), userAlarmRooms.get(userAlarm.getInteractionId())); | ||
| } | ||
| if (alarm instanceof ServerToUserAlarm serverAlarm){ | ||
| return AlarmResponse.of(serverAlarm, serverAlarmRooms.get(serverAlarm.getInteractionId())); | ||
| } | ||
| throw new CoreaException(ExceptionType.UNDEFINED_ALARM_TYPE); | ||
| }) |
There was a problem hiding this comment.
instanceof 를 이용하여 외부에서 타입을 검사하는 것 대신,
Alarm 객체가 isUserToUser 과 같은 메서드를 가지는건 어떻게 생각하세요?
둘 다 종류가 많아질수록 길어지는 건 매한가지 일 것 같긴한데, 지금 타입이 2개밖에 없는 상황에서 type 을 정의하는 Enum 으로 분리하는 것도 과한작업 같다고 느껴지긴하네요.
| } | ||
|
|
||
| private List<Alarm> mergeAlarms(AlarmsByActionType userAlarms, AlarmsByActionType serverAlarms) { | ||
| return Stream.concat( |
There was a problem hiding this comment.
stream.concat 을 하면, 두 스트림이 순서대로 합쳐지는 거로 알고 있는데,
그럼 유저알람이 먼저 보이고 아래에 서버알람이 보이게 될 것 같아요.
혹시 시간 순으로 정렬은 어떤가요?
근데 할꺼면 모든 알람 객체들에게 생성 시간이 들어가야해서 쉽지 않겠네요,, 다른 분들 의견도 궁금하네요
youngsu5582
approved these changes
Nov 18, 2024
| } | ||
|
|
||
| @Override | ||
| public boolean isStatus(boolean status) { |
There was a problem hiding this comment.
isRead 대신 isStatus 를 한 이유가 있나요?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
✨ PR 세부 내용
스케줄러에 등록된 매칭이 완료되면 결과에 해당하는 알람을 생성합니다.
MATCH_COMPLETEMATCH_FAILP2
MatchingExecutorTest마지막 테스트가 자꾸 깨지는데(제가 추가했어요)아무래도 트랜잭션 문제 같아요... 🤔 근데 몇 시간 고민해도 해결책을 잘 모르겠네요
집단 지성을 구하고 싶어 일단
@Disabled해두고 PR 올렸습니다. 백엔드 분들 도움요.P4
현재
user->user로 가는 알람과 달리 매칭 완료 로직은server->user로 가는 알람입니다.따라서
Alarm인터페이스를 사용하고 두 가지의 알람 도메인을 해당 인터페이스의 구현체로 만들었는데요.생각보다 기존 로직에 손댄 부분이 조금 있어서 이 부분도 잘 작성된 게 맞는지? 잘 돌아가는지? 함께 확인해주시면 감사하겠습니다.
우선 테스트는 다 돌아감.
P4
매칭 로직이 스케줄러에 의존하고 있다보니 알람 생성을 어느 단에서 해야할지 조금 고민했습니다.
MatchingExecutor가 다른 서비스들을 의존하고 있는 클래스길래 얘가 알람 서비스도 사용하도록 짜봤어요.그런데 정확히 저 부분에 들어가는 게 맞는지는 조금 아리까리하네요.
P2질문과 함께 봐주시면 감사합니다.