Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31594

Decimal in manual grading are truncated when using comma

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ppichet 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
            ppichet 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
            timhunt 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
            timhunt 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
            ppichet 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
            ppichet 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
            ppichet 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
            ppichet 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
            ppichet Pierre Pichet added a comment -

            I can find time to do the git and the tests

            Show
            ppichet Pierre Pichet added a comment - I can find time to do the git and the tests
            Hide
            ppichet 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
            ppichet 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
            timhunt 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
            timhunt 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
            ppichet 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
            ppichet 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
            timhunt 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
            timhunt 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Tim, this has now been integrated.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Tim, this has now been integrated.
            Hide
            salvetore 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
            salvetore 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
            samhemelryk 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
            samhemelryk 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:
                  Fix Release Date:
                  14/May/12