-
Notifications
You must be signed in to change notification settings - Fork 0
[TSK-56-151] 고전 독서 영역 별 인증 권수 만족 시, 인증 여부 보정 로직 구현 #305
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
Changes from 7 commits
d72466f
e173ea6
346f20c
8bab07a
e789a64
b9d2be6
d7f3cf9
1f11810
86348d7
de12f25
a364f9d
a9f47c9
cf2c485
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package kr.allcll.backend.domain.graduation.check.cert; | ||
|
|
||
| import kr.allcll.backend.domain.graduation.certification.ClassicAltCoursePolicy; | ||
| import kr.allcll.backend.domain.graduation.check.cert.dto.ClassicsCounts; | ||
| import kr.allcll.backend.domain.graduation.check.cert.dto.ClassicsResult; | ||
| import kr.allcll.backend.domain.user.User; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import okhttp3.OkHttpClient; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class GraduationClassicsCertResolver { | ||
|
|
||
| private final GraduationClassicsCertFetcher graduationClassicsCertFetcher; | ||
| private final ClassicAltCoursePolicy classicAltCoursePolicy; | ||
|
|
||
| public ClassicsResult resolve( | ||
| User user, | ||
| OkHttpClient client, | ||
| GraduationCheckCertResult certResult | ||
| ) { | ||
| ClassicsCounts fallbackCounts = ClassicsCounts.fallback(certResult); | ||
|
|
||
| if (isClassicsAlreadyPassed(certResult)) { | ||
| return ClassicsResult.passedWith(fallbackCounts); | ||
| } | ||
|
|
||
| ClassicsResult classicsResult = fetchClassicsResultFromExternal(client, fallbackCounts); | ||
| boolean isSatisfiedByCrawledResult = classicsResult.isSatisfiedByCrawledResult(); | ||
| boolean satisfiedByAltCourse = classicAltCoursePolicy.isSatisfiedByAltCourse(user); | ||
| if (isSatisfiedByCrawledResult || satisfiedByAltCourse) { | ||
| return ClassicsResult.passedWith(classicsResult.counts()); | ||
| } | ||
|
|
||
| return ClassicsResult.failedWith(classicsResult.counts()); | ||
| } | ||
|
|
||
| private ClassicsResult fetchClassicsResultFromExternal(OkHttpClient client, ClassicsCounts fallbackCounts) { | ||
| try { | ||
| ClassicsResult classicsResult = graduationClassicsCertFetcher.fetchClassics(client); | ||
| if (classicsResult == null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앞서 단 코멘트와 동일한 이유로 여기도 null이 없어도 될 것 같군요!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 엇 여기는 외부에서 새로 크롤링한 결과에대한 Null검사라 중복되지 않는 것 같아요!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 다시 확인해보니 null이 반환될일은 없을 것 같아요! |
||
| return ClassicsResult.empty(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 급 우려되는점이 생겼는데요, 요거 기존 DB에 저장된 값이 있더라도 요청이 실패하면 empty값을 반환하게 되는거 같은데 맞을까요? Exception catch가 있지만, null이 반환되는 경우가 정확히 어떤때인지 확실하게 몰라서... 일단 이런 의견을 내봅니다.
그리고 프론트에게 요청이 정상적으로 안되었다는걸 알려서 업데이트가 안되었고 재시도 부탁한다는 문구를 추후 추가해도 좋겠군요.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🫢 그렇네용 꼼꼼한 리뷰 감사합니다! |
||
| return classicsResult.withFallbackCounts(fallbackCounts); | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제가 간단히 이해하기로는
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말씀 주신 것 처럼 다만 위 메서드가 외부 시스템 호출을 포함하고 있어서 예상치 못한 다양한 예외(네트워크, 파싱 등..)가 발생할 수 있다고 생각해, 일단 Exception으로 넓게 잡아 예외처리를 하도록했는데, 이에 대해서는 어떻게 생각하시나요?!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
그렇네요 ㅎ... catch하는 에러 타입을 Exception -> AllcllException으로 수정했습니다! |
||
| log.error("[졸업요건검사] 고전인증 여부를 불러오지 못했습니다.", e); | ||
| return ClassicsResult.failedWith(fallbackCounts); | ||
| } | ||
| } | ||
|
|
||
| private boolean isClassicsAlreadyPassed(GraduationCheckCertResult certResult) { | ||
| if (certResult == null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이미 앞
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앗 중복 검증이 있었군요 😅 |
||
| return false; | ||
| } | ||
| return Boolean.TRUE.equals(certResult.getIsClassicsCertPassed()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package kr.allcll.backend.domain.graduation.check.cert; | ||
|
|
||
| import java.util.List; | ||
| import kr.allcll.backend.domain.graduation.certification.GraduationCertificationAltCoursePolicy; | ||
| import kr.allcll.backend.domain.graduation.check.excel.CompletedCourse; | ||
| import kr.allcll.backend.domain.graduation.department.GraduationDepartmentInfo; | ||
| import kr.allcll.backend.domain.user.User; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import okhttp3.OkHttpClient; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class GraduationCodingCertResolver { | ||
|
|
||
| private final GraduationCodingCertFetcher graduationCodingCertFetcher; | ||
| private final GraduationCertificationAltCoursePolicy codingAltCoursePolicy; | ||
|
|
||
| public boolean resolve( | ||
| User user, | ||
| OkHttpClient client, | ||
| GraduationDepartmentInfo userDept, | ||
| List<CompletedCourse> completedCourses, | ||
| GraduationCheckCertResult certResult | ||
| ) { | ||
| if (isCodingAlreadyPassed(certResult)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (codingAltCoursePolicy.isSatisfiedByAltCourse(user, userDept, completedCourses)) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| return graduationCodingCertFetcher.fetchCodingPass(client); | ||
| } catch (Exception e) { | ||
| log.error("[졸업요건검사] 코딩인증 정보를 불러오지 못했습니다.", e); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private boolean isCodingAlreadyPassed(GraduationCheckCertResult certResult) { | ||
| if (certResult == null) { | ||
| return false; | ||
| } | ||
| return Boolean.TRUE.equals(certResult.getIsCodingCertPassed()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package kr.allcll.backend.domain.graduation.check.cert; | ||
|
|
||
| import kr.allcll.backend.domain.graduation.certification.GraduationCertificationAltCoursePolicy; | ||
| import kr.allcll.backend.domain.graduation.check.excel.CompletedCourse; | ||
| import kr.allcll.backend.domain.graduation.department.GraduationDepartmentInfo; | ||
| import kr.allcll.backend.domain.user.User; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import okhttp3.OkHttpClient; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class GraduationEnglishCertResolver { | ||
|
|
||
| private final GraduationEnglishCertFetcher graduationEnglishCertFetcher; | ||
| private final GraduationCertificationAltCoursePolicy englishAltCoursePolicy; | ||
|
|
||
| public boolean resolve( | ||
| User user, | ||
| OkHttpClient client, | ||
| GraduationDepartmentInfo userDept, | ||
| List<CompletedCourse> completedCourses, | ||
| GraduationCheckCertResult certResult | ||
| ) { | ||
| if (isEnglishAlreadyPassed(certResult)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (englishAltCoursePolicy.isSatisfiedByAltCourse(user, userDept, completedCourses)) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| return graduationEnglishCertFetcher.fetchEnglishPass(client); | ||
| } catch (Exception e) { | ||
| log.error("[졸업요건검사] 영어인증 여부를 불러오지 못했습니다.", e); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private boolean isEnglishAlreadyPassed(GraduationCheckCertResult certResult) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기도 동일한 코멘트 입니당!!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 위 코멘트와 같은 이유로, 검증이 필요한 것 같은데 확인부탁드려요 ...!🤔
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 가 아니라 이건 또 다른 케이스군요! 다만 이렇게 db의 기존 certResult를 가져오고있어 certResult에 대한 null 검증은 필요해보입니다!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 그러네요..! 제가 놓쳤습니다. 처음 검사하는 사용자 판별을 위한 분기문으로 필요한 코드로 보여요! 다만 null 가드를 매번 검사해야 하는 구조는 휴먼 에러가 생길 가능성이 클 것 같아서요. (실제로 그래서 Null Object 패턴을 제안드려 봅니다. GraduationCheckCertResult certResult = graduationCheckCertResultRepository
.findByUserId(user.getId())
.orElseGet(GraduationCheckCertResult::empty);이렇게 하면 하위 resolver들의 null 분기문이 전부 사라지고, "상태"가 타입으로 표현될 것 같아서요. 해윤님은 어떻게 생각하시나요? 직접 적용한번 해보니까 여러 파일에서 변경이 필요해서 변경 사항이 포함된 patch 파일로 추출해봤습니다. graduation-cert-null-object.patch
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
매우 좋은 것 같습니다 👍 단순 null이 아닌 empty라는 상태로 분기처리를 하면 의미 전달이 명확해질 것 같아요!
대박..... 이런 방법이 있다니 신기하네요! 변경 범위가 꽤 크던데 감사합니다 ㅎㅎ😇 |
||
| if (certResult == null) { | ||
| return false; | ||
| } | ||
| return Boolean.TRUE.equals(certResult.getIsEnglishCertPassed()); | ||
| } | ||
| } | ||

Uh oh!
There was an error while loading. Please reload this page.