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

[회비 생성] 회비 생성 기능 리팩토링 및 변경된 회비로 생성 가능하도록 수정 #193

Closed
wants to merge 10 commits into from
7 changes: 4 additions & 3 deletions src/component/YearPagination/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import * as S from './style';
interface YearPaginationProps {
duesYear: number;
setDuesYear: React.Dispatch<React.SetStateAction<number>>;
routeParam: string;
}

export default function YearPagination({ duesYear, setDuesYear }: YearPaginationProps) {
export default function YearPagination({ duesYear, setDuesYear, routeParam }: YearPaginationProps) {
const navigate = useNavigate();
const currentYear = new Date().getFullYear();
const param = useQueryParam('page');
Expand All @@ -20,14 +21,14 @@ export default function YearPagination({ duesYear, setDuesYear }: YearPagination
// 재학생 회비 내역이 2021년부터 시작하므로 2021년 이전으로 이동할 수 없음
const prevYear = page ? page + 1 : 2;
if (prevYear <= currentYear - 2020) {
navigate(`/dues?page=${prevYear}`);
navigate(`/${routeParam}?page=${prevYear}`);
setDuesYear((prev) => prev - 1);
}
};

const goToNextYear = () => {
if (page && page > 1) {
navigate(`/dues?page=${page - 1}`);
navigate(`/${routeParam}?page=${page - 1}`);
setDuesYear((prev) => prev + 1);
}
};
Copy link

Choose a reason for hiding this comment

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

해당 코드에서 몇 가지 개선 사항이 있습니다. 주요 사항은 YearPaginationProps 인터페이스에 routeParam 속성이 추가되었으나, 이를 사용하는 부분을 포함한 코드에서 경로 변경이 필요합니다. 전체적으로 경로를 동적으로 변경하여 재사용성을 높이는 것이 좋습니다.

다음은 수정이 필요한 부분입니다:

@@ -8,10 +8,11 @@ interface YearPaginationProps {
 
-export default function YearPagination({ duesYear, setDuesYear }: YearPaginationProps) {
+export default function YearPagination({ duesYear, setDuesYear, routeParam }: YearPaginationProps) {
   const navigate = useNavigate();
   const currentYear = new Date().getFullYear();
   const param = useQueryParam('page');
 
     // 타 Navigation 함수에서의 다른 경로 사용을 위해 수정
-      navigate(`/dues?page=${prevYear}`);
+      navigate(`/${routeParam}?page=${prevYear}`);
 
-      navigate(`/dues?page=${page - 1}`);
+      navigate(`/${routeParam}?page=${page - 1}`);

개선 사항 설명:

  1. 동적 경로 처리: navigate 함수 호출 시 고정된 /dues 대신 routeParam을 활용하여 경로를 동적으로 구성했습니다. 이렇게 하면 다양한 경로에 대해 이 컴포넌트를 재사용할 수 있어 유연성이 증가합니다.

  2. 주석 추가: 타 내비게이션 함수에서의 다른 경로 사용을 위한 주석을 추가하여 코드 이해도를 높였습니다.

이러한 변경으로 코드의 유지보수성과 가독성이 향상됩니다.

Expand Down
1 change: 1 addition & 0 deletions src/model/member.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export interface Member {
updatedAt: string;
isAuthed: boolean;
isDeleted: boolean;
isFeeExempt: boolean;
deleteReason: string;
birthday: string;
}
Copy link

Choose a reason for hiding this comment

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

보여주신 코드에서 특정 오류는 발견되지 않았습니다. 그러나 Member 인터페이스에 isFeeExempt 속성을 추가한 부분은 관련성이 높아 보입니다. 이 경우, 속성의 기본값이나 설명을 주석으로 추가하는 것이 좋습니다.

주요 사항:

  • isFeeExempt 속성 추가에 대한 명확한 이유와 설명이 필요합니다.

개선 제안:

   isFeeExempt: boolean; // 회원이 수수료 면제인지 여부

이렇게 주석을 추가하여 isFeeExempt의 역할을 명확히 해주는 것이 좋습니다.

Expand Down
4 changes: 2 additions & 2 deletions src/page/DuesManagement/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function DefaultTable() {
const updatedTrack = [...prevTrack];
updatedTrack[trackIndex] = !updatedTrack[trackIndex];
setFilteredValue(allDues.dues.filter((row) => updatedTrack[tracks.map((track) => track.name).indexOf(row.track.name)]
&& members?.content.some((member) => member.memberType === 'REGULAR' && member.id === row.memberId)));
&& members?.content.some((member) => member.memberType === 'REGULAR' && member.id === row.memberId)));
return updatedTrack;
});
};
Expand Down Expand Up @@ -154,7 +154,7 @@ function DefaultTable() {
<>
<div css={S.searchAndPagination}>
<div css={S.pagination}>
<YearPagination duesYear={duesYear} setDuesYear={setDuesYear} />
<YearPagination duesYear={duesYear} setDuesYear={setDuesYear} routeParam="dues" />
</div>
<div>
<Input
Copy link

Choose a reason for hiding this comment

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

주요 사항:

  1. 코드에서 setFilteredValue 호출 시 줄바꿈이 잘못되어 가독성이 떨어짐.
  2. YearPagination 컴포넌트에 필요한 prop인 routeParam이 추가됨.

개선 제안 사항:

@@ -94,7 +94,7 @@ function DefaultTable() {
       setFilteredValue(allDues.dues.filter((row) => 
         updatedTrack[tracks.map((track) => track.name).indexOf(row.track.name)] 
-        && members?.content.some((member) => member.memberType === 'REGULAR' && member.id === row.memberId)));
+        && members?.content.some((member) => member.memberType === 'REGULAR' && member.id === row.memberId)));
 
@@ -154,7 +154,7 @@ function DefaultTable() {
           <YearPagination duesYear={duesYear} setDuesYear={setDuesYear} 
-            />
+            routeParam="dues" 
+          />

주요 변경 사항은 다음과 같습니다:

  • setFilteredValue 부분은 가독성을 위해 줄바꿈을 수정했습니다.
  • YearPaginationrouteParam prop을 추가하여 컴포넌트를 더 유용하게 만들었습니다.

Expand Down
50 changes: 50 additions & 0 deletions src/page/DuesSetup/hooks/findMemberDuesInfo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as Excel from 'exceljs';
import { DuesInfo } from 'model/dues/allDues';
import { Pagination } from 'model/page';
import { Member } from 'model/member';

const NAME_ROW_NUMBER = 4;

interface FindMemberDuesInfoProps {
worksheet: Excel.CellValue[][];
members: Pagination<Member>;
prevYearDues: DuesInfo;
currentYearDues: DuesInfo;
}

export type MemberDuesInfo = { id: number, name: string, notPaidMonthInfo: { year: number, month: number }[] };

function findUnpaidMonths(duesInfo: DuesInfo | undefined, memberId: number, year: number) {
if (!duesInfo) return [];
const memberDues = duesInfo.dues.find((dues) => dues.memberId === memberId);
return memberDues && memberDues.unpaidCount > 0
? memberDues.detail.filter((detail) => detail.status === 'NOT_PAID').map((detail) => ({ year, month: detail.month }))
: [];
}

export function findMemberDuesInfo({
worksheet, members, prevYearDues, currentYearDues,
}: FindMemberDuesInfoProps) {
const result: MemberDuesInfo[] = [];
const currentYear = new Date().getFullYear();

worksheet.forEach((row) => {
const memberName = row[NAME_ROW_NUMBER] as string;
const memberInfo = members?.content.find((member) => member.name === memberName);

if (memberInfo) {
const unpaidMonthsPrevYear = findUnpaidMonths(prevYearDues, memberInfo.id, currentYear - 1);
const unpaidMonthsCurrentYear = findUnpaidMonths(currentYearDues, memberInfo.id, currentYear);

if (unpaidMonthsPrevYear.length > 0 || unpaidMonthsCurrentYear.length > 0) {
result.push({
id: memberInfo.id,
name: memberName,
notPaidMonthInfo: [...unpaidMonthsPrevYear, ...unpaidMonthsCurrentYear],
});
}
}
});

return result;
}
130 changes: 130 additions & 0 deletions src/page/DuesSetup/hooks/updateDues.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import * as Excel from 'exceljs';
import { DuesInfo } from 'model/dues/allDues';
import { Pagination } from 'model/page';
import { Member } from 'model/member';
import { UseMutationResult } from '@tanstack/react-query';
import { NewDuesData } from 'api/dues';
import { MemberDuesInfo } from './findMemberDuesInfo';

interface UpdateDuesProps {
worksheet: Excel.CellValue[][];
members: Pagination<Member>;
unpaidMemberDuesInfo: MemberDuesInfo[];
currentYearDues: DuesInfo;
putDuesMutation: UseMutationResult<any, Error, NewDuesData, unknown>;
postDuesMutation: UseMutationResult<any, Error, NewDuesData, unknown>;
}

type UpdateNotPaidDuesToPaidProps = Pick<UpdateDuesProps, 'unpaidMemberDuesInfo' | 'putDuesMutation'> & {
name: string;
depositDues: number;
};

type UpdateNullDuesToPaidProps = Pick<UpdateDuesProps, 'postDuesMutation'> & {
id: number;
name: string;
depositDues: number;
};

type UpdateWavierDuesProps = Pick<UpdateDuesProps, 'postDuesMutation' | 'members'>;
type UpdateNullToNotPaidDuesProps = Pick<UpdateDuesProps, 'postDuesMutation' | 'currentYearDues'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pick 굳


// depositDues만큼 unpaidMemberDuesInfo를 반영하여 mutate하기
function updateNotPaidDuesToPaid({
name, depositDues, unpaidMemberDuesInfo, putDuesMutation,
}: UpdateNotPaidDuesToPaidProps) {
let leftDepositDues = depositDues;
if (depositDues % 10000 === 0) {
Array.from({ length: depositDues }).forEach((_, index) => {
// TODO: 동명이인 처리
const memberDuesInfo = unpaidMemberDuesInfo.find((info) => info.name === name);
if (memberDuesInfo) {
const { id, notPaidMonthInfo } = memberDuesInfo;
const { year, month } = notPaidMonthInfo[index];
putDuesMutation.mutate({
memberId: id,
year,
month,
status: 'PAID',
});
leftDepositDues -= 10000;
}
});
}
return leftDepositDues;
}

function updateNullDuesToPaid({ id, depositDues, postDuesMutation }: UpdateNullDuesToPaidProps) {
// 회비를 매 달 1일에 정리하기 때문에 저번 달을 기준으로 처리한다.
const currentYear = new Date().getFullYear();
const prevMonth = new Date().getMonth();
Array.from({ length: depositDues }).forEach((_, index) => {
const year = prevMonth + index > 12 ? currentYear + 1 : currentYear;
const month = ((prevMonth + index) % 12) + 1;
postDuesMutation.mutate({
memberId: id,
year,
month,
status: 'PAID',
});
});
}

function updateWavierDues({ postDuesMutation, members }: UpdateWavierDuesProps) {
members.content.forEach((member) => {
if (member.isFeeExempted) {
postDuesMutation.mutate({
memberId: member.id,
year: new Date().getFullYear(),
month: new Date().getMonth() + 1,
status: 'SKIP',
});
}
});
}

function updateNullToNotPaidDues({ currentYearDues, postDuesMutation }: UpdateNullToNotPaidDuesProps) {
currentYearDues.dues.forEach((dues) => {
const prevMonth = new Date().getMonth();
if (dues.detail[prevMonth].status === null) {
postDuesMutation.mutate({
memberId: dues.memberId,
year: new Date().getFullYear(),
month: prevMonth + 1,
status: 'NOT_PAID',
});
}
});
}

export function updateDues({
worksheet, members, unpaidMemberDuesInfo, currentYearDues, putDuesMutation, postDuesMutation,
}: UpdateDuesProps) {
worksheet.forEach((row) => {
const [, , depositValue, , content, , note] = row;
const depositDues = Number(depositValue);
const name = String(content);
const id = members.content.find((member) => member.name === name)?.id;
let leftDepositDues;

if (name && depositDues && id) {
// type note = 'X' | '-' | `id=${id}`;
if (note !== 'X') {
/**
* 1. 미납된 회비를 납부한 경우, 미납된 회비를 납부 처리한다.
* 2. 미납된 회비가 없는 경우, 납부한 달의 회비를 납부 처리한다.
* 3. 면제 인원의 경우 면제 처리한다.
* 4. 납부한 회비가 없는 경우 미납 처리한다. (1~3번 까지 처리가 끝난 후 아무값도 없는 경우 NOT_PAID 처리)
*/
leftDepositDues = updateNotPaidDuesToPaid({
name, depositDues, unpaidMemberDuesInfo, putDuesMutation,
});
updateNullDuesToPaid({
id, name, depositDues: leftDepositDues, postDuesMutation,
});
updateWavierDues({ members, postDuesMutation });
updateNullToNotPaidDues({ postDuesMutation, currentYearDues });
}
}
});
}
29 changes: 29 additions & 0 deletions src/page/DuesSetup/hooks/updateWorksheetwithDuesInfo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as Excel from 'exceljs';
import { MemberDuesInfo } from './findMemberDuesInfo';

const NAME_ROW_NUMBER = 4;
const UNPAID_INFO_ROW_NUMBER = 7;

export function updateWorksheetWithDuesInfo(
worksheet: Excel.CellValue[][],
memberDuesInfo: MemberDuesInfo[],
) {
const updatedWorksheet = worksheet.filter((_, index) => index > 0);
return updatedWorksheet.map((row) => {
const memberName = row[NAME_ROW_NUMBER] as string;
const memberInfo = memberDuesInfo.find((info) => info.name === memberName);
const updatedRow = [...row];

if (memberInfo) {
const unpaidMonths = memberInfo.notPaidMonthInfo
.map(({ year, month }) => `${year}년 ${month}월`)
.join(', ');

updatedRow[UNPAID_INFO_ROW_NUMBER] = unpaidMonths;
} else {
updatedRow[UNPAID_INFO_ROW_NUMBER] = '';
}

return updatedRow;
});
}
Copy link

Choose a reason for hiding this comment

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

주요 사항:

  1. Excel.CellValue[][]worksheet 타입으로 사용되는 것이 적절한지 확인해야 합니다. 이 타입이 예상한 자원형과 맞는지 검토가 필요합니다.
  2. 상수 NAME_ROW_NUMBERUNPAID_INFO_ROW_NUMBER의 설정 값이 실제 엑셀 데이터 구조에 맞는지도 확인해볼 필요가 있습니다.

개선 제안 사항:

@@ -1,4 +1,4 @@
-import * as Excel from 'exceljs';
+import { Worksheet } from 'exceljs'; // Worksheet 타입을 명시적으로 가져오기
+
-  worksheet: Excel.CellValue[][],
+  worksheet: Worksheet, // CellValue 배열 대신 Worksheet로 변경

위의 개선 사항은 worksheet 변수의 타입을 Worksheet로 바꾸어 ExcelJS에서 제공하는 적절한 워크시트 인터페이스를 활용하도록 하고, 코드 가독성을 높이는 데 도움을 줄 것입니다.

43 changes: 43 additions & 0 deletions src/page/DuesSetup/hooks/useReadExcelFile.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import * as Excel from 'exceljs';

function makeWorksheetToArray(worksheet: Excel.Worksheet): Excel.CellValue[][] {
const result: Excel.CellValue[][] = [];
worksheet.eachRow((row, rowNumber) => {
result.push([]);
row.eachCell((cell) => {
if (cell.value !== null && cell.value !== undefined) {
result[rowNumber - 1].push(cell.value);
}
});
});
return result;
}

export function useReadExcelFile(excelFileRef: React.RefObject<HTMLInputElement>): () => Promise<Excel.CellValue[][] | null> {
const readFile: () => Promise<Excel.CellValue[][] | null> = async () => {
const workbook = new Excel.Workbook();
const file = excelFileRef.current?.files?.[0];
if (!file) return null;

Copy link
Contributor

Choose a reason for hiding this comment

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

액셀 읽기 굳

const reader = new FileReader();
return new Promise((resolve, reject) => {
reader.onload = async (e) => {
try {
const data = new Uint8Array(e.target?.result as ArrayBuffer);
await workbook.xlsx.load(data);
const worksheet = workbook.getWorksheet(1);
if (!worksheet) {
reject(new Error('worksheet is not found'));
}
resolve(worksheet ? makeWorksheetToArray(worksheet) : null);
} catch (error) {
reject(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

reject하고 promise를 전달 받은 컴포넌트에서 에러를 처리하는 방식인가요?
error를 throw 해서 error boundary를 활용하는 방식도 나중에 고려하면 좋을 것 같아요

}
};
reader.onerror = (error) => reject(error);
reader.readAsArrayBuffer(file);
});
};

return readFile;
}
Copy link

Choose a reason for hiding this comment

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

코드 리뷰 결과, 다음과 같은 주요 사항과 개선 필요 사항이 있습니다.

  1. 오류 처리: worksheet가 존재하지 않을 경우 reject 후에 return이 없어 추가 처리가 불가능합니다.
  2. 타입 안정성: reader.onload 내에서 e.target?.result의 타입 체크가 제대로 되어 있지 않습니다.
  3. Promise 체이닝: resolve 호출 후에도 여전히 비동기 처리를 해야 하므로 잘못된 흐름을 막는 구조로 개선할 필요가 있습니다.
  4. 주석 누락: 함수나 주요 로직에 대한 설명이 없어 주석 추가가 필요합니다.

아래는 제안하는 개선 부분입니다.

@@ -15,10 +15,12 @@
           if (!worksheet) {
             reject(new Error('worksheet is not found'));
+            return; // worksheet가 없으면 조기 리턴
           }
-          resolve(worksheet ? makeWorksheetToArray(worksheet) : null);
+          const result = makeWorksheetToArray(worksheet);
+          resolve(result); 
         } catch (error) {
           reject(error);
         }
       };
-      reader.onerror = (error) => reject(error);
+      reader.onerror = (error) => { 
+        console.error('File reading error:', error); 
+        reject(error); 
+      };
       reader.readAsArrayBuffer(file);
     });

위 개선 사항은 코드의 오류 처리 및 안정성을 높이고 가독성을 향상시키는 데 기여합니다.

Loading
Loading