Moodle
  1. Moodle
  2. MDL-38315

Completion: Bugs with 'locked' status in activity form

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.9, 2.3.3, 2.4.1
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Activity completion
    • Labels:
      None
    • Testing Instructions:
      Hide

      0. Enable activity completion at site level and in a test course.
      1. Create a new forum. Set completion tracking to 'when conditions are met', tick both 'require view' and 'require posts' (leaving the latter at default value 1), and save and return to course.
      2. Turn editing off.

      • Note that the forum now shows an unticked box.

      3. Start a new discussion in the forum, with any text and name.
      4. Go back to the course page.

      • Note that the forum now shows a ticked box.

      5. Edit forum settings again. This time, just change the forum name e.g. by adding '2' to the end. Hit 'save and display'.

      EXPECTED: The forum is renamed.
      BEFORE FIX: The form reloads with an error message 'When you select automatic completion, you must also enable at least one requirement (below).'

      6. Edit forum settings again. Look in the completion settings.

      EXPECTED: The 'require posts' and 'view' tickboxes, which are currently disabled because completion is locked, should nevertheless still be ticked.
      BEFORE FIX: The tickboxes are unticked - completion options were saved even though completion was not unlocked.

      THE FOLLOWING TEST STEPS NEED TO BE REPEATED FOR THESE MODULES:

      • assign (in Moodle 2.4+ only)
      • choice
      • feedback (make visible in site settings if not showing)
      • forum
      • glossary
      • scorm

      (This is to test that I didn't break the saving of custom completion rule fields when the data isn't locked.)

      7. Create an instance of the module. Enable automatic completion, and tick all the automatic completion option boxes. (In the case of forum and glossary, you must also turn on the 'Aggregate type' option under 'Ratings' as otherwise the grade completion will cause an error. For SCORM I used an example SCORM package from http://www.xquestion.com/Examples.html.) Save and display.

      8. Edit settings. Verify that all the boxes are ticked. Now untick all the boxes except 'require view', save and display.

      9. Edit settings. Verify that the boxes (except require view) are now unticked. (Note: Completion will probably now be locked because you've viewed it, but this doesn't matter as you can still tell if the boxes are ticked or not.)

      Show
      0. Enable activity completion at site level and in a test course. 1. Create a new forum. Set completion tracking to 'when conditions are met', tick both 'require view' and 'require posts' (leaving the latter at default value 1), and save and return to course. 2. Turn editing off. Note that the forum now shows an unticked box. 3. Start a new discussion in the forum, with any text and name. 4. Go back to the course page. Note that the forum now shows a ticked box. 5. Edit forum settings again. This time, just change the forum name e.g. by adding '2' to the end. Hit 'save and display'. EXPECTED: The forum is renamed. BEFORE FIX: The form reloads with an error message 'When you select automatic completion, you must also enable at least one requirement (below).' 6. Edit forum settings again. Look in the completion settings. EXPECTED: The 'require posts' and 'view' tickboxes, which are currently disabled because completion is locked, should nevertheless still be ticked. BEFORE FIX: The tickboxes are unticked - completion options were saved even though completion was not unlocked. THE FOLLOWING TEST STEPS NEED TO BE REPEATED FOR THESE MODULES: assign (in Moodle 2.4+ only) choice feedback (make visible in site settings if not showing) forum glossary scorm (This is to test that I didn't break the saving of custom completion rule fields when the data isn't locked.) 7. Create an instance of the module. Enable automatic completion, and tick all the automatic completion option boxes. (In the case of forum and glossary, you must also turn on the 'Aggregate type' option under 'Ratings' as otherwise the grade completion will cause an error. For SCORM I used an example SCORM package from http://www.xquestion.com/Examples.html .) Save and display. 8. Edit settings. Verify that all the boxes are ticked. Now untick all the boxes except 'require view', save and display. 9. Edit settings. Verify that the boxes (except require view) are now unticked. (Note: Completion will probably now be locked because you've viewed it, but this doesn't matter as you can still tell if the boxes are ticked or not.)
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-38315-m24
    • Pull Master Branch:
      MDL-38315-master
    • Rank:
      48180

      Description

      I have duplicated this with forum, glossary and OU blog on 2.3.3+, and on demo.moodle.net using 2.4.1+. This occurs when requiring a certain number of posts. No changes can be made to any settings without removing activity completion requirements. I was not even able to change the max attachment size. It does not happen when allowing a student to manually mark an item complete or requiring a student to view to mark it complete.

      Discussion: https://moodle.org/mod/forum/discuss.php?d=223412

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Thanks for this report. This also happens on current master. I've written test instructions and will do a fix. (I expect somebody broke it as part of other changes to these forms.)

          Show
          Sam Marshall added a comment - Thanks for this report. This also happens on current master. I've written test instructions and will do a fix. (I expect somebody broke it as part of other changes to these forms.)
          Hide
          Sam Marshall added a comment -

          Extended test to cover related issues. This bug has been around forever, I'm kind of surprised nobody noticed it before (or maybe they did and I missed the report). I'm working on fixes.

          Show
          Sam Marshall added a comment - Extended test to cover related issues. This bug has been around forever, I'm kind of surprised nobody noticed it before (or maybe they did and I missed the report). I'm working on fixes.
          Hide
          Sam Marshall added a comment -

          Another slight test update. Also to note, while going through this test script I found an existing bug in the Choice module 'complete on submit' option (it wasn't possible to turn it off) which I am fixing as part of this.

          Show
          Sam Marshall added a comment - Another slight test update. Also to note, while going through this test script I found an existing bug in the Choice module 'complete on submit' option (it wasn't possible to turn it off) which I am fixing as part of this.
          Hide
          Sam Marshall added a comment -

          More test script updates - I'm at the end now and the code is nearly working, so hopefully that will be all of it. I also suspect the SCORM package options can't be turned off at present either (similar to choice) although not sure, that might be something I broke.

          Show
          Sam Marshall added a comment - More test script updates - I'm at the end now and the code is nearly working, so hopefully that will be all of it. I also suspect the SCORM package options can't be turned off at present either (similar to choice) although not sure, that might be something I broke.
          Hide
          Rob added a comment -

          Thank you for the quick action Sam. As a work around, my teachers have been successful in removing completion condition completely, making changes to settings, and adding completion requirements back afterward. I hope that information is helpful to another Moodler that encounters the bug.

          Show
          Rob added a comment - Thank you for the quick action Sam. As a work around, my teachers have been successful in removing completion condition completely, making changes to settings, and adding completion requirements back afterward. I hope that information is helpful to another Moodler that encounters the bug.
          Hide
          Sam Marshall added a comment -

          Here's the fixes, I'm about to submit for review.

          Show
          Sam Marshall added a comment - Here's the fixes, I'm about to submit for review.
          Hide
          Sam Marshall added a comment -

          Submitting for peer review. Explanation:

          1. Most of this change simply deals with making sure that values are NOT saved to the completion fields in the database, in the case when completion is locked.

          2. The core change for this is trivial (modedit.php) but for each module that has custom completion fields where they use checkboxes, I needed to fix it there as well. The reason it applies mainly to checkboxes is that with checkboxes, you need code that's like 'if it wasn't set at all, treat it as 0' so it isn't possible to distinguish between the checkbox being disabled (which it is when completion is locked) and being enabled but not ticked. So you need an extra condition on the already-existing completionlocked field.

          3. The diff looks shorter if you use --ignore-space-change (some of it is the same code but just indented). I did fix a few bugs as well so it isn't all identical.

          4. The change is identical [cherry-pick] on all branches except that in 2.3 the change to mod/assign/locallib.php has been removed (assign didn't support custom completion options in 2.3 so it's not needed).

          I also added this to the documentation for this API feature so that people don't make the same mistake in future (in the example get_data function):

          http://docs.moodle.org/dev/Activity_completion_API#Form_changes_for_completion_settings

          By the way, I'm sure the locking system used to work, but I'm not sure how it ever did! Maybe it only worked in our custom 1.9 implementation and never in 2.x...

          Show
          Sam Marshall added a comment - Submitting for peer review. Explanation: 1. Most of this change simply deals with making sure that values are NOT saved to the completion fields in the database, in the case when completion is locked. 2. The core change for this is trivial (modedit.php) but for each module that has custom completion fields where they use checkboxes, I needed to fix it there as well. The reason it applies mainly to checkboxes is that with checkboxes, you need code that's like 'if it wasn't set at all, treat it as 0' so it isn't possible to distinguish between the checkbox being disabled (which it is when completion is locked) and being enabled but not ticked. So you need an extra condition on the already-existing completionlocked field. 3. The diff looks shorter if you use --ignore-space-change (some of it is the same code but just indented). I did fix a few bugs as well so it isn't all identical. 4. The change is identical [cherry-pick] on all branches except that in 2.3 the change to mod/assign/locallib.php has been removed (assign didn't support custom completion options in 2.3 so it's not needed). I also added this to the documentation for this API feature so that people don't make the same mistake in future (in the example get_data function): http://docs.moodle.org/dev/Activity_completion_API#Form_changes_for_completion_settings By the way, I'm sure the locking system used to work, but I'm not sure how it ever did! Maybe it only worked in our custom 1.9 implementation and never in 2.x...
          Hide
          Nathan Hammond added a comment -

          Any update on the fix for this? I need to fix the text in a forum I've created and can't because I can't SAVE my changes (due to the ACTIVITY COMPLETION module thinking that I've not made check mark selections).

          Show
          Nathan Hammond added a comment - Any update on the fix for this? I need to fix the text in a forum I've created and can't because I can't SAVE my changes (due to the ACTIVITY COMPLETION module thinking that I've not made check mark selections).
          Hide
          Lorenzo Nicora added a comment -

          I may confirm the same problem exists on Moodle 2.2 (tested on 2.2.9+)

          Show
          Lorenzo Nicora added a comment - I may confirm the same problem exists on Moodle 2.2 (tested on 2.2.9+)
          Hide
          Sam Marshall added a comment -

          Nathan: I coded the fix on 5th March, it's just waiting for somebody to peer-review it. There's currently a backlog of 49 issues waiting for peer review. I'll see if I can pester somebody.

          Lorenzo: Moodle 2.2 support ended in December, except for serious security bugs - if you need fixes to other bugs such as this one, you'll have to either apply the patch yourself, or upgrade to a more recent Moodle version. (After the fix actually appears in a more recent Moodle version!) See http://docs.moodle.org/dev/Releases for info on release support.

          Show
          Sam Marshall added a comment - Nathan: I coded the fix on 5th March, it's just waiting for somebody to peer-review it. There's currently a backlog of 49 issues waiting for peer review. I'll see if I can pester somebody. Lorenzo: Moodle 2.2 support ended in December, except for serious security bugs - if you need fixes to other bugs such as this one, you'll have to either apply the patch yourself, or upgrade to a more recent Moodle version. (After the fix actually appears in a more recent Moodle version!) See http://docs.moodle.org/dev/Releases for info on release support.
          Hide
          Dan Poltawski added a comment -

          Sorry for us being so crap with reviews, thanks for your patience.

          Show
          Dan Poltawski added a comment - Sorry for us being so crap with reviews, thanks for your patience.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Thanks for the fixes, looks good to me.

          But just to cover my back, i've added Dan Marsden/Damyon to check their modules and Fred in case he thinks it might be impacted by all the changes in master.

          cheers!

          Show
          Dan Poltawski added a comment - Hi Sam, Thanks for the fixes, looks good to me. But just to cover my back, i've added Dan Marsden/Damyon to check their modules and Fred in case he thinks it might be impacted by all the changes in master. cheers!
          Hide
          Frédéric Massart added a comment -

          I don't know much about completion, but I had a quick look. I first tried to replicate the problem by following the testing instructions, but for some reason I had to untick "Require view" and keep "Require posts" to replicate the error on step 5. Also, there is a very high risk of conflict now as it seems that most of the logic has been moved to course/modlib.php. Apart from that, it looks fine to me (I didn't check the module-specific changes).

          Show
          Frédéric Massart added a comment - I don't know much about completion, but I had a quick look. I first tried to replicate the problem by following the testing instructions, but for some reason I had to untick "Require view" and keep "Require posts" to replicate the error on step 5. Also, there is a very high risk of conflict now as it seems that most of the logic has been moved to course/modlib.php. Apart from that, it looks fine to me (I didn't check the module-specific changes).
          Hide
          Sam Marshall added a comment -

          As suggested there was a conflict in modedit.php (luckily only one line was changed in that file so it was easy to find the right place in modlib.php). I've rebased the master version to current master.

          Show
          Sam Marshall added a comment - As suggested there was a conflict in modedit.php (luckily only one line was changed in that file so it was easy to find the right place in modlib.php). I've rebased the master version to current master.
          Hide
          Damyon Wiese added a comment -

          I checked the assign changes and they look fine. It's a shame we are pushing this completion logic into all the activities - but I don't think there is a better way. We will need to update the developer docs.

          Show
          Damyon Wiese added a comment - I checked the assign changes and they look fine. It's a shame we are pushing this completion logic into all the activities - but I don't think there is a better way. We will need to update the developer docs.
          Hide
          Sam Marshall added a comment -

          Thanks for reviews, now submitting for integration.

          I think completion logic belongs in the activities when it relates to activity-specific conditions (like require posts, for the forum). There might have been a more elegant way to do it though...

          Show
          Sam Marshall added a comment - Thanks for reviews, now submitting for integration. I think completion logic belongs in the activities when it relates to activity-specific conditions (like require posts, for the forum). There might have been a more elegant way to do it though...
          Hide
          Damyon Wiese added a comment -

          Thanks Sam,

          The patches all look correct, I tested forums on all branches and found no issues.

          I noticed you updated the dev docs already: http://docs.moodle.org/dev/Activity_completion_API

          (Thanks!)

          Integrated to 23, 24 and master.

          Show
          Damyon Wiese added a comment - Thanks Sam, The patches all look correct, I tested forums on all branches and found no issues. I noticed you updated the dev docs already: http://docs.moodle.org/dev/Activity_completion_API (Thanks!) Integrated to 23, 24 and master.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for 2.3, 2.4 and master

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4 and master Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: