Skip to content
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

feat(apply): link the my application page with the example judgment api #660

Merged
merged 22 commits into from
Oct 14, 2022

Conversation

KangYunHo1221
Copy link
Contributor

@KangYunHo1221 KangYunHo1221 commented Oct 11, 2022

Resolves #662

현재상태 : 백엔드 로컬서버와 연동하여 작동테스트 마친 상태입니다.

😀 해결하려는 문제가 무엇인가요?

  • 테스트 실행시간이 초과됐을 때 해당하는 UI를 보여주지 않습니다
  • 백엔드와 프론트엔드에서 사용하는 도메인 용어가 일치하지 않습니다(test result, judgment result)
  • 자동채점 기능의 예외발생 케이스에 대한 에러메세지가 보여지지 않습니다.
  • API 명세를 일치
  • 수정하기 버튼이 수정하기 페이지로 보내는 작동을 하지 않는 오류가 있습니다.
  • 새로고침 버튼을 눌렀을 시 제대로 눌렸는지 알려주지 않음

🤔 어떻게 해결했나요?

  • 테스트 실행시간이 초과했을 때 예기치 못한 오류로 실행 시간을 초과하였습니다. 라는 메세지를 붉은 글씨로 보여줍니다.
    • getTime을 기준으로 5분을 계산하여 시간이 초과했을시 해당하는 메세지를 보여줍니다.
    • msw와 mockData에도 해당 로직을 적용하였습니다.
  • 테스트 실행시간이 초과했을 때 새로고침 버튼을 보여주지 않습니다.
  • test-result 를 judgment-result로 변경하였습니다.
  • 자동채점 실행시 아래의 케이스에 대해서 alert를 보여주고, 해당 미션에 대한 정보를 다시 불러옵니다.
    • 미션이 존재하지 않는 경우
    • 미션 제출 기한이 아닌 경우
    • 제출한 과제가 없는 경우
    • 자동채점항목이 존재하지 않는 경우
    • 깃허브 API 요청에 실패한 경우
    • 과제가 완료되지 않은 상태(STARTED) 인 상황에서 5분 내로 요청을 보낸 경우
  • 변경된 API 명세를 적용하였습니다.
  • 수정하기 버튼이 수정하기 페이지로 이동하도록 변경하였습니다(화살표함수의 오타를 수정했습니다.)
  • 새로고침 버튼을 눌렀을 시 alert를 통해 사용자에게 알려줍니다.

👉 어떤 부분에 집중하여 리뷰해야 할까요?

  • 에러핸들링이 부족한 부분이 있다면 알려주세요
  • 변경된 judgmentResult에 대한 구조가 마음에 들지 않는다면 알려주세요
  • 에러메세지 문구(예기치 못한 오류로 실행 시간을 초과하였습니다., 예기치 못한 오류로 실행에 실패였습니다.) 에 대한 아이디어가 있다면 알려주세요
  • 새로고침시 alert를 띄워주는게 맞는 방식인지 모르겠습니다. 의견이나 스타일 원하는 코드가 있으시면 알려주세요!

참고 자료

  • 여러분들의 오프라인 코드리뷰

RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

- SUCCESS -> SUCCESSED
- FAIL -> FAILED
- judgment->judgments
- axios post config 위치 변경

Co-authored-by: Jungmin Hwang <[email protected]>
@KangYunHo1221 KangYunHo1221 force-pushed the feature/implement-api-connection branch from c9ce9ba to aadd98c Compare October 11, 2022 11:56
Comment on lines 66 to 67
(judgment?.status === JUDGMENT_STATUS.STARTED &&
!isJudgmentTimedOut(judgment.startedDateTime)) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분이 조금 이해하기 어려운데요,
평가중이면서 5분이 지난 경우에는 예제테스트 실행 버튼을 보여주어야 하기 때문에 이렇게 구현하였습니다.
평가중이면서 5분이 안지나면 예제테스트 실행버튼을 비활성 하여야 합니다.

Copy link
Contributor

@soyi47 soyi47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비녀, 자스민~ 고생 많으십니다!

잘 훑어봤고, 궁금한 점과 함께 소소하게 코멘트 남겼습니다.
확인 부탁드려요~

@@ -147,7 +150,7 @@ export const missionsDummy = {
judgment: {
pullRequestUrl: "https://github.com/woowacourse/service-apply/pull/367",
commitHash: "642951e1324eaf66914bd53df339d94cad5667e3",
status: "SUCCESS" as TestStatus,
status: "SUCCEEDED" as TestStatus,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 여기 타입 TestStatusJudgmentStatus로 바뀌는 걸까요? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test result 만 일단 고려한것이긴 한데 통일하는게 좋아보이네요


type JudgmentResultType = { text: string; type: "default" | "fail" | "pass" | "pending" };

const formatJudgmentResult = (judgment: Mission["judgment"]): JudgmentResultType => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: util 함수 파일인데 확장자가 tsx네요. ts로 변경할까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에 hook으로 작성했다가 변경해서 바뀌었나보네요.
저희 컨벤션이 ts와 tsx를 구분하기로 했으므로 변경해야 할 것 같습니다!

Comment on lines 24 to 26
case JUDGMENT_STATUS.FAILED:
case JUDGMENT_STATUS.CANCELLED:
return { text: "예기치 못한 오류로 인하여 실행에 실패하였습니다.", type: "fail" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a: 헷갈려서 질문합니다. Failed와 Canceled의 차이가 뭔가요?
judgment 데이터를 보니 message가 있던데, dummy에서는 빌드 실패, 성공 여부나 빈 문자열이더라고요. 이 아이는 빌드 관련 결과를 안내하는 메시지 인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저희도 헷갈려서 백엔드분들께 여쭤봤더니, 자동채점에 대해서

  • Failed = 빌드 실패 등 실행에 실패한 경우
  • Canceled = 채점 사이트에서 실행 취소를 당한 경우

라고 합니다.

@@ -44,7 +44,7 @@ const MyApplicationFormItem = ({ recruitment, submitted }: MyApplicationFormItem
const isButtonDisabled = isApplicationDisabled(submitted, recruitment.recruitable);
const buttonLabel = applicationLabel(submitted, recruitment.recruitable);

const routeToApplicationForm = (recruitment: Recruitment) => () => {
const routeToApplicationForm = (recruitment: Recruitment) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a: 👏👏👏

Copy link
Contributor

@woowapark woowapark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트 확인 부탁드립니다~!
로컬 테스트는 별도 브랜치에서 따로 진행해보신 걸까요?

const errorMessage = error.response?.data.message;
alert(errorMessage);

const response = await fetchMyMissionJudgment({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 여기에서 다시 에러가 발생하면 어떻게 되나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리팩토링 하다보니, handleJudgeError를 호출하는 경우 이미 response가 없기 때문에 굳이 fetchMyMissionJudgment해 올 필요가 없을 것 같습니다.
기존에 코드를 짤 때 canceled나 failed와 handleJudgeError를 혼용 된 것 같습니다.

만약 '실행하기' 가 실패한다면 이전 상태를 그대로 보여주고 '실행하기가 안됐다는 에러메세지만 보여주면 될 것 같습니다.
fetchMyMissionJudgment를 통째로 제거하는 게 맞는 것 같아요.


const response = await fetchMyMissionJudgment({
recruitmentId: Number(recruitmentId),
missionId: Number(missionId),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c: missionId는 핸들러 함수의 파라미터이고, recruitementId는 컴포넌트의 props를 그대로 사용하고 있는데요. 혼동이 생길 수 있지 않을까요?

@@ -1,5 +1,6 @@
export const JUDGMENT_STATUS = {
STARTED: "STARTED",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c: CSS 클래스는 started 대신 pending이라는 용어를 사용하는데요. 동일하게 양쪽 다 started로 맞추면 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

용어통일이 되고 있지 않았네요! pending으로 된 용어들 모두 started로 변경해주었습니다.

@@ -125,7 +128,7 @@ export const missionsDummy = {
judgment: {
pullRequestUrl: "https://github.com/woowacourse/service-apply/pull/367",
commitHash: "642951e1324eaf66914bd53df339d94cad5667e3",
status: "STARTED" as TestStatus,
status: "STARTED" as JudgmentStatus,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 상수화 이미 해둔 게 있는데, "STARTED"같은 문자열 대신JUDGMENT_STATUS 상수를 쓰는 게 어떨까요? 값을 변경하게 되면 하나하나 따로 수정해주어야 하는 불편함이 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JUDGMENT_STATUS에서 사용하도록 변경하였고,
MISSION_STATUS도 동일하게 적용하였습니다.

import { JUDGMENT_STATUS } from "../../constants/judgment";
import { isJudgmentTimedOut } from "../validation/judgmentTime";

type JudgmentResultType = { text: string; type: "default" | "fail" | "pass" | "pending" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 'pending' 은 서버에서 내려주는 값이랑 무관하게 따로 선언해서 사용하는 값이죠?-? 위의 css 코멘트와 같이 통일할 수 있으면 'started'로 맞추면 어떨까 해서요 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 코멘트 와 동일하게 pending 관련 용어를 모두 started로 도메인용어 통일하였습니다.

@@ -51,6 +61,7 @@ const RefreshButton = ({ recruitmentId, missionItem, setMission }: RefreshButton
variant={BUTTON_VARIANT.CONTAINED}
cancel={false}
onClick={() => {
alert("새로고침 되었습니다");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 아직 새로고침 전이지 않나요?-?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refresh data를 fetch 이후 alert('새로고침 되었습니다')를 시행하도록 변경하였습니다.

return { text: `${passCount} / ${totalCount}`, type: isPass ? "pass" : "fail" };
case JUDGMENT_STATUS.FAILED:
case JUDGMENT_STATUS.CANCELLED:
return { text: "예기치 못한 오류로 인하여 실행에 실패하였습니다.", type: "fail" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 케이스가 다른데 'type'이 같아 혼란스러울 수 있을 것 같은데요. 텍스트 스타일은 같더라도 'error'로 타입을 따로 분류하면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그럴 수 있겠네요.
FAILED, CANCELLED, TIMEOUTTED 경우에도 error로 처리하도록 했습니다!

case JUDGMENT_STATUS.STARTED:
return { text: "테스트 중", type: "pending" };
case JUDGMENT_STATUS.SUCCEEDED:
const isPass = passCount / totalCount === 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r:

  • '전체 케이스 통과'라는 의미가 좀 더 드러나지 않아도 될까요?
  • 결과가 같긴 하지만 passCount === totalCount로 판단해도 될 것 같은데 어떠신가요? '통과한 개수가 전체 개수와 같다'는 것이니까요

Copy link
Contributor Author

@KangYunHo1221 KangYunHo1221 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isPass = passCount / totalCount === 1;
const isPass = passCount === totalCount;

으로 변경하였습니다.

전체 케이스 통과 라는 뜻이 드러나도록 isPass -> isPassAllCases로 변경하였습니다.

@@ -0,0 +1,7 @@
import { ISO8601DateString } from "../../../types/domains/common";

export const isJudgmentTimedOut = (time: ISO8601DateString) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: Judgment는 도메인을 다루는 내용이라, utils 하위에 있기는 어색하지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

judgmentButon, refreshButton, judgmentResult 세군데서 사용하고 있습니다.
custom hook으로 빼기에는 hook을 사용하지도 않아서 안 맞는 것 같고
도메인 별 함수를 따로 정리하지 않고 있기 때문에 utils에 넣는게 맞다고 판단했습니다.

혹시 어디로 옮기면 좋을 것 같은지 의견 있으시면 알려주시면 감사하겠습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흠...그러네요 지금 디렉토리 구조에서는 애매하군요.
타입 외에 domains/ 디렉토리가 필요할 수도 있겠네요 🤔
현재 상태에서는 이대로 두고 다시 고민해보시죠!

export const isJudgmentTimedOut = (time: ISO8601DateString) => {
const startedDateTime = new Date(time);
const now = new Date();
return now.getTime() - startedDateTime.getTime() > 300000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r: 5분이라는 게 더 명확하게 읽히게 상수화 하면 어떨까요?

Copy link
Contributor Author

@KangYunHo1221 KangYunHo1221 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return now.getTime() - startedDateTime.getTime() > 300000;
const timeoutMinute = 5;
const timeoutMillisecond = timeoutMinute * 60 * 1000;
return now.getTime() - startedDateTime.getTime() > timeoutMillisecond;

로 변경하였습니다.

@KangYunHo1221
Copy link
Contributor Author

KangYunHo1221 commented Oct 12, 2022

✅ 추가 변경사항

  • 예제테스트 PR 주소가 사용자에게 헷갈리는 ux를 제공하고 제출물 수정시 변경되는 오류가 있어 삭제했습니다.
    • pr주소를 missionDetail에서 삭제하였고, 해당 컴포넌트도 프로젝트에서 제거했습니다.
  • 성공, 실패, 실행중 여부와 관계없이 실행하기 버튼이 5분 지난 테스트를 '시간초과'로 보여주는 오류를 수정하였습니다.
    • 실행 중이면서 시간초과인 경우에만 시간초과를 보여줍니다.
    • 성공, 실패, 캔슬이면서 시간초과인 경우 성공, 실패, 캔슬 정보를 보여줍니다.
    • 해당 데이터를 더미에 추가하였고 잘못된 설명을 수정하였습니다.
  • 불필요한 오류처리 로직 수정
    • '실행하기' fetch 실패시 새 데이터를 불러와서 missionItem을 refetch하지 않고 이전 예제테스트 데이터를 보여줍니다.
  • 새로고침 로직 수정
    • '새로고침' 요청이 실제로 성공한 이후에 새로고침 메세지를 alert로 보여줍니다.
  • 리팩토링
    • dummy의 status가 문자열이 아닌 상수를 사용하도록 변경하였습니다.
    • 도메인 용어 일치와 가독성을 위한 변수명 변경이 있습니다.

Copy link
Contributor

@woowapark woowapark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가 수정 사항까지 확인하였습니다~!
고생 많으셨어요! 👏👏👏👏👏

Copy link
Contributor

@soyi47 soyi47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경 확인했습니다. 고생하셨어요! 👍👍

Copy link
Contributor

@woowahan-pjs woowahan-pjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다. 👍

@woowahan-pjs woowahan-pjs changed the title feat: implement api connection feat(apply): link the my application page with the example judgment api Oct 14, 2022
@woowahan-pjs woowahan-pjs merged commit 82d9d9e into develop Oct 14, 2022
@woowahan-pjs woowahan-pjs deleted the feature/implement-api-connection branch October 14, 2022 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants