지원서(application) 테이블에서 학점/어학 인증 분리 및 지원 절차 수정 #109

leesewon00 · 2024-11-12T10:06:52Z

작업 내용

학점,어학,지원 분리
지원 절차 수정
테스트 코드 작성

테스트 수행 결과

특이 사항

(1)
[post] /score/gpaScore : 학점 등록 api
[post] /score/languageTestScore : 어학성적 등록 api

(2)
[get] /score/gpaScore/status : 로그인한 유저에게 등록된 학점 정보 및 인증 상태 조회
[get] /score/languageTestScore/status : 로그인한 유저에게 등록된 어학성적 정보 및 인증상태 조회

(3)
[post] /application : 지원서 제출하기 api

(1) 과정에서 학점과 어학성적을 등록 한 뒤,
(2) 과정을 통해 조회된 성적 중 승인된 성적들을 선택하여
(3) 과정을 통해 지원서를 제출하는 순으로 진행됩니다.

아래 두 개의 api의 경우 n+1 문제가 발생하여 추가적인 수정이 필요합니다.
[get] /application
[get] /application/competitors

리뷰 요구사항 (선택)

wibaek · 2024-11-12T15:07:01Z

1. api naming에 관하여

아직 내부 코드를 확인해보지는 않았고, 경로에 대해서만 우선 간단히 커멘트 남깁니다.

1.1

다음 api endpoint 명명에 관하여 이후 확장성에 문제가 없다면 중복된 부분을 지워도 될 것 같습니다!
/score/gpaScore -> /score/gpa
/score/languageTestScore -> /score/languageTest

1.2

api에서 여러 단어로 이루어진 리소스에 대하여 현재 api들을 보면 kebab-case와 camelCase가 혼재되어 있습니다.

예시
/my-page/update/profileImage
/posts/{post_id}/comments/{comment_id}
/file/s3-url-prefixS3

이 중 어떤것을 써도 좋지만 통일성을 유지하는게 중요하다고 생각합니다. 그래서 앞으로의 api에 대해서는 kebab-case를 사용하는 것을 제안드립니다.
다음은 주로 쓰이는 3가지 케이스에 대해 gpt로 간단히 정리해보았습니다

Q. rest api에서 여러 단어가 있다면 어떤 네이밍케이스로 쓰는게 좋을까?
1 케밥 케이스 (kebab-case)
추천 상황: URL 경로는 주로 케밥 케이스를 선호하는 것이 일반적입니다.
2 스네이크 케이스 (snake_case)
추천 상황: JSON 형식의 속성 이름 또는 쿼리 파라미터 등에 사용될 수 있습니다.
3 카멜 케이스 (camelCase)
추천 상황: 주로 JSON 응답에서 속성 네이밍에 사용되며, URL 경로로는 비추천입니다.
결론
URL 경로: 케밥 케이스 (/user-profiles)
쿼리 파라미터 및 JSON 속성: 스네이크 케이스(user_profile) 또는 카멜 케이스(userProfile)

이러한 이유로 languageTestScore 대신 language-test-score과 같이 카멜케이스를 사용해보는게 어떨까 건의드립니다.

1.3

[post] /score/gpaScore : 학점 등록 api
[get] /score/gpaScore/status : 로그인한 유저에게 등록된 학점 정보 및 인증 상태 조회

해당 API endpoint에서 post나 get이라는 method 자체가 조회와 등록의 의미를 가지고 있기 때문에, 조회시에 뒤에 /status를 붙이지 않고 post와 같은 endpoint를 써도 괜찮을 것 같습니다.
예) GET /score/gpaScore

아직 내부 코드 구현을 확인해보지는 않았으나, 저는 각 학점과 어학 인증을 개별 리소스로 취급하는 구조를 생각했는데, 이 경우 각 인증을 삭제하거나 할때 개별 리소스 api endpoint가 필요할텐데 이때 /score/gpaScore/(id) 와 같이 사용된다면 status를 제외하는게 좋을 것 같다는 이유도 있습니다.

메모: https://learn.microsoft.com/ko-kr/azure/architecture/best-practices/api-design

wibaek · 2024-11-17T08:39:28Z

src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java

+
+        Long gpaScoreId = applyRequest.gpaScoreId();
+        Long languageTestScoreId = applyRequest.languageTestScoreId();
+        GpaScore gpaScore = validateGpaScore(siteUser, gpaScoreId);


궁금한것이 있는데 기존에 값을 검증하되 따로 리턴값은 없는 로직들을 validate~() 메소드로 정의해서 사용하시는 걸 보고 좋은 아이디어라는 생각을 가지고 있었는데요, validateGpaScore는 검증도 있지만 siteUser와 gpaScoreId를 이용해 find 하는것에 가깝다는 느낌도 들어서 혹시 메서드 명을 짓는 기준이 있으신지 궁금합니다!

우선 꼼꼼히 확인해주셔서 감사합니다.
지금 살펴보니 해당 validate메소드의 경우 검증을 진행하지만 리턴값도 존재하여
검증 및 조회라는 두가지 역할을 수행하는 상황이라고 생각됩니다.

이 부분의 경우 사실 말씀하신대로 getter 느낌이 더 강한 것 같은데
두개의 메소드로 분리하는것이 좋을지, getGpaScore와 같이 네이밍을 바꾸는게 좋을지 고민되네요.
개인적으로는 메소드를 분리하는게 더 좋을 것 같긴한데 의견주시면 좋을 것 같습니다.

상당히 애매한 부분같은데, 확실히 검증의 의미가 포함되어 있기는 하나 저는 다음과 같은 상황을 가정해보았습니다.

현재 저희 코드에서는 scoreId는 유저와 관련없이 고유합니다. 그러나 유저별로 scoreId가 따로 결정되는, 즉 GpaScore의 candidate key가 (userId, scoreId)인 경우를 가정해보았습니다. 이 경우에 findGpaScore(userId, scoreId)를 하는것은 검증을 하기보다는 말 그대로 조회를 해서 엔티티를 가져오는 느낌입니다.

그래서 저는 validate보다는 get~ find~등을 사용해도 좋을 것 같습니다.

반영했습니다.

wibaek · 2024-11-17T08:47:29Z

src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java

+        LanguageTestScore languageTestScore = validateLanguageTestScore(siteUser, languageTestScoreId);
+
+        Optional<Application> application = applicationRepository.findBySiteUserAndTerm(siteUser, term);
+        application.ifPresentOrElse(before -> {


//메모

이후에 경쟁률 변화에 대한 기록을 남기고 거짓 제출 경향등을 보기 위해서, 소프트 딜리트 방식을 도입해 기존에 application이 있을 때 덮어 씌우는 것이 아닌 isDelete = true 등으로 설정하는 방식을 사용하는 것도 좋을 것 같습니다.

이후에 경쟁률 변화에 대한 기록을 남기고 거짓 제출 경향등을 보기 위해서

라는 부분에서 추후 새로운 정보를 얻기 위해서 저장해야할 필요성이 있다고 생각이 드네요.
좋은 의견인 것 같습니다.
soft delete에 대해서 제가 깊게 생각해본적이 없는데 이번기회에 한번 학습해보면 좋을 것 같네요.
감사합니다!

반영했습니다.

wibaek · 2024-11-17T08:58:05Z

src/main/java/com/example/solidconnection/score/domain/LanguageTestScore.java

+@Entity
+@NoArgsConstructor
+@AllArgsConstructor
+public class LanguageTestScore extends BaseEntity {


// 메모

이후에 GpaScore와 LanguageTestScore에 expirationDate 만료 날짜 필드를 추가해 만료된것은 application시 사용 불가와 같은 로직을 추가하는 것도 좋을 것 같습니다.

개인적으로는 만료된 성적의 경우 주기적으로 DB에서 삭제할 생각이였는데
이 역시도 soft delete 느낌을 추천하시는 것 같은데 제가 이해한바가 맞는지 궁금합니다.

성적의 경우 application에 id값이 아닌 value값이 삽입되는 구조라 데이터를 유지해야할 필요를 크게 느끼지 못했던 것 같은데
저장했을때 어떤 측면에서 용이하다고 생각하시는지 궁금합니다.

soft delete 방식을 추구하는 것도 맞지만, 말씀하신것처럼 '만료된 성적을 주기적으로 삭제' 를 할때도 만료날짜를 기록해둔다면 삭제하기 편하니 말 그대로 만료 날짜를 기록하고 만료된 성적은 사용하지 못하게 하는 용도입니다. 실제로 교한학생 지원시에도 만료된 성적은 내지 못하니, 만료된 성적은 disable 시켜버리는 것이지요.

편의성에 대해서는 이해가되네요. 감사합니다.

이후에 GpaScore와 LanguageTestScore에 expirationDate 만료 날짜 필드를 추가해 만료된것은 application시 사용 불가와 같은 로직을 추가하는 것도 좋을 것 같습니다.

추가적으로 이부분에서 만료날짜 설정에 있어서 궁금한점이 있습니다.
만료일자를 한학기로 설정하는게 좋을 지, 어학성적의 경우 실제 어학만료일(2년뒤)을 기입하는게 좋을 지 궁금합니다.
만료일자에 대해서 어떻게 생각하고 제안하신건지 궁금합니다.

wibaek

전체 코드 확인하는데 시간이 좀 걸렸습니다. 늦어서 죄송합니다.

코드는 모두 좋아보입니다! 현재 코드대로 확정해도 될 것 같으나, 코드를 보며 2가지 점에 대해서 이런방식으로 하는 건 어떨까 라는 생각이 들어서 커멘트 남겨둡니다!

application에 성적값 복사해서 저장되는 것을 GpaScore, LanguageTestScore id FK를 저장하는 방식으로 수정하기

우선 해당 사항은 저 역시 FK를 저장하는 것이 아닌 값을 복사해 저장하는 것이 좋겠다고 말씀드렸던 것으로 기억합니다.
당시에 이렇게 복사해서 저장하는것이 좋겠다 생각한 이유는 유저 탈퇴나 성적 데이터 삭제시에 application 정보는 ON DELETE CASCADE가 아닌 ON DELETE SET NULL로 설정하여 과거 경쟁률 데이터들을 보존하려는 목적이었습니다.

그러나 이를 두고 고민 해보았을 때 성적 데이터들도 soft delete를 하거나 ON DELETE SET NULL로 저장하고 정규화를 진행하는 것이 좋겠다는 생각이 들었습니다.

해당 부분을 수정했을 때의 장점은 정규화 했을 때의 장점이 되겠고, 단점은 현재도 복잡한 application 리스트 조회 쿼리에서 성적 join로직이 추가되었을 때 더 복잡해질 수 있다는 것이 되겠습니다.

GpaScore과 LanguageTestScore 수정 대신 새로 생성

현재 로직을 보았을때 GpaScore는 기존에 GpaScore가 있으면 무조건 덮어쓰기, LanguageTestScore은 같은 테스트 타입이면 덮어쓰기 방식으로 구현되어 있는 것으로 확인했습니다.

저는 처음 application 관련 코드를 보았을 때, 그리고 DB에서 수동으로 성적인증요청을 확인하고 상태를 APPROVE 하며 느낀 점이, 엔티티를 계속 수정해서 사용하는 것이 코드와 로직의 전반적인 복잡도를 올린다는 느낌을 받았습니다.

그래서 각 성적인증을 immutable?한 방식으로 다루는 것을 제안드립니다. 이를 통해 복잡도를 낮추고 데이터의 history를 더욱 잘 남길 수 있다고 생각합니다.

즉 구현에서 현재 score나 이미 존재하는 종류의 시험 데이터가 있을 때 이를 덮어 씌우는 것을 그냥 새로운 엔티티를 만드는 것으로 변경하는 것입니다.

기존에는 s3에 저장된 성적인증 파일들에 대해 성적인증이 덮어씌워지면 기존의 파일들은 추적이 어려워집니다. 즉 파일을 가리키는 포인터가 사라지는 느낌입니다. 이런것들에 대해서도 계속 추적할 수 있고 관리가 용이해집니다.
그리고 created_at, updated_at 기록도 보다 정확하게 할 수 있습니다.

꼭 이런방식으로 진행할 필요는 없고 오늘 오후동안 생각해본 것들을 적어둔것이니 한번 검토만 부탁드립니다!

leesewon00 · 2024-11-18T07:56:24Z

application에 성적값 복사해서 저장되는 것을 GpaScore, LanguageTestScore id FK를 저장하는 방식으로 수정하기

GpaScore과 LanguageTestScore 수정 대신 새로 생성

이 부분에 있어서는 조금 더 고민해보고 답변드리겠습니다.

wibaek · 2024-11-26T13:06:10Z

메모: 2번만 적용

leesewon00 · 2024-11-27T06:32:59Z

2번 반영완료

nayonsoso

세원님 몇가지 질문이 있어 rc 드립니다~

(1) 과정에서 학점과 어학성적을 등록 한 뒤,
(2) 과정을 통해 조회된 성적 중 승인된 성적들을 선택하여
(3) 과정을 통해 지원서를 제출하는 순으로 진행됩니다.

승인된 성적들을 어떻게 선택을 하는건가요?
예를 들어서, 토익도 제출하고 토플도 제출을할 수 있는데, 지원서를 제출할 때 둘 중 하나를 고르게 되는건가요?

nayonsoso · 2024-11-29T17:03:59Z

src/main/java/com/example/solidconnection/application/dto/ApplyRequest.java

+@Schema(description = "지원서 제출")
+public record ApplyRequest(
+        @NotNull(message = "gpa score id를 입력해주세요.")
+        @Schema(description = "지원하는 유저의 gpa score id", example = "1")
+        Long gpaScoreId,
+
+        @NotNull(message = "language test score id를 입력해주세요.")
+        @Schema(description = "지원하는 유저의 language test score id", example = "1")
+        Long languageTestScoreId,
+
+        UniversityChoiceRequest universityChoiceRequest
+) {
+}


사용자가 직접 id 를 입력하도록 바뀌는 건가요?

말씀하신 대로 토익과 토플 성적이 모두 등록될 수 있기에 여기서 골라서 지원하는 방식입니다.
유저가 화면상의 본인 성적을 선택하게되면 해당 성적의 id를 api로 전송하는 방식입니다.

회원마다 성적을 여러개를 보유할 수 있어서 선택하여 지원하는 방식으로 진행했습니다.

충분한 답변이 되었는지 잘 모르겠네요ㅜ
궁금한점 있으시면 말씀해주세요!

본인 성적을 선택하게되면 해당 성적의 id를 api로 전송하는 방식

아하 이해했습니다😊

nayonsoso · 2024-11-29T17:07:50Z

src/main/java/com/example/solidconnection/score/domain/GpaScore.java

+public class GpaScore extends BaseEntity {
+    @Id
+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+    @Embedded
+    private Gpa gpa;


Gpa 가 Embedded 로 Application 에도 저장되고 GpaScore 에도 저장되고 있는게 맞나요?
중복된 저장으로 데이처 정합성 유지가 힘들어지고, 저장 공간을 이중으로 차지할 것 같은데
이렇게 구성하신 이유가 있으신가요?
(혹시 제가 맥락을 이해 못하고 있는거라면 말씀해주세요 😊)

제 생각을 말씀드리자면
Application에 Gpa가 id값이 아닌 값으로 저장되는 방식이 회원탈퇴 시점에도 값을 자연스레 유지할 수 있어 좋다고 생각하였고,
join하는 방식으로 기록을 가져오지 않아서 쿼리 성능에서도 이득을 볼 수 있을 것이라고 생각하였습니다.

물론 말씀하신대로 Application에는 값을 직접 저장하지 않고, id 값을 저장해도 될 것 같습니다.
현재 지원서를 조회하는 api 성능이 매우 저하되어서 해당 api를 리팩토링 할 때 다시 고려해보면 좋을 것 같습니다.

join하는 방식으로 기록을 가져오지 않아서 쿼리 성능에서도 이득을 볼 수 있을 것

비정규화를 통해 성능 향상을 기대해볼 수 있단 말씀이신군요👍
개인적 의견으로는..
하나의 join이 사라진다고 해서 큰 효과가 있을것이라고 생각이 되진 않습니다🥲
하지만 이것도 개인적 의견이니 api를 리팩토링할 때 다시 살펴보겠습니다!!

혹시라도 가능하시다면 두 경우의 성능을 비교해주신다면 더욱 좋을 것 같습니다!

nayonsoso · 2024-11-29T17:18:50Z

src/main/java/com/example/solidconnection/score/controller/ScoreController.java

+    // 어학성적을 등록하는 api
+    @PostMapping("/languageTest")


api에서 여러 단어로 이루어진 리소스에 대하여 현재 api들을 보면 kebab-case와 camelCase가 혼재되어 있습니다.
이 중 어떤것을 써도 좋지만 통일성을 유지하는게 중요하다고 생각합니다.

위백님이 말씀하신 api 네이밍에 대해서는, 저도 kebab-case 로 통일하는 것 찬성합니다🙋🏻‍♀️
그런데 위백님도 말씀하셨다시피 통일성이 중요하기 때문에, 모든 api 를 하나로 통일하는게 아니라면 의미가 없겠다는 생각이 듭니다.
그래서 ❗️api 네이밍 수정과 관련된 PR 을 따로 만들어서 전체 api 를 통일❗️하는게 좋겠습니다.
(지금 PR에서 하기에는 PR이 너무 뚱뚱해질 것 같아요)

위백님은 '앞으로는 통일했으면 좋겠다'고 하셨는데, 기존 것을 포함하여 전체를 통일하는 것은 어떻게 생각하시나요?
@wibaek

저도 가능하면 모두 통일하는게 좋다고 생각합니다

추후 issue 등록하여 모두 통일하면 좋을 것 같습니다.

leesewon00 · 2024-12-04T08:17:16Z

추후 배포시점에 신규 테이블은 정상으로 생성될 것이라고 예상되지만,
Application에 isDelete 컬럼의 경우 직접 생성해야 될 가능성이 존재하여 쿼리문 첨부합니다.
ALTER TABLE Application ADD COLUMN isDelete TINYINT(1) NOT NULL DEFAULT 0;

leesewon00 added 10 commits November 12, 2024 18:48

feat: 학점, 어학 엔티티 정의

a28e917

feat: 학점, 어학 레포지토리 정의

f451720

feat: 학점, 어학 서비스 로직 추가

9382dc4

feat: 학점, 어학 컨트롤러 로직 추가

1e10e9c

feat: 학점, 어학 관련 DTO 정의

18c9cda

feat: 학점, 어학 관련 예외 정의

d067ba4

feat: 학점, 어학 레포지토리 테스트 로직 추가

23be4d7

feat: 학점, 어학 서비스 테스트 로직 추가

802475b

refactor: 수정안에 따른 지원 절차 수정

feb7109

refactor: 수정안에 따른 지원 절차 테스트 로직 수정

5f2f330

leesewon00 added the 기능 label Nov 12, 2024

leesewon00 self-assigned this Nov 12, 2024

leesewon00 requested a review from wibaek November 12, 2024 10:17

feat: api naming 통일을 위한 경로 수정

e270e23

wibaek reviewed Nov 17, 2024

View reviewed changes

wibaek approved these changes Nov 17, 2024

View reviewed changes

refactor: 조회 목적에 맞게 함수명 수정

2faebcd

leesewon00 added 2 commits November 19, 2024 11:45

refactor: 지원서 제출에서 기존이력을 soft delete 방식으로 처리

1ccf4a5

refactor: 예상하지 못한 쿼리 실행을 방지하기 위해 즉시로딩 지연로딩으로 수정

b9f470f

refactor: GpaScore, LanguageTestScore 이력 수정 대신 새로운 객체 생성하여 처리

3f2e719

nayonsoso requested changes Nov 29, 2024

View reviewed changes

nayonsoso reviewed Nov 29, 2024

View reviewed changes

fix: 추천 대학 목록 조회 테스트 실패로 즉시로딩으로 수정

96d6ab3

nayonsoso approved these changes Dec 3, 2024

View reviewed changes

leesewon00 merged commit ff6f7b7 into solid-connection:main Dec 4, 2024

wibaek mentioned this pull request Dec 21, 2024

chore: flyway 도입 #130

Merged

wibaek mentioned this pull request Jan 6, 2025

지원서(application) 테이블에서 학점/어학 인증 분리 및 인증 담당자 승인 관리자 페이지 기능 추가 #57

Closed

Gyuhyeok99 mentioned this pull request Mar 29, 2025

refactor: Application에서 Gpa와 LanguageTest 중복 제거 및 정규화 #262

Open

2 tasks

		// 어학성적을 등록하는 api
		@PostMapping("/languageTest")

지원서(application) 테이블에서 학점/어학 인증 분리 및 지원 절차 수정 #109

지원서(application) 테이블에서 학점/어학 인증 분리 및 지원 절차 수정 #109

Uh oh!

Conversation

leesewon00 commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

관련 이슈

작업 내용

특이 사항

리뷰 요구사항 (선택)

Uh oh!

wibaek commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. api naming에 관하여

1.1

1.2

1.3

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesewon00 Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wibaek left a comment

Choose a reason for hiding this comment

Uh oh!

leesewon00 commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wibaek commented Nov 26, 2024

Uh oh!

leesewon00 commented Nov 27, 2024

Uh oh!

nayonsoso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesewon00 Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nayonsoso Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesewon00 commented Nov 12, 2024 •

edited

Loading

wibaek commented Nov 12, 2024 •

edited

Loading

leesewon00 Nov 18, 2024 •

edited

Loading

leesewon00 commented Nov 18, 2024 •

edited

Loading

nayonsoso left a comment •

edited

Loading

leesewon00 Nov 30, 2024 •

edited

Loading

nayonsoso Dec 3, 2024 •

edited

Loading