-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
- dues로 전부 페이지가 이동되는 것을 routeParam 인자값으로 이동 되도록 수정
- 엑셀 파일을 읽는 프로미스를 리턴함 - 엑셀 파일을 읽을 수 있도록 배열 형태로 변경하는 makeWorksheetToArray 함수 추가
- 회비 업데이트는 다음 4단계를 통해 업데이트 된다. - 미납된 회비를 납부한 경우, 미납된 회비를 납부 처리한다. - 미납된 회비가 없는 경우, 납부한 달의 회비를 납부 처리한다. - 면제 인원의 경우 면제 처리한다. - 납부한 회비가 없는 경우 미납 처리한다.
- 엑셀 읽기 - 엑셀 읽은 후 엑셀 정보를 화면에 출력 - 정보에 맞게 회비 업데이트하기
setDuesYear((prev) => prev - 1); | ||
} | ||
}; | ||
|
||
const goToNextYear = () => { | ||
if (page && page > 1) { | ||
navigate(`/dues?page=${page - 1}`); | ||
navigate(`/${routeParam}?page=${page - 1}`); | ||
setDuesYear((prev) => prev + 1); | ||
} | ||
}; |
There was a problem hiding this comment.
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}`);
개선 사항 설명:
-
동적 경로 처리:
navigate
함수 호출 시 고정된/dues
대신routeParam
을 활용하여 경로를 동적으로 구성했습니다. 이렇게 하면 다양한 경로에 대해 이 컴포넌트를 재사용할 수 있어 유연성이 증가합니다. -
주석 추가: 타 내비게이션 함수에서의 다른 경로 사용을 위한 주석을 추가하여 코드 이해도를 높였습니다.
이러한 변경으로 코드의 유지보수성과 가독성이 향상됩니다.
@@ -42,6 +42,7 @@ export interface Member { | |||
updatedAt: string; | |||
isAuthed: boolean; | |||
isDeleted: boolean; | |||
isFeeExempt: boolean; | |||
deleteReason: string; | |||
birthday: string; | |||
} |
There was a problem hiding this comment.
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
의 역할을 명확히 해주는 것이 좋습니다.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주요 사항:
- 코드에서
setFilteredValue
호출 시 줄바꿈이 잘못되어 가독성이 떨어짐. 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
부분은 가독성을 위해 줄바꿈을 수정했습니다.YearPagination
에routeParam
prop을 추가하여 컴포넌트를 더 유용하게 만들었습니다.
|
||
return updatedRow; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주요 사항:
Excel.CellValue[][]
는worksheet
타입으로 사용되는 것이 적절한지 확인해야 합니다. 이 타입이 예상한 자원형과 맞는지 검토가 필요합니다.- 상수
NAME_ROW_NUMBER
와UNPAID_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에서 제공하는 적절한 워크시트 인터페이스를 활용하도록 하고, 코드 가독성을 높이는 데 도움을 줄 것입니다.
}; | ||
|
||
return readFile; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰 결과, 다음과 같은 주요 사항과 개선 필요 사항이 있습니다.
- 오류 처리:
worksheet
가 존재하지 않을 경우reject
후에return
이 없어 추가 처리가 불가능합니다. - 타입 안정성:
reader.onload
내에서e.target?.result
의 타입 체크가 제대로 되어 있지 않습니다. - Promise 체이닝:
resolve
호출 후에도 여전히 비동기 처리를 해야 하므로 잘못된 흐름을 막는 구조로 개선할 필요가 있습니다. - 주석 누락: 함수나 주요 로직에 대한 설명이 없어 주석 추가가 필요합니다.
아래는 제안하는 개선 부분입니다.
@@ -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);
});
위 개선 사항은 코드의 오류 처리 및 안정성을 높이고 가독성을 향상시키는 데 기여합니다.
@@ -246,7 +246,7 @@ function DefaultTable() { | |||
<> | |||
<div css={S.searchAndPagination}> | |||
<div css={S.pagination}> | |||
<YearPagination duesYear={duesYear} setDuesYear={setDuesYear} /> | |||
<YearPagination duesYear={duesYear} setDuesYear={setDuesYear} routeParam="edit-dues" /> | |||
</div> | |||
<div> | |||
{(myInfo.authority === 'ADMIN' || myInfo.authority === 'MANAGER') && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드의 주요 사항은 YearPagination
컴포넌트에 새로운 prop인 routeParam
을 추가한 것입니다. 이로 인해 기능상 문제가 없고, 오히려 더 많은 정보를 전달하게 되어 개선된 것으로 보입니다.
다만, 해당 변경이 잘 작동하는지 확인하기 위해 YearPagination
컴포넌트에서 routeParam
을 처리하는 로직이 존재하는지 검토해야 합니다. 만약 YearPagination
에 대한 수정이 필요하다면 그 부분도 함께 점검해야 합니다.
개선 사항:
routeParam
prop이 필요한 경우에 적절하게 다뤄져야 하므로, 관련 문서 또는 주석을 달아 사용 의도를 명확히 하는 것이 좋습니다.
개선 제안
@@ -245,7 +245,8 @@ function DefaultTable() {
<div css={S.pagination}>
- <YearPagination duesYear={duesYear} setDuesYear={setDuesYear} />
+ {/* routeParam을 추가하여 YearPagination에 편집 모드 정보 전달 */}
+ <YearPagination duesYear={duesYear} setDuesYear={setDuesYear} routeParam="edit-dues" />
위의 diff는 routeParam
추가에 대한 설명 주석을 넣음으로써 코드의 가독성과 유지보수성을 높이는 데 기여합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것이 네부캠 코드인가
}; | ||
|
||
type UpdateWavierDuesProps = Pick<UpdateDuesProps, 'postDuesMutation' | 'members'>; | ||
type UpdateNullToNotPaidDuesProps = Pick<UpdateDuesProps, 'postDuesMutation' | 'currentYearDues'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pick 굳
const workbook = new Excel.Workbook(); | ||
const file = excelFileRef.current?.files?.[0]; | ||
if (!file) return null; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
액셀 읽기 굳
} | ||
resolve(worksheet ? makeWorksheetToArray(worksheet) : null); | ||
} catch (error) { | ||
reject(error); |
There was a problem hiding this comment.
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를 활용하는 방식도 나중에 고려하면 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거시 네이버 코드..! 수고하셨습니다!
버그가 어마어마하게 터져서... 우선 PR 닫아 놓겠습니다! |
회비 생성 기능 리팩토링
기능 설명
동아리 계좌가 변경되어 새롭게 받아오는 엑셀 시트의 형식에 맞게 변경하였습니다.
하나의 컴포넌트에 너무 많은 기능을 가지고 있다고 판단하여, 회비 업데이트 기능 및 엑셀 불러오기 기능을 전부 훅으로 변경하여 필요할 때 사용할 수 있고, 재사용 가능한 구조로 변경하였습니다.
엑셀을 읽는
useReadExcelFile
훅을 제외한 나머지는 전부 회의 생성 기능에 종속되는 훅이기 때문에 함수명 맨 앞에use
를 사용하지 않도록 하였습니다.useReadExcelFile
엑셀 파일을 읽고 해당 파일을 프로미스 객체로 받고, 해당 프로미스 객체는 읽은 엑셀 파일을 배열 형태로 리턴하도록 하였습니다.
updateDues
updateWorksheetWithDuesInfo
엑셀 시트에서 각각의 회원의 미납 정보를 포함하여 화면에 출력하도록 하였습니다.
출력 형태 예시) 김도훈 - 2024년 10월
${year}년 ${month}월 형태로 미납한 날의 정보를 출력하도록 하였습니다.
findMemberDuesInfo
엑셀 시트에 포함된 인원의 미납 정보를 가져옵니다. 위에
updateWorksheetWithDuesInfo
함수에 종속되어 사용됩니다.Please check if the PR fulfills these requirements
main
branch?Screenshot
Precautions (main files for this PR ...)