-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 근무자 고정 근무 시간 API 구현 #59
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
base: dev
Are you sure you want to change the base?
Conversation
ysw789
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 고생하셨습니다~ 코드베이스 파악을 빨리 하신 티가 납니다
공통적인 코멘트는 여기에 남기겠습니다
- UseCase 네이밍을 '고정 근무 일정 관리' 에 더 적합하도록 수정하면 어떨까 합니다. (ex. ManagerCreateFixedWorkerScheduleUseCase (수정, 삭제, 조회에도 비슷하게 적용))
- JpaRepository 인터페이스를 상속받는 리포지토리는 오직 저장만을 위해 사용하고, 조회를 위한 쿼리는 QueryDSL을 이용한 ReadOnlyRepository 로 구성해주세요 (사전에 구성되어있는 ReadOnlyRepository 구조 참조!)
- 컨벤션 상 삭제 원칙은 Soft Delete를 사용하고 있으므로 상태 속성 Enum을 만들어
WorkspaceWorkerSchedule엔티티에 status 라는 필드로 넣어 관리하게 해주세요. 삭제 시에는 삭제 상태로 status를 변경하고, 조회 시에도 삭제 상태는 제외하고 조회시키도록 하면 됩니다. 사전에 구현되어있는 다른 엔티티 구조 참조해보세요~ - 디스코드로 멘션했던 부분이기도 한데, 만일 알바 근무시간이
월요일 오후 10시 ~ 화요일 오전 4시일 경우도 고려하여 문제 없을지 검토가 필요해보입니다. 엔티티 구조를 보니 지금은 한 날짜에 시작시간 ~ 종료시간만 관리하고 있는데, 이를 (날짜와 시간을 포함한) 시작일시 ~ 종료일시 로 바꾸면 어떨까 합니다.
...mteam/alter/adapter/inbound/manager/schedule/controller/ManagerWorkerScheduleController.java
Outdated
Show resolved
Hide resolved
...om/dreamteam/alter/adapter/inbound/manager/workspace/dto/CreateWorkerScheduleRequestDto.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dreamteam/alter/common/exception/ErrorCode.java
Outdated
Show resolved
Hide resolved
월 22:00 ~ 화 04:00에 대한 정보가 한 row로 들어가는 게 어떨까요? |
ysw789
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 고정 스케줄 설정에서 FCM 알림 발송까지 진행할 필요가 있을까요? 추후에 자동 배정 되고 나서 배정 된 스케줄을 알림으로 전송하는 것은 좋아보이는데 어떻게 생각하시나요?
- null check하는 로직을 저희는
org.apache.commons.lang3.ObjectUtils의isEmpty(),isNotEmpty()를 이용하고 있습니다 참고해서 반영해주세용
| package com.dreamteam.alter.domain.workspace.type; | ||
|
|
||
| public enum WorkspaceWorkerScheduleStatus { | ||
| ACTIVE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'활성 상태' 속성은 공통적으로 ACTIVATED 로 명시하고 있습니다 그에 맞게 바꿔주세용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
따로 ObjectUtils를 사용하는 이유가있나요 정해둔 컨벤션이라면 따르겠지만
내부 구현을보니 제네릭을 사용할때 유용하게 쓸수있는것같은데
이미 타입이 정해져있는 객체에 대해서 사용하는 이유가 따로 있는지 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매니저가 근무자 고정 스케줄을 변경하니 해당 근무자에게 알림으로 변경되었다는 사실을 알려주면 좋겠다고 생각했는데 필요없다면 안해도됩니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반 객체나 String 등의 null check 방법을 일원화 하려고 사용합니당
프로젝트의 다른 부분에선 일단 ObjectUtils 로 확인하고 있습니다
...java/com/dreamteam/alter/application/workspace/usecase/ManagerDeleteFixedWorkerSchedule.java
Outdated
Show resolved
Hide resolved
...java/com/dreamteam/alter/application/workspace/usecase/ManagerCreateFixedWorkerSchedule.java
Outdated
Show resolved
Hide resolved
...mteam/alter/adapter/outbound/workspace/persistence/WorkspaceWorkerScheduleJapRepository.java
Outdated
Show resolved
Hide resolved
...alter/adapter/outbound/workspace/persistence/WorkspaceWorkerScheduleQueryRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...mteam/alter/adapter/outbound/workspace/persistence/WorkspaceWorkerScheduleJapRepository.java
Outdated
Show resolved
Hide resolved
...mteam/alter/adapter/inbound/manager/schedule/controller/ManagerWorkerScheduleController.java
Outdated
Show resolved
Hide resolved
request 필드명 변경
request 필드명 변경
관련 문서
https://www.notion.so/BE-2e486553162880598be5fc683c71f280?source=copy_link