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
    • Rank:
      38071

      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.

        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: