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

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

    Details

    • Type: Improvement
    • Status: Integration review in progress
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.9.3, 2.0.8, 2.1, 2.2, 2.3.2, 2.4, 2.5, 2.6, 2.7
    • Fix Version/s: None
    • Component/s: Authentication, Groups
    • Environment:
      group/autogroup.php?
    • 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

      Note: during the following disucssion the scope has been changed to:

      Allow to automatically sync (add and remove) users from the child course to the group in the meta course as part of Meta enrolment method

      Original 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

            clarkshahnelson Clark Shah-Nelson created issue -
            Hide
            danleighton Dan Leighton added a comment -

            Definitely!

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

            Show
            danleighton Dan Leighton added a comment - Definitely! This would save me many hours of administration. Please implement it as soon as you can.
            skodak Petr Skoda made changes -
            Field Original Value New Value
            Link This issue has been marked as being related by MDL-17949 [ MDL-17949 ]
            Hide
            ds0101 David Smith added a comment -

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

            Show
            ds0101 David Smith added a comment - I agree, this is a modification we would find very, very useful.
            Hide
            trobb 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
            trobb 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.
            dougiamas Martin Dougiamas made changes -
            Workflow jira [ 30292 ] MDL Workflow [ 44364 ]
            skodak Petr Skoda made changes -
            Assignee Petr Škoda (skodak) [ skodak ] moodle.com [ moodle.com ]
            Fix Version/s DEV backlog [ 10464 ]
            dougiamas Martin Dougiamas made changes -
            Workflow MDL Workflow [ 44364 ] MDL Full Workflow [ 72742 ]
            Hide
            joemurphy 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
            joemurphy 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
            kiddlisa 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
            kiddlisa 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.
            cfulton Charles Fulton made changes -
            Link This issue is duplicated by MDL-17828 [ MDL-17828 ]
            Hide
            leblangi 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
            leblangi 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
            bobpuffer 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
            bobpuffer 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.
            gaudreaj Jean-Philippe Gaudreau made changes -
            Link This issue has been marked as being related by MDL-32091 [ MDL-32091 ]
            Hide
            gaudreaj 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
            gaudreaj 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
            bobpuffer 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
            bobpuffer 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
            gaudreaj 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
            gaudreaj 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!
            gaudreaj Jean-Philippe Gaudreau made changes -
            Affects Version/s 2.2 [ 10656 ]
            Affects Version/s 2.1 [ 10370 ]
            Affects Version/s 2.0.8 [ 11554 ]
            trobb Thomas Robb made changes -
            Link This issue blocks MDL-32161 [ MDL-32161 ]
            Hide
            trobb Thomas Robb added a comment -

            Discussion of cohorts vs metacourses

            Show
            trobb Thomas Robb added a comment - Discussion of cohorts vs metacourses
            trobb Thomas Robb made changes -
            Link This issue blocks MDL-32161 [ MDL-32161 ]
            Hide
            gogosh 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
            gogosh 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
            dra 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
            dra 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.
            mspall Michael Spall made changes -
            Affects Version/s 2.3.2 [ 12353 ]
            Affects Version/s 2.4 [ 12255 ]
            Hide
            boa Matthew Cheng added a comment -

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

            Show
            boa Matthew Cheng added a comment - Has there been any update on a patch that implements this?
            Hide
            bobpuffer 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
            bobpuffer 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
            mebenson 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
            mebenson 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
            ncharette 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
            ncharette 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
            bobpuffer 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
            bobpuffer 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
            trobb 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
            trobb 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
            mebenson 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
            mebenson 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
            bobpuffer 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
            bobpuffer 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
            trobb Thomas Robb added a comment -

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

            Show
            trobb Thomas Robb added a comment - Just place the dearchived file in the blocks directory and go to site admin/notifications.
            trobb Thomas Robb made changes -
            Attachment groupreg.tar [ 34070 ]
            Hide
            trobb 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
            trobb 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
            joshuarbholden 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
            joshuarbholden 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
            trobb 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
            trobb 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
            fuhrmanator 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
            fuhrmanator 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
            nadavkav Nadav Kavalerchik added a comment -

            Adding Moodle 2.5 since it is still relevant to Moodle 2.5

            Show
            nadavkav Nadav Kavalerchik added a comment - Adding Moodle 2.5 since it is still relevant to Moodle 2.5
            nadavkav Nadav Kavalerchik made changes -
            Affects Version/s 2.5 [ 12452 ]
            Hide
            pholden 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
            pholden 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
            pholden 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
            pholden 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
            leblangi Gilles-Philippe Leblanc made changes -
            Affects Version/s 2.6 [ 12579 ]
            Affects Version/s 2.7 [ 12850 ]
            Hide
            chuang 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
            chuang 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
            bobpuffer 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
            bobpuffer 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
            pholden 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
            pholden 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
            willylee 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
            willylee 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
            markpearson 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
            markpearson 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
            willylee 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
            willylee 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
            willylee 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
            willylee 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 Marina Glancy added a comment -

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

            Show
            marina Marina Glancy added a comment - Hi, I've just created MDL-48346 as the suggested solution - automatically synchronise groups with enrolment methods.
            marina Marina Glancy made changes -
            Link This issue has a non-specific relationship to MDL-48346 [ MDL-48346 ]
            marina Marina Glancy made changes -
            Status Open [ 1 ] Open [ 1 ]
            Labels triaged
            Assignee moodle.com [ moodle.com ]
            Component/s Authentication [ 10067 ]
            Sprint candidate Soon [ 10241 ]
            danielneis Daniel Neis made changes -
            Assignee Daniel Neis [ danielneis ]
            Hide
            danielneis 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
            danielneis 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
            danielneis Daniel Neis made changes -
            Pull Master Diff URL https://github.com/moodle/moodle/compare/master...danielneis:MDL-17929
            Pull Master Branch MDL-17929
            Testing Instructions * 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 "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.
            Priority Minor [ 4 ] Major [ 3 ]
            Pull from Repository https://github.com/danielneis/moodle
            danielneis Daniel Neis made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            danielneis Daniel Neis made changes -
            Testing Instructions * 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 "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.
            * 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.
            cibot CiBoT made changes -
            Labels triaged ci triaged
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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
            kwiliarty 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
            kwiliarty 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.
            danielneis Daniel Neis made changes -
            Labels ci triaged ci cime triaged
            cibot CiBoT made changes -
            Labels ci cime triaged ci triaged
            Hide
            cibot 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 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 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 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
            marina Marina Glancy made changes -
            Labels ci triaged ci docs_required triaged ui_change
            damyon Damyon Wiese made changes -
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer Damyon Wiese [ damyon ]
            Hide
            damyon 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 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 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 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!
            damyon Damyon Wiese made changes -
            Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Original Estimate 0 minutes [ 0 ]
            Integration priority 0
            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!
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Currently in integration Yes [ 10041 ]
            cibot CiBoT made changes -
            Labels ci docs_required triaged ui_change docs_required triaged ui_change
            cibot CiBoT made changes -
            Labels docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot 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 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
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Labels ci docs_required triaged ui_change docs_required triaged ui_change
            cibot CiBoT made changes -
            Labels docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot 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 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
            dmonllao David Monllaó made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator David Monllaó [ davmon ]
            Hide
            dmonllao 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
            dmonllao 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
            dmonllao David Monllaó made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            cibot CiBoT made changes -
            Labels ci docs_required triaged ui_change docs_required triaged ui_change
            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.
            cibot CiBoT made changes -
            Status Reopened [ 4 ] Reopened [ 4 ]
            Fix Version/s DEV backlog [ 10464 ]
            Currently in integration Yes [ 10041 ]
            Hide
            danielneis 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
            danielneis 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
            danielneis Daniel Neis made changes -
            Status Reopened [ 4 ] Waiting for peer review [ 10012 ]
            cibot CiBoT made changes -
            Labels docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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
            danielneis Daniel Neis made changes -
            Labels ci docs_required triaged ui_change ci cime docs_required triaged ui_change
            Hide
            danielneis 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
            danielneis 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
            cibot CiBoT made changes -
            Labels ci cime docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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
            dmonllao 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
            dmonllao 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 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 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 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 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 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 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 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 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
            marina Marina Glancy made changes -
            marina Marina Glancy made changes -
            Labels ci docs_required triaged ui_change ci cime docs_required triaged ui_change
            cibot CiBoT made changes -
            Labels ci cime docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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
            marina Marina Glancy made changes -
            Testing Instructions * 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.
            * 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 :)
            marina Marina Glancy made changes -
            Labels ci docs_required triaged ui_change ci cime docs_required triaged ui_change
            cibot CiBoT made changes -
            Labels ci cime docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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
            damyon Damyon Wiese made changes -
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            damyon 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 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.
            marina Marina Glancy made changes -
            Labels ci docs_required triaged ui_change ci cime docs_required triaged ui_change
            cibot CiBoT made changes -
            Labels ci cime docs_required triaged ui_change ci docs_required triaged ui_change
            Hide
            cibot CiBoT added a comment -
            Show
            cibot 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 Marina Glancy added a comment -

            done.

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

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

            Show
            damyon Damyon Wiese added a comment - Thanks Marina - looks good to me - sending for integration.
            damyon Damyon Wiese made changes -
            Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - This seems to be covered well by behat. Just confirming if QA test is needed for this ?
            Hide
            poltawski 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
            poltawski 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.
            poltawski Dan Poltawski made changes -
            Labels ci docs_required triaged ui_change ci docs_required integration_held triaged ui_change
            Hide
            danielneis 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
            danielneis 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
            poltawski 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
            poltawski 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
            dmonllao David Monllaó added a comment -

            I've already +1 it a few days ago

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

            +1 ti this from me too.

            Show
            dobedobedoh Andrew Nicols added a comment - +1 ti this from me too.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            +1, although:

            1) https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-17929-master#diff-3b07a91899c1565b7a966e5d9d8b2457R590 should not be global, but restricted to the child course. Also there is a misleading "cohort" comment 1 line above.

            2) The summary and description of this issue really made me think this was exactly the opposite (and non-sense) implementation. This is about to create enrollments into child courses populating users into ONE fixed (new or existing) group. Summary seems to indicate the opposite ("from child in meta").

            3) Disclaimer, I've not looked deeply to code. Just to unit tests in order to understand the expected behavior.

            4) Just realized: Unit tests should cover the case of other users (say manually enrolled in child course) manually belonging to those groups in use by the meta enrollment. Those memberships should not be modified ever by the meta_sync(). Note I think code does it properly, leaving users not being meta-enrolled apart, but tests should confirm that expected behavior.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited +1, although: 1) https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-17929-master#diff-3b07a91899c1565b7a966e5d9d8b2457R590 should not be global, but restricted to the child course. Also there is a misleading "cohort" comment 1 line above. 2) The summary and description of this issue really made me think this was exactly the opposite (and non-sense) implementation. This is about to create enrollments into child courses populating users into ONE fixed (new or existing) group. Summary seems to indicate the opposite ("from child in meta"). 3) Disclaimer, I've not looked deeply to code. Just to unit tests in order to understand the expected behavior. 4) Just realized: Unit tests should cover the case of other users (say manually enrolled in child course) manually belonging to those groups in use by the meta enrollment. Those memberships should not be modified ever by the meta_sync(). Note I think code does it properly, leaving users not being meta-enrolled apart, but tests should confirm that expected behavior.
            Hide
            damyon Damyon Wiese added a comment -

            +1 from me too.

            Show
            damyon Damyon Wiese added a comment - +1 from me too.
            marina Marina Glancy made changes -
            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. *Note: during the following disucssion the scope has been changed to:*

            Allow to automatically sync (add and remove) users from the child course to the group in the meta course as part of Meta enrolment method

            *Original 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.
            Hide
            marina Marina Glancy added a comment -

            I changed the line that Eloy pointed out to include courseid in the query, thanks for noticing it. Which means that the same problem is present in the MDL-49380, see commit, I'll create another issue to address it.

            As for #2 from Eloy's comment: yes, the implementation of this issue does not match the description.
            Lots of comments on this issue actually say that people would prefer groups to be created automatically and synced with courses enrolments, rather than one-off populating that was described in the original description. I have created a separate issue MDL-48346 for the sync implementation. However Daniel Neis, who is the author of the initial commit on this issue, has submitted the sync solution under this issue. I just added several commits on top of his to implement even more functionality.
            Technically we could integrate this solution under MDL-48346 but this issue will remain open with 114 votes that will no longer be relevant. I have changed the description.

            I'll try to come up with unit test for #4 soon, I just tested it manually and it works ok

            P.S. removing integration_held

            Show
            marina Marina Glancy added a comment - I changed the line that Eloy pointed out to include courseid in the query, thanks for noticing it. Which means that the same problem is present in the MDL-49380 , see commit , I'll create another issue to address it. As for #2 from Eloy's comment: yes, the implementation of this issue does not match the description. Lots of comments on this issue actually say that people would prefer groups to be created automatically and synced with courses enrolments, rather than one-off populating that was described in the original description. I have created a separate issue MDL-48346 for the sync implementation. However Daniel Neis , who is the author of the initial commit on this issue, has submitted the sync solution under this issue. I just added several commits on top of his to implement even more functionality. Technically we could integrate this solution under MDL-48346 but this issue will remain open with 114 votes that will no longer be relevant. I have changed the description. I'll try to come up with unit test for #4 soon, I just tested it manually and it works ok P.S. removing integration_held
            marina Marina Glancy made changes -
            Labels ci docs_required integration_held triaged ui_change ci docs_required triaged ui_change
            marina Marina Glancy made changes -
            Link This issue Testing discovered MDL-49953 [ MDL-49953 ]
            Hide
            marina Marina Glancy added a comment -

            I created MDL-49953 for the same bug in the enrol_cohort

            Show
            marina Marina Glancy added a comment - I created MDL-49953 for the same bug in the enrol_cohort
            Hide
            marina Marina Glancy added a comment -

            in the comments in existing code we refer to the course where we fetch enrolments from as "parents". In this issue summary and commit messages we call it "child". That's confusing, I agree.

            Myself I would prefer the word "child" for linked courses because one metacourse may have many courses where it synchronises enrolments from, people can't have 10 parents, why meta course would?

            BTW in the docs we also call them "children": https://docs.moodle.org/28/en/Course_meta_link

            I guess the best will be to avoid child/parent words in summary and commit messages and call them "linked" or "source" courses

            Show
            marina Marina Glancy added a comment - in the comments in existing code we refer to the course where we fetch enrolments from as "parents". In this issue summary and commit messages we call it "child". That's confusing, I agree. Myself I would prefer the word "child" for linked courses because one metacourse may have many courses where it synchronises enrolments from, people can't have 10 parents, why meta course would? BTW in the docs we also call them "children": https://docs.moodle.org/28/en/Course_meta_link I guess the best will be to avoid child/parent words in summary and commit messages and call them "linked" or "source" courses
            Hide
            dmonllao David Monllaó added a comment -

            Marina Glancy, do you want us to wait for I'll try to come up with unit test for #4 soon, I just tested it manually and it works ok before reviewing this?

            Show
            dmonllao David Monllaó added a comment - Marina Glancy , do you want us to wait for I'll try to come up with unit test for #4 soon, I just tested it manually and it works ok before reviewing this?
            Hide
            marina Marina Glancy added a comment -

            unittest added

            Show
            marina Marina Glancy added a comment - unittest added
            dmonllao David Monllaó made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Currently in integration Yes [ 10041 ]
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Marina, it looks good (I'm still checking backup/restore before integrating it) Can you please add some testing instructions for enrol_cohort (ensure there are no regressions) and enrol_meta backup/restore?

            Show
            dmonllao David Monllaó added a comment - Thanks Marina, it looks good (I'm still checking backup/restore before integrating it) Can you please add some testing instructions for enrol_cohort (ensure there are no regressions) and enrol_meta backup/restore?
            Hide
            dmonllao David Monllaó added a comment -

            A couple of things I've noticed:

            1. https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R77 This looks intentional, why are we checking managegroups only to create a new group? Shouldn't add group members require managegroups capability too? Shouldn't we check the capability also in https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-3dd8f0e1a69b7169d2210d541dceba32R63?
            2. https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R58 if we have $instance, do we really need to execute the query and fill https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R86?

            Also (unrelated to this issue) I found an unexpected behaviour (undesired IMO) I've noticed that, setting an enrol_meta to a parent course which contains enrol_manual users and this enrol_manual instance is disabled, user_enrolments in the child course are created as suspended. When the parent course enrol_manual's instance is enabled, user_enrolments are not synchronized and they remain suspended, although once I edit any of the child course user_enrolment's (changing any value, not just submitting the form) the user_enrolment is not suspended any more. I will create an issue later.

            Show
            dmonllao David Monllaó added a comment - A couple of things I've noticed: https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R77 This looks intentional, why are we checking managegroups only to create a new group? Shouldn't add group members require managegroups capability too? Shouldn't we check the capability also in https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-3dd8f0e1a69b7169d2210d541dceba32R63? https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R58 if we have $instance, do we really need to execute the query and fill https://github.com/marinaglancy/moodle/compare/master...wip-MDL-17929-master#diff-842b02c6817b091b7d4e521eeb84e266R86? Also (unrelated to this issue) I found an unexpected behaviour (undesired IMO) I've noticed that, setting an enrol_meta to a parent course which contains enrol_manual users and this enrol_manual instance is disabled, user_enrolments in the child course are created as suspended. When the parent course enrol_manual's instance is enabled, user_enrolments are not synchronized and they remain suspended, although once I edit any of the child course user_enrolment's (changing any value, not just submitting the form) the user_enrolment is not suspended any more. I will create an issue later.
            dmonllao David Monllaó made changes -
            Link This issue Testing discovered MDL-49982 [ MDL-49982 ]
            Hide
            dmonllao David Monllaó added a comment -

            Created MDL-49982 (last comment)

            Show
            dmonllao David Monllaó added a comment - Created MDL-49982 (last comment)

              Dates

              • Created:
                Updated: