Skip to content

Batch processing for retroactively enrolling members to courses.#108

Draft
andrewlimaza wants to merge 3 commits into
strangerstudios:devfrom
andrewlimaza:retroactive-enrollment-for-modules
Draft

Batch processing for retroactively enrolling members to courses.#108
andrewlimaza wants to merge 3 commits into
strangerstudios:devfrom
andrewlimaza:retroactive-enrollment-for-modules

Conversation

@andrewlimaza

Copy link
Copy Markdown
Contributor
  • ENHANCEMENT: Retroactively enroll members that may be missing from courses, by existing before a new course is added. This now automatically enrolls members in batches, and requires PMPro 3.5+

This won't re-run batches whenever a member is added to a level programmatically and bypasses any WordPress hooks. This needs a bit more thought, but I am adding this PR for the time being. This is definitely a starting point, and something to think about - a workaround would be to delete post meta "_pmpro_courses_batch_enrollment_levels" for the post types and rerun the scheduled actions (which I think is good enough).

Resolves #72.

How to test the changes in this Pull Request:

  1. Before pulling this PR into your local development environment, make sure you have members.
  2. Create a bunch of courses inside a module that has enrollment feature (LifterLMS, Learndash etc) and require the relevant membership levels. Just do "All".
  3. See that no members are given enrollment to these courses.
  4. Pull this PR into your local development environment.
  5. Manually run the pmpro_schedule_daily action to invoke it OR go to a pre-existing course and save the post. The scheduled action should then asynchronously be added to Action Scheduler to enroll the members to the course.
  6. This logic runs on 2 actions: Once a day and whenever you save a post for the first time (since this PR/Update).

Tested with Learndash, other modules are untested but logic looks correct and to be tested before next release.

* FEATURE: Added support for batch enrollment for course integrations that use "enrollment".

@flintfromthebasement flintfromthebasement left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #108 — Batch processing for retroactively enrolling members to courses.
andrewlimaza → dev | 15 files, +450 -46 lines
#108

Summary
Solid foundation for retroactive batch enrollment via Action Scheduler — the class structure, pagination via offset chaining, and per-course processed-levels tracking are all sound. One correctness issue undermines the stated goal for old courses; a few minor things to clean up before merge. Not blocking, but the major issue should be fixed.


Issues

  • Major includes/batch-enrollment.php:206-218 (get_published_course_ids_for_post_type) — The daily cron is supposed to retroactively enroll members in courses that existed before this plugin version, but the query filters AND p.post_modified >= %s (last 24 hours). This means any course that hasn't been touched in more than a day is silently skipped every single daily run, forever. The PROCESSED_LEVELS_META already prevents re-processing enrolled levels — the time filter is redundant and actively breaks the retroactive goal. Remove the post_modified filter entirely, or query for courses that don't yet have _pmpro_courses_batch_enrollment_levels set and let the metadata be the deduplication gate.

  • Minor includes/modules/learndash.php:228 (retroactive_enroll_user) — ld_update_course_access() does not have a documented return value and returns void in some LearnDash versions. if ( ! $result ) will always be truthy when it returns null/false, logging a spurious error on every successful enrollment. Either drop the return-value check or verify the current LD version actually returns a boolean here.

  • Minor includes/batch-enrollment.php:96-109 (schedule) vs process_batch:163-172 — The first batch task goes through PMPro_Action_Scheduler::instance()->maybe_add_task() (presumably deduplication-aware), but chained batches call as_enqueue_async_action() directly. This inconsistency is probably intentional (chained offsets shouldn't deduplicate), but a comment explaining why the two paths differ would prevent someone from "fixing" it later.

  • Minor includes/batch-enrollment.php:278-280 (init guard) — The class_exists('PMPro_Action_Scheduler') check runs at include-time. Since batch-enrollment.php is require_once'd directly in pmpro-courses.php (before plugins_loaded fires), this guard could fail if PMPro loads after pmpro-courses depending on alphabetical plugin order. Wrapping the init() call in add_action('plugins_loaded', ...) with a late priority would make this robust.


Looks Good

get_active_members() — correct use of ORDER BY user_id before LIMIT/OFFSET ensures consistent pagination across batch runs. array_map('intval', $level_ids) + sort() before building placeholders prevents both injection and unstable query plans.

maybe_schedule_for_course() — the autosave/revision guards and the post_status === 'publish' check are all in the right place. Clean.


Questions

  1. The PR description notes this "won't re-run batches whenever a member is added to a level programmatically and bypasses any WordPress hooks." Is that known gap documented anywhere user-facing, or does it need a notice in the admin or release notes?

  2. tutorlms.php:retroactive_enroll_userdo_enroll( $user_id, 0, $course_id ) matches the pre-existing pattern at line 295, but TutorLMS's documented signature is do_enroll($course_id, $order_id, $user_id). Can you confirm this arg order is correct for the TutorLMS version being targeted? The pre-existing code may have a latent bug here that this PR is replicating.

@dparker1005

Copy link
Copy Markdown
Member

Hey Andrew — going to mark this draft for now while we rethink the approach. The architecture is solid as a starting point, but after looking at how PMPro core handles the same problem for the LifterLMS streamline mode (paid-memberships-pro/includes/compatibility/lifterlms.php, especially pmpro_lifter_repair_course_enrollments and pmpro_lifter_repair_all_course_enrollments_callback), I think we should mirror that pattern here rather than ship this as-is. Three reasons:

  1. Trigger. Core hooks pmpro_after_updating_post_level_restrictions (added in PMPro 3.6), which fires precisely when a post's level restrictions actually change — including from the standard Require Membership meta box save. save_post priority 20 fires on every post save (autosave guards help, but still noisy) and won't fire if restrictions are changed via the REST API or a programmatic call that goes through pmpro_update_post_level_restrictions() without a save_post. The dedicated hook is the right signal.

  2. Idempotency. Core's repair function diffs the user's actual enrolled courses against their current level → course map every time it runs, so it's self-healing — if a save_post event was missed, or post meta got cleared, or anything else drifted, the next run reconciles. This PR uses _pmpro_courses_batch_enrollment_levels post meta as a dedup gate, which means once a level is marked processed, retroactive enrollment for that level will never re-fire on that course — even if it should. Combined with the daily backstop having AND p.post_modified >= last 24 hours, courses that aren't touched daily can fall through both safety nets permanently.

  3. Concurrency / batching. Core queues one AS task per user via PMPro_Action_Scheduler::halt() / resume() while building up the queue, so there's no shared OFFSET cursor and no risk of pagination skip/repeat if users are added or removed mid-run. This PR's chained-offset approach works but is fragile under churn.

The shape I'd suggest mirroring:

  • Drop the save_post hook and the daily cron. Trigger off pmpro_after_updating_post_level_restrictions instead.
  • Replace process_batch (offset-paginated) with a fanout pattern: queue one pmpro_courses_repair_user_enrollment AS task per active member of any level tied to the affected course. Use PMPro_Action_Scheduler::halt()/resume() around the queue-up loop.
  • Drop the _pmpro_courses_batch_enrollment_levels post meta. Each per-user task is idempotent because it checks the LMS-specific is_enrolled API before calling enroll.
  • Optionally: consolidate the per-module retroactive_enroll_user methods into a single repair_user_enrollments($user_id) per module that handles both directions (the way pmpro_lifter_repair_course_enrollments does for LifterLMS). That would also let pmpro_after_all_membership_level_changes call the same repair function instead of duplicating its own enroll/unenroll logic.

One more thing while we're here: tutorlms.php line 252 calls tutor_utils()->do_enroll( $user_id, 0, $course_id ) but Tutor's documented signature is do_enroll( $course_id, $order_id, $user_id ). The pre-existing code at line 295 has the same arg order so it might be a latent bug elsewhere too — worth a follow-up issue regardless of how this PR ends up.

Going to convert this to Draft for now.

@dparker1005 dparker1005 marked this pull request as draft May 13, 2026 16:12
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.

Include ability to add a new course to existing members

3 participants