Moodle
  1. Moodle
  2. MDL-37974

Add "Disable course enrolment" enrol_manual expiration action

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.6
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ InSite administration ⟩ Plugins ⟩ Enrollments ⟩ Manual enrollments, set Enrolment expiration action to "Disable course enrolment".
      2/ Enrol some users
      3/ Edit enrolment to end in the past
      4/ Run enrol/manual/cli/sync.php in verbose mode
      5/ Verify the enrolments are suspended and roles kept in place
      6/ Repeat with "Disable course enrolment, remove roles"

      Show
      1/ InSite administration ⟩ Plugins ⟩ Enrollments ⟩ Manual enrollments, set Enrolment expiration action to "Disable course enrolment". 2/ Enrol some users 3/ Edit enrolment to end in the past 4/ Run enrol/manual/cli/sync.php in verbose mode 5/ Verify the enrolments are suspended and roles kept in place 6/ Repeat with "Disable course enrolment, remove roles"
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w24_MDL-37974_m26_manexpire
    • Rank:
      47749

      Description

      This is an enhancement to MDL-35063 "Add setting for cron action after enrolment expiration - enrol_manual".
      There was some discussion of it there. This adds an option to suspend the ser without deleting their roles
      and other information.

      With the recently merged MDL-35063, you can either have an expiration event AND delete data (roles and role dependent data),
      or not have an expiration event and not delete anything. This patch adds the option to fire an event but NOT remove roles.
      Like MDL-35063, this patch marks the enrolment "suspended", partially as a flag to avoid firing the event repeatedly.

      To reproduce:
      In Site administration ⟩ Plugins ⟩ Enrollments ⟩ Manual enrollments, set Enrolment expiration action to
      "Disable course enrolment and remove roles".
      Let an enrolment expire and the cron run.

      What happens:
      The enrolment is suspended, and the user data is removed. For example, with the "student" role removed, their grades are no longer available.

      What is desired:
      With this patch, one can avoid deleting student data by using the new ""Disable course enrolment" option
      rather than the existing ""Disable course enrolment and remove roles" option.

        Issue Links

          Activity

          Hide
          Ray Morris added a comment -

          I'm new to git, being a SVN user, so please excuse me if I got one of the URLs wrong or similar.

          Show
          Ray Morris added a comment - I'm new to git, being a SVN user, so please excuse me if I got one of the URLs wrong or similar.
          Hide
          Michael de Raadt added a comment -

          Thanks for sharing that.

          Hopefully Petr will be able to review that soon.l

          Show
          Michael de Raadt added a comment - Thanks for sharing that. Hopefully Petr will be able to review that soon.l
          Hide
          Ray Morris added a comment -

          Slightly extends MDL-35063

          Show
          Ray Morris added a comment - Slightly extends MDL-35063
          Hide
          Petr Škoda added a comment -

          Hello, there are some problems in the commit messages - see http://docs.moodle.org/dev/Commit_cheat_sheet

          All new features need to go to master only fist, it is now also mandatory to include unit tests.

          The rest looks ok, thanks!

          Show
          Petr Škoda added a comment - Hello, there are some problems in the commit messages - see http://docs.moodle.org/dev/Commit_cheat_sheet All new features need to go to master only fist, it is now also mandatory to include unit tests. The rest looks ok, thanks!
          Hide
          Ray Morris added a comment -

          I'm sorry, I'm somewhat new to git and Moodle's policies for how it's used.
          After reading the linked documentation I'm not sure what needs to be changed with the commit messages, nor how to do so. Is the message on the main commit okay? I'm not sure how to safely change the message "Fixing typo in 0119348" at this point, after it's been pushed.

          Show
          Ray Morris added a comment - I'm sorry, I'm somewhat new to git and Moodle's policies for how it's used. After reading the linked documentation I'm not sure what needs to be changed with the commit messages, nor how to do so. Is the message on the main commit okay? I'm not sure how to safely change the message "Fixing typo in 0119348" at this point, after it's been pushed.
          Hide
          Petr Škoda added a comment -

          there are multiple ways to do that, see http://git-scm.com/book

          for this type of change I would recommend just one commit, commits in moodle must always start with "MDL-xxxx "

          Show
          Petr Škoda added a comment - there are multiple ways to do that, see http://git-scm.com/book for this type of change I would recommend just one commit, commits in moodle must always start with "MDL-xxxx "
          Hide
          James Henestofel added a comment - - edited

          I'm looking forward to this fix. One thing I would like to add about your Description.

          'With this patch, one can avoid deleting student data by using the new ""Disable course enrolment" option
          rather than the existing ""Disable course enrolment and remove roles" option.'

          From this it seems you are saying that the student data is deleted. I've just tested this and as long as you give the user back the student role and keep them suspended. The user still won't have access but all the data comes back. The removal of the role is only hiding the users info from the instructor. I'm using Moodle 2.4.4 (Build: 20130513). Other versions may not do this...not sure I don't have time to test.

          Show
          James Henestofel added a comment - - edited I'm looking forward to this fix. One thing I would like to add about your Description. 'With this patch, one can avoid deleting student data by using the new ""Disable course enrolment" option rather than the existing ""Disable course enrolment and remove roles" option.' From this it seems you are saying that the student data is deleted. I've just tested this and as long as you give the user back the student role and keep them suspended. The user still won't have access but all the data comes back. The removal of the role is only hiding the users info from the instructor. I'm using Moodle 2.4.4 (Build: 20130513). Other versions may not do this...not sure I don't have time to test.
          Hide
          Ray Morris added a comment -

          James - I'm addressing Petr's comments right now, implementing the tests and combining it into one commit, so it's possible that it will integrated fairly soon. The patch linked above is working on our production site, though, so you can use it today if you wish.

          The data about the student - their name, etc. isn't deleted (the same student may be enrolled in other courses), but some data IS deleted on unenrolment. Creating a new enrollment does not bring everything back. With this patch, nothing should be deleted.

          Show
          Ray Morris added a comment - James - I'm addressing Petr's comments right now, implementing the tests and combining it into one commit, so it's possible that it will integrated fairly soon. The patch linked above is working on our production site, though, so you can use it today if you wish. The data about the student - their name, etc. isn't deleted (the same student may be enrolled in other courses), but some data IS deleted on unenrolment. Creating a new enrollment does not bring everything back. With this patch, nothing should be deleted.
          Hide
          James Henestofel added a comment -

          Not sure if we are talking about the same thing.

          For the option "Disable course enrolment and remove roles". This just removes the users role and suspends them from the course but they are technically still enrolled in the course but just don't have a role. This still may very well remove some data. But I quickly tested the assignment file upload and both the file and the grade returned when I gave the user the student role again.

          If that option does remove student data then I'm kind of in a pickle until your fix is integrated. I need to keep all student data in case they do re-enroll(during the add/drop period or are de-registered by accident). I've yet to run into an issue with the users data missing from the course after re-enrolling(keeping fingers crossed).

          Unfortunately I'm stuck with a hosting solution so I can't add your fix until its in core. Thanks for your work though.

          Show
          James Henestofel added a comment - Not sure if we are talking about the same thing. For the option "Disable course enrolment and remove roles". This just removes the users role and suspends them from the course but they are technically still enrolled in the course but just don't have a role. This still may very well remove some data. But I quickly tested the assignment file upload and both the file and the grade returned when I gave the user the student role again. If that option does remove student data then I'm kind of in a pickle until your fix is integrated. I need to keep all student data in case they do re-enroll(during the add/drop period or are de-registered by accident). I've yet to run into an issue with the users data missing from the course after re-enrolling(keeping fingers crossed). Unfortunately I'm stuck with a hosting solution so I can't add your fix until its in core. Thanks for your work though.
          Hide
          Ray Morris added a comment - - edited

          Petr - I have added tests and squashed them into a single commit as requested.
          https://github.com/MorrisR2/moodle/commit/03e63f6141d6e1157829ec47ee60616911985b6f

          I'm sorry, I don't know how to fix "All new features need to go to master only fist".
          A little help with what you're wanting me to do? I have been reading a lot about git, including http://git-scm.com/book , but that documentation is what I was referring to on 03 June - it says you should absolutely NOT combine commits after you've pushed them. I did force it through anyway as requested, but anyway the git docs aren't making clear to me what it is you want me to do in the "master only fist" case, and in the other the docs say I absolutely should NOT do what you asked. Thus I'm not sure how useful that doc is for understanding what you're wanting.

          Show
          Ray Morris added a comment - - edited Petr - I have added tests and squashed them into a single commit as requested. https://github.com/MorrisR2/moodle/commit/03e63f6141d6e1157829ec47ee60616911985b6f I'm sorry, I don't know how to fix "All new features need to go to master only fist". A little help with what you're wanting me to do? I have been reading a lot about git, including http://git-scm.com/book , but that documentation is what I was referring to on 03 June - it says you should absolutely NOT combine commits after you've pushed them. I did force it through anyway as requested, but anyway the git docs aren't making clear to me what it is you want me to do in the "master only fist" case, and in the other the docs say I absolutely should NOT do what you asked. Thus I'm not sure how useful that doc is for understanding what you're wanting.
          Hide
          Rex Lorenzo added a comment -

          Hey Ray, just looking at your branch and you committed some merge conflicts. Notice the <<<<<<<, =====, and >>>>>> lines. These are the same line notes that you get when you cannot cleanly merge branches in SVN. With these line notes the code will not run.

          See https://github.com/MorrisR2/moodle/commit/03e63f6141d6e1157829ec47ee60616911985b6f

          Please try to remove the conflict line notes (the <<<<<<<, =====, and >>>>>>) by picking which parts of the code you want to be kept from which branch and make sure the code runs on your local dev instance.

          Show
          Rex Lorenzo added a comment - Hey Ray, just looking at your branch and you committed some merge conflicts. Notice the <<<<<<<, =====, and >>>>>> lines. These are the same line notes that you get when you cannot cleanly merge branches in SVN. With these line notes the code will not run. See https://github.com/MorrisR2/moodle/commit/03e63f6141d6e1157829ec47ee60616911985b6f Please try to remove the conflict line notes (the <<<<<<<, =====, and >>>>>>) by picking which parts of the code you want to be kept from which branch and make sure the code runs on your local dev instance.
          Hide
          Ray Morris added a comment -
          Show
          Ray Morris added a comment - Rex, that should be fixed now. https://github.com/MorrisR2/moodle/commit/08214bee6b4e45fd8b02c9fe000fa93f7b887af4
          Hide
          Rex Lorenzo added a comment -

          Ray, looks good now. Just a note that if this goes into integration they are going to want you to remove the line change you have for version.php.

          Show
          Rex Lorenzo added a comment - Ray, looks good now. Just a note that if this goes into integration they are going to want you to remove the line change you have for version.php.
          Hide
          Petr Škoda added a comment -

          Thanks a lot for the patch, I have amended it a bit to match the recommended coding style.

          Show
          Petr Škoda added a comment - Thanks a lot for the patch, I have amended it a bit to match the recommended coding style.
          Hide
          Dan Poltawski added a comment -

          Petr: it might be good to tell Ray what you changed so he can understand for next time

          Show
          Dan Poltawski added a comment - Petr: it might be good to tell Ray what you changed so he can understand for next time
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks.

          Show
          Dan Poltawski added a comment - Integrated to master, thanks.
          Hide
          Ray Morris added a comment -

          > it might be good to tell Ray what you changed so he can understand for next time

          I have since learned of the Code Checker tool for coding style, so that may cover it.

          Show
          Ray Morris added a comment - > it might be good to tell Ray what you changed so he can understand for next time I have since learned of the Code Checker tool for coding style, so that may cover it.
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr - tested and passed.

          Show
          Sam Hemelryk added a comment - Thanks Petr - tested and passed.
          Hide
          Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: