Moodle
  1. Moodle
  2. MDL-37114

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

    Details

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

      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.

        Activity

        Hide
        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
        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
        Joseph Rézeau added a comment -

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

        Show
        Joseph Rézeau added a comment - Thanks Tim, yes the commit you pushed works OK. I'll try to write some testing instructions.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

        OK, submitting for integration.

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

        Integrated, thanks Joseph.

        Show
        Dan Poltawski added a comment - Integrated, thanks Joseph.
        Hide
        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
        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
        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
        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: