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] 리뷰 폼 생성 및 공유하기 #6

Merged
merged 31 commits into from
Aug 23, 2024

Conversation

heejjinkim
Copy link
Collaborator

@heejjinkim heejjinkim commented Aug 20, 2024

✏️ Description

  • 사용자가 리뷰 받고 싶은 카테고리와 팀원을 선택하면, 리뷰 폼을 생성하고 선택된 팀원들에게 리뷰 요청 알림 보내기
  • 기본 질문, 옵션 삽입하기 위해 DataInitializer 생성
  • Question, ChoiceOption, ReviewQuestion 모두 JdbcTemplate을 사용하여 batch Insert 수행

🙏🏻 To Reviewers

  • BaseEntity 관련
    • createdAt이 null로 삽입된다는 오류가 떠서, BaseEntity에서 AuditingEntityListener로 바꾸니 해결됐습니다.
  • GlobalExceptionHandler 관련
    • request로 enum 타입이 잘못 들어왔을 때 발생하는 에러를 다루는 함수 추가했습니다.
    • 레퍼런스
  • Question, ChoiceOption 엔티티 관련
    • ChoiceOption의 sequence 필드는 해당 질문 내에서의 선택지 순서입니다. 그래서 보기가 4개면 sequence가 1,2,3,4 이런 식으로 할당 되게 했습니다.
    • Question을 먼저 batch insert 한 후 ChoiceOption을 batch insert 하는데, 이 때 Question의 id를 수동으로 세팅해줘야 추후에 ChoiceOption insert 하기가 편해서 그렇게 했습니다. id를 수동으로 순서대로 세팅하다 보니, Question에는 sequence 필드는 따로 만들지 않았습니다.
  • 기본 질문과 옵션 삽입
    • ApplicationRunner를 사용했습니다! data.sql은 ddl-auto:create 옵션을 써서 모든 데이터를 삭제한 뒤 테스트 용도의 데이터를 삽입하는 느낌이라서.. ApplicationRunner를 사용해 질문과 옵션 데이터가 없는 경우에만 삽입되도록 구현했습니다.
    • 만약 질문과 옵션 데이터에 수정이 필요할 때는, delete api 하나 만들어서 전부 삭제 한 후에 시스템 다시 시작하면 삽입되도록 하는 것이 어떤가 생각하고 있어요.
  • ReviewQuestion 엔티티
    • ReviewForm과 Question가 다대다 관계여서 그 중간의 매핑을 위해 만들었습니다.
  • 질문
    • 리뷰를 이미 요청한 팀원에 대해서는 다시 리뷰 요청을 못하는 로직이 필요할 것 같은데, 이 부분은 리뷰 요청할 팀원을 선택하는 화면에서 이미 리뷰를 요청 받은 팀원인지 여부를 반환해주면 되겠죠??
    • 피그마 다시 확인해보니 리뷰 생성하기를 누른 후에 모든 질문 조회가 가능한 걸로 되어 있는데, 생성하기를 눌렀을 때는 리뷰폼을 생성하지 않고 사용자의 이름과 카테고리에 따른 질문,옵션 리스트를 반환해주는 것에 대해 어떻게 생각하시나요? 저는 리뷰 폼을 생성하는 로직과 리뷰 요청 알림이 가는 로직은 항상 같이 가야한다고 생각해서 둘을 하나의 api로 합친 거였거든요. 생성하기를 누르고 뒤로 가기 해버리면 안 쓰는 ReviewFormQuestion 데이터가 남아있게되니까..
    • 성능 개선이나 리팩토링 관련해서 의견 있으시면 적어주세요🙏

💡 Issue Number

project, reviewform, user 관련 errorcode
review request 알람 생성 시 활용
Related to: #4
batchInsert를 통한 성능 개선
related to: #4
batch insert를 통한 성능 개선
기본 옵션 삽입 시 사용
related to: #4
batch insert를 통한 성능 개선
리뷰 폼 생성 및 공유 시 사용
 related to: #4
@heejjinkim heejjinkim requested a review from minje0204 August 20, 2024 07:26
@heejjinkim heejjinkim self-assigned this Aug 20, 2024
Copy link
Collaborator

@minje0204 minje0204 left a comment

Choose a reason for hiding this comment

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

reviewService.createReviewForm 에서 실행되는 로직은 categorType에 따라서 캐싱을 할 수 있다면 처리하는것도 좋을 것 같아요

수고하셨어요~!

String content) {
Member receiver = memberRepository.findById(receiverId).orElseThrow(()
-> new RestApiException(UserErrorCode.USER_NOT_FOUND));
content = sender.getName() + content;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alarm이라는 도메인에 관련된 내용이니 Alarm Entity에 관련된 내용이 포함시켜도 좋을 것 같아요

setContent 함수에 들어갈 수도 있지 않을까 싶네요?


@Getter
@NoArgsConstructor
@MappedSuperclass
@EntityListeners(AuditingConfiguration.class)
@EntityListeners(AuditingEntityListener.class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 다른 Class를 적어두었었네요. 수정해주셔서 감사해요

}
}

private void addCategoryQuestionsAndOptions(List<Question> allQuestions, List<ChoiceOption> allChoiceOptions, CategoryType categoryType, List<QuestionWithOptions> questionWithOptionsList) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이부분 Intellij code Style이 적용이 안된 것 같아요!

private void setDefaultQuestionsAndChoiceOption() {

// DB에 데이터 없는 경우만 새로 생성
if (questionRepository.count() == 0 && choiceOptionRepository.count() == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional]
Count vs Exitsts 키워드간 성능차이가 있다고 하네요
이는 exists는 첫번째 결과에서 바로 true 를 리턴하면 되지만, count의 경우엔 결국 총 몇건인지 확인하기 위해 전체를 확인해봐야하기 때문에 성능 차이는 당연할 수 밖에 없습니다.

레퍼런스

allChoiceOptions,
COMMUNICATION,
List.of(
new QuestionWithOptions(
Copy link
Collaborator

Choose a reason for hiding this comment

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

하드코딩된 부분은 .json 파일로 따로 빼는게 가독성에 좋을 것 같아요.

question.json

[
  {
    "categoryType": "COMMUNICATION",
    "questions": [
      {
        "text": "${nickname}님은 회의 중 의견을 나눌 때 어떠했나요?",
        "options": [
          "명확하고 논리적으로 의견을 전달하였다.",
          "의견이 모호하여 이해하기 어려운 경우가 있었다.",
          "회의 중 의견을 나누지 않았다.",
          "의견을 잘 전달했으나 다른 의견을 듣는 데는 소극적이었다."
        ]
      },
      {
        "text": "${nickname}님은 슬랙과 같은 온라인 커뮤니케이션에서 어떻게 대응했나요?",
        "options": [
          "빠르고 정확한 답변을 제공하였다.",
          "답변이 늦거나 불명확한 경우가 있었다.",
          "이메일에 대한 대응이 거의 없었다.",
          "답변은 빠르지만 내용이 부실하였다."
        ]
      }
      // 나머지 질문과 선택지들...
    ]
  },
  {
    "categoryType": "COLLABORATION",
    "questions": [
      {
        "text": "${nickname}님이 팀 프로젝트에서 역할을 수행하는 태도는 어땠나요?",
        "options": [
          "적극적으로 자신의 역할을 수행하고 팀을 지원하였다.",
          "자신의 역할은 수행했으나 팀 지원은 부족하였다.",
          "자신의 역할조차 제대로 수행하지 못하였다.",
          "역할 수행은 잘했으나 팀과의 협력은 부족하였다."
        ]
      }
      // 나머지 질문과 선택지들...
    ]
  }
  // 나머지 카테고리들...
]
Resource resource = new ClassPathResource("questions.json");
List<CategoryQuestions> categoryQuestionsList = objectMapper.readValue(resource.getInputStream(),
                        new TypeReference<List<CategoryQuestions>>() {});

.flatMap(category -> questionRepository.findByCategoryType(category).stream())
.collect(Collectors.toList());
if (questions.isEmpty()) {
throw new RestApiException(ReviewErrorCode.QUESTION_NOT_FOUND);
Copy link
Collaborator

Choose a reason for hiding this comment

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

브라우저에서 입력된 요청값 "categories"가 controller단에서 유효성검사를 마치고 나면, question.isEmtpy인 경우는 DB상 카테고리타입에 해당하는 질문들이 없는 경우이니

해당하는 카테고리에 속하는 질문이 없습니다라고 보다 명시적으로 error message가 작성되면 좋을 것 같아요

String message = "님이 리뷰를 요청했습니다.";
request.getReviewerIdList().forEach(reviewerId ->
alarmService.createAlarm(result.getSender(), reviewerId, AlarmType.REVIEW_REQUEST, result.getReviewFormId(), message)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

알림생성 부분은 서비스단에 코드가 위치해야할 것 같아요

.collect(Collectors.toList());
reviewQuestionJdbcRepository.batchInsert(reviewQuestions);

return ReviewFormCreateResponse.builder()
Copy link
Collaborator

Choose a reason for hiding this comment

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

ResponseDto도 정적팩토리 메서드를 활용하면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 좋습니다!

List<CategoryType> categories = request.getCategories();
List<Question> questions = categories.stream()
.flatMap(category -> questionRepository.findByCategoryType(category).stream())
.collect(Collectors.toList());
Copy link
Collaborator

Choose a reason for hiding this comment

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

.toList()만 사용해도 되는 것 같아요 (IDE 추천)


private final JdbcTemplate jdbcTemplate;

public void batchInsert(List<ReviewQuestion> reviewQuestions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

batchInsert는 이런식으로 진행되는군요! 첨알았네요

@minje0204
Copy link
Collaborator

minje0204 commented Aug 20, 2024

리뷰를 이미 요청한 팀원에 대해서는 다시 리뷰 요청을 못하는 로직이 필요할 것 같은데, 이 부분은 리뷰 요청할 팀원을 선택하는 화면에서 이미 리뷰를 요청 받은 팀원인지 여부를 반환해주면 되겠죠??

네! 리뷰를 요청할 팀원 list 조회 요청을 할때 응답값으로 이미 요청을 보낸 팀원인지도 같이 응답해주면 될 것 같아요.

피그마 다시 확인해보니 리뷰 생성하기를 누른 후에 모든 질문 조회가 가능한 걸로 되어 있는데, 생성하기를 눌렀을 때는 리뷰폼을 생성하지 않고 사용자의 이름과 카테고리에 따른 질문,옵션 리스트를 반환해주는 것에 대해 어떻게 생각하시나요? 저는 리뷰 폼을 생성하는 로직과 리뷰 요청 알림이 가는 로직은 항상 같이 가야한다고 생각해서 둘을 하나의 api로 합친 거였거든요. 생성하기를 누르고 뒤로 가기 해버리면 안 쓰는 ReviewFormQuestion 데이터가 남아있게되니까..

리뷰폼생성 로직과 리뷰요청알림 로직은 분리될 수도 있을 것 같다는 생각이 들긴하는데, 제가 질문 이해를 정확히 못한것 같아요. 혹시 고민이되신 피그마 화면을 공유해주실 수 있을까요?

성능 개선이나 리팩토링 관련해서 의견 있으시면 적어주세요🙏
코멘트로 남겨두었습니다.

@heejjinkim
Copy link
Collaborator Author

리뷰폼생성 로직과 리뷰요청알림 로직은 분리될 수도 있을 것 같다는 생각이 들긴하는데, 제가 질문 이해를 정확히 못한것 같아요. 혹시 고민이되신 피그마 화면을 공유해주실 수 있을까요?

image

두 로직이 분리되어 있는 경우, "생성하기" 버튼을 눌렀을 때 리뷰 폼을 생성하고, 생성된 리뷰 폼과 관련된 질문들을 반환하고 끝나게 될 것 같은데요. 이렇게 되면 사용자가 뒤로 가기를 눌렀을 때 리뷰 폼만 생성되고 리뷰 요청 알림은 생성되지 않아, 결과적으로 생성된 리뷰 폼 데이터가 어디에도 사용되지 않는 무의미한 데이터로 남을 수 있다고 생각해요.

@minje0204
Copy link
Collaborator

리뷰폼생성 로직과 리뷰요청알림 로직은 분리될 수도 있을 것 같다는 생각이 들긴하는데, 제가 질문 이해를 정확히 못한것 같아요. 혹시 고민이되신 피그마 화면을 공유해주실 수 있을까요?

image 두 로직이 분리되어 있는 경우, "생성하기" 버튼을 눌렀을 때 리뷰 폼을 생성하고, 생성된 리뷰 폼과 관련된 질문들을 반환하고 끝나게 될 것 같은데요. 이렇게 되면 사용자가 뒤로 가기를 눌렀을 때 리뷰 폼만 생성되고 리뷰 요청 알림은 생성되지 않아, 결과적으로 생성된 리뷰 폼 데이터가 어디에도 사용되지 않는 무의미한 데이터로 남을 수 있다고 생각해요.

그럴수있겠네요! 로직을 합치는 것도 좋을 것 같아요.
만약 화면 flow상 액션을 나누어야만 한다면 중간에, 리뷰폼 생성 후 알림이 발생했는지 여부를 boolean으로 db에 저장해서 한번 checkpoint를 저장하는 느낌으로 가는 방법도 있을 것 같네요

ReviewFormQuestion으로 이름 변경
사용하지 않는 setter 제거
toList로 수정
response of 함수 추가
카테고리에 해당하는 질문이 없는 경우에 대한 error code 추가

related to: #4
count를 querydsl을 사용하여 exist로 수정
기본 질문과 옵션 텍스트를 json으로 추출

related to: #4
@heejjinkim heejjinkim merged commit f25c444 into develop Aug 23, 2024
@heejjinkim heejjinkim added the ✨ feature New feature or request label Aug 23, 2024
@heejjinkim heejjinkim deleted the feature/#4-create-reviewform branch October 14, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants