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 3 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,104 @@
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 } = useQuery(
["applicantState", applicantId],
() => getApplicantState(navbarId, applicantId as string, generation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

);

const { data: myInfo } = useQuery(["user"], getMyInfo);

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

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

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

const handlePassedButtonClick = () => {
mutate("pass");
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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 (isLoading) {
return <></>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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로 받아온 데이터가 falsy 값이면 로딩처리를 해주고 있네요.
변경하겠습니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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={handleFailedButtonClick}
className="bg-zinc-200 w-20 h-20 hover:bg-sky-400 rounded-xl"
>
불합격
</button>
<button
onClick={handlePassedButtonClick}
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
23 changes: 23 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,25 @@ export const getApplicantTimeTables = async (id: string) => {

return data;
};

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

return data;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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


export const getApplicantState = async (
navigationId: string,
applicantId: string,
generation: string
): Promise<ApplicantPassState | undefined> => {
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;
};