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

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

merged 13 commits into from
Oct 19, 2023

Conversation

jeomxon
Copy link
Collaborator

@jeomxon jeomxon commented Oct 18, 2023

🔥 연관 이슈

close: #774

📝 작업 요약

최신 알림 읽기 기능 및 최신 알림인지 여부를 보여주는 로직 추가했습니다.

⏰ 소요 시간

3~4시간? 컬럼 추가되고 로직에 대해 의견을 나누는 시간이 있어 예상보다 조금 더 소요된 것 같아요.

  • 개발 중 로직을 한번 갈아 엎어서 시간이 더 오래 걸린 것 같네요 ㅠㅠ

🔎 작업 상세 설명

  • 내 정보 조회 시 최신 알림 시각과 내 최신 알림 읽은 시각을 비교하여 최신인지 여부 반환
  • 최신 알림 읽기 기능 추가

🌟 논의 사항

없음.

@github-actions
Copy link

github-actions bot commented Oct 18, 2023

Test Results

490 tests   489 ✔️  30s ⏱️
156 suites      1 💤
156 files        0

Results for commit be74d45.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

바쁜와중에도 꼼꼼하게 잘 구현해주셨네요!!
몇가지 커멘트 남겨드렸는데 확인부탁드려용
컨디션도 안좋고 바쁘신데 고생하셨습니다~! 👍

@@ -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.

👍

Comment on lines 105 to 118
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()에 밑줄 뜨는게 싫어서 바꿨는데 로직이 완전 엉망이 되었군요..ㅠㅠ
늦은 시간에 꼼꼼한 리뷰 쉽지 않으셨을텐데 감사합니다!
바로 수정하겠습니다

Comment on lines 93 to 103
private boolean hasLatestAlarm(final Member member) {
final Optional<Alarm> maybeAlarm = alarmRepository.findByMemberOrderByCreatedAtDesc(member);
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을 사용해야하는데 이렇게 되면 비즈니스 로직을 코드 상으로 담을 수가 없어서
좋은 구조에서 벗어나게 된다고 합니다!
따라서 현재 방식을 유지하는 것을 선택하겠습니다 :)

Comment on lines 70 to 71
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.

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

}

@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.

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

}

@Test
@DisplayName("신고 조치 내역 알림과 게시글 내역 알림이 모두 존재한다면 회원의 최신 알림 확인 시각과 비교하여 신고 조치 알림이 더 최신인 경우 true를 반환한다.")
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
@DisplayName("신고 조치 내역 알림과 게시글 내역 알림이 모두 존재한다면 회원의 최신 알림 확인 시각과 비교하여 신고 조치 알림이 더 최신인 경우 true를 반환한다.")
@DisplayName("신고 조치 내역 알림과 게시글 내역 알림이 모두 존재한다면 회원의 최신 알림 확인 시각과 비교하여 알림이 더 최신인 경우 true를 반환한다.")

++ 신고조치알림 이나 게시글 내역 알림 둘중 하나만 알림 확인 시각 보다 최신일때도 테스트가 추가되면 좋을거같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

추가하겠습니다!

Comment on lines +11 to +20
public class ReportActionAlarmTestPersister {

private final ReportActionAlarmRepository reportActionAlarmRepository;
private final MemberTestPersister memberTestPersister;

public ReportActionAlarmBuilder builder() {
return new ReportActionAlarmBuilder();
}

public final class ReportActionAlarmBuilder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 🙇 👍 🙇 👍 🙇 👍 🙇 👍 🙇 👍 🙇

Comment on lines 309 to 322
@Test
@DisplayName("신고 조치 내역 알림이 존재한다면 회원의 최신 알림 확인 시각과 비교하여 게시글 알림이 더 최신인 경우 true를 반환한다.")
void returnsTrueWhenReportActionAlarmIsLatest() {
// given
Member member = memberTestPersister.builder().save();
memberMetricTestPersister.builder().member(member).save();
reportActionAlarmTestPersister.builder().member(member).save();

// when
MemberInfoResponse memberInfoResponse = memberService.findMemberInfo(member);

// then
assertThat(memberInfoResponse.hasLatestAlarm()).isTrue();
}
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
@Test
@DisplayName("신고 조치 내역 알림이 존재한다면 회원의 최신 알림 확인 시각과 비교하여 게시글 알림이 더 최신인 경우 true를 반환한다.")
void returnsTrueWhenReportActionAlarmIsLatest() {
// given
Member member = memberTestPersister.builder().save();
memberMetricTestPersister.builder().member(member).save();
reportActionAlarmTestPersister.builder().member(member).save();
// when
MemberInfoResponse memberInfoResponse = memberService.findMemberInfo(member);
// then
assertThat(memberInfoResponse.hasLatestAlarm()).isTrue();
}
@Test
@DisplayName("신고 조치 내역 알림이 존재한다면 회원의 최신 알림 확인 시각과 비교하여 신고조치 알림이 더 최신인 경우 true를 반환한다.")
void returnsTrueWhenReportActionAlarmIsLatest() {
// given
Member member = memberTestPersister.builder().save();
memberMetricTestPersister.builder().member(member).save();
reportActionAlarmTestPersister.builder().member(member).save();
// when
MemberInfoResponse memberInfoResponse = memberService.findMemberInfo(member);
// then
assertThat(memberInfoResponse.hasLatestAlarm()).isTrue();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

꼼꼼한 확인 👍🏻

Copy link
Collaborator

@tjdtls690 tjdtls690 left a comment

Choose a reason for hiding this comment

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

깔끔하게 잘 구현해주었네요 :)

사소한 리뷰 몇 가지 남겼습니다.

Comment on lines +227 to +230
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());
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 도메인을 리팩터링 하면서 통일성있게 변경해보는 것은 어떨까요?

Comment on lines 109 to 114
if (maybeAlarm.isEmpty()) {
return maybeReportActionAlarm.get().getCreatedAt();
}
if (maybeReportActionAlarm.isEmpty()) {
return maybeAlarm.get().getCreatedAt();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybeAlarm과 maybeReportActionAlarm이 둘 다 비어있으면

return maybeReportActionAlarm.get().getCreatedAt();

이 부분에서 예외가 터질 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

말씀해주신 부분은 public메서드에서 검증해주고 있습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

위에 메서드에서 진행하고 있었군요.

그러면 hasLatestAlarm, getLatestAlarmCreatedAt 메서드 포함해서

private boolean hasLatestAlarm(final Member member) {
    final Optional<Alarm> maybeAlarm = alarmRepository.findByMemberOrderByCreatedAtDesc(member);
    final Optional<ReportActionAlarm> maybeReportActionAlarm =
            reportActionAlarmRepository.findByMemberOrderByCreatedAtDesc(member);
    
    LocalDateTime latestAlarmCreatedAt = getLatestAlarmCreatedAt(maybeAlarm, maybeReportActionAlarm);
    return latestAlarmCreatedAt != null && member.hasLatestAlarmCompareTo(latestAlarmCreatedAt);
}

private LocalDateTime getLatestAlarmCreatedAt(
        final Optional<Alarm> maybeAlarm,
        final Optional<ReportActionAlarm> maybeReportActionAlarm
) {
    return Stream.of(maybeAlarm.map(Alarm::getCreatedAt), maybeReportActionAlarm.map(ReportActionAlarm::getCreatedAt))
            .filter(Optional::isPresent)
            .map(Optional::get)
            .max(LocalDateTime::compareTo)
            .orElse(null);
}

이런 식으로 바꿔보는 것은 어떨까요?? if문이 너무 많아서 stream으로 처리하게 해보았습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

private boolean hasLatestAlarm(final Member member) {
    final Optional<Alarm> maybeAlarm = alarmRepository.findByMemberOrderByIdDesc(member);
    final Optional<ReportActionAlarm> maybeReportActionAlarm =
            reportActionAlarmRepository.findByMemberOrderByIdDesc(member);
    final List<Optional<LocalDateTime>> maybeCreatedAts = List.of(
            maybeAlarm.map(Alarm::getCreatedAt),
            maybeReportActionAlarm.map(ReportActionAlarm::getCreatedAt)
    );

    return getLatestAlarmCreatedAt(maybeCreatedAts, member);
}

private boolean getLatestAlarmCreatedAt(
        final List<Optional<LocalDateTime>> maybeCreatedAts,
        final Member member
) {
    return maybeCreatedAts.stream()
            .filter(Optional::isPresent)
            .map(Optional::get)
            .anyMatch(member::hasLatestAlarmCompareTo);
}

그렇게해서 만들어진 레전드 로직...

Copy link
Collaborator

@woo-chang woo-chang left a comment

Choose a reason for hiding this comment

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

안녕하세요 저문 ㅎㅎ

늦은 시간까지 고생하셨습니다 👍🏻
소소한 커멘트 남겼으니 확인해주시면 감사합니다 :)

@@ -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.

좋은 생각 잘 들었습니다!

@@ -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.

위에 답변 남겼습니다!

Comment on lines 26 to 27
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.

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

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

Choose a reason for hiding this comment

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

꼼꼼히 챙겨주셨네요 👍🏻

@@ -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.

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

Comment on lines +130 to +132
public void checkAlarm() {
alarmCheckedAt = 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.

간결하고 깔끔한 구현 👍🏻

);
}

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을 감싸서 반환되는 것으로 알고 있습니다!

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.

변경해보았습니다!

@@ -238,6 +255,163 @@ void updateDetailsSameBirthYear() {

}

@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.

둘다 존재하지 않을 때에 대한 테스트도 있으면 좋을 것 같아요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

추가했습니다!


@RequiredArgsConstructor
@Persister
public class AlarmTestPersister {
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.

써보니깐 너무 편해서 좋더라구요!
다즐이 이전 것들 만들면서 고생해주셨으니, 저도 시간내서 한번 만들어두면 모두가 편하게 쓸 수 있을 것 같았어요
앞으로 자주 사용하게 될 것 같아요 👍🏻

Copy link
Collaborator

@woo-chang woo-chang left a comment

Choose a reason for hiding this comment

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

확인했습니다 마지막 기능까지 고생하셨어요 👍 ✅

Copy link
Collaborator

@tjdtls690 tjdtls690 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! ㅡ 문 ㅡ

Copy link
Collaborator

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~!!

@jeomxon jeomxon merged commit dc698bf into dev Oct 19, 2023
3 checks passed
@jeomxon jeomxon deleted the feat/#774 branch October 19, 2023 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

최신 알림 읽기 기능 구현
4 participants