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

Validation error does not always show correct messages for numerical and calculateds question types

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      To test, create a numerical question with units shown on select or radio element.
      On preview with adaptative mode verify that you have the correct error message

      1. use a , in your number if your are set in english , select a unit

      the error :Please enter your answer without using the thousand separator (,).

      2. use a , in your number if your are set in english , DO NOT select a unit

      the error : You must select a unit.

      This confirms that the error function have the same codeflow that the is_complete_response()

      3. put a unit in the number field and select or not a unit

      the error : You must enter a valid number. Do not include a unit in your response.

      and so on

      As calculated and calculatedsimple use the numerical renderer, there in no need to test them.

      Show
      To test, create a numerical question with units shown on select or radio element. On preview with adaptative mode verify that you have the correct error message 1. use a , in your number if your are set in english , select a unit the error :Please enter your answer without using the thousand separator (,). 2. use a , in your number if your are set in english , DO NOT select a unit the error : You must select a unit. This confirms that the error function have the same codeflow that the is_complete_response() 3. put a unit in the number field and select or not a unit the error : You must enter a valid number. Do not include a unit in your response. and so on As calculated and calculatedsimple use the numerical renderer, there in no need to test them.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      I was using my french keyboard so the , was used for the numbers.
      I received the message that the unit was not selected.
      This message comes from numerical/question.php
      public function get_validation_error(array $response) {
      which code reproduces the steps of public function is_complete_response(array $response)

      { to select the correct message. This will work as long as the two functions receive the same data. In numerical this imply answer and unit, the actual call in numerical/renderer.php is $question->get_validation_error(array('answer' => $currentanswer) the $selectedunit was forgotten so I received the no unit selected message. Adding the unitseletected {noformat}

      if ($qa->get_state() == question_state::$invalid)

      { $result .= html_writer::nonempty_tag('div', $question->get_validation_error(array('answer' => $currentanswer,'unit' => $selectedunit)), array('class' => 'validationerror')); }

       

      solves the problem.

        Gliffy Diagrams

          Attachments

            Activity

            ppichet Pierre Pichet created issue -
            ppichet Pierre Pichet made changes -
            Field Original Value New Value
            Assignee Tim Hunt [ timhunt ] Pierre Pichet [ ppichet ]
            timhunt Tim Hunt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels triaged
            Hide
            ppichet Pierre Pichet added a comment -

            Another simple bug in a learning process

            Show
            ppichet Pierre Pichet added a comment - Another simple bug in a learning process
            ppichet Pierre Pichet made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Pull Master Diff URL https://github.com/ppichet/moodle/compare/moodle%3Amaster...MDL-32358
            Pull Master Branch MDL-32358
            Pull from Repository https://github.com/ppichet/moodle/
            Peer reviewer timhunt
            Pull 2.1 Branch MDL-32358_21
            Pull 2.2 Diff URL https://github.com/ppichet/moodle/compare/moodle%3AMOODLE_22_STABLE...MDL-32358_22
            Pull 2.1 Diff URL https://github.com/ppichet/moodle/compare/moodle%3AMOODLE_21_STABLE...MDL-32358_21
            Pull 2.2 Branch MDL-32358_22
            Hide
            ppichet Pierre Pichet added a comment - - edited

            To test, create a numerical question with units shown on select or radio element.
            On preview with adaptative mode verify that you have the correct error message
            1. use a , in your number if your are set in english , select a unit

            the error :Please enter your answer without using the thousand separator (,).

            2. use a , in your number if your are set in english , DO NOT select a unit

            the error : You must select a unit.

            This confirms that the error function have the same codeflow that the is_complete_response()

            3. put a unit in the number field and select or not a unit

            the error : You must enter a valid number. Do not include a unit in your response.

            and so on

            As calculated and calculatedsimple use the numerical renderer, there in no need to test them.

            Show
            ppichet Pierre Pichet added a comment - - edited To test, create a numerical question with units shown on select or radio element. On preview with adaptative mode verify that you have the correct error message 1. use a , in your number if your are set in english , select a unit the error :Please enter your answer without using the thousand separator (,). 2. use a , in your number if your are set in english , DO NOT select a unit the error : You must select a unit. This confirms that the error function have the same codeflow that the is_complete_response() 3. put a unit in the number field and select or not a unit the error : You must enter a valid number. Do not include a unit in your response. and so on As calculated and calculatedsimple use the numerical renderer, there in no need to test them.
            timhunt Tim Hunt made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Pierre. This is a good fix. Submitting for integration now.

            I will just note that the separate unit field is only used if $question->has_separate_unit_field() returns true. However, when that returns false, we set $selectedunit to null, and it will cause no harm to pass the extra 'unit' => null to get_validation_error.

            Two minor comments:
            1. Moodle coding style likes a space after a ,
            2. We don't use the "Signed-off-by" thing in Moodle. I suppose it does no harm to include it, but it would be more consistent with other people to leave it out.

            Show
            timhunt Tim Hunt added a comment - Thanks Pierre. This is a good fix. Submitting for integration now. I will just note that the separate unit field is only used if $question->has_separate_unit_field() returns true. However, when that returns false, we set $selectedunit to null, and it will cause no harm to pass the extra 'unit' => null to get_validation_error. Two minor comments: 1. Moodle coding style likes a space after a , 2. We don't use the "Signed-off-by" thing in Moodle. I suppose it does no harm to include it, but it would be more consistent with other people to leave it out.
            timhunt Tim Hunt made changes -
            Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
            Hide
            timhunt Tim Hunt added a comment -

            One other thing Pierre:

            3. You should put the testing instructions in the special 'Testing instructions' field. You may have to use the edit issue button to do this.

            I have just done the necessary copy-and-paste.

            Show
            timhunt Tim Hunt added a comment - One other thing Pierre: 3. You should put the testing instructions in the special 'Testing instructions' field. You may have to use the edit issue button to do this. I have just done the necessary copy-and-paste.
            timhunt Tim Hunt made changes -
            Testing Instructions To test, create a numerical question with units shown on select or radio element.
            On preview with adaptative mode verify that you have the correct error message

            1. use a , in your number if your are set in english , select a unit

            the error :Please enter your answer without using the thousand separator (,).

            2. use a , in your number if your are set in english , DO NOT select a unit

            the error : You must select a unit.

            This confirms that the error function have the same codeflow that the is_complete_response()

            3. put a unit in the number field and select or not a unit

            the error : You must enter a valid number. Do not include a unit in your response.

            and so on

            As calculated and calculatedsimple use the numerical renderer, there in no need to test them.
            Hide
            ppichet Pierre Pichet added a comment -

            I take note of your comments. I did not know how to edit Testing instructions.

            I have noted the lines in the renderer

               if ($question->has_separate_unit_field()) {
                        $selectedunit = $qa->get_last_qt_var('unit');
                    } else {
                        $selectedunit = null;
                    }
             

            and I did tests with questions without units or with units in a single input field

            Show
            ppichet Pierre Pichet added a comment - I take note of your comments. I did not know how to edit Testing instructions. I have noted the lines in the renderer if ($question->has_separate_unit_field()) { $selectedunit = $qa->get_last_qt_var('unit'); } else { $selectedunit = null; } and I did tests with questions without units or with units in a single input field
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            "Moodle coding style likes a space after a ,"
            As I need to rebase everything, I could correct the space in the process.
            One way to do it could be to amend the commit ( as this is the last commit) in each version before
            doing the rebase for each of them.

            What do you suggest ?

            Show
            ppichet Pierre Pichet added a comment - Tim, "Moodle coding style likes a space after a ," As I need to rebase everything, I could correct the space in the process. One way to do it could be to amend the commit ( as this is the last commit) in each version before doing the rebase for each of them. What do you suggest ?
            Hide
            timhunt Tim Hunt added a comment -

            Yes, please fix the code style with an amend before rebasing if you have the time. Thanks.

            Show
            timhunt Tim Hunt added a comment - Yes, please fix the code style with an amend before rebasing if you have the time. Thanks.
            samhemelryk Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            Hide
            ppichet Pierre Pichet added a comment -

            Amend and rebasing done .

            Show
            ppichet Pierre Pichet added a comment - Amend and rebasing done .
            poltawski Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator poltawski
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Pierre,

            This has been integrated now

            Show
            poltawski Dan Poltawski added a comment - Thanks Pierre, This has been integrated now
            poltawski Dan Poltawski made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.1.6 [ 12052 ]
            Fix Version/s 2.2.3 [ 12053 ]
            Fix Version/s STABLE backlog [ 10463 ]
            salvetore Michael de Raadt made changes -
            Tester ankit_frenz
            ankit_frenz Ankit Agarwal made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            works as expected!
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - works as expected! Thanks
            ankit_frenz Ankit Agarwal made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Bonza mate!

            Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

            Hooroo

            Show
            poltawski Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            poltawski Dan Poltawski made changes -
            Integration date 19/Apr/12

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12