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

Refactor #121 출석마감 자동화 #122

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

huncozyboy
Copy link
Member

PR 내용

출석마감이 자동적으로 처리되도록 자동화

PR 세부사항

동아리 딱 끝나는 시간에 맞추기 보다는, 매주 목요일 22시마다 실행되도록 @scheduled 어노테이션으로 설정해주었습니다
기존 수동 마감 로직과는 별개로 구현된 내용으로, 출석 마감이 자동화 + 수동로 진행된다고 생각해주시면 됩니다

AttendanceUseCaseImpl에 구현해도 무방하긴하지만 서비스단 목적상 비즈니스 로직이라 스케쥴링이랑 분리가 되어야할거같다는 생각에
별도의 AttendanceScheduler를 생성해 구현하였습니다

관련 스크린샷

image
Pending 상태인 출석상태

image
autoCloseAttendance 실행 후

image
ABSENT로 변경

image

정상적으로 마감처리됨

image
위 테스트 사항 로그입니다

주의사항

목요일마다 해당하는 정기모임이 있을거지만 목요일에 해당하는 정기모임이 없는거 또한 정상적인 플로우라고 생각해서, 따로 예외처리를 해주지않는 방식으로 구현했습니다. 그리고 해당 부분을 추가로 찾아보니 스케줄러 로직에 예외를 던지는 방식은 권장되지 않는다는 것도 알게되어 별도의 예외처리는 해주지 않았습니다

로직에 대한 테스트는 @scheduled(cron = "0 0 22 * * THU") 어노테이션 부분을 주석처리하고, 별도의 컨트롤러를 만들어서 단위테스트로 진행하였습니다. 더미데이터 생성 후 테스트 진행하였는데, 정기모임 날짜가 오늘(테스트 기준 2월9일, Scheduled 적용시 매주 목요일) 로만 되어있는 더미데이터에만 적용되는거 확인했고, 출석 마감 후 PENDING 상태였던 status가 올바르게 ABSENT로 변경되는거 또한 확인했습니다


체크 리스트

  • 리뷰어 설정
  • Assignee 설정
  • Label 설정
  • 제목 양식 맞췄나요? (ex. #0 Feat: 기능 추가)
  • 변경 사항에 대한 테스트

Copy link
Collaborator

@jj0526 jj0526 left a comment

Choose a reason for hiding this comment

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

수고하셨어요

private final AttendanceGetService attendanceGetService;
private final AttendanceUpdateService attendanceUpdateService;

@Scheduled(cron = "0 0 22 * * THU")
Copy link
Collaborator

@jj0526 jj0526 Feb 9, 2025

Choose a reason for hiding this comment

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

@scheduled(cron = "0 0 22 * * THU", zone = "Asia/Seoul")로 설정해는게 더 정확할거같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

zone = "Asia/Seoul"가 없으면 예상 시간과 실제 스케줄 동작 시간의 차이가 발생할 수도 있을거같네용
수정 완료했습니다 !

@Scheduled(cron = "0 0 22 * * THU")
public void autoCloseAttendance() {
LocalDate today = LocalDate.now();
Cardinal currentCardinal = cardinalRepository.findTopByOrderByCardinalNumberDesc();
Copy link
Collaborator

Choose a reason for hiding this comment

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

설정하지 않아도 자동으로 최근 기수 가져오는거 좋아요!

meetings.stream()
.filter(meeting -> meeting.getStart().toLocalDate().isEqual(today) &&
meeting.getEnd().toLocalDate().isEqual(today))
.findAny()
Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그리고 findAny()를 쓰신 이유가 어떻게 될까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

.forEach와 비교해서 findAny()를 사용하신 이유를 여쭤보신거죵 ??


제가 이해한 게 맞다면, 저희가 설정한 조건이 @scheduled(cron = "0 0 22 * * THU")라서 목요일 22시에 조건에 맞는 정기모임 하나만 마감하는 형태가 출석 자동화라는 목적에 더 맞겠다라고 판단했습니다…!

 forEach()는 해당 요일인 목요일에 여러 정기모임이 있다는 가정하에 사용해줘야하니까 구현 목적상 맞지않을거같다는 생각도 있었습니다


또 forEach()는 스트림이 모든 요소를 순회해서 처리해줘서, 모든 정기모임에 필터링 작업이 처리된다면 연산 속도가 떨어질 수도 있지만, findAny()는 이와 비교해서 조건을 만족하는 첫번째 정기모임을 마감해주기때문에 스트림 처리 시간이 짧아 성능성 이점이 있겠다는 생각도 있어서 해당 방식으로 구현했습니당

Copy link
Collaborator

Choose a reason for hiding this comment

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

성능보다는 정기 모임이 하루에 2개인 경우 또는 잘못 만들어졌을 경우를 생각해서 .forEach가 나아보이는데 어떻게 생각하시나요?

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.

2 participants