Moodle
  1. Moodle
  2. MDL-32358

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

    Details

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

      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.

        Activity

        Hide
        Pierre Pichet added a comment -

        Another simple bug in a learning process

        Show
        Pierre Pichet added a comment - Another simple bug in a learning process
        Hide
        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
        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.
        Hide
        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
        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.
        Hide
        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
        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.
        Hide
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - Yes, please fix the code style with an amend before rebasing if you have the time. Thanks.
        Hide
        Pierre Pichet added a comment -

        Amend and rebasing done .

        Show
        Pierre Pichet added a comment - Amend and rebasing done .
        Hide
        Dan Poltawski added a comment -

        Thanks Pierre,

        This has been integrated now

        Show
        Dan Poltawski added a comment - Thanks Pierre, This has been integrated now
        Hide
        Ankit Agarwal added a comment -

        works as expected!
        Thanks

        Show
        Ankit Agarwal added a comment - works as expected! Thanks
        Hide
        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
        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

          People

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

            Dates

            • Created:
              Updated:
              Resolved: