Skip to content
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

[FEAT] 수정된 주제, 선택지 조회 API 반영 #53

Merged
merged 8 commits into from
Jul 24, 2024
Merged

Conversation

leegwichan
Copy link
Contributor

Issue Number

#52

As-Is

  • Sprint 2 P2에 맞추어 주제, 선택지 조회 API 변경

To-Be

  • 회의에서 논의된 Exception 관리 적용
  • 변경된 ERD 적용 (v1.1)
  • 변경된 주제, 선택지 조회 API 적용

Check List

  • 테스트가 전부 통과되었나요?
  • 모든 commit이 push 되었나요?
  • merge할 branch를 확인했나요?
  • Assignee를 지정했나요?
  • Label을 지정했나요?

Test Screenshot

image

(Optional) Additional Description

  • 공통적으로 사용할 로직을 빠르게 반영하는 것을 목표로 했습니다!

@leegwichan leegwichan added ✨ feat 기능 추가 🍃 BE back end labels Jul 23, 2024
@leegwichan leegwichan self-assigned this Jul 23, 2024
GIVEN53
GIVEN53 previously approved these changes Jul 23, 2024
Copy link
Member

@GIVEN53 GIVEN53 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견만 있어서 approve합니다 빠르게 반영해줘서 감사해요 커찬!

private int currentRound;

@Column(nullable = false)
private int totalRound = DEFAULT_TOTAL_ROUND;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견) totalRound가 currentRound보다 위에 있으면 좋겠어요 ㅎㅎㅎ

return roomContentRepository.findTopByRoomIdOrderByCreatedAtDesc(roomId)
.orElseThrow(() -> new BusinessLogicException("해당 방의 질문이 존재하지 않습니다."))
.getBalanceContent();
.orElseThrow(() -> new BadRequestException("해당 방의 질문이 존재하지 않습니다."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의견) RoomContent에 round 필드가 추가되면서 관련 로직도 변경이 있을 것 같은데 내일 더 얘기해보죠!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문을 언제 얼마나 추가할 것인지에 따라 달라질 수 있겠네요.
내일 더 이야기 해보도록 하겠습니다~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

room에서 currentRound, roomId로 recentBalanceContent를 조회하면 될 듯 합니다만
그 데이터를 어디서 어떻게 받아 메서드로 넘길지 내일 같이 얘기해봐요

@@ -1,5 +1,5 @@
INSERT INTO room ()
VALUES ();
INSERT INTO room (current_round, total_round)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견) 여기도 total_round 먼저 ㅎㅎㅎ

Copy link
Member

@jhon3242 jhon3242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㅋㅋㅋㅋ currentRound 순서 바꿔준 커찬 칭찬해~

Copy link
Member

@PgmJun PgmJun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영 꼼꼼히 해주셨네요~ 수고하셨습니다 커찬! findRecentRoomContent 관련 비즈니스 로직은 내일 같이 플로우 생각해보면서 오전내로 고쳐보고 오후에 각자 작업합시다!

log.warn(e.getMessage());

return new ErrorResponse(e.getMessage());
}

@ExceptionHandler
@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
public ErrorResponse handleViolateDataException(ViolateDataException e) {
public ErrorResponse handleInternalServerErrorException(InternalServerErrorException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견) 넘 길어서 InternalServerException으로 사용해도 괜찮지 않을까 싶어요!

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@Column(nullable = false)
private int totalRound = DEFAULT_TOTAL_ROUND;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견) default 값이지만 필드 주입보다는 생성자에서 넣는 것은 어떠신지요..!
곧 바뀔 거긴 한데 불..편합니다.. 못참겠어요ㅋㅋㅋㅋㅋ
사실 바꿀거면 지금이 눈에 잘띄어서 괜찮을 것 같기도 해서 사소한 의견으로 남겨봅니당ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

나중에 Room 생성 기능 구현 때에 추가하는 것이 좋을 것 같습니다! 이번에는 넘기도록 하겠습니다~

return roomContentRepository.findTopByRoomIdOrderByCreatedAtDesc(roomId)
.orElseThrow(() -> new BusinessLogicException("해당 방의 질문이 존재하지 않습니다."))
.getBalanceContent();
.orElseThrow(() -> new BadRequestException("해당 방의 질문이 존재하지 않습니다."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

room에서 currentRound, roomId로 recentBalanceContent를 조회하면 될 듯 합니다만
그 데이터를 어디서 어떻게 받아 메서드로 넘길지 내일 같이 얘기해봐요

@leegwichan
Copy link
Contributor Author

leegwichan commented Jul 24, 2024

TODO

  • 회의 결과에 따른 구현 변경
    • 미리 질문지와 Room 정보를 연결하도록 하여, 해당 라운드의 주제를 조회하도록 수정
  • InternalServerErrorExceptionInternalServerException으로 변경
  • Room의 DEFAULT_TOTAL_ROUND를 생성자에서 넣도록 변경

@leegwichan leegwichan added the 🔥 Emergency 긴급 처리(2명 리뷰 머지 PR) label Jul 24, 2024
Copy link
Member

@jhon3242 jhon3242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total, current 도입 반영 수고했어~

Copy link
Member

@PgmJun PgmJun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다:) 커찬 굿굿!!

.getBalanceContent();
private RoomContent findCurrentRoomContent(Long roomId) {
Room room = roomRepository.findById(roomId)
.orElseThrow(() -> new BadRequestException("해당 방이 존재하지 않습니다."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 의견) 이후에 중복 로직이 될 것 같아서 Repository에서 getById로 만들어두는 것은 어떨까 싶습니다:)
그리고 몇 번 roomId가 없는지 같이 에러 메시지 만들어주면 좋을 것 같아요! roomId 정보는 클라에게 노출해도 괜찮은 부분이지 않을까 싶습니다

@leegwichan leegwichan merged commit 37e807c into develop Jul 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍃 BE back end 🔥 Emergency 긴급 처리(2명 리뷰 머지 PR) ✨ feat 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants