-
Notifications
You must be signed in to change notification settings - Fork 4
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 추가 #234
채팅방 생성 api 추가 #234
Changes from 6 commits
6fb3d89
21ade4c
5dfba44
537de24
1c8ba25
c72d0ae
db1f7e8
927a6c2
9e23903
c8dabf1
0dcf9d8
f8cab38
6402af0
8e8eaf1
8099e62
6b953d2
ea2f171
2930458
ecc4f9d
473695d
da6c18f
8df6a77
b4fcd7e
3dd1da0
acbb62f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package com.ddang.ddang.auction.domain.exception; | ||
|
||
public class WinnerNotFoundException extends IllegalArgumentException { | ||
|
||
public WinnerNotFoundException(final String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
package com.ddang.ddang.chat.application; | ||
|
||
import com.ddang.ddang.auction.application.exception.AuctionNotFoundException; | ||
import com.ddang.ddang.auction.domain.Auction; | ||
import com.ddang.ddang.auction.infrastructure.persistence.JpaAuctionRepository; | ||
import com.ddang.ddang.chat.application.dto.CreateChatRoomDto; | ||
import com.ddang.ddang.chat.application.dto.ReadParticipatingChatRoomDto; | ||
import com.ddang.ddang.chat.application.exception.ChatAlreadyExistException; | ||
import com.ddang.ddang.chat.application.exception.ChatRoomNotFoundException; | ||
import com.ddang.ddang.chat.application.exception.InvalidAuctionToChatException; | ||
import com.ddang.ddang.chat.application.exception.UserNotAccessibleException; | ||
import com.ddang.ddang.chat.application.exception.UserNotFoundException; | ||
import com.ddang.ddang.chat.domain.ChatRoom; | ||
|
@@ -14,6 +20,7 @@ | |
|
||
import java.time.LocalDateTime; | ||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
@Service | ||
@Transactional(readOnly = true) | ||
|
@@ -22,21 +29,64 @@ public class ChatRoomService { | |
|
||
private final JpaChatRoomRepository chatRoomRepository; | ||
private final JpaUserRepository userRepository; | ||
private final JpaAuctionRepository auctionRepository; | ||
|
||
public List<ReadParticipatingChatRoomDto> readAllByUserId(final Long userId) { | ||
@Transactional | ||
public Long create(final Long userId, final CreateChatRoomDto chatRoomDto) { | ||
final User findUser = findUser(userId); | ||
final List<ChatRoom> chatRooms = chatRoomRepository.findAllByUserId(findUser.getId()); | ||
final Auction findAuction = findAuction(chatRoomDto); | ||
|
||
return chatRooms.stream() | ||
.map(chatRoom -> toDto(findUser, chatRoom)) | ||
.toList(); | ||
checkAuctionStatus(findAuction); | ||
checkUserCanParticipate(findUser, findAuction); | ||
checkAlreadyExist(findAuction); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. auction에서 auction의 상태를 확인하는 경우가 chatRoom 말고도 더 생길 수도 있을 것 같아요 auction에서 종료된 경매인지, 삭제된 경매인지에 대한 예외 메시지는 enum value로 갖게 해서 다르게 하면 어떨까 생각해봤는데, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 맞아요 아마 이 부분 경매 목록 조회 시에도 필요한 기능입니다. 경매 목록 조회 같은 경우에도 저번에 임시로 하는 바람에 dto에서 변환하고 있는 것으로 알고 있어요. 다른 분들과 이야기하고 결정하겠습니다! |
||
|
||
final ChatRoom chatRoom = chatRoomDto.toEntity(findAuction); | ||
|
||
return chatRoomRepository.save(chatRoom) | ||
.getId(); | ||
} | ||
|
||
private User findUser(final Long userId) { | ||
return userRepository.findById(userId) | ||
.orElseThrow(() -> new UserNotFoundException("사용자 정보를 찾을 수 없습니다.")); | ||
} | ||
|
||
private Auction findAuction(final CreateChatRoomDto chatRoomDto) { | ||
return auctionRepository.findById(chatRoomDto.auctionId()) | ||
.orElseThrow(() -> new AuctionNotFoundException("해당 경매를 찾을 수 없습니다.")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 부분과 관련해 궁금한 점이 있습니다. |
||
} | ||
|
||
private void checkAuctionStatus(final Auction findAuction) { | ||
if (!findAuction.isClosed(LocalDateTime.now())) { | ||
throw new InvalidAuctionToChatException("경매가 아직 종료되지 않았습니다."); | ||
} | ||
if (findAuction.isDeleted()) { | ||
throw new InvalidAuctionToChatException("삭제된 경매입니다."); | ||
Comment on lines
+62
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기에서는 서비스에서 예외를 처리해주고 있었네요 Auction.checkWinnerExist()랑 이거랑 통일성을 지켜주는 방향은 어떨까 건의드려봅니다 |
||
} | ||
} | ||
|
||
private void checkUserCanParticipate(final User findUser, final Auction findAuction) { | ||
if (!findAuction.isOwner(findUser) && !findAuction.isWinner(findUser, LocalDateTime.now())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 생각입니다. 메서드가 너무 잘게 분리되어 있어서 불편하지 않을까 하는 생각에 그냥 두었는데 가독성이 안좋아졌군요.. |
||
throw new UserNotAccessibleException("경매의 판매자 또는 최종 낙찰자만 채팅이 가능합니다."); | ||
} | ||
} | ||
|
||
private void checkAlreadyExist(final Auction findAuction) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 변경된 부분 리마인드 차원에서 코멘트 남깁니다!
지토가 말씀해주신 의견도 좋은데 boolean과 chatRoomId 조회를 위해 두 번 조회하게 될 것 같아 의견 남겨요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 친절하네요 메..리... 👍 |
||
final Optional<ChatRoom> nullableChatRoom = chatRoomRepository.findByAuctionId(findAuction.getId()); | ||
if (nullableChatRoom.isPresent()) { | ||
throw new ChatAlreadyExistException("해당 경매에 대한 채팅방이 이미 존재합니다."); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이러한 경우는 find를 통해 존재 여부를 확인하는 것보다는 chatRoomRepository에 existsByAuctionId() 메서드를 추가하고 이를 통해 존재 여부를 boolean으로 반환받아서 처리해주는것이 더 자연스러울 것 같습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 맞네요 적용하겠습니다! |
||
} | ||
|
||
public List<ReadParticipatingChatRoomDto> readAllByUserId(final Long userId) { | ||
final User findUser = findUser(userId); | ||
final List<ChatRoom> chatRooms = chatRoomRepository.findAllByUserId(findUser.getId()); | ||
|
||
return chatRooms.stream() | ||
.map(chatRoom -> toDto(findUser, chatRoom)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 PR에는 없어서 여기에다가 남깁니다... toDto() 메서드에서 보면 ReadParticipatingChatRoomDto.of()를 통해서 dto로 변환하고 있는데 그래서 ReadParticipatingChatRoomDto.of(findUser, chatRoom) 혹은 시간 테스트를 조금 더 편하게 하기 위해 ReadParticipatingChatRoomDto.of(findUser, chatRoom, targetTime)으로 하고 of() 내부에 chatRoom.calculateChatPartnerOf() 이런 메서드를 호출하는건 어떨까 건의드려봅니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋습니다. 반영하겠습니다! |
||
.toList(); | ||
} | ||
|
||
public ReadParticipatingChatRoomDto readByChatRoomId(final Long chatRoomId, final Long userId) { | ||
final User findUser = findUser(userId); | ||
final ChatRoom chatRoom = chatRoomRepository.findById(chatRoomId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 부분과 관련해 갑자기 궁금해진 부분이 있어 여쭤봅니다. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package com.ddang.ddang.chat.application.dto; | ||
|
||
import com.ddang.ddang.auction.domain.Auction; | ||
import com.ddang.ddang.chat.domain.ChatRoom; | ||
import com.ddang.ddang.chat.presentation.dto.request.CreateChatRoomRequest; | ||
import com.ddang.ddang.user.domain.User; | ||
|
||
import java.time.LocalDateTime; | ||
|
||
public record CreateChatRoomDto(Long auctionId) { | ||
|
||
public static CreateChatRoomDto from(final CreateChatRoomRequest chatRoomRequest) { | ||
return new CreateChatRoomDto(chatRoomRequest.auctionId()); | ||
} | ||
|
||
public ChatRoom toEntity(final Auction auction) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 메서드는 테스트에서만 사용되는 메서드인 것 같아요! 그리고 dto에는 비즈니스 로직을 최대한 빼는 것이 좋을 것 같다고 생각하는데, 예외를 던지는 부분은 비즈니스 로직이라고 생각해서 dto가 아닌 다른 도메인이나 서비스에서 수행되는 것이 좋아보입니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아 그렇네요! 제거하였습니다~ |
||
final User buyer = auction.findWinner(LocalDateTime.now()); | ||
|
||
return new ChatRoom(auction, buyer); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package com.ddang.ddang.chat.application.exception; | ||
|
||
public class ChatAlreadyExistException extends IllegalArgumentException { | ||
|
||
public ChatAlreadyExistException(final String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package com.ddang.ddang.chat.application.exception; | ||
|
||
public class InvalidAuctionToChatException extends IllegalArgumentException { | ||
|
||
public InvalidAuctionToChatException(final String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package com.ddang.ddang.chat.presentation.dto.request; | ||
|
||
import jakarta.validation.constraints.NotNull; | ||
import jakarta.validation.constraints.Positive; | ||
|
||
public record CreateChatRoomRequest( | ||
@NotNull(message = "경매 아이디가 입력되지 않았습니다.") | ||
@Positive(message = "경매 아이디는 양수입니다.") | ||
Long auctionId | ||
) { | ||
} |
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.
해당 메서드가 다른 메서드와 조금 다른 것 같아서 어색하게 느껴집니다
예시로 BidService.checkInvalidAuction()에서는 Auction.isClose()나 Auction.isDelete()를 호출하고 그 결과를 통해서 Service에서 예외를 던져주고 있는데 지금 코드는 도메인 내부에서 예외를 던져줘서 좀 어색하게 느껴지네요
상태에 따른 제어를 도메인에서 할지 서비스에서 할지도 좀 고민되기는 하네요
억지기는 하지만 만약 정책이 auctioneerCount가 0일 때 낙찰자는 판매자 자기자신으로 변경되었을 때 이러한 정책은 도메인에서 관리하는게 자연스러울지 서비스에서 관리하는게 자연스러울지...어렵습니다 정말
혹시 도메인에서 예외를 던지는 이유에 대해서 알려주실 수 있으실까요?
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.
이 부분에 대해서 저도 뭔가 이상하긴 했습니다.
우선 낙찰자를 판단하는 것은 auction 도메인에 있는 것이 적절하다고 생각했습니다.
그런데 findWinner()를 호출했는데 낙찰자가 없어서 null이 들어가게 되는 경우를 만들지 않고 싶었고,
그러려면 findWinner() 전에 낙찰자가 존재하는지 확인해야 했고,
그 findWinner()를 호출했을 때 무조건 낙찰자 존재 여부를 확인하는 것을 강제하고 싶었습니다.
findWinner() 내부에서 boolean으로 낙찰자 존재 여부를 확인하고 false일 경우 반환할 User가 없기 때문에 낙찰자 존재 여부를 확인할 때 예외를 터뜨리게 되었습니다.
이 부분을 어떻게 하면 잘 해결할 수 있을까요? 저도 궁금합니다..ㅠㅠ
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.
일단 findWinner() 내부에서 낙찰자가 있는지 확인하고 findWinner()는 Optional를 반환하도록 수정하였습니다!
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.
@kwonyj1022 확실히 좀 애매하기는 하네요
도메인에서 낙찰자가 존재하는지 확인하는 boolean 반환 메서드와 실제 낙찰자를 반환하는 User 반환 메서드로 분리하거나
엔초가 말씀해신대로 Optional로 반환하는 방식 두 가지로 할 것 같은데
전 엔초가 지금 작성해주신 방식이 더 좋은 것 같습니다