Don't generate all exam questions at the start of the exam (#1359)

* remove question generation on exam start; close #848

* regenerate broken questions on exams

* add tests for brokenGeneration question on Exam

* do not log errors when restarting an exiting PythonCaller

* fix linter errors

ChangeLog.md

@@ -27,6 +27,8 @@
 
   * Change external grading to receive results from an SQS queue instead of a webhook (Nathan Walters).
 
+  * Change Exam question generation to first-access time (Matt West).
+
   * Fix load-reporting close during unit tests (Matt West).
 
   * Fix PL / scheduler linking stored procedure to allow linked exams and fix bugs (Dave Mussulman).

exampleCourse/courseInstances/Sp15/assessments/exam1/infoAssessment.json

@@ -26,7 +26,8 @@
                 {"id": "fossilFuelsRadio",        "points": [17, 13, 7, 2]},
                 {"id": "partialCredit1",          "points": [19]},
                 {"id": "partialCredit2",          "points": [9, 7, 5, 3]},
-                {"id": "partialCredit3",          "points": [13, 13, 8, 0.5, 0.1]}
+                {"id": "partialCredit3",          "points": [13, 13, 8, 0.5, 0.1]},
+                {"id": "brokenGeneration",        "points": [10]}
               ]
         }
     ],

exampleCourse/infoCourse.json

@@ -30,6 +30,7 @@
         {"name": "fa17", "color": "gray1", "description": "Question written in Fall 2017"},
         {"name": "sp18", "color": "gray1", "description": "Question written in Spring 2018"},
         {"name": "su18", "color": "gray1", "description": "Question written in Summer 2018"},
+        {"name": "fa18", "color": "gray1", "description": "Question written in Fall 2018"},
         {"name": "v2", "color": "red1", "description": "v2 generation question"},
         {"name": "v3", "color": "green1", "description": "v3 generation question (freeform)"},
         {"name": "pearl", "color": "yellow3", "description": "Question originally written by pearl"}

exampleCourse/questions/brokenGeneration/info.json

@@ -0,0 +1,7 @@
+{
+    "uuid": "a2f12dbe-e030-4782-87aa-713d61728539",
+    "title": "Broken generation function",
+    "topic": "Algebra",
+    "tags": ["mwest", "fa18", "tpl101", "v3"],
+    "type": "v3"
+}

exampleCourse/questions/brokenGeneration/question.html

@@ -0,0 +1,7 @@
+<pl-question-panel>
+  <p>What is the answer?</p>
+
+  <p><i>WARNING: This question has a deliberately broken generation function. It will give an error when attempting to generate a random question variant.</i></p>
+</pl-question-panel>
+
+<pl-integer-input answers-name="x" label="$x=$"></pl-integer-input>

exampleCourse/questions/brokenGeneration/server.py

@@ -0,0 +1,2 @@
+def generate(data):
+    raise Exception('deliberately broken generate function')

lib/assessment.js

@@ -97,18 +97,18 @@ module.exports = {
      * @param {string} mode - The mode for the new assessment instance.
      * @param {?number} time_limit_min - The time limit for the new assessment instance.
      * @param {Date} date - The date of creation for the new assessment instance.
-     * @param {?number} course_instance_id - The course instance for the new assessment.
-     * @param {Object} course - The course for the new assessment instance.
      * @param {function} callback - A callback(err, assessment_instance_id) function.
      */
-    makeAssessmentInstance(assessment_id, user_id, authn_user_id, mode, time_limit_min, date, course_instance_id, course, callback) {
-        let brokenVariants = [];
-
+    makeAssessmentInstance(assessment_id, user_id, authn_user_id, mode, time_limit_min, date, callback) {
         sqldb.beginTransaction((err, client, done) => {
             if (ERR(err, callback)) return;
 
-            var assessment_instance_id, new_instance_question_ids;
+            var assessment_instance_id;
             async.series([
+                // Even though we only have a single series function,
+                // we use the async.series pattern for consistency and
+                // to make sure we correctly call endTransaction even
+                // in the presence of errors.
                 (callback) => {
                     var params = [
                         assessment_id,
@@ -121,82 +121,13 @@ module.exports = {
                     sqldb.callWithClientOneRow(client, 'assessment_instances_insert', params, (err, result) => {
                         if (ERR(err, callback)) return;
                         assessment_instance_id = result.rows[0].assessment_instance_id;
-                        // new_instance_question_ids is empty for Homework, non-empty for Exam
-                        new_instance_question_ids = result.rows[0].new_instance_question_ids;
-                        callback(null);
-                    });
-                },
-                (callback) => {
-                    async.each(new_instance_question_ids, (instance_question_id, callback) => {
-                        const question_id = null; // use instance_question_id to determine the question
-                        const options = {};
-                        const require_open = true;
-                        question._ensureVariantWithClient(client, question_id, instance_question_id, user_id, authn_user_id, course_instance_id, course, options, require_open, (err, variant) => {
-                            if (ERR(err, callback)) return;
-                            if (variant.broken) {
-                                brokenVariants.push(variant);
-                            }
-                            callback(null);
-                        });
-                    }, (err) => {
-                        if (ERR(err, callback)) return;
-                        callback(null);
-                    });
-                },
-                (callback) => {
-                    if (brokenVariants.length == 0) return callback(null);
-                    // At least one variant is broken; unlink the
-                    // broken variants so they will persist in the DB
-                    // even after we delete the assessment_instance
-                    async.eachSeries(brokenVariants, (variant, callback) => {
-                        sqldb.callWithClient(client, 'variants_unlink', [variant.id], (err, _result) => {
-                            if (ERR(err, callback)) return;
-                            callback(null);
-                        });
-                    }, (err) => {
-                        if (ERR(err, callback)) return;
-                        callback(null);
-                    });
-                },
-                (callback) => {
-                    if (brokenVariants.length == 0) return callback(null);
-                    // At least one variant is broken, so delete the assessment instance
-                    var params = [
-                        assessment_instance_id,
-                        authn_user_id,
-                    ];
-                    sqldb.callWithClientOneRow(client, 'assessment_instances_delete', params, (err, _result) => {
-                        if (ERR(err, callback)) return;
-                        callback(null);
-                    });
-                },
-                (callback) => {
-                    if (brokenVariants.length == 0) return callback(null);
-                    // At least one variant is broken, so log an issue
-                    const params = [
-                        assessment_id,
-                        'Unable to generate exam due to a broken question', // student_message
-                        'Unable to generate exam due to a broken question', // instructor_message
-                        true, // course_caused
-                        {brokenVariants, mode, time_limit_min, course}, // course_data
-                        null, // system_data
-                        user_id,
-                        authn_user_id,
-                    ];
-                    sqldb.call('issues_insert_for_assessment', params, (err, _result) => {
-                        if (ERR(err, callback)) return;
                         callback(null);
                     });
                 },
             ], (err) => {
                 sqldb.endTransaction(client, done, err, (err) => {
                     if (ERR(err, callback)) return;
-                    if (brokenVariants.length > 0) {
-                        // At least one variant is broken, so return an error
-                        callback(error.makeWithData('unable to generate exam due to a broken question', {assessment_id, brokenVariants}));
-                    } else {
-                        callback(null, assessment_instance_id);
-                    }
+                    callback(null, assessment_instance_id);
                 });
             });
         });

lib/code-caller.js

@@ -161,18 +161,35 @@ class PythonCaller {
         debug(`exit call(), state: ${String(this.state)}, uuid: ${this.uuid}`);
     }
 
+    /*
+     * @param {function} callback - A callback(err, success) function. If 'success' is false then this PythonCaller should be discarded and a new one created by the parent.
+     */
     restart(callback) {
-        debug('enter restart()');
+        debug(`enter restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
+        this._checkState([CREATED, WAITING, EXITING, EXITED]);
+
         if (!config.useWorkers) {
+            debug(`exit restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
             return callback(new Error('cannot restart when useWorkers=false'));
         }
-        if (this.state == CREATED) return callback(null); // no need to restart if we don't have a worker
-        this.call(null, 'restart', [], {}, (err, ret_val, _consoleLog) => {
-            if (ERR(err, callback)) return;
-            if (ret_val != 'success') return callback(new Error(`Error while restarting: ${ret_val}`));
+        if (this.state == CREATED) {
+            // no need to restart if we don't have a worker
             debug(`exit restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
-            callback(null);
-        });
+            callback(null, true);
+        } else if (this.state == WAITING) {
+            this.call(null, 'restart', [], {}, (err, ret_val, _consoleLog) => {
+                if (ERR(err, callback)) return;
+                if (ret_val != 'success') return callback(new Error(`Error while restarting: ${ret_val}`));
+                debug(`exit restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
+                callback(null, true);
+            });
+        } else if (this.state == EXITING || this.state == EXITED) {
+            debug(`exit restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
+            callback(null, false);
+        } else {
+            debug(`exit restart(), state: ${String(this.state)}, uuid: ${this.uuid}`);
+            callback(new Error(`invalid state ${this.state}`));
+        }
     }
 
     done() {

lib/question.js

@@ -1162,9 +1162,7 @@ module.exports = {
         if (variant.broken) {
             locals.showGradeButton = false;
             locals.showSaveButton = false;
-            if (assessment && assessment.type == 'Homework') {
-                locals.showTryAgainButton = true;
-            }
+            locals.showTryAgainButton = true;
         }
 
         if (question && question.grading_method == 'Manual') {

lib/workers.js

@@ -108,17 +108,28 @@ module.exports = {
             return;
         }
         callback(null);
-        pc.restart((err) => {
+        pc.restart((err, success) => {
+            let needsFullRestart = false;
             if (err) {
                 debug('returnPythonCaller(): restart returned error: ${err}');
                 logger.error(`Error restarting pythonCaller: ${err}`);
+                needsFullRestart = true;
+            } else {
+                if (!success) {
+                    debug('returnPythonCaller(): restart requested a full restart');
+                    // no error logged here, everything is still ok
+                    needsFullRestart = true;
+                }
+            }
+            if (needsFullRestart) {
                 const i = pc.number;
                 pc.done();
                 debug('returnPythonCaller(): making new pythonCaller');
                 pc = new codeCaller.PythonCaller();
                 pc.number = i;
                 pythonCallers[i] = pc;
             }
+            // by this point either the restart succeeded or we have a brand new PythonCaller
             if (waitingCallbacks.length > 0) {
                 debug('returnPythonCaller(): passing to a waiting callback');
                 const cb = waitingCallbacks[0];

pages/studentAssessmentExam/studentAssessmentExam.js

@@ -33,7 +33,7 @@ router.post('/', function(req, res, next) {
     if (res.locals.assessment.type !== 'Exam') return next();
     if (!res.locals.authz_result.authorized_edit) return next(error.make(403, 'Not authorized', res.locals));
     if (req.body.__action == 'newInstance') {
-        assessment.makeAssessmentInstance(res.locals.assessment.id, res.locals.user.user_id, res.locals.authn_user.user_id, res.locals.authz_data.mode, res.locals.authz_result.time_limit_min, res.locals.req_date, res.locals.course_instance.id, res.locals.course, (err, assessment_instance_id) => {
+        assessment.makeAssessmentInstance(res.locals.assessment.id, res.locals.user.user_id, res.locals.authn_user.user_id, res.locals.authz_data.mode, res.locals.authz_result.time_limit_min, res.locals.req_date, (err, assessment_instance_id) => {
             if (ERR(err, next)) return;
             res.redirect(res.locals.urlPrefix + '/assessment_instance/' + assessment_instance_id);
         });

pages/studentAssessmentHomework/studentAssessmentHomework.js

@@ -31,7 +31,7 @@ router.get('/', function(req, res, next) {
             debug('no assessment instance');
 
             const time_limit_min = null;
-            assessment.makeAssessmentInstance(res.locals.assessment.id, res.locals.user.user_id, res.locals.authn_user.user_id, res.locals.authz_data.mode, time_limit_min, res.locals.authz_data.date, res.locals.course_instance.id, res.locals.course, (err, assessment_instance_id) => {
+            assessment.makeAssessmentInstance(res.locals.assessment.id, res.locals.user.user_id, res.locals.authn_user.user_id, res.locals.authz_data.mode, time_limit_min, res.locals.authz_data.date, (err, assessment_instance_id) => {
                 if (ERR(err, next)) return;
                 debug('redirecting');
                 res.redirect(res.locals.urlPrefix + '/assessment_instance/' + assessment_instance_id);

sprocs/assessment_instances_insert.sql

@@ -75,9 +75,7 @@ BEGIN
     IF assessment.type = 'Homework' THEN
         new_instance_question_ids := array[]::bigint[];
     ELSIF assessment.type = 'Exam' THEN
-        SELECT new_instance_question_ids
-        INTO assessment_instances_insert.new_instance_question_ids
-        FROM assessment_instances_update(assessment_instance_id, authn_user_id);
+        PERFORM assessment_instances_update(assessment_instance_id, authn_user_id);
     ELSE
         RAISE EXCEPTION 'invalid assessment.type: %', assessment.type;
     END IF;

sprocs/instance_questions_select_variant.sql

@@ -13,7 +13,7 @@ BEGIN
     WHERE
         v.instance_question_id = instance_questions_select_variant.instance_question_id
         AND (NOT require_open OR v.open)
-        AND (NOT require_open OR NOT v.broken)
+        AND NOT v.broken
     ORDER BY v.date DESC
     LIMIT 1;
 END;

tests/helperAssessment.js → tests/helperExam.js

@@ -1,17 +1,37 @@
-var ERR = require('async-stacktrace');
-var assert = require('chai').assert;
-var request = require('request');
-var cheerio = require('cheerio');
+const ERR = require('async-stacktrace');
+const _ = require('lodash');
+const assert = require('chai').assert;
+const request = require('request');
+const cheerio = require('cheerio');
 
-var config = require('../lib/config');
-var sqldb = require('@prairielearn/prairielib/sql-db');
-var sqlLoader = require('@prairielearn/prairielib/sql-loader');
-var sql = sqlLoader.loadSqlEquiv(__filename);
+const config = require('../lib/config');
+const sqldb = require('@prairielearn/prairielib/sql-db');
+const sqlLoader = require('@prairielearn/prairielib/sql-loader');
+const sql = sqlLoader.loadSqlEquiv(__filename);
 
-var res, page, elemList;
+// sorted alphabetically by qid
+const questionsArray = [
+    {qid: 'addNumbers', type: 'Freeform', maxPoints: 5},
+    {qid: 'addVectors', type: 'Calculation', maxPoints: 11},
+    {qid: 'brokenGeneration', type: 'Freeform', maxPoints: 10},
+    {qid: 'fossilFuelsRadio', type: 'Calculation', maxPoints: 17},
+    {qid: 'partialCredit1', type: 'Freeform', maxPoints: 19},
+    {qid: 'partialCredit2', type: 'Freeform', maxPoints: 9},
+    {qid: 'partialCredit3', type: 'Freeform', maxPoints: 13},
+];
+
+const questions = _.keyBy(questionsArray, 'qid');
+
+const assessmentMaxPoints = 84; // must be the sum of maxPoints in questionsArray, but we hard-code it for reference
+
+let res, page, elemList;
 
 module.exports = {
-    startExam(locals, questionsArray) {
+    questionsArray,
+    questions,
+    assessmentMaxPoints,
+
+    startExam(locals) {
         describe('startExam-1. the locals object', function() {
             it('should be cleared', function() {
                 for (var prop in locals) {

tests/testApi.js

@@ -5,24 +5,11 @@ const cheerio = require('cheerio');
 
 const helperServer = require('./helperServer');
 const helperQuestion = require('./helperQuestion');
-const helperAssessment = require('./helperAssessment');
+const helperExam = require('./helperExam');
 
 const locals = {};
 
-// sorted alphabetically by qid
-const questionsArray = [
-    {qid: 'addNumbers', type: 'Freeform', maxPoints: 5},
-    {qid: 'addVectors', type: 'Calculation', maxPoints: 11},
-    {qid: 'fossilFuelsRadio', type: 'Calculation', maxPoints: 17},
-    {qid: 'partialCredit1', type: 'Freeform', maxPoints: 19},
-    {qid: 'partialCredit2', type: 'Freeform', maxPoints: 9},
-    {qid: 'partialCredit3', type: 'Freeform', maxPoints: 13},
-];
-
-const questions = _.keyBy(questionsArray, 'qid');
-
 const assessmentPoints = 5;
-const assessmentMaxPoints = 74;
 
 describe('API', function() {
     this.timeout(60000);
@@ -32,21 +19,21 @@ describe('API', function() {
 
     let elemList, page;
 
-    helperAssessment.startExam(locals, questionsArray);
+    helperExam.startExam(locals);
 
     describe('1. grade correct answer to question addNumbers', function() {
         describe('setting up the submission data', function() {
             it('should succeed', function() {
                 locals.shouldHaveButtons = ['grade', 'save'];
                 locals.postAction = 'grade';
-                locals.question = questions.addNumbers;
+                locals.question = helperExam.questions.addNumbers;
                 locals.expectedResult = {
                     submission_score: 1,
                     submission_correct: true,
                     instance_question_points: assessmentPoints,
                     instance_question_score_perc: assessmentPoints/5 * 100,
                     assessment_instance_points: assessmentPoints,
-                    assessment_instance_score_perc: assessmentPoints/assessmentMaxPoints * 100,
+                    assessment_instance_score_perc: assessmentPoints/helperExam.assessmentMaxPoints * 100,
                 };
                 locals.getSubmittedAnswer = function(variant) {
                     return {
@@ -241,7 +228,7 @@ describe('API', function() {
         });
         it('should have the correct points', function() {
             assert.equal(locals.json[0].points, assessmentPoints);
-            assert.equal(locals.json[0].max_points, assessmentMaxPoints);
+            assert.equal(locals.json[0].max_points, helperExam.assessmentMaxPoints);
         });
     });
 
@@ -311,7 +298,7 @@ describe('API', function() {
         });
         it('should have the correct points', function() {
             assert.equal(locals.gradebookEntry.points, assessmentPoints);
-            assert.equal(locals.gradebookEntry.max_points, assessmentMaxPoints);
+            assert.equal(locals.gradebookEntry.max_points, helperExam.assessmentMaxPoints);
         });
     });
 });