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 : 호스트 관련 슬랙 알림과 이벤트 추가 #329

Merged
merged 12 commits into from
Feb 10, 2023

Conversation

gengminy
Copy link
Member

@gengminy gengminy commented Feb 9, 2023

개요

작업사항

  • Slack Api 및 SlackMessageProvider 추가
  • 호스트에 등록된 SlackApi 를 가져와서 Message 를 보내줌
  • 호스트 관련 이벤트 구현
  • 호스트 관련 이벤트 핸들러 구현
  • 호스트 슬랙 알림 구현
  • 슬랙 알림 위해 Incoming Webhook 에서 관련 URL 을 입력해야함!!!! 중요
  • 호스트 알림 관련 이메일 템플릿 구현 필요
  • 호스트 알림 관련 static util 추가

스크린샷 2023-02-09 17 31 31

  • 메세지 형식 같은 것은 추후 리팩토링이 필요할듯
  • 환경변수 업데이트 되었으니 확인 바랍니다
  • 도메인 검증 로직 추가 및 변경

변경로직

  • 위와 같음

@gengminy gengminy added For: API [이슈 대상] 외부 API Type: Feature [이슈 목적] 새로운 기능 추가 labels Feb 9, 2023
@gengminy gengminy added this to the 두둥 스프린트 2차 milestone Feb 9, 2023
@gengminy gengminy self-assigned this Feb 9, 2023
Copy link
Member

@ImNM ImNM left a comment

Choose a reason for hiding this comment

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

엘쥐티엠


@Async
@TransactionalEventListener(
classes = HostRegisterSlackEvent.class,
Copy link
Member

Choose a reason for hiding this comment

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

HostRegisterSlackEvent 네이밍이
음 호스트 도메인 입장에서는 슬랙이 있는지없는지 모른다고 판단하면될듯해요!
HostRegisterEvent 가 적절할듯 싶습니다!

받아서 처리하는 쪽 문제니깜!

Copy link
Member Author

Choose a reason for hiding this comment

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

HostRegisterSlackEvent 네이밍이 음 호스트 도메인 입장에서는 슬랙이 있는지없는지 모른다고 판단하면될듯해요! HostRegisterEvent 가 적절할듯 싶습니다!

받아서 처리하는 쪽 문제니깜!

호스트를 생성했을 때 메일 날리던가 그런 이벤트도 생길 거 같은데 구분할 필요가 있지 않을까요?

final Host host = hostAdaptor.findById(hostRegisterSlackEvent.getHostId());
final String message = HostSlackAlarm.slackRegistrationOf(host);

slackMessageProvider.sendMessage(host.getSlackUrl(), message);
Copy link
Member

Choose a reason for hiding this comment

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

호스트에 슬랙 url 없을 때도 있으니
인자로 host 넘겨주고

sendMessage 내부에서
처리해도될것같기도해염

Copy link
Member Author

@gengminy gengminy Feb 10, 2023

Choose a reason for hiding this comment

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

호스트에 슬랙 url 없을 때도 있으니 인자로 host 넘겨주고

sendMessage 내부에서 처리해도될것같기도해염

인프라에서 호스트를 가지고 있으면 별로인거 같은데 이대로 하고 내부에서 NULL 체크 하는게 더 나을 거 같아요

Comment on lines 8 to 34

@Getter
@AllArgsConstructor
public class HostSlackAlarm {

public static String joinOf(Host host, User user) {
return user.toUserProfileVo().getUserName()
+ "님이 "
+ host.toHostProfileVo().getName()
+ "에 가입했습니다!";
}

public static String slackRegistrationOf(Host host) {
return host.toHostProfileVo().getName() + "에 슬랙 알림이 등록되었습니다!";
}

public static String changeMasterOf(Host host, User user) {
return host.toHostProfileVo().getName()
+ "의 마스터 유저가 "
+ user.toUserProfileVo().getUserName()
+ "으로 변경되었습니다.";
}

public static String disabledOf(User user) {
return user.toUserProfileVo().getUserName() + "님이 호스트에서 추방당했습니다.";
}
}
Copy link
Member

Choose a reason for hiding this comment

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

인프라 레이어로 옮기면 좋겠지만..
도메인 로직이있으시 아쉽긴하네요 까뷔

Copy link
Member Author

Choose a reason for hiding this comment

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

인프라 레이어로 옮기면 좋겠지만.. 도메인 로직이있으시 아쉽긴하네요 까뷔

인자로 String 받아서 인프라에 옮길까요?

@@ -3,6 +3,7 @@ jar { enabled = true }

dependencies {
api("com.slack.api:slack-api-client:1.27.2")
api("net.gpedro.integrations.slack:slack-webhook:1.4.0")
Copy link
Member

Choose a reason for hiding this comment

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

저희 기존에 있는 슬랙 공식 sdk에도
웹훅을 지원하지 않나염..?

Copy link
Member Author

Choose a reason for hiding this comment

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

저희 기존에 있는 슬랙 공식 sdk에도 웹훅을 지원하지 않나염..?

@Deprecated 표시 되어 있어서 안되는줄 알았는데 잘 되네요 바꿨씁니다

Comment on lines 20 to 25

@Bean
public Feign.Builder feignBuilder() {
return Feign.builder();
}

Copy link
Member

Choose a reason for hiding this comment

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

페인 사용안하시니깐 제거 부탁드릴게유!

Copy link
Member

@sanbonai06 sanbonai06 left a comment

Choose a reason for hiding this comment

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

슬랙 좋아용 나중에 형 코드 보고 참고해야겠어요 ㅎㅎ LGTM

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 319 Code Smells

16.2% 16.2% Coverage
0.0% 0.0% Duplication

@gengminy gengminy merged commit 7fe04bc into dev Feb 10, 2023
@gengminy gengminy deleted the feature/324-host-alarm branch February 10, 2023 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For: API [이슈 대상] 외부 API Type: Feature [이슈 목적] 새로운 기능 추가
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🚀 [feature] 호스트 관련 알림 전송 기능
3 participants