-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
채팅방 정보 가져오기 API 구현 #636
채팅방 정보 가져오기 API 구현 #636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolver, Registry 구조가 잘 이해가 안되는데 .. 급한 PR이 아니라면 내일 설명해주세요 😉
수고했습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Participant를 재활용하지 않고 Loser를 만든 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loser 에 대한 요구사항이 변경될 수 있다고 생각했는데 지금은 불필요한 것 같네요. Participant로 교체했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영 감사합니다 ~
} | ||
|
||
@Override | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement 레이어에 트랜잭션을 건 이유가 있나요?
조회만 하고 있는 것 같은데 read only = true설정도 필요할 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트의 lazyinitializationexception 문제 때문인데요. 서비스 코드에서 이미 트랜잭션을 열어준 상태라 좋지는 않아보입니다. 이것도 이슈로 달아두고 같이 고민해보시죠.
private final AttributeManagerRegistry attributeManagerRegistry; | ||
private final ParticipantResolverRegistry participantResolverRegistry; | ||
|
||
@Transactional(readOnly = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 마찬가지입니다 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이하동문~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외처리도 해주시면 좋을 것 같아용 ㅎㅎ
private final String title; | ||
private final Boolean isLoser; | ||
private final Long betId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullable 한가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아닙니다. 수정했어요~
@Override | ||
public Attributes create(ChatRoom chatRoom, DarakbangMember darakbangMember) { | ||
Bet bet = betFinder.find(darakbangMember.getDarakbang().getId(), chatRoom.getTargetId()); | ||
boolean isLoser = bet.isLoser(darakbangMember.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내가 당첨자인지는 추가 기획을 고려하고 넣은신 건가여 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 맞습니다. 프론트 로직상 필요한 경우가 있을 수 있어서 추가했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 👍
PR의 목적이 무엇인가요?
변경된 요구사항(안내면진다 도메인의 채팅방 오픈 기능) 에 맞추어 채팅방 디테일 정보 가져오기 API 구현
이슈 ID는 무엇인가요?
설명
예시
모임
안내면진다
질문 혹은 공유 사항 (Optional)