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

Quiz settings validation does not trap plain text error in Grade boundary field

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1.- In a moodle 2.4 installation, create a new Quiz or (Edit an existing one).
      2.- On the Settings page, go down to the Overall feedback section.
      3.- In the Grade boundary 100% Feedback field enter some feedback text.
      4.- In the next Grade boundary box, instead of entering a number, enter a word, e.g. "good".
      5.- Now go down to the bottom of the Quiz Settings page and click the Save and display button.
      6.- Check that the quiz settings are not saved and you are returned to the Editing page.
      7.- On the Settings page, go down to the Overall feedback section and check that you can see the error message above the second feedback boundary box: Feedback grade boundaries must be either a percentage or a number. The value you entered in boundary 1 is not recognised.

      Show
      1.- In a moodle 2.4 installation, create a new Quiz or (Edit an existing one). 2.- On the Settings page, go down to the Overall feedback section. 3.- In the Grade boundary 100% Feedback field enter some feedback text. 4.- In the next Grade boundary box, instead of entering a number, enter a word, e.g. "good". 5.- Now go down to the bottom of the Quiz Settings page and click the Save and display button. 6.- Check that the quiz settings are not saved and you are returned to the Editing page. 7.- On the Settings page, go down to the Overall feedback section and check that you can see the error message above the second feedback boundary box: Feedback grade boundaries must be either a percentage or a number. The value you entered in boundary 1 is not recognised.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Quiz settings validation correctly detects a non-numeric value followed by the % sign, but it fails to detect a non-numeric value (i.e. plain text).
      I am attaching a simple patch, which does work but is maybe not the most elegant solution.
      Only tested on current 2.4 version but I expect the bug exists in previous Moodle 2 versions.

        Gliffy Diagrams

          Activity

          Hide
          timhunt Tim Hunt added a comment -

          I have fixed the coding style problems, and pushed it as a git commit. Does https://github.com/timhunt/moodle/compare/MDL-37114 look OK to you Joseph? Also, would you like to write some testing instructions?

          Show
          timhunt Tim Hunt added a comment - I have fixed the coding style problems, and pushed it as a git commit. Does https://github.com/timhunt/moodle/compare/MDL-37114 look OK to you Joseph? Also, would you like to write some testing instructions?
          Hide
          rezeau Joseph Rézeau added a comment -

          Thanks Tim, yes the commit you pushed works OK.
          I'll try to write some testing instructions.

          Show
          rezeau Joseph Rézeau added a comment - Thanks Tim, yes the commit you pushed works OK. I'll try to write some testing instructions.
          Hide
          rezeau Joseph Rézeau added a comment -

          Testing instructions.
          1.- In a moodle 2.4 installation, create a new Quiz or (Edit an existing one).
          2.- On the Settings page, go down to the Overall feedback section.
          3.- In the Grade boundary 100% Feedback field enter some feedback text.
          4.- In the next Grade boundary box, instead of entering a number, enter a word, e.g. "good".
          5.- Now go down to the bottom of the Quiz Settings page and click the Save and display button.
          6.- Check that the quiz settings are not saved and you are returned to the Editing page.
          7.- On the Settings page, go down to the Overall feedback section and check that you can see the error message above the second feedback boundary box: Feedback grade boundaries must be either a percentage or a number. The value you entered in boundary 1 is not recognised.

          Show
          rezeau Joseph Rézeau added a comment - Testing instructions. 1.- In a moodle 2.4 installation, create a new Quiz or (Edit an existing one). 2.- On the Settings page, go down to the Overall feedback section. 3.- In the Grade boundary 100% Feedback field enter some feedback text. 4.- In the next Grade boundary box, instead of entering a number, enter a word, e.g. "good". 5.- Now go down to the bottom of the Quiz Settings page and click the Save and display button. 6.- Check that the quiz settings are not saved and you are returned to the Editing page. 7.- On the Settings page, go down to the Overall feedback section and check that you can see the error message above the second feedback boundary box: Feedback grade boundaries must be either a percentage or a number. The value you entered in boundary 1 is not recognised.
          Hide
          rezeau Joseph Rézeau added a comment -

          @Tim,
          Why do you have the same validation "// Check the boundary value is a number or a percentage, and in range."
          in the quiz lib function quiz_process_options($quiz) ? It seems superfluous...

          Show
          rezeau Joseph Rézeau added a comment - @Tim, Why do you have the same validation "// Check the boundary value is a number or a percentage, and in range." in the quiz lib function quiz_process_options($quiz) ? It seems superfluous...
          Hide
          timhunt Tim Hunt added a comment -

          I think the code is duplicated because the data can also come from other sources: for example a web service call, or restoring a backup. Therefore it is better to validate it twice.

          However, it would be better if the code was re-factored so the logic was not duplicated. Oh well. It looks like the code in lib.php is already correct. Do you agree?

          Show
          timhunt Tim Hunt added a comment - I think the code is duplicated because the data can also come from other sources: for example a web service call, or restoring a backup. Therefore it is better to validate it twice. However, it would be better if the code was re-factored so the logic was not duplicated. Oh well. It looks like the code in lib.php is already correct. Do you agree?
          Hide
          rezeau Joseph Rézeau added a comment -

          @Tim,
          How does one manage to put the testing instructions in the Testing Instructions section at the top of an issue in the Tracker (like you did)? Can any-one do it?
          Joseph

          Show
          rezeau Joseph Rézeau added a comment - @Tim, How does one manage to put the testing instructions in the Testing Instructions section at the top of an issue in the Tracker (like you did)? Can any-one do it? Joseph
          Hide
          timhunt Tim Hunt added a comment -

          Do you see the edit button in the top left of the issue? That is one way. I hope you have permissions to see that button.

          You also get a chance when you click Submit for integration, but that probably requires an even rarer set of permissions.

          Show
          timhunt Tim Hunt added a comment - Do you see the edit button in the top left of the issue? That is one way. I hope you have permissions to see that button. You also get a chance when you click Submit for integration, but that probably requires an even rarer set of permissions.
          Hide
          timhunt Tim Hunt added a comment -

          OK, submitting for integration.

          Show
          timhunt Tim Hunt added a comment - OK, submitting for integration.
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated, thanks Joseph.

          Show
          poltawski Dan Poltawski added a comment - Integrated, thanks Joseph.
          Hide
          abgreeve Adrian Greeve added a comment -

          Tested on the 2.2, 2.3 and master integration branches.
          Everything worked as described.
          No issues found.
          Test passed.

          Show
          abgreeve Adrian Greeve added a comment - Tested on the 2.2, 2.3 and master integration branches. Everything worked as described. No issues found. Test passed.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Changes are now upstream, thanks for your collaboration!

          If you are going to have any celebration next days, enjoy with your gang, if not, too!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Changes are now upstream, thanks for your collaboration! If you are going to have any celebration next days, enjoy with your gang, if not, too! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/Jan/13