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] 질문 새로고침 구현 #144

Merged
merged 7 commits into from
Mar 28, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,8 @@ protected ResponseEntity<ApiResponse> handleCustomException(CustomException e, f

if (e.getHttpStatus() == 501) {
notificationService.sendExceptionToSlack(e, request);
return ResponseEntity.status(e.getHttpStatus())
Copy link
Member Author

Choose a reason for hiding this comment

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

501 코드를 반환하는 경우가 추가되어서, 인위적으로 ErrorType을 리턴하지 않도록 변경했습니다.

  1. 질문 DB가 고갈됐을때
  2. 가까워지기 질문 DB가 고갈됐을때
  3. 기록하기에서 일정 수 이상으로 사진을 업로드했을 때

.body(ApiResponse.error(ErrorType.NEED_MORE_QUESTION));
} else {
return ResponseEntity.status(e.getHttpStatus())
.body(ApiResponse.error(e.getErrorType(), e.getMessage()));
}
return ResponseEntity.status(e.getHttpStatus())
.body(ApiResponse.error(e.getErrorType(), e.getMessage()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,10 @@

@Getter
@NoArgsConstructor(access = AccessLevel.PRIVATE)
@AllArgsConstructor(access = AccessLevel.PRIVATE)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public class TodayCloserAnswerRequestDto {

@Min(value = 1, message = "답변은 1 혹은 2여야 합니다.")
@Max(value = 2, message = "답변은 1 혹은 2여야 합니다.")
int answer;

public static TodayCloserAnswerRequestDto of (int answer) {
return new TodayCloserAnswerRequestDto(answer);
}
Comment on lines -22 to -25
Copy link
Member Author

Choose a reason for hiding this comment

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

Request DTO에 불필요한 생성자가 존재하는 것을 정리했습니다.

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.*;
import sopt.org.umbba.api.config.jwt.JwtProvider;
import sopt.org.umbba.api.controller.qna.dto.request.RerollChangeRequestDto;
import sopt.org.umbba.api.controller.qna.dto.request.TodayAnswerRequestDto;
import sopt.org.umbba.api.controller.qna.dto.response.*;
import sopt.org.umbba.api.service.qna.QnAService;
Expand Down Expand Up @@ -33,8 +34,8 @@ public ApiResponse<TodayQnAResponseDto> getTodayQna(Principal principal) {
@PostMapping("/qna/answer")
@ResponseStatus(HttpStatus.CREATED)
public ApiResponse answerTodayQuestion(
Principal principal,
Copy link
Member 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.

image 전 이렇게 되어 있습니다!!

Copy link
Member Author

@ddongseop ddongseop Mar 27, 2024

Choose a reason for hiding this comment

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

엇 저도 똑같은데,,, 뭔가 그 전에 잘못 설정되어 있던것 같아요!
제가 최근에 8로 되어있던걸 4로 바꿨어가지구, 지금 바뀌고 있는게 제대로 되고 있는 것 같습니다!

@Valid @RequestBody final TodayAnswerRequestDto request) {
Principal principal,
@Valid @RequestBody final TodayAnswerRequestDto request) {

qnAService.answerTodayQuestion(JwtProvider.getUserFromPrincial(principal), request);

Expand All @@ -45,7 +46,7 @@ public ApiResponse answerTodayQuestion(
@GetMapping("/qna/answer/remind")
@ResponseStatus(HttpStatus.OK)
public ApiResponse remindQuestion(
Principal principal) {
Principal principal) {

qnAService.remindQuestion(JwtProvider.getUserFromPrincial(principal));

Expand All @@ -55,21 +56,21 @@ public ApiResponse remindQuestion(
@GetMapping("/qna/list/{sectionId}")
@ResponseStatus(HttpStatus.OK)
public ApiResponse<List<QnAListResponseDto>> getQnaList(
Principal principal,
@PathVariable(name = "sectionId") Long sectionId) {
Principal principal,
@PathVariable(name = "sectionId") Long sectionId) {

return ApiResponse.success(SuccessType.GET_QNA_LIST_SUCCESS,
qnAService.getQnaList(JwtProvider.getUserFromPrincial(principal), sectionId));
qnAService.getQnaList(JwtProvider.getUserFromPrincial(principal), sectionId));
}

@GetMapping("/qna/{qnaId}")
@ResponseStatus(HttpStatus.OK)
public ApiResponse<SingleQnAResponseDto> getSingleQna(
Principal principal,
@PathVariable(name = "qnaId") Long qnaId) {
Principal principal,
@PathVariable(name = "qnaId") Long qnaId) {

return ApiResponse.success(SuccessType.GET_SINGLE_QNA_SUCCESS,
qnAService.getSingleQna(JwtProvider.getUserFromPrincial(principal), qnaId));
qnAService.getSingleQna(JwtProvider.getUserFromPrincial(principal), qnaId));
}

@PatchMapping("/qna/restart")
Expand Down Expand Up @@ -106,4 +107,16 @@ public ApiResponse<FirstEntryResponseDto> firstEntry(Principal principal) {
return ApiResponse.success(SuccessType.GET_USER_FIRST_ENTRY_SUCCESS, qnAService.updateUserFirstEntry(getUserFromPrincial(principal)));
}

@GetMapping("/reroll/check")
@ResponseStatus(HttpStatus.OK)
public ApiResponse<RerollCheckResponseDto> rerollCheck(Principal principal) {
return ApiResponse.success(SuccessType.GET_REROLL_CHECK_SUCCESS, qnAService.rerollCheck(getUserFromPrincial(principal)));
}

@PatchMapping("/reroll/change")
@ResponseStatus(HttpStatus.OK)
public ApiResponse rerollChange(Principal principal, @RequestBody RerollChangeRequestDto request) {
qnAService.rerollChange(getUserFromPrincial(principal), request.getQuestionId());
return ApiResponse.success(SuccessType.REROLL_CHANGE_SUCCESS);
}
Comment on lines +110 to +121
Copy link
Member Author

Choose a reason for hiding this comment

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

해당 2가지 API가 추가되었습니다.
명세서 참고하셔서 봐주시면 좋을 것 같아요!

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package sopt.org.umbba.api.controller.qna.dto.request;

import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Getter
@NoArgsConstructor(access = AccessLevel.PRIVATE)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public class RerollChangeRequestDto {

Long questionId;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,9 @@

@Getter
@NoArgsConstructor(access = AccessLevel.PRIVATE)
@AllArgsConstructor(access = AccessLevel.PRIVATE)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public class TodayAnswerRequestDto {

@NotBlank // null, "", " "을 모두 허용하지 X
String answer;

public static TodayAnswerRequestDto of (String answer) {
return new TodayAnswerRequestDto(answer);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package sopt.org.umbba.api.controller.qna.dto.response;

import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import lombok.Builder;
import lombok.Getter;
import sopt.org.umbba.domain.domain.qna.QnA;
import sopt.org.umbba.domain.domain.qna.Question;

@Getter
@Builder
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public class RerollCheckResponseDto {

private Long questionId;
private String childQuestion;
private String parentQuestion;
private String section;
private String topic;

public static RerollCheckResponseDto of(Question question) {
return RerollCheckResponseDto.builder()
.questionId(question.getId())
.childQuestion(question.getChildQuestion())
.parentQuestion(question.getParentQuestion())
.section(question.getSection().getValue())
.topic(question.getTopic())
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public TodayCloserQnAResponseDto getTodayCloserQnA(Long userId) {
@Transactional
public void addFirstCloserQnA(Parentchild parentchild) {
CloserQuestion firstCloserQuestion = closerQuestionRepository.findRandomExceptIds(new ArrayList<>())
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION));
.orElseThrow(() -> new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION));
Copy link
Member Author

Choose a reason for hiding this comment

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

가까워지기 질문이 고갈되었을 때도 501 에러를 리턴하도록 변경했습니다.


CloserQnA newCloserQnA = CloserQnA.builder()
.closerQuestion(firstCloserQuestion)
Expand Down Expand Up @@ -117,7 +117,7 @@ public void passToNextCloserQnA(Long userId) {
} else if (parentchild.getCloserChildCount() == parentchild.getCloserParentCount()) {
parentchild.addCloserChildCount();
CloserQuestion newCloserQuestion = closerQuestionRepository.findRandomExceptIds(getCloserQuestionIds(parentchild))
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION));
.orElseThrow(() -> new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION));
CloserQnA newCloserQnA = CloserQnA.builder()
.closerQuestion(newCloserQuestion)
.isParentAnswer(false)
Expand All @@ -134,7 +134,7 @@ public void passToNextCloserQnA(Long userId) {
} else if (parentchild.getCloserParentCount() == parentchild.getCloserChildCount()) {
parentchild.addCloserParentCount();
CloserQuestion newCloserQuestion = closerQuestionRepository.findRandomExceptIds(getCloserQuestionIds(parentchild))
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION));
.orElseThrow(() -> new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION));
CloserQnA newCloserQnA = CloserQnA.builder()
.closerQuestion(newCloserQuestion)
.isParentAnswer(false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

import javax.validation.constraints.NotNull;

import java.time.Duration;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.temporal.ChronoUnit;
import java.util.List;
Expand Down Expand Up @@ -252,7 +254,7 @@ public void filterAllQuestion(Long userId) {
// 가까워지기 QnA도 추가
if (parentchild.getCloserQnaList().isEmpty()) {
CloserQuestion firstCloserQuestion = closerQuestionRepository.findRandomExceptIds(new ArrayList<>())
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION));
.orElseThrow(() -> new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION));

CloserQnA newCloserQnA = CloserQnA.builder()
.closerQuestion(firstCloserQuestion)
Expand Down Expand Up @@ -452,6 +454,9 @@ public void restartQna(Long userId) {
.map(qna -> qna.getQuestion().getId())
.collect(Collectors.toList());

// 2.5 새로고침으로 버린 블랙리스트 질문 제외하기
doneQuestionIds.addAll(parentchild.getQuestionBlackList());

// 5. 이 경우 아예 추가될 질문이 없으므로 예외 발생시킴
List<Question> targetQuestions = questionRepository.findByTypeInAndIdNotIn(types, doneQuestionIds);
if (targetQuestions.isEmpty()) {
Expand All @@ -460,8 +465,8 @@ public void restartQna(Long userId) {

QuestionSection section = qnaList.get(parentchild.getCount() - 1).getQuestion().getSection();
List<Question> differentSectionQuestions = targetQuestions.stream()
.filter(question -> !question.getSection().equals(section))
.collect(Collectors.toList());
.filter(question -> !question.getSection().equals(section))
.collect(Collectors.toList());

Random random = new Random();
Question randomQuestion;
Expand All @@ -471,8 +476,8 @@ public void restartQna(Long userId) {
} else {
// 4. 없다면 동일한 section의 질문 중에서라도 랜덤하게 추출
List<Question> equalSectionQuestions = targetQuestions.stream()
.filter(question -> !question.getSection().equals(section))
.collect(Collectors.toList());
.filter(question -> question.getSection().equals(section))
.collect(Collectors.toList());
randomQuestion = equalSectionQuestions.get(random.nextInt(equalSectionQuestions.size()));
}

Expand All @@ -487,6 +492,87 @@ public void restartQna(Long userId) {
parentchild.addCount();
}

public RerollCheckResponseDto rerollCheck(Long userId) {
User user = getUserById(userId);
Parentchild parentchild = user.getParentChild();

// 7일 이후가 아닌 경우 질문 새로고침 불가능
if (parentchild.getCount() <= 7) {
throw new CustomException(ErrorType.INVALID_REROLL_BEFORE_SEVEN);
}
// 답변이 진행됐을 경우 질문 새로고침 불가능
List<QnA> qnaList = parentchild.getQnaList();
QnA currentQnA = qnaList.get(parentchild.getCount() - 1);
if (currentQnA.isParentAnswer() || currentQnA.isChildAnswer()) {
throw new CustomException(ErrorType.INVALID_REROLL_AFTER_ANSWER);
}

// 1. 메인 타입과 미사용 타입에 대해서 불러오기
List<QuestionType> types = Arrays.asList(MAIN, YET);

// 2. 내가 이미 주고받은 질문 제외하기
List<Long> doneQuestionIds = qnaList.stream()
.map(qna -> qna.getQuestion().getId())
.collect(Collectors.toList());

// 2.5 새로고침으로 버린 블랙리스트 질문 제외하기
doneQuestionIds.addAll(parentchild.getQuestionBlackList());

// 5. 이 경우 아예 추가될 질문이 없으므로 예외 발생시킴
List<Question> targetQuestions = questionRepository.findByTypeInAndIdNotIn(types, doneQuestionIds);
if (targetQuestions.isEmpty()) {
throw new CustomException(NEED_MORE_QUESTION);
}

QuestionSection section = qnaList.get(parentchild.getCount() - 1).getQuestion().getSection();
List<Question> equalSectionQuestions = targetQuestions.stream()
.filter(question -> question.getSection().equals(section))
.collect(Collectors.toList());

Random random = new Random();
Copy link
Member

Choose a reason for hiding this comment

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

NativeQuery로 rand()를 사용하면 쿼리로 한번에 가져올 수 있어 비즈니스 로직의 복잡성이 줄 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

우선 지금은 쿼리로 List를 가져옴 -> 비즈니스 로직 상에서 stream으로 돌며 필터링 진행 -> 랜덤하게 추출
순으로 진행을 하고 있다보니, 역시 랜덤 호출을 비즈니스 로직 상에서 진행을 하게 되었는데요..!

아래 리뷰와 관련한 리팩토링을 진행할때 NativeQuery로 rand()까지 사용해보면 좋을 것 같습니다!!

Question randomQuestion;
if (!equalSectionQuestions.isEmpty()) {
// 3. 최근에 주고받은 질문의 section과 같은 질문들 중에서 랜덤하게 추출
randomQuestion = equalSectionQuestions.get(random.nextInt(equalSectionQuestions.size()));
} else {
// 4. 없다면 다른 section의 질문 중에서라도 랜덤하게 추출
List<Question> differentSectionQuestions = targetQuestions.stream()
.filter(question -> !question.getSection().equals(section))
.collect(Collectors.toList());
randomQuestion = differentSectionQuestions.get(random.nextInt(differentSectionQuestions.size()));
}

return RerollCheckResponseDto.of(randomQuestion);
}
Comment on lines +534 to +546
Copy link
Member

Choose a reason for hiding this comment

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

전반적으로 Java stream api를 사용하신 것 같은데 추후 리팩토링 시 쿼리로 최대한 필터링해서 가져오는 방식으로 바꿔보면 좋을 것 같아요 !! (저희 프로젝트 상에는 대부분 stream 으로 구현되어 있어서 대대적인 수정이 필요할 것 같네요 ㅎㅎ..)

*참고 링크 - https://www.inflearn.com/questions/184494/sql-where-%EB%AC%B8-vs-java-stream-filter-%EC%84%B1%EB%8A%A5%EC%B0%A8%EC%9D%B4%EC%97%90-%EA%B4%80%ED%95%9C-%EC%A7%88%EB%AC%B8%EC%9E%85%EB%8B%88%EB%8B%A4

Copy link
Member Author

Choose a reason for hiding this comment

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

와아,, 아주 좋은 레퍼런스네요!
그러게요 현재는 질문 DB가 크지 않아서 성능상 큰 문제는 없겠지만,
쿼리적으로 더 잘 해결한다면 추후 질문 DB가 늘어났을 때 크게 체감될 것 같습니다!

지금 코드들이 전체적으로 정돈이 안된 부분도 엄청 많아서 대대적인 리팩토링 꼭 해야할 것 같아요...!


@Transactional
public void rerollChange(Long userId, Long questionId) {
User user = getUserById(userId);
Question question = questionRepository.findById(questionId)
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_QUESTION));

// 하루에 한번만 질문 새로고침 가능
LocalDateTime lastRerollChange = user.getLastRerollChange();
LocalDateTime now = LocalDateTime.now();
if (lastRerollChange != null) {
Duration duration = Duration.between(lastRerollChange, now);
long hoursPassed = duration.toHours();

if (hoursPassed < 24) {
throw new CustomException(ErrorType.INVALID_REROLL_ONCE_A_DAY);
}
}

Parentchild parentchild = user.getParentChild();
List<QnA> qnaList = parentchild.getQnaList();
QnA currentQnA = qnaList.get(parentchild.getCount() - 1);

// 새로고침으로 버린 질문은 블랙리스트에 추가
parentchild.addQuestionBlackList(currentQnA.getQuestion().getId());
currentQnA.changeQuestion(question);
user.updateLastRerollChange();
}

@NotNull
private Parentchild getParentchild(Long userId) {
Parentchild parentchild = getUserById(userId).getParentChild();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ public enum ErrorType {
// Closer
INVALID_COUNT_STATUS(HttpStatus.BAD_REQUEST, "count 조건으로 인해 다음 가까워지기 질문으로 넘어갈 수 없습니다."),

// Reroll
INVALID_REROLL_BEFORE_SEVEN(HttpStatus.BAD_REQUEST, "7일 이후에만 질문 새로고침을 할 수 있습니다."),
INVALID_REROLL_AFTER_ANSWER(HttpStatus.BAD_REQUEST, "답변이 진행되기 이전에만 질문 새로고침을 할 수 있습니다."),
INVALID_REROLL_ONCE_A_DAY(HttpStatus.BAD_REQUEST, "하루에 한번만 질문 새로고침을 할 수 있습니다."),


/**
* 401 UNAUTHORIZED
*/
Expand Down Expand Up @@ -67,7 +73,7 @@ public enum ErrorType {
PARENTCHILD_HAVE_NO_OPPONENT(HttpStatus.NOT_FOUND, "부모자식 관계에 1명만 참여하고 있습니다."),
NOT_FOUND_SECTION(HttpStatus.NOT_FOUND, "해당 아이디와 일치하는 섹션이 없습니다."),
NOT_FOUND_ALBUM(HttpStatus.NOT_FOUND, "존재하지 않는 앨범입니다."),
NOT_FOUND_CLOSER_QUESTION(HttpStatus.NOT_FOUND, "일치하는 가까워지기 질문이 없습니다."),
NOT_FOUND_QUESTION(HttpStatus.NOT_FOUND, "해당 아이디와 일치하는 Question 데이터가 없습니다."),

/**
* About Apple (HttpStatus 고민)
Expand Down Expand Up @@ -105,9 +111,8 @@ public enum ErrorType {
* 501 NOT_IMPLEMENTED
*/
NEED_MORE_QUESTION(HttpStatus.NOT_IMPLEMENTED, "남은 질문이 없습니다. 질문을 추가해주세요."),
NO_MORE_CLOSER_QUESTION(HttpStatus.NOT_IMPLEMENTED, "남은 가까워지기 질문이 없습니다."),
MAX_LIMIT_ALBUM_UPLOAD(HttpStatus.NOT_IMPLEMENTED, "한 부모자식마다 최대 15개의 업로드까지만 허용합니다."),


;

private final HttpStatus httpStatus;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ public enum SuccessType {
GET_TODAY_CLOSER_QNA_SUCCESS(HttpStatus.OK, "오늘의 가까워지기 문답 조회에 성공했습니다."),
ANSWER_TODAY_CLOSER_QUESTION_SUCCESS(HttpStatus.OK, "오늘의 가까워지기 문답에 답변을 완료하였습니다."),
PASS_TO_NEXT_CLOSER_QUESTION_SUCCESS(HttpStatus.OK, "다음 가까워지기 문답으로 넘어가는 데에 성공했습니다."),
GET_REROLL_CHECK_SUCCESS(HttpStatus.OK, "새로고침 할 수 있는 질문 조회에 성공했습니다."),
REROLL_CHANGE_SUCCESS(HttpStatus.OK, "질문 새로고침이 완료되었습니다."),

/**
* 201 CREATED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ default Optional<CloserQuestion> findRandomExceptIds(List<Long> ids) {
List<CloserQuestion> allQuestions = findAll();

if (allQuestions.isEmpty()) {
throw new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION);
throw new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION);
}
if (ids.isEmpty()) {
int randomIndex = random.nextInt(allQuestions.size());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ public class Parentchild extends AuditingTimeEntity {
@OneToMany(mappedBy = "parentchild", fetch = FetchType.LAZY, cascade = CascadeType.PERSIST)
private final List<Album> albumList = new ArrayList<>();

@Column(name = "question_id", nullable = false)
@ElementCollection
private List<Long> questionBlackList;

public void addQuestionBlackList(Long questionId) {
questionBlackList.add(questionId);
}

Comment on lines +42 to +49
Copy link
Member

Choose a reason for hiding this comment

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

ID 리스트로 관리하는 게 Question 객체 리스트로 유지하는 방식보다 훨씬 좋은 것 같네요 !!

@Column(nullable = false)
private int count;

Expand Down
Loading
Loading