Fix "Report an issue" button on manual grading interface (#9881)

* Fix "Report an issue" button on manual grading interface * Move function to question-submission.ts to avoid circular dependency * Change order of arguments for consistency with processSubmission * Move validateVariantAgainstQuestion to variant model code * Move report issue function to issues library * Fix reference to context variable --------- Co-authored-by: Nathan Sarang-Walters <nathan@prairielearn.com>
PrairieLearn · May 23, 2024 · aa93f3e · aa93f3e
1 parent a831ebe
commit aa93f3e
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 99 deletions.
diff --git a/apps/prairielearn/src/lib/issues.ts b/apps/prairielearn/src/lib/issues.ts
@@ -1,8 +1,13 @@
 import * as async from 'async';
+import { type Request, type Response } from 'express';
+import _ from 'lodash';
 
+import { HttpStatusError } from '@prairielearn/error';
 import * as sqldb from '@prairielearn/postgres';
 import { recursivelyTruncateStrings } from '@prairielearn/sanitize';
 
+import { validateVariantAgainstQuestion } from '../models/variant.js';
+
 import { Variant } from './db-types.js';
 
 interface IssueForErrorData {
@@ -99,3 +104,39 @@ export async function writeCourseIssues(
     });
   });
 }
+export async function reportIssueFromForm(
+  req: Request,
+  res: Response,
+  studentSubmission = false,
+): Promise<string> {
+  if (studentSubmission && !res.locals.assessment.allow_issue_reporting) {
+    throw new HttpStatusError(403, 'Issue reporting not permitted for this assessment');
+  }
+  const description = req.body.description;
+  if (typeof description !== 'string' || description.length === 0) {
+    throw new HttpStatusError(400, 'A description of the issue must be provided');
+  }
+
+  const variant = await validateVariantAgainstQuestion(
+    req.body.__variant_id,
+    res.locals.question.id,
+    studentSubmission ? res.locals.instance_question?.id : null,
+  );
+  await insertIssue({
+    variantId: variant.id,
+    studentMessage: description,
+    instructorMessage: `${studentSubmission ? 'student' : 'instructor'}-reported issue`,
+    manuallyReported: true,
+    courseCaused: true,
+    courseData: _.pick(res.locals, [
+      'variant',
+      'question',
+      'course_instance',
+      'course',
+      ...(studentSubmission ? ['instance_question', 'assessment_instance', 'assessment'] : []),
+    ]),
+    systemData: {},
+    authnUserId: res.locals.authn_user.user_id,
+  });
+  return variant.id;
+}
diff --git a/apps/prairielearn/src/lib/question-submission.ts b/apps/prairielearn/src/lib/question-submission.ts
@@ -1,37 +1,11 @@
 import { type Request, type Response } from 'express';
 import _ from 'lodash';
 
-import * as error from '@prairielearn/error';
+import { HttpStatusError } from '@prairielearn/error';
 
-import { selectVariantById } from '../models/variant.js';
+import { validateVariantAgainstQuestion } from '../models/variant.js';
 
-import { type Variant } from './db-types.js';
 import { saveAndGradeSubmission, saveSubmission } from './grading.js';
-import { idsEqual } from './id.js';
-
-export async function validateVariantAgainstQuestion(
-  unsafe_variant_id: string,
-  question_id: string,
-  instance_question_id: string | null = null,
-): Promise<Variant> {
-  const variant = await selectVariantById(unsafe_variant_id);
-  if (variant == null || !idsEqual(variant.question_id, question_id)) {
-    throw new error.HttpStatusError(
-      400,
-      `Client-provided variant ID ${unsafe_variant_id} is not valid for question ID ${question_id}.`,
-    );
-  }
-  if (
-    instance_question_id != null &&
-    (!variant.instance_question_id || !idsEqual(variant.instance_question_id, instance_question_id))
-  ) {
-    throw new error.HttpStatusError(
-      400,
-      `Client-provided variant ID ${unsafe_variant_id} is not valid for instance question ID ${instance_question_id}.`,
-    );
-  }
-  return variant;
-}
 
 export async function processSubmission(
   req: Request,
@@ -44,13 +18,13 @@ export async function processSubmission(
     submitted_answer = _.omit(req.body, ['__action', '__csrf_token', '__variant_id']);
   } else {
     if (!req.body.postData) {
-      throw new error.HttpStatusError(400, 'No postData');
+      throw new HttpStatusError(400, 'No postData');
     }
     let postData;
     try {
       postData = JSON.parse(req.body.postData);
     } catch (e) {
-      throw new error.HttpStatusError(400, 'JSON parse failed on body.postData');
+      throw new HttpStatusError(400, 'JSON parse failed on body.postData');
     }
     variant_id = postData.variant ? postData.variant.id : null;
     submitted_answer = postData.submittedAnswer;
@@ -82,7 +56,7 @@ export async function processSubmission(
   // force-breaks variants, as we could be in a case where the variant wasn't
   // broken when the user loaded the page but it is broken when they submit.
   if (variant.broken_at) {
-    throw new error.HttpStatusError(403, 'Cannot submit to a broken variant');
+    throw new HttpStatusError(403, 'Cannot submit to a broken variant');
   }
 
   if (req.body.__action === 'grade') {
@@ -99,6 +73,6 @@ export async function processSubmission(
     await saveSubmission(submission, variant, res.locals.question, res.locals.course);
     return submission.variant_id;
   } else {
-    throw new error.HttpStatusError(400, `unknown __action: ${req.body.__action}`);
+    throw new HttpStatusError(400, `unknown __action: ${req.body.__action}`);
   }
 }
diff --git a/apps/prairielearn/src/models/variant.ts b/apps/prairielearn/src/models/variant.ts
@@ -1,6 +1,8 @@
+import { HttpStatusError } from '@prairielearn/error';
 import { loadSqlEquiv, queryAsync, queryOptionalRow, queryRows } from '@prairielearn/postgres';
 
 import { IdSchema, SubmissionSchema, Variant, VariantSchema } from '../lib/db-types.js';
+import { idsEqual } from '../lib/id.js';
 
 const sql = loadSqlEquiv(import.meta.url);
 
@@ -56,3 +58,26 @@ export async function selectVariantsByInstanceQuestion({
     }),
   );
 }
+export async function validateVariantAgainstQuestion(
+  unsafe_variant_id: string,
+  question_id: string,
+  instance_question_id: string | null = null,
+): Promise<Variant> {
+  const variant = await selectVariantById(unsafe_variant_id);
+  if (variant == null || !idsEqual(variant.question_id, question_id)) {
+    throw new HttpStatusError(
+      400,
+      `Client-provided variant ID ${unsafe_variant_id} is not valid for question ID ${question_id}.`,
+    );
+  }
+  if (
+    instance_question_id != null &&
+    (!variant.instance_question_id || !idsEqual(variant.instance_question_id, instance_question_id))
+  ) {
+    throw new HttpStatusError(
+      400,
+      `Client-provided variant ID ${unsafe_variant_id} is not valid for instance question ID ${instance_question_id}.`,
+    );
+  }
+  return variant;
+}
diff --git a/...rielearn/src/pages/instructorAssessmentManualGrading/instanceQuestion/instanceQuestion.ts b/...rielearn/src/pages/instructorAssessmentManualGrading/instanceQuestion/instanceQuestion.ts
@@ -1,12 +1,13 @@
 import * as express from 'express';
 import asyncHandler from 'express-async-handler';
-import * as qs from 'qs';
+import qs from 'qs';
 import { z } from 'zod';
 
 import * as error from '@prairielearn/error';
 import * as sqldb from '@prairielearn/postgres';
 
 import { IdSchema, UserSchema } from '../../../lib/db-types.js';
+import { reportIssueFromForm } from '../../../lib/issues.js';
 import * as manualGrading from '../../../lib/manualGrading.js';
 import { getAndRenderVariant, renderPanelsForSubmission } from '../../../lib/question-render.js';
 
@@ -161,6 +162,11 @@ const PostBodySchema = z.union([
       (val) => typeof val === 'string' && val.startsWith('reassign_'),
     ),
   }),
+  z.object({
+    __action: z.literal('report_issue'),
+    __variant_id: IdSchema,
+    description: z.string(),
+  }),
 ]);
 
 router.post(
@@ -263,6 +269,9 @@ router.post(
           res.locals.instance_question.id,
         ),
       );
+    } else if (body.__action === 'report_issue') {
+      await reportIssueFromForm(req, res);
+      res.redirect(req.originalUrl);
     } else {
       throw new error.HttpStatusError(400, `unknown __action: ${req.body.__action}`);
     }

diff --git a/apps/prairielearn/src/pages/instructorQuestionPreview/instructorQuestionPreview.js b/apps/prairielearn/src/pages/instructorQuestionPreview/instructorQuestionPreview.js
@@ -1,48 +1,23 @@
 // @ts-check
 import * as express from 'express';
 import asyncHandler from 'express-async-handler';
-import _ from 'lodash';
 import { z } from 'zod';
 
 import * as error from '@prairielearn/error';
 
 import { setQuestionCopyTargets } from '../../lib/copy-question.js';
 import { IdSchema } from '../../lib/db-types.js';
-import * as issues from '../../lib/issues.js';
+import { reportIssueFromForm } from '../../lib/issues.js';
 import {
   getAndRenderVariant,
   renderPanelsForSubmission,
   setRendererHeader,
 } from '../../lib/question-render.js';
-import {
-  processSubmission,
-  validateVariantAgainstQuestion,
-} from '../../lib/question-submission.js';
+import { processSubmission } from '../../lib/question-submission.js';
 import { logPageView } from '../../middlewares/logPageView.js';
 
 const router = express.Router();
 
-async function processIssue(req, res) {
-  const description = req.body.description;
-  if (!_.isString(description) || description.length === 0) {
-    throw new error.HttpStatusError(400, 'A description of the issue must be provided');
-  }
-
-  const variantId = req.body.__variant_id;
-  await validateVariantAgainstQuestion(variantId, res.locals.question.id);
-  await issues.insertIssue({
-    variantId,
-    studentMessage: description,
-    instructorMessage: 'instructor-reported issue',
-    manuallyReported: true,
-    courseCaused: true,
-    courseData: _.pick(res.locals, ['variant', 'question', 'course_instance', 'course']),
-    systemData: {},
-    authnUserId: res.locals.authn_user.user_id,
-  });
-  return variantId;
-}
-
 router.post(
   '/',
   asyncHandler(async (req, res) => {
@@ -52,7 +27,7 @@ router.post(
         `${res.locals.urlPrefix}/question/${res.locals.question.id}/preview/?variant_id=${variant_id}`,
       );
     } else if (req.body.__action === 'report_issue') {
-      const variant_id = await processIssue(req, res);
+      const variant_id = await reportIssueFromForm(req, res);
       res.redirect(
         `${res.locals.urlPrefix}/question/${res.locals.question.id}/preview/?variant_id=${variant_id}`,
       );

diff --git a/apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.js b/apps/prairielearn/src/pages/studentInstanceQuestion/studentInstanceQuestion.js
@@ -1,7 +1,6 @@
 // @ts-check
 import * as express from 'express';
 import asyncHandler from 'express-async-handler';
-import _ from 'lodash';
 
 import * as error from '@prairielearn/error';
 import * as sqldb from '@prairielearn/postgres';
@@ -12,18 +11,18 @@ import { IdSchema } from '../../lib/db-types.js';
 import { uploadFile, deleteFile } from '../../lib/file-store.js';
 import { getQuestionGroupPermissions } from '../../lib/groups.js';
 import { idsEqual } from '../../lib/id.js';
-import { insertIssue } from '../../lib/issues.js';
+import { reportIssueFromForm } from '../../lib/issues.js';
 import {
   getAndRenderVariant,
   renderPanelsForSubmission,
   setRendererHeader,
 } from '../../lib/question-render.js';
+import { processSubmission } from '../../lib/question-submission.js';
+import { logPageView } from '../../middlewares/logPageView.js';
 import {
-  processSubmission,
   validateVariantAgainstQuestion,
-} from '../../lib/question-submission.js';
-import { logPageView } from '../../middlewares/logPageView.js';
-import { selectVariantsByInstanceQuestion } from '../../models/variant.js';
+  selectVariantsByInstanceQuestion,
+} from '../../models/variant.js';
 
 const sql = sqldb.loadSqlEquiv(import.meta.url);
 
@@ -132,37 +131,6 @@ async function processDeleteFile(req, res) {
   return await getValidVariantId(req, res);
 }
 
-async function processIssue(req, res) {
-  if (!res.locals.assessment.allow_issue_reporting) {
-    throw new error.HttpStatusError(403, 'Issue reporting not permitted for this assessment');
-  }
-  const description = req.body.description;
-  if (!_.isString(description) || description.length === 0) {
-    throw new error.HttpStatusError(400, 'A description of the issue must be provided');
-  }
-
-  const variantId = await getValidVariantId(req, res);
-  await insertIssue({
-    variantId,
-    studentMessage: description,
-    instructorMessage: 'student-reported issue',
-    manuallyReported: true,
-    courseCaused: true,
-    courseData: _.pick(res.locals, [
-      'variant',
-      'instance_question',
-      'question',
-      'assessment_instance',
-      'assessment',
-      'course_instance',
-      'course',
-    ]),
-    systemData: {},
-    authnUserId: res.locals.authn_user.user_id,
-  });
-  return variantId;
-}
-
 async function validateAndProcessSubmission(req, res) {
   if (!res.locals.assessment_instance.open) {
     throw new error.HttpStatusError(400, 'assessment_instance is closed');
@@ -257,7 +225,7 @@ router.post(
         `${res.locals.urlPrefix}/instance_question/${res.locals.instance_question.id}/?variant_id=${variant_id}`,
       );
     } else if (req.body.__action === 'report_issue') {
-      const variant_id = await processIssue(req, res);
+      const variant_id = await reportIssueFromForm(req, res, true);
       res.redirect(
         `${res.locals.urlPrefix}/instance_question/${res.locals.instance_question.id}/?variant_id=${variant_id}`,
       );