Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
DB 스키마 sql/init.sql |
admin, inquiry, inquiry_image_file, inquiry_answer 테이블 추가(외래키·타임스탬프·유니크 제약 포함). |
Admin 도메인 src/main/java/im/toduck/domain/admin/... |
엔티티 Admin, 리포지토리(+QueryDSL custom/impl), 서비스 AdminService, 유스케이스 AdminUseCase, 매퍼 AdminMapper, API/컨트롤러, DTO(요청·응답) 추가 — CRUD, 사용자 역할 전환(promote/demote), 복구 로직 포함. |
Inquiry 도메인 (엔티티·리포지토리) src/main/java/im/toduck/domain/inquiry/persistence/... |
엔티티 Inquiry, InquiryImage, InquiryAnswer, enum Type/Status 추가. soft-delete 구현(어노테이션·프로그램적 softDelete 혼재), 연관관계 관리 메서드 및 unique 제약 추가. |
Inquiry 도메인 (서비스/유스케이스/매퍼/컨트롤러/DTO) src/main/java/im/toduck/domain/inquiry/... |
InquiryService, InquiryAnswerService, InquiryUseCase, InquiryAnswerUseCase, 매퍼(InquiryMapper/InquiryImgMapper), API/컨트롤러, 요청·응답 DTO 및 검증 로직 추가. 이미지·답변 CRUD와 상태 전환 비즈니스 로직 포함. |
QueryDSL 구현 src/main/java/.../persistence/repository/querydsl/* |
Inquiry/Admin 관련 커스텀 리포지토리 인터페이스 및 QueryDSL 기반 구현 추가(이미지·답변 포함 조회, deleted 포함/제외 조회). |
이미지 엔티티 동작 변경 src/main/java/im/toduck/domain/diary/..., .../events/detail/... |
DiaryImage/EventsDetailImg에서 ORM @SQLDelete/@SQLRestriction 제거 또는 softDelete 메서드 추가, orphanRemoval 제거 — 삭제 흐름에서 자식 소프트삭제 호출 추가/변경. |
예외 코드·유틸 변경 src/main/java/im/toduck/global/exception/ExceptionCode.java, src/main/java/im/toduck/domain/user/..., src/main/java/im/toduck/domain/mypage/... |
문의·답변·관리자 관련 ExceptionCode 6개 추가. User에 role 전환 메서드(promoteToAdmin/demoteToUser) 추가. MyPage 유저 삭제 시 Admin 삭제 연동 추가. |
테스트 추가/수정 src/test/java/im/toduck/domain/admin/..., .../inquiry/..., .../events/... |
Admin 및 Inquiry/InquiryAnswer 관련 통합·단위 테스트 추가 및 Events 테스트 중복 제거. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Client as 사용자
participant API as InquiryController
participant UseCase as InquiryUseCase
participant UserSvc as UserService
participant Service as InquiryService
participant Repo as InquiryRepository
Client->>API: 문의 생성 요청 (InquiryCreateRequest)
API->>UseCase: createInquiry(request, userId)
UseCase->>UserSvc: getUser(userId)
UserSvc-->>UseCase: User
UseCase->>Service: createInquiry(request, user)
Service->>Repo: save(inquiry)
Repo-->>Service: 저장된 Inquiry
Service->>Service: addInquiryImages(inquiry, imgs)
Service-->>UseCase: Inquiry 반환
UseCase-->>API: InquiryResponse
API-->>Client: ApiResponse(200)
sequenceDiagram
autonumber
participant Admin as 관리자
participant API as InquiryAnswerController
participant UseCase as InquiryAnswerUseCase
participant AdminSvc as AdminService
participant AnswerSvc as InquiryAnswerService
participant InquirySvc as InquiryService
participant Repo as InquiryAnswerRepository
Admin->>API: 답변 생성 요청 (inquiryId, content)
API->>UseCase: createInquiryAnswer(request, adminUserId)
UseCase->>AdminSvc: getAdminBySameUser(adminUserId)
AdminSvc-->>UseCase: Admin
UseCase->>AnswerSvc: createInquiryAnswer(request, admin)
AnswerSvc->>Repo: findAnyByInquiryIdIncludingDeleted(inquiryId)
Repo-->>AnswerSvc: 기존 답변 반환(있음/없음)
alt 이미 답변 존재 (비삭제)
AnswerSvc-->>UseCase: ALREADY_ANSWERED_INQUIRY 예외
else 신규 또는 복구
AnswerSvc->>InquirySvc: update inquiry.status -> ANSWERED
InquirySvc-->>AnswerSvc: 상태 반영
AnswerSvc->>Repo: save(answer)
Repo-->>AnswerSvc: 저장된 Answer
end
AnswerSvc-->>UseCase: Answer 반환
UseCase-->>API: ApiResponse(200)
API-->>Admin: 응답
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50분
Possibly related PRs
- Main <- Develop #155 — DiaryImage/EventsDetailImg 관련 soft-delete 변경과 유사한 ORM 삭제 처리 수정.
- Main <- Develop #149 — 동일한 파일(ExceptionCode) 변경을 포함하여 에러코드 확장 연관.
- 일기 키워드 기능 개발 #129 — Diary 엔티티 관련 변경(연관 필드 수정)과 중복 영역 존재.
Suggested labels
✨ Feature
Suggested reviewers
- kang20
- Seol-JY
Poem
🐰 새 엔티티 띄워 토끼도 폴짝
관리자와 문의가 줄지어 왔네
답변 달고 사진 매달고
지울 땐 살짝, 되살릴 땐 또 반짝
당근으로 축하할게요 🥕✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.86% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목은 '문의, 문의 답변, 관리자 기능 구현'으로 변경사항의 핵심 내용을 명확하게 요약하고 있습니다. |
| Description check | ✅ Passed | PR 설명이 템플릿 구조를 따르고 있으며, 작업 내용(문의, 문의 답변, 관리자 기능)이 구체적으로 작성되어 있습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feature/inquiry
Tip
Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/im/toduck/domain/diary/domain/usecase/DiaryUseCase.java (1)
54-57:⚠️ Potential issue | 🟡 Minor로그 메시지 오류: "소셜 게시판" → "일기"로 수정 필요
Line 55의 로그 메시지가 "소셜 게시판 삭제 시도"로 되어 있지만, 이 메서드는 일기(Diary) 삭제를 처리합니다. 복사-붙여넣기 오류로 보입니다.
🔧 수정 제안
if (!isDiaryOwner(diary, user)) { - log.warn("권한이 없는 유저가 소셜 게시판 삭제 시도 - UserId: {}, DiaryId: {}, ", user.getId(), diary.getId()); + log.warn("권한이 없는 유저가 일기 삭제 시도 - UserId: {}, DiaryId: {}", user.getId(), diary.getId()); throw CommonException.from(ExceptionCode.UNAUTHORIZED_ACCESS_DIARY); }
🤖 Fix all issues with AI agents
In `@src/main/java/im/toduck/domain/admin/common/mapper/AdminMapper.java`:
- Around line 13-31: The two methods toAdminResponse(Admin) and fromAdmin(Admin)
are identical; remove the duplication by deleting fromAdmin(Admin) or making
fromAdmin simply delegate to toAdminResponse by returning
toAdminResponse(admin); then update all call sites to use toAdminResponse(Admin)
(or leave callers unchanged if you keep the delegating method) and run/adjust
any tests that referenced fromAdmin to ensure consistency.
In `@src/main/java/im/toduck/domain/admin/domain/service/AdminService.java`:
- Around line 33-50: The code currently creates an Admin for any user when no
admin record exists; update getAdminBySameUser/createDefaultAdmin to first
verify the user's role before creating an Admin: load the User (via
userRepository.findById) and check that User has the required admin role (e.g.,
user.hasRole(Role.ADMIN) or user.getRole() == Role.ADMIN), and if the role check
fails throw an appropriate CommonException (e.g., FORBIDDEN) instead of creating
the Admin; you can either perform the role check in getAdminBySameUser before
calling createDefaultAdmin or change createDefaultAdmin to accept a validated
User object and only persist when the role check passes.
In `@src/main/java/im/toduck/domain/admin/persistence/entity/Admin.java`:
- Around line 20-25: Admin 엔티티에 soft-delete 필터가 빠져 있어 기본 조회에 삭제된 레코드가 포함될 수 있으니,
Admin 클래스 선언부(현재에 `@Entity/`@Table와 `@SQLDelete가` 붙은 곳)에
org.hibernate.annotations.SQLRestriction을 추가하고 clause를 "deleted_at IS NULL"로 설정해
기본 쿼리에 삭제된 행이 제외되도록 하세요; 어노테이션을 추가한 뒤 필요한
import(org.hibernate.annotations.SQLRestriction)를 추가해 다른 soft-delete
엔티티들(InquiryAnswer, Inquiry, Events 등)과 일관되게 만드세요.
In
`@src/main/java/im/toduck/domain/admin/presentation/controller/AdminController.java`:
- Around line 52-61: The controller methods createAdmin and updateAdmin lack
`@Valid` on their `@RequestBody` parameters so JSR-303 validation on
AdminCreateRequest/AdminUpdateRequest is not triggered; update both method
signatures (createAdmin and updateAdmin) to annotate the request parameters with
`@Valid` and ensure the javax.validation.Valid (or jakarta.validation.Valid)
import is present so `@NotNull/`@Size constraints on the DTOs are enforced and
invalid requests return validation errors.
In
`@src/main/java/im/toduck/domain/inquiry/domain/service/InquiryAnswerService.java`:
- Around line 65-75: In updateInquiryAnswer, calling inquiry.getInquiryAnswer()
can return null and cause an NPE; before calling
inquiryAnswer.updateAnswer(...), check whether inquiry.getInquiryAnswer() is
null or in a deleted state and throw a clear domain exception (e.g.
CommonException.from(...) with a NOT_FOUND or NOT_FOUND_INQUIRY_ANSWER code) if
absent/removed; update the method (references: updateInquiryAnswer,
Inquiry#getInquiryAnswer, InquiryAnswer#updateAnswer) to perform the
null/deleted check and only call updateAnswer when a valid InquiryAnswer exists.
In `@src/main/java/im/toduck/domain/inquiry/domain/service/InquiryService.java`:
- Around line 60-75: The current updateInquiry method in InquiryService always
calls inquiryImgRepository.deleteAllByInquiry(inquiry) when
request.inquiryImgs() is null, causing data loss; change the branch logic so
that if request.inquiryImgs() == null you leave images unchanged, if
request.inquiryImgs().isEmpty() you delete all images via
inquiryImgRepository.deleteAllByInquiry(inquiry), and if non-empty you delete
existing images and call addInquiryImages(inquiry, request.inquiryImgs()) —
update the conditional around inquiryImgRepository.deleteAllByInquiry and
addInquiryImages in updateInquiry to implement this null = no-op, empty =
delete, non-empty = replace behavior.
- Around line 83-85: The deleteInquiry method in InquiryService currently
deletes the Inquiry via inquiryRepository.delete(inquiry) but InquiryAnswer is
not configured for cascading so deleting an Inquiry leaves orphaned
InquiryAnswer rows; update the domain mapping or service to prevent orphans:
either add cascade = CascadeType.ALL (and optionally orphanRemoval = true) to
the `@OneToOne` mapping between Inquiry and InquiryAnswer in the Inquiry (or
InquiryAnswer) entity, or modify InquiryService.deleteInquiry to explicitly
remove the associated InquiryAnswer before calling
inquiryRepository.delete(inquiry) (use the Inquiry#getAnswer or repository
delete method for InquiryAnswer); ensure referential integrity and tests reflect
the chosen approach.
In `@src/main/java/im/toduck/domain/inquiry/domain/usecase/InquiryUseCase.java`:
- Around line 50-67: After fetching the User and Inquiry in updateInquiry,
verify the requester is the inquiry owner by comparing the fetched user's id
with the inquiry owner id (e.g., compare userId to inquiry.getUser().getId() or
inquiry.getUserId()), and if they differ throw an appropriate permission
exception via CommonException.from(ExceptionCode.FORBIDDEN or a project-specific
NOT_AUTHORIZED code); add the identical ownership check to the corresponding
deleteInquiry method so non-owners cannot modify or delete others' inquiries.
In `@src/main/java/im/toduck/domain/inquiry/persistence/entity/Inquiry.java`:
- Around line 89-92: The removeAnswer() method on Inquiry currently calls
inquiryAnswer.setInquiry(null) which conflicts with the non-null FK
(InquiryAnswer.inquiry_id nullable = false) and will cause constraint violations
on flush; fix by either making the FK nullable (update the `@JoinColumn` on the
InquiryAnswer mapping to nullable = true and keep removeAnswer() clearing the
relationship) or by enabling proper child deletion: add orphanRemoval = true to
the OneToOne mapping in Inquiry and change removeAnswer() to remove the child
entity (so the child is deleted instead of setting its inquiry to null). Ensure
you update the mapping on the Inquiry/InquiryAnswer relationship (the OneToOne
in Inquiry and the `@JoinColumn` on InquiryAnswer) and adjust removeAnswer()
accordingly.
In `@src/main/java/im/toduck/domain/inquiry/persistence/entity/InquiryImage.java`:
- Around line 19-33: InquiryImage is missing the same soft-delete Hibernate
annotations as its parent; add matching `@SQLDelete` and `@SQLRestriction`
annotations to the InquiryImage entity (class InquiryImage) so Hibernate uses
the same soft-delete SQL and automatically filters out deleted rows (use the
inquiry_image_file table and id primary key in the SQL). Ensure the clause
matches whatever soft-delete column BaseEntity/softDelete() uses (e.g., "deleted
= true" in the UPDATE SQL and "deleted = false" in the restriction) so behavior
aligns with Inquiry.
In
`@src/main/java/im/toduck/domain/inquiry/persistence/repository/InquiryImgRepository.java`:
- Around line 10-11: The repository method deleteAllByInquiry in
InquiryImgRepository performs hard DELETEs which bypass the soft-delete
implemented on InquiryImage (deletedAt) and may cause N+1 deletes; change this
to a modifying JPQL update that sets deletedAt (e.g., add a `@Modifying` `@Query` on
InquiryImgRepository such as an update statement "UPDATE InquiryImage i SET
i.deletedAt = CURRENT_TIMESTAMP WHERE i.inquiry = :inquiry" with a matching
method name like softDeleteAllByInquiry and `@Param`("inquiry") Inquiry inquiry)
so the soft-delete semantics are preserved and N+1 DELETEs are avoided; ensure
the method is transactional or called within a transaction and that
InquiryRepositoryCustomImpl's existing filters remain compatible.
In
`@src/main/java/im/toduck/domain/inquiry/persistence/repository/querydsl/InquiryRepositoryCustomImpl.java`:
- Around line 29-38: The query in InquiryRepositoryCustomImpl uses a fetch join
with an ON filter (leftJoin(iq.inquiryImages, iqi).on(...)) which causes
Hibernate runtime error; fix by either (A) applying a Hibernate `@Filter` on
InquiryImage (add `@FilterDef` and `@Filter` on the InquiryImage entity to exclude
soft-deleted rows and enable the filter before running the query so you can
safely use fetchJoin on iq.inquiryImages), or (B) convert the repository method
to a DTO projection (select required fields into an Inquiry DTO instead of
fetchJoining entities) to keep soft-delete filtering in the join condition;
choose one approach and update the method in InquiryRepositoryCustomImpl (or the
InquiryImage entity) accordingly.
In
`@src/main/java/im/toduck/domain/inquiry/presentation/api/InquiryAnswerApi.java`:
- Around line 75-77: The deleteInquiryAnswer endpoint is missing authentication:
add an `@AuthenticationPrincipal` CustomUserDetails parameter to the
deleteInquiryAnswer method signature (to match createInquiryAnswer and
updateInquiryAnswer), update any implementing controller/service method
signatures that call or implement InquiryAnswerApi.deleteInquiryAnswer (e.g.,
the controller handler and service delete method) to accept and use the
CustomUserDetails argument, and ensure authorization checks (admin role) use
that principal before performing the delete.
In
`@src/main/java/im/toduck/domain/inquiry/presentation/dto/request/InquiryCreateRequest.java`:
- Around line 7-21: The content field in the InquiryCreateRequest record uses
`@NotNull` which allows empty strings; change the validation annotation on the
String content parameter from `@NotNull` to `@NotBlank` to disallow blank/empty
values and match project conventions (update the annotation on the content
parameter in InquiryCreateRequest and ensure imports reflect
jakarta.validation.constraints.NotBlank instead of NotNull).
In `@src/main/java/im/toduck/domain/mypage/domain/usecase/MyPageUseCase.java`:
- Around line 71-74: The deletion flow in MyPageUseCase currently calls
adminService.getAdmin(user.getId()) which throws when no Admin exists, causing
account deletion to fail for ADMIN-role users without an Admin entity; change
the logic to safely handle missing Admin by using an Optional-returning lookup
(e.g., adminService.getAdminBySameUser(user.getId()) or a new find/getOptional
method) or by catching the exception from adminService.getAdmin and treating it
as "not found" — only call adminService.deleteAdmin(admin) when the Admin is
present, and allow the overall user deletion to continue regardless of Admin
entity absence.
🟡 Minor comments (9)
src/main/java/im/toduck/domain/events/detail/persistence/entity/EventsDetailImg.java-49-51 (1)
49-51:⚠️ Potential issue | 🟡 MinorEventsDetailImg에
@SQLRestriction어노테이션 추가 필요합니다.
softDelete()메서드만으로는 soft delete된 이미지가 조회에서 자동으로 필터링되지 않습니다. 현재findAllByOrderByIdAsc()등의 쿼리 메서드들이 삭제된 항목을 포함할 수 있으며,findAllWithImgs()의 leftJoin도 soft-deleted 이미지를 함께 반환할 수 있습니다.다른 soft delete 엔티티들(Schedule, Diary, Events 등)과 동일하게
@SQLRestriction(value = "deleted_at is NULL")어노테이션을 클래스에 추가하여 일관성을 유지하고 데이터 누출을 방지해주세요.src/main/java/im/toduck/domain/admin/presentation/dto/request/AdminUpdateRequest.java-6-11 (1)
6-11:⚠️ Potential issue | 🟡 MinordisplayName 입력 검증이 없어 빈 문자열이 통과할 수 있습니다.
AdminCreateRequest는@Size검증을 포함하지만 AdminUpdateRequest는 누락되어 있습니다. displayName이 제공될 때 길이 제약을 검증해야 합니다. 단, API 명세상 null은 허용되므로@NotBlank대신@Size만추가하세요.✅ 제안 변경(diff)
import io.swagger.v3.oas.annotations.media.Schema; +import jakarta.validation.constraints.Size; import lombok.Builder; `@Builder` `@Schema`(description = "관리자 수정 요청 DTO") public record AdminUpdateRequest( + `@Size`(max = 255, message = "관리자 표시명은 255자를 초과할 수 없습니다.") `@Schema`(description = "관리자 표시명", example = "토덕 관리자") String displayName ) { }src/main/java/im/toduck/domain/admin/common/mapper/AdminMapper.java-21-23 (1)
21-23:⚠️ Potential issue | 🟡 Minor메서드 이름에 오타가 있습니다.
toLAdminListResponse에서 대문자L이 포함되어 있습니다.toAdminListResponse가 올바른 명명입니다.✏️ 권장 수정안
- public static AdminListResponse toLAdminListResponse(final List<AdminResponse> admins) { + public static AdminListResponse toAdminListResponse(final List<AdminResponse> admins) { return AdminListResponse.toListAdminResponse(admins); }src/main/java/im/toduck/domain/inquiry/presentation/api/InquiryAnswerApi.java-51-54 (1)
51-54:⚠️ Potential issue | 🟡 Minor
updateInquiryAnswer의 에러 문서에NOT_FOUND_INQUIRY_ANSWER가 누락되었습니다.기존 답변을 수정하는 작업이므로 답변이 존재하지 않는 경우에 대한 에러 코드(
NOT_FOUND_INQUIRY_ANSWER)도 문서화해야 합니다.deleteInquiryAnswer에서는 해당 에러를 포함하고 있습니다.📝 권장 수정안
errors = { `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_ADMIN), - `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_INQUIRY) + `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_INQUIRY), + `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_INQUIRY_ANSWER) }src/main/java/im/toduck/domain/inquiry/presentation/dto/response/InquiryResponse.java-44-45 (1)
44-45:⚠️ Potential issue | 🟡 Minor
answerContent필드의 Schema 설명이 잘못되었습니다.
answerContent의@Schema(description = "문의 내용", ...)은 "답변 내용"이어야 합니다. 현재 "문의 내용"으로 되어 있어content필드(Line 24)와 중복됩니다.📝 권장 수정안
- `@Schema`(description = "문의 내용", example = "현재로써는 루틴을 반복할 기간을 따로 설정할 수 있는 기능은 없습니다!") + `@Schema`(description = "답변 내용", example = "현재로써는 루틴을 반복할 기간을 따로 설정할 수 있는 기능은 없습니다!") String answerContent,src/test/java/im/toduck/domain/inquiry/presentation/controller/InquiryAnswerControllerTest.java-4-7 (1)
4-7:⚠️ Potential issue | 🟡 Minor중복 import와 JUnit 버전 불일치가 있습니다.
- Line 4-5:
assertThat이 두 번 import되었습니다 (정적 import와 일반 import).- Line 7:
org.junit.Assert는 JUnit 4 클래스입니다. JUnit 5 테스트에서는org.junit.jupiter.api.Assertions를 사용해야 합니다.🧹 권장 수정안
import static im.toduck.fixtures.user.UserFixtures.*; -import static org.assertj.core.api.Assertions.*; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.SoftAssertions.*; -import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;src/main/java/im/toduck/domain/inquiry/common/mapper/InquiryMapper.java-18-35 (1)
18-35:⚠️ Potential issue | 🟡 Minor답변 작성자 null 안전성 보강
answer가 존재하더라도admin이 null이면 NPE가 발생합니다. soft-delete/참조 정리 상황을 대비해 admin null 체크를 추가하세요.🛠️ 제안 변경
- answer != null ? answer.getAdmin().getDisplayName() : null, + answer != null && answer.getAdmin() != null ? answer.getAdmin().getDisplayName() : null,src/test/java/im/toduck/domain/admin/presentation/controller/AdminControllerTest.java-91-95 (1)
91-95:⚠️ Potential issue | 🟡 Minor목록 순서를 가정한 검증은 플래키할 수 있습니다.
조회 결과는 정렬을 보장하지 않으므로 인덱스 기반 비교는 실패할 수 있습니다. 순서 무관 검증(또는 정렬 후 비교)으로 바꿔주세요.
✅ 개선 예시
- softly.assertThat(adminListResponse.adminDtos().get(0).displayName()).isEqualTo(admin.getDisplayName()); - softly.assertThat(adminListResponse.adminDtos().get(1).displayName()) - .isEqualTo(admin2.getDisplayName()); + softly.assertThat(adminListResponse.adminDtos()) + .extracting("displayName") + .containsExactlyInAnyOrder(admin.getDisplayName(), admin2.getDisplayName());src/main/java/im/toduck/domain/inquiry/presentation/api/InquiryApi.java-43-92 (1)
43-92:⚠️ Potential issue | 🟡 MinorAPI 문서의 예외 정의가 실제 구현과 불일치합니다.
createInquiry는 실제로NOT_FOUND_USER를 발생시키지만 문서에는NOT_FOUND_ADMIN으로 잘못 표기되어 있습니다. 또한updateInquiry와deleteInquiry는NOT_FOUND_USER와ALREADY_ANSWERED_INQUIRY예외를 발생시키지만 문서에는NOT_FOUND_INQUIRY만 명시되어 있습니다.✅ 개선 예시
errors = { - `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_ADMIN) + `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_USER) }errors = { `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_INQUIRY), + `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.NOT_FOUND_USER), + `@ApiErrorResponseExplanation`(exceptionCode = ExceptionCode.ALREADY_ANSWERED_INQUIRY) }
🧹 Nitpick comments (9)
src/main/java/im/toduck/domain/events/detail/domain/usecase/EventsDetailUseCase.java (1)
80-81: 대량 이미지일 경우 일괄 업데이트도 고려해주세요.
이미지 수가 많을 때는 개별 엔티티 로딩/업데이트 대신UPDATE ... SET deleted_at = now()형태의 벌크 쿼리가 더 효율적일 수 있습니다.src/main/java/im/toduck/domain/inquiry/persistence/entity/Type.java (1)
3-8: 더 구체적인 enum 이름을 고려해 보세요.
Type은 너무 일반적인 이름으로, 다른 패키지에서 import 시 혼란을 줄 수 있습니다.InquiryType과 같이 도메인 컨텍스트를 명시하는 이름이 더 명확할 것입니다.♻️ 이름 변경 제안
-public enum Type { +public enum InquiryType { ERROR, USAGE, SUGGESTION, ETC }src/main/java/im/toduck/domain/inquiry/presentation/dto/response/InquiryListResponse.java (1)
8-16: API 응답 필드명 개선 제안
inquiryDtos는 내부 구현 세부사항(DTO)을 노출합니다. API 소비자 관점에서inquiries와 같이 더 직관적인 이름이 좋습니다.♻️ 필드명 변경 제안
`@Schema`(description = "문의 내역 목록 응답") public record InquiryListResponse( `@Schema`(description = "문의 내역 목록") - List<InquiryResponse> inquiryDtos + List<InquiryResponse> inquiries ) {src/main/java/im/toduck/domain/inquiry/common/mapper/InquiryImgMapper.java (1)
10-14: 단일 URL인데 매개변수명이 복수형입니다.
호출부 혼동을 줄이기 위해 단수형으로 맞추는 편이 낫습니다.🛠️ 제안 수정
- public static InquiryImage toInquiryImg(Inquiry inquiry, String imgUrls) { + public static InquiryImage toInquiryImg(Inquiry inquiry, String imgUrl) { return InquiryImage.builder() .inquiry(inquiry) - .url(imgUrls) + .url(imgUrl) .build(); }src/main/java/im/toduck/domain/inquiry/presentation/dto/request/InquiryUpdateRequest.java (2)
11-23: 요청 본문에inquiryId를 포함하는 것은 일반적이지 않습니다.업데이트 요청 DTO에
inquiryId를 포함하는 것은 RESTful API 설계 관례와 맞지 않습니다. 일반적으로 리소스 ID는 경로 변수(@PathVariable)로 전달됩니다. 이렇게 하면 URL과 요청 본문 간의 ID 불일치 문제를 방지할 수 있습니다.또한
InquiryAnswerApi에서 업데이트 엔드포인트가inquiryId를 경로 변수로 받고 있는 것과 일관성이 없어 보입니다.♻️ 권장 수정안
`@Builder` `@Schema`(description = "문의 수정 요청 DTO") public record InquiryUpdateRequest( - `@Schema`(description = "문의 ID", example = "1") - Long inquiryId, - `@Schema`(description = "문의 유형", example = "USAGE") Type type, `@Schema`(description = "문의 내용", example = "루틴을 매 달 반복되도록 설정할 수 있나요?") String content, `@Schema`(description = "변경된 이미지 URL 목록", example = "[\"https://cdn.app/image123.jpg\"]") List<String> inquiryImgs ) { }
16-22: 필수 필드에 대한 유효성 검사가 누락되었습니다.
AdminCreateRequest에서는@NotNull,@Size등의 검증 어노테이션을 사용하고 있으나, 이 DTO에는 유효성 검사 어노테이션이 없습니다.content와 같은 필수 필드에는 최소한@NotBlank또는@NotNull제약 조건을 추가하는 것이 좋습니다.♻️ 유효성 검사 추가 예시
+import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Size; `@Builder` `@Schema`(description = "문의 수정 요청 DTO") public record InquiryUpdateRequest( `@Schema`(description = "문의 유형", example = "USAGE") Type type, + `@NotBlank`(message = "문의 내용은 비어있을 수 없습니다.") + `@Size`(max = 1000, message = "문의 내용은 1000자를 초과할 수 없습니다.") `@Schema`(description = "문의 내용", example = "루틴을 매 달 반복되도록 설정할 수 있나요?") String content, `@Schema`(description = "변경된 이미지 URL 목록", example = "[\"https://cdn.app/image123.jpg\"]") List<String> inquiryImgs ) { }src/test/java/im/toduck/domain/inquiry/presentation/controller/InquiryAnswerControllerTest.java (1)
176-177: 사용되지 않는 변수inquiryAnswer가 있습니다.
inquiryAnswer변수가 선언되었지만 이후에 사용되지 않습니다. 테스트 목적상 답변 생성만 필요하다면 변수 할당을 제거하거나, 해당 변수를 활용한 추가 검증을 고려해 보세요.♻️ 불필요한 변수 할당 제거
- InquiryAnswer inquiryAnswer = inquiryAnswerUseCase.createInquiryAnswer(request, - admin.getUser().getId()); + inquiryAnswerUseCase.createInquiryAnswer(request, admin.getUser().getId());src/main/java/im/toduck/domain/inquiry/domain/service/InquiryService.java (1)
55-58: getInquiryById / findById 중복 메서드 통합 고려
동일 동작 메서드가 공존해 호출 혼란이 생길 수 있습니다. 하나로 정리하면 가독성이 좋아집니다.Also applies to: 78-81
sql/init.sql (1)
422-429: type/status 컬럼 무결성 강화 고려
현재VARCHAR라서 애플리케이션 enum 외 값이 저장될 수 있습니다. 데이터 무결성 목적이라면ENUM또는 참조 테이블로 제한하는 방식을 검토해 주세요.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/test/java/im/toduck/domain/inquiry/presentation/controller/InquiryAnswerControllerTest.java`:
- Line 5: The test imports JUnit 4 assertions via "import static
org.junit.Assert.*;" which conflicts with JUnit 5; replace that import with the
JUnit 5 assertions (e.g. "import static org.junit.jupiter.api.Assertions.*;") in
InquiryAnswerControllerTest and update any assertion usages (such as
assertThrows, assertEquals, etc.) to the JUnit 5 signatures so the test methods
compile and run under JUnit Jupiter.
🧹 Nitpick comments (4)
src/test/java/im/toduck/domain/inquiry/presentation/controller/InquiryAnswerControllerTest.java (4)
54-57: 중복된@Transactional어노테이션을 제거하세요.
InquiryAnswerTest중첩 클래스(Line 54)에 이미@Transactional이 선언되어 있으므로, 개별 테스트 메서드(Lines 94, 156, 188, 233, 281, 321)의@Transactional은 불필요합니다. 클래스 레벨의 어노테이션이 모든 메서드에 상속됩니다.♻️ 수정 제안 예시 (첫 번째 테스트 메서드)
- `@Transactional` `@Test` void 성공적으로_생성_조회한다() {Also applies to: 94-96
37-37: 클래스 이름이 테스트 대상과 일치하지 않습니다.클래스 이름이
InquiryAnswerControllerTest이지만, 실제로는InquiryAnswerUseCase를 직접 테스트하고 있습니다. Controller 엔드포인트에 대한 HTTP 요청/응답 테스트가 아닌 Use Case 레벨의 통합 테스트입니다.명확성을 위해
InquiryAnswerUseCaseTest또는InquiryAnswerIntegrationTest로 이름을 변경하는 것을 고려해 주세요.
174-175: 사용되지 않는 변수가 있습니다.
inquiryAnswer변수가 생성 후 사용되지 않습니다. 반환값이 필요 없다면 변수 할당을 제거하거나, 혹은 추가적인 검증(예: 생성된 답변의 상태 확인)을 고려해 보세요.♻️ 변수 할당 제거 예시
- InquiryAnswer inquiryAnswer = inquiryAnswerUseCase.createInquiryAnswer(request, - admin.getUser().getId()); + inquiryAnswerUseCase.createInquiryAnswer(request, admin.getUser().getId());
101-106: Admin 생성 코드 중복을 줄일 수 있습니다.모든 테스트 메서드에서 동일한 패턴으로 Admin 객체를 생성하고 있습니다.
@BeforeEach의setUp()메서드로 이동하거나 헬퍼 메서드로 추출하면 코드 중복을 줄이고 가독성을 높일 수 있습니다.♻️ setUp() 메서드에 Admin 생성 추가 예시
+ private Admin savedAdmin; + `@BeforeEach` void setUp() { savedAdminUser = userRepository.save( User.builder() .role(UserRole.ADMIN) .nickname("admin") .email("admin@naver.com") .provider(OAuthProvider.APPLE) .build() ); savedGeneralUser = testFixtureBuilder.buildUser(GENERAL_USER()); + + savedAdmin = adminRepository.save( + Admin.builder() + .user(savedAdminUser) + .displayName("토덕 관리자") + .build() + ); }
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/main/java/im/toduck/domain/admin/domain/service/AdminService.java`:
- Around line 41-56: Remove the misleading `@Transactional` annotation from the
private method createDefaultAdmin in AdminService (private methods are not
proxied by Spring AOP so the annotation is ineffective); if transactional
behavior is required ensure the caller method (e.g., getAdminBySameUser) carries
`@Transactional` (or make createDefaultAdmin non-private and document it),
otherwise simply delete the `@Transactional` on createDefaultAdmin to avoid
confusion.
In `@src/main/java/im/toduck/domain/admin/domain/usecase/AdminUseCase.java`:
- Around line 86-98: The null-check on admin.getDeletedAt() in deleteAdmin is
dead code because adminService.getAdmin(userId) already filters for active
admins; remove the if (admin.getDeletedAt() != null) { return; } block from the
deleteAdmin method so the code proceeds directly to user.demoteToUser() and
adminService.deleteAdmin(admin). Keep references to deleteAdmin,
adminService.getAdmin, getDeletedAt, User.demoteToUser, and
adminService.deleteAdmin when locating the lines to change.
In `@src/main/java/im/toduck/domain/inquiry/domain/service/InquiryService.java`:
- Around line 55-58: The class defines duplicate methods getInquiryById and
findById that both call inquiryRepository.findById(inquiryId); remove one of
them (prefer keeping findById) and update all callers (e.g., in InquiryUseCase)
to use the retained method name; ensure any `@Transactional` or visibility
annotations from the removed method are preserved on the kept method and run
tests to confirm no usages remain of the deleted method.
In `@src/main/java/im/toduck/domain/inquiry/domain/usecase/InquiryUseCase.java`:
- Around line 58-59: Both getInquiryById and findById in InquiryService perform
the same repository.findById(inquiryId) logic; unify to a single method name
(pick one, e.g., getInquiryById) in InquiryService and remove the duplicate
method, update all callers (e.g., updateInquiry and deleteInquiry in
InquiryUseCase) to use the chosen method, and ensure the remaining method still
throws CommonException.from(ExceptionCode.NOT_FOUND_INQUIRY) when not found;
also remove the unused duplicate method implementation and any imports it
required.
In
`@src/main/java/im/toduck/domain/inquiry/presentation/dto/request/InquiryUpdateRequest.java`:
- Around line 14-17: Remove the inquiryId field from the InquiryUpdateRequest
record: delete the Long inquiryId declaration and its annotations in the
InquiryUpdateRequest record, leaving only the fields used by
InquiryService.updateInquiry() (type, content, inquiryImgs); ensure
imports/Schema/@NotNull references are cleaned up and that controllers keep
passing the path param `/v1/inquiries/{inquiryId}` into the use case rather than
relying on the request body.
- Around line 23-26: The DTO validation for content in InquiryUpdateRequest (and
InquiryCreateRequest) mismatches the Inquiry entity column length: DTOs use
`@Size`(max = 500) while the Inquiry entity defines `@Column`(length = 1024); decide
on the correct constraint and make them consistent — either increase the DTO
annotations to `@Size`(max = 1024) in InquiryUpdateRequest and
InquiryCreateRequest to match Inquiry.content, or change Inquiry.content
`@Column`(length = ...) to 500 to match the DTOs (also ensure InquiryAnswer
remains unchanged if intended), then run tests to verify validation and
persistence behave as expected.
🧹 Nitpick comments (13)
src/main/java/im/toduck/domain/admin/domain/usecase/AdminUseCase.java (2)
28-40: 읽기 전용 메서드에readOnly = true누락
getAdmin과getAdmins는 데이터를 변경하지 않으므로@Transactional(readOnly = true)를 사용하는 것이 적절합니다. 읽기 전용 트랜잭션은 JPA 더티 체킹을 건너뛰어 성능상 이점이 있고, 의도를 명확히 합니다.♻️ 제안 변경
- `@Transactional` + `@Transactional`(readOnly = true) public AdminResponse getAdmin(final Long userId) { - `@Transactional` + `@Transactional`(readOnly = true) public AdminListResponse getAdmins() {
76-84:updateAdmin에서 불필요한 조회 중복
user(Line 78)와admin(Line 81) 변수를 조회하지만 둘 다 사용되지 않습니다. 실제 업데이트 로직은adminService.updateAdmin(userId, request)내부에서 admin을 다시 조회하므로, 여기서의 조회는 불필요한 DB 호출입니다.존재 검증이 목적이라면, 서비스 내부에서 이미 수행하고 있으므로 UseCase에서는 제거하거나, 반대로 UseCase에서 검증 후 admin 객체를 서비스에 전달하는 방식으로 통일하세요.
♻️ 제안 변경 (불필요한 조회 제거)
`@Transactional` public void updateAdmin(final Long userId, final AdminUpdateRequest request) { - User user = userService.getUserById(userId) - .orElseThrow(() -> CommonException.from(ExceptionCode.NOT_FOUND_USER)); - - Admin admin = adminService.getAdmin(userId); - adminService.updateAdmin(userId, request); }src/main/java/im/toduck/domain/admin/domain/service/AdminService.java (2)
28-32:getAdmin에readOnly = true누락읽기 전용 조회이므로
@Transactional(readOnly = true)가 적절합니다.♻️ 제안 변경
- `@Transactional` + `@Transactional`(readOnly = true) public Admin getAdmin(final Long userId) {
81-88:updateAdmin의 null 체크와@NotBlank유효성 검증 중복
AdminUpdateRequest.displayName에@NotBlank가 설정되어 있으므로, 컨트롤러 진입 시 이미 null/blank가 거부됩니다. Line 83의request.displayName() != null체크는 방어적 코딩이지만, 결과적으로if블록이 항상 실행되어 조건문이 무의미합니다.♻️ 조건문 제거로 단순화
`@Transactional` public void updateAdmin(final Long userId, final AdminUpdateRequest request) { - if (request.displayName() != null) { - Admin admin = adminRepository.findActiveAdminByUserId(userId) - .orElseThrow(() -> CommonException.from(ExceptionCode.NOT_FOUND_ADMIN)); - admin.updateDisplayName(request.displayName()); - } + Admin admin = adminRepository.findActiveAdminByUserId(userId) + .orElseThrow(() -> CommonException.from(ExceptionCode.NOT_FOUND_ADMIN)); + admin.updateDisplayName(request.displayName()); }src/main/java/im/toduck/domain/inquiry/persistence/entity/Inquiry.java (2)
55-56:Inquirysoft-delete 시 자식InquiryImage레코드가 함께 soft-delete 되지 않습니다.
@SQLDelete는 부모 엔티티의 DELETE SQL만 오버라이드하며,CascadeType.ALL은 JPA의EntityManager.remove()호출 시에만 자식에게 전파됩니다. 따라서Inquiry가 soft-delete 되더라도 연관된InquiryImage행들은deleted_at이 NULL인 상태로 남게 됩니다.현재 조회 쿼리(
InquiryRepositoryCustomImpl)가iq.deletedAt.isNull()조건으로 필터링하므로 즉각적인 기능 결함은 아니지만, 고아 이미지 데이터가 DB에 누적됩니다. 서비스 레이어에서Inquiry삭제 시 자식 이미지들도 명시적으로 soft-delete 처리하는 것을 권장합니다.
89-94:removeAnswer()구현이 개선되었으나, soft-delete된 answer의 메모리 참조 해제 시 주의가 필요합니다.현재
this.inquiryAnswer.delete()로 soft-delete 후this.inquiryAnswer = null로 참조를 해제하는 방식은 동작합니다. 다만 같은 트랜잭션 내에서removeAnswer()호출 후 다시getInquiryAnswer()를 조회하면, Hibernate 1차 캐시에 의해 soft-delete된 answer가 여전히 로드될 수 있으니 이 점을 인지하고 사용해 주세요.src/test/java/im/toduck/domain/inquiry/presentation/controller/InquiryAnswerControllerTest.java (2)
37-37: 테스트 클래스 이름이 실제 테스트 범위와 불일치합니다.
InquiryAnswerControllerTest라는 이름이지만ServiceTest를 상속하며, 실제로는 HTTP 요청이 아닌 유스케이스/서비스 레이어를 직접 호출하는 통합 테스트입니다.InquiryAnswerServiceTest또는InquiryAnswerUseCaseTest와 같이 실제 범위에 맞는 이름으로 변경하는 것을 권장합니다.
96-154: Admin 생성 로직이 모든 테스트 메서드에서 중복됩니다.
Admin엔티티 생성 코드(Lines 101-106, 162-167, 193-198, 238-243, 286-291, 326-331)가 6개 테스트에서 동일하게 반복되고 있습니다.@BeforeEach의setUp()메서드로 이동하면 중복을 제거하고 가독성을 높일 수 있습니다.♻️ setUp() 메서드에 Admin 생성 통합 제안
`@BeforeEach` void setUp() { savedAdminUser = userRepository.save( User.builder() .role(UserRole.ADMIN) .nickname("admin") .email("admin@naver.com") .provider(OAuthProvider.APPLE) .build() ); savedGeneralUser = testFixtureBuilder.buildUser(GENERAL_USER()); + + savedAdmin = adminRepository.save( + Admin.builder() + .user(savedAdminUser) + .displayName("토덕 관리자") + .build() + ); }Also applies to: 156-185, 187-230, 232-278, 280-318, 320-362
src/main/java/im/toduck/domain/inquiry/persistence/entity/InquiryAnswer.java (1)
40-43:@Setter를 사용한inquiry필드 노출을 최소화하는 것을 고려해 주세요.
@Setter는inquiry필드에 대한 public setter를 노출하여,Inquiry.addAnswer()이외의 곳에서도 임의로 변경할 수 있게 합니다. 패키지-프라이빗 setter나 별도의 연관관계 설정 메서드로 범위를 제한하면 캡슐화가 개선됩니다.src/main/java/im/toduck/domain/inquiry/persistence/repository/querydsl/InquiryRepositoryCustomImpl.java (1)
42-55:findAllWithImgs()에 페이지네이션이 없어 대량 데이터 시 메모리 문제 발생 가능.관리자용 전체 문의 조회로 보이는데, 데이터가 증가하면 모든 문의를 한 번에 메모리에 로딩합니다. 현재 규모가 작다면 당장 문제는 아니지만, 향후 페이지네이션(cursor 또는 offset 기반) 도입을 권장합니다.
src/main/java/im/toduck/domain/inquiry/domain/usecase/InquiryUseCase.java (1)
29-37: 조회 전용 메서드에서user변수를 사용하지 않습니다.
getInquiries와getAllInquiries모두user를 조회하지만 실제로 사용하지 않고userId만 서비스에 전달합니다. 사용자 존재 검증 목적이라면 의도가 명확하나, 미사용 변수가 남는 것은 코드 가독성을 저해합니다.userService.getUserById(userId).orElseThrow(...)결과를 변수에 할당하지 않거나, 별도 검증 메서드를 사용하는 것이 깔끔합니다.Also applies to: 96-104
src/main/java/im/toduck/domain/inquiry/domain/service/InquiryService.java (2)
55-58: 읽기 전용 메서드에@Transactional(readOnly = true)를 사용해야 합니다.
getInquiryById(또는 통합 후findById)는 조회만 수행하므로readOnly = true가 적절합니다. 현재findById(Line 79)도 동일하게@Transactional만 선언되어 있습니다.♻️ 제안
- `@Transactional` + `@Transactional`(readOnly = true) public Optional<Inquiry> findById(final Long inquiryId) { return inquiryRepository.findById(inquiryId); }
70-76: 이미지 삭제 시 개별delete호출 대신 배치 삭제를 사용하세요.
images.forEach(inquiryImgRepository::delete)는 이미지 수만큼 개별 DELETE 쿼리를 발생시킵니다.deleteAll또는deleteAllInBatch를 사용하면 쿼리 수를 줄일 수 있습니다.♻️ 제안
if (request.inquiryImgs() != null) { List<InquiryImage> images = inquiryImgRepository.findAllByInquiry(inquiry); - images.forEach(inquiryImgRepository::delete); + inquiryImgRepository.deleteAllInBatch(images); if (!request.inquiryImgs().isEmpty()) { addInquiryImages(inquiry, request.inquiryImgs()); } }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/im/toduck/domain/admin/domain/service/AdminService.java`:
- Around line 80-87: The current updateAdmin method skips validating admin
existence when request.displayName() is null; move the admin lookup out of the
displayName null-check so
adminRepository.findActiveAdminByUserId(userId).orElseThrow(...) is always
executed, then conditionally call admin.updateDisplayName(request.displayName())
only if displayName is non-null; updateAdmin should thus always verify the admin
exists (using adminRepository.findActiveAdminByUserId) before applying any
conditional updates from AdminUpdateRequest.
In `@src/main/java/im/toduck/domain/admin/domain/usecase/AdminUseCase.java`:
- Around line 76-84: The updateAdmin method in AdminUseCase currently performs
redundant DB calls: it calls userService.getUserById(...) and
adminService.getAdmin(...) but never uses the returned user or admin before
delegating to adminService.updateAdmin(...). Either remove these unused calls
entirely from AdminUseCase (if existence checks are not required) or move the
necessary existence/validation logic into adminService.updateAdmin so only that
method performs the DB lookup; specifically remove the
userService.getUserById(...) and adminService.getAdmin(...) calls from
AdminUseCase.updateAdmin and rely on adminService.updateAdmin(userId, request)
to validate and fetch the admin (or add a short comment before keeping
userService.getUserById to indicate intent if the user existence check is a
required business rule).
In `@src/main/java/im/toduck/domain/inquiry/domain/usecase/InquiryUseCase.java`:
- Around line 86-93: Remove the manual soft-delete call on inquiry images in
InquiryUseCase: delete the line that invokes
inquiry.getInquiryImages().forEach(InquiryImage::softDelete) and rely on the
existing cascade = CascadeType.ALL / `@SQLDelete` behavior on the Inquiry entity
so that calling inquiryService.deleteInquiry(inquiry) will let Hibernate perform
the soft-delete for related InquiryImage records automatically.
🧹 Nitpick comments (7)
src/main/java/im/toduck/domain/admin/domain/service/AdminService.java (1)
28-32: 읽기 전용 작업에@Transactional(readOnly = true)사용 권장
getAdmin은 조회만 수행하는 메서드인데 read-write 트랜잭션을 엽니다.getAdmins,getExistingAdmin과 동일하게readOnly = true를 적용하면 DB 플러시 모드 최적화 및 일관성 측면에서 이점이 있습니다.단,
getAdminBySameUser에서 내부적으로 write가 발생할 수 있으므로 해당 메서드는 현재대로 유지하면 됩니다.♻️ 제안 변경
- `@Transactional` + `@Transactional`(readOnly = true) public Admin getAdmin(final Long userId) {src/main/java/im/toduck/domain/admin/domain/usecase/AdminUseCase.java (2)
28-40: 읽기 전용 작업에@Transactional(readOnly = true)사용 권장
getAdmin과getAdmins는 조회만 수행합니다.readOnly = true를 적용하면 불필요한 flush를 방지하고, 일부 DB에서는 read replica로 라우팅하는 이점을 얻을 수 있습니다.♻️ 제안 변경
- `@Transactional` + `@Transactional`(readOnly = true) public AdminResponse getAdmin(final Long userId) { Admin admin = adminService.getAdmin(userId); return AdminMapper.toAdminResponse(admin); } - `@Transactional` + `@Transactional`(readOnly = true) public AdminListResponse getAdmins() {
42-74:promoteToAdmin()호출 중복 제거 가능Lines 59-61과 69-71에서 동일한
promoteToAdmin로직이 반복됩니다. 분기 후에 한 번만 호출하도록 리팩터링하면 가독성이 향상됩니다.♻️ 제안 변경
`@Transactional` public Admin createAdmin(final AdminCreateRequest request) { User user = userService.getUserById(request.userId()) .orElseThrow(() -> CommonException.from(ExceptionCode.NOT_FOUND_USER)); Admin existingAdmin = adminService.getExistingAdmin(user.getId()).orElse(null); // 이미 활성화된 관리자인 경우 if (existingAdmin != null && existingAdmin.getDeletedAt() == null) { throw CommonException.from(ExceptionCode.DUPLICATE_ADMIN); } + Admin admin; + // 삭제된 관리자 복구 if (existingAdmin != null) { existingAdmin.revive(); existingAdmin.updateDisplayName(request.displayName()); - - if (user.getRole() == UserRole.USER) { - user.promoteToAdmin(); - } - - return existingAdmin; + admin = existingAdmin; + } else { + // 신규 관리자 + admin = adminService.createAdmin(request, user); } - // 신규 관리자 - Admin admin = adminService.createAdmin(request, user); - if (user.getRole() == UserRole.USER) { user.promoteToAdmin(); } return admin; }src/main/java/im/toduck/domain/inquiry/presentation/dto/request/InquiryUpdateRequest.java (1)
24-25:inquiryImgs리스트에 대한 검증이 없습니다.리스트 크기 제한(
@Size)이나 각 URL 요소에 대한@NotBlank검증이 없어, 빈 문자열이나 과도한 수의 이미지 URL이 전달될 수 있습니다. 의도적으로 허용하는 것인지 확인해 주세요.src/main/java/im/toduck/domain/inquiry/domain/service/InquiryService.java (3)
55-58:getInquiryById에 불필요한 쓰기 트랜잭션이 설정되어 있습니다.읽기 전용 조회임에도
@Transactional이 사용되고 있습니다.@Transactional(readOnly = true)로 변경하세요.♻️ 수정 제안
- `@Transactional` + `@Transactional`(readOnly = true) public Optional<Inquiry> getInquiryById(final Long inquiryId) { return inquiryRepository.findById(inquiryId); }
70-76: 이미지 삭제 시 개별 delete 호출로 N+1 쿼리가 발생합니다.
images.forEach(inquiryImgRepository::delete)는 이미지 수만큼 DELETE 쿼리를 실행합니다. 배치 삭제를 사용하세요.♻️ 수정 제안
if (request.inquiryImgs() != null) { List<InquiryImage> images = inquiryImgRepository.findAllByInquiry(inquiry); - images.forEach(inquiryImgRepository::delete); + inquiryImgRepository.deleteAllInBatch(images); if (!request.inquiryImgs().isEmpty()) { addInquiryImages(inquiry, request.inquiryImgs()); } }
30-36: 서비스 계층에서 응답 DTO를 직접 반환합니다.
getInquiries와getAllInquiries가InquiryResponse(프레젠테이션 DTO)를 반환하고 있어 서비스와 프레젠테이션 계층 간 결합이 발생합니다. 엔티티 또는 도메인 객체를 반환하고 유스케이스/컨트롤러 계층에서 매핑하는 것이 더 깔끔합니다.
|
@wafla p3: git checkout develop
git pull origin develop
git checkout feature/inquiry
git rebase develop
# 충돌 해결 후
git push --force-with-lease origin feature/inquiry |
Updated README.md to include additional authors and API information.
wafla
force-pushed
the
feature/inquiry
branch
from
5f36dff to
fc15b24
Compare
2 participants
✨ 작업 내용
1️⃣ 문의
사용자가 문의를 생성하고 조회할 수 있는 API입니다. 답변이 달리기 전에만 수정, 삭제가 가능합니다.
관리자는 전체 문의를 조회할 수 있습니다.
2️⃣ 문의 답변(관리자 기능)
관리자가 문의에 답변을 할 수 있는 API입니다.
답변을 작성한 관리자 엔티티 정보가 삭제되더라도 기존 정보를 가져올 수 있도록 구현했습니다.
문의 답변 조회 기능은 문의 조회 기능을 사용하면 됩니다.
3️⃣ 관리자(관리자 기능)
문의 답변을 한 관리자 정보를 관리하기 위해 관리자 엔티티를 추가했으며 API를 만들었습니다.
기존 Role이 Admin이지만 관리자 테이블에 정보가 없는 경우, 관리자 테이블을 신경쓰지 않고 답변을 작성해도 기본값(토덕 관리자)으로 관리자 엔티티를 생성하도록 구현했습니다.
엔티티 생성/삭제 시 관리자 권한을 부여/회수하도록 했으며 회원탈퇴 시에도 관리자 권한을 회수하는 코드를 추가했습니다.
✅ 리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Bug Fixes
Tests