feat: 멘토 관련 엔티티 추가 #348
feat: 멘토 관련 엔티티 추가 #348
Conversation
- application.domain 하위 -> common 하위
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
이 PR에서 VerifyStatus 위치 바꾼 거 좋은 거 같습니다! 고생하셨습니다 ㅎㅎ
src/main/java/com/example/solidconnection/mentor/domain/Channel.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/domain/Channel.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/mentor/domain/Mentor.java
Outdated
Show resolved
Hide resolved
|
확인했습니다! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lsy1307
left a comment
lsy1307
left a comment
There was a problem hiding this comment.
다들 매의 눈으로 어떻게 이렇게 잘 찾아내시는지... 문제가 없어 보여 approve 하겠습니당
| PRIMARY KEY (id), | ||
| CONSTRAINT fk_mentoring_mentor_id FOREIGN KEY (mentor_id) REFERENCES mentor (id), | ||
| CONSTRAINT fk_mentoring_site_user_id FOREIGN KEY (mentee_id) REFERENCES site_user (id) | ||
| ); |
There was a problem hiding this comment.
CONSTRAINT chk_verify_status_rejected_reason
CHECK (
(verify_status = 'REJECTED' AND rejected_reason IS NOT NULL)
OR
(verify_status != 'REJECTED' AND rejected_reason IS NULL)
)
멘토링 거절 시 반드시 거절 사유를 입력하는 거로 알고 있습니다. 관련 검증 있으면 좋을 거 같습니다!
조금 걱정이 되는건, 분명 이렇게 작성하면 무결성 측면에서는 분명 좋은데, 너무 빡빡(?)하게 구는 게 아닌가 생각이 드네요 ..ㅋㅋ
There was a problem hiding this comment.
저도 “빡빡한 check 제약”의 장단점에 대해 아는게 없어, 이번에 Gemini 한테 물어봤습니다.
답변 내용이 합리적이라는 생각이 들어 인용하고, 특히나 공감가는 부분을 표시해봤습니다. 💁🏻♀️
There was a problem hiding this comment.
위 내용을 참고하여 생각을 정리해봤는데,
"거절 시 사유를 입력해야한다"는 절대 변경되지 않을 규칙은 아니므로
(입력 시 검증 Dto Validation + 도메인 생성자에서 검증)으로 충분할 것 같다는 생각이 들었습니다!
우선 이 PR에서는 도메인 생성자에 검증을 추가하는 방식만 구현해도 괜찮을까요?
There was a problem hiding this comment.
저도 동의합니다. '검증의 세기'를 결정하는 기준 중 하나로 '과연 이 규칙이 변경되지 않을까?'를 따져보는 것도 괜찮네요!
|
|
||
| @Column | ||
| @Enumerated(EnumType.STRING) | ||
| private VerifyStatus verifyStatus; |
There was a problem hiding this comment.
다른 도메인은 verifyStatus 에 @Column(columnDefinition = "varchar(50) not null default 'PENDING'")을 작성하였는데, Mentoring 엔티티에도 작성하면 좋을 것 같습니다.
There was a problem hiding this comment.
링크 걸어주셔서 빠르게 확인할 수 있었습니다! 🙇🏻♀️
다른 도메인은 verifyStatus 에
@Column(columnDefinition = "varchar(50) not null default 'PENDING'")을 작성하였는데
이걸 보고 ‘오잉? ENUM이 아니라 VARCHAR로 columnDefinition을 설정했었나?!’라는 생각이 들어 DB 확인해보니..
다른 도메인들의 verifyStatus는 정말로 varchar(50)으로 되어있더라고요🥲
모든 도메인의 verifyStatus은 ENUM으로 관리하는 것이 원래 의도였습니다. 혼란을 드려 죄송합니다.
그리고 실수 짚어주셔서 감사합니다!
수정은 다음과 같이 진행하려합니다.
1/ DB 스키마 변경
Flyway 스크립트를 사용하여 gpa_score, language_test_score, application의
verify_status 컬럼들을 ENUM 타입으로 수정하겠습니다.
- as-is : VARCHAR(50) DEFAULT 'PENDING' NOT NULL,
- to-do : ENUM(‘PENDING', 'REJECTED', 'APPROVED') NOT NULL DEFAULT 'PENDING'
2/ JPA 코드 통일
아래와 같이 도메인 필드에서 직접 기본값을 초기화하는 방식으로 통일하겠습니다.
@Column(nullable = false)
@Enumerated(EnumType.STRING)
private VerifyStatus verifyStatus = VerifyStatus.PENDING; // 필드에서 기본값 초기화columnDefinition 옵션을 사용하지 않는 이유는,
columnDefinition을 사용한다면 코드가 지나치게 길어집니다.
→ @Column(nullable = false, columnDefinition = "ENUM('PENDING', 'REJECTED', 'APPROVED') DEFAULT 'PENDING'")
그리고 이 경우, 새로운 enum이 추가될 때 관리 포인트가 증가한다고 생각했습니다.
There was a problem hiding this comment.
성혁님이 이 방식에 동의하시면, 이슈 만들고 다른 PR에서 수정 진행하겠습니다! 🫡
There was a problem hiding this comment.
그러고보니 ENUM 을 VARCHAR 로 관리하는 것이 어색하긴 하네요. 제안하신 방법 좋습니다!
4 participants
관련 이슈
작업 내용
6.25 (수) 회의에서 논의된대로
했습니다.
이번 PR에서 'JPA 엔티티 추가'까지 한 이유는,
flyway 스크립트만 추가한 상태에서는 PR을 머지했을 때,
"스크립트랑 JPA엔티티랑 구조가 일치하지 않는다"라는 이유로 validate 실패할 것이기 때문입니다.
리뷰 요구사항
❗️회의에서 논의된 내용 이외에 이런 것들을 추가 수정했습니다. 검토 부탁드려요❗️
university_id 컬럼으로 대학을 fetch 할 때 함께 가져올 수 있는 데이터라 생각했기 때문입니다.
order는 MySQL 예약어더라고요😅
더 이상 application 패키지에서만 쓰이는게 아니라, Mentoring에서도 쓰이기 때문입니다.
기타
로컬에서 flyway validate 통과 확인했습니다👌