Moodle
  1. Moodle
  2. MDL-31521

Calculated questions should allow more than one unit with multiplier equal to 1

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.2.1, 2.6.1
    • Fix Version/s: STABLE backlog
    • Component/s: Questions
    • Labels:
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_26_STABLE

      Description

      See http://moodle.org/mod/forum/discuss.php?d=195449
      the
      public function comment_on_datasetitems($qtypeobj, $questionid, $questiontext,
      $answers, $data, $number) {
      global $DB;
      $comment = new stdClass();
      $comment->stranswers = array();
      $comment->outsidelimit = false;
      $comment->answers = array();
      // Find a default unit:
      if (!empty($questionid) && $unit = $DB->get_record('question_numerical_units',
      array('question' => $questionid, 'multiplier' => 1.0))) {
      $unit = $unit->unit;
      should use another $DB function call to allow for more than one record.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Pierre Pichet added a comment -

            Tim,
            This is the initial code for this function, the more stringent control of database calls
            seems to do their wrk so we need to put another call.

            If you put here what should be the best $DB call, I could do the testing.

            Show
            Pierre Pichet added a comment - Tim, This is the initial code for this function, the more stringent control of database calls seems to do their wrk so we need to put another call. If you put here what should be the best $DB call, I could do the testing.
            Hide
            Tim Hunt added a comment -

            I suspect you want something like

            $unit = '';
            if (!empty($questionid) {
                $units = $DB->get_records('question_numerical_units',
                        array('question' => $questionid, 'multiplier' => 1.0),
                        'id ASC', '*', 0, 1);
                if ($units) {
                    $unit = reset($units);
                    $unit = $unit->unit;
                }
            }
            

            Show
            Tim Hunt added a comment - I suspect you want something like $unit = ''; if (!empty($questionid) { $units = $DB->get_records('question_numerical_units', array('question' => $questionid, 'multiplier' => 1.0), 'id ASC', '*', 0, 1); if ($units) { $unit = reset($units); $unit = $unit->unit; } }
            Hide
            Pierre Pichet added a comment -

            Looking more closely at the code I just realize that finally the unit is not used
            in multichoice calculated
            so in calculated/questiontype.php function
            public function multichoice_comment_on_datasetitems($questionid, $questiontext,
            $answers, $data, $number) {

            the lines

                    // Find a default unit:
                    if (!empty($questionid) && $unit = $DB->get_record('question_numerical_units',
                            array('question' => $questionid, 'multiplier' => 1.0))) {
                        $unit = $unit->unit;
                    } else {
                        $unit = '';
                    }
            

            should be removed.

            The numerical/questiontype.php retrieve units using

                public function get_numerical_units(&$question) {
                    global $DB;
             
                    if ($units = $DB->get_records('question_numerical_units',
                            array('question' => $question->id), 'id ASC')) {
                        $units = array_values($units);
                    } else {
                        $units = array();
                    }
                    foreach ($units as $key => $unit) {
                        $units[$key]->multiplier = clean_param($unit->multiplier, PARAM_NUMBER);
                    }
                    $question->options->units = $units;
                    return true;
                }
            
            

            However the $qtypeobject in public function comment_on_datasetitems($qtypeobj,...
            is not the $question so we cannot use directly get_numerical_units().

            qtype_calculated Object
            (
                [wizardpagesnumber] => 3
                [fileoptions:protected] => Array
                    (
                        [subdirs] => 
                        [maxfiles] => -1
                        [maxbytes] => 0
                    )
             
            )
            

            Show
            Pierre Pichet added a comment - Looking more closely at the code I just realize that finally the unit is not used in multichoice calculated so in calculated/questiontype.php function public function multichoice_comment_on_datasetitems($questionid, $questiontext, $answers, $data, $number) { the lines // Find a default unit: if (!empty($questionid) && $unit = $DB->get_record('question_numerical_units', array('question' => $questionid, 'multiplier' => 1.0))) { $unit = $unit->unit; } else { $unit = ''; } should be removed. The numerical/questiontype.php retrieve units using public function get_numerical_units(&$question) { global $DB;   if ($units = $DB->get_records('question_numerical_units', array('question' => $question->id), 'id ASC')) { $units = array_values($units); } else { $units = array(); } foreach ($units as $key => $unit) { $units[$key]->multiplier = clean_param($unit->multiplier, PARAM_NUMBER); } $question->options->units = $units; return true; } However the $qtypeobject in public function comment_on_datasetitems($qtypeobj,... is not the $question so we cannot use directly get_numerical_units(). qtype_calculated Object ( [wizardpagesnumber] => 3 [fileoptions:protected] => Array ( [subdirs] => [maxfiles] => -1 [maxbytes] => 0 )   )
            Hide
            Pierre Pichet added a comment - - edited

            However we should note that $unit->multiplier is not necessarily stored as 1.0 as shown by the line

            $units[$key]->multiplier = clean_param($unit->multiplier, PARAM_NUMBER);
            

            BUT in db/install

                    <FIELD NAME="multiplier" TYPE="number" LENGTH="40" NOTNULL="true" UNSIGNED="false" DEFAULT="1.00000000000000000000" SEQUENCE="false" DECIMALS="20" COMMENT="The multiplier for this unit. For example, if the first unit is (1.0, 'cm'), another unit might be (0.1, 'mm') or (100.0, 'm')." PREVIOUS="question" NEXT="unit"/>
            
            

            So we should not need to use clean_param ?

            Show
            Pierre Pichet added a comment - - edited However we should note that $unit->multiplier is not necessarily stored as 1.0 as shown by the line $units[$key]->multiplier = clean_param($unit->multiplier, PARAM_NUMBER); BUT in db/install <FIELD NAME="multiplier" TYPE="number" LENGTH="40" NOTNULL="true" UNSIGNED="false" DEFAULT="1.00000000000000000000" SEQUENCE="false" DECIMALS="20" COMMENT="The multiplier for this unit. For example, if the first unit is (1.0, 'cm'), another unit might be (0.1, 'mm') or (100.0, 'm')." PREVIOUS="question" NEXT="unit"/> So we should not need to use clean_param ?
            Hide
            Pierre Pichet added a comment -

            Tim,
            Because of scrolling, I did not notice your suggestion until now.

            I will use your proposal to complete the work.

            Thanks

            Show
            Pierre Pichet added a comment - Tim, Because of scrolling, I did not notice your suggestion until now. I will use your proposal to complete the work. Thanks
            Hide
            Pierre Pichet added a comment -

            The suggested code solve the problem.
            Then the git update and testing instructions to interlace with course...

            Show
            Pierre Pichet added a comment - The suggested code solve the problem. Then the git update and testing instructions to interlace with course...
            Hide
            Pierre Pichet added a comment -

            Tim,
            As the testing will imply the same process, I want to include MDL_31056 Increase length on answer field when editing calculated questiontypes.
            Can I do it

            Show
            Pierre Pichet added a comment - Tim, As the testing will imply the same process, I want to include MDL_31056 Increase length on answer field when editing calculated questiontypes. Can I do it
            Hide
            Pierre Pichet added a comment -

            As usual when you go back to solve a given bug, you look at the code with new eyes.
            Then I realized that this function should not search for the units.
            This should be handled by the calling function that already set the values of the other parameters.

            So I am going back to the drawing board.

            Show
            Pierre Pichet added a comment - As usual when you go back to solve a given bug, you look at the code with new eyes. Then I realized that this function should not search for the units. This should be handled by the calling function that already set the values of the other parameters. So I am going back to the drawing board.
            Hide
            Tim Hunt added a comment -

            Loading the unit in the calling function does sound like a good idea.

            Generally, it is better to fix one bug at a time, each as a separate commit, so, ideally do the fix for MDL-31056 separately.

            Show
            Tim Hunt added a comment - Loading the unit in the calling function does sound like a good idea. Generally, it is better to fix one bug at a time, each as a separate commit, so, ideally do the fix for MDL-31056 separately.
            Hide
            Pierre Pichet added a comment -

            Ok

            Show
            Pierre Pichet added a comment - Ok
            Hide
            Tim Hunt added a comment -

            It looks like we know what the solution is here, so we should try to get the fix in.

            I am changing the status so I don't forget about this again.

            Show
            Tim Hunt added a comment - It looks like we know what the solution is here, so we should try to get the fix in. I am changing the status so I don't forget about this again.
            Hide
            Irfan Ali added a comment -

            Same problem here

            Error: mdb->get_record() found more than one record!◦line 1309 of /lib/dml/moodle_database.php: call to debugging()
            ◦line 1269 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
            ◦line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
            ◦line 103 of /index.php: call to moodle_database->get_record()

            Show
            Irfan Ali added a comment - Same problem here Error: mdb->get_record() found more than one record!◦line 1309 of /lib/dml/moodle_database.php: call to debugging() ◦line 1269 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() ◦line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() ◦line 103 of /index.php: call to moodle_database->get_record()
            Hide
            Sam Hemelryk added a comment -

            Hi Tim,

            I see you marked this as peer-review in process back in Sept but didn't make yourself peer-reviewer.
            By chance has this being missed off your lists, or are you still on top of this?

            Show
            Sam Hemelryk added a comment - Hi Tim, I see you marked this as peer-review in process back in Sept but didn't make yourself peer-reviewer. By chance has this being missed off your lists, or are you still on top of this?
            Hide
            Tim Hunt added a comment -

            Yes, I did lose sight of this. We still need a completed fix as a git commit.

            Show
            Tim Hunt added a comment - Yes, I did lose sight of this. We still need a completed fix as a git commit.
            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this issue.

            We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

            If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

            Michael d.

            TW9vZGxlDQo=

            Show
            Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
            Hide
            Pierre Pichet added a comment -

            This is always valid as seen on actual master.

            Show
            Pierre Pichet added a comment - This is always valid as seen on actual master.

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated: