Feature/#297 댓글 알림 조회 api, 읽음 상태 변경 api 및 is read 추가

[java-saeng]

#️⃣연관된 이슈

• 댓글 알림 조회 API, 읽음 상태 API 및 isRead 추가 #297

스크린샷 (선택)

예상 소요 시간 및 실제 소요 시간

5시간 / 5시간

[amaran-th]

수고하셨습니다!
변수명/예외 명명 관련해서 코멘트 남겼는데, 다시 생각해보니 이건 관점의 차이인 것 같기도 해서 설득되지 않으시면 그냥 무시하셔도 될 것 같아요!
approve하겠습니다!

[amaran-th]

해당 예외가 발생하는 상황을 고려했을 때
'알림을 조회할 권한이 없습니다'와 같은 식으로 변경하면 예외 상황을 이해하기 좀 더 쉬울 것 같아요!

[hyeonjerry]

저도 아마란스 의견에 동의합니다.

[java-saeng]

Member 관련 Exception에 굳이 알림이라는 단어가 필요할까라는 생각이 듭니다.

그리고 해당 예외는 저번에 얘기했던 header에 있는 토큰의 member ID는 자원을 찾기 위해서가 아니라 Authorization에서만 사용해야하는 것이 올바르게 사용하는 것이라 생각이 들어서 위와 같은 예외를 하나 만들어두었습니다.

혹시 어떻게 생각하시나요? @amaran-th @hyeonjerry

[hyeonjerry]

제가 이해한게 맞다면 댓글과 같은 리소스의 소유자와 그것을 변경하려는 사용자 사이에 불일치가 일어날 경유 위 예외가 발샹하는 것 같습니다.
그렇다면 저는 "사용자가 일치하지 않습니다" 정도로 포괄적으로 처리할 것 같습니다.

[java-saeng]

오 좋네요 수정할게요

[amaran-th]

제가 이해한 바로는 알람 대상자(receiver)의 id를 의미하는 것 같은데, loginMemberId보다는 receiverId는 어떨지 제안드려봅니다!

처음 봤을 때 authMember의 id와 무엇이 다른건지 잘 이해가 안됐어서요...!

[java-saeng]

반영 완료했습니다 !

[hyeonjerry]

수고하셨습니다.
크게 수정할 점은 발견하지 못했습니다.

[hong-sile]

전체적으로 잘 짜주신 것 같아요. EventListner를 별도의 Component로 분리한게 인상적이네요.

남긴 리뷰는 우르가 보시고 적용할만하다 싶은 것들을 적용해주시면 될 것 같아요.
Approve하겠습니다.

[hong-sile]

Service에 있던 Listner가 별도의 Component로 분리가 됬군요.
확실히 분리를 한 게 더 나아보이네요. 👍

[hong-sile]

매번 BeforeEach에서 NotificationListener를 초기화해준다면, @Autowired를 제거할 수 있을 것 같아요.

[hong-sile]

사이즈 말고, 객체 자체를 비교해보는건 어떤가요?

[java-saeng]

사이즈는 좀 부족한 감이 있네요,,

근데 객체 자체를 비교하게 되면 ID가 있어야하는데 기존에 썼던 usingRecursiveComparison으로는 힘들지 않을까요?

@Test
@DisplayName("findAllByReceiverId() : 현재 접속해 있는 사용자가 받은 행사&댓글 알림들을 조회할 수 있다.")
void test_findAllByReceiverId() throws Exception {
  //given
  final long receiverId = 1L;

  updateNotificationRepository.saveAll(
      List.of(
          new UpdateNotification(
              receiverId, 2L,
              UpdateNotificationType.COMMENT, LocalDateTime.now()
          ),
          new UpdateNotification(
              receiverId, 3L,
              UpdateNotificationType.EVENT, LocalDateTime.now()
          ),
          new UpdateNotification(
              2L, 4L,
              UpdateNotificationType.COMMENT, LocalDateTime.now()
          )
      ));

  final List<UpdateNotification> actual = List.of(
      new UpdateNotification(
          receiverId, 2L,
          UpdateNotificationType.COMMENT, LocalDateTime.now()
      ),
      new UpdateNotification(
          receiverId, 3L,
          UpdateNotificationType.EVENT, LocalDateTime.now()
      )
  );

  //when
  final List<UpdateNotification> expected =
      updateNotificationRepository.findAllByReceiverId(receiverId);

  //then
  assertThat(actual)
      .usingRecursiveComparison()
      .ignoringCollectionOrder()
      .ignoringFields("createdAt")
      .ignoringFields("id")
      .isEqualTo(expected);
}

요렇게 될텐데 다른 방법이 있을까요?

[hong-sile]

given이랑 when은 어쩔수 없을 것 같긴해요.
then부분에서

    assertThat(actual)
        .usingRecursiveFieldByFieldElementComparatorIgnoringFields("createdAt", "id")
        .containsExactlyInAnyOrderElementsOf(expected);

을 시도해보셔도 될 것 같고,
ignoringFileds는 무시할 필드를 복수개를 넣을 수 있어서,

  assertThat(actual)
      .usingRecursiveComparison()
      .ignoringCollectionOrder()
      .ignoringFields("createdAt", "id")
      .isEqualTo(expected);

도 될것 같네요

NIT : 지금 테스트 코드 좀 더 디테일하게 보다가 알았는데, given이랑 when에서 expected랑 actual이 바뀌어있네요.
then에서는 제대로 되어있습니다.

[java-saeng]

반영했습니다 감사합니다 !