E2612 - course based reports#170
Open
kamatmihir2002 wants to merge 11 commits into
Open
Conversation
Enable tables to be viewable without pagination entirely
Feat/course based reports
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a course assignment reporting feature: types and service to build report rows/columns, a new CourseReport page and protected route, Course page wiring and action to navigate, a Table prop to disable pagination row model, and comprehensive tests for pages, columns, and the service. ChangesCourse Assignment Reporting (single change DAG)
Sequence Diagram(s)sequenceDiagram
participant User
participant CoursePage as Course Page
participant Router
participant CourseReportPage as CourseReport Page
participant API
participant Service as Build Service
participant Table
User->>CoursePage: Click "View Course Report"
CoursePage->>Router: Navigate to /courses/:courseId/class_assignment_overview
Router->>CourseReportPage: Mount with courseId param
CourseReportPage->>API: GET /course_reports?course_id=X
API-->>CourseReportPage: Return assignments + students
CourseReportPage->>Service: buildRows(students, assignments)
Service-->>CourseReportPage: CourseReportRow[] (incl. class avg)
CourseReportPage->>Service: buildColumns(assignments, visibleFields)
Service-->>CourseReportPage: Column definitions[]
CourseReportPage->>Table: Render students Table + class-average Table
Table-->>User: Display rows with toggled column visibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/Courses/CourseReport.tsx (1)
166-174: Consider explicitly disabling column filters on the student table for consistency.The class-average table disables column filters, but the student table relies on the default. Setting
showColumnFilter={false}here keeps report tables behavior aligned.🛠️ Suggested tweak
<Row> <Table key={tableKey} data={studentRows} columns={columns} columnVisibility={columnVisibility} showPagination={false} disablePaginationRowModel showGlobalFilter={false} + showColumnFilter={false} /> </Row>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Courses/CourseReport.tsx` around lines 166 - 174, The student Table component (rendered with props key={tableKey}, data={studentRows}, columns={columns}, columnVisibility={columnVisibility}) currently relies on the default column filter behavior; explicitly disable column filters by adding the prop showColumnFilter={false} to that Table invocation so its behavior matches the class-average table and keeps report tables consistent.src/services/courseAssignmentOverviewService.ts (1)
18-26: Avoid wrapping every cell in a<span>when not needed.Only class-average cells need bold styling; returning plain text for normal rows reduces DOM nodes and render cost on large reports.
♻️ Proposed refactor
const renderCellValue = ( value: number | string | boolean | null | undefined, isClassAverage: boolean | undefined -) => - React.createElement( - "span", - { className: isClassAverage ? "fw-bold" : undefined }, - value === null || value === undefined ? "" : String(value) - ); +) => { + const text = value === null || value === undefined ? "" : String(value); + return isClassAverage + ? React.createElement("span", { className: "fw-bold" }, text) + : text; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/courseAssignmentOverviewService.ts` around lines 18 - 26, renderCellValue currently always returns a React <span>, which needlessly creates DOM nodes for non-class-average cells; change renderCellValue so that when isClassAverage is truthy it returns a <span> with className "fw-bold" and the stringified value, otherwise return the plain string (or empty string for null/undefined) directly. Update the function signature and return branches in renderCellValue to only create the React element for the class-average path and return primitive text for the normal path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Table/Table.tsx`:
- Around line 27-31: TableProps currently declares disablePaginationRowModel
twice which causes a TypeScript duplicate identifier error; remove the redundant
declaration so disablePaginationRowModel only appears once in the TableProps
interface (leave the original declaration and delete the duplicate among the
other props like showGlobalFilter, showColumnFilter, and showPagination). Ensure
the remaining single disablePaginationRowModel has the intended optional boolean
type.
In `@src/pages/Courses/Course.tsx`:
- Around line 172-174: The Course component currently returns <Outlet /> when
isReportPage is true but the data-fetching useEffect still runs; to fix, prevent
the effect from firing on report routes by either moving the early return for
isReportPage above the useEffect so the effect is never mounted, or add
isReportPage as a guard inside the effect (e.g., if (isReportPage) return)
before calling the fetch logic (the effect that dispatches/fetches
courses/institutions/users/assignments). Update references to isReportPage and
the existing useEffect (the function that calls
fetchCourses/fetchInstitutions/fetchUsers/fetchAssignments or dispatches those
actions) accordingly so no network calls occur for report routes.
---
Nitpick comments:
In `@src/pages/Courses/CourseReport.tsx`:
- Around line 166-174: The student Table component (rendered with props
key={tableKey}, data={studentRows}, columns={columns},
columnVisibility={columnVisibility}) currently relies on the default column
filter behavior; explicitly disable column filters by adding the prop
showColumnFilter={false} to that Table invocation so its behavior matches the
class-average table and keeps report tables consistent.
In `@src/services/courseAssignmentOverviewService.ts`:
- Around line 18-26: renderCellValue currently always returns a React <span>,
which needlessly creates DOM nodes for non-class-average cells; change
renderCellValue so that when isClassAverage is truthy it returns a <span> with
className "fw-bold" and the stringified value, otherwise return the plain string
(or empty string for null/undefined) directly. Update the function signature and
return branches in renderCellValue to only create the React element for the
class-average path and return primitive text for the normal path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dfed429-bfe2-4145-959a-f4844d557d3d
📒 Files selected for processing (11)
src/App.tsxsrc/components/Table/Table.tsxsrc/pages/Courses/Course.test.tsxsrc/pages/Courses/Course.tsxsrc/pages/Courses/CourseColumns.test.tsxsrc/pages/Courses/CourseColumns.tsxsrc/pages/Courses/CourseReport.test.tsxsrc/pages/Courses/CourseReport.tsxsrc/services/__tests__/courseAssignmentOverviewService.test.tssrc/services/courseAssignmentOverviewService.tssrc/types/courseAssignmentOverview.ts
Comment on lines
+27
to
+31
| disablePaginationRowModel?: boolean; | ||
| showGlobalFilter?: boolean; | ||
| showColumnFilter?: boolean; | ||
| showPagination?: boolean; | ||
| disablePaginationRowModel?: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate declaration count/location
rg -n 'disablePaginationRowModel\?: boolean;' src/components/Table/Table.tsxRepository: expertiza/reimplementation-front-end
Length of output: 164
Remove the duplicate disablePaginationRowModel property declaration in TableProps.
Lines 27 and 31 both declare the same property, which will cause a TypeScript compile-time error.
Proposed fix
interface TableProps {
data: Record<string, any>[];
columns: ColumnDef<any, any>[];
disableGlobalFilter?: boolean;
disablePaginationRowModel?: boolean;
showGlobalFilter?: boolean;
showColumnFilter?: boolean;
showPagination?: boolean;
- disablePaginationRowModel?: boolean;
tableSize?: { span: number; offset: number };
columnVisibility?: Record<string, boolean>;
onSelectionChange?: (selectedData: Record<any, any>[]) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| disablePaginationRowModel?: boolean; | |
| showGlobalFilter?: boolean; | |
| showColumnFilter?: boolean; | |
| showPagination?: boolean; | |
| disablePaginationRowModel?: boolean; | |
| disablePaginationRowModel?: boolean; | |
| showGlobalFilter?: boolean; | |
| showColumnFilter?: boolean; | |
| showPagination?: boolean; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Table/Table.tsx` around lines 27 - 31, TableProps currently
declares disablePaginationRowModel twice which causes a TypeScript duplicate
identifier error; remove the redundant declaration so disablePaginationRowModel
only appears once in the TableProps interface (leave the original declaration
and delete the duplicate among the other props like showGlobalFilter,
showColumnFilter, and showPagination). Ensure the remaining single
disablePaginationRowModel has the intended optional boolean type.
Comment on lines
+172
to
+174
| if (isReportPage) { | ||
| return <Outlet />; | ||
| } |
There was a problem hiding this comment.
Report-route short-circuit still triggers unnecessary course-page API fetches.
Although Line 172 returns only <Outlet />, the fetch useEffect above still runs and calls /courses, /institutions, /users, and /assignments on report routes.
🛠️ Suggested fix
useEffect(() => {
+ if (isReportPage) return;
// Ensure the API fetch happens unless modals are active
if (!showDeleteConfirmation.visible || !showCopyConfirmation.visible) {
fetchCourses({ url: `/courses` });
fetchInstitutions({ url: `/institutions` });
fetchInstructors({ url: `/users` });
fetchAssignments({ url: `/assignments` });
}
}, [
+ isReportPage,
fetchCourses,
fetchInstitutions,
fetchInstructors,
fetchAssignments,
location,
showDeleteConfirmation.visible,
auth.user.id,
showCopyConfirmation.visible,
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Courses/Course.tsx` around lines 172 - 174, The Course component
currently returns <Outlet /> when isReportPage is true but the data-fetching
useEffect still runs; to fix, prevent the effect from firing on report routes by
either moving the early return for isReportPage above the useEffect so the
effect is never mounted, or add isReportPage as a guard inside the effect (e.g.,
if (isReportPage) return) before calling the fetch logic (the effect that
dispatches/fetches courses/institutions/users/assignments). Update references to
isReportPage and the existing useEffect (the function that calls
fetchCourses/fetchInstitutions/fetchUsers/fetchAssignments or dispatches those
actions) accordingly so no network calls occur for report routes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the frontend part of the topic E2612 (course reports) for the CSC517 Final Project. We have started with an empty initial commit to signify the start of this PR commit history. This will be bookended by an empty commit signifying the end of this specific feature's commit history once this feature is ready.
This PR contains the changes to the frontend to receive the data from the newly created backend controller and display it in a course-report page in a table based format.
Summary by CodeRabbit
New Features
Reports
Table
Tests