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

Add bulk unenrolment functionality to 'self enrolment' plugin

    Details

    • Testing Instructions:
      Hide
      1. Create a course with self enrolment
      2. Self-enrol as several users into it
      3. As teacher go to Course administration>Users>Enrolled users
      4. Filter users by the self enrolment method
      5. Make sure you can bulk edit and delete these enrolments

      Changes are also made to manual enrolment plugin to re-use the same function, make sure there are no regressions:

      1. Enrol several users into course manually
      2. As teacher go to Course administration>Users>Enrolled users
      3. Filter users by the self enrolment method
      4. Make sure you can bulk edit and delete these enrolments
      Show
      Create a course with self enrolment Self-enrol as several users into it As teacher go to Course administration>Users>Enrolled users Filter users by the self enrolment method Make sure you can bulk edit and delete these enrolments Changes are also made to manual enrolment plugin to re-use the same function, make sure there are no regressions: Enrol several users into course manually As teacher go to Course administration>Users>Enrolled users Filter users by the self enrolment method Make sure you can bulk edit and delete these enrolments
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_28_STABLE, MOODLE_29_STABLE, MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Pull Master Branch:
      MDL-31012-master

      Description

      Notice the missing functionality while testing MDL-30945.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment -

              oh, I thought it would be easier, reassigning back to hq, sorry.

              Show
              skodak Petr Skoda added a comment - oh, I thought it would be easier, reassigning back to hq, sorry.
              Hide
              keeneb13793e Becky Keene added a comment -

              This is a huge issue for our schools. We have a very fluid student population and it is a disappointment for our teachers to find that they have lost this capability in Moodle 2!

              Show
              keeneb13793e Becky Keene added a comment - This is a huge issue for our schools. We have a very fluid student population and it is a disappointment for our teachers to find that they have lost this capability in Moodle 2!
              Hide
              istanley iain stanley added a comment -

              Yes this is a very basic feature that should be available to teachers and owners of courses. I'm not quite sure why it was removed, but it would be nice to have it back

              Show
              istanley iain stanley added a comment - Yes this is a very basic feature that should be available to teachers and owners of courses. I'm not quite sure why it was removed, but it would be nice to have it back
              Hide
              mhjackson123 Michael Jackson added a comment -

              The lack of this feature is a real problem for our schools which have used self enrolment extensively.

              Show
              mhjackson123 Michael Jackson added a comment - The lack of this feature is a real problem for our schools which have used self enrolment extensively.
              Hide
              lsanocki Ł.Sanocki added a comment -

              I've upgraded from 1.9 to 2.5 and can't imagine that new version is lack of this feature.
              Is there a problem to add enrollment page in 'old style' (two windows with users). This method was fast.

              Show
              lsanocki Ł.Sanocki added a comment - I've upgraded from 1.9 to 2.5 and can't imagine that new version is lack of this feature. Is there a problem to add enrollment page in 'old style' (two windows with users). This method was fast.
              Hide
              kedstacuk Martin Woolley added a comment -

              This is a huge problem for us too.

              Show
              kedstacuk Martin Woolley added a comment - This is a huge problem for us too.
              Hide
              giugiupir giulia added a comment - - edited

              Hope see this basic & essential feature in future Moodle versions... (I'm working with 2.9)

              Show
              giugiupir giulia added a comment - - edited Hope see this basic & essential feature in future Moodle versions... (I'm working with 2.9)
              Hide
              marina Marina Glancy added a comment -

              This is a copy-paste from manual enrolment plugin. However there is a problem with behat tests at the moment, it does not pass because of MDL-54778. It passes if you comment out the line https://github.com/moodle/moodle/blob/master/lib/form/form.js#L106

              Show
              marina Marina Glancy added a comment - This is a copy-paste from manual enrolment plugin. However there is a problem with behat tests at the moment, it does not pass because of MDL-54778 . It passes if you comment out the line https://github.com/moodle/moodle/blob/master/lib/form/form.js#L106
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git master (2 errors / 1 warnings) [branch: MDL-31012-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (2/1) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              alvareza Anibal Alvarez added a comment -

              Why with "manual enrolement" there are the unenrolement function and no for self enrolment method?
              As Giulia: Hope see this basic & essential feature in future Moodle versions... (I'm working with 2.9)

              Show
              alvareza Anibal Alvarez added a comment - Why with "manual enrolement" there are the unenrolement function and no for self enrolment method? As Giulia: Hope see this basic & essential feature in future Moodle versions... (I'm working with 2.9)
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git master (0 errors / 2 warnings) [branch: MDL-31012-master | CI Job ] phplint (0/0) , phpcs (0/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              jaked Jake Dallimore added a comment -

              Hi Marina,
              This looks like quite an important and useful issue to solve - I'm surprised it hasn't got more votes to be honest. Definitely a useful feature to have, albeit a little hidden (not your patch, just the way it has been set up in the past).

              I like what you've done with the bulk_update_user_enrol implementation, rather than duplicating code in the different enrolment methods. Much tidier and simplifies each plugin's process() method significantly.

              It took me a while to get my head around the $instances array, as I could only see that it would ever hold the current enrolment method instance (i.e. Manual, Self, etc). I was going to suggest making a single call to bulk_update_user_enrol would be sufficient and cleaner, but after we spoke I can see that you are simply future-proofing this method - Preparing it for the possible scenario in which the user has multiple enrolment instances.

              Looks good a whole, just some minor things that should be addressed.

              1. Fix the cibot-related warnings.
              2. Missing a newline at the end of the file enrol/self/classes/editselectedusers_form.php.

              Otherwise, feel free to submit for integration when ready.

              Show
              jaked Jake Dallimore added a comment - Hi Marina, This looks like quite an important and useful issue to solve - I'm surprised it hasn't got more votes to be honest. Definitely a useful feature to have, albeit a little hidden (not your patch, just the way it has been set up in the past). I like what you've done with the bulk_update_user_enrol implementation, rather than duplicating code in the different enrolment methods. Much tidier and simplifies each plugin's process() method significantly. It took me a while to get my head around the $instances array, as I could only see that it would ever hold the current enrolment method instance (i.e. Manual, Self, etc). I was going to suggest making a single call to bulk_update_user_enrol would be sufficient and cleaner, but after we spoke I can see that you are simply future-proofing this method - Preparing it for the possible scenario in which the user has multiple enrolment instances. Looks good a whole, just some minor things that should be addressed. Fix the cibot-related warnings. Missing a newline at the end of the file enrol/self/classes/editselectedusers_form.php . Otherwise, feel free to submit for integration when ready.
              Hide
              marina Marina Glancy added a comment -

              Done,
              thanks Jake

              Show
              marina Marina Glancy added a comment - Done, thanks Jake
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git master (0 errors / 0 warnings) [branch: MDL-31012-master | CI Job ] More information about this report
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-31012 using repository: git://github.com/marinaglancy/moodle.git master (0 errors / 0 warnings) [branch: MDL-31012-master | CI Job ] More information about this report
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
              Hide
              markn Mark Nelson added a comment -

              Thanks for working on this.

              A couple of points before I continue testing -

              1. Would be great if there was a select all option.
              2. If you edit a whole group of the enrolment instances and alter the start time and end time to some time other than the current time, then re-edit the instances again but do not alter any time field you end up resetting the fields back to the current date. This seems wrong. You may want to bulk update a bunch of people by changing their 'status' to suspended but inadvertently alter the start and end time as well.
              Show
              markn Mark Nelson added a comment - Thanks for working on this. A couple of points before I continue testing - Would be great if there was a select all option. If you edit a whole group of the enrolment instances and alter the start time and end time to some time other than the current time, then re-edit the instances again but do not alter any time field you end up resetting the fields back to the current date. This seems wrong. You may want to bulk update a bunch of people by changing their 'status' to suspended but inadvertently alter the start and end time as well.
              Hide
              marina Marina Glancy added a comment - - edited

              3. Will be good if one does not need to filter by enrolment method before getting to the bulk enrolment functionality
              4. Will be good if there was an option to remove "start time" or "end time" in the edit form
              5. Will be good if the whole Moodle was intuitive and with great UI

              Let's fail all issues that are in testing based on the last point

              Or another option is to search for existing issues or create new ones for the points mentioned above.

              Show
              marina Marina Glancy added a comment - - edited 3. Will be good if one does not need to filter by enrolment method before getting to the bulk enrolment functionality 4. Will be good if there was an option to remove "start time" or "end time" in the edit form 5. Will be good if the whole Moodle was intuitive and with great UI Let's fail all issues that are in testing based on the last point Or another option is to search for existing issues or create new ones for the points mentioned above.
              Hide
              markn Mark Nelson added a comment -

              Really? Search for an issue on something specific to a new feature? The first point I said can be dealt with in another issue (it was a suggestion) but the second one is valid. It is not possible to bulk edit without over-writing dates. That is wrong and will throw people off and then they can never revert the change without knowing each date for each user. Imagine bulk editing 1000 people and then messing up the dates unintentionally.

              Show
              markn Mark Nelson added a comment - Really? Search for an issue on something specific to a new feature? The first point I said can be dealt with in another issue (it was a suggestion) but the second one is valid. It is not possible to bulk edit without over-writing dates. That is wrong and will throw people off and then they can never revert the change without knowing each date for each user. Imagine bulk editing 1000 people and then messing up the dates unintentionally.
              Hide
              marina Marina Glancy added a comment -

              ... Any further change in the bulk unenrolment or enrolment editing should be applied to the base classes used for both manual and (after this issue) self enrolment plugin. The number of improvements there is almost unlimited, I have only created MDL-54777 because it was annoying for me when writing behat tests

              Show
              marina Marina Glancy added a comment - ... Any further change in the bulk unenrolment or enrolment editing should be applied to the base classes used for both manual and (after this issue) self enrolment plugin. The number of improvements there is almost unlimited, I have only created MDL-54777 because it was annoying for me when writing behat tests
              Hide
              marina Marina Glancy added a comment -

              Sorry Mark, I did not realise that you were talking about the new bug and not existing functionality.
              I have created a follow up issue MDL-55056 (that relates to my #4) and added a patch to this issue.
              https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-31012-fix

              However I feel that the current functionality/API is so bad that maybe it is better to revert this issue at the moment and re-write the bulk actions completely. This can potentially break the existing enrolment plugins but it's better to do it right way.

              Show
              marina Marina Glancy added a comment - Sorry Mark, I did not realise that you were talking about the new bug and not existing functionality. I have created a follow up issue MDL-55056 (that relates to my #4) and added a patch to this issue. https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-31012-fix However I feel that the current functionality/API is so bad that maybe it is better to revert this issue at the moment and re-write the bulk actions completely. This can potentially break the existing enrolment plugins but it's better to do it right way.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Hi Marina,

              Just to confirm, you're suggesting reverting the patchset from this issue?

              Show
              dobedobedoh Andrew Nicols added a comment - Hi Marina, Just to confirm, you're suggesting reverting the patchset from this issue?
              Hide
              marina Marina Glancy added a comment - - edited

              yes, I think this is a deadend, we should think of better bulk actions UI and API that can apply to enrol_manual, enrol_self and any other individual enrolment method.

              At the moment I don't even think many users know about this functionality because they need to filter by the enrolment method to see the checkboxes.

              Show
              marina Marina Glancy added a comment - - edited yes, I think this is a deadend, we should think of better bulk actions UI and API that can apply to enrol_manual, enrol_self and any other individual enrolment method. At the moment I don't even think many users know about this functionality because they need to filter by the enrolment method to see the checkboxes.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Marina,

              I've now reverted this change. Details of the push are at https://gist.github.com/andrewnicols/5bdc7648510528b07f41ddad328713b6

              History rewritten for master only.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Marina, I've now reverted this change. Details of the push are at https://gist.github.com/andrewnicols/5bdc7648510528b07f41ddad328713b6 History rewritten for master only.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

                People

                • Votes:
                  30 Vote for this issue
                  Watchers:
                  18 Start watching this issue

                  Dates

                  • Created:
                    Updated: