알림 읽기 기능 구현 #775

woo-chang · 2023-10-18T05:21:07Z

🔥 연관 이슈

close: 알림 읽기 기능 구현 #766

📝 작업 요약

알림 읽기 기능을 구현하였습니다.

⏰ 소요 시간

1시간

🔎 작업 상세 설명

알림을 읽을 수 있는 대상인 경우 알림을 읽은 상태로 변경하는 기능입니다.

🌟 논의 사항

리뷰 감사합니다 :)

github-actions · 2023-10-18T05:24:08Z

Test Results

467 tests 466 ✔️ 33s ⏱️
150 suites     1 💤
150 files     0 ❌

Results for commit 4355004.

♻️ This comment has been updated with latest results.

jeomxon

빠르게 구현해주셨네요!
궁금한 점이 남아있어 RC드립니다!

jeomxon · 2023-10-18T05:25:26Z

backend/src/main/java/com/votogether/domain/alarm/service/AlarmCommandService.java

+ final Alarm alarm = alarmRepository.findById(alarmId)
+ .orElseThrow(() -> new NotFoundException(AlarmExceptionType.NOT_FOUND));
+
+ alarm.checkOwner(loginMember);


Q
조회하는 시점부터 회원 + 알림ID로 하는 방법과의 차이는 무엇인가요?

멤버와도 같이 조회해버리면 ID가 없어서 발생하는 예외인지, 멤버가 일치하지 않아서 발생하는 예외인지 확인할 수가 없더라구요 🥲

또한 알림의 대상이 누구인지 검증하는 책임은 알림 도메인에 있다고 생각하여 도메인 로직을 통해 검증할 수 있도록 하였습니다 !

jeomxon · 2023-10-18T05:28:41Z

backend/src/test/java/com/votogether/domain/alarm/entity/AlarmTest.java

+ @Test
+ @DisplayName("알림을 읽으면 읽음 여부가 TRUE로 수정된다.")
+ void readAlarm() {
+ // given
+ Member member = MemberFixtures.MALE_30.get();
+ Alarm alarm = Alarm.builder()
+ .member(member)
+ .alarmType(AlarmType.COMMENT)
+ .targetId(1L)
+ .detail("detail")
+ .isChecked(false)
+ .build();
+
+ // when
+ alarm.read();
+
+ // then
+ assertThat(alarm.isChecked()).isTrue();
+ }


P2
setter에 대한 테스트가 필요한가요?

전 개인적으로 파라미터로 값을 전달해주는 완전한 setter가 아니면 추후에 해당 메서드의 내부 로직이 어떻게 변할 지 모르고, 이 또한 해당 메서드의 명세가 될 수 있기 때문에 테스트가 필요하다고 생각하는데,

다즐의 의견도 궁금하네요.

저도 아벨의 의견에 동의해요 :)

캡슐화 되어있기 떄문에 내부적으로 세터인지 외부에서는 알 수 없습니다. 추후에 로직이 변경되었을 때 로직을 확인할 수 있는 하나의 문서로 테스트가 존재할 수 있기에 테스트가 있으면 좋을 것 같아요 !

오 그렇네요 제가 빠르게 보다보니 일반적인 setter와 같다고 생각했던 것 같아요
테스트 있으면 좋을 것 같습니다!

jeomxon · 2023-10-18T05:31:31Z

backend/src/test/java/com/votogether/domain/alarm/controller/AlarmCommandControllerTest.java

+ .when().patch("/alarms/{id}", alarmId)
+ .then().log().all()
+ .status(HttpStatus.BAD_REQUEST)
+ .body("code", equalTo(201))


Q
이 코드 201은 어디서 나온 코드인가요?

ui 계층에서 발생하는 예외 코드는 GlobalExceptionHandler에서 정의하고 있습니다 😀

tjdtls690

수고하셨습니다 다즐!
몇 가지 리뷰 남겼어요 :)

tjdtls690 · 2023-10-18T06:16:41Z

backend/src/test/java/com/votogether/domain/alarm/entity/AlarmTest.java

+ @Test
+ @DisplayName("알림을 읽으면 읽음 여부가 TRUE로 수정된다.")
+ void readAlarm() {
+ // given
+ Member member = MemberFixtures.MALE_30.get();
+ Alarm alarm = Alarm.builder()
+ .member(member)
+ .alarmType(AlarmType.COMMENT)
+ .targetId(1L)
+ .detail("detail")
+ .isChecked(false)
+ .build();
+
+ // when
+ alarm.read();
+
+ // then
+ assertThat(alarm.isChecked()).isTrue();
+ }


전 개인적으로 파라미터로 값을 전달해주는 완전한 setter가 아니면 추후에 해당 메서드의 내부 로직이 어떻게 변할 지 모르고, 이 또한 해당 메서드의 명세가 될 수 있기 때문에 테스트가 필요하다고 생각하는데,

다즐의 의견도 궁금하네요.

tjdtls690 · 2023-10-18T06:20:58Z

backend/src/main/java/com/votogether/domain/alarm/controller/AlarmQueryControllerDocs.java

+ @ApiResponse(
+ responseCode = "201",
+ description = "조회 성공"
+ )


페이지가 0미만이면 400 반환한다는 내용도 포함되면 좋을 것 같아요 :)

tjdtls690 · 2023-10-18T06:22:57Z

backend/src/main/java/com/votogether/domain/alarm/service/AlarmQueryService.java

+ final PageRequest pageRequest = PageRequest.of(page, BASIC_PAGE_SIZE,
+ Sort.by(Sort.Direction.DESC, "createdAt"));


Suggested change

final PageRequest pageRequest = PageRequest.of(page, BASIC_PAGE_SIZE,

Sort.by(Sort.Direction.DESC, "createdAt"));

final PageRequest pageRequest = PageRequest.of(

page,

BASIC_PAGE_SIZE,

Sort.by(Sort.Direction.DESC, "createdAt")

);

이렇게 바꿔보는 것은 어떨까요

tjdtls690 · 2023-10-18T06:28:03Z

backend/src/main/java/com/votogether/domain/alarm/entity/Alarm.java

+ public void checkOwner(final Member member) {
+ if (!Objects.equals(this.member.getId(), member.getId())) {
+ throw new BadRequestException(AlarmExceptionType.NOT_OWNER);
+ }
+ }


Q
자신 것이 아닌 알람을 읽을 수 있는 경우는 어떤 상황에 있을 수 있는지 궁금합니다

알림 ID가 노출되어 다른 회원이 해당 ID로 API를 요청하는 것을 방지하기 위한 방어 코드입니다 :)

tjdtls690 · 2023-10-18T06:32:00Z

backend/src/main/java/com/votogether/domain/alarm/controller/AlarmCommandControllerDocs.java

+ @ApiResponse(
+ responseCode = "400",
+ description = "알림을 읽을 수 있는 대상이 아닌 경우"
+ ),


alarmId가 1미만인 경우도 적어주면 좋을 것 같아요.

tjdtls690

고생하셨습니다 ㅡ 즐 ㅡ

aiaiaiai1

빠르게 수정해서 구현해주셨네요!
딱히 피드백이 보이지 않아서 바로 approve 하겠습니다
고생하셨습니다!

aiaiaiai1 · 2023-10-18T08:53:57Z

backend/src/main/java/com/votogether/domain/alarm/service/AlarmCommandService.java

+
+ private final AlarmReadStrategyProvider alarmReadStrategyProvider;
+
+ public void readAlarm(final Long alarmId, final String type, final Member loginMember) {


개행이 빠져있어요~

# Conflicts: # backend/src/main/java/com/votogether/domain/alarm/controller/AlarmCommandControllerDocs.java # backend/src/main/java/com/votogether/domain/alarm/controller/AlarmQueryController.java # backend/src/main/java/com/votogether/domain/alarm/service/AlarmQueryService.java # backend/src/test/java/com/votogether/domain/alarm/controller/AlarmCommandControllerTest.java # backend/src/test/java/com/votogether/domain/alarm/service/AlarmQueryServiceTest.java

jeomxon

👍🏻

woo-chang added 6 commits October 18, 2023 13:25

chore: (#766) 코드 컨벤션 수정

5096f62

feat: (#766) 알림 읽음 기능 구현

d2f942c

refactor: (#766) 알림 리팩터링

3c68733

feat: (#766) 알림 대상 검증 기능 구현

4966dd2

feat: (#766) 알림 읽는 기능 구현

c8244aa

feat: (#766) 알림 읽기 API 구현

04d5212

woo-chang added BE 🐾 개발 🚨 우선 순위 높음 labels Oct 18, 2023

woo-chang requested review from jeomxon, tjdtls690 and aiaiaiai1 October 18, 2023 05:21

woo-chang self-assigned this Oct 18, 2023

jeomxon requested changes Oct 18, 2023

View reviewed changes

woo-chang added 3 commits October 18, 2023 14:33

fix: (#766) 테이블 추가

85fa66d

fix: (#766) 비동기 테스트를 위한 sleep 추가

b3a4d72

feat: (#766) 알림 타입 추가

289b63c

tjdtls690 requested changes Oct 18, 2023

View reviewed changes

woo-chang added 7 commits October 18, 2023 15:45

test: (#766) 알림 타입이 없는 경우 테스트

9e6273a

feat: (#766) 신고 알림 읽는 기능 구현

4abffa8

feat: (#766) 알림 액션 타입 생성 기능 구현

40618e1

feat: (#766) 타입에 따른 알림 읽기 기능 구현

3998d1d

docs: (#766) 예외 상황 문서화

0e10b78

chore: (#766) 코드 컨벤션 수정

0242e3a

docs: (#766) 예외 발생 문서화

b3e0ba1

tjdtls690 approved these changes Oct 18, 2023

View reviewed changes

aiaiaiai1 approved these changes Oct 18, 2023

View reviewed changes

woo-chang added 2 commits October 18, 2023 18:10

Merge branch 'dev' into feat/#766

b5b54c9

woo-chang added 4 commits October 18, 2023 18:16

chore: (#766) 코드 컨벤션 수정

a54c9b2

chore: (#766) 개행 컨벤션 수정

6f432d7

Merge branch 'dev' into feat/#766

d6f81ec

fix: (#766) DB 예약어 수정

68e6bec

jeomxon approved these changes Oct 18, 2023

View reviewed changes

fix: (#766) 이벤트 테스트 임시 수정

4355004

woo-chang merged commit df585f2 into dev Oct 18, 2023
3 checks passed

woo-chang deleted the feat/#766 branch October 18, 2023 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

알림 읽기 기능 구현 #775

알림 읽기 기능 구현 #775

woo-chang commented Oct 18, 2023

github-actions bot commented Oct 18, 2023 •

edited

Loading

jeomxon left a comment

jeomxon Oct 18, 2023

woo-chang Oct 18, 2023

jeomxon Oct 18, 2023

tjdtls690 Oct 18, 2023

woo-chang Oct 18, 2023

jeomxon Oct 18, 2023

jeomxon Oct 18, 2023

woo-chang Oct 18, 2023

tjdtls690 left a comment

tjdtls690 Oct 18, 2023

tjdtls690 Oct 18, 2023

tjdtls690 Oct 18, 2023

tjdtls690 Oct 18, 2023

woo-chang Oct 18, 2023

tjdtls690 Oct 18, 2023

tjdtls690 left a comment

aiaiaiai1 left a comment

aiaiaiai1 Oct 18, 2023

jeomxon left a comment

		final PageRequest pageRequest = PageRequest.of(page, BASIC_PAGE_SIZE,
		Sort.by(Sort.Direction.DESC, "createdAt"));


		private final AlarmReadStrategyProvider alarmReadStrategyProvider;

		public void readAlarm(final Long alarmId, final String type, final Member loginMember) {

알림 읽기 기능 구현 #775

알림 읽기 기능 구현 #775

Conversation

woo-chang commented Oct 18, 2023

🔥 연관 이슈

📝 작업 요약

⏰ 소요 시간

🔎 작업 상세 설명

🌟 논의 사항

github-actions bot commented Oct 18, 2023 • edited Loading

Test Results

jeomxon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdtls690 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdtls690 left a comment

Choose a reason for hiding this comment

aiaiaiai1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeomxon left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2023 •

edited

Loading