[TSK-71] 기존 기필 카테고리 대체과목 보정 로직 수정하여 동일과목 학점 미인정 이슈 해결#310
[TSK-71] 기존 기필 카테고리 대체과목 보정 로직 수정하여 동일과목 학점 미인정 이슈 해결#310
Conversation
…uired_courses`) 검증 로직 수정
Test Results188 tests 188 ✅ 26s ⏱️ Results for commit de8ae84. ♻️ This comment has been updated with latest results. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e272f14ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| select e.groupCode from CourseEquivalence e | ||
| where e.curiNo = :curiNo | ||
| """) | ||
| Optional<String> findGroupCodeByCuriNo(String curiNo); |
There was a problem hiding this comment.
findGroupCodeByCuriNo assumes a single row, but course_equivalences is populated from a sheet that is only string-validated (no uniqueness check on curi_no), so duplicate rows for the same course code will make this query return multiple results and throw IncorrectResultSizeDataAccessException at runtime during academic-basic evaluation. This should either be made tolerant (distinct/first) or guarded by validation/constraints so one bad sheet row does not break graduation checks.
Useful? React with 👍 / 👎.
| graduationSheetTable.getString(row, "curi_no"), | ||
| graduationSheetTable.getString(row, "curi_nm"), | ||
| graduationSheetTable.getString(row, "alt_group"), | ||
| graduationSheetTable.getString(row, "group_code"), |
There was a problem hiding this comment.
This sync now persists group_code and the new findRequiredCourseInGroup logic depends on it, but required-courses validation still does not require that column/value; if the sheet is not updated (or cells are blank), rows are saved with null group codes and group-based matching always misses, causing equivalent/replacement academic-basic courses to be incorrectly treated as not required instead of failing fast during sync.
Useful? React with 👍 / 👎.
…urseReplacement`기반 클래스 -> `CourseEquivalence`기반 클래스)
… 판단 책임을 `CourseEquivalence`로 위임
…ement` 관련 클래스 제거
|
설계 자체가 바뀌는 PR이라, 아래 내용을 바탕으로 설계 방향성에 대한 논의를 먼저 진행하고 진행해보면 좋겠습니다! 설계 변경 요약
이점
예상되는 한계
|
|
먼저 설계 방향성에 대한 의견을 남기자면, 현재 방향이 훨씬 좋다고 생각합니다!!! |
| select e.curiNo from CourseEquivalence e | ||
| join CourseEquivalence e2 on e.groupCode = e2.groupCode | ||
| where e2.curiNo in :curiNos |
There was a problem hiding this comment.
요거 중복 값이 포함될 여지가 있어보이는데요, 어떻게 생각하시나요?!
현재 유일하게 사용하는 메서드에서 Set으로 받아와서 중복 값이 사라지긴 하지만, 해당 쿼리를 buildEarnedCuriNos메서드 말고 다른 곳에서도 사용하게 될 수도 있잖아요. 그때 증복값이 있을 것이라고 예상하기 쉽지 않을 것 같아요.
중복 값을 포함한 이유가 따로 있다면 말씀해주세요~!
There was a problem hiding this comment.
엇 그러네요! 꼼꼼 리뷰 감사드립니다
쿼리에 distinct를 추가해서 중복을 제거하는 방식은 어떠한가요?
아니면 다른 좋은 방식 있다면 다른분들도 언급 주시면 감사하겠습니당
| return requiredCourse.getDeptNm().equals(departmentName); | ||
| } | ||
|
|
||
| private RequiredCourseResponse mapToCurrentCourse(RequiredCourse requiredCourse) { |
There was a problem hiding this comment.
요 친구의 역할이 무엇인지 설명해주실 수 있을까요?
mapToCurrentCourse라고 해서, DEPRECATED된 애를 group code로 course_equivalences에서 받아오는 건줄 알았는데, requiredCourse에서 같은 그룹을 하나 찾아서 넣어주는 애네요?!
RequiredCourse에서 찾는 이유가 무엇인가요?? 이 메서드가 loadRequiredCourses의 메서드에서 호출이 되는데,,,,, requiredCourse는 DEPRECATED된 상태만 저장할 뿐 그 대체 과목을 저장하지는 않지 않나요?
(메서드명과) 흐름이 잘 이해가 가지 않아 여쭤봅니다.
There was a problem hiding this comment.
우선, 현 로직은 [사용자의 카테고리 별 졸업인증 기준 데이터 조회 API]에 사용되는 점 먼저 언급드립니다!
이를 기반으로 [사용자 미이수 과목 추천 기능]을 제공해주게 됩니다.
따라서, 사용자의 졸업인증 기준 데이터 과목들의 최신 과목 데이터를 얻어오는 것이 현재 언급주신 메서드의 역할이라고 생각하시면 됩니다!
mapToCurrentCourse라고 해서, DEPRECATED된 애를 group code로 course_equivalences에서 받아오는 건줄 알았는데, requiredCourse에서 같은 그룹을 하나 찾아서 넣어주는 애네요?! RequiredCourse에서 찾는 이유가 무엇인가요??
넵 맞습니다!
초기에는 저도 보예님께서 말씀하신 방식대로 구현하고자 했습니다!
-> 즉, course_equivalences에는 최신순 혹은 그 역순으로 시간별 과목 히스토리로 관리하고, 같은group_code 과목 중 맨 앞 혹은 맨 뒤 아이를 꺼내 최신 과목이라고 판단하는 방식이었습니다.
하지만 문제가 존재합니다!
위 방식은 데이터 적재 순서에 의존하고, 동일과목/대체과목 이력이 항상 일관된 순서로 관리된다고 보장하기 어렵습니다.
course_equivalences의 데이터는 학사정보시스템 동일과목 조회 탭에서 제공하는 csv 파일을 이용해 적재해보았는데요!
동일과목에 대해 확인해본 결과, 과목 순서가 변경이력대로 조회되고 있지 않음을 확인했습니다.
따라서, 기존 논의대로 course_equivalences에서 최신 과목 여부에 대한 판단을 하게 된다면 문제가 발생할 여지가 있다고 판단했습니다.
이에 따라, 최신 과목 판단을 어떻게 할까 고민해보았습니다.
[변경된 최신 과목 판단 방식]
최신 과목 판단 여부는 두 가지 케이스에 따라 달라집니다.
Case1) 현재 기준 과목 데이터가 DPRECATED 되지 않았을 경우
DPRECATED 상태는 requiredCourses가 관리합니다.
현재 기준 과목 데이터가 DPRECATED되지 않은 과목일 경우 -> 최신 과목입니다.
따라서, 대체 과목 매핑 과정이 이루어지지 않고 해당 과목을 그대로 사용하게 됩니다.
mapToCurrentCourse
if (isNotDeprecated(requiredCourse.getCuriNo())) {
return RequiredCourseResponse.of(requiredCourse.getCuriNo(), requiredCourse.getCuriNm());
}
이 부분입니다.
Case2) 현재 기준 과목 데이터가 DPRECATED 되었을 경우
DPRECATED 상태는 requiredCourses가 관리합니다.
현재 기준 과목 데이터가 DPRECATED 되었을 경우 -> 이미 대체과목이 존재하는 과목이겠죠?
따라서, 해당 과목의 대체과목 중 최신 과목을 찾아 반환해야 합니다.
앞서 언급드렸듯이, course_equivalences의 데이터들에서는 최신 과목을 판단할 수 없으니,
requiredCourses에서 대체 과목의 최신 버전을 찾아야 합니다.
찾는 방식은 다음과 같습니다.
현재 DPRECATED된 과목과 동일한 그룹 코드를 가진 과목을 조회합니다.
return requiredCourseRepository.findCurrentCourseByGroupCode(requiredCourse.getGroupCode(), DEPRECATED)
.stream().findFirst()
.map(currentCourse -> RequiredCourseResponse.of(currentCourse.getCuriNo(), currentCourse.getCuriNm()))
.orElseGet(() -> {
log.error(
"[졸업요건] DEPRECATED된 과목의 현재 과목 조회에 실패했습니다. groupCode={}", requiredCourse.getGroupCode()
);
return RequiredCourseResponse.of(requiredCourse.getCuriNo(), requiredCourse.getCuriNm());
});
-> 이 방식이 최신 대체 과목이라고 판단한 이유는 다음과 같습니다.
requiredCourses가 DPRECATED 상태를 관리하고 있고, 학번 별로 필요한 모든 과목 데이터를 적재했기 때문입니다.
그리고 동일한 카테고리에 대해 적재를 진행했습니다.
따라서, DPRECATED된 과목 데이터는 동일한 그룹 코드 내에 DPRECATED되지 않은 과목 데이터가 최소 하나는 존재할 것이라고 판단했습니다.
혹시 이해가 가지 않거나 엣지 케이스가 발생할 여지가 있다고 판단되시면 코멘트 마구마구 부탁드립니다!
| @Query(""" | ||
| select r from RequiredCourse r | ||
| where r.groupCode = :groupCode | ||
| and r.curiNo != :deprecatedCuriNo | ||
| """) |
There was a problem hiding this comment.
동일과목/대체과목 이력이 항상 일관된 순서로 관리된다고 보장하기 어려워
라고 하셨는데, 요 메서드를 쓰는 친구는 findFirst를 쓰는 것 같아요! (위에 쓴 코멘트와 같은 맥락입니다.)
추가 설명 부탁드려용~~❤️❤️
There was a problem hiding this comment.
맞습니다!
findFirst()는 일종의 안전장치라고 보시면 됩니다!!
호오오오옥시나,, 하는 마음에 걸어두었슴니다
위 코멘트 읽고 읽어주세용
| requiredCourseRepository.findRequiredCoursesByGroupCode( | ||
| List.of(WILD_CARD_DEPT_NM, departmentName), | ||
| admissionYear, | ||
| categoryType, | ||
| groupCode | ||
| ); |
There was a problem hiding this comment.
저희 요거 WILD_CARD인데 FALSE면 제외해줘야하지 않나요...???? wild card면 대상인 것으로 조회하는 것 같은데 맞을까요?!
There was a problem hiding this comment.
넵! 맞습니다!
public boolean findRequiredCourseInGroup(
String departmentName,
Integer admissionYear,
CategoryType categoryType,
String groupCode
) {
List<RequiredCourse> requiredCourseCandidatesWithWildCard =
requiredCourseRepository.findRequiredCoursesByGroupCode(
List.of(WILD_CARD_DEPT_NM, departmentName),
admissionYear,
categoryType,
groupCode
);
List<RequiredCourse> requiredCoursesWithStatus
= getDepartmentRequiredCourses(requiredCourseCandidatesWithWildCard, departmentName);
return requiredCoursesWithStatus.stream()
.anyMatch(RequiredCourse::getRequired);
}
우선, 결론부터 말씀드리면 와일드카드 및 사용자 학과로 기준 데이터를 모두 조회한 뒤,
DB 적재 정책을 적용한 메서드인 getDepartmentRequiredCourses를 거쳐
최종적으로 return 부에서 false를 필터링 해주게 됩니다!
위 메서드에서 해당 쿼리가 실행되는데, 쿼리에서 제외하지 않은 이유는 RequiredCourses에 필요한 DB 적재 정책을 적용하기 위함입니다!
해당 DB 적재 정책은 이미 구현되어 있던 getDepartmentRequiredCourses 요놈을 재사용했습니다! (기존 로직에서 사용 중이었던 아이입니당)
예전에 Repository와 Resolver의 계층 분리를 통해, 졸업 인증 기준 데이터 조회 시 안정성을 확보하자는 논의가 이루어진 바 있습니다! 졸업인증 기준 데이터는 사람이 직접 적재했다보니 비즈니스 로직에서 Repository 계층을 바로 사용하게 되면 적재 정책이나 예외 케이스가 누락될 위험이 존재합니다!
따라서, 와일드카드 학과 데이터와 실제 학과 데이터를 함께 조회한 뒤, getDepartmentRequiredCourses를 통해 기존 적재 정책을 반영한 최종 결과를 만든 후 판단하도록 했습니당
혹시 추가적으로 왜 와일드카드와 학과를 모두 조회하고 getDepartmentRequiredCourses메서드를 실행하는지 더 궁금하시다면 코멘트 남겨주세요! 예시 들어서 더 설명드리겠슴니다!
전과 보정이라는 것이 무슨 맥락인지 모르겠습니다!!! 조금 더 자세히 설명 부탁드려도 될까요!!! 🙇🏻♂️ 감사합니다 🙇🏻♂️
로직이 변경된 api는 보예님께서 언급해주신 [카테고리별 기준 데이터 조회 API] 이외에도, |
아 죄송해요 제가 너무 컨텍스트를 생략하고 말했네요. 저희 전과생들이 엑셀 표 업데이트가 늦게 되어서 생겼던 문제 말하는 거였어요!!! 전선, 전필 등이 옮긴 과 기준으로 계산되지 않잖아요, 이 보정을 이야기한 것이었습니다!! |
3 participants
이슈
기존 기필 카테고리 졸업요건 검사 시, 동일과목 커버리지가 낮아 미인정 처리되는 문제가 있었습니다.
[기존 로직]
이수 과목이
required_courses에 존재하는지 확인하고, 없으면course_replacement기반으로 대체된 최신 과목을 조회하여 과목명 일치 여부를 판단했습니다.제가 판단한 문제 상황을 예시로 들어보겠습니다.
case1) 사용자가 지정 과목보다 이전 버전의 과목을 들었을 경우
case2) 수강편람 내에 사용자가 이수한 과거 과목에 대한 정보가 없고, 동일과목 목록에는 정의되어 있는 경우
-> 이 경우는 A'라는 과목이 현재 A라는 과목으로 대체된 과거 과목인데, 수강편람에서 누락된 게 아닐까 추측해봅니다.
혹은 대체된 년도의 수강편람에는 존재하지만, 최신 수강편람에서 누락된 경우로 보입니다.
이런 누락된 케이스가 많이 존재할 거라고 가정하고 작업을 진행해보았습니다.
결과적으로, 기존 로직은 위 두가지 케이스를 커버하지 못했습니다.
원인
기존
course_replacements는 학번 단위로 대체과목을 관리하는 구조였습니다.이 구조는 다음 문제가 있었습니다.
required_courses에 적재된 과목보다 더 이전 버전의 과목을 이수한 경우 커버하지 못합니다.즉, 학번 단위 대체과목 관리 방식 자체가 동일과목/대체과목 판별의 커버리지를 떨어뜨리는 원인이라고 판단했습니다!
작업 내용
1. 졸업요건검사 API - 동일 및 대체과목 통합 관리 엔티티(
course_equivalences) 추가[기존]
대체과목 관리 테이블(
course_replacements)에는 학번 별로 대체과목(A→B)만 관리되었습니다.그러나 동일과목도 함께 관리되어야 합니다.
[개선]
기존
course_replacements는 학번별로 대체과목을 관리했기 때문에, 특정 학번의 학생이 이전 버전의 과목을 이수하는 엣지 케이스에서 커버리지가 낮았습니다. 또한 대체과목이 추가될 때마다 학번마다 모두 추가해줘야 하는 운영 리소스 문제도 존재했습니다.따라서 학번 단위가 아닌, 과목 자체를 기준으로 동일 및 대체과목 이력을 통합 관리하는
course_equivalences엔티티를 새로 추가했습니다.course_equivalences는 모든 학번 및 학과에 공통으로 적용되는 대상으로 관리합니다.[동일과목 및 대체과목을 통합하여 관리하는 방식을 채택한 이유]
동일과목이 대체과목의 상위 개념이라고 판단했습니다.
즉, 동일과목 카테고리 안에 대체과목 카테고리가 포함되는 구조입니다.
이렇게 판단한 이유는 다음과 같습니다.
따라서 동일과목으로 통합 관리하는 것이 커버리지와 운영 효율 모두에서 유리하다고 판단했습니다.
2. 졸업요건검사 API -
group_code기반으로 동일 및 대체과목 판단하도록 방식 변경[기존] 에는 다음과 같이 deprecated된 대체과목의 이수 여부를 판단하도록 했습니다.
동일 및 대체과목 통합 관리 엔티티(
course_equivalences)를 추가함에 따라,동일 및 대체과목 판단 방식을 변경했습니다.
[개선]
required_courses및course_equivalence에group_code칼럼을 추가했습니다.group_code로 묶었습니다.required_courses에 존재하는 과목들과course_equivalence에 존재하는 과목들도 같은group_code로 매핑되어 있습니다.개선된 동일 및 대체과목 판단 -> 동일성 체크 플로우는 다음과 같습니다.
이를 통해 이수한 과목이 구버전이더라도,
group_code가 일치하면 이수한 것으로 판단합니다.3. 카테고리 별 졸업요건 기준 데이터 조회 API
이 API는 사용자의 입학년도/학과/이수구분별 졸업 기준 데이터를 반환하며, 미이수 과목도 함께 내려줍니다.
course_replacements기반으로 최신 과목으로 보정하는 곳은 두 곳이 존재했습니다.이를
course_equivalences기반으로 보정하도록 책임을 위임하는 작업을 진행했습니다.3-1. deprecated 된 과목의 최신 과목 반환 로직 변경
[기존]
카테고리별 졸업요건 기준 데이터를 구성할 때, deprecated된 지정 과목은
course_replacements기반으로 최신 과목으로 보정하여 반환했습니다.즉, 기준 데이터에 과거 과목이나 deprecated 과목이 포함되어 있더라도, 응답을 내려주기 전에
course_replacements에서 학번 기준 대체 관계를 조회하여 현재 기준의 과목명/학수번호로 치환하는 방식이었습니다.흐름으로 보면 아래와 같습니다.
이 방식은 학번 단위 대체과목 데이터에 의존하기 때문에, 변경 이력이 길거나 동일과목만 존재하는 케이스에서는 최신 과목 보정이 누락될 수 있었습니다.
[변경]
deprecated 과목의 최신 과목 반환 로직을
course_equivalences와group_code기준으로 변경했습니다.이제 deprecated된 과목을 직접 대체과목 테이블에서 최신 과목으로 치환하는 것이 아니라,
해당 과목이 속한
group_code를 기준으로 같은 그룹에 속한 현재 과목을 찾아 응답에 내려주도록 변경했습니다.변경된 흐름은 아래와 같습니다.
즉, 기존처럼 학번별 대체과목 매핑 방식이 아니라, 동일/대체과목 그룹 안에서 현재 기준 과목을 찾아 내려주는 방식으로 변경했습니다.
이를 통해 deprecated 과목 응답 보정 로직이 특정 학번의 대체과목 데이터에 종속되지 않고, 동일과목/대체과목 전체 이력을 기준으로 동작하도록 개선했습니다.
3-2. 미이수 과목 판단 로직 변경
[기존]
기존에는 사용자의 이수 과목으로부터 인정 가능한 과목 목록을 만들 때,
course_replacements기반으로 최신 과목을 보정한 뒤 미이수 여부를 판단했습니다.즉, 사용자가 실제로 이수한 과목들의 학수번호만 바로 비교하는 것이 아니라,
이수 과목명들을 기준으로
course_replacements에서 현재 과목의 대체과목들을 추가 조회하여, 그 결과를 포함한 뒤 미이수 과목을 걸러내는 방식이었습니다.흐름으로 보면 아래와 같습니다.
이 방식 역시 학번 단위 대체과목 정보에 의존하기 때문에, 미이수 판단이 부정확할 수 있었습니다.
[변경]
미이수 과목 판단도
course_equivalences기반으로 내려주도록 변경했습니다.사용자의 이수 과목 학수번호 집합을 먼저 만든 뒤,
course_equivalences에서 같은group_code에 속한 과목들의 학수번호를 모두 조회하여 인정 가능한 과목들로 포함해 미이수 과목 판단을 하도록 변경했습니다.변경된 흐름은 아래와 같습니다.
즉, 기존에는 이수 과목명을 기반으로 최신 과목만을 포함시켰다면,
이수한 과목이 속한 동일 과목 전체를 인정 가능한 과목으로 확장하는 방식으로 변경했습니다.
이를 통해 사용자가 어떤 버전의 과목을 이수했는지와 관계없이, 같은
group_code에 포함된 과목이라면 모두 동일/대체과목으로 인정할 수 있습니다.검증
case1) 지정 과목보다 더 이전 버전의 과목을 이수한 경우
해당 과목이 동일한 group_code에 속해 있으면 정상적으로 이수로 인정되는지 검증했습니다.
-> 정상적으로 학점 인정이 되는 것을 확인했습니다.
case2) 수강편람에는 없지만 동일과목 목록에는 존재하는 과거 과목을 이수한 경우
course_equivalences에 동일과목/대체과목으로 등록되어 있고 동일한group_code에 속해있다면 정상적으로 이수로 인정되는지 검증했습니다.-> 기필 영역에서 해당 과목이 인정되었고, 미이수 과목 추천 목록에도 포함되지 않은 것을 확인했습니다.
case3) 미이수 추천 과목 정상 작동하는지
미이수 과목이 정상적으로 필터링되고 추천 과목 목록이 기대한 형태로 내려오는지 검증했습니다.
-> 미이수 과목 추천에서 과거 과목 정보가 아닌, 최신 과목 정보로 반환되는 것을 확인했습니다.
고민 지점과 리뷰 포인트
제가 판단한 방식이 적절한지 의견 부탁드립니다!!
TODO (현재 구현된 방향으로 머지가 된다면)