feat: 칸반보드 상세 페이지에서 지원 상태 확인 및 지원 상태 변경 가능하게 구현 #208

smb0123 · 2024-09-16T05:41:38Z

주요 변경사항

지원자의 지원 상태를 변경하는 api 함수 구현 (patchApplicantState)
특정한 지원자의 상태를 가져오는 api 함수 구현 (getApplicantState)
관리자와 회장단에게만 지원자의 상태를 변경할 수 있는 버튼을 보이게 함.
지원자의 지원 상태를 학과 옆에 표시

리뷰어에게...

문제 상황

회장단과 관리자만 지원자의 지원 상태 변경이 가능

해결 방안

회장단과 관리자에게만 버튼이 보이도록 한다.
모든 사람에게 버튼이 보이도록 하고 권한이 없는 사람이 버튼을 클릭 시 alert를 띄운다.

저는 1번이 맞다고 생각하여 1번으로 구현하였는데 다른 분들의 의견도 궁금합니다 !

관련 이슈

closes #199

관리자만 지원 상태가 변경 가능하게 버튼 구현

loopy-lim

추석은 잘 보내시고 있으신가요? 추석때도 코딩을 놓지 않는다니 역시 에코노이군요.
한마디 얹어 보았습니다... 늘 아시듯이 변경을... 아닙니다.

이번꺼는 변경해도 좋을 듯 해보이는 부분이 있어 다시 봐주시면 감사하겠습니다.

loopy-lim · 2024-09-16T11:55:06Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+
+  const { data: initialState, isLoading } = useQuery(
+    ["applicantState", applicantId],
+    () => getApplicantState(navbarId, applicantId as string, generation)


런타임에서도 동작을 보장하고 확보하고 싶다면 다음과 같은 형태도 나쁘지 않아보입니다.

Suggested change

() => getApplicantState(navbarId, applicantId as string, generation)

() => getApplicantState(navbarId, `${applicantId}`, generation)

저도 타입 단언말고 다른 방법을 사용하고 싶었는데 템플릿 리터럴 되게 좋은 방법이네요.
감사합니다 !

loopy-lim · 2024-09-16T11:56:41Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  const handleFailedButtonClick = () => {
+    mutate("non-pass");
+  };
+
+  const handlePassedButtonClick = () => {
+    mutate("pass");
+  };


handle도 좋은 prefix이기도 하나, 이 프로젝트에서는 on을 prefix로 사용하고 있기 때문에 맞추면 더 이해하기 좋은 코드가 되지 않을까 싶습니다.

별 생각 없이 제가 평소에 코드를 작성하던 방식으로 해버렸네요....
on으로 수정하겠습니다 !

loopy-lim · 2024-09-16T11:57:41Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  useEffect(() => {
+    if (initialState) {
+      setPassState(initialState);
+    }
+  }, [initialState]);


혹시 어떤 일을 하는 코드이지 설명 부탁해도 될까요?

페이지를 들어왔을 때 useQuery가 작동하고 initialState가 undefined 였다가 api 요청이 끝나면 지원 상태를 받아와지기 때문에 useEffect와 setPassState를 이용해서 현재 지원 상태를 표시해주었습니다.

const [passState, setPassState] = useState<ApplicantPassState | undefined>(initialState);

위와 같이 작성해도 되지만 이렇게 해버리면 KanbanCardApplicantStatusLabel 컴포넌트에 대해서도 undefined 처리를 해줘야하기 때문에 현재 코드를 선택하게 되었습니다.

혹시 다른 좋은 방법이 있을까요??

제 질문 사항이 부족한 것 같아 더 추가적으로 댓글을 적습니다

passState가 필요한 이유는 무엇인지?

로딩 중 server와 client간의 데이터 사이 달라지는 동안은 어떻게 처리하면 좋을 것인지?
간략하게 위와 같은 상황을 고민해보시면 좋습니다.

또한 useEffect는 최대한 사용하지 말아주세요.(react의 성능 및 철학에 위배됩니다. 키워드는 함수형에 대해 찾아보면 좋습니다.) 특히 useQuery안에 사용하는 데이터와 useState는 혼용해서 사용하면 데이터를 복제하여 사용하는 효과가 있기 때문에 복잡성을 높이기 쉽습니다

passState는 페이지 처음 들어왔을 때 지원 상태를 보여주는 것 뿐만 아니라 mutate의 요청이 성공했을 때 mutationFn의 리턴값을 반영하기 위해 사용했습니다.

이 부분을 해결하기 위해 로딩 처리를 하였습니다 !

useEffect를 사용하지 않는 방향으로 코드를 변경해보겠습니다.

useEffect와 passState를 사용하지 않고 queryData를 변경하는 방법으로 진행하면 어떨까요?

사실 passState라는 상태가 initialState라는 상태와 같은 느낌이라고 생각하는데, SSOT를 위배하는 코드 같아용

따라서 queryData를 조작하는 식으로 가는 느낌이 좋을듯합니다!

참고할만 한 예제 코드를 작성해봤는데 참고해보시기 바랍니당

const { mutate } = useMutation({ mutationFn: (afterState: "non-pass" | "pass") => patchApplicantState(`${applicantId}`, afterState), onMutate: async (afterState) => { // 데이터 꼬임 방지 await queryClient.cancelQueries(["applicantState", applicantId]); const snapshotState = queryClient.getQueryData<getApplicantStateRes>([ "applicantState", applicantId, ]); // 적절하게 업데이트 해준다. 혹은 복잡하다면, 이부분은 무시해도 된다. queryClient.setQueryData<getApplicantStateRes>( ["applicantState", applicantId], (prev) => { if (prev === "non-processed" && afterState === "pass") { return "first-passed"; } return prev; } ); return { snapshotState }; }, onSuccess: (data) => { queryClient.invalidateQueries(["kanbanDataArray", generation]); }, onError: (error, variables, context) => { window.alert("상태 변경에 실패했습니다."); // rollback한다. if (context !== undefined) { queryClient.setQueryData<getApplicantStateRes>( ["applicantState", applicantId], context.snapshotState ); } }, onSettled: () => { // 요청에 성공하면 새로운 passState를 가져온다. queryClient.invalidateQueries(["applicantState", applicantId]); }, });

@2yunseong
onMutate에서 setQueryData 부분은 Optimistic Update로 이해했는데 맞을까요?

onError 부분의 아래 코드와 onSettled의 내용이 중복 코드라고 생각이 되는데 아래 코드를 작성하신 이유가 있을까요?
(onSettled는 onError가 작동한 뒤에 작동하는 걸로 알고 있어서 OnError 부분에서 snapshotState으로 되돌리고 OnSettled에서 invalidateQueries를 하면 의미적으로 같은 동작을 한다고 생각합니다.)

if (context !== undefined) { queryClient.setQueryData<getApplicantStateRes>( ["applicantState", applicantId], context.snapshotState ); }

onError 부분의 아래 코드와 onSettled의 내용이 중복 코드라고 생각이 되는데 아래 코드를 작성하신 이유가 있을까요?
(onSettled는 onError가 작동한 뒤에 작동하는 걸로 알고 있어서 OnError 부분에서 snapshotState으로 되돌리고 OnSettled에서 invalidateQueries를 하면 의미적으로 같은 동작을 한다고 생각합니다.)

@smb0123
좋은지적이네요 👍
하지만 stale-while-revalidate(이하 swr)를 생각해보면 어떨까요?
swr 관점에서 본다면, 민보님이 지적해주신 두 동작이 같은 동작으로 여겨지지 않습니다.
전자의 동작은 "사용자가 새로운 데이터를 보기 전 기존의 stale한 데이터를 보여주는 것" 이고
후자의 동작은 "새로운 데이터를 보여주는 것" 이기 때문입니다.

이 말이 잘 이해가 가지 않는다면 swr 전략에 대해 공부해보시기 바랍니다 :)

loopy-lim · 2024-09-16T11:59:28Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  if (isLoading) {
+    return <></>;
+  }


해도 좋고 아니어도 좋은 부분이지만, useQuery중에서는 isLoading말고 하나더 다루어야 하는 데이터가 있답니다.(더 자세하면 몇개 더 있긴 하지만...)
기억나시는 부분이 있다면 댓글로 적어주세요!!(다른 useQuery를 사용하는 부분을 참고하셔도 좋습니다.)

useQuery로 받아온 데이터가 falsy 값이면 로딩처리를 해주고 있네요.
변경하겠습니다 !

오 좋습니다! 사실 위 생각은 error인 경우였습니다. 이에 대해 어떻게 처리할지 고민해보면 좋을 것 같습니다.
(참고로 오류는 정말 어려운 개념이기 때문에 여기서는 임시 처리만 해도 괜찮지 않을까 라는 생각도 해봅니다.)

error는 일단 임시 처리하겠습니다 !

loopy-lim · 2024-09-16T12:03:24Z

frontend/src/apis/applicant/index.ts

+export const patchApplicantState = async (
+  id: string,
+  afterState: "non-pass" | "pass"
+) => {
+  const { data } = await https.patch<KanbanCardReq["state"]>(
+    `/applicants/${id}/state?afterState=${afterState}`
+  );
+
+  return data;
+};


과도한 오버엔지니어링일 수도 있고 선택 차이지만 한마디 얹자면 econovation recruit는 아직 완성된 프로젝트가 아니며, 안정적인 프로젝트가 아닙니다. 즉 요구사항이 언제든지 변할 수 있다는 것을 시사하죠. 그래서 저와 같은 경우에는 interface를 적극적으로 만듧니다. 사실 한술 더 떠서 요즘에서는 DTO객체를 활용하기도 하네요. 하지만 이렇게 진행하셔도 좋을 것 같습니다.

DTO 객체에 대해서는 아직 자세히 알지 못해서 interface를 활용하겠습니다 !

흠... 사실 return의 경우에는 이미 140번째 줄로 인하여 type 추론을 하고 있답니다. 제가 하고 싶었던 말은 require에 관한 경우였어요. 1. 만약 ApplicantState가 더 늘어가는 경우
2. 들어오는 데이터가 보장이 안되어 있다면
어떻게 처리하면 좋을지 생각하면 좋을 것 같아요!!(사실 저기 data도 DTO를 통해 필요한 데이터로만 처리하면 더욱 안전성이 높은 프로젝트를 만들어 갈 수 있을 꺼에요)

관련된 아티클은 여기에 있어요.

loopy-lim · 2024-09-16T12:04:54Z

frontend/src/apis/applicant/index.ts

+  applicantId: string,
+  generation: string
+): Promise<ApplicantPassState | undefined> => {
+  const cardsData = await getKanbanCards(navigationId, generation);


아쉽지만 저희의 https객체에는 cache 기능이 없습니다. 그래서 지원자 수만큼 같은 요청이 backend에게 주어 부담을 줄 것입니다. 그러기에 이를 캐시를 적용하여 접근할 수 있는 방법이 있으면 더 좋지 않을까 라는 생각을 합니다.(캐시를 구현하는 것보다 구현되어 있는 것을 사용하면 좋을 듯 합니다.)

개인적으로 서버 요청을 통해서 가져온 데이터를 가공해서 반환하는 함수는 api 로직들과 함께 있으면 안될 것 같다는 생각이 듭니다!
그리고 요청을 통해서 받아온 데이터를 사용하여 가공하고자 한다면 기존의 절차 (api -> query -> 사용) 방식으로 해야하지 않을까 생각이 듭니다.

해당 방법은 채승님께서도 말씀해주셨지만, 요청을 여러번 하는 문제도 존재합니다.

이를 수정한다면 순수하게 getKanbanCard 의 반환값을 인자로 받고, getApplicantStateRes 형태를 반환하는 유틸 함수로 만드는 것도 좋아보입니다.

저의 개인적인 생각이므로 참고만 해주세요!

추가적으로 getKanbanCards(navigationId, generation)의 반환타입이 명확히 정의되어있다면 Promise<getApplicantStateRes>형태로 타입을 명시하지 않아도 올바르게 추론이 될 것 같습니다!

2yunseong

추석에 고생하셨습니다~~~!!

2yunseong · 2024-09-17T08:41:36Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+import { useSearchParams } from "next/navigation";
+import { useEffect, useState } from "react";
+import KanbanCardApplicantStatusLabel from "@/components/kanban/card/CardApplicantStatusLabel";
+import { ApplicantPassState, getAllKanbanData } from "@/src/apis/kanban";


getAllKanbanData는 안쓰이는 모듈입니다. 삭제 부탁드려요~~

2yunseong · 2024-09-17T08:43:17Z

frontend/src/apis/applicant/index.ts

+interface patchApplicantStateRes {
+  passState: ApplicantPassState;
+}


타입은 Pascal Case로 작성해주세요 :)

2yunseong · 2024-09-17T08:56:00Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  const {
+    data: initialState,
+    isLoading,
+    isError,
+  } = useQuery(["applicantState", applicantId], () =>
+    getApplicantState(navbarId, `${applicantId}`, generation)
+  );


navbarId에 대한 의존성이 빠진 것 같네요~

2yunseong · 2024-09-17T08:58:58Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  const {
+    data: myInfo,
+    isLoading: myInfoLoading,
+    isError: myInfoError,
+  } = useQuery(["user"], getMyInfo);


추후 버전업에 유리하기 위해 parameter로 하나의 객체로 표현할 수 있게 하는건 어떨까요?
https://tanstack.com/query/v5/docs/framework/react/guides/migrating-to-v5#supports-a-single-signature-one-object

2yunseong · 2024-09-17T09:00:09Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+  useEffect(() => {
+    if (initialState) {
+      setPassState(initialState);
+    }
+  }, [initialState]);


useEffect와 passState를 사용하지 않고 queryData를 변경하는 방법으로 진행하면 어떨까요?

사실 passState라는 상태가 initialState라는 상태와 같은 느낌이라고 생각하는데, SSOT를 위배하는 코드 같아용

따라서 queryData를 조작하는 식으로 가는 느낌이 좋을듯합니다!

참고할만 한 예제 코드를 작성해봤는데 참고해보시기 바랍니당

const { mutate } = useMutation({ mutationFn: (afterState: "non-pass" | "pass") => patchApplicantState(`${applicantId}`, afterState), onMutate: async (afterState) => { // 데이터 꼬임 방지 await queryClient.cancelQueries(["applicantState", applicantId]); const snapshotState = queryClient.getQueryData<getApplicantStateRes>([ "applicantState", applicantId, ]); // 적절하게 업데이트 해준다. 혹은 복잡하다면, 이부분은 무시해도 된다. queryClient.setQueryData<getApplicantStateRes>( ["applicantState", applicantId], (prev) => { if (prev === "non-processed" && afterState === "pass") { return "first-passed"; } return prev; } ); return { snapshotState }; }, onSuccess: (data) => { queryClient.invalidateQueries(["kanbanDataArray", generation]); }, onError: (error, variables, context) => { window.alert("상태 변경에 실패했습니다."); // rollback한다. if (context !== undefined) { queryClient.setQueryData<getApplicantStateRes>( ["applicantState", applicantId], context.snapshotState ); } }, onSettled: () => { // 요청에 성공하면 새로운 passState를 가져온다. queryClient.invalidateQueries(["applicantState", applicantId]); }, });

2yunseong · 2024-09-17T09:12:59Z

회장단과 관리자에게만 버튼이 보이도록 한다.

모든 사람에게 버튼이 보이도록 하고 권한이 없는 사람이 버튼을 클릭 시 alert를 띄운다.

저는 1번이 맞다고 생각하여 1번으로 구현하였는데 다른 분들의 의견도 궁금합니다 !

저도 1번이 좋다고 생각합니다 👍 👍

geongyu09

수고하셨습니다! 리뷰 늦게 달아서 죄송해요...
남은 추석 즐겁게 보내세요!

회장단과 관리자에게만 버튼이 보이도록 한다.

모든 사람에게 버튼이 보이도록 하고 권한이 없는 사람이 버튼을 클릭 시 alert를 띄운다.
저는 1번이 맞다고 생각하여 1번으로 구현하였는데 다른 분들의 의견도 궁금합니다 !

저도 해당 부분에서 1번이 올바르다고 생각합니다!

geongyu09 · 2024-09-17T13:04:07Z

frontend/components/applicant/applicantNode/CustomResource.component.tsx

+          )}] ${applicantDataFinder(data, "name")}`}</Txt>
+        </div>
+        {(myInfo?.role === "ROLE_OPERATION" ||
+          myInfo?.role === "ROLE_PRESIDENT") && (


해당 부분은 윗 부분에 작성된 if 문으로 인하여 undefined가 될 수 없습니다!
혹시 옵셔널로 한 특별한 이유가 있나요?

이 부분은 처음에 if문에 myInfo를 조건으로 작성하지 않았다가 나중에 수정하였는데 옵셔널에 대한 수정은 하지 않았네요 .....
수정하겠습니다 !

geongyu09 · 2024-09-17T13:32:02Z

frontend/src/apis/applicant/index.ts

+  applicantId: string,
+  generation: string
+): Promise<ApplicantPassState | undefined> => {
+  const cardsData = await getKanbanCards(navigationId, generation);


개인적으로 서버 요청을 통해서 가져온 데이터를 가공해서 반환하는 함수는 api 로직들과 함께 있으면 안될 것 같다는 생각이 듭니다!
그리고 요청을 통해서 받아온 데이터를 사용하여 가공하고자 한다면 기존의 절차 (api -> query -> 사용) 방식으로 해야하지 않을까 생각이 듭니다.

해당 방법은 채승님께서도 말씀해주셨지만, 요청을 여러번 하는 문제도 존재합니다.

이를 수정한다면 순수하게 getKanbanCard 의 반환값을 인자로 받고, getApplicantStateRes 형태를 반환하는 유틸 함수로 만드는 것도 좋아보입니다.

저의 개인적인 생각이므로 참고만 해주세요!

geongyu09 · 2024-09-17T13:33:07Z

frontend/src/apis/applicant/index.ts

+  applicantId: string,
+  generation: string
+): Promise<ApplicantPassState | undefined> => {
+  const cardsData = await getKanbanCards(navigationId, generation);


추가적으로 getKanbanCards(navigationId, generation)의 반환타입이 명확히 정의되어있다면 Promise<getApplicantStateRes>형태로 타입을 명시하지 않아도 올바르게 추론이 될 것 같습니다!

smb0123

수정 사항

타입 단언 -> 템플릿 리터럴로 수정
handler 함수의 prefix를 on으로 수정
useEffect 부분 제거 후 tanstack query의 기능으로 변경
로딩 처리 조건 추가
에러 처리
tanstack query의 캐시 기능 사용
getApplicantState를 util 함수로 변경

하지못한것

DTO 사용 ( 어떻게 사용해야 하는지 잘 모르겠습니다 ....)

geongyu09

좋네요! 수고많으셨습니다!
DTO적용을 하게 된다면 해당 부분 뿐만 아니라 전체적으로 적용해야 한다고 생각해서, 해당 부분은 추후에 함께 논의해봐도 될 것 같습니다!

2yunseong

👍

smb0123 added 3 commits September 16, 2024 14:28

feat: 지원자의 상태를 변경하는 api 함수 구현

1fa563e

feat: 특정 지원자에 대한 지원 상태를 가져오는 api 함수 구현

beb5efe

feat: 칸반 보드 상세 페이지에서 지원자의 지원 상태 ui 구현, 회장단과

165b80c

관리자만 지원 상태가 변경 가능하게 버튼 구현

smb0123 added the feature 기능개발 label Sep 16, 2024

smb0123 requested review from 2yunseong and geongyu09 September 16, 2024 05:41

smb0123 self-assigned this Sep 16, 2024

smb0123 requested review from BlackBean99 and loopy-lim as code owners September 16, 2024 05:41

loopy-lim reviewed Sep 16, 2024

View reviewed changes

smb0123 added 6 commits September 17, 2024 13:48

refactor: 타입 단언을 템플릿 리터럴로 변경

79168a4

refactor: 핸들러 함수의 prefix 변경 handle -> on

b47fb18

refactor: 로딩 처리 조건 추가

1656aa1

refactor: api 함수의 return 값 타입을 interface와 type으로 지정

6abdd75

feat: 에러 처리

e860e3c

feat: 로딩 처리 메시지 추가

8f1095c

2yunseong requested changes Sep 17, 2024

View reviewed changes

geongyu09 approved these changes Sep 17, 2024

View reviewed changes

smb0123 added 7 commits September 18, 2024 09:22

refactor: 인터페이스를 파스칼 케이스로 변경

4bc5185

refactor: 사용하지 않는 모듈 제거

1bb9f56

refactor: useQuery 버전업, queryKey에 navbarId 추가

e692ac4

refactor: 옵셔널 연산자 제거

03153e8

refactor: useState와 useEffect 제거

cc6dae1

refactor: getApplicantState 함수를 유틸함수로 변경

e2d85e0

refactor: staleTime 적용

b3e062f

smb0123 commented Sep 18, 2024

View reviewed changes

smb0123 requested review from loopy-lim, geongyu09 and 2yunseong September 18, 2024 10:04

geongyu09 approved these changes Sep 18, 2024

View reviewed changes

smb0123 mentioned this pull request Sep 18, 2024

feat: 지원서 조회 페이지 합불 상태 표시 및 변경 버튼 추가 #212

Closed

2yunseong approved these changes Sep 19, 2024

View reviewed changes

smb0123 merged commit 0909c09 into main Sep 19, 2024
1 check passed

	() => getApplicantState(navbarId, applicantId as string, generation)
	() => getApplicantState(navbarId, `${applicantId}`, generation)

feat: 칸반보드 상세 페이지에서 지원 상태 확인 및 지원 상태 변경 가능하게 구현 #208

feat: 칸반보드 상세 페이지에서 지원 상태 확인 및 지원 상태 변경 가능하게 구현 #208

Conversation

smb0123 commented Sep 16, 2024 • edited Loading

주요 변경사항

리뷰어에게...

문제 상황

관련 이슈

loopy-lim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loopy-lim Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2yunseong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2yunseong commented Sep 17, 2024 • edited Loading

geongyu09 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smb0123 left a comment

Choose a reason for hiding this comment

수정 사항

하지못한것

geongyu09 left a comment

Choose a reason for hiding this comment

2yunseong left a comment

Choose a reason for hiding this comment

smb0123 commented Sep 16, 2024 •

edited

Loading

loopy-lim Sep 17, 2024 •

edited

Loading

geongyu09 Sep 17, 2024 •

edited

Loading

2yunseong commented Sep 17, 2024 •

edited

Loading

geongyu09 left a comment •

edited

Loading

geongyu09 Sep 17, 2024 •

edited

Loading