Moodle
  1. Moodle
  2. MDL-31594

Decimal in manual grading are truncated when using comma

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Questions, Quiz
    • Labels:
    • Environment:
      Windows 2008 Server
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Prerequiste: a weekly course with a student and a teacher. Moodle site set to use lang en.
      1- Create a quiz. Make sure "Decimal places in grades" is not zero.
      2- Add at least one essay question to the quiz, set it to be marked out of 50.
      3- With a student account, complete the quiz and submit it
      4- With a teacher account, go to the quiz > results > manual grading
      5- Click the Grade link for one of the essay questions.
      6- On the Mark field, you should see a blank value
      7- Enter the "25,75" value in the Mark field and click Save
      8- Go back to the the front page of the manual grading report, and click "Update grades". Make sure the grade is displayed as 25.75.
      9- Now save the new grade "35.75" (with a point)
      10- Verify that this grade is also saved and displayed correctly.

      11- Now switch your language to fr (or any other languages that uses , as a decimal. Repeat steps 4 - 10. The only difference is that Moodle should output grades using , instead of .

      Show
      Prerequiste: a weekly course with a student and a teacher. Moodle site set to use lang en. 1- Create a quiz. Make sure "Decimal places in grades" is not zero. 2- Add at least one essay question to the quiz, set it to be marked out of 50. 3- With a student account, complete the quiz and submit it 4- With a teacher account, go to the quiz > results > manual grading 5- Click the Grade link for one of the essay questions. 6- On the Mark field, you should see a blank value 7- Enter the "25,75" value in the Mark field and click Save 8- Go back to the the front page of the manual grading report, and click "Update grades". Make sure the grade is displayed as 25.75. 9- Now save the new grade "35.75" (with a point) 10- Verify that this grade is also saved and displayed correctly. 11- Now switch your language to fr (or any other languages that uses , as a decimal. Repeat steps 4 - 10. The only difference is that Moodle should output grades using , instead of .
    • Workaround:
      Hide

      Use point instead of comma and be very careful when editing existing answer.

      Show
      Use point instead of comma and be very careful when editing existing answer.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38158

      Description

      Using "fr" language pack, when we manually grades an answer, every decimals are truncated if we use the comma. Using the a point instead keep the decimal value correctly. However, once saved using a point, value is then print on screen with a comma, so saving it as is for a second time truncate the decimals again.

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          Tim,
          In behaviourbase.php

              public function process_comment(question_attempt_pending_step $pendingstep) {
                  if (!$this->qa->get_state()->is_finished()) {
                      throw new coding_exception('Cannot manually grade a question before it is finshed.');
                  }
          
                  if ($this->is_same_comment($pendingstep)) {
                      return question_attempt::DISCARD;
                  }
          
                  if ($pendingstep->has_behaviour_var('mark')) {
                      $fraction = $pendingstep->get_behaviour_var('mark') /
                                      $pendingstep->get_behaviour_var('maxmark');
                      if ($pendingstep->get_behaviour_var('mark') === '') {
                          $fraction = null;
                      } else if ($fraction > 1 || $fraction < $this->qa->get_min_fraction()) {
                          throw new coding_exception('Score out of range when processing ' .
                                  'a manual grading action.', $pendingstep);
                      }
                      $pendingstep->set_fraction($fraction);
                  }
          
                  $pendingstep->set_state($this->qa->get_state()->corresponding_commented_state(
                          $pendingstep->get_fraction()));
                  return question_attempt::KEEP;
          

          $pendingstep->get_behaviour_var('mark')
          the result is already truncated if there is a , in the number (1,25 gives 1) i.e this will be the case if the param is considered as a float.

          I cannot locate in this special pop-up window where (or if) the param 'mark' type is defined.

          Show
          Pierre Pichet added a comment - Tim, In behaviourbase.php public function process_comment(question_attempt_pending_step $pendingstep) { if (!$this->qa->get_state()->is_finished()) { throw new coding_exception('Cannot manually grade a question before it is finshed.'); } if ($this->is_same_comment($pendingstep)) { return question_attempt::DISCARD; } if ($pendingstep->has_behaviour_var('mark')) { $fraction = $pendingstep->get_behaviour_var('mark') / $pendingstep->get_behaviour_var('maxmark'); if ($pendingstep->get_behaviour_var('mark') === '') { $fraction = null; } else if ($fraction > 1 || $fraction < $this->qa->get_min_fraction()) { throw new coding_exception('Score out of range when processing ' . 'a manual grading action.', $pendingstep); } $pendingstep->set_fraction($fraction); } $pendingstep->set_state($this->qa->get_state()->corresponding_commented_state( $pendingstep->get_fraction())); return question_attempt::KEEP; $pendingstep->get_behaviour_var('mark') the result is already truncated if there is a , in the number (1,25 gives 1) i.e this will be the case if the param is considered as a float. I cannot locate in this special pop-up window where (or if) the param 'mark' type is defined.
          Hide
          Tim Hunt added a comment -

          Have a look in behaviourbase.php at the get_expected_data method. That defines the type as question_attempt::PARAM_MARK, which is not one of the standard ones.

          That is dealt with by get_submitted_var in question/engine/questionattempt.php.

          You are right. It is not properly taking into account localised numbers. It is just using PARAM_NUMBER, which just casts the result to a PHP float.

          I think we should fix this in get_submitted_var. Do you want to try to devise a fix?

          Show
          Tim Hunt added a comment - Have a look in behaviourbase.php at the get_expected_data method. That defines the type as question_attempt::PARAM_MARK, which is not one of the standard ones. That is dealt with by get_submitted_var in question/engine/questionattempt.php. You are right. It is not properly taking into account localised numbers. It is just using PARAM_NUMBER, which just casts the result to a PHP float. I think we should fix this in get_submitted_var. Do you want to try to devise a fix?
          Hide
          Pierre Pichet added a comment -

          Do you want to try to devise a fix? Yes
          The problem should be solved here: ( in question attempt.php

          public function get_submitted_var($name, $type, $postdata = null) {
                  switch ($type) {
                      case self::PARAM_MARK:
                          // Special case to work around PARAM_NUMBER converting '' to 0.
                          $mark = $this->get_submitted_var($name, PARAM_RAW_TRIMMED, $postdata);
                          if ($mark === '') {
                              return $mark;
                          } else {
                              return $this->get_submitted_var($name, PARAM_NUMBER, $postdata);
                          }
          

          the conversion should be done on the PARAM_RAW_TRIMMED value
          replacing any , by . should be sufficient or we use full filtering using actual decsep and thousandsep

          Show
          Pierre Pichet added a comment - Do you want to try to devise a fix? Yes The problem should be solved here: ( in question attempt.php public function get_submitted_var($name, $type, $postdata = null) { switch ($type) { case self::PARAM_MARK: // Special case to work around PARAM_NUMBER converting '' to 0. $mark = $this->get_submitted_var($name, PARAM_RAW_TRIMMED, $postdata); if ($mark === '') { return $mark; } else { return $this->get_submitted_var($name, PARAM_NUMBER, $postdata); } the conversion should be done on the PARAM_RAW_TRIMMED value replacing any , by . should be sufficient or we use full filtering using actual decsep and thousandsep
          Hide
          Pierre Pichet added a comment -

          The following simple solution solve the problem.

          @@ -874,11 +882,14 @@ class question_attempt {
                           // Special case to work around PARAM_NUMBER converting '' to 0.
                           $mark = $this->get_submitted_var($name, PARAM_RAW_TRIMMED, $postdata);
                           if ($mark === '') {
                               return $mark;
                           } else {
          -                    return $this->get_submitted_var($name, PARAM_NUMBER, $postdata);
          +                    if (strpos($mark, ',') !== false){
          +                        $mark = str_replace(',', '.', $mark);
          +                    }    
          +                    return $mark ;
                           }
           
                       case self::PARAM_FILES:
                           return $this->process_response_files($name, $name, $postdata);
          

          I have some reports to finish so I presume that you can manage the gits in less time than me unless you want me to test other solutions.

          Show
          Pierre Pichet added a comment - The following simple solution solve the problem. @@ -874,11 +882,14 @@ class question_attempt { // Special case to work around PARAM_NUMBER converting '' to 0. $mark = $this->get_submitted_var($name, PARAM_RAW_TRIMMED, $postdata); if ($mark === '') { return $mark; } else { - return $this->get_submitted_var($name, PARAM_NUMBER, $postdata); + if (strpos($mark, ',') !== false){ + $mark = str_replace(',', '.', $mark); + } + return $mark ; } case self::PARAM_FILES: return $this->process_response_files($name, $name, $postdata); I have some reports to finish so I presume that you can manage the gits in less time than me unless you want me to test other solutions.
          Hide
          Pierre Pichet added a comment -

          I can find time to do the git and the tests

          Show
          Pierre Pichet added a comment - I can find time to do the git and the tests
          Hide
          Pierre Pichet added a comment -

          I set the language to english ( decsep = .) or french (decsep = ,) I either case I could write the mark either i.e. 0.95 or 0,95 .
          The result was stored correctly and always displayed following the language set.

          The git will follow.

          Show
          Pierre Pichet added a comment - I set the language to english ( decsep = .) or french (decsep = ,) I either case I could write the mark either i.e. 0.95 or 0,95 . The result was stored correctly and always displayed following the language set. The git will follow.
          Hide
          Tim Hunt added a comment -

          Pierre,

          Oops! I did not see that last comment until I had already made the git branches.

          Also, when I ran the unit tests, I found that the code needed to be adjusted slightly to get them all to pass.

          Anyway, could you have a look at my changes in git, and say if you think they are OK? Thanks.

          Show
          Tim Hunt added a comment - Pierre, Oops! I did not see that last comment until I had already made the git branches. Also, when I ran the unit tests, I found that the code needed to be adjusted slightly to get them all to pass. Anyway, could you have a look at my changes in git, and say if you think they are OK? Thanks.
          Hide
          Pierre Pichet added a comment -

          The main objective is that the user can put a good grade (i.e. 123.45) independently of the language setting of his keyboard.
          Mine is always set to french so I can write current text, email correctly and I like to be able to put decimal number in a form without having to switch my language keyboard.
          So the most valid test would be 123,45 in the two cases

          public function test_get_submitted_var_param_mark_number_uk_decimal()

          { $this->assertIdentical(123.45, $this->qa->get_submitted_var( 'name', question_attempt::PARAM_MARK, array('name' => '123,45'))); }

          I will finish the WebCT import ( they are new moodle users ) and come back to the number handling problem with languages as deutch.

          Thanks for handling this and your solution as always reflects your pro status.

          clean_param(str_replace(',', '.', $mark), PARAM_NUMBER);

          The output is a number and I learn that the , test if not necessary before using the str_replace function.
          You use it elsewhere in grading the question because the compute time is less although here this is not important ( the string is so short).

          Show
          Pierre Pichet added a comment - The main objective is that the user can put a good grade (i.e. 123.45) independently of the language setting of his keyboard. Mine is always set to french so I can write current text, email correctly and I like to be able to put decimal number in a form without having to switch my language keyboard. So the most valid test would be 123,45 in the two cases public function test_get_submitted_var_param_mark_number_uk_decimal() { $this->assertIdentical(123.45, $this->qa->get_submitted_var( 'name', question_attempt::PARAM_MARK, array('name' => '123,45'))); } I will finish the WebCT import ( they are new moodle users ) and come back to the number handling problem with languages as deutch. Thanks for handling this and your solution as always reflects your pro status. clean_param(str_replace(',', '.', $mark), PARAM_NUMBER); The output is a number and I learn that the , test if not necessary before using the str_replace function. You use it elsewhere in grading the question because the compute time is less although here this is not important ( the string is so short).
          Hide
          Tim Hunt added a comment -

          Pierre, thanks for your comments.

          I think this new code works better than the code that is currently there, so I am going to submit it for integration.

          If we need to do more work on this in the future, we can create a new issue.

          Show
          Tim Hunt added a comment - Pierre, thanks for your comments. I think this new code works better than the code that is currently there, so I am going to submit it for integration. If we need to do more work on this in the future, we can create a new issue.
          Hide
          Sam Hemelryk added a comment -

          Thanks Tim, this has now been integrated.

          Show
          Sam Hemelryk added a comment - Thanks Tim, this has now been integrated.
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          Works consistently switching between English and French.

          Tested in 2.1, 2.2 and master.

          Show
          Michael de Raadt added a comment - Test result: Success. Works consistently switching between English and French. Tested in 2.1, 2.2 and master.
          Hide
          Sam Hemelryk added a comment -

          Congratulations are in order, you've made it, or at least your code has!
          It's now part of Moodle and both the git and cvs repositories have been updated.

          This issue is being marked as fixed and closed.

          Thank you.

          Show
          Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: