Moodle
  1. Moodle
  2. MDL-17929

Auto-Create Groups: Populate groups from Child Courses in META courses

    Details

    • Testing Instructions:
      Hide
      • Create a course A, enrol some users
      • Create another course B
      • Create a group called "group a" on course b
      • On course B, add a meta enrol method, selecting "course A" as the link and "group a" as the group
      • Make sure all users from course A are enroled on course B and are groups members for "group a"
      • Go to course A, unenrol some users, go back to course B and see that they not belong to the "group a" anymore
      • Go to course A, enrol some users, go back to course B and see that they now belong to the "group a"
      • Remove the meta enrol method from course B and see that everyone was removed from the group
      • On course B, add another meta enrol method, selecting same course A as link but selecting "none" as group. See that users are enrolled but not added to any group.

      Also play around with "Create new group" option. Try editing the meta enrolment and changing the group. Be creative

      Show
      Create a course A, enrol some users Create another course B Create a group called "group a" on course b On course B, add a meta enrol method, selecting "course A" as the link and "group a" as the group Make sure all users from course A are enroled on course B and are groups members for "group a" Go to course A, unenrol some users, go back to course B and see that they not belong to the "group a" anymore Go to course A, enrol some users, go back to course B and see that they now belong to the "group a" Remove the meta enrol method from course B and see that everyone was removed from the group On course B, add another meta enrol method, selecting same course A as link but selecting "none" as group. See that users are enrolled but not added to any group. Also play around with "Create new group" option. Try editing the meta enrolment and changing the group. Be creative
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull Master Branch:
      wip-MDL-17929-master

      Description

      It doesn't appear that one can auto-create Groups from the child courses in META courses. On the auto-create page in Groups, (group/autogroup.php?) it would be an excellent improvement if users could select to "create groups from Child Courses" so that groups could automatically be created if there are several child courses in a meta course.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Leighton added a comment -

            Definitely!

            This would save me many hours of administration. Please implement it as soon as you can.

            Show
            Dan Leighton added a comment - Definitely! This would save me many hours of administration. Please implement it as soon as you can.
            Hide
            David Smith added a comment -

            I agree, this is a modification we would find very, very useful.

            Show
            David Smith added a comment - I agree, this is a modification we would find very, very useful.
            Hide
            Thomas Robb added a comment -

            I see that this feature was recommended for 1.9.3, makes sense but still hasn't been implemented. Can it be implemented in 2.1? It would certainly save me, as well, considerable time when setting up child courses which require the same groups as the parent course.

            Show
            Thomas Robb added a comment - I see that this feature was recommended for 1.9.3, makes sense but still hasn't been implemented. Can it be implemented in 2.1? It would certainly save me, as well, considerable time when setting up child courses which require the same groups as the parent course.
            Hide
            Joe Murphy added a comment -

            This remains something which would be very useful. Our use case is that this is a frequent request when we have multiple faculty members using a single metacourse. (For example, 4 sections of the same Chemistry course.) Groups make the gradebook, quiz, and assignment modules much easier to navigate for the faculty members, but it takes significant human time to maintain the groups.

            Show
            Joe Murphy added a comment - This remains something which would be very useful. Our use case is that this is a frequent request when we have multiple faculty members using a single metacourse. (For example, 4 sections of the same Chemistry course.) Groups make the gradebook, quiz, and assignment modules much easier to navigate for the faculty members, but it takes significant human time to maintain the groups.
            Hide
            Lisa Kidder added a comment -

            Or perhaps the groups could be created when the child courses are added. A check box that says "Create group based on the course meta link" when you add the child course.

            Show
            Lisa Kidder added a comment - Or perhaps the groups could be created when the child courses are added. A check box that says "Create group based on the course meta link" when you add the child course.
            Hide
            Gilles-Philippe Leblanc added a comment -

            This task is marked for 1.9. Are you planning to do the same for version 2.x?

            Show
            Gilles-Philippe Leblanc added a comment - This task is marked for 1.9. Are you planning to do the same for version 2.x?
            Hide
            Bob Puffer added a comment -

            Yes, we are committed to groupings and available for this group only functionality and will continue to enhance it until Moodle core developers, in their wisdom kill this incredibly useful feature. Our CLAMP team is currently piloting 2.2.1 and we'll be working on porting our enhancements over the next four months.

            Show
            Bob Puffer added a comment - Yes, we are committed to groupings and available for this group only functionality and will continue to enhance it until Moodle core developers, in their wisdom kill this incredibly useful feature. Our CLAMP team is currently piloting 2.2.1 and we'll be working on porting our enhancements over the next four months.
            Hide
            Jean-Philippe Gaudreau added a comment -

            This patch may help solve the problem : http://tracker.moodle.org/browse/MDL-32091

            As I can see, the main problem with the situation is that it's really complicated to add members to a group from a meta link because you can't difference them in the potential members list (+ list only shows a max of 100 users).

            It sure don't auto sync with the course meta links but it's closer to how groups work right now (less side effects) and is really helping users to add members from specific meta link.

            Show
            Jean-Philippe Gaudreau added a comment - This patch may help solve the problem : http://tracker.moodle.org/browse/MDL-32091 As I can see, the main problem with the situation is that it's really complicated to add members to a group from a meta link because you can't difference them in the potential members list (+ list only shows a max of 100 users). It sure don't auto sync with the course meta links but it's closer to how groups work right now (less side effects) and is really helping users to add members from specific meta link.
            Hide
            Bob Puffer added a comment - - edited

            Hmmm... we've been doing this for about eight months now. Wasn't a problem. Perhaps I should clarify – we autocreate and maintain population for groups named after each child course. We don't give the instructor choice in the matter, though none have asked for choice – they've all been quite gratified to have this seemingly obvious capability where it didn't exist previously.

            Show
            Bob Puffer added a comment - - edited Hmmm... we've been doing this for about eight months now. Wasn't a problem. Perhaps I should clarify – we autocreate and maintain population for groups named after each child course. We don't give the instructor choice in the matter, though none have asked for choice – they've all been quite gratified to have this seemingly obvious capability where it didn't exist previously.
            Hide
            Jean-Philippe Gaudreau added a comment - - edited

            Hi Robert,

            Good I still think that auto creating group for course meta links (auto-synchronisation) is a good idea and I'm glad to hear that you're working on it! In fact, I think the best scenario is to have both solutions.

            Is you're patch soon to be released? Can we see the code on github?

            thx!

            Show
            Jean-Philippe Gaudreau added a comment - - edited Hi Robert, Good I still think that auto creating group for course meta links (auto-synchronisation) is a good idea and I'm glad to hear that you're working on it! In fact, I think the best scenario is to have both solutions. Is you're patch soon to be released? Can we see the code on github? thx!
            Hide
            Thomas Robb added a comment -

            Discussion of cohorts vs metacourses

            Show
            Thomas Robb added a comment - Discussion of cohorts vs metacourses
            Hide
            Grzegorz Woloszyn added a comment -

            Based on requests and feedback we have received from multiple instructors I can say that it would be useful to have more options:
            a) creating groups exactly reflecting child course assignments
            b) creating smaller groups of users belonging to the same child course - manually
            c) creating smaller groups of users belonging to the same child course - automatically

            Show
            Grzegorz Woloszyn added a comment - Based on requests and feedback we have received from multiple instructors I can say that it would be useful to have more options: a) creating groups exactly reflecting child course assignments b) creating smaller groups of users belonging to the same child course - manually c) creating smaller groups of users belonging to the same child course - automatically
            Hide
            Michael Drawe added a comment -

            We need the method to merge / to inherit the exact groups of the child course.

            In the past, each of our classes had got a new course per topic.

            Because of the growing numbers of courses for each class it is our new plan to have only one course per topic / issue. We only try to change the groups (classes). So it would be good, if we could manage that via Meta-Courses and inheritance groups. Thank you.

            Show
            Michael Drawe added a comment - We need the method to merge / to inherit the exact groups of the child course. In the past, each of our classes had got a new course per topic. Because of the growing numbers of courses for each class it is our new plan to have only one course per topic / issue. We only try to change the groups (classes). So it would be good, if we could manage that via Meta-Courses and inheritance groups. Thank you.
            Hide
            Matthew Cheng added a comment -

            Has there been any update on a patch that implements this?

            Show
            Matthew Cheng added a comment - Has there been any update on a patch that implements this?
            Hide
            Bob Puffer added a comment -

            I looked into duplicating our effort on the Moodle 2.x side and found the groups and groupings code to have entirely changed. Have not had anytime to pursue a port since we've just gone live with Moodle 2.x in February.

            Show
            Bob Puffer added a comment - I looked into duplicating our effort on the Moodle 2.x side and found the groups and groupings code to have entirely changed. Have not had anytime to pursue a port since we've just gone live with Moodle 2.x in February.
            Hide
            Melissa Benson added a comment -

            I'm looking into Meta Courses more with the popular requests of sharing resources in Moodle courses and I was surprised to see that Groups did not transfer over into META courses . Voted, hopefully this issue gets some activity!

            Show
            Melissa Benson added a comment - I'm looking into Meta Courses more with the popular requests of sharing resources in Moodle courses and I was surprised to see that Groups did not transfer over into META courses . Voted, hopefully this issue gets some activity!
            Hide
            Normand Charette added a comment -

            I agree this would be a nice addition to enhance the Meta courses. We continue to manually add users to Groups based upon Course sections (A,B,C,etc). This would make the process much easier. I am curious... What are the pitfalls of making a checkbox to automate the group creation process when we enroll meta courses?

            Show
            Normand Charette added a comment - I agree this would be a nice addition to enhance the Meta courses. We continue to manually add users to Groups based upon Course sections (A,B,C,etc). This would make the process much easier. I am curious... What are the pitfalls of making a checkbox to automate the group creation process when we enroll meta courses?
            Hide
            Bob Puffer added a comment -

            I looked at this again in June and found the coding project to be very complex. The major issues are keeping the groups synchronized with the child course enrollments.

            Show
            Bob Puffer added a comment - I looked at this again in June and found the coding project to be very complex. The major issues are keeping the groups synchronized with the child course enrollments.
            Hide
            Thomas Robb added a comment -

            I don't think we can assume that people will always want the child courses synced. Once the groups have been made, there can be a button for manual triggering a re-sync if desired, or (more complex) a cron job to regularly sync but this should be optional (or perhaps the default with a way to deselect it).

            I'd be happy if it just did the initial group assignments!

            Show
            Thomas Robb added a comment - I don't think we can assume that people will always want the child courses synced. Once the groups have been made, there can be a button for manual triggering a re-sync if desired, or (more complex) a cron job to regularly sync but this should be optional (or perhaps the default with a way to deselect it). I'd be happy if it just did the initial group assignments!
            Hide
            Melissa Benson added a comment -

            I agree that in some instances you wouldn't need or want the groups to sync so an optional sync would be ideal. My needs right now are to sync groups but I'm sure once that's implemented I'll run into a situation where it's not desired.

            Show
            Melissa Benson added a comment - I agree that in some instances you wouldn't need or want the groups to sync so an optional sync would be ideal. My needs right now are to sync groups but I'm sure once that's implemented I'll run into a situation where it's not desired.
            Hide
            Bob Puffer added a comment -

            I have a fork off from master that addresses this problem and the problem with how adding child courses to meta courses is handled (we revert back to the old way). Because the latter requires a (mostly) rewrite of enrol/meta/addinstance.php and that also would be where auto-groups would be created for the children, I've had to gang these two issues (this one and MDL-27628) together – I'm not going to maintain separate branches for such trivial code as required by the auto-groups. The auto-groups feature auto-populates the groups initially but does nothing for maintaining them as child course enrollments are updated (a significant effort). You can pick this up at https://github.com/bobpuffer/moodle_fork/tree/MDL-17929_27628-revert_meta_enrol-auto-groups_for_children

            Show
            Bob Puffer added a comment - I have a fork off from master that addresses this problem and the problem with how adding child courses to meta courses is handled (we revert back to the old way). Because the latter requires a (mostly) rewrite of enrol/meta/addinstance.php and that also would be where auto-groups would be created for the children, I've had to gang these two issues (this one and MDL-27628 ) together – I'm not going to maintain separate branches for such trivial code as required by the auto-groups. The auto-groups feature auto-populates the groups initially but does nothing for maintaining them as child course enrollments are updated (a significant effort). You can pick this up at https://github.com/bobpuffer/moodle_fork/tree/MDL-17929_27628-revert_meta_enrol-auto-groups_for_children
            Hide
            Thomas Robb added a comment -

            Just place the dearchived file in the blocks directory and go to site admin/notifications.

            Show
            Thomas Robb added a comment - Just place the dearchived file in the blocks directory and go to site admin/notifications.
            Hide
            Thomas Robb added a comment - - edited

            I've made a small block called "groupreg" that allows teachers to register students in a course and add them to existing or new groups from a .csv file. Hopefully, this will help offset the lack of functionality when adding child courses to a parent meta-course. The .csv file contains one or two fields. The first is either the students' usernames or email addresses. The second field is the new or existing group name. An Excel report of the results can be downloaded. Just place the dearchived file in the blocks directory and go to site admin/notifications.

            The block should be compatible with 2.3 or newer, but has not been tested on all versions. I've attached the block herewith. Feedback welcome!

            Show
            Thomas Robb added a comment - - edited I've made a small block called "groupreg" that allows teachers to register students in a course and add them to existing or new groups from a .csv file. Hopefully, this will help offset the lack of functionality when adding child courses to a parent meta-course. The .csv file contains one or two fields. The first is either the students' usernames or email addresses. The second field is the new or existing group name. An Excel report of the results can be downloaded. Just place the dearchived file in the blocks directory and go to site admin/notifications. The block should be compatible with 2.3 or newer, but has not been tested on all versions. I've attached the block herewith. Feedback welcome!
            Hide
            Joshua Holden added a comment -

            Hi, Thomas! How your block compare with the "Mass enrolments" plugin at https://moodle.org/plugins/view.php?plugin=local_mass_enroll? Thanks!

            Show
            Joshua Holden added a comment - Hi, Thomas! How your block compare with the "Mass enrolments" plugin at https://moodle.org/plugins/view.php?plugin=local_mass_enroll? Thanks!
            Hide
            Thomas Robb added a comment -

            In response to Joshua Holden >How your block compare with the "Mass enrolments" plugin...?

            Frankly, I wasn't aware of its existence, but the accomplish a similar task. Mine is a block rather than a line in the admin menu. It is simpler with no options apart from what you place in the .csv file. If a new group name is present, the group is created. If the group already exists, the student is added to the group. There are no problems with capabilities, that I know of. It's simple and it works!

            Show
            Thomas Robb added a comment - In response to Joshua Holden >How your block compare with the "Mass enrolments" plugin...? Frankly, I wasn't aware of its existence, but the accomplish a similar task. Mine is a block rather than a line in the admin menu. It is simpler with no options apart from what you place in the .csv file. If a new group name is present, the group is created. If the group already exists, the student is added to the group. There are no problems with capabilities, that I know of. It's simple and it works!
            Hide
            Christopher Fuhrman added a comment -

            It think this feature should be something simpler and on a higher level.

            It would be logical and useful if every child course simply had its own group (that exists automatically, doesn't require a manual step, and is automatically updated as the enrollment changes in the child course). The name for the group would be the same as the name for the child course. Call it a "dynamic group" function for child courses if you like.

            Show
            Christopher Fuhrman added a comment - It think this feature should be something simpler and on a higher level. It would be logical and useful if every child course simply had its own group (that exists automatically, doesn't require a manual step, and is automatically updated as the enrollment changes in the child course). The name for the group would be the same as the name for the child course. Call it a "dynamic group" function for child courses if you like.
            Hide
            Nadav Kavalerchik added a comment -

            Adding Moodle 2.5 since it is still relevant to Moodle 2.5

            Show
            Nadav Kavalerchik added a comment - Adding Moodle 2.5 since it is still relevant to Moodle 2.5
            Hide
            Paul Holden added a comment -

            Still relevant in Moodle 2.6 According to the following docs page this feature did exist at some point in the past:

            http://docs.moodle.org/22/en/Metacourse#Groups_do_not_transfer

            Show
            Paul Holden added a comment - Still relevant in Moodle 2.6 According to the following docs page this feature did exist at some point in the past: http://docs.moodle.org/22/en/Metacourse#Groups_do_not_transfer
            Hide
            Paul Holden added a comment -

            I've created a plugin for Moodle 2.6 that adds this functionality; once meta-course enrolments have been set up, any amendments made to groups in 'child' courses will automatically be reflected in the linked meta-course. The link between the groups in each course is then maintained.

            https://moodle.org/plugins/view.php?plugin=local_metagroups

            Show
            Paul Holden added a comment - I've created a plugin for Moodle 2.6 that adds this functionality; once meta-course enrolments have been set up, any amendments made to groups in 'child' courses will automatically be reflected in the linked meta-course. The link between the groups in each course is then maintained. https://moodle.org/plugins/view.php?plugin=local_metagroups
            Hide
            Wen Hao Chuang added a comment -

            Hi Paul, thanks for this plugin.
            Do you know if this plugin would work for Moodle 2.4.x and 2.7.x? Please let us know. Thanks!

            Show
            Wen Hao Chuang added a comment - Hi Paul, thanks for this plugin. Do you know if this plugin would work for Moodle 2.4.x and 2.7.x? Please let us know. Thanks!
            Hide
            Bob Puffer added a comment -

            Paul, I'd really like to get away from creating groups at all for child courses and subscribe to the idea that all I need automated is a group for each child course and maintenance of the members based on child course enrollments. Maybe that's what your plugin does and I've just misunderstood

            Show
            Bob Puffer added a comment - Paul, I'd really like to get away from creating groups at all for child courses and subscribe to the idea that all I need automated is a group for each child course and maintenance of the members based on child course enrollments. Maybe that's what your plugin does and I've just misunderstood
            Hide
            Paul Holden added a comment -

            Wen Hao Chuang, the plugin requires at least Moodle 2.6 to function and should continue to work in 2.7.x

            Bob Puffer, the plugin maintains actual groups/membership from child courses, so you would need to create these in your child courses before the plugin can propagate them up to the linked meta-course.

            Show
            Paul Holden added a comment - Wen Hao Chuang , the plugin requires at least Moodle 2.6 to function and should continue to work in 2.7.x Bob Puffer , the plugin maintains actual groups/membership from child courses, so you would need to create these in your child courses before the plugin can propagate them up to the linked meta-course.
            Hide
            Willy Lee added a comment -

            Nice plugin Paul Holden!

            Like some of the other commenters, we'd be interested in getting it to sync a group with the whole linked course's enrolment. I've looked at your plugin code as well as the syncing code in the meta enrolment plugin and I can see how we could make this happen. Not sure how much value would be in getting these in the same plugin. Do you think that's worth it, or should I make a completely separate one for making groups from the course enrolment of the linked courses?

            Show
            Willy Lee added a comment - Nice plugin Paul Holden ! Like some of the other commenters, we'd be interested in getting it to sync a group with the whole linked course's enrolment. I've looked at your plugin code as well as the syncing code in the meta enrolment plugin and I can see how we could make this happen. Not sure how much value would be in getting these in the same plugin. Do you think that's worth it, or should I make a completely separate one for making groups from the course enrolment of the linked courses?
            Hide
            Mark Pearson added a comment -

            Go for it Willy. There's only 114 votes for this issue! Faculty are baffled as to why this is not default behaviour. If the gradebook is viewable by groups that would be the icing on the cake.

            Show
            Mark Pearson added a comment - Go for it Willy. There's only 114 votes for this issue! Faculty are baffled as to why this is not default behaviour. If the gradebook is viewable by groups that would be the icing on the cake.
            Hide
            Willy Lee added a comment -

            Paul is taking a look at what I've got and we hope to put it out next week.

            The gradebook can be filtered by groups if you set a primary grouping in the course settings. I'm not sure that's exactly what you are looking for, but it works.

            Show
            Willy Lee added a comment - Paul is taking a look at what I've got and we hope to put it out next week. The gradebook can be filtered by groups if you set a primary grouping in the course settings. I'm not sure that's exactly what you are looking for, but it works.
            Hide
            Willy Lee added a comment -

            After forking Paul's work and having him look over it, I'm happy to release our local plugin, local_metasync.

            https://github.com/willylee/moodle-local_metasync

            I'll be doing some tweaking and will try to get it in the plugins directory soon, but I believe it to be feature complete.

            When you first install it, you'll want to run the CLI script to create groups for all your current metalinks, but afterwards, the plugin watches for every subsequent enrollment to keep everything in sync.

            Show
            Willy Lee added a comment - After forking Paul's work and having him look over it, I'm happy to release our local plugin, local_metasync. https://github.com/willylee/moodle-local_metasync I'll be doing some tweaking and will try to get it in the plugins directory soon, but I believe it to be feature complete. When you first install it, you'll want to run the CLI script to create groups for all your current metalinks, but afterwards, the plugin watches for every subsequent enrollment to keep everything in sync.
            Hide
            Marina Glancy added a comment -

            Hi, I've just created MDL-48346 as the suggested solution - automatically synchronise groups with enrolment methods.

            Show
            Marina Glancy added a comment - Hi, I've just created MDL-48346 as the suggested solution - automatically synchronise groups with enrolment methods.
            Hide
            Daniel Neis added a comment -

            Hello,

            I've made a patch to this, based on existing code.
            Hope to add it to core after 114 votes and a simple patch =)

            Kind regards,
            Daniel

            Show
            Daniel Neis added a comment - Hello, I've made a patch to this, based on existing code. Hope to add it to core after 114 votes and a simple patch =) Kind regards, Daniel
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master (8 errors / 0 warnings) [branch: MDL-17929 | CI Job ] phplint (0/0) , php (8/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Kevin Wiliarty added a comment -

            To the testing instructions I suggest we add that when users are suspended from Course A they also be removed from the corresponding group in Course B. It is not enough to let them be suspended users in Course B. Imagine there is a Course C that is also a child course of Course B. User X starts out in Course A and so is in group A in Course B. Imagine then that User X's enrollment is deactivated in Course B. User X should not only be suspended in Course B, but removed from group A so that if/when user X is enrolled in Course C and made an active user in Course B, user X will not still be in group A.

            This may sound far-flung, but really it's not. At my institution it happens regularly as students switch from one (child course) section of a bigger (metacourse) class to another.

            Show
            Kevin Wiliarty added a comment - To the testing instructions I suggest we add that when users are suspended from Course A they also be removed from the corresponding group in Course B. It is not enough to let them be suspended users in Course B. Imagine there is a Course C that is also a child course of Course B. User X starts out in Course A and so is in group A in Course B. Imagine then that User X's enrollment is deactivated in Course B. User X should not only be suspended in Course B, but removed from group A so that if/when user X is enrolled in Course C and made an active user in Course B, user X will not still be in group A. This may sound far-flung, but really it's not. At my institution it happens regularly as students switch from one (child course) section of a bigger (metacourse) class to another.
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-17929 using repository: https://github.com/danielneis/moodle

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master (0 errors / 0 warnings) [branch: MDL-17929 | CI Job ] More information about this report
            Hide
            Marina Glancy added a comment - - edited

            Thank you Daniel, this looks great.
            Can you please not use the strings from another plugin (enrol_cohort) and instead copy them into this one?

            We really need to have automated tests with this issue (unittests or behat) - such functionality needs to be automatically tested on all databases. Do you think you'd be able to create one?

            P.S. FYI enrol_cohort is evolving as we speak: MDL-49380

            Show
            Marina Glancy added a comment - - edited Thank you Daniel, this looks great. Can you please not use the strings from another plugin (enrol_cohort) and instead copy them into this one? We really need to have automated tests with this issue (unittests or behat) - such functionality needs to be automatically tested on all databases. Do you think you'd be able to create one? P.S. FYI enrol_cohort is evolving as we speak: MDL-49380
            Hide
            Damyon Wiese added a comment -

            Hi Daniel, thanks for this patch. I can see the code is 99% the same as cohort group sync - but still cohort group sync has unit tests that ensure the functionality is working now, and keeps working in future. We really cannot add new features like this that are not covered by tests.

            Cohort sync is also getting a new feature this week to create a new group if requested which would be a nice addition here too (definitely not a blocker).

            As for the strings mentioned by Marina - an AMOS command in the commit message would be the best way to handle this.

            These are not big changes - sorry this did not get a look further out from the freeze. I'll keep this under review and see if you managed to add the tests/string fixes tomorrow.

            Show
            Damyon Wiese added a comment - Hi Daniel, thanks for this patch. I can see the code is 99% the same as cohort group sync - but still cohort group sync has unit tests that ensure the functionality is working now, and keeps working in future. We really cannot add new features like this that are not covered by tests. Cohort sync is also getting a new feature this week to create a new group if requested which would be a nice addition here too (definitely not a blocker). As for the strings mentioned by Marina - an AMOS command in the commit message would be the best way to handle this. These are not big changes - sorry this did not get a look further out from the freeze. I'll keep this under review and see if you managed to add the tests/string fixes tomorrow.
            Hide
            Damyon Wiese added a comment -

            Well - this issue does have a lot of votes, and the patch is almost there. Sending to integration and the integrators can decide if it is close enough to get over the line. If you can look at the AMOS and unit tests on Monday/Tuesday this would help the integrators greatly.

            Thanks!

            Show
            Damyon Wiese added a comment - Well - this issue does have a lot of votes, and the patch is almost there. Sending to integration and the integrators can decide if it is close enough to get over the line. If you can look at the AMOS and unit tests on Monday/Tuesday this would help the integrators greatly. Thanks!
            Hide
            CiBoT added a comment -

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

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

            Fails against automated checks.

            Checked MDL-17929 using repository: https://github.com/danielneis/moodle

            • master [branch: MDL-17929 | CI Job]
              • Info: the branch is based off moodle.git
              • Info: base commit 1d3fd63f97de06d447f76591055b716436718df5 being used as initial commit.

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master [branch: MDL-17929 | CI Job ] Info: the branch is based off moodle.git Info: base commit 1d3fd63f97de06d447f76591055b716436718df5 being used as initial commit. More information about this report
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi,

            because of some coincidences & missing controls, last pre-check runs by CiBoT (maybe) did end incorrectly above.

            FYI, the issue has been tracked @ MDLSITE-3940, where we are introducing some extra validations to avoid this to happen again.

            Once applied, CiBoT checks will be re-executed again to have the results updated here.

            Sorry for the noise and the troubles, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi, because of some coincidences & missing controls, last pre-check runs by CiBoT (maybe) did end incorrectly above. FYI, the issue has been tracked @ MDLSITE-3940 , where we are introducing some extra validations to avoid this to happen again. Once applied, CiBoT checks will be re-executed again to have the results updated here. Sorry for the noise and the troubles, ciao
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-17929 using repository: https://github.com/danielneis/moodle

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master (0 errors / 0 warnings) [branch: MDL-17929 | CI Job ] More information about this report
            Hide
            David Monllaó added a comment - - edited

            Thanks for your contribution Daniel, but I'm afraid that we need to fix a few things before this is ready for integration, would be great to address them as many people is interested in this feature:

            1. In sites with just one course enrol/meta/addinstance.php I can see PHP notices, if we are listing the current course groups we should use the current course context not the context groups we link context

              Notice: Undefined variable: coursecontext in /home/davidm/Desktop/moodlecode/INTEGRATION/master/enrol/meta/addinstance_form.php on line 69
              

            2. Would be better to add a new string rather than depending on another plugin's language strings https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-842b02c6817b091b7d4e521eeb84e266R77
            3. As Marina said, would be great to have unit test or acceptance test for this new feature, enrol/cohort/tests/sync_test.php is a good example that covers many cases
            4. enrol_meta_sync is not only called from CLI_SCRIPTs so we should not call https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-3b07a91899c1565b7a966e5d9d8b2457R571 as it is being displayed in moodle's UI. Going further than that, if we are getting g.name and e.courseid just to show this message I would re-think if the mtrace message is necessary.
            5. Continuing with https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-3b07a91899c1565b7a966e5d9d8b2457R559, I may be missing something, but I think that we can improve these 2 db queries. I think we don't need to join with user_enrolments. I haven't tried it but it should be enough with something like:

              SELECT gm.*
              FROM {group_members} gm
              JOIN {enrol} e ON gm.component = 'enrol_meta' AND gm.itemid = e.id
              WHERE gm.courseid != e.customid2
              

            Or switching JOIN enrol conditions and WHERE conditions. I haven't run EXPLAIN on them. If we don't filter by courseid we neither need to join with groups (unless we want that mtrace()) we should use our indexes. Also, I think that given the number of updates on mdl_enrol we could perfectly add an index for customid1 and customid2 (part of this issue?) and (not sure about this as group_members has many more updates) another one for gm.component + gm.itemid

            Show
            David Monllaó added a comment - - edited Thanks for your contribution Daniel, but I'm afraid that we need to fix a few things before this is ready for integration, would be great to address them as many people is interested in this feature: In sites with just one course enrol/meta/addinstance.php I can see PHP notices, if we are listing the current course groups we should use the current course context not the context groups we link context Notice: Undefined variable: coursecontext in /home/davidm/Desktop/moodlecode/INTEGRATION/master/enrol/meta/addinstance_form.php on line 69 Would be better to add a new string rather than depending on another plugin's language strings https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-842b02c6817b091b7d4e521eeb84e266R77 As Marina said, would be great to have unit test or acceptance test for this new feature, enrol/cohort/tests/sync_test.php is a good example that covers many cases enrol_meta_sync is not only called from CLI_SCRIPTs so we should not call https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-3b07a91899c1565b7a966e5d9d8b2457R571 as it is being displayed in moodle's UI. Going further than that, if we are getting g.name and e.courseid just to show this message I would re-think if the mtrace message is necessary. Continuing with https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929#diff-3b07a91899c1565b7a966e5d9d8b2457R559 , I may be missing something, but I think that we can improve these 2 db queries. I think we don't need to join with user_enrolments. I haven't tried it but it should be enough with something like: SELECT gm.* FROM {group_members} gm JOIN {enrol} e ON gm.component = 'enrol_meta' AND gm.itemid = e.id WHERE gm.courseid != e.customid2 Or switching JOIN enrol conditions and WHERE conditions. I haven't run EXPLAIN on them. If we don't filter by courseid we neither need to join with groups (unless we want that mtrace()) we should use our indexes. Also, I think that given the number of updates on mdl_enrol we could perfectly add an index for customid1 and customid2 (part of this issue?) and (not sure about this as group_members has many more updates) another one for gm.component + gm.itemid
            Hide
            CiBoT added a comment -

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

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

            Hello,

            thanks for your review =)

            I've fixed the string usage and also the missing $coursecontext .
            I've also tried to copy some unit tests from the cohort plugin but the sequence used on both plugin differs so much that i could not copy much things. I would appreciate if someone with better skills/knowledge could help.

            About the queries, as I said before, i've just copy/paste the code from cohort plugin for it to work, so if we can make it better here, we should make it better both points or, better yet, find a way to deduplicate this code.

            Kind regards,
            Daniel

            Show
            Daniel Neis added a comment - Hello, thanks for your review =) I've fixed the string usage and also the missing $coursecontext . I've also tried to copy some unit tests from the cohort plugin but the sequence used on both plugin differs so much that i could not copy much things. I would appreciate if someone with better skills/knowledge could help. About the queries, as I said before, i've just copy/paste the code from cohort plugin for it to work, so if we can make it better here, we should make it better both points or, better yet, find a way to deduplicate this code. Kind regards, Daniel
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master (49 errors / 2 warnings) [branch: MDL-17929 | CI Job ] phplint (1/0) , php (48/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Daniel Neis added a comment - - edited

            Hello,

            i've fixed the phplint error.
            The other errors are from code copied from cohort or the code already on meta codebase.
            The person who will add the tests could fix it

            Show
            Daniel Neis added a comment - - edited Hello, i've fixed the phplint error. The other errors are from code copied from cohort or the code already on meta codebase. The person who will add the tests could fix it
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/danielneis/moodle master (48 errors / 2 warnings) [branch: MDL-17929 | CI Job ] phplint (0/0) , php (48/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            David Monllaó added a comment -

            Thanks for the fixes Daniel. Let's see if someone has some time to help here, the thing is that even though we have many unit and acceptance tests that runs automatically, every time we change code there is some risk of breaking stuff, but when we introduce new code we try to do things correctly adding new tests and passing all the required checks.

            Show
            David Monllaó added a comment - Thanks for the fixes Daniel. Let's see if someone has some time to help here, the thing is that even though we have many unit and acceptance tests that runs automatically, every time we change code there is some risk of breaking stuff, but when we introduce new code we try to do things correctly adding new tests and passing all the required checks.
            Hide
            Marina Glancy added a comment -

            I thought about helping with tests but looks like it needs more help than that:

            Fatal error: Class 'course_context' not found in /home/marina/repositories/master/moodle/enrol/meta/addinstance_form.php on line 69
            

            Show
            Marina Glancy added a comment - I thought about helping with tests but looks like it needs more help than that: Fatal error: Class 'course_context' not found in /home/marina/repositories/master/moodle/enrol/meta/addinstance_form.php on line 69
            Hide
            Marina Glancy added a comment -

            and that....

            PHP Fatal error:  Call to undefined function groups_create_group() in /home/marina/repositories/behat_master/moodle/enrol/meta/tests/plugin_test.php on line 103
             
            Fatal error: Call to undefined function groups_create_group() in /home/marina/repositories/behat_master/moodle/enrol/meta/tests/plugin_test.php on line 103
            

            Show
            Marina Glancy added a comment - and that.... PHP Fatal error: Call to undefined function groups_create_group() in /home/marina/repositories/behat_master/moodle/enrol/meta/tests/plugin_test.php on line 103   Fatal error: Call to undefined function groups_create_group() in /home/marina/repositories/behat_master/moodle/enrol/meta/tests/plugin_test.php on line 103
            Hide
            Marina Glancy added a comment -

            ok, so I made attempt to fix that. And started fixing unittests.

            This is the branch:
            https://github.com/moodle/moodle/compare/master...marinaglancy:wip-MDL-17929-master
            in line 209 of the test I decided to check that if I unenrol user from course1 (which is synced with group1), user still remains member of group2 (because he is still enrolled in course2, synced with this group). But this check fails. So there is a mistake in the main function.

            Also because number of enrolments was changed in the beginning of the function, all number-of-enrolments assertions need to be changed throughout the test. This is also not good. Groups test should be separate.

            Show
            Marina Glancy added a comment - ok, so I made attempt to fix that. And started fixing unittests. This is the branch: https://github.com/moodle/moodle/compare/master...marinaglancy:wip-MDL-17929-master in line 209 of the test I decided to check that if I unenrol user from course1 (which is synced with group1), user still remains member of group2 (because he is still enrolled in course2, synced with this group). But this check fails. So there is a mistake in the main function. Also because number of enrolments was changed in the beginning of the function, all number-of-enrolments assertions need to be changed throughout the test. This is also not good. Groups test should be separate.
            Hide
            Marina Glancy added a comment -

            Couple more points:

            1. backup/restore and group mapping is not implemented
            2. There is not "edit" cog next to the meta enrolment, it was not needed before (since there was nothing to edit) but now it should be implemented
            Show
            Marina Glancy added a comment - Couple more points: backup/restore and group mapping is not implemented There is not "edit" cog next to the meta enrolment, it was not needed before (since there was nothing to edit) but now it should be implemented
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/marinaglancy/moodle master (99 errors / 8 warnings) [branch: wip-MDL-17929-master | CI Job ] phplint (0/0) , php (97/8) , js (0/0) , css (0/0) , phpdoc (2/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/marinaglancy/moodle master (3 errors / 1 warnings) [branch: wip-MDL-17929-master | CI Job ] phplint (0/0) , php (2/1) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Damyon Wiese added a comment -

            Hi Marina, nice job - I'm glad it includes lots of tests.

            The only thing I don't like the look of in this patch is the repeated complex groups DB queries outside of grouplib.

            We now have this:

            // Remove invalid.
            +    $sql = "SELECT gm.*, e.courseid, g.name AS groupname
            +              FROM {groups_members} gm
            +              JOIN {groups} g ON (g.id = gm.groupid)
            +              JOIN {enrol} e ON (e.enrol = 'meta' AND e.courseid = g.courseid $onecourse)
            +              JOIN {user_enrolments} ue ON (ue.userid = gm.userid AND ue.enrolid = e.id)
            +             WHERE gm.component='enrol_meta' AND gm.itemid = e.id AND g.id <> e.customint2";
            +    $params = array();
            +    $params['courseid'] = $courseid;
            +
            +    $rs = $DB->get_recordset_sql($sql, $params);
            +    foreach ($rs as $gm) {
            +        groups_remove_member($gm->groupid, $gm->userid);
            +        if ($verbose) {
            +            mtrace("removing user from group: $gm->userid ==> $gm->courseid - $gm->groupname", 1);
            +        }
            +    }
            +    $rs->close();
            +
            +    // Add missing.
            +    $sql = "SELECT ue.*, g.id AS groupid, e.courseid, g.name AS groupname
            +              FROM {user_enrolments} ue
            +              JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'meta' $onecourse)
            +              JOIN {groups} g ON (g.courseid = e.courseid AND g.id = e.customint2)
            +              JOIN {user} u ON (u.id = ue.userid AND u.deleted = 0)
            +         LEFT JOIN {groups_members} gm ON (gm.groupid = g.id AND gm.userid = ue.userid)
            +             WHERE gm.id IS NULL";
            +    $params = array();
            +    $params['courseid'] = $courseid;
            
            

            in meta enrolment and this:

               $sql = "SELECT gm.*, e.courseid, g.name AS groupname                                                                            
                          FROM {groups_members} gm                                                                                              
                          JOIN {groups} g ON (g.id = gm.groupid)                                                                                
                          JOIN {enrol} e ON (e.enrol = 'cohort' AND e.courseid = g.courseid $onecourse)                                         
                          JOIN {user_enrolments} ue ON (ue.userid = gm.userid AND ue.enrolid = e.id)                                            
                         WHERE gm.component='enrol_cohort' AND gm.itemid = e.id AND g.id <> e.customint2";                                      
                $params = array();                                                                                                              
                $params['courseid'] = $courseid;                                                                                                
                                                                                                                                                
                $rs = $DB->get_recordset_sql($sql, $params);                                                                                    
                foreach($rs as $gm) {                                                                                                           
                    groups_remove_member($gm->groupid, $gm->userid);                                                                            
                    $trace->output("removing user from group: $gm->userid ==> $gm->courseid - $gm->groupname", 1);                              
                }                                                                                                                               
                $rs->close();                                                                                                                   
                                                                                                                                                
                // Add missing.                                                                                                                 
                $sql = "SELECT ue.*, g.id AS groupid, e.courseid, g.name AS groupname                                                           
                          FROM {user_enrolments} ue                                                                                             
                          JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'cohort' $onecourse)                                               
                          JOIN {groups} g ON (g.courseid = e.courseid AND g.id = e.customint2)                                                  
                          JOIN {user} u ON (u.id = ue.userid AND u.deleted = 0)                                                                 
                     LEFT JOIN {groups_members} gm ON (gm.groupid = g.id AND gm.userid = ue.userid)                                             
                         WHERE gm.id IS NULL";                                                                                                  
                $params = array();                                                                                                              
                $params['courseid'] = $courseid;                                                                                                
            

            in cohort enrolment which is exactly the same. Can we add a function to grouplib that covers both these uses?

            We also should have some AMOS comments for copying the new strings.

            Show
            Damyon Wiese added a comment - Hi Marina, nice job - I'm glad it includes lots of tests. The only thing I don't like the look of in this patch is the repeated complex groups DB queries outside of grouplib. We now have this: // Remove invalid. + $sql = "SELECT gm.*, e.courseid, g.name AS groupname + FROM {groups_members} gm + JOIN {groups} g ON (g.id = gm.groupid) + JOIN {enrol} e ON (e.enrol = 'meta' AND e.courseid = g.courseid $onecourse) + JOIN {user_enrolments} ue ON (ue.userid = gm.userid AND ue.enrolid = e.id) + WHERE gm.component='enrol_meta' AND gm.itemid = e.id AND g.id <> e.customint2"; + $params = array(); + $params['courseid'] = $courseid; + + $rs = $DB->get_recordset_sql($sql, $params); + foreach ($rs as $gm) { + groups_remove_member($gm->groupid, $gm->userid); + if ($verbose) { + mtrace("removing user from group: $gm->userid ==> $gm->courseid - $gm->groupname", 1); + } + } + $rs->close(); + + // Add missing. + $sql = "SELECT ue.*, g.id AS groupid, e.courseid, g.name AS groupname + FROM {user_enrolments} ue + JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'meta' $onecourse) + JOIN {groups} g ON (g.courseid = e.courseid AND g.id = e.customint2) + JOIN {user} u ON (u.id = ue.userid AND u.deleted = 0) + LEFT JOIN {groups_members} gm ON (gm.groupid = g.id AND gm.userid = ue.userid) + WHERE gm.id IS NULL"; + $params = array(); + $params['courseid'] = $courseid; in meta enrolment and this: $sql = "SELECT gm.*, e.courseid, g.name AS groupname FROM {groups_members} gm JOIN {groups} g ON (g.id = gm.groupid) JOIN {enrol} e ON (e.enrol = 'cohort' AND e.courseid = g.courseid $onecourse) JOIN {user_enrolments} ue ON (ue.userid = gm.userid AND ue.enrolid = e.id) WHERE gm.component='enrol_cohort' AND gm.itemid = e.id AND g.id <> e.customint2"; $params = array(); $params['courseid'] = $courseid; $rs = $DB->get_recordset_sql($sql, $params); foreach($rs as $gm) { groups_remove_member($gm->groupid, $gm->userid); $trace->output("removing user from group: $gm->userid ==> $gm->courseid - $gm->groupname", 1); } $rs->close(); // Add missing. $sql = "SELECT ue.*, g.id AS groupid, e.courseid, g.name AS groupname FROM {user_enrolments} ue JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'cohort' $onecourse) JOIN {groups} g ON (g.courseid = e.courseid AND g.id = e.customint2) JOIN {user} u ON (u.id = ue.userid AND u.deleted = 0) LEFT JOIN {groups_members} gm ON (gm.groupid = g.id AND gm.userid = ue.userid) WHERE gm.id IS NULL"; $params = array(); $params['courseid'] = $courseid; in cohort enrolment which is exactly the same. Can we add a function to grouplib that covers both these uses? We also should have some AMOS comments for copying the new strings.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-17929 using repository: https://github.com/marinaglancy/moodle master (1 errors / 0 warnings) [branch: wip-MDL-17929-master | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Marina Glancy added a comment -

            done.

            Show
            Marina Glancy added a comment - done.
            Hide
            Damyon Wiese added a comment -

            Thanks Marina - looks good to me - sending for integration.

            Show
            Damyon Wiese added a comment - Thanks Marina - looks good to me - sending for integration.
            Hide
            Rajesh Taneja added a comment -

            This seems to be covered well by behat. Just confirming if QA test is needed for this ?

            Show
            Rajesh Taneja added a comment - This seems to be covered well by behat. Just confirming if QA test is needed for this ?
            Hide
            Dan Poltawski added a comment -

            I've added this to integration_held as its a new feature after freeze. Please get Martin Dougiamas to unblock it if we want this to make it for 2.9.

            Show
            Dan Poltawski added a comment - I've added this to integration_held as its a new feature after freeze. Please get Martin Dougiamas to unblock it if we want this to make it for 2.9.
            Hide
            Daniel Neis added a comment -

            Hello,

            i hope that the 114 votes and waiting since 2009 makes Dougiamas think about it with love
            Mainly after the article on opensource.com saying that moodle is community driven by votes in tracker ;P

            thanks all for your hard work!

            kind regards,
            Daniel

            Show
            Daniel Neis added a comment - Hello, i hope that the 114 votes and waiting since 2009 makes Dougiamas think about it with love Mainly after the article on opensource.com saying that moodle is community driven by votes in tracker ;P thanks all for your hard work! kind regards, Daniel
            Hide
            Dan Poltawski added a comment -

            +1 to allow this to 'break freeze'. (Looks pretty self contained and hopefully limited impact on existing tests which have been run, as well as only 'just' missing). A few vote winners are always good for our ego

            Show
            Dan Poltawski added a comment - +1 to allow this to 'break freeze'. (Looks pretty self contained and hopefully limited impact on existing tests which have been run, as well as only 'just' missing). A few vote winners are always good for our ego
            Hide
            David Monllaó added a comment -

            I've already +1 it a few days ago

            Show
            David Monllaó added a comment - I've already +1 it a few days ago
            Hide
            Andrew Nicols added a comment -

            +1 ti this from me too.

            Show
            Andrew Nicols added a comment - +1 ti this from me too.

              Dates

              • Created:
                Updated: