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

feat: 스터디 알림 기능을 구현한다. #593

Draft
wants to merge 8 commits into
base: BE/develop
Choose a base branch
from

Conversation

poi1649
Copy link
Collaborator

@poi1649 poi1649 commented Oct 31, 2023

현재 구현 중 / 팀원 참고용으로 draft 상태 업로드합니다.

@poi1649 poi1649 added backend🤎 백엔드 feat 기능 추가 jasmine👙 민트 phoa'i🏃 포이 labels Oct 31, 2023
@poi1649 poi1649 changed the base branch from develop to BE/develop October 31, 2023 05:07
@poi1649 poi1649 marked this pull request as draft October 31, 2023 05:07
Copy link
Member

@yujamint yujamint left a comment

Choose a reason for hiding this comment

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

포이 고생하셨습니다~ 이거 쉽지 않은 작업인 것 같네요 ㅋㅋㅋ..
몇 가지 궁금한 내용 코멘트 남겼습니다.


@Bean
public GoogleCredentials googleCredentials() throws IOException {
final var inputStream = new ClassPathResource("yigongil-private/firebase-adminsdk.json").getInputStream();
Copy link
Member

Choose a reason for hiding this comment

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

서브모듈에서 확인이 안 되는데 firebase-adminsdk.json 파일은 어디 있는 건가요??

Copy link
Member

Choose a reason for hiding this comment

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

참고로 이거 서브모듈에 추가했습니다! 추가 안 해도 되는 거였다면 말씀해 주세요~


void save(FirebaseToken firebaseToken);

List<FirebaseToken> findAllByMemberId(Long memberId);
Copy link
Member

Choose a reason for hiding this comment

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

한 멤버가 여러 토큰을 가지고 있을 수 있나요?? List로 반환하는 이유가 궁금합니다!

Comment on lines +83 to +85
public void sendToMember(String message, Long memberId) {
doSend(message, getMemberTopicById(memberId));
}
Copy link
Member

Choose a reason for hiding this comment

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

호출을 위임만 하고 있는 메서드가 여럿 보이네요.
메서드명을 통해 어떤 동작을 하는지 명확하게 드러내고 싶었던 걸까요??
호출을 위임하는 메서드 수가 많아서 이 클래스의 코드를 볼 때 복잡성이 느껴졌어요.

@Override
public void registerToken(String token, Member member) {
tokenRepository.save(new FirebaseToken(member, token));
subscribeToTopic(List.of(token), getMemberTopicById(member.getId()));
Copy link
Member

Choose a reason for hiding this comment

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

id가 1인 Member가 있다고 가정했을 때, registerToken() 메서드는 해당 Member가 "member 1"이라는 토픽을 구독하도록 동작하는 것 같네요.
이렇게 한 이유가 있을까요??
저번에 함께 찾아봤을 때, 특정 기기에 알림을 보낼 때는 토픽을 사용하지 않고 토큰만으로 보낼 수 있었던 것 같아서요. 멤버 개인의 토픽이 필요한 이유가 궁금합니다!

Comment on lines +55 to +63
@PostMapping("/login/fake/messaging/tokens")
public ResponseEntity<Void> registerDevice(
@Authorization Member member,
@RequestBody MessagingTokenRequest request
) {
messagingService.registerToken(request.token(), member);
return ResponseEntity.ok().build();
}

Copy link
Member

Choose a reason for hiding this comment

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

이거 프로덕션 코드와 같은 동작을 하고 있는 것 같은데, 어떤 목적으로 Fake를 만든 건가요??

String authorGithubId,
String content,
String imageUrl,
LocalDateTime createdAt
Copy link
Member

Choose a reason for hiding this comment

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

이벤트 중에서 이 녀석만 createdAt을 가지고 있는 이유가 무엇인가요??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend🤎 백엔드 feat 기능 추가 jasmine👙 민트 phoa'i🏃 포이
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants