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: 칸반보드 상세 페이지에서 지원 상태 확인 및 지원 상태 변경 가능하게 구현 #208

Merged
merged 16 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/components/applicant/DetailLeft.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const ApplicantDetailLeft = ({

return (
<>
<ApplicantResource data={data} postId={postId} />
<ApplicantResource data={data} postId={postId} generation={generation} />
<ApplicantLabel postId={postId} generation={generation} />
<ApplicantComment
cardId={cardId}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,115 @@
import Txt from "@/components/common/Txt.component";
import { ApplicantReq } from "@/src/apis/applicant";
import {
ApplicantReq,
getApplicantState,
patchApplicantState,
} from "@/src/apis/applicant";
import { applicantDataFinder } from "@/src/functions/finder";
import Portfolio from "./Portfolio";
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { useSearchParams } from "next/navigation";
import { useEffect, useState } from "react";
import KanbanCardApplicantStatusLabel from "@/components/kanban/card/CardApplicantStatusLabel";
import { ApplicantPassState, getAllKanbanData } from "@/src/apis/kanban";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

import { useAtomValue } from "jotai";
import { KanbanSelectedButtonNumberState } from "@/src/stores/kanban/Navbar.atoms";
import { getMyInfo } from "@/src/apis/interview";
interface ApplicantResourceProps {
data: ApplicantReq[];
postId: string;
generation: string;
}

const ApplicantResource = ({ data, postId }: ApplicantResourceProps) => {
const ApplicantResource = ({
data,
postId,
generation,
}: ApplicantResourceProps) => {
const navbarId = useAtomValue(KanbanSelectedButtonNumberState);
const searchParams = useSearchParams();
const applicantId = searchParams.get("applicantId");
const queryClient = useQueryClient();

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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


const [passState, setPassState] =
useState<ApplicantPassState>("non-processed");

const { mutate } = useMutation({
mutationFn: (afterState: "non-pass" | "pass") =>
patchApplicantState(`${applicantId}`, afterState),
onSuccess: (data) => {
queryClient.invalidateQueries(["kanbanDataArray", generation]);
setPassState(data.passState);
},
});

const onFailedButtonClick = () => {
mutate("non-pass");
};

const onPassedButtonClick = () => {
mutate("pass");
};

useEffect(() => {
if (initialState) {
setPassState(initialState);
}
}, [initialState]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@loopy-lim loopy-lim Sep 17, 2024

Choose a reason for hiding this comment

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

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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]);
    },
  });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

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


if (!initialState || isLoading || !myInfo || myInfoLoading) {
return <div>로딩중...</div>;
}

if (isError || myInfoError) {
return <div>에러 발생</div>;
}

return (
<>
<div className="flex flex-col gap-1 mb-2">
<Txt className="text-xl text-secondary-200 font-medium">
{applicantDataFinder(data, "major")}
</Txt>
<Txt typography="h2">{`[${applicantDataFinder(
data,
"field"
)}] ${applicantDataFinder(data, "name")}`}</Txt>
<div className="flex justify-between items-center">
<div className="flex flex-col gap-1 mb-2">
<div className="flex justify-between items-center">
<Txt className="text-xl text-secondary-200 font-medium">
{applicantDataFinder(data, "major")}
</Txt>
<KanbanCardApplicantStatusLabel passState={passState} />
</div>
<Txt typography="h2">{`[${applicantDataFinder(
data,
"field"
)}] ${applicantDataFinder(data, "name")}`}</Txt>
</div>
{(myInfo?.role === "ROLE_OPERATION" ||
myInfo?.role === "ROLE_PRESIDENT") && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

<div className="flex gap-5">
<button
onClick={onFailedButtonClick}
className="bg-zinc-200 w-20 h-20 hover:bg-sky-400 rounded-xl"
>
불합격
</button>
<button
onClick={onPassedButtonClick}
className="bg-zinc-200 w-20 h-20 hover:bg-sky-400 rounded-xl"
>
합격
</button>
</div>
)}
</div>
<div className="flex gap-4 mb-8">
<div className="flex gap-1">
Expand Down
29 changes: 29 additions & 0 deletions frontend/src/apis/applicant/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getAllInterviewerWithOrder } from "@/src/apis/interview";
import { APPLICANT_KEYS } from "@/src/constants";
import { https } from "@/src/functions/axios";
import { ApplicantPassState, getKanbanCards, KanbanCardReq } from "../kanban";

export interface ApplicantReq {
name: string;
Expand Down Expand Up @@ -131,3 +132,31 @@ export const getApplicantTimeTables = async (id: string) => {

return data;
};

interface patchApplicantStateRes {
passState: ApplicantPassState;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

return data;
};

type getApplicantStateRes = ApplicantPassState | undefined;

export const getApplicantState = async (
navigationId: string,
applicantId: string,
generation: string
): Promise<getApplicantStateRes> => {
const cardsData = await getKanbanCards(navigationId, generation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@geongyu09 geongyu09 Sep 17, 2024

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return cardsData.find((card) => card.applicantId === applicantId)?.state
.passState;
};