Skip to content

Add optional group filter: only enrol members of selected source-course groups - #48

Open
Danimarqz wants to merge 7 commits into
ewallah:mainfrom
Danimarqz:groupfilter
Open

Add optional group filter: only enrol members of selected source-course groups#48
Danimarqz wants to merge 7 commits into
ewallah:mainfrom
Danimarqz:groupfilter

Conversation

@Danimarqz

Copy link
Copy Markdown

What

Adds an optional per-instance group filter. When one or more groups of the completed (source) course are selected, only students who belong to at least one of them are auto-enrolled into the target course. Leaving it empty keeps the current behaviour (everyone who completes the course is enrolled) — fully backward compatible.

Use case: course B enrols users on completion of course A, but you only want a specific cohort (e.g. an "AGOSTO" group in A) enrolled.

How

  • Stored in the instance customtext2 as a JSON array of group ids ([] = no restriction). No DB schema change.
  • Edit form: an autocomplete multiselect whose options load by AJAX (core_group_get_course_groups) from the course chosen in customint1, via AMD module enrol_coursecompleted/groupselector. Selected groups preloaded server-side.
  • Enforcement in enrol_user() (single choke point → covers immediate and delayed enrolment paths): users not in any selected group are skipped.
  • Full es translation added.
  • Tests: tests/groupfilter_test.php.

Notes

  • version.php bump and v5.3.3-groups1

Daniel Márquez added 4 commits July 12, 2026 21:24
Per-instance group filter: when one or more groups of the completed course are selected, only students who belong to at least one of them are auto-enrolled into the target course. Empty keeps the original behaviour (everyone who completes the course is enrolled).

Stored in the instance customtext2 as a JSON array of group ids. The group list is loaded by AJAX (core_group_get_course_groups) from the course chosen in customint1, via amd module enrol_coursecompleted/groupselector. The filter is enforced in enrol_user(), covering both the immediate and the delayed enrolment paths. Includes tests/groupfilter_test.php.
Translate every plugin string to Spanish, plus the new group-filter strings.
getDataGenerator()->create_group() returns a record; the plugin stores group
ids, so intval() on the object warned and stored [1]. Use the group id.
Replace the hand-written amd/build with the real grunt output and add the
sourcemap, so the CI JS build check passes.
@ewallah

ewallah commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Thank you for your suggestion. I do have a few remarks:

  • The introduction of groups could open a Pandora's box. Some time ago, someone created a version with cohorts. What's next - limiting enrollment based on payment status or the number of discussion forum posts? At the moment, Moodle enrollment plugins only support selecting roles and dates. Perhaps group assignment should instead be implemented at the enrollment plugin level, allowing each enrollment plugin to provide its own group-related functionality where appropriate.
  • Your changes are not reflected on the enrollment page, so students may incorrectly assume they will be enrolled in a future course. Group membership should be calculated on the enrollment page (taking into account whether the user has permission to view groups) and displayed in a way that does not reveal information a teacher or a policy wants to hide.
  • I don't like the idea of storing the group field as plain text. Since a course can have multiple enrollment methods, support for multiple groups could instead be implemented at that level. An additional benefit would be that each group could receive its own welcome message.
  • Language strings should be submitted through AMOS so they can be reviewed and translated using the standard Moodle process, so the full Spanish translation is not needed here.
  • The plugin is already fairly complex, and I receive weekly reports claiming that it is not working as expected. In most cases, the users later realize that their understanding of course completion differs from Moodle's implementation. Any new functionality should therefore be documented clearly to avoid additional confusion.
  • Your changes are not covered by Behat tests. I'm particularly interested in understanding the performance impact, both with and without these changes.

Just collecting ideas...

@Danimarqz

Danimarqz commented Jul 23, 2026

Copy link
Copy Markdown
Author

@ewallah Hi, and thank you so much for taking the time to go through this so carefully — I really appreciate the detailed feedback, it's genuinely helpful. Let me walk through it, because I think a couple of the points assume the change is broader than it actually is, and that's probably my fault for not explaining it well enough up front.

The key thing: this is a filter, not group assignment. It doesn't create groups, assign anyone to a group, or add per-group welcome messages. All it does is add an optional condition to the trigger: when the admin picks one or more groups from the completed (source) course, only members of those groups get auto-enrolled. Leave it empty and nothing changes — everyone is enrolled, exactly as today. So it really lives right next to the role/date settings the plugin already has: a condition on who the existing automation acts on, not a new group-management feature inside the plugin.

With that framing, point by point:

  1. Scope / "Pandora's box". Totally fair worry, and honestly I share the same instinct against features that keep growing conditions. But this one isn't in cohorts/payment/forum-posts territory — it reuses the single relationship the plugin already depends on (the source course) and just narrows it by group. No new data model, no new subsystem.

  2. Not reflected on the enrolment page. You're absolutely right, that was a real gap and a good catch. I've fixed it: the filter check now lives in a single passes_group_filter() method, reused by both enrol_user() and enrol_page_hook(). The page no longer promises enrolment to someone who wouldn't actually qualify. It only ever looks at the viewing user's own membership, so it never exposes hidden-group information about anyone else.

  3. Storing the group as plain text / doing it at enrol-method level. Good point on the storage — it's actually a JSON array of group ids in customtext2 (the instance's own storage) rather than free text, and multiple groups already work. The reason I didn't push it down to a generic per-enrol-method group feature is that here nothing is being assigned — it's purely a filter on the source course, so there's no group to hang a welcome message off. That said, if renaming or better documenting the field would help, I'm very happy to do that.

  4. Language strings via AMOS. Completely agree — I've dropped the full Spanish translation from the PR, so only the English strings remain and can go through AMOS the normal way.

  5. Documentation. Agreed, and especially given how often course completion itself trips people up, I want to be careful here. I've added a section that spells out exactly what the filter does and, just as importantly, what it doesn't do.

  6. Behat + performance. I'm adding Behat coverage for it. On performance: it's a single groups_get_user_groups() call per enrolment attempt (already cached by core), plus one call for the current user on the enrolment page — and it's skipped entirely when no groups are selected, so the default path is untouched. Happy to put together concrete before/after numbers if that's useful.

And genuinely — if after all that you'd still rather keep groups out of this plugin, that's completely fine and I'll just maintain it as a local customisation, no hard feelings at all. But framed as an optional source-course filter, I do think it stays within the spirit of what the plugin already does. Thanks again for engaging with it so thoughtfully.

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

Successfully merging this pull request may close these issues.

2 participants