Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when self-unenrolling from a course causes fill_from_queue() to fail #36

Open
mi-dave opened this issue May 20, 2024 · 1 comment

Comments

@mi-dave
Copy link

mi-dave commented May 20, 2024

When a student self-unenrols from a course, they are deregistered from the groups, but no-one is moved from the waiting list because fill_from_queue() fails.

Steps to reproduce:

  • Create a course with self-enrolment enabled
  • Add a Grouptool instance to it
  • Set "If group members get removed" to "Follow changes"
  • Create a group with only 1 place available and queues enabled
  • As Student 1, enrol in the course and register for the session
  • As Student 2, enrol in the course and join the queue for the session
  • As Student 1, unenrol from the course

If debugdisplay is enabled, it now outputs:

Exception encountered in event observer '\mod_grouptool\observer::group_member_removed': Sorry, but you do not currently have permissions to do that (View active groups).

  • line 2695 of /mod/grouptool/locallib.php: call to require_capability()
  • line 2835 of /mod/grouptool/locallib.php: call to mod_grouptool->get_active_groups()
  • line 182 of /mod/grouptool/classes/observer.php: call to mod_grouptool->fill_from_queue()
  • line ? of unknownfile: call to mod_grouptool\observer::group_member_removed()
  • line 155 of /lib/classes/event/manager.php: call to call_user_func()
  • line 75 of /lib/classes/event/manager.php: call to core\event\manager::process_buffers()
  • line 795 of /lib/classes/event/base.php: call to core\event\manager::dispatch()
  • line 233 of /group/lib.php: call to core\event\base->trigger()
  • line 689 of /group/lib.php: call to groups_remove_member()
  • line 2295 of /lib/enrollib.php: call to groups_delete_group_members()
  • line 51 of /enrol/self/unenrolself.php: call to enrol_plugin->unenrol_user()

This is because mod_grouptool\observer::group_member_removed() calls $instance->fill_from_queue() which calls $this->get_active_groups() which calls require_capability('mod/grouptool:view_groups') - but Student 1 no longer has that permission because they are no longer a student on that course.

As a result, now no-one is registered for that group, but the queue still has one person in it.

I don't think get_active_groups() should be checking permissions at all when called from fill_from_queue(), as the latter should work no matter what capabilities the currently logged in user has (they never see the end result). However, it can't just be removed from that method, because it is called from many places, so maybe it needs another parameter to tell it to skip the capability check - or maybe it should be a separate method, or maybe fill_from_queue() should do its own query directly...

As a manual fix for groups already in this situation, go to Administration > Administrate groups (as a teacher/manager/admin), click the "Resize" (pencil) icon next to the group size, and press Enter to save it without actually changing the group size. This calls resize_group(), which calls fill_from_queue() again, this time as a user that does have permission to run it.

@mi-dave
Copy link
Author

mi-dave commented Jul 16, 2024

After digging into the code some more, I discovered it's not just the one call to $this->get_active_groups() that's an issue - it is also called again further down, and there's also a call to $this->get_user_reg_count() which uses it too.

So I decided to go with a simpler (though more hacky) workaround for now - replacing this line in mod_grouptool\observer::group_member_removed():

                            $instance->fill_from_queue($agrp[$grouptool->id]->id);

With this:

                            $olduser = $GLOBALS['USER'];
                            try {
                                $GLOBALS['USER'] = get_admin();
                                $instance->fill_from_queue($agrp[$grouptool->id]->id);
                            } finally {
                                $GLOBALS['USER'] = $olduser;
                            }

Probably not a good long-term solution, so I won't make it into a PR, but I figured it might help someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant