Skip to content
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.

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

최신 알림 존재 여부 및 읽기 기능 구현 #783

Merged
merged 13 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import java.time.LocalDateTime;
import java.util.Objects;
import lombok.AccessLevel;
import lombok.Builder;
Expand Down Expand Up @@ -74,4 +75,11 @@ public void checkOwner(final Member member) {
}
}

public LocalDateTime getLatestAlarmCreatedAt(final LocalDateTime reportActionAlarmCreatedAt) {
if (this.getCreatedAt().isAfter(reportActionAlarmCreatedAt)) {
return this.getCreatedAt();
}
return reportActionAlarmCreatedAt;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
@Getter
public enum AlarmExceptionType implements ExceptionType {

NOT_FOUND_ACTION(1300, "신고조치알림이 존재하지 않습니다."),
NOT_FOUND_ACTION(1300, "신고 조치 알림이 존재하지 않습니다."),
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

NOT_FOUND(1301, "알림이 존재하지 않습니다."),
NOT_OWNER(1302, "알림을 읽을 대상이 아닙니다."),
NOT_FOUND_ACTION_TYPE(1303, "등록되지 않은 알림 동작입니다."),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.votogether.domain.alarm.entity.Alarm;
import com.votogether.domain.member.entity.Member;
import java.util.List;
import java.util.Optional;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Slice;
import org.springframework.data.jpa.repository.JpaRepository;
Expand All @@ -13,4 +14,6 @@ public interface AlarmRepository extends JpaRepository<Alarm, Long> {

List<Alarm> findAllByMember(final Member member);

Optional<Alarm> findByMemberOrderByCreatedAtDesc(final Member member);
Copy link
Collaborator

Choose a reason for hiding this comment

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

인덱스를 사용하는 ID를 정렬에 사용하는건 어떨까요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋습니다!

변경하려다보니 궁금한 점이 생겼는데, id로 정렬하는 경우 메서드 명이 비즈니스 로직을 잘 드러낸다고 생각하시나요?
현재 메서드 명을 확인하면 최신 날짜를 가져온다는 것을 바로 알 수 있지만, id로 가져오는 경우 지금보다는 덜 직관적이라고 생각해요.
성능을 위해서는 id로 정렬하여 가져오는 것이 훨씬 좋다는 것은 알지만,
이런 경우 개발자로서 항상 고민이 되네요..ㅎㅎ

다즐은 어떻게 생각하시나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 말씀하신 것처럼 생성순 조회를 드러내기 위해서는 createdAtid보다는 더욱 직관적이라 생각해요!

이는 JPA 메서드를 사용하기에 생기는 고민인 것 같아요. JPA 메서드는 필드와 매핑되기 때문에 DB 테이블의 필드를 직접적으로 드러내서 사용하게 돼요. 만약 JdbcTemplate를 사용하는 경우 메서드명에서 필드 매핑을 고려하지 않아도 되기 때문에 findLatestAlarmByMember와 같은 메서드로 캡슐화하여 내부에서 createdAt 순으로 정렬하는지, id 순으로 정렬하는지 사용하는 입장에서 알 수 없고 최신 알림을 조회한다로 메서드를 이해할 것 같아요.

메서드명을 통해 의미를 드러내고 싶다면 JPQL이나 Querydsl을 통해 메서드명을 수정하는 방법도 있을 것 같고, 성능도 챙기면서 의미도 드러내기 위한 방법으로는 변수명을 수정하는 방법도 있을 것 같아요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋은 생각 잘 들었습니다!


}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ public interface ReportActionAlarmRepository extends JpaRepository<ReportActionA

Optional<ReportActionAlarm> findByIdAndMember(final Long Id, final Member member);

Optional<ReportActionAlarm> findByMemberOrderByCreatedAtDesc(final Member member);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 동일합니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

위에 답변 남겼습니다!


}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ public ResponseEntity<Void> updateDetails(
return ResponseEntity.ok().build();
}

@PatchMapping("/me/check-alarm")
public ResponseEntity<Void> checkLatestAlarm(@Auth final Member member) {
memberService.checkLatestAlarm(member);
return ResponseEntity.ok().build();
}

@DeleteMapping("/me/delete")
public ResponseEntity<Void> deleteMember(@Auth final Member member) {
memberService.deleteMember(member);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.votogether.domain.member.dto.response.MemberInfoResponse;
import com.votogether.domain.member.entity.Member;
import com.votogether.global.exception.ExceptionResponse;
import com.votogether.global.jwt.Auth;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
Expand All @@ -17,7 +18,15 @@
public interface MemberControllerDocs {

@Operation(summary = "회원 정보 조회", description = "회원 정보를 조회한다.")
@ApiResponse(responseCode = "200", description = "회원 정보 조회 성공")
@ApiResponses({
@ApiResponse(responseCode = "200", description = "회원 정보 조회 성공"),
@ApiResponse(
responseCode = "400",
description = "회원에 해당하는 통계 정보가 없는 경우",
content = @Content(schema = @Schema(implementation = ExceptionResponse.class)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

개행이 살짝 어색해요 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

엇 그러네요..!
수정하겠습니다

)
})
ResponseEntity<MemberInfoResponse> findMemberInfo(final Member member);

@Operation(summary = "회원 닉네임 변경", description = "회원 닉네임을 변경한다.")
Expand Down Expand Up @@ -48,6 +57,10 @@ ResponseEntity<Void> updateDetails(
final Member member
);

@Operation(summary = "회원 최신 알림 확인", description = "회원의 최신 알림 읽은 시간을 수정한다.")
@ApiResponse(responseCode = "200", description = "최신 알림 읽기 성공")
ResponseEntity<Void> checkLatestAlarm(@Auth final Member member);

@Operation(summary = "회원 탈퇴", description = "회원 탈퇴한다.")
@ApiResponse(responseCode = "200", description = "회원 탈퇴 성공")
ResponseEntity<Void> deleteMember(final Member member);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@ public record MemberInfoResponse(
@Schema(description = "출생년도", example = "2002")
Integer birthYear,

@Schema(description = "권한", example = "MEMBER")
Roles roles,

@Schema(description = "작성한 게시글 수", example = "5")
long postCount,

@Schema(description = "투표한 수", example = "10")
long voteCount
long voteCount,

@Schema(description = "권한", example = "MEMBER")
Roles roles,
Comment on lines +24 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

꼼꼼히 챙겨주셨네요 👍🏻


@Schema(description = "최신 알림 존재 여부", example = "false")
boolean hasLatestAlarm
) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,26 @@ public class Member extends BaseEntity {
@Column(nullable = false, length = 20)
private Roles roles;

@Column(columnDefinition = "datetime(6)", nullable = false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 컬럼과 일관성있게 nullable이 앞으로 오면 좋을 것 같아요 😺

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저희 컨벤션을 정하기로는 nullable = false가 가장 마지막으로 오는 것으로 정했는데,
Roles가 추가되면서 혼동이 있었던 것 같아요.
Rolesnullable = false를 뒤로 이동시킬게요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

헉 그런가요 감사합니다 ㅎㅎ

private LocalDateTime alarmCheckedAt;

@Builder
private Member(
final String nickname,
final Gender gender,
final Integer birthYear,
final SocialType socialType,
final String socialId,
final Roles roles
final Roles roles,
final LocalDateTime alarmCheckedAt
) {
this.nickname = new Nickname(nickname);
this.gender = gender;
this.birthYear = birthYear;
this.socialType = socialType;
this.socialId = socialId;
this.roles = roles;
this.alarmCheckedAt = alarmCheckedAt;
}

public static Member from(final KakaoMemberResponse response) {
Expand All @@ -82,6 +87,7 @@ public static Member from(final KakaoMemberResponse response) {
.socialType(SocialType.KAKAO)
.socialId(String.valueOf(response.id()))
.roles(Roles.MEMBER)
.alarmCheckedAt(LocalDateTime.now())
.build();
}

Expand Down Expand Up @@ -117,6 +123,14 @@ public boolean hasEssentialInfo() {
return (this.gender != null && this.birthYear != null);
}

public boolean hasLatestAlarmCompareTo(final LocalDateTime latestAlarmCreatedAt) {
return alarmCheckedAt.isBefore(latestAlarmCreatedAt);
}

public void checkAlarm() {
alarmCheckedAt = LocalDateTime.now();
}
Comment on lines +130 to +132
Copy link
Collaborator

Choose a reason for hiding this comment

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

간결하고 깔끔한 구현 👍🏻


public String getNickname() {
return this.nickname.getValue();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.votogether.domain.member.service;

import com.votogether.domain.alarm.entity.Alarm;
import com.votogether.domain.alarm.entity.ReportActionAlarm;
import com.votogether.domain.alarm.repository.AlarmRepository;
import com.votogether.domain.alarm.repository.ReportActionAlarmRepository;
import com.votogether.domain.member.dto.request.MemberDetailRequest;
import com.votogether.domain.member.dto.response.MemberInfoResponse;
import com.votogether.domain.member.entity.Member;
Expand All @@ -23,6 +25,7 @@
import com.votogether.domain.vote.repository.VoteRepository;
import com.votogether.global.exception.BadRequestException;
import com.votogether.global.exception.NotFoundException;
import java.time.LocalDateTime;
import java.util.List;
import java.util.Optional;
import lombok.RequiredArgsConstructor;
Expand All @@ -43,6 +46,7 @@ public class MemberService {
private final ReportRepository reportRepository;
private final CommentRepository commentRepository;
private final AlarmRepository alarmRepository;
private final ReportActionAlarmRepository reportActionAlarmRepository;

@Transactional
public Member register(final Member member) {
Expand Down Expand Up @@ -73,17 +77,46 @@ public Member findById(final Long memberId) {
public MemberInfoResponse findMemberInfo(final Member member) {
final MemberMetric memberMetric = memberMetricRepository.findByMember(member)
.orElseThrow(() -> new NotFoundException(MemberExceptionType.NOT_FOUND_METRIC));
final boolean hasLatestAlarm = hasLatestAlarm(member);

return new MemberInfoResponse(
member.getNickname(),
member.getGender(),
member.getBirthYear(),
member.getRoles(),
memberMetric.getPostCount(),
memberMetric.getVoteCount()
memberMetric.getVoteCount(),
member.getRoles(),
hasLatestAlarm
);
}

private boolean hasLatestAlarm(final Member member) {
final Optional<Alarm> maybeAlarm = alarmRepository.findByMemberOrderByCreatedAtDesc(member);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FindFirst 요런거 안해도 1개만 나오나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optinal로 반환하면 항상 최신의 데이터 하나만 가져오거나
값이 없는 경우 null을 감싸서 반환되는 것으로 알고 있습니다!

final Optional<ReportActionAlarm> maybeReportActionAlarm =
reportActionAlarmRepository.findByMemberOrderByCreatedAtDesc(member);

if (maybeAlarm.isEmpty() && maybeReportActionAlarm.isEmpty()) {
return false;
}
final LocalDateTime latestAlarmCreatedAt = getLatestAlarmCreatedAt(maybeAlarm, maybeReportActionAlarm);
return member.hasLatestAlarmCompareTo(latestAlarmCreatedAt);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2
DB에서 알림 객체를 가져와서 시간을 비교하는방법 말고도
DB에서 alarmCheckedAt을 파라미터로 넘겨주고 최신 알림이 있는지 확인하는 쿼리를 날려 boolean값을 가져와서 확인하는 방법도 고려해보면 좋을거같은데요??

Copy link
Collaborator Author

@jeomxon jeomxon Oct 19, 2023

Choose a reason for hiding this comment

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

루쿠와 쿼리문에 대해서 이야기 나눠볼 수 있는 시간이 있어서 좋았습니다 ㅎㅎ
쿼리문을 하나로 만들 수 있다는 장점이 있을 것 같아요.

하지만 불가피하게 case when을 사용해야하는데 이렇게 되면 비즈니스 로직을 코드 상으로 담을 수가 없어서
좋은 구조에서 벗어나게 된다고 합니다!
따라서 현재 방식을 유지하는 것을 선택하겠습니다 :)


private LocalDateTime getLatestAlarmCreatedAt(
final Optional<Alarm> maybeAlarm,
final Optional<ReportActionAlarm> maybeReportActionAlarm
) {
if (maybeAlarm.isPresent()) {
return maybeAlarm.get().getCreatedAt();
}
if (maybeReportActionAlarm.isPresent()) {
return maybeReportActionAlarm.get().getCreatedAt();
}
final Alarm alarm = maybeAlarm.get();
final ReportActionAlarm reportActionAlarm = maybeReportActionAlarm.get();
return alarm.getLatestAlarmCreatedAt(reportActionAlarm.getCreatedAt());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1
알림이 둘다 존재하면 무조건 알림의 시간비교가 없이 alram의 시간이 반환되지않나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optional.get()에 밑줄 뜨는게 싫어서 바꿨는데 로직이 완전 엉망이 되었군요..ㅠㅠ
늦은 시간에 꼼꼼한 리뷰 쉽지 않으셨을텐데 감사합니다!
바로 수정하겠습니다


@Transactional
public void changeNickname(final Member member, final String nickname) {
validateExistentNickname(nickname);
Expand Down Expand Up @@ -112,6 +145,11 @@ private void validateExistentDetails(final Member member) {
}
}

@Transactional
public void checkLatestAlarm(final Member member) {
member.checkAlarm();
}

@Transactional
public void deleteMember(final Member member) {
final List<Post> posts = deletePosts(member);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.votogether.domain.member.entity.Member;
import com.votogether.global.exception.BadRequestException;
import com.votogether.test.fixtures.MemberFixtures;
import java.time.LocalDateTime;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.test.util.ReflectionTestUtils;
Expand Down Expand Up @@ -54,4 +55,26 @@ void checkOwner() {
.hasMessage("알림을 읽을 대상이 아닙니다.");
}

@Test
@DisplayName("알림이 생성된 시각과 인자로 받은 시각을 비교하여 최신 시각을 반환한다.")
void getLatestAlarmCreatedAt() {
// given
Member member = MemberFixtures.MALE_30.get();
Alarm alarm = Alarm.builder()
.member(member)
.alarmType(AlarmType.COMMENT)
.targetId(1L)
.detail("detail")
.isChecked(false)
.build();
ReflectionTestUtils.setField(alarm, "createdAt", LocalDateTime.of(2023, 10, 18, 12, 0));
LocalDateTime now = LocalDateTime.now();
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3
시간을 좀 더더더 과거로 하면 바로 과거 시간인지 알 수 있을것같아여

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

년도를 과거로 변경해보았어요!


// when
LocalDateTime latestAlarmCreatedAt = alarm.getLatestAlarmCreatedAt(now);

// then
assertThat(latestAlarmCreatedAt).isEqualTo(now);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@ void findMemberInfo() throws Exception {
"저문",
Gender.MALE,
1988,
Roles.MEMBER,
0,
0
0,
Roles.MEMBER,
false
);

given(tokenProcessor.resolveToken(anyString())).willReturn("token");
Expand Down Expand Up @@ -219,6 +220,26 @@ void invalidNullOfBirthYear(Integer birthYear) throws Exception {

}

@Test
@DisplayName("최신 알림 읽기에 성공하면 200을 반환한다.")
void checkLatestAlarm() throws Exception {
// given
TokenPayload tokenPayload = new TokenPayload(1L, 1L, 1L);
given(tokenProcessor.resolveToken(anyString())).willReturn("token");
given(tokenProcessor.parseToken(anyString())).willReturn(tokenPayload);
given(memberService.findById(anyLong())).willReturn(MemberFixtures.FEMALE_20.get());
Comment on lines +227 to +230
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TokenPayload tokenPayload = new TokenPayload(1L, 1L, 1L);
given(tokenProcessor.resolveToken(anyString())).willReturn("token");
given(tokenProcessor.parseToken(anyString())).willReturn(tokenPayload);
given(memberService.findById(anyLong())).willReturn(MemberFixtures.FEMALE_20.get());
mockingAuthArgumentResolver();

이렇게 바꾸면 어떻게 되나요??

지금 만드는 기능과 관련은 없지만 혹시나 하고 MemberControllerTest에서 전부 mockingAuthArgumentResolver(); 로 바꾸고 테스트를 실행해봤는데,

    @Test
    @DisplayName("회원 정보를 조회한다.")
    void findMemberInfo() throws Exception

이 테스트 제외하고는 전부 성공하더라구요. 그래서 MemberControllerTest 안에서 위 메서드 하나만 제외하고 mockingAuthArgumentResolver(); 로 다 바꿔주시는 것은 어떨까요

Copy link
Collaborator Author

@jeomxon jeomxon Oct 19, 2023

Choose a reason for hiding this comment

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

추후에 member 도메인을 리팩터링 하면서 통일성있게 변경해보는 것은 어떨까요?


willDoNothing().given(memberService).checkLatestAlarm(any(Member.class));

// when, then
RestAssuredMockMvc
.given().log().all()
.headers(HttpHeaders.AUTHORIZATION, "Bearer token")
.when().patch("/members/me/check-alarm")
.then().log().all()
.statusCode(HttpStatus.OK.value());
}

@Test
@DisplayName("회원 탈퇴에 성공하면 204를 반환한다.")
void deleteMember() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.votogether.domain.member.entity.vo.Gender;
import com.votogether.domain.member.entity.vo.SocialType;
import com.votogether.global.exception.BadRequestException;
import com.votogether.test.fixtures.MemberFixtures;
import java.time.LocalDateTime;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
Expand Down Expand Up @@ -112,4 +113,52 @@ void notAllowedChangeToInitialNicknamePrefix() {

}

@Test
@DisplayName("회원은 알림을 확인하면 이전에 저장되어있던 시간과는 다르다.")
void checkAlarm() {
// given
Member member = MemberFixtures.MALE_20.get();
LocalDateTime beforeAlarmCheckedAt = member.getAlarmCheckedAt();

// when
member.checkAlarm();
Copy link
Collaborator

Choose a reason for hiding this comment

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

알림 읽기 전 시간을 정해두고 알림을 읽으면 해당 시간 이후임을 검증해보는건 어떨까요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

변경해보았습니다!


// then
LocalDateTime afterAlarmCheckedAt = member.getAlarmCheckedAt();
assertThat(beforeAlarmCheckedAt).isNotEqualTo(afterAlarmCheckedAt);
}

@Nested
@DisplayName("회원의 최신 알림 읽은 시각이 인자로 받은 시각보다")
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3
마지막에 띄어쓰기가 빠져있어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이게 결국 작성해보니깐 띄워쓰기가 있고 없고의 큰 차이가 없는 것 같더라구요...
앞으로 하나의 규칙만 정해서 밀고 가겠습니다..!

class HasLatestAlarmCompareTo {

@Test
@DisplayName("이전이면 true를 반환한다.")
void returnsTrue() {
// given
Member member = MemberFixtures.MALE_20.get();

// when
boolean hasLatestAlarm = member.hasLatestAlarmCompareTo(LocalDateTime.now());

// then
assertThat(hasLatestAlarm).isTrue();
}

@Test
@DisplayName("이후이면 false를 반환한다.")
void returnsFalse() {
// given
Member member = MemberFixtures.MALE_20.get();
LocalDateTime beforeTime = LocalDateTime.of(2023, 10, 18, 12, 0);

// when
boolean hasLatestAlarm = member.hasLatestAlarmCompareTo(beforeTime);

// then
assertThat(hasLatestAlarm).isFalse();
}

}

}
Loading
Loading