Moodle
  1. Moodle
  2. MDL-36412

Grade condition mark range validation allows for impossible conditions

    Details

    • Testing Instructions:
      Hide

      (On a course, where availability is enabled.)

      1. Select to add a Label. In the form, type the label text.
      2. Under 'Grade condition', select 'Course total' must be at least 50% and less than 50%.
      3. Save changes.
      - A suitable error should be displayed next to the field.
      4. Change so it must be at least 50% and less than 51%.
      5. Save changes.
      - The creation should go through OK.
      6. Edit settings for a section on the course.
      7. Set a grade condition on course total for at least 50% and less than 50%.
      8. Save changes.
      - An error should be displayed.
      9. Change it to less than 51%.
      10. Save changes.
      - The edit should go through.

      Show
      (On a course, where availability is enabled.) 1. Select to add a Label. In the form, type the label text. 2. Under 'Grade condition', select 'Course total' must be at least 50% and less than 50%. 3. Save changes. - A suitable error should be displayed next to the field. 4. Change so it must be at least 50% and less than 51%. 5. Save changes. - The creation should go through OK. 6. Edit settings for a section on the course. 7. Set a grade condition on course total for at least 50% and less than 50%. 8. Save changes. - An error should be displayed. 9. Change it to less than 51%. 10. Save changes. - The edit should go through.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36412-master
    • Rank:
      45236

      Description

      I noted that, when adding a grade condition in conditional activities, it is possible to:

      • set percentage values less than 0%
      • set percentage values greater than 100%
      • set the "must be at least" value to be the same as the "and less than" value.

      The first two of these two possibilities are possibly acceptable under some unusual circumstances. Certainly, if the values were constrained to be between 0 and 100%, someone would come up with a case where this is not appropriate.

      For the final possibility, this is a boundary condition. I couldn't find the place where this is actually evaluated in code, but from reading the labels, if the two values are the same, it seems that you could create a condition that a student could never meet. If I am wrong, please correct me, but if not, the validation should probably change so that it does not allow this impossible range.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          OK, this is just a case where it was > and should have been >=. I have changed it for activities.

          For sections, I notice this entire section of the form validation had been omitted. I have added it in. Also, in the section validation there was a similar error (> instead of >=) for the date condition validation (which was not present for activities), so I changed that too.

          Show
          Sam Marshall added a comment - OK, this is just a case where it was > and should have been >=. I have changed it for activities. For sections, I notice this entire section of the form validation had been omitted. I have added it in. Also, in the section validation there was a similar error (> instead of >=) for the date condition validation (which was not present for activities), so I changed that too.
          Hide
          Sam Marshall added a comment -

          Requesting peer review. I have done this change only for master so far, it can be cherry-picked to other branches after review is done.

          Show
          Sam Marshall added a comment - Requesting peer review. I have done this change only for master so far, it can be cherry-picked to other branches after review is done.
          Hide
          Andrew Davis added a comment -

          This could really do with some unit tests. I'd recommend adding a file course/tests/editsection_form_test.php that puts validation($data, $files) through its paces.

          Show
          Andrew Davis added a comment - This could really do with some unit tests. I'd recommend adding a file course/tests/editsection_form_test.php that puts validation($data, $files) through its paces.
          Hide
          Sam Marshall added a comment -

          agree that would be helpful, I'll try to add that unit test at some point.

          Show
          Sam Marshall added a comment - agree that would be helpful, I'll try to add that unit test at some point.
          Hide
          Sam Hemelryk added a comment -

          Thanks Sam, putting this straight up for integration review as its been around for a while.
          Unit tests would be nice but I think this is fine to land without given its a simple and easy change to understand.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Sam, putting this straight up for integration review as its been around for a while. Unit tests would be nice but I think this is fine to land without given its a simple and easy change to understand. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Dan Poltawski added a comment -

          Hi Sam,

          This isn't cherry-picking cleanly into 2.3, any chance you could create a branch for it?

          thanks

          Show
          Dan Poltawski added a comment - Hi Sam, This isn't cherry-picking cleanly into 2.3, any chance you could create a branch for it? thanks
          Hide
          Sam Marshall added a comment -

          I've done a new branch based on MOODLE_23_STABLE. Change is identical - looks like the only reason it didn't cherry-pick is that some extra code below the part I added was present in 2.4 and not in 2.3.

          Show
          Sam Marshall added a comment - I've done a new branch based on MOODLE_23_STABLE. Change is identical - looks like the only reason it didn't cherry-pick is that some extra code below the part I added was present in 2.4 and not in 2.3.
          Hide
          Dan Poltawski added a comment -

          Thanks Sam, I probably could've resolved it myself, but in most cases the integrator is the worst person to that

          Integrated to master, 24 and 23.

          Show
          Dan Poltawski added a comment - Thanks Sam, I probably could've resolved it myself, but in most cases the integrator is the worst person to that Integrated to master, 24 and 23.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in Master, 2.4 and 2.3.

          Thanks for fixing that.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in Master, 2.4 and 2.3. Thanks for fixing that.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: