MDL-20636 Fix 30 TODOs
authorTim Hunt <T.J.Hunt@open.ac.uk>
Thu, 24 Feb 2011 17:47:51 +0000 (17:47 +0000)
committerTim Hunt <T.J.Hunt@open.ac.uk>
Thu, 24 Feb 2011 17:47:51 +0000 (17:47 +0000)
42 files changed:
lang/en/question.php
lib/questionlib.php
mod/quiz/comment.php
mod/quiz/lang/en/quiz.php
mod/quiz/lib.php
mod/quiz/locallib.php
mod/quiz/report/attemptsreport.php
mod/quiz/report/overview/overviewsettings_form.php
mod/quiz/report/overview/report.php
mod/quiz/report/reportlib.php
mod/quiz/report/responses/report.php
mod/quiz/report/responses/responsessettings_form.php
mod/quiz/reviewquestion.php
mod/quiz/view.php
question/behaviour/behaviourbase.php
question/behaviour/missing/simpletest/testmissingbehaviour.php
question/behaviour/opaque/behaviour.php
question/editlib.php
question/engine/bank.php
question/engine/datalib.php
question/engine/lib.php
question/engine/renderer.php
question/engine/simpletest/helpers.php
question/engine/simpletest/testquestionattempt.php
question/engine/simpletest/testquestionattemptstepiterator.php
question/format.php
question/format/qti_two/format.php
question/import.php
question/type/calculated/backup/moodle2/backup_qtype_calculated_plugin.class.php
question/type/calculated/questiontype.php
question/type/calculatedmulti/questiontype.php
question/type/calculatedsimple/questiontype.php
question/type/missingtype/simpletest/testmissingtype.php
question/type/multichoice/backup/moodle2/backup_qtype_multichoice_plugin.class.php
question/type/multichoice/question.php
question/type/multichoice/styles.css
question/type/numerical/backup/moodle2/backup_qtype_numerical_plugin.class.php
question/type/numerical/questiontype.php
question/type/questionbase.php
question/type/random/questiontype.php
question/type/randomsamatch/questiontype.php
question/type/rendererbase.php

index fbfb405..bacdbae 100644 (file)
@@ -178,6 +178,7 @@ $string['movedquestionsandcategories'] = 'Moved questions and question categorie
 $string['movelinksonly'] = 'Just change where links point to, do not move or copy files.';
 $string['moveq'] = 'Move question(s)';
 $string['moveqtoanothercontext'] = 'Move question to another context.';
+$string['moveto'] = 'Move to >>';
 $string['movingcategory'] = 'Moving category';
 $string['movingcategoryandfiles'] = 'Are you sure you want to move category {$a->name} and all child categories to context for "{$a->contextto}"?<br /> We have detected {$a->urlcount} files linked from questions in {$a->fromareaname}, would you like to copy or move these to {$a->toareaname}?';
 $string['movingcategorynofiles'] = 'Are you sure you want to move category "{$a->name}" and all child categories to context for "{$a->contextto}"?';
@@ -347,5 +348,6 @@ $string['submitted'] = 'Submit: {$a}';
 $string['unknownquestion'] = 'Unknown question: {$a}.';
 $string['unknownquestioncatregory'] = 'Unknown question category: {$a}.';
 $string['whethercorrect'] = 'Whether correct';
+$string['withselected'] = 'With selected';
 $string['xoutofmax'] = '{$a->mark} out of {$a->max}';
 $string['yougotnright'] = 'You have correctly selected {$a->num}.';
index 410d1e4..2292117 100644 (file)
@@ -777,7 +777,7 @@ function question_load_questions($questionids, $extrafields = '', $join = '') {
  */
 function _tidy_question($question, $loadtags = false) {
     global $CFG;
-    if (question_bank::is_qtype_installed($question->qtype)) {
+    if (!question_bank::is_qtype_installed($question->qtype)) {
         $question->questiontext = html_writer::tag('p', get_string('warningmissingtype',
                 'qtype_missingtype')) . $question->questiontext;
     }
@@ -1153,18 +1153,18 @@ function question_add_tops($categories, $pcontexts){
 }
 
 /**
- * Returns a comma separated list of ids of the category and all subcategories
+ * @return array of question category ids of the category and all subcategories.
  */
 function question_categorylist($categoryid) {
     global $DB;
 
-    // returns a comma separated list of ids of the category and all subcategories
-    $categorylist = $categoryid;
-    if ($subcategories = $DB->get_records('question_categories', array('parent' => $categoryid), 'sortorder ASC', 'id, 1')) {
-        foreach ($subcategories as $subcategory) {
-            $categorylist .= ','. question_categorylist($subcategory->id);
-        }
+    $subcategories = $DB->get_records('question_categories', array('parent' => $categoryid), 'sortorder ASC', 'id, 1');
+
+    $categorylist = array($categoryid);
+    foreach ($subcategories as $subcategory) {
+        $categorylist = array_merge($categorylist, question_categorylist($subcategory->id));
     }
+
     return $categorylist;
 }
 
index e752fae..1f5271c 100644 (file)
@@ -57,7 +57,6 @@ echo $OUTPUT->heading(format_string($attemptobj->get_question_name($slot)));
 // Process any data that was submitted.
 if (data_submitted() && confirm_sesskey()) {
     if (optional_param('submit', false, PARAM_BOOL)) {
-        // TODO better error handling.
         $transaction = $DB->start_delegated_transaction();
         $attemptobj->process_all_actions(time());
         $transaction->allow_commit();
index 604f841..4731b4f 100644 (file)
@@ -420,7 +420,6 @@ $string['modulename'] = 'Quiz';
 $string['modulename_help'] = 'The quiz module enables the teacher to design and set quizzes consisting of multiple choice, true-false, matching and other question types. Each attempt is automatically marked, and the teacher can choose whether to give feedback and/or show correct answers.';
 $string['modulenameplural'] = 'Quizzes';
 $string['moveselectedonpage'] = 'Move selected questions to page: {$a}';
-$string['moveto'] = 'Move to >>';
 $string['multianswer'] = 'Embedded answers (cloze)';
 $string['multichoice'] = 'Multiple choice';
 $string['multipleanswers'] = 'Choose at least one answer.';
@@ -789,7 +788,6 @@ $string['warningmissingtype'] = '<b>This question is of a type that has not been
 $string['wheregrade'] = 'Where\'s my grade?';
 $string['wildcard'] = 'Wild card';
 $string['windowclosing'] = 'This window will close shortly.';
-$string['withselected'] = 'With selected';
 $string['withsummary'] = 'with summary statistics';
 $string['wronguse'] = 'You can not use this page like that';
 $string['xhtml'] = 'XHTML';
index a3dc853..0d0871b 100644 (file)
@@ -305,9 +305,8 @@ function quiz_update_effective_access($quiz, $userid) {
 function quiz_delete_all_attempts($quiz) {
     global $CFG, $DB;
     require_once($CFG->libdir . '/questionlib.php');
-    // TODO remove $CFG->prefix
-    question_engine::delete_questions_usage_by_activities("{$CFG->prefix}question_usages.id IN (
-            SELECT uniqueid FROM {$CFG->prefix}quiz_attempts WHERE quiz = $quizid)");
+    question_engine::delete_questions_usage_by_activities("{question_usages}.id IN (
+            SELECT uniqueid FROM {quiz_attempts} WHERE quiz = :quizid)", array('quizid' => $quizid));
     $DB->delete_records('quiz_attempts', array('quiz' => $quiz->id));
     $DB->delete_records('quiz_grades', array('quiz' => $quiz->id));
 }
@@ -1232,7 +1231,7 @@ function quiz_reset_userdata($data) {
                 SELECT uniqueid
                 FROM {quiz_attempts} quiza
                 JOIN {quiz} quiz ON quiza.quiz = quiz.id
-                WHERE quiz.course = ?)', array($data->courseid));
+                WHERE quiz.course = :courseid)', array('courseid' => $data->courseid));
 
         $DB->delete_records_select('quiz_attempts',
                 'quiz IN (SELECT id FROM {quiz} WHERE course = ?)', array($data->courseid));
index 76b2630..bef0e2b 100644 (file)
@@ -217,21 +217,6 @@ function quiz_has_attempts($quizid) {
 
 /// Functions to do with quiz layout and pages ////////////////////////////////
 
-/**
- * Returns a comma separated list of question ids for the current page
- *
- * @param string $layout the string representing the quiz layout. Each page is represented as a
- *      comma separated list of question ids and 0 indicating page breaks.
- *      So 5,2,0,3,0 means questions 5 and 2 on page 1 and question 3 on page 2
- * @param int $page the number of the current page.
- * @return string comma separated list of question ids
- */
-function quiz_questions_on_page($layout, $page) {
-    $pages = explode(',0', $layout);
-    // TODO is this still used.
-    return trim($pages[$page], ',');
-}
-
 /**
  * Returns a comma separated list of question ids for the quiz
  *
@@ -267,7 +252,6 @@ function quiz_number_of_pages($layout) {
  * @return int The number of questions in the quiz.
  */
 function quiz_number_of_questions_in_quiz($layout) {
-    // TODO is this still used.
     $layout = quiz_questions_in_quiz(quiz_clean_layout($layout));
     $count = substr_count($layout, ',');
     if ($layout !== '') {
@@ -276,29 +260,6 @@ function quiz_number_of_questions_in_quiz($layout) {
     return $count;
 }
 
-/**
- * Returns the first question number for the current quiz page
- *
- * @param string $quizlayout The string representing the layout for the whole quiz
- * @param string $pagelayout The string representing the layout for the current page
- * @return int the number of the first question
- */
-function quiz_first_questionnumber($quizlayout, $pagelayout) {
-    // TODO is this still used.
-    // this works by finding all the questions from the quizlayout that
-    // come before the current page and then adding up their lengths.
-    global $CFG, $DB;
-    $start = strpos($quizlayout, ','.$pagelayout.',')-2;
-    if ($start > 0) {
-        $prevlist = substr($quizlayout, 0, $start);
-        list($usql, $params) = $DB->get_in_or_equal(explode(',', $prevlist));
-        return $DB->get_field_sql("SELECT sum(length)+1 FROM {question}
-         WHERE id $usql", $params);
-    } else {
-        return 1;
-    }
-}
-
 /**
  * Re-paginates the quiz layout
  *
index b0e2226..55e0b5c 100644 (file)
@@ -342,8 +342,10 @@ abstract class quiz_attempt_report extends quiz_default_report {
      *      Empty means all users.
      */
     protected function delete_selected_attempts($quiz, $cm, $attemptids, $allowed) {
+        global $DB;
+
         foreach ($attemptids as $attemptid) {
-            $attempt = get_record('quiz_attempts', 'id', $attemptid);
+            $attempt = $DB->get_record('quiz_attempts', array('id' => $attemptid));
             if (!$attempt || $attempt->quiz != $quiz->id || $attempt->preview != 0) {
                 // Ensure the attempt exists, and belongs to this quiz. If not skip.
                 continue;
@@ -610,8 +612,8 @@ abstract class quiz_attempt_report_table extends table_sql {
 
         $this->sql->fields .= ",\n$fields";
         $this->sql->from .= "\nLEFT JOIN $inlineview ON " .
-                "$alias.questionusageid = quiza.uniqueid AND $alias.slot = $slot";
-        // TODO params
+                "$alias.questionusageid = quiza.uniqueid AND $alias.slot = :{$alias}slot";
+        $this->sql->params[$alias . 'slot'] = $slot;
     }
 
     /**
index 1f1a2bb..9dff2e5 100644 (file)
@@ -38,8 +38,7 @@ require_once($CFG->libdir . '/formslib.php');
  */
 class mod_quiz_report_overview_settings extends moodleform {
 
-    function definition() {
-        global $COURSE; // TODO get rid of this.
+    public function definition() {
         $mform = $this->_form;
 
         $mform->addElement('header', 'preferencespage', get_string('preferencespage', 'quiz_overview'));
@@ -60,7 +59,7 @@ class mod_quiz_report_overview_settings extends moodleform {
         if (!$this->_customdata['currentgroup']) {
             $options[QUIZ_REPORT_ATTEMPTS_ALL] = get_string('optallattempts','quiz_overview');
         }
-        if ($this->_customdata['currentgroup'] || $COURSE->id != SITEID) {
+        if ($this->_customdata['currentgroup'] || !is_inside_frontpage($this->_customdata['context'])) {
             $options[QUIZ_REPORT_ATTEMPTS_ALL_STUDENTS] = get_string('optallstudents','quiz_overview', $studentsstring);
             $options[QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH] =
                      get_string('optattemptsonly','quiz_overview', $studentsstring);
index 0323ebc..59ea18a 100644 (file)
@@ -58,7 +58,8 @@ class quiz_overview_report extends quiz_attempt_report {
         $qmsubselect = quiz_report_qm_filter_select($quiz);
 
         $mform = new mod_quiz_report_overview_settings($reporturl,
-                array('qmsubselect' => $qmsubselect, 'quiz' => $quiz, 'currentgroup' => $currentgroup, 'context'=>$this->context));
+                array('qmsubselect' => $qmsubselect, 'quiz' => $quiz,
+                'currentgroup' => $currentgroup, 'context' => $this->context));
 
         if ($fromform = $mform->get_data()) {
             $regradeall = false;
index 93da23f..2006671 100644 (file)
@@ -188,15 +188,15 @@ function quiz_report_grade_bands($bandwidth, $bands, $quizid, $userids = array()
     global $DB;
 
     if ($userids) {
-        list($usql, $params) = $DB->get_in_or_equal($userids);
+        list($usql, $params) = $DB->get_in_or_equal($userids, SQL_PARAMS_NAMED, 'u000000');
         $usql = "qg.userid $usql AND";
     } else {
-        $usql ='';
+        $usql = '';
         $params = array();
     }
     $sql = "
 SELECT
-    FLOOR(qg.grade/$bandwidth) AS band,
+    FLOOR(qg.grade / :bandwidth1) AS band,
     COUNT(1) AS num
 
 FROM {quiz_grades} qg
@@ -204,15 +204,17 @@ JOIN {quiz} q ON qg.quiz = q.id
 
 WHERE
     $usql
-    qg.quiz = ?
+    qg.quiz = :quizid
 
 GROUP BY
-    FLOOR(qg.grade/$bandwidth)
+    FLOOR(qg.grade / :bandwidth2)
 
 ORDER BY
     band";
-// TODO bandiwth with a placeholder.
-    $params[] = $quizid;
+
+    $params['quizid'] = $quizid;
+    $params['bandwidth1'] = $bandwidth;
+    $params['bandwidth2'] = $bandwidth;
 
     $data = $DB->get_records_sql_menu($sql, $params);
 
index 0783dda..4ee7456 100644 (file)
@@ -66,7 +66,8 @@ class quiz_responses_report extends quiz_attempt_report {
         $qmsubselect = quiz_report_qm_filter_select($quiz);
 
         $mform = new mod_quiz_report_responses_settings($reporturl,
-                array('qmsubselect' => $qmsubselect, 'quiz' => $quiz, 'currentgroup' => $currentgroup));
+                array('qmsubselect' => $qmsubselect, 'quiz' => $quiz,
+                'currentgroup' => $currentgroup, 'context' => $this->context));
 
         if ($fromform = $mform->get_data()) {
             $attemptsmode = $fromform->attemptsmode;
index 5918119..b36c3a8 100644 (file)
@@ -38,8 +38,7 @@ require_once($CFG->libdir . '/formslib.php');
  */
 class mod_quiz_report_responses_settings extends moodleform {
 
-    function definition() {
-        global $COURSE; // TODO get rid of this.
+    public function definition() {
         $mform = $this->_form;
 
         $mform->addElement('header', 'preferencespage', get_string('preferencespage', 'quiz_overview'));
@@ -60,7 +59,7 @@ class mod_quiz_report_responses_settings extends moodleform {
         if (!$this->_customdata['currentgroup']) {
             $options[QUIZ_REPORT_ATTEMPTS_ALL] = get_string('optallattempts','quiz_overview');
         }
-        if ($this->_customdata['currentgroup'] || $COURSE->id != SITEID) {
+        if ($this->_customdata['currentgroup'] || !is_inside_frontpage($this->_customdata['context'])) {
             $options[QUIZ_REPORT_ATTEMPTS_ALL_STUDENTS] = get_string('optallstudents','quiz_overview', $studentsstring);
             $options[QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH] =
                      get_string('optattemptsonly','quiz_overview', $studentsstring);
index 8b5eb30..4b6483b 100644 (file)
@@ -48,21 +48,21 @@ $attemptobj = quiz_attempt::create($attemptid);
 require_login($attemptobj->get_courseid(), false, $attemptobj->get_cm());
 $attemptobj->check_review_capability();
 
+echo $OUTPUT->header();
+
 // Check permissions.
 if ($attemptobj->is_own_attempt()) {
     if (!$attemptobj->is_finished()) {
-        echo $OUTPUT->header();
         echo $OUTPUT->notification(get_string('cannotreviewopen', 'quiz'));
         echo $OUTPUT->close_window_button();
         echo $OUTPUT->footer();
-        die;
+        die();
     } else if (!$options->responses) {
         $accessmanager = $attemptobj->get_access_manager(time());
-        echo $OUTPUT->header();
         echo $OUTPUT->notification($accessmanager->cannot_review_message($attemptobj->get_review_options()));
         echo $OUTPUT->close_window_button();
         echo $OUTPUT->footer();
-        die;
+        die();
     }
 
 } else if (!$attemptobj->is_review_allowed()) {
index 2dc79af..5fa3482 100644 (file)
@@ -118,7 +118,6 @@ if (isguestuser()) {
 }
 
 // If they are not enrolled in this course in a good enough role, tell them to enrol.
-// TODO, review this.
 if (!($canattempt || $canpreview || $canreviewmine)) {
     echo $OUTPUT->box('<p>' . get_string('youneedtoenrol', 'quiz') . "</p>\n\n<p>" .
             $OUTPUT->continue_button($CFG->wwwroot . '/course/view.php?id=' . $course->id) .
index 1b366e7..da25319 100644 (file)
@@ -118,7 +118,7 @@ abstract class question_behaviour {
      */
     public function render(question_display_options $options, $number,
             core_question_renderer $qoutput, qtype_renderer $qtoutput) {
-        $behaviouroutput = $this->get_renderer();
+        $behaviouroutput = $this->get_renderer($qoutput->get_page());
         $options = clone($options);
         $this->adjust_display_options($options);
         return $qoutput->question($this->qa, $behaviouroutput, $qtoutput, $options, $number);
@@ -139,11 +139,11 @@ abstract class question_behaviour {
     }
 
     /**
+     * @param moodle_page $page the page to render for.
      * @return qbehaviour_renderer get the appropriate renderer to use for this model.
      */
-    public function get_renderer() {
-        global $PAGE;
-        return $PAGE->get_renderer(get_class($this));
+    public function get_renderer(moodle_page $page) {
+        return $page->get_renderer(get_class($this));
     }
 
     /**
index 33a1bec..14527f6 100644 (file)
@@ -40,21 +40,21 @@ require_once(dirname(__FILE__) . '/../behaviour.php');
  */
 class qbehaviour_missing_test extends UnitTestCase {
     public function test_missing_cannot_start() {
-        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0, 0);
+        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0);
         $behaviour = new qbehaviour_missing($qa, 'deferredfeedback');
         $this->expectException();
         $behaviour->init_first_step(new question_attempt_step(array()));
     }
 
     public function test_missing_cannot_process() {
-        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0, 0);
+        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0);
         $behaviour = new qbehaviour_missing($qa, 'deferredfeedback');
         $this->expectException();
         $behaviour->process_action(new question_attempt_pending_step(array()));
     }
 
     public function test_missing_cannot_get_min_grade() {
-        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0, 0);
+        $qa = new question_attempt(test_question_maker::make_a_truefalse_question(), 0);
         $behaviour = new qbehaviour_missing($qa, 'deferredfeedback');
         $this->expectException();
         $behaviour->get_min_fraction();
index c334433..34348f7 100644 (file)
@@ -156,7 +156,7 @@ class qbehaviour_opaque extends question_behaviour {
             $opaquestate = qtype_opaque_update_state($this->qa, $pendingstep);
         } catch (SoapFault $sf) {
             print_object($sf);
-            return question_attempt::DISCARD; // TODO
+            return question_attempt::DISCARD; // TODO better Opaque error handling.
         }
 
         if ($opaquestate->resultssequencenumber != $this->qa->get_num_steps()) {
index b11aced..4cc811a 100644 (file)
@@ -69,17 +69,16 @@ function get_questions_category( $category, $noparent=false, $recurse=true, $exp
       $npsql = " and parent='0' ";
     }
 
-    // get (list) of categories
+    // Get list of categories
     if ($recurse) {
         $categorylist = question_categorylist($category->id);
-    }
-    else {
-        $categorylist = $category->id;
+    } else {
+        $categorylist = array($category->id);
     }
 
     // get the list of questions for the category
-    list ($usql, $params) = $DB->get_in_or_equal(explode(',', $categorylist));
-    if ($questions = $DB->get_records_select("question","category $usql $npsql", $params, "qtype, name ASC")) {
+    list($usql, $params) = $DB->get_in_or_equal($categorylist);
+    if ($questions = $DB->get_records_select('question', "category $usql $npsql", $params, 'qtype, name')) {
 
         // iterate through questions, getting stuff we need
         foreach($questions as $question) {
@@ -1103,7 +1102,7 @@ class question_bank_view {
         }
 
         if ($recurse) {
-            $categoryids = explode(',', question_categorylist($category->id));
+            $categoryids = question_categorylist($category->id);
         } else {
             $categoryids = array($category->id);
         }
@@ -1302,7 +1301,7 @@ class question_bank_view {
         $cmoptions->hasattempts = !empty($this->quizhasattempts);
 
         $strselectall = get_string('selectall');
-        $strselectnone = get_string('selectnone');
+        $strselectnone = get_string('deselectall');
         $strdelete = get_string('delete');
 
         list($categoryid, $contextid) = explode(',', $categoryandcontext);
@@ -1374,7 +1373,7 @@ class question_bank_view {
             }
 
             if ($canmoveall && count($addcontexts)) {
-                echo '<input type="submit" name="move" value="'.get_string('moveto')."\" />\n";
+                echo '<input type="submit" name="move" value="'.get_string('moveto', 'question')."\" />\n";
                 question_category_select_menu($addcontexts, false, 0, "$category->id,$category->contextid");
             }
 
@@ -1561,9 +1560,6 @@ class question_bank_view {
 function question_edit_setup($edittab, $baseurl, $requirecmid = false, $requirecourseid = true) {
     global $DB, $PAGE;
 
-    //$thispageurl is used to construct urls for all question edit pages we link to from this page. It contains an array
-    //of parameters that are passed from page to page.
-//    $thispageurl = new moodle_url($PAGE->url); //TODO: this looks dumb, because this method is called BEFORE $PAGE->set_page() !!!!
     $thispageurl = new moodle_url($baseurl);
     $thispageurl->remove_all_params(); // We are going to explicity add back everything important - this avoids unwanted params from being retained.
 
index c1e9060..e01240c 100644 (file)
@@ -139,8 +139,9 @@ abstract class question_bank {
         foreach (get_plugin_list('qtype') as $plugin => $notused) {
             try {
                 $qtypes[$plugin] = self::get_qtype($plugin);
-            } catch (Exception $e) {
-                // TODO ingore, but review this later.
+            } catch (coding_exception $e) {
+                // Catching coding_exceptions here means that incompatible
+                // question types do not cause the rest of Moodle to break.
             }
         }
         return $qtypes;
@@ -317,27 +318,26 @@ abstract class question_bank {
 class question_finder {
     /**
      * Get the ids of all the questions in a list of categoryies.
-     * @param int|string|array $categoryids either a categoryid, or a comma-separated list
+     * @param array $categoryids either a categoryid, or a comma-separated list
      *      category ids, or an array of them.
-     * @param string $extraconditions extra conditions to AND with the rest of the where clause.
+     * @param string $extraconditions extra conditions to AND with the rest of
+     *      the where clause. Must use named parameters.
+     * @param array $extraparams any parameters used by $extraconditions.
      * @return array questionid => questionid.
      */
-    public function get_questions_from_categories($categoryids, $extraconditions) {
+    public function get_questions_from_categories($categoryids, $extraconditions, $extraparams = array()) {
         global $DB;
 
-        if (is_array($categoryids)) {
-            $categoryids = implode(',', $categoryids);
-        }
+        list($qcsql, $qcparams) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED, 'qc0000');
 
         if ($extraconditions) {
             $extraconditions = ' AND (' . $extraconditions . ')';
         }
-        // TODO switch to using $DB->in_or_equal.
-        $questionids = $DB->get_records_select_menu('question',
-                "category IN ($categoryids)
+
+        return $DB->get_records_select_menu('question',
+                "category $qcsql
                  AND parent = 0
                  AND hidden = 0
-                 $extraconditions", array(), '', 'id,id AS id2');
-        return $questionids;
+                 $extraconditions", $qcparams + $extraparams, '', 'id,id AS id2');
     }
 }
index c20370a..a4af365 100644 (file)
@@ -747,7 +747,8 @@ ORDER BY
      * this method.
      *
      * @param string $qubaid SQL fragment that controls which usage is summed.
-     * This might be the name of a column in the outer query.
+     * This will normally be the name of a column in the outer query. Not that this
+     * SQL fragment must not contain any placeholders.
      * @return string SQL code for the subquery.
      */
     public function sum_usage_marks_subquery($qubaid) {
@@ -763,7 +764,6 @@ ORDER BY
             JOIN {question_attempt_steps} qas ON qas.id = lateststepid.latestid
             WHERE qa.questionusageid = $qubaid
             HAVING COUNT(CASE WHEN qas.state = 'needsgrading' AND qa.maxmark > 0 THEN 1 ELSE NULL END) = 0";
-        // TODO handle $qubaid with placeholders.
     }
 
     public function question_attempt_latest_state_view($alias) {
index 4b6cf64..2b2d7a7 100644 (file)
@@ -102,8 +102,11 @@ abstract class question_engine {
     }
 
     /**
-     * Delete a {@link question_usage_by_activity} from the database, based on its id.
-     * @param int $qubaid the id of the usage to delete.
+     * Delete {@link question_usage_by_activity}s from the database that match
+     * an arbitrary SQL where clause.
+     * @param string $where a where clause. Becuase of MySQL limitations, you
+     *      must refer to {question_usages}.id in full like that.
+     * @param array $params values to substitute for placeholders in $where.
      */
     public static function delete_questions_usage_by_activities($where, $params) {
         $dm = new question_engine_data_mapper();
@@ -728,8 +731,7 @@ class question_usage_by_activity {
      * @return int the number used to identify this question within this usage.
      */
     public function add_question(question_definition $question, $maxmark = null) {
-        $qa = new question_attempt($question, $this->get_id(),
-                $this->context->id, $this->observer, $maxmark);
+        $qa = new question_attempt($question, $this->get_id(), $this->observer, $maxmark);
         if (count($this->questionattempts) == 0) {
             $this->questionattempts[1] = $qa;
         } else {
@@ -1165,7 +1167,7 @@ class question_usage_by_activity {
         $this->observer->notify_delete_attempt_steps($oldqa);
 
         $newqa = new question_attempt($oldqa->get_question(), $oldqa->get_usage_id(),
-                $this->context->id, $this->observer, $newmaxmark);
+                $this->observer, $newmaxmark);
         $newqa->set_database_id($oldqa->get_database_id());
         $newqa->regrade($oldqa, $finished);
 
@@ -1321,10 +1323,6 @@ class question_attempt {
     /** @var integer|string the id of the question_usage_by_activity we belong to. */
     protected $usageid;
 
-    /** @var integer the id of the context this question_attempt belongs to. */
-    // TODO I don't think this is actually needed.
-    protected $owningcontextid = null;
-
     /** @var integer the number used to identify this question_attempt within the usage. */
     protected $slot = null;
 
@@ -1395,11 +1393,10 @@ class question_attempt {
      * @param number $maxmark the maximum grade for this question_attempt. If not
      * passed, $question->defaultmark is used.
      */
-    public function __construct(question_definition $question, $usageid, $owningcontextid,
+    public function __construct(question_definition $question, $usageid,
             question_usage_observer $observer = null, $maxmark = null) {
         $this->question = $question;
         $this->usageid = $usageid;
-        $this->owningcontextid = $owningcontextid;
         if (is_null($observer)) {
             $observer = new question_usage_null_observer();
         }
@@ -1830,10 +1827,13 @@ class question_attempt {
      * @param string|null $number The question number to display.
      * @return string HTML fragment representing the question.
      */
-    public function render($options, $number) {
-        global $PAGE;
-        $qoutput = $PAGE->get_renderer('core', 'question');
-        $qtoutput = $this->question->get_renderer();
+    public function render($options, $number, $page = null) {
+        if (is_null($page)) {
+            global $PAGE;
+            $page = $PAGE;
+        }
+        $qoutput = $page->get_renderer('core', 'question');
+        $qtoutput = $this->question->get_renderer($page);
         return $this->behaviour->render($options, $number, $qoutput, $qtoutput);
     }
 
@@ -1842,9 +1842,14 @@ class question_attempt {
      * attempt is displayed in the body.
      * @return string HTML fragment.
      */
-    public function render_head_html() {
-        return $this->question->get_renderer()->head_code($this) .
-                $this->behaviour->get_renderer()->head_code($this);
+    public function render_head_html($page = null) {
+        if (is_null($page)) {
+            global $PAGE;
+            $page = $PAGE;
+        }
+        // TODO go via behaviour.
+        return $this->question->get_renderer($page)->head_code($this) .
+                $this->behaviour->get_renderer($page)->head_code($this);
     }
 
     /**
@@ -2221,7 +2226,7 @@ class question_attempt {
         }
 
         $qa = new question_attempt($question, $record->questionusageid,
-                $record->contextid, null, $record->maxmark + 0);
+                null, $record->maxmark + 0);
         $qa->set_database_id($record->questionattemptid);
         $qa->set_number_in_usage($record->slot);
         $qa->minfraction = $record->minfraction + 0;
index c076d50..b869bab 100644 (file)
@@ -37,6 +37,9 @@ defined('MOODLE_INTERNAL') || die();
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class core_question_renderer extends plugin_renderer_base {
+    public function get_page() {
+        return $this->page;
+    }
 
     /**
      * Generate the display of a question in a particular state, and with certain
index bc89995..7e2b9c6 100644 (file)
@@ -70,7 +70,7 @@ class test_question_maker {
      * @return question_attempt the question attempt.
      */
     public function get_a_qa($question, $maxmark = 3) {
-        return new question_attempt($question, 13, 0, null, $maxmark);
+        return new question_attempt($question, 13, null, $maxmark);
     }
 
     /**
index 3e30eeb..ea11aca 100644 (file)
@@ -51,7 +51,7 @@ class question_attempt_test extends UnitTestCase {
         $this->question = test_question_maker::make_a_description_question();
         $this->question->defaultmark = 3;
         $this->usageid = 13;
-        $this->qa = new question_attempt($this->question, $this->usageid, 0);
+        $this->qa = new question_attempt($this->question, $this->usageid);
     }
 
     public function tearDown() {
@@ -61,13 +61,13 @@ class question_attempt_test extends UnitTestCase {
     }
 
     public function test_constructor_sets_maxmark() {
-        $qa = new question_attempt($this->question, $this->usageid, 0);
+        $qa = new question_attempt($this->question, $this->usageid);
         $this->assertIdentical($this->question, $qa->get_question());
         $this->assertEqual(3, $qa->get_max_mark());
     }
 
     public function test_maxmark_beats_default_mark() {
-        $qa = new question_attempt($this->question, $this->usageid, 0, null, 2);
+        $qa = new question_attempt($this->question, $this->usageid, null, 2);
         $this->assertEqual(2, $qa->get_max_mark());
     }
 
@@ -143,7 +143,7 @@ class question_attempt_with_steps_test extends UnitTestCase {
 
     public function setUp() {
         $this->question = test_question_maker::make_a_description_question();
-        $this->qa = new testable_question_attempt($this->question, 0, 0, null, 2);
+        $this->qa = new testable_question_attempt($this->question, 0, null, 2);
         for ($i = 0; $i < 3; $i++) {
             $step = new question_attempt_step(array('i' => $i));
             $this->qa->add_step($step);
@@ -247,7 +247,7 @@ class question_attempt_with_steps_test extends UnitTestCase {
     }
 
     public function test_cannot_get_min_fraction_before_start() {
-        $qa = new question_attempt($this->question, null, 0);
+        $qa = new question_attempt($this->question, 0);
         $this->expectException();
         $qa->get_min_fraction();
     }
index 6f0cd28..a7ec5b2 100644 (file)
@@ -43,7 +43,7 @@ class question_attempt_step_iterator_test extends UnitTestCase {
 
     public function setUp() {
         $question = test_question_maker::make_a_description_question();
-        $this->qa = new testable_question_attempt($question, 0, 0);
+        $this->qa = new testable_question_attempt($question, 0);
         for ($i = 0; $i < 3; $i++) {
             $step = new question_attempt_step(array('i' => $i));
             $this->qa->add_step($step);
index 514d705..655ae4c 100644 (file)
@@ -838,22 +838,6 @@ class qformat_default {
         return NULL;
     }
 
-    /**
-     * get directory into which export is going
-     * @return string file path
-     */
-    function question_get_export_dir() {
-        // TODO is this still used.
-        global $USER;
-        if ($this->canaccessbackupdata) {
-            $dirname = get_string('exportfilename', 'question');
-            $path = $this->course->id . '/backupdata/' . $dirname; // backupdata is protected directory
-        } else {
-            $path = 'temp/questionexport/' . $USER->id;
-        }
-        return $path;
-    }
-
     /**
      * Convert the question text to plain text, so it can safely be displayed
      * during import to let the user see roughly what is going on.
index 1e6238a..cf57887 100644 (file)
@@ -261,14 +261,15 @@ function handle_questions_media(&$questions, $path, $courseid) {
  */
     function exportprocess() {
 
-        global $CFG, $OUTPUT;
+        global $CFG, $OUTPUT, $USER;
         $courseid = $this->course->id;
 
+        $path = 'temp/qformat_qti_two/' . $USER->id . '/' . $this->filename;
         // create a directory for the exports (if not already existing)
-        if (!$export_dir = make_upload_directory($this->question_get_export_dir().'/'.$this->filename)) {
-              print_error('cannotcreatepath', 'question', '', $export_dir);
+        if (!make_upload_directory($path)) {
+              throw new moodle_exception('cannotcreatepath', 'question', '', $path);
         }
-        $path = $CFG->dataroot.'/'.$this->question_get_export_dir().'/'.$this->filename;
+        $path = $CFG->dataroot . '/' . $path;
 
         // get the questions (from database) in this category
         $questions = get_questions_category( $this->category );
index 357932d..673257a 100644 (file)
@@ -115,19 +115,16 @@ if ($form = $import_form->get_data()) {
 
     // Do anything before that we need to
     if (!$qformat->importpreprocess()) {
-        //TODO: need more detailed error info
         print_error('cannotimport', '', $thispageurl->out());
     }
 
     // Process the uploaded file
     if (!$qformat->importprocess($category)) {
-        //TODO: need more detailed error info
         print_error('cannotimport', '', $thispageurl->out());
     }
 
     // In case anything needs to be done after
     if (!$qformat->importpostprocess()) {
-        //TODO: need more detailed error info
         print_error('cannotimport', '', $thispageurl->out());
     }
 
index 2442001..cad5eee 100644 (file)
@@ -98,15 +98,7 @@ class backup_qtype_calculated_plugin extends backup_qtype_plugin {
      * files to be processed both in backup and restore.
      */
     public static function get_qtype_fileareas() {
-        // TODO: Discuss. Commented below are the "in theory" correct
-        // mappings for those fileareas. Instead we are using question for
-        // them, that will cause problems in the future if we want to change
-        // any of them to be 1..n (i.e. we should be always pointing to own id)
         return array(
-            //'instruction' => 'question_numerical_option',
-            //'correctfeedback' => 'question_calculated_option',
-            //'partiallycorrectfeedback' => 'question_calculated_option',
-            //'incorrectfeedback' => 'question_calculated_option');
             'instruction' => 'question_created',
             'correctfeedback' => 'question_created',
             'partiallycorrectfeedback' => 'question_created',
index e602254..bc538f8 100644 (file)
@@ -2145,15 +2145,14 @@ class question_calculated_qtype extends question_type {
             }
             return true;
         } else if ($filearea == 'instruction') {
-            // TODO: should it be display all the time like questiontext?
-            // check if question id exists
+            // Displayed all the time like the question text. Check if question id exists
             if ($itemid != $question->id) {
                 return false;
             } else {
                 return true;
             }
         } else if (in_array($filearea, array('correctfeedback', 'partiallycorrectfeedback', 'incorrectfeedback'))) {
-            // TODO: calculated type doesn't display question feedback yet
+            // Note: calculated type doesn't display question feedback yet
             return false;
         } else {
             return parent::check_file_access($question, $state, $options, $contextid, $component,
index 0abd637..6d998d0 100644 (file)
@@ -107,8 +107,6 @@ class question_calculatedmulti_qtype extends question_calculated_qtype {
 
         // Save the units.
         $virtualqtype = $this->get_virtual_qtype($question);
-        // TODO: What is this?
-        // $result = $virtualqtype->save_numerical_units($question);
         if (isset($result->error)) {
             return $result;
         } else {
index 380ed93..687c77f 100644 (file)
@@ -378,7 +378,7 @@ class question_calculatedsimple_qtype extends question_calculated_qtype {
         $itemid = reset($args);
         if ($component == 'question' && $filearea == 'answerfeedback') {
 
-            // check if answer id exists
+            // Check if answer id exists
             $result = $options->feedback && array_key_exists($itemid, $question->options->answers);
             if (!$result) {
                 return false;
@@ -389,8 +389,7 @@ class question_calculatedsimple_qtype extends question_calculated_qtype {
             }
             return true;
         } else if ($filearea == 'instruction') {
-            // TODO: should it be display all the time like questiontext?
-            // check if question id exists
+            // Displayed all the time like the question text. Check if question id exists
             if ($itemid != $question->id) {
                 return false;
             } else {
index 56261e8..a257c19 100644 (file)
@@ -95,7 +95,7 @@ class qtype_missing_test extends UnitTestCase {
     public function test_render_missing() {
         $questiondata = $this->get_unknown_questiondata();
         $q = question_bank::make_question($questiondata);
-        $qa = new testable_question_attempt($q, 0, 0);
+        $qa = new testable_question_attempt($q, 0);
 
         $step = new question_attempt_step(array('answer' => 'frog'));
         $step->set_state(question_state::$todo);
index 9d5a760..0f6ba14 100644 (file)
@@ -76,14 +76,7 @@ class backup_qtype_multichoice_plugin extends backup_qtype_plugin {
      * files to be processed both in backup and restore.
      */
     public static function get_qtype_fileareas() {
-        // TODO: Discuss. Commented below are the "in theory" correct
-        // mappings for those fileareas. Instead we are using question for
-        // them, that will cause problems in the future if we want to change
-        // any of them to be 1..n (i.e. we should be always pointing to own id)
         return array(
-            //'correctfeedback' => 'question_multichoice',
-            //'partiallycorrectfeedback' => 'question_multichoice',
-            //'incorrectfeedback' => 'question_multichoice');
             'correctfeedback' => 'question_created',
             'partiallycorrectfeedback' => 'question_created',
             'incorrectfeedback' => 'question_created');
index 3f929f1..6d495f3 100644 (file)
@@ -129,9 +129,8 @@ abstract class qtype_multichoice_base extends question_graded_automatically {
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class qtype_multichoice_single_question extends qtype_multichoice_base {
-    public function get_renderer() {
-        global $PAGE; // TODO get rid of this global.
-        return $PAGE->get_renderer('qtype_multichoice', 'single');
+    public function get_renderer(moodle_page $page) {
+        return $page->get_renderer('qtype_multichoice', 'single');
     }
 
     public function get_min_fraction() {
@@ -228,9 +227,8 @@ class qtype_multichoice_single_question extends qtype_multichoice_base {
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class qtype_multichoice_multi_question extends qtype_multichoice_base {
-    public function get_renderer() {
-        global $PAGE; // TODO get rid of this global.
-        return $PAGE->get_renderer('qtype_multichoice', 'multi');
+    public function get_renderer(moodle_page $page) {
+        return $page->get_renderer('qtype_multichoice', 'multi');
     }
 
     public function get_min_fraction() {
index 311d9cb..a9a2936 100644 (file)
@@ -1,4 +1,3 @@
-/* TODO */
 .que.multichoice .answer .specificfeedback {
     display: inline;
     padding: 0 0.7em;
index fdf077c..468d9e1 100644 (file)
@@ -82,12 +82,6 @@ class backup_qtype_numerical_plugin extends backup_qtype_plugin {
      * files to be processed both in backup and restore.
      */
     public static function get_qtype_fileareas() {
-        // TODO: Discuss. Commented below are the "in theory" correct
-        // mappings for those fileareas. Instead we are using question for
-        // them, that will cause problems in the future if we want to change
-        // any of them to be 1..n (i.e. we should be always pointing to own id)
-        return array(
-            //'instruction' => 'question_numerical_option');
-            'instruction' => 'question_created');
+        return array('instruction' => 'question_created');
     }
 }
index 587a9c9..1393861 100644 (file)
@@ -1067,7 +1067,7 @@ class question_numerical_qtype extends qtype_shortanswer {
         $mform->setType('unitpenalty', PARAM_NUMBER);
         $mform->setDefault('unitpenalty', 0.1);
         $mform->setDefault('unitgradingtypes', 1);
-        $mform->addHelpButton('penaltygrp', 'unitpenalty', 'qtype_numerical'); // TODO help did not exist before MDL-21695
+        $mform->addHelpButton('penaltygrp', 'unitpenalty', 'qtype_numerical');
         $mform->setDefault('unitsleft', 0);
         $mform->setType('instructions', PARAM_RAW);
         $mform->addHelpButton('instructions', 'numericalinstructions', 'qtype_numerical');
index 49cef95..4636840 100644 (file)
@@ -204,9 +204,8 @@ abstract class question_definition {
     /**
      * @return qtype_renderer the renderer to use for outputting this question.
      */
-    public function get_renderer() {
-        global $PAGE; // TODO get rid of this global.
-        return $PAGE->get_renderer('qtype_' . $this->qtype->name());
+    public function get_renderer(moodle_page $page) {
+        return $page->get_renderer($this->qtype->plugin_name());
     }
 
     /**
index fc80e3c..52b4f8f 100644 (file)
@@ -80,14 +80,16 @@ class qtype_random extends question_type {
         if ($question->questiontext) {
             $categorylist = question_categorylist($question->category);
         } else {
-            $categorylist = $question->category;
+            $categorylist = array($question->category);
         }
+        list($qcsql, $qcparams) = $DB->get_in_or_equal($categorylist);
+        // TODO use in_or_equal for $otherquestionsinuse and $this->manualqtypes
         return $DB->record_exists_select('question',
-                "category IN ($categorylist)
+                "category $qcsql
                      AND parent = 0
                      AND hidden = 0
                      AND id NOT IN ($otherquestionsinuse)
-                     AND qtype IN ($this->manualqtypes)");
+                     AND qtype IN ($this->manualqtypes)", $qcparams);
     }
 
     /**
@@ -186,7 +188,7 @@ class qtype_random extends question_type {
         if ($subcategories) {
             $categoryids = question_categorylist($categoryid);
         } else {
-            $categoryids = $categoryid;
+            $categoryids = array($categoryid);
         }
 
         $questionids = question_bank::get_finder()->get_questions_from_categories(
index 39e884a..7a712f6 100644 (file)
@@ -109,36 +109,18 @@ class question_randomsamatch_qtype extends qtype_match {
             // recurse into subcategories
             $categorylist = question_categorylist($question->category);
         } else {
-            $categorylist = $question->category;
+            $categorylist = array($question->category);
         }
 
         $saquestions = $this->get_sa_candidates($categorylist, $cmoptions->questionsinuse);
 
         $count  = count($saquestions);
         $wanted = $question->options->choose;
-        $errorstr = '';
-        if ($count < $wanted && has_coursecontact_role($USER->id)) { //TODO: this teacher test is far from optimal
-            if ($count >= 2) {
-                $errorstr =  "Error: could not get enough Short-Answer questions!
-                 Got $count Short-Answer questions, but wanted $wanted.
-                 Reducing number to choose from to $count!";
-                $wanted = $question->options->choose = $count;
-            } else {
-                $errorstr = "Error: could not get enough Short-Answer questions!
-                 This can happen if all available Short-Answer questions are already
-                 taken up by other Random questions or Random Short-Answer question.
-                 Another possible cause for this error is that Short-Answer
-                 questions were deleted after this Random Short-Answer question was
-                 created.";
-            }
-            echo $OUTPUT->notification($errorstr);
-            $errorstr = '<span class="notifyproblem">' . $errorstr . '</span>';
-        }
 
         if ($count < $wanted) {
-            $question->questiontext = "$errorstr<br /><br />Insufficient selection options are
-             available for this question, therefore it is not available in  this
-             quiz. Please inform your teacher.";
+            $question->questiontext = "Insufficient selection options are
+                available for this question, therefore it is not available in  this
+                quiz. Please inform your teacher.";
             // Treat this as a description from this point on
             $question->qtype = DESCRIPTION;
             return true;
@@ -263,9 +245,9 @@ class question_randomsamatch_qtype extends qtype_match {
         return $response;
     }
 
-    function get_sa_candidates($categorylist, $questionsinuse=0) {
+    function get_sa_candidates($categorylist, $questionsinuse = 0) {
         global $DB;
-        list ($usql, $params) = $DB->get_in_or_equal(explode(',', $categorylist));
+        list ($usql, $params) = $DB->get_in_or_equal($categorylist);
         list ($ques_usql, $ques_params) = $DB->get_in_or_equal(explode(',', $questionsinuse), SQL_PARAMS_QM, null, false);
         $params = array_merge($params, $ques_params);
         return $DB->get_records_select('question',
@@ -310,7 +292,7 @@ class question_randomsamatch_qtype extends qtype_match {
                 // recurse into subcategories
                 $categorylist = question_categorylist($question->category);
             } else {
-                $categorylist = $question->category;
+                $categorylist = array($question->category);
             }
 
             $question->options->subquestions = $this->get_sa_candidates($categorylist);
index 7d336e6..2354c8e 100644 (file)
@@ -187,7 +187,10 @@ abstract class qtype_renderer extends plugin_renderer_base {
      * @return string HTML fragment.
      */
     public function head_code(question_attempt $qa) {
-        // TODO I think we can get rid of this, but what about Opaque?
+        // This method is used by the Opaque question type. The remote question
+        // engine can send back arbitrary CSS that we have to link to in the
+        // page header. If it was not for that, we might be able to eliminate
+        // this method and load the required CSS and JS some other way.
         $qa->get_question()->qtype->find_standard_scripts();
     }