-
Notifications
You must be signed in to change notification settings - Fork 113
E2612 - course based reports #170
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
Open
kamatmihir2002
wants to merge
11
commits into
expertiza:main
Choose a base branch
from
ravisatyarsg:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b6df474
E2612 - empty init commit
kamatmihir2002 5b4c098
E2612: Add course-based report page initial test
ravisatyarsg 503b0ee
E2612: Fix brown report button and hide courses table on report page
ravisatyarsg d7b2117
Separate class avg row and disable pagination on course reports
kamatmihir2002 e5341db
E2612: Added 40 tests to the frontend.
b80099f
Add average author feedback scores
kamatmihir2002 bb07872
rectify avg calculation
kamatmihir2002 dcf4a5e
Change course report endpoint
kamatmihir2002 7660d0f
Merge pull request #1 from ravisatyarsg/feat/course-based-reports
kamatmihir2002 6bc8e84
E2612: Add descriptive comments to service and types files
a857314
E2612: Format class average values to 2 decimal places
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| import "@testing-library/jest-dom"; | ||
| import { render, screen, waitFor } from "@testing-library/react"; | ||
| import Courses from "./Course"; | ||
|
|
||
| const mockUseLocation = vi.fn(); | ||
| const mockNavigate = vi.fn(); | ||
| const mockDispatch = vi.fn(); | ||
| const mockUseAPI = vi.fn(); | ||
| const mockTable = vi.fn(); | ||
|
|
||
| vi.mock("react-router-dom", async (importOriginal) => ({ | ||
| ...(await importOriginal<typeof import("react-router-dom")>()), | ||
| useNavigate: () => mockNavigate, | ||
| useLocation: () => mockUseLocation(), | ||
| Outlet: () => <div data-testid="courses-outlet" />, | ||
| })); | ||
|
|
||
| vi.mock("react-redux", () => ({ | ||
| useDispatch: () => mockDispatch, | ||
| useSelector: (selector: any) => | ||
| selector({ | ||
| authentication: { | ||
| isAuthenticated: true, | ||
| user: { | ||
| id: 7, | ||
| full_name: "Taylor Teach", | ||
| role: "Instructor", | ||
| }, | ||
| }, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock("../../hooks/useAPI", () => ({ | ||
| default: () => mockUseAPI(), | ||
| })); | ||
|
|
||
| vi.mock("components/Table/Table", () => ({ | ||
| default: (props: any) => { | ||
| mockTable(props); | ||
| return <div data-testid="courses-table" />; | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("./CourseDelete", () => ({ | ||
| default: () => <div>Delete Course Modal</div>, | ||
| })); | ||
|
|
||
| vi.mock("./CourseCopy", () => ({ | ||
| default: () => <div>Copy Course Modal</div>, | ||
| })); | ||
|
|
||
| vi.mock("./CourseAssignments", () => ({ | ||
| default: () => <div>Course Assignments</div>, | ||
| })); | ||
|
|
||
| const setUseApiSequence = () => { | ||
| mockUseAPI | ||
| .mockReturnValueOnce({ | ||
| error: "", | ||
| isLoading: false, | ||
| data: { | ||
| data: [ | ||
| { | ||
| id: 1, | ||
| name: "CSC 517", | ||
| directory_path: "csc517", | ||
| info: "Course info", | ||
| private: true, | ||
| created_at: "2024-01-01T00:00:00.000Z", | ||
| updated_at: "2024-01-02T00:00:00.000Z", | ||
| institution_id: 1, | ||
| instructor_id: 7, | ||
| institution: { id: 1, name: "NCSU" }, | ||
| instructor: { id: 7, name: "Taylor Teach" }, | ||
| date_format_pref: "MM/DD/YYYY", | ||
| }, | ||
| ], | ||
| }, | ||
| sendRequest: vi.fn(), | ||
| }) | ||
| .mockReturnValueOnce({ | ||
| data: { data: [{ id: 1, name: "NCSU" }] }, | ||
| sendRequest: vi.fn(), | ||
| }) | ||
| .mockReturnValueOnce({ | ||
| data: { data: [{ id: 7, name: "Taylor Teach" }] }, | ||
| sendRequest: vi.fn(), | ||
| }) | ||
| .mockReturnValueOnce({ | ||
| data: { data: [{ course_id: 1 }] }, | ||
| sendRequest: vi.fn(), | ||
| }); | ||
| }; | ||
|
|
||
| describe("Courses report route behavior", () => { | ||
| beforeEach(() => { | ||
| mockUseLocation.mockReturnValue({ | ||
| pathname: "/courses", | ||
| search: "", | ||
| hash: "", | ||
| }); | ||
| mockUseAPI.mockReset(); | ||
| mockTable.mockReset(); | ||
| setUseApiSequence(); | ||
| }); | ||
|
|
||
| it("renders only the outlet when the current path is the course report page", () => { | ||
| mockUseLocation.mockReturnValue({ | ||
| pathname: "/courses/1/class_assignment_overview", | ||
| search: "", | ||
| hash: "", | ||
| }); | ||
|
|
||
| render(<Courses />); | ||
|
|
||
| expect(screen.getByTestId("courses-outlet")).toBeInTheDocument(); | ||
| expect(screen.queryByTestId("courses-table")).not.toBeInTheDocument(); | ||
| expect(screen.queryByText(/Manage Courses/i)).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders the normal courses page when the current path is not the report page", () => { | ||
| render(<Courses />); | ||
|
|
||
| expect(screen.getByTestId("courses-outlet")).toBeInTheDocument(); | ||
| expect(screen.getByTestId("courses-table")).toBeInTheDocument(); | ||
| expect(screen.getByText("Instructed by: Taylor Teach")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("passes course rows to the shared table on the normal courses page", async () => { | ||
| render(<Courses />); | ||
|
|
||
| await waitFor(() => expect(mockTable).toHaveBeenCalledTimes(1)); | ||
| expect(mockTable.mock.calls[0][0].data).toHaveLength(1); | ||
| expect(mockTable.mock.calls[0][0].data[0].name).toBe("CSC 517"); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,8 @@ const Courses = () => { | |
| const currUserRole = auth.user.role.valueOf(); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const dispatch = useDispatch(); | ||
| const dispatch = useDispatch(); | ||
| const isReportPage = location.pathname.includes("class_assignment_overview"); | ||
|
|
||
| const [showDeleteConfirmation, setShowDeleteConfirmation] = useState<{ | ||
| visible: boolean; | ||
|
|
@@ -96,25 +97,30 @@ const Courses = () => { | |
| [] | ||
| ); | ||
|
|
||
| const onCopyHandle = useCallback( | ||
| (row: TRow<ICourseResponse>) => | ||
| setShowCopyConfirmation({ visible: true, data: row.original }), | ||
| [] | ||
| ); | ||
|
|
||
| const renderSubComponent = useCallback(({ row }: { row: TRow<ICourseResponse> }) => { | ||
| return ( | ||
| const onCopyHandle = useCallback( | ||
| (row: TRow<ICourseResponse>) => | ||
| setShowCopyConfirmation({ visible: true, data: row.original }), | ||
| [] | ||
| ); | ||
|
|
||
| const onReportHandle = useCallback( | ||
| (row: TRow<ICourseResponse>) => navigate(`${row.original.id}/class_assignment_overview`), | ||
| [navigate] | ||
| ); | ||
|
|
||
| const renderSubComponent = useCallback(({ row }: { row: TRow<ICourseResponse> }) => { | ||
| return ( | ||
| <CourseAssignments | ||
| courseId={row.original.id} | ||
| courseName={row.original.name} | ||
| /> | ||
| ); | ||
| }, []); | ||
| const tableColumns = useMemo( | ||
| () => COURSE_COLUMNS(onEditHandle, onDeleteHandle, onTAHandle, onCopyHandle), | ||
| [onDeleteHandle, onEditHandle, onTAHandle, onCopyHandle] | ||
| ); | ||
|
|
||
| const tableColumns = useMemo( | ||
| () => COURSE_COLUMNS(onEditHandle, onDeleteHandle, onTAHandle, onCopyHandle, onReportHandle), | ||
| [onDeleteHandle, onEditHandle, onTAHandle, onCopyHandle, onReportHandle] | ||
| ); | ||
|
|
||
| const tableData = useMemo( | ||
| () => (isLoading || !CourseResponse?.data ? [] : CourseResponse.data), | ||
|
|
@@ -158,14 +164,18 @@ const renderSubComponent = useCallback(({ row }: { row: TRow<ICourseResponse> }) | |
| ); | ||
| }, [mergedTableData, loggedInUserRole]); | ||
|
|
||
| const coursesWithAssignments = useMemo(() => { | ||
| if (!assignmentResponse?.data) return new Set(); | ||
| return new Set(assignmentResponse.data.map((a: any) => a.course_id)); | ||
| }, [assignmentResponse?.data]); | ||
|
|
||
| return ( | ||
| <> | ||
| <Outlet /> | ||
| const coursesWithAssignments = useMemo(() => { | ||
| if (!assignmentResponse?.data) return new Set(); | ||
| return new Set(assignmentResponse.data.map((a: any) => a.course_id)); | ||
| }, [assignmentResponse?.data]); | ||
|
|
||
| if (isReportPage) { | ||
| return <Outlet />; | ||
| } | ||
|
Comment on lines
+172
to
+174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Report-route short-circuit still triggers unnecessary course-page API fetches. Although Line 172 returns only 🛠️ 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 |
||
|
|
||
| return ( | ||
| <> | ||
| <Outlet /> | ||
| <main> | ||
| <Container fluid className="px-md-4"> | ||
| <Row className="mt-4 mb-4"> | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import "@testing-library/jest-dom"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { courseColumns } from "./CourseColumns"; | ||
|
|
||
| const mockRow = { | ||
| original: { | ||
| id: 15, | ||
| name: "CSC 517", | ||
| institution: { id: 1, name: "NCSU" }, | ||
| instructor: { id: 2, name: "Prof. Lane" }, | ||
| created_at: "2024-01-01T00:00:00.000Z", | ||
| updated_at: "2024-01-02T00:00:00.000Z", | ||
| }, | ||
| } as any; | ||
|
|
||
| describe("courseColumns report action", () => { | ||
| const handleEdit = vi.fn(); | ||
| const handleDelete = vi.fn(); | ||
| const handleTA = vi.fn(); | ||
| const handleCopy = vi.fn(); | ||
| const handleReport = vi.fn(); | ||
|
|
||
| const renderActionsCell = () => { | ||
| const columns = courseColumns( | ||
| handleEdit, | ||
| handleDelete, | ||
| handleTA, | ||
| handleCopy, | ||
| handleReport | ||
| ); | ||
| const actionsColumn = columns.find((column: any) => column.id === "actions") as any; | ||
|
|
||
| return render(actionsColumn.cell({ row: mockRow })); | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| handleEdit.mockReset(); | ||
| handleDelete.mockReset(); | ||
| handleTA.mockReset(); | ||
| handleCopy.mockReset(); | ||
| handleReport.mockReset(); | ||
| }); | ||
|
|
||
| it("renders the report action with the existing tooltip text", async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| renderActionsCell(); | ||
| const reportButton = screen.getByRole("button", { name: "View Course Report" }); | ||
|
|
||
| await user.hover(reportButton); | ||
|
|
||
| expect(await screen.findByText("View Course Report")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("calls handleReport with the clicked row", async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| renderActionsCell(); | ||
| await user.click(screen.getByRole("button", { name: "View Course Report" })); | ||
|
|
||
| expect(handleReport).toHaveBeenCalledWith(mockRow); | ||
| }); | ||
|
|
||
| it("renders the report action as a brown rectangular button instead of a link-style icon button", () => { | ||
| renderActionsCell(); | ||
| const reportButton = screen.getByRole("button", { name: "View Course Report" }); | ||
|
|
||
| expect(reportButton).toHaveStyle({ | ||
| backgroundColor: "#8B4513", | ||
| borderColor: "#8B4513", | ||
| width: "25px", | ||
| height: "25px", | ||
| }); | ||
| expect(reportButton).not.toHaveClass("btn-link"); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
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.
🧩 Analysis chain
🏁 Script executed:
Repository: expertiza/reimplementation-front-end
Length of output: 164
Remove the duplicate
disablePaginationRowModelproperty declaration inTableProps.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
🤖 Prompt for AI Agents