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

feat: preview assessment from assessments page #604

Open
wants to merge 5 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const AssessmentEditorHeader = ({
const onPreview = () => {
validateForm();
disableEditorPrompt(history.push)(
Routes.ASSESSMENT_EDITOR_PREVIEW_PAGE({
Routes.ASSESSMENT_PREVIEW_PAGE({
assessmentId,
}),
);
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React from "react";
import { useHistory } from "react-router-dom";
import { useQuery } from "@apollo/client";

import { GET_TEST } from "../../../../APIClients/queries/TestQueries";
import type { TestResponse } from "../../../../APIClients/types/TestClientTypes";
import * as Routes from "../../../../constants/Routes";
import useToast from "../../../common/info/useToast";
import PopoverButton from "../../../common/popover/PopoverButton";

interface PreviewButtonProps {
assessmentId: string;
}

const PreviewButton = ({
assessmentId,
}: PreviewButtonProps): React.ReactElement => {
const history = useHistory();
const { data } = useQuery<{ test: TestResponse }>(GET_TEST, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc the tests table already loads this data for each row. Is there any way you could reuse that instead? Otherwise each user will be hammering the backend twice for each test that exists in the database

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, but does it? Because the table runs the GET_ALL_TESTS query, right? Which is all the test metadata without the specific questions.

Here, we call GET_TEST which queries for the test metadata + questions for one specific test. I think this is similar to the implementation of EditButton.

Let me know if I am misunderstanding your comment here!

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I'm wondering if it would be better to just add the additional data to the GET_ALL_TESTS query? Since imho our concern here might not be bandwidth but compute scale, since we have a single-replica base tier backend

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm also thinking that EditButton could use a different implementation

fetchPolicy: "cache-and-network",
variables: { id: assessmentId },
});
const { showToast } = useToast();

return (
<PopoverButton
name="Preview"
onClick={() => {
if (data) {
history.push({
pathname: Routes.ASSESSMENT_PREVIEW_PAGE({ assessmentId }),
state: data.test,
});
} else {
showToast({
message:
"This assessment cannot be previewed at this time. Please try again.",
status: "error",
});
}
}}
/>
);
};

export default PreviewButton;
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import ArchiveButton from "./EditStatusButtons/ArchiveButton";
import DeleteButton from "./EditStatusButtons/DeleteButton";
import DuplicateButton from "./EditStatusButtons/DuplicateButton";
import EditButton from "./EditStatusButtons/EditButton";
import PreviewButton from "./EditStatusButtons/PreviewButton";
import PublishButton from "./EditStatusButtons/PublishButton";
import UnarchiveButton from "./EditStatusButtons/UnarchiveButton";

Expand Down Expand Up @@ -38,6 +39,7 @@ const EditStatusPopover = ({
<DuplicateButton assessmentId={assessmentId} />
</>
)}
<PreviewButton assessmentId={assessmentId} />
<DeleteButton assessmentId={assessmentId} />
</VStack>
</Popover>
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/components/pages/admin/AdminRouting.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Navbar from "../../common/navigation/Navbar";
import NotFound from "../NotFound";

import AssessmentEditorPage from "./AssessmentEditorPage";
import AssessmentPreviewPage from "./AssessmentPreviewPage";
import DisplayAssessmentsPage from "./DisplayAssessmentsPage";
import UsersPage from "./UsersPage";

Expand All @@ -33,6 +34,13 @@ const AdminRouting = (): React.ReactElement => {
path={Routes.ASSESSMENT_EDITOR_BASE({ assessmentId: ":assessmentId" })}
roles={["Admin"]}
/>
<PrivateRoute
component={AssessmentPreviewPage}
path={Routes.ASSESSMENT_PREVIEW_PAGE({
assessmentId: ":assessmentId",
})}
roles={["Admin"]}
/>
<Route path="*">
<VStack align="left" flex="1" height="100vh">
<Navbar pages={pages} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import type { Question } from "../../../../types/QuestionTypes";
import { FormValidationError } from "../../../../utils/GeneralUtils";
import { formatQuestionsRequest } from "../../../../utils/QuestionUtils";
import AssessmentEditorHeader from "../../../admin/assessment-creation/AssessmentEditorHeader";
import AssessmentPreview from "../../../admin/assessment-creation/AssessmentPreview";
import AssessmentQuestions from "../../../admin/assessment-creation/AssessmentQuestions";
import BasicInformation from "../../../admin/assessment-creation/BasicInformation";
import QuestionEditor from "../../../admin/question-creation/QuestionEditor";
Expand Down Expand Up @@ -272,22 +271,6 @@ const AssessmentEditor = ({ state }: AssessmentEditorProps): ReactElement => {
</VStack>
</VStack>
</Route>
<Route
Copy link
Contributor

@jfdoming jfdoming Oct 25, 2023

Choose a reason for hiding this comment

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

Wondering why this change to remove the routes here was necessary? I think this might be the cause of the routing woes from your other comment. In particular, my understanding is that the AssessmentEditor component "owns" the route /admin/assessments/, which makes it capture all sub-routes. We could potentially rename the "owner" component to something more generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfdoming I can't recall exactly, but we resolved this discussion synchronously right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know something wasn't working that did require a code change. Did you commit that change?

path={Routes.ASSESSMENT_EDITOR_PREVIEW_PAGE({
assessmentId: state?.id && ":assessmentId",
})}
>
<AssessmentPreview
goBack={() =>
disableEditorPrompt(history.push)(
Routes.ASSESSMENT_EDITOR_PAGE({
assessmentId: state?.id,
}),
)
}
questions={questions}
/>
</Route>
<Route component={NotFound} exact path="*" />
</Switch>
</FormProvider>
Expand Down
71 changes: 71 additions & 0 deletions frontend/src/components/pages/admin/AssessmentPreviewPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React, { useMemo } from "react";
import { useLocation, useParams } from "react-router-dom";
import { useHistory } from "react-router-dom";
import { useQuery } from "@apollo/client";
import { Box, Button } from "@chakra-ui/react";

import { GET_TEST } from "../../../APIClients/queries/TestQueries";
import type { TestResponse } from "../../../APIClients/types/TestClientTypes";
import { ArrowBackOutlineIcon } from "../../../assets/icons";
import { formatQuestionsResponse } from "../../../utils/QuestionUtils";
import usePageTitle from "../../auth/usePageTitle";
import QueryStateHandler from "../../common/QueryStateHandler";
import AssessmentExperience from "../../student/AssessmentExperience";

const AssessmentPreviewPage = () => {
const history = useHistory();

// Data could come from the previous page.
const { state: locationState } = useLocation<TestResponse | undefined>();

// If data is not available from the previous page, then we have to fetch it.
const { assessmentId } = useParams<{ assessmentId?: string }>();
const {
data: testData,
loading,
error,
} = useQuery<{
test: TestResponse;
}>(GET_TEST, {
variables: { id: assessmentId },
skip: !!locationState || !assessmentId,
});

const test = locationState || testData?.test;
const state = useMemo(
() =>
test && {
...test,
questions: formatQuestionsResponse(test.questions),
},
[test],
);

const assessmentName = state?.name;
usePageTitle(`Previewing "${assessmentName || "assesssment"}`);

const closeAssessmentPreviewButton = (
<Button
leftIcon={<ArrowBackOutlineIcon />}
onClick={() => history.goBack()}
variant="tertiary"
>
Back
</Button>
);

return (
<Box mx={4}>
<QueryStateHandler error={error} loading={loading}>
<AssessmentExperience
headerButton={closeAssessmentPreviewButton}
isPreviewMode
questions={state?.questions || []}
title="Preview Assessment"
/>
</QueryStateHandler>
</Box>
);
};

export default AssessmentPreviewPage;
11 changes: 6 additions & 5 deletions frontend/src/constants/Routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,6 @@ export const ASSESSMENT_EDITOR_PAGE = ({
}: {
assessmentId?: string;
}) => ASSESSMENT_EDITOR_BASE({ assessmentId }) + (assessmentId ? "/edit" : "");
export const ASSESSMENT_EDITOR_PREVIEW_PAGE = ({
assessmentId,
}: {
assessmentId?: string;
}) => ASSESSMENT_EDITOR_BASE({ assessmentId }) + "/preview";
export const ASSESSMENT_EDITOR_QUESTION_EDITOR_BASE = ({
assessmentId,
questionIndex,
Expand Down Expand Up @@ -55,6 +50,12 @@ export const ASSESSMENT_EDITOR_QUESTION_PREVIEW_PAGE = ({
ASSESSMENT_EDITOR_QUESTION_EDITOR_BASE({ assessmentId, questionIndex }) +
"/preview";

export const ASSESSMENT_PREVIEW_PAGE = ({
assessmentId,
}: {
assessmentId?: string;
}) => "/admin/assessment/" + assessmentId + "/preview";
Copy link
Contributor Author

@joyce-shi joyce-shi Oct 24, 2023

Choose a reason for hiding this comment

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

I think it would be kind of nice to overload the route here... i.e /admin/assessments/ + assessmentId + /preview, but I can't figure out why that isn't working & doesn't seem like a big enough issue to be a blocker... let me know if you have thoughts (:

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah routing is tricky for this page... Happy to pair at some point if that would help. Or I can look into what's happening later this week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah, we can pair on Sunday? This PR isn't a blocker so I don't mind leaving it open

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, we resolved this synchronously right? 😅


// Private Teacher Routes
export const TEACHER_LANDING_PAGE = "/teacher";
export const TEACHER_DASHBOARD_PAGE = "/teacher/dashboard";
Expand Down
Loading