feat: 생일 메시지 기능 - #42
Hidden character warning
Conversation
There was a problem hiding this comment.
안녕하세요 ! 수고많으셨어요👏👏👏👏
설계부터 구현까지 요구사항이 복잡한 기능이셨을텐데 매우 잘 구현해주셨네요!!
관리자 기능까지 매우 다양한 명령어를 구현해주셨는데, 덕분에 관리하기 편할 것 같아요👍
기능 사용해보면서 느낀 몇가지 코멘트 남겨보았습니다! 또한 공통 피드백도 확인 부탁드려요~
공통 피드백
1. EOF해결
EOF가 가 몇군데 보여요! 확인부탁드립니다~
2. 로그 사용 범위
로그를 매우 꼼꼼하게 달아주셨어요. 로그는 디버깅이나, 개발 중 기능 흐름을 파악하는 데 아주 유용하죠!
다만 로그를 너무 많이 찍는다면, 로그 파일의 크기도 커지고, 로그 파일에서 확인 시, 진짜 중요한 로그(에러, 경고 등)를 추적하기 어려워질 수 있어요.
따라서 리뷰 남긴 부분 외에도, 핵심적인 상태 변화나 장애 추적에 도움이 되는 로그를 제외한 나머지 로그들은 정리(삭제 또는 레벨 조정)해주시면 좋을 것 같아요!
| private final JDA jda; | ||
| private final BirthdayService service; | ||
|
|
||
| @Value("${discord.birthday_message_channel_id}") |
There was a problem hiding this comment.
공통 자유 채널을 생각하고 설계했는데, 테스트를 위해 일단 테스트 채널로 해뒀습니다.
| public String pickMessage(LocalDate today) { | ||
| //final long randomSeed = today.toEpochDay(); | ||
| final int[] probabilityBox = messageTodayData.probabilityBox(); | ||
| // final int todayMessageIndex = new Random(randomSeed).nextInt(probabilityBox.length); |
There was a problem hiding this comment.
생일축하 기능은 하루에 한번만 작동해야 하기때문에, 제 생각에도 랜덤시드를 만들어 랜덤 값을 일정 시간 고정할 필요는 없다고 생각합니다~
주석부분 삭제해도 될 것 같아용
There was a problem hiding this comment.
저도 말씀 주신 내용에 동의해서, 랜덤 시드 관련 주석은 삭제했습니다!
| public void checkBirthdays() { | ||
| List<Birthday> birthdays = service.findTodayBirthdays(); | ||
| if (birthdays.isEmpty()) { | ||
| log.info("📢 오늘 생일자가 없습니다!"); |
There was a problem hiding this comment.
해당 if문은 대부분의 경우에 걸리게될텐데, 그때마다 == 30초마다 계속 찍히게 된다면 너무 많은 로그가 쌓이게 될 것 같아요!
삭제해도 좋을 것 같습니다..!
There was a problem hiding this comment.
말씀 주신 부분 공감합니다!
현재 테스트를 위해 30초 주기로 실행 중이라 해당 로그가 과도하게 쌓일 수 있을 것 같아 로그는 제거했고, 생일자가 없는 경우 이후 로직이 실행되지 않도록 return은 유지했습니다
| log.info("📢 오늘의 생일은 : {} {}", | ||
| birthday.getUserId(), | ||
| birthday.getUserName()); |
There was a problem hiding this comment.
이 부분도 이미 ui상에 노출되는 부분이기에 필요성을 체감하지 못했습니다..! 오류가 날 경우의 로그를 남기는 것이 더욱 의미있을 것 같아요
There was a problem hiding this comment.
저도 말씀 주신 내용에 공감해서,해당 로그는 UI 상으로 이미 확인 가능한 정보라 제거했습니다!
| } | ||
|
|
||
| private void sendBirthdayMessage(TextChannel channel, Birthday birthday) { | ||
| String message = "오늘은 <@%s> (**%s**)님의 생일입니다!".formatted( |
There was a problem hiding this comment.
앞에 @ 빼야할 것 같아요!
그리고 요건 개인적인 제안사항인데요..
오늘은 <@@남해윤> 윤해남님의 생일입니다!
현재는 이런식으로 언급되고 있는데,
@남해윤 오늘은 윤해남님의 생일입니다!
이런식으로 메세지 구성을 바꾸는 것은 어떤가요? 언급을 먼저 하는 것이 자연스럽기도하고, 닉네임을 설정할 수 있는 기능이 있기 때문에 닉네임으로 불러주는 것이 더 기분좋을(?) 것 같아서요 ㅋㅋㅋ
There was a problem hiding this comment.
디스코드 멘션 형식에 맞게 <@userid>로 수정하고,멘션이 문장 앞에 오도록 메시지 구조를 변경했습니다. 좋은 제안 감사합니다!
| //@Scheduled(cron = "0 0 0 * * *") | ||
| @Scheduled(fixedDelay = 30000) |
There was a problem hiding this comment.
현재 한번 생일을 등록하면, 30초마다 무한 축하받는 오류가 생기고있어요..! 생일자가 잠못이루는 밤을 보내지 않도록 생일이 된 순간 00시 00분에 한번 축하해주고 반복을 멈추면 좋을 것 같습니다🥹
위 부분을
@Scheduled(cron = "0 0 0 * * *", zone = "Asia/Seoul") 이런식으로 바꿔주세요!
근데 지금 당장 바꾸면 테스트가 어렵기 때문에, 마지막에 한번 테스트하고 바꿔주시면 될 것 같아요
There was a problem hiding this comment.
해당 이슈 인지하고 있고, 운영 시에는 자정 1회 실행되도록 cron 스케줄로 변경할 예정입니다.
현재는 테스트 편의를 위해 fixedDelay를 사용 중이며, 마지막 테스트 후 반영하겠습니다!
There was a problem hiding this comment.
테스트 과정에서 이전 코드 구조로 인해 등록하지 않았던 데이터가 남아 있는 경우가 있어서 우선 필요하다고 판단했습니다.
그리고 이후 운영 단계에서 관리자가 전체 데이터를 한 번에 초기화해야 하는 상황도 충분히 있을 것 같아, 미리 만들어두었습니다.
| @Override | ||
| public Set<DiscordRole> allowedRoles() { | ||
| return Set.of(DiscordRole.MEMBER); | ||
| } |
There was a problem hiding this comment.
BirthdayInfoCommandListener와, BirthdayListCommandListener와, BirthdayClearCommandListener는
사용자 기능보다는 관리자 기능에 가까워보여요
특히 생일정보를 전체 초기화하는 BirthdayClearCommandListener의 경우는 아무나 사용할 수 있다면 위험할 것 같아요
세 Listener의 Role을 MEMBER가 아닌 저희 = 개발자 = DiscordRole.DEVELOPER가 되어야할 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
저도 말씀 주신 의견에 동의합니다.
생일 등록 및 관리 기능은 관리자 전용으로 설계한 부분이라, Listener 모두 DiscordRole.DEVELOPER로 수정하겠습니다.
| .addOption(OptionType.STRING, "userid", "디스코드 아이디", true) | ||
| .addOption(OptionType.STRING, "username", "이름", true) | ||
| .addOption(OptionType.STRING, "birthday", "생일은 MM-DD형식으로 입력해주세요", true); |
There was a problem hiding this comment.
단순히 디스코드 멘션으로만 불러주는 것이 아니라, 사용자가 직접 설정한 닉네임으로 불러주는 거 매우 좋은 것 같습니다!
제안 사항
대부분의 사용자는 본인의 생일만 등록할 가능성이 높기 때문에,
디스코드 아이디를 직접 입력받기보다는 명령어를 실행한 사용자의 디스코드 ID를 자동으로 추출해서 사용하는 방식도 있을 것 같은데, 해당 방식을 선택하신 이유는 무엇인가요?!
다른 사람의 생일을 대신 등록해주는 경우도 충분히 있기때문에, 지금 방식 그대로 유지하는 것도 좋은 것 같기도하고... 어렵네요
어떤 방식이 더욱 적절하다 판단하셨는지 의견이 궁금합니다!
추가 제안 (안내 문구 개선)
사용자 혼란을 줄이기 위해 입력 안내 문구를 조금 더 구체화하면 좋을 것 같습니다.
- 디스코드 아이디 → 생일을 등록할 사람의 디스코드를 멘션해주세요.
- 이름 → 생일 축하 메시지에 표시될 이름(닉네임)을 입력해주세요.
- 생일 → MM-DD 형식으로 입력해주세요. (예시: 01-23)
There was a problem hiding this comment.
좋은 질문 감사합니다!
생일 등록 기능을 일반 사용자 기능이라기보다는, 디스코드 봇을 운영·개발하는 관리자(개발자)들이 데이터 정합성을 관리하는 운영 기능으로 보고 설계했습니다.
그래서 userid를 자동 추출하는 셀프 등록 방식보다는, 관리자가 등록 대상 사용자를 명시적으로 지정할 수 있도록 userid를 입력받는 방식을 선택했습니다.
디스코드 ID는 변경 가능성이 낮고, 현재 데이터 규모도 크지 않아 관리자 기준에서 충분히 관리 가능하다고 판단했습니다.
또한 혹시 변경되더라도 관리자에게 요청하여 기존 데이터를 정리·갱신하는 운영 방식으로 대응할 수 있다고 보았습니다.
다만 사용자(= 디스코드 봇 개발자) 입장에서 혼란을 줄이기 위해,
입력 안내 문구는 제안해주신 것처럼 더 구체적으로 개선하겠습니다. 감사합니다.
| return birthdayRepository.findByMonthDay(now.getMonthValue(), now.getDayOfMonth()); | ||
| } | ||
|
|
||
| public String pickMessage(LocalDate today) { |
There was a problem hiding this comment.
해당 메서드 현재 작동하지 않는 것 같은데 확인부탁드려요!
Test Results28 tests 24 ✅ 0s ⏱️ For more details on these failures, see this check. Results for commit eaa070a. ♻️ This comment has been updated with latest results. |
157e197 to
eaa070a
Compare
eaa070a to
157e197
Compare
157e197 to
eaa070a
Compare
2 participants
작업 내용
관리자 권한 기능
1)
birthday-add(생일 등록)2)
birthday-clear(생일 전체 삭제)3)
birthday-delete(생일 삭제)4)
birthday-info(생일 조회)5)
birthday-list(생일 목록 조회)전체 기능 설명
DB에 저장된 생일 정보를 기반으로, 디스코드 봇이 특정 시점에 생일자를 조회합니다.
해당 날짜에 생일자가 존재할 경우, 미리 준비된 생일 축하 메시지 중 하나를 확률적으로 선택하고,
이모지와 함께 공통 채널에 축하 메시지를 전송하는 것을 목표로 합니다.
참고 사항
원래는 Google Calendar 연동 → 캘린더에 저장하는 방식으로 설계하려고 했습니다.
다만 첫 개발 단계에서 캘린더 연동의 난이도를 고려해,
우선은 단일 디스코드 채널 + DB 기반으로 기능을 구현하며 전체 흐름을 파악하는 데 집중했습니다.
이후 단계에서 Google Calendar 연동 기능으로 확장할 예정입니다.
생일 축하 메시지 선택 확률을 환경별로 조절하기 위해
application-discord.yml과application-local.yml에 확률 관련 설정을 추가했습니다.application-discord.yml
application-local.yml
관련 이슈
PR 체크리스트