Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive report functionality by adding API endpoints, services, and data models for project reports and budget reports. The implementation includes list/detail views for project reports, CRUD operations for project report revisions and timelines, and management of budget report income/expenses.
- Adds API endpoints (apiRpt001-024) for project reports, budget reports, and their revisions
- Implements service layers for handling report operations with proper business logic
- Updates database schemas and repository mappings to support new report features
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/interface/src/api/report/type/project-report.type.ts | Defines TypeScript types and Zod schemas for project report APIs |
| packages/interface/src/api/report/endpoint/*.ts | Implements API endpoint configurations for all report-related operations |
| packages/api/src/feature/report/service/*.ts | Implements business logic services for project and budget reports |
| packages/api/src/feature/report/controller/report.controller.ts | Adds REST API endpoints with proper validation and routing |
| Database schemas and repositories | Updates to support new report data structures and relationships |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| timelines: z.array( | ||
| z.object({ | ||
| // name: z.string().max(255), | ||
| projectReportRevision: z.object({ id: zId }).optional(), |
There was a problem hiding this comment.
[nitpick] The timeline object contains an optional projectReportRevision field that should be consistent with how other revision references are handled. Consider using zExtractId(zProjectReportRevision) for consistency with the established pattern used elsewhere in the codebase.
| projectReportRevision: z.object({ id: zId }).optional(), | |
| projectReportRevision: zExtractId(zProjectReportRevision).optional(), |
packages/api/src/feature/report/service/project-report.service.ts
Outdated
Show resolved
Hide resolved
packages/api/src/feature/report/service/project-report.service.ts
Outdated
Show resolved
Hide resolved
| return Array.from(budgetReportInfoMap, ([key, value]) => { | ||
| // eslint-disable-next-line | ||
| const { reportSubmittedAt, proposalSubmittedAt, ...info } = value; | ||
| return { id: key.id, ...info }; |
There was a problem hiding this comment.
Same issue as in project-report.service.ts - accessing key.id is incorrect since key is the map key (a number). This should be { id: key, ...info }.
| return { id: key.id, ...info }; | |
| return { id: key, ...info }; |
| > = { | ||
| id: BudgetReportIncomeRevision, | ||
| budgetReportIncome: BudgetReportIncomeRevision, | ||
| budgetReportIncomeId: BudgetReportIncomeRevision, |
There was a problem hiding this comment.
[nitpick] The field mapping appears to have been corrected from budgetReportIncome to budgetReportIncomeId, but this change should be verified to ensure it aligns with the actual database column name and usage patterns.
| endTerm: timeline.duration.endTerm, | ||
| detail: timeline.detail, | ||
| note: timeline.note, | ||
| projectReportRevisionId: timeline.projectReportRevision.id, |
There was a problem hiding this comment.
The timeline mapping assumes timeline.projectReportRevision.id exists, but according to the type definition, this field is optional. Add null checking or make the field required to prevent potential runtime errors.
| projectReportRevisionId: timeline.projectReportRevision.id, | |
| projectReportRevisionId: timeline.projectReportRevision?.id, |
| endTerm: timeline.duration.endTerm, | ||
| detail: timeline.detail, | ||
| note: timeline.note, | ||
| projectReportRevisionId: timeline.projectReportRevision.id, |
There was a problem hiding this comment.
The timeline mapping assumes timeline.projectReportRevision.id exists, but according to the type definition, this field is optional. Add null checking or make the field required to prevent potential runtime errors.
There was a problem hiding this comment.
혹시 이 수많은 apiRpt00X 들은 뭐하는 친구들일까요?
There was a problem hiding this comment.
결산안 + 사업보고서 관련 API들입니다!
There was a problem hiding this comment.
각 api문서들에 description이 빠져 있는 것 같습니다!
thxx132
left a comment
There was a problem hiding this comment.
오타랑 conflict 해결하고 다시 리뷰가 필요할 것 같습니다. 그런데 api 구성 자체는 큰 문제 없긴 한데 예산안 쪽이랑 달라서... 프론트에서 괜찮을까요?
|
근데 어차피 연결하다보면 또 문제가 생길 수도 있어서 일단 연결을 한번 해보고 결정하는 것도 괜찮아 보이네요 |
|
전체적으로 copilot 리뷰 내용에 더해서 |
|
이 PR 남은 부분은 제가 목요일 미팅 전까지 얼추 해서 리뷰 가능한 상태로 만들어둬보겠습니다 |
|
화이팅입니다! 아 그런데 previousProposalIncome 같은 항목들은 어차피 organization이랑 semester id들로 특정되기도 하고 넣으면 괜히 오류가 생길 수 있어서 빼는 것이 좋을 것 같습니다! |
요약 *
It closes #262
이후 Task *