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

Feature/세미나 지각 & 결석 벌점 처리 구현 #307

Merged
merged 10 commits into from
Aug 20, 2023

Conversation

shkisme
Copy link
Contributor

@shkisme shkisme commented Aug 16, 2023

🔥 Related Issue

close: #없음

📝 Description

  • 세미나 지각 2회와 결석에 따른 벌점 부여 코드를 추가로 작성했습니다.
  • 출석을 미완료한 회원의 출석 상태를 매주 금요일 자정에 스프링 스케줄러를 돌려서 결석으로 처리하고, 벌점을 부여했습니다.

⭐️ Review Request

  • 스프링 스케줄러가 제때 잘 동작하는 지도 테스트 하여야 할까요? 해야 한다면 어떤 방법으로 테스트하는 게 좋은 지 궁금합니다.

지각 2회도 결석과 구분되어야 할 벌점 사유인 것 같아 merit_type에 기본으로 데이터 넣도록 하겠습니다! -> 기획자님께 전달하기 ⭐️

@shkisme shkisme added 🚀 feature 새로운 기능 구현 및 개선사항 ⭐️ backend 백엔드 코드 작성 🐬 DB DB 변경 사항 labels Aug 16, 2023
@shkisme shkisme self-assigned this Aug 16, 2023
@shkisme shkisme changed the title Feature/세미나 지각 결석 벌점처리 Feature/세미나 지각 & 결석 벌점처리 구현 Aug 16, 2023
@shkisme shkisme changed the title Feature/세미나 지각 & 결석 벌점처리 구현 Feature/세미나 지각 & 결석 벌점 처리 구현 Aug 16, 2023
Copy link
Member

@gusah009 gusah009 left a comment

Choose a reason for hiding this comment

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

테스트도 너무 잘 짜주셨고 로직도 훨씬 간단해진 것 같네요~

개인적으론 LATENESS가 언제 되는지 로직을 찾느라 좀 해맸습니다 ㅎㅎ;;

스프링 스케줄러가 제때 잘 동작하는 지도 테스트 하여야 할까요? 해야 한다면 어떤 방법으로 테스트하는 게 좋은 지 궁금합니다.

제 생각엔 스프링 스케줄러 테스트는 딱히 필요 없을 것 같아요~ LocalDate.now()를 사용할 때 실제로 현재 시간을 가져오는지 테스트 하지 않는 것 처럼 말이죵.

스케줄러가 잘 동작할지 걱정되는건 이해합니다 ㅎㅎ 이럴 때 있는게 dev 서버니까, dev 서버에 한 번 올려서 세미나 만들고 출석안하고 기다려보시죵 출석 하지 않은 회원들이 금요일 자정 지나서 결석으로 바뀌는지 dev 서버에서 보시면 될 것 같습니다.

+ 하나 궁금한게 있는데 회원별 SeminarAttendace는 세미나가 생성될 때 생기는걸까요? 그럼 세미나가 생긴 후에 가입한 회원은 그 세미나에서 출석을 할 수 없는건지..?

Comment on lines +28 to +29
private static final long SEMINAR_ABSENCE_MERIT_TYPE_ID = 2;
private static final long SEMINAR_DUAL_LATENESS_MERIT_TYPE_ID = 3;
Copy link
Member

Choose a reason for hiding this comment

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

상벌점 종류가 추가될 때마다 이렇게 DB에 하드하게 넣어야 하는걸까요?

만약 그렇다면, id numbering은 어떻게 하죠? 겹치는게 있을수도, 빠뜨린게 있을 수도 있지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

세미나 결석지각 2회 상벌점 사유는 세미나 출석 구현할 때 필요하기도 해서, 기본 사유로 등록해두는 게 좋을 것 같다고 판단했습니다.! 그래서 하드하게 넣는 건 이거 두개만 해당할 것 같아요! 나머지는 상벌점 추가 api로...

Comment on lines 55 to 58
checkAttemptNumberLimit(key);

validAttendanceCode(seminar, request, key);
validAttendanceCode(seminar, attendanceCode, key);
checkDuplicateAttendance(seminarAttendance);
Copy link
Member

Choose a reason for hiding this comment

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

checkvalid로 prefix를 통일하면 좋을 것 같습니다~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check로 통일하겠습니다 ㅎㅎ

@@ -86,4 +111,15 @@ public void changeStatus(long seminarId, long memberId, SeminarAttendanceStatusR

seminarAttendance.changeStatus(request.excuse(), request.statusType());
}

@Scheduled(cron = "0 0 0 ? * 5", zone = "Asia/Seoul") // 매주 금요일 자정에 실행
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

Choose a reason for hiding this comment

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

세미나가 항상 금요일에 한다는 보장은 없으니 그냥 매일 자정에 cron이 돌도록 해도 좋을 것 같아요~ 만약 세미나가 없다면 어차피 find 할 때 안찾아질테니까요 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그렇네요..ㅎㅎ 수, 목요일날 하는 날이 있었던 것 같기도..
매일 자정에 돌도록 수정하겠습니다~!

@shkisme
Copy link
Contributor Author

shkisme commented Aug 19, 2023

하나 궁금한게 있는데 회원별 SeminarAttendace는 세미나가 생성될 때 생기는걸까요? 그럼 세미나가 생긴 후에 가입한 회원은 그 세미나에서 출석을 할 수 없는건지..?

지금 로직 상으로는 그렇습니다...! (이전 리뉴얼1때도 같은 방식)
아예 출석을 하지 않은 회원들을 결석 처리 할려면 미리 출석 전 데이터를 쌓아놓는 방법 말고는 떠오르지 않아서.. 세미나 생성될 때 회원별 출석 전 상태의 SeminarAttendace를 생성하도록 하였습니다.

이렇게 하면 문제가 세미나 생긴 후에 가입한 회원이거나, 세미나 생긴 후에 휴면회원->활동회원으로 바뀐 회원들은 출석이 안되는데...!

  • 방법으로는 세미나 출석 api를 호출한 당사자가 그냥 활동회원이면 넘기는 방식으로 수정해보겠습니다!
  • 위 처럼 수정한다 해도, 세미나 생긴 후에 가입한 회원이거나, 세미나 생긴 후에 휴면회원->활동회원으로 바뀐 회원들이 아예 출석을 하지 않을 경우 결석으로 간주하여 벌점 매기는 건 어려울 것 같은데 좋은 방법이 있을까요...?

@gusah009
Copy link
Member

gusah009 commented Aug 19, 2023

세미나가 생성될 때만 하지 말고, 회원이 추가되거나 활동인원으로 바뀔 때 SeminarAttendance를 추가하는 방식은 어떨까요?

(이 기회에 생산자 / 소비자 옵저버 패턴을 사용해보는것도 좋을 것 같습니당~)

Copy link
Collaborator

@02ggang9 02ggang9 left a comment

Choose a reason for hiding this comment

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

수고많으셨습니다~ 저도 옵저버 패턴 한번 살펴봐야겠어요!!

@shkisme
Copy link
Contributor Author

shkisme commented Aug 20, 2023

@gusah009

위 처럼 수정한다 해도, 세미나 생긴 후에 가입한 회원이거나, 세미나 생긴 후에 휴면회원->활동회원으로 바뀐 회원들이 아예 출석을 하지 않을 경우 결석으로 간주하여 벌점 매기는 건 어려울 것 같은데 좋은 방법이 있을까요...?

이 문제에 대한 해결방법이 있는건가... 하면서 고민 진짜 많이 했는데

세미나가 생성될 때만 하지 말고, 회원이 추가되거나 활동인원으로 바뀔 때 SeminarAttendance를 추가하는 방식은 어떨까요? (이 기회에 옵저버 패턴을 사용해보는것도 좋을 것 같습니당~)

옵저버 패턴을 연구해서 적용해보도록 하겠습니다 👍 감사합니다!

@shkisme
Copy link
Contributor Author

shkisme commented Aug 20, 2023

위 처럼 수정한다 해도, 세미나 생긴 후에 가입한 회원이거나, 세미나 생긴 후에 휴면회원->활동회원으로 바뀐 회원들이 아예 출석을 하지 않을 경우 결석으로 간주하여 벌점 매기는 건 어려울 것 같은데 좋은 방법이 있을까요...?

아 생각해보니 자정에 스케줄러 돌릴 때 처리해도 되긴 하겠네요 ㅋㅋㅋ 차선책으로 두겠습니당!

@shkisme
Copy link
Contributor Author

shkisme commented Aug 20, 2023

남은 부분 처리는 #239 에서 처리하겠습니다~!

# Conflicts:
#	src/main/java/com/keeper/homepage/domain/member/entity/Member.java
@shkisme shkisme merged commit 82f6931 into develop Aug 20, 2023
1 check passed
@shkisme shkisme deleted the feature/세미나-지각-결석-벌점처리 branch August 20, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ backend 백엔드 코드 작성 🐬 DB DB 변경 사항 🚀 feature 새로운 기능 구현 및 개선사항
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants