Skip to content

Feature/board service: Suspended item 조회 #79

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature/board service: Suspended item 조회 #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce520d4
test(board): update imports for board
shin-jingyu Feb 28, 2025
a2eb945
feat(board): set title to null for SUSPENDED status
shin-jingyu Feb 28, 2025
8c20da3
feat(board): throw exception when findById() board is in suspended st…
github-insu Feb 28, 2025
6484464
test(board): throw exception when boardDetail status is in suspended
github-insu Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/main/java/me/nettee/board/application/service/BoardQueryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Service;

import static me.nettee.board.application.exception.BoardQueryErrorCode.BOARD_FORBIDDEN;
import static me.nettee.board.application.exception.BoardQueryErrorCode.BOARD_NOT_FOUND;

@Service
Expand All @@ -22,12 +23,26 @@ public class BoardQueryService implements BoardReadUseCase, BoardReadByStatusesU

@Override
public BoardDetail getBoard(Long id) {
return boardQueryPort.findById(id)
BoardDetail boardDetail = boardQueryPort.findById(id)
.orElseThrow(BOARD_NOT_FOUND::exception);

if (boardDetail.status() == BoardStatus.SUSPENDED) {
throw BOARD_FORBIDDEN.exception();
}

return boardDetail;
}

@Override
public Page<BoardSummary> findByStatuses(Set<BoardStatus> statuses, Pageable pageable) {
return boardQueryPort.findByStatusesList(statuses, pageable);
var boardPage = boardQueryPort.findByStatusesList(statuses, pageable);

var filterBoardPage = boardPage.map(board ->
board.status() == BoardStatus.SUSPENDED
? new BoardSummary(board.id(), null, board.status(), board.createdAt(), board.updatedAt())
: board
);

Comment on lines +39 to +45
Copy link
Member

Choose a reason for hiding this comment

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

👍 훌륭하고 깔끔한 작업입니다.

이 항목들이 특히 보기 좋습니다.
팀 내 작업 방식에 대한 리뷰를 지금까지 꼼꼼하게 확인해서 반영해 주신 것 같습니다.

  • 👍 Page 객체에서 아이템 매핑에 적절한 map 함수 사용
  • 👍 Enum 열거상수 비교에 == 연산자 허용
  • 👍 적절한 삼항연산자 사용


이 부분은 조금 더 생각해 볼 수 있을 것 같습니다.

  • 💬 기술적인 이야기는 아닐 수 있지만, title을 null로 할지 기타 기본값을 내릴지 나중에 정할 수 있어 보입니다. 이번 단계 온보딩 프로젝트에서는 다국어 등을 고려하지 않으므로 나중에 생각해 볼 수 있을 것 같습니다.
  • 💊 record 인스턴스 생성 시, 생성자보다 빌더나 mapstruct 함수, record 내부 메서드 등을 권장하고 싶습니다.
    • 생성자를 사용하는 방식은 데이터 스펙이 바뀔 때마다 많은 곳을 함께 수정하거나, 생성자를 매번 추가적으로 작성해야 하기 때문입니다. 
      // 기존 레코드에 age 필드가 추가되었을 때 예시
public record ExampleRecord(String name, Integer age) {
    // 기존 스펙을 생성자로 작성합니다. (단, 스펙이 바뀔 때마다 이런 생성자가 추가되어야 할 수 있습니다.)
    public ExampleRecord(String name) {
        // 모든 인자를 전체인자생성자로 넘기고, 나머지는 기본값으로 처리할 수 있습니다.
        this(name, null);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merge-simpson 좋은 의견 감사합니다.
온보딩에서는 다중 사용이 필요 없다고 판단해 생성자를 사용했지만,
향후 다양한 활용 가능성을 고려하면 record 내부 메서드를 사용하는 것이 더 적절해 보입니다.

return filterBoardPage;
}
}
43 changes: 39 additions & 4 deletions src/test/kotlin/me/nettee/board/application/service/BoardQueryServiceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ package me.nettee.board.application.service
import io.kotest.assertions.throwables.shouldThrow
import io.kotest.core.spec.style.FreeSpec
import io.kotest.matchers.shouldBe
import io.mockk.clearMocks
import io.mockk.every
import io.mockk.mockk
import io.mockk.verify
import me.nettee.board.application.domain.type.BoardStatus
import me.nettee.board.application.exception.BoardCommandErrorCode.BOARD_NOT_FOUND
import me.nettee.board.application.exception.BoardCommandException
import me.nettee.board.application.exception.BoardQueryErrorCode.BOARD_FORBIDDEN
import me.nettee.board.application.exception.BoardQueryErrorCode.BOARD_NOT_FOUND
import me.nettee.board.application.exception.BoardQueryException
import me.nettee.board.application.model.BoardQueryModels.BoardDetail
import me.nettee.board.application.model.BoardQueryModels.BoardSummary
import me.nettee.board.application.port.BoardQueryPort
Expand All @@ -23,6 +25,10 @@ class BoardQueryServiceTest : FreeSpec({
val boardQueryPort = mockk<BoardQueryPort>() // mocking
val boardQueryService = BoardQueryService(boardQueryPort) // 주입

beforeTest {
clearMocks(boardQueryPort)
}

Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

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

❓ beforeTest에서 clearMocks(...)를 사용한 이유가 무엇인가요?

다른 분들께서도 이 사용 목적과 대안이 있는지 찾아보면 좋을 것 같습니다!
멀티모듈이 시작되면 짬이 안 나겠지만, 적절한 시기를 봐서
따로 테스트 코드와 관련해서 토픽을 정하고 공동 리서치 기간이 필요한가 싶기도 하네요.

리서치와 공유 빈도를 다시 높이면 좋을 것 같다는 생각이 듭니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

@merge-simpson 님 코드 리뷰 감사합니다.

beforeTest에서 clearMocks(...)를 사용한 이유

  • mock 객체(boardQueryPort)를 사용하는 테스트 코드 작성을 하고 있습니다.
  • mock 객체는 호출 기록을 저장합니다.
  • veryfy(exactly = 1)... 코드는 호출 횟수를 검증합니다.
    -> 다른 테스트 코드에서 mock 객체를 이용하여 검증하고자 하는 메서드를 호출한 적이 있다면 예상한 결과와 다를 수 있습니다. 따라서 테스트 케이스마다 호출 기록을 초기화할 필요가 있다고 생각했습니다.

대안

  • 1] 호출 기록을 초기화합니다.
  • 2] 각 테스트를 시작하기 전에, mock 객체 상태를 재설정합니다.

리서치 내용

  • 1] clearMocks(mock): 호출 기록을 초기화합니다.
    • clearMocks(mock, answers = false)와 같은 코드입니다.
    • answers가 true일 때, 호출 기록과 다른 설정들을 초기화합니다.
  • 2] clearInvocations(mock): 호출 기록을 초기화합니다.
  • 3] reset(mock): 호출 기록과 다른 설정들을 초기화합니다.
    -> 호출 기록만을 초기화함으로써 불필요한 추가 작업을 피할 수 있다고 생각합니다.

Copy link
Member

Choose a reason for hiding this comment

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

좋은 리서치 공유 감사합니다.
혹시 각 테스트 종료 후가 아니라 beforeTest에서 작성한 데에도 이유가 있나요?

"BoardQueryService" - {
"getBoard" - {
"board detail 조회 by board id" {
Expand Down Expand Up @@ -52,6 +58,35 @@ class BoardQueryServiceTest : FreeSpec({
verify(exactly = 1) { boardQueryPort.findById(boardId) }
}

"[예외] boardDetail status가 suspended 일 때 Exception 발생" {
// given
val boardId = 1L
val now = Instant.now()

val expectedDetail = BoardDetail(
boardId,
"Test Title",
"test Content",
BoardStatus.SUSPENDED,
now,
now
)

every {
boardQueryPort.findById(boardId)
} returns Optional.of(expectedDetail)

// when
val exception = shouldThrow<BoardQueryException> {
boardQueryService.getBoard(boardId)
}

// then
exception.errorCode shouldBe BOARD_FORBIDDEN

verify(exactly = 1) { boardQueryPort.findById(boardId) }
}

"board id에 해당하는 게시판이 없으면 예외 반환" {
// given
val boardId = 999L
Expand All @@ -60,7 +95,7 @@ class BoardQueryServiceTest : FreeSpec({
} returns Optional.empty()

// when & then
val exception = shouldThrow<BoardCommandException> {
val exception = shouldThrow<BoardQueryException> {
boardQueryService.getBoard(boardId)
}
exception.errorCode shouldBe BOARD_NOT_FOUND
Expand All @@ -87,7 +122,7 @@ class BoardQueryServiceTest : FreeSpec({
),
BoardSummary(
2L,
"Suspended Board",
null,
BoardStatus.SUSPENDED,
now,
now
Expand Down