Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tnals0924
left a comment
tnals0924
left a comment
There was a problem hiding this comment.
PR 설명이 꼼꼼해서 화면마다 어떤 엔드포인트가 필요한지 바로 이해됐어요. 임시저장을 서버로 옮긴 판단과 시간대 버그를 같이 잡아주신 것도 좋았습니다. 감사합니다!
꼭 반영이 필요해 보이는 건 두 가지예요. 자세한 내용은 각 줄 코멘트에 적었습니다.
- 회차 API 경로를
/v1/admin/feedback/rounds로 바꾸기 (AdminFeedbackRoundController) - 운영진 전용 메서드를
AdminFeedbackService로 나누기 (FeedbackService)
리뷰 포인트 주신 내용은 해당 줄에 의견을 달아 두었고, 줄에 달기 어려운 것만 여기 적을게요.
- 임시저장/공개를 나누고 명세를 고친 건 좋아 보여요. 위 경로 변경도 노션 명세와 프론트에 같이 공유 부탁드립니다.
answerCount에 임시저장분이 들어가는 건 그대로 둬도 될 것 같아요. 공개가 회차 단위라 공개 전에는 전부 임시저장, 공개 후에는 전부 공개라서status와 같이 보면 구분돼요. 화면 라벨만 "작성된 답변 수"처럼 맞추면 될 것 같습니다.- 부서 권한을 공지와 맞춰 ADMIN이면 되게 둔 것도 동의해요.
그 밖에 PR 전체에 걸친 것 두 가지예요.
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/v1/admin/feedback-rounds") |
There was a problem hiding this comment.
[r] 회차 경로를 /v1/admin/feedback/rounds로 바꾸면 어떨까요? 회비도 /v1/admin/fee/requests·/v1/admin/fee/account처럼 도메인 경로 아래에 하위 자원을 두고 있어요. 회차도 열린피드백에 딸린 자원이라 같은 모양으로 두면 경로만 보고 어느 도메인 API인지 알 수 있을 것 같아요. 같은 이유로 질문 목록(/v1/admin/feedbacks)도 /v1/admin/feedback/questions로 맞추면 운영진 열린피드백 API가 한 경로 아래로 모일 것 같고요.
프론트가 붙기 전에 바꾸는 게 부담이 적어서 [r]로 남겨요. 바꾸시면 노션 명세에도 같이 반영 부탁드립니다. 리뷰 포인트 주신 컨트롤러 분리는 하위 자원이 다르니 지금처럼 둘로 두는 게 좋아 보여요!
| List<Integer> getRoundsOf(int year); | ||
|
|
||
| // 접수 기간만 받아 회차를 연다. 연도는 시작일 기준, 회차 번호는 그 연도의 최대 + 1로 서버가 배정한다 | ||
| FeedbackRoundSummary createRound(LocalDate opensOn, LocalDate closesOn); |
There was a problem hiding this comment.
[r] 이번에 추가된 createRound~publishAnswers는 모두 운영진 화면에서만 쓰여서, coding-style.md 2-7절대로 AdminFeedbackService로 나누면 좋겠어요. 컨벤션에 "운영진 전용 서비스가 필요한 도메인부터 점진적으로 나눈다"고 되어 있는데, 열린피드백은 이번 PR로 운영진 기능이 처음 생겨서 지금이 나눌 시점 같아요. 공지도 학생 조회는 NoticeService, 운영진 목록·작성·수정·삭제는 AdminNoticeService로 나뉘어 있고요.
이렇게 나누면 될 것 같아요.
AdminFeedbackService와AdminFeedbackServiceImpl을 같은service·service.impl패키지에 두고, 두 레포지토리를 같이 써요.FeedbackServiceImpl에 있는 회차 조회·집계·페이징 private 메서드는 운영진 쪽에서만 쓰여서 그대로 옮기면 돼요.- 두 서비스는 서로 호출하지 않으니(
architecture.md레이어 규칙 4), 학생 쪽getById·search·create·getAvailableYears·getRoundsOf는FeedbackService에 그대로 남겨요. - 컨트롤러와 두 UseCase는
AdminFeedbackService를 주입받게 바꿔주시면 돼요.
학생 앱 기능을 고칠 때와 운영진 기능을 고칠 때 건드리는 파일이 갈려서, 다음 PR(학생 앱 노출 차단) 리뷰 범위도 더 선명해질 것 같아요.
| * 답변을 단다. 이미 답변이 있으면 덮어쓴다 — 답변 수정이 같은 경로로 들어오기 때문이다. | ||
| * record라 값을 바꾸지 않고 새 인스턴스를 돌려준다. | ||
| */ | ||
| public Feedback withAnswer(String answer, Long answeredBy, LocalDateTime answeredAt) { |
There was a problem hiding this comment.
[c] 이번 PR로 Feedback에 답변을 다는 상태 변경이 생겼는데, record라서 withAnswer가 필드 9개를 전부 다시 넘겨 새 인스턴스를 만들고 있어요. 필드가 하나 늘면 create·of·withAnswer 세 곳의 생성자 호출을 같이 고쳐야 해요. 게다가 같은 타입(LocalDateTime 두 개, Long 세 개)이 섞여 있어서 자리가 바뀌어도 컴파일러가 잡아주지 못하고요.
상태가 바뀌는 다른 도메인 객체(Member, Locker, Event, StudentTransferStatus 등)는 모두 @Getter @EqualsAndHashCode @AllArgsConstructor(access = AccessLevel.PRIVATE) 클래스에 의도가 드러나는 변경 메서드를 두고 있어요. Feedback도 같은 형태로 맞추면 좋겠어요.
@Getter
@EqualsAndHashCode
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class Feedback {
private Long id;
private int year;
private int round;
private String question;
private LocalDateTime questionedAt;
private String answer;
private LocalDateTime answeredAt;
private Long createdBy;
private Long answeredBy;
// create·of는 지금과 같다
public boolean isAnswered() {
return answer != null;
}
// 답변을 작성한다. 이미 답변이 있으면 덮어쓴다 — 답변 수정이 같은 경로로 들어오기 때문이다
public void writeAnswer(String answer, Long answeredBy, LocalDateTime answeredAt) {
this.answer = answer;
this.answeredBy = answeredBy;
this.answeredAt = answeredAt;
}
}이렇게 하면 withAnswer가 없어지고, 이름만 봐도 무엇을 하는지 드러나는 writeAnswer가 그 자리를 대신해요. 접근자가 feedback.id()에서 feedback.getId()로 바뀌니, FeedbackJpaEntity·응답 DTO·Feedback::id 같은 메서드 참조도 같이 바꿔주셔야 해요. FeedbackRound.publishAnswers도 같은 모양이라 함께 클래스로 맞추면 일관될 것 같습니다.
| * 관리자 화면이 말하는 "진행 중 회차"(SCHEDULED·OPEN·ANSWERING)가 이 조건과 같다 — | ||
| * 접수가 끝나 답변을 쓰는 중인 회차도 포함해야 답변 작성 화면이 질문을 받아올 수 있다. | ||
| */ | ||
| Optional<FeedbackRound> findLatestUnpublished(); |
There was a problem hiding this comment.
[c] 리뷰 포인트 주신 "진행 중 회차" 정의에 대한 의견이에요. 접수가 끝나 답변을 쓰는 회차까지 잡아야 한다는 방향은 좋아요. 그런데 "가장 최근"을 연도·회차 번호가 가장 큰 것으로 보다 보니, 다음 회차를 미리 등록해 두면 그쪽이 잡힐 것 같아요. 예를 들어 2차가 질문접수중일 때 3차(다음 달 시작)를 미리 등록하면 이렇게 돼요.
/current요약이 3차(오픈예정, 질문 0건)를 보여줘요.roundId없이 여는 질문 탭도 3차를 보여줘서 빈 목록이 돼요.
회차 번호도 등록 순서대로 붙어서(그 연도 최대 + 1), 나중에 더 이른 기간의 회차를 등록하면 번호 순서와 기간 순서가 어긋날 수 있어요. 공개 전 회차 중 접수가 이미 시작된 것(opensAt <= now)을 시작일이 늦은 순으로 먼저 고르고, 그런 회차가 없을 때만 가장 가까운 오픈예정 회차를 고르는 쪽이 화면 의도에 더 맞을 것 같아요.
|
|
||
| @Override | ||
| @Transactional | ||
| public FeedbackRoundDetail publishAnswers(Long roundId) { |
There was a problem hiding this comment.
[c] 접수 기간 중에도 답변을 공개할 수 있는데, 그러면 상태가 어긋나는 것 같아요. FeedbackRoundStatus는 공개 여부를 먼저 봐서 바로 CLOSED(회차종료)가 돼요. 반면 학생 질문 등록은 findOpenAt으로 기간만 보니 그 회차에 질문이 계속 들어와요.
- 관리자 화면에는 "회차종료"인데 질문이 계속 늘어나요.
findLatestUnpublished에서 빠져서 기본 질문 탭에도 보이지 않아요.- 그 뒤에 단 답변은 임시저장 단계 없이 바로 학생에게 보여요.
공개는 접수가 끝난 회차(ANSWERING)에서만 받도록 막는 게 어떨까요? 오픈예정·질문접수중이면 409로 막고, 이미 공개한 회차는 지금처럼 멱등으로 두면 될 것 같아요. 아래 "답변이 없으면 막는" 검사와 같이 validatePublishable(...) 같은 validate~ 메서드로 빼면 읽기도 편할 것 같고요.
| * 연도는 시작일 기준이고, 회차 번호는 그 연도의 다음 번호를 호출부가 계산해 넘긴다. | ||
| */ | ||
| public static FeedbackRound create(int round, LocalDate opensOn, LocalDate closesOn) { | ||
| if (closesOn.isBefore(opensOn)) { |
There was a problem hiding this comment.
[a] 저희가 throw하는 검증은 validate~ 메서드로 빼서 부르고 있어서, 여기는 validatePeriod(opensOn, closesOn)처럼 맞추면 좋겠어요. FeedbackServiceImpl.createRound의 기간 겹침 검사도 validateNotOverlapped(round)처럼 같이 빼주시면 좋을 것 같습니다.
|
|
||
| // 질문에 작성자를 붙인 결과. DTO 변환은 Controller가 한다. | ||
| // student는 탈퇴(소프트 삭제)한 회원의 질문이면 null일 수 있다. | ||
| public record FeedbackWithStudent(Feedback feedback, Member student) { |
There was a problem hiding this comment.
[a] 작성자 타입은 Member인데 필드·변수·응답에서는 student(student, students, withStudents, studentName)로 불려서, 같은 대상을 두 이름으로 부르게 되는 것 같아요. 회비 관리자 목록의 MemberFeeStatus(StudentTransferStatus status, Member member)와 같은 조합이라, 이름도 그쪽에 맞춰 MemberFeedback으로 두고 member로 통일하면 어떨까요?
// 질문에 작성자를 붙인 결과. DTO 변환은 Controller가 한다.
// member는 탈퇴(소프트 삭제)한 회원의 질문이면 null일 수 있다.
public record MemberFeedback(Feedback feedback, Member member) {
public String memberName() {
return member == null ? null : member.getName();
}
}AdminFeedbackResponse와 FeedbackAnswerResponse에 똑같이 있는 student == null ? null : student.getName()도 memberName() 하나로 모여서, 탈퇴 회원 처리가 한 곳에서 끝나요.
UseCase의 students는 members로 바꾸고, withStudents는 하는 일이 드러나게 attachMembers처럼 바꾸면 좋겠어요. 지금 이름은 무엇을 하는지(작성자를 조회해서 붙인다)가 드러나지 않아서요. 응답 필드 studentName도 회비 응답의 memberId처럼 memberName으로 맞추면 좋겠는데, 프론트와 이미 맞춘 이름이라면 그대로 두셔도 괜찮습니다!
| List<@Valid @NotNull(message = "답변을 입력해 주세요.") Item> answers | ||
| ) { | ||
|
|
||
| public List<FeedbackAnswerCommand> toCommands() { |
There was a problem hiding this comment.
[a] 같은 feedbackId가 두 번 오면 둘 다 저장되고 나중 것이 남아요. 화면에서 그럴 일은 드물겠지만 요청만 보고는 어느 답변이 남을지 알 수 없어요. toCommands() 안에서 validateFeedbackIdsNotDuplicated()로 막아두면 좋을 것 같아요(MemberSignUpRequest의 약관 중복 검사와 같은 방식이에요).
| * 다 읽어도 수십 건이다. 회차가 수백 개로 늘면 상태를 질의할 수 있는 형태로 바꾸는 쪽을 다시 보자. | ||
| */ | ||
| @Override | ||
| public PageResult<FeedbackRoundSummary> getRounds( |
There was a problem hiding this comment.
[a] 리뷰 포인트 주신 메모리 페이징은 지금 규모에서 괜찮아 보여요. 회차가 연 5~10개라 전체를 읽어도 수십 건이고, 상태를 컬럼으로 두면 시각이 지날 때마다 갱신해야 하는 문제가 더 커 보여요. 다시 볼 기준을 주석에 남겨주신 것도 좋습니다!
| import java.time.LocalDateTime; | ||
|
|
||
| // 회차 한 건의 현황 — 기간·상태·질문 수·답변 수. 관리자 요약 화면과 회차 목록이 같은 모양을 쓴다. | ||
| public record FeedbackRoundSummary( |
There was a problem hiding this comment.
[a] 리뷰 포인트 주신 FeedbackRoundSummary/FeedbackRoundDetail은 응답 모양을 따라 만든 묶음이라기보다 "상태 판정에 기간과 집계가 같이 필요하다"는 도메인 이유가 있어서 괜찮아 보여요. 행사(EventSummary·EventDetail)와 아카이브(ArchiveDetail)도 같은 이름으로 도메인에 두고 있고요.
#️⃣연관된 이슈
🎯 해결하려는 문제가 무엇인가요?
학생 앱의 열린피드백은 이미 돌아갑니다 — 질문을 쓰고 목록을 보는 쪽이죠. 그런데 회차를 여는 것도, 답변을 다는 것도 지금은 할 방법이 없습니다. DB에 직접 INSERT 하지 않으면 운영이 안 되는 상태였습니다.
관리자
열린피드백 관리화면이 요구하는 7개 엔드포인트를 만들었습니다.GET /v1/admin/feedback-rounds/currentGET /v1/admin/feedback-roundsPOST /v1/admin/feedback-roundsGET /v1/admin/feedback-rounds/{roundId}PATCH /v1/admin/feedback-rounds/{roundId}/answersPOST /v1/admin/feedback-rounds/{roundId}/answers/publishGET /v1/admin/feedbacks❓ 왜 해결해야 하나요?
답변 3종 컬럼(
answer/answered_at/answered_by)은 V3부터 있었는데 채우는 코드가 없어 지금까지 전부 NULL이었습니다. 질문만 받고 답은 못 다는 게시판이라 기능이 반쪽입니다.⭐ 어떻게 해결했나요?
1. 임시저장을 서버에 둡니다 (명세를 고쳤습니다)
원래 명세는
임시저장을 "브라우저 localStorage"로 적어뒀는데 그대로 가면 안 됐습니다. 답변 작성은 질문 여러 개에 각각 글을 쓰는 일이라 한 자리에서 끝나지 않습니다. localStorage면 기기를 바꾸면 사라지고, 둘이 나눠 쓸 수도 없습니다 — 두 관리자가 같은 회차를 열면 서로의 작업을 모른 채 나중에 저장한 쪽이 이깁니다.그래서 저장과 공개를 나눴습니다.
feedback_rounds.answers_published_at(V22)이 둘을 가릅니다. 공개 단위는 회차입니다 — 화면이 "질문 여러 개를 골라 한 번에 등록"이고, 학생 입장에서도 "N차 답변이 올라왔다"가 자연스럽습니다.2. 회차 상태는 저장하지 않고 파생합니다
명세 초안은
ANSWERING/CLOSED를 "답변 0건 / 1건 이상"으로 갈랐는데, 그러면 질문 8개 중 1개만 답해도 회차가 "종료"로 뜹니다. 공개 개념이 생기면서 "공개 전/후"로 깔끔하게 갈렸습니다.3. 검색어가 두 도메인에 걸치는 건 ID를 징검다리로 넘깁니다
keyword는 질문 내용 또는 작성자 이름에 걸립니다. 이름은 member에 있어 welfare가 조인할 수 없으니, 회비 관리자 목록에서 쓴 패턴을 그대로 가져왔습니다.searchIdsByKeyword("김학생")→[3, 17, 42]WHERE ... (question LIKE ? OR created_by IN (3,17,42)).created_by는 welfare 자기 테이블의BIGINT라 welfare는 그게 "회원"인 줄도 모릅니다getMapByIds(...)member 모듈은 한 줄도 바뀌지 않습니다. 페이징도 여전히 welfare 쿼리의
LIMIT이 합니다.4. 시간대 버그를 같이 고쳤습니다 (첫 커밋)
FeedbackServiceImpl.create와Feedback.create가LocalDateTime.now()를 시간대 없이 쓰고 있었습니다. 컨테이너가 UTC면 여기만 9시간 어긋나고, 하필 그 값이 "지금이 접수 기간인가"를 판정하는 데 쓰여 경계에서 질문이 엉뚱한 회차로 가거나 막힐 수 있었습니다.DateUtil.KST를 명시하고, 시각을 한 번만 읽어 회차 판정과questionedAt에 같은 값이 들어가게 했습니다.🧩 이 PR의 한계 & 트레이드오프
회차 목록은 "페이징은 하지만 실제로는 전건을 읽습니다."
status가 저장 안 된 파생값이라WHERE를 걸 수 없고, 따라서LIMIT도 걸 수 없습니다. 연도까지만 SQL로 좁히고 상태 필터와 페이징은 메모리에서 합니다.GROUP BY year, round)members.fee_status를 넣었다가 되돌린 것과 같은 종류의 실수가 됩니다기간 겹침 검사에 동시성 구멍이 있습니다. 범위 겹침은 DB 유니크로 막을 수 없어 조회-후-삽입입니다. 회차 번호 자동 배정(그 연도 최대 + 1)도 같습니다 — 동시에 두 건이 들어오면
uk_feedback_rounds_year_round가 막아주지만 500이 납니다. 회차를 등록하는 사람이 소수라 전화번호 중복 때와 같은 수준으로 봤습니다.searchIdsByKeyword는 매칭 ID를 전부 돌려줍니다. 검색어가 "김"이면 수천 개가IN에 들어갑니다. 기존 회비 목록도 같은 한계를 안고 있어 새 문제는 아니지만, 검색이 느려지면 다시 봐야 합니다.공개한 뒤 답변을 더 쓰면 그 자리에서 바로 보입니다. 회차 단위 공개라 그렇습니다. 빠뜨린 답을 나중에 채우는 상황에 맞아서 막지 않았습니다.
⛓️ 기존 기능에 미치는 영향
학생 앱은 아직 바뀌지 않습니다. 지금
FeedbackResponse가answer·answeredAt을 아무 필터 없이 내려주는데, 비공개 회차의 답변을 가리는 작업은 다음 스택 PR로 분리했습니다.나눈 이유는 건드리는 영역과 리뷰 포인트가 다르기 때문입니다. 이 PR은 새 코드가 대부분이고, 다음 PR은 이미 돌아가는 학생 앱 조회를 바꾸는 일이라 회귀 위험이 다릅니다.
순서가 어긋나도 사고는 안 납니다 — 이 PR만 머지된 상태에서 답변을 임시저장하면 학생에게 보이지만, 그 사이에 임시저장된 답변이 없으면 노출도 없습니다. 둘을 같은 배포에 올리거나, 다음 PR 전까지 답변 작성을 미루면 됩니다.
그 밖에:
FeedbackRound.of에answersPublishedAt인자가 하나 늘었습니다. 호출부는FeedbackRoundJpaEntity하나입니다Feedback.create가questionedAt을 받습니다. 호출부는FeedbackServiceImpl.create하나입니다members테이블과 member 모듈은 건드리지 않았습니다NULL허용 컬럼 추가 하나라 백필이 필요 없습니다(운영 데이터에 답변이 한 건도 없어 전부 비공개가 맞습니다)🔀 Edge Case & 실패 시나리오
FEEDBACK_ROUND_OVERLAPPED— 안 막으면 학생 질문 등록이 깨집니다INVALID_FEEDBACK_ROUND_PERIODMAX(round)가 null이라 1차로 배정FEEDBACK_NOT_IN_ROUND. 아예 없는 질문이면 404FEEDBACK_NOT_FOUND로 나눠서 알려줍니다FEEDBACK_ANSWER_NOT_FOUNDFeedbackRound.publishAnswers가this를 돌려줍니다roundId생략인데 진행 중 회차 없음GROUP BY결과에 안 나오므로FeedbackCount.empty로 0을 채웁니다IN ()은 JPQL이 깨지므로IN (0)으로 치환 — 어떤created_by와도 안 맞습니다studentName이null(회비 목록과 같은 방식)@Transactional이라 전부 롤백📋 검토한 대안과 선택 이유
(1) 검색 —
infrastructure:db에서 조인하기. db 모듈은 두 엔티티를 다 보므로JOIN members를 쓸 수 있고, 조인이WHERE에만 쓰여 포트 시그니처에Member가 나오지 않습니다(쿼리 1번으로 줄어듭니다). 그런데 member가 컬럼명을 바꾸면 welfare 쿼리가 조용히 깨집니다. 결합을 피하는 쪽을 택했고, 검색이 느려지면 다시 보겠습니다.(2) 상태 — 컬럼으로 저장하기. 위 "한계" 참고. 회비 때와 같은 실수라 접었습니다.
(3) 답변 — 단건 엔드포인트(
PATCH /feedbacks/{id}/answer). 관리자 행동 1회가 답변 N건이라 클라가 N번 호출해야 하고, 중간에 실패하면 일부만 저장된 채 화면은 완료로 넘어갑니다. 회차 일괄로 바꿨습니다. 재답변(수정)도 같은 엔드포인트라FEEDBACK_ALREADY_ANSWERED는 쓰지 않습니다.(4) 답변 단위 공개. 세밀하지만 화면에 그런 조작이 없습니다. 회차 단위로 뒀습니다.
(5) 학생 앱 노출 차단 —
open_feedbacks에 플래그 복사. 상태를 두 곳에 두게 되므로 접었습니다. 다음 PR에서 공개된(year, round)를 넘기는 쪽으로 갑니다.(6) 부서 권한. 회비 관리는
@RequireDepartment(GENERAL_AFFAIRS)로 제한하지만 공지 관리는 제한이 없습니다. 열린피드백도 게시판 도메인이라 공지와 맞춰 ADMIN이면 되게 뒀습니다. 다르게 가야 하면 알려주세요.💬 리뷰 포인트
[r]FeedbackServiceImpl.getRounds의 메모리 페이징 — "페이징인데 전건을 읽는다"가 지금 규모에서 받아들일 만한지. 주석에 기준을 남겨뒀습니다[r]임시저장/공개 분리가 명세 변경을 동반합니다 — 노션은 이미 갱신했고피드백 답변 공개페이지를 새로 만들었습니다. 프론트와 합의가 더 필요한지[r]GET /v1/admin/feedbacks에서roundId생략 시 "진행 중 회차"의 정의 —findOpenAt(접수 기간 안)이 아니라answersPublishedAt IS NULL인 가장 최근 회차로 잡았습니다. 접수가 끝나 답변을 쓰는 중(ANSWERING)인 회차도 잡혀야 답변 작성 화면이 질문을 받아올 수 있어서입니다. 요약 조회(/current)도 같은 정의를 씁니다[c]FeedbackRoundSummary/FeedbackRoundDetailVO를 둔 것 — "응답 모양을 따라 만든 묶음"으로 보이지 않는지. 상태 판정에 기간과 집계가 같이 필요해서 묶었습니다[c]answerCount에 임시저장분이 포함됩니다 — 관리자 진행 상황을 보여주는 값이라 그렇게 뒀는데, 화면 라벨이답변 수라 혼동 여지가 있습니다[a]컨트롤러를 둘로 나눴습니다(/feedback-rounds,/feedbacks) — 기준 경로가 달라서입니다NotificationType주석의 "게시판 — 새 공지·열린피드백 답변")은 알림 스택([Feat/#113] 아웃박스 릴레이 추가 #117·[Feat/#114] 알림 발송 공통 처리 추가 #118) 머지 뒤에 따로 잇겠습니다