Auto enrollment removes student upon un-enroll from course. Other membership's auto enroll should still grant access

[willmiddleton-lifterlms]

Reproduction Steps

1. Create 2 Courses (Course A, Course B)
2. Create 2 Memberships:
   • Membership A -> Auto Enroll to Course A
   • Membership B -> Auto Enroll to Course A & Course B
3. Create free access plans on your memberships.
4. Sign up as a test user into Membership A
5. Sign up as the same test user to Membership B
6. (at this point, you should have access to: Membership A and Membership B. Also because of auto-enrollment, access to both courses)
7. As the website admin, cancel the test user's order, removing them from Membership A
8. Check the test user's account. They'll only have access to Membership B and Course B (they should still have access to Course A, but they don't)

Video Reproduction: https://www.loom.com/share/cf5bd92b3ba84e179447de060169910e

Expected Behavior

Users would have access to all courses under the auto-enroll settings for any memberships they're currently enrolled in.

Actual Behavior

If there are many memberships with overlapping auto-enrollment, and a membership is canceled, it will remove you from the auto-enroll courses under that membership (even if you should still have access under a different membership).

Error Messages / Logs

System and Environment Information

System Report

``` Wordpress ------------------------------------------- Home Url: [removed] Site Url: [removed] Login Url: [removed]/wp-login.php Version: 5.8.2 Debug Mode: No Debug Log: No Debug Display: Yes Locale: en_US Multisite: No Page For Posts: Not Set Page On Front: Home (#145) [[removed]/] Permalink Structure: /%postname%/ Show On Front: page Wp Cron: Yes Settings ------------------------------------------- Version: 5.5.0 Db Version: 5.5.0 Course Catalog: Course Catalog (#14) [[removed]/courses/] Membership Catalog: Membership Catalog (#15) [[removed]/memberships/] Student Dashboard: Dashboard (#17) [[removed]/dashboard/] Checkout Page: Enrollment (#16) [[removed]/enrollment/] Course Catalog Per Page: 9 Course Catalog Sorting: menu_order,ASC Membership Catalog Per Page: 9 Membership Catalog Sorting: menu_order Site Membership: Not Set Courses Endpoint: my-courses Edit Endpoint: edit-account Lost Password Endpoint: lost-password Vouchers Endpoint: redeem-voucher Autogenerate Username: no Password Strength Meter: no Minimum Password Strength: Terms Required: no Terms Page: Not Set Checkout Names: Checkout Address: Checkout Phone: Checkout Email Confirmation: no Open Registration: yes Registration Names: Registration Address: Registration Phone: Registration Voucher: Registration Email Confirmation: no Account Names: Account Address: Account Phone: Account Email Confirmation: no Confirmation Endpoint: confirm-payment Force Ssl Checkout: no Country: US Currency: USD Currency Position: left Thousand Separator: , Decimal Separator: . Decimals: 2 Trim Zero Decimals: no Recurring Payments: yes Email From Address: [removed] Email From Name: [removed] Email Footer Text: Email Header Image: Cert Bg Width: 800 Cert Bg Height: 616 Cert Legacy Compat: no Constants ------------------------------------------- LLMS_REMOVE_ALL_DATA: undefined LLMS_REST_DISABLE: undefined LLMS_SITE_FEATURE_RECURRING_PAYMENTS: undefined LLMS_SITE_IS_CLONE: undefined Gateways ------------------------------------------- Manual: Disabled Manual Logging: Manual Order: 1 Server ------------------------------------------- Mysql Version: 5.7.36 Php Curl: Yes Php Default Timezone: UTC Php Fsockopen: Yes Php Max Input Vars: 5000 Php Max Upload Size: 512 MB Php Memory Limit: 256M Php Post Max Size: 1024M Php Soap: Yes Php Suhosin: No Php Time Limt: 30 Php Version: 7.3.32 Software: Apache/2.4.51 (Unix) OpenSSL/1.1.1 Wp Memory Limit: 256M Browser ------------------------------------------- HTTP USER AGENT: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.69 Safari/537.36 Theme ------------------------------------------- Name: Kadence Version: 1.1.8 Themeuri: https://www.kadence-theme.com/ Authoruri: https://www.kadencewp.com/ Template: Child Theme: No Llms Support: No Plugins ------------------------------------------- LifterLMS: 5.5.0 Integrations ------------------------------------------- BbPress: No BuddyPress: No Template Overrides ------------------------------------------- course/lesson-preview.php (ver: 4.4.0): /srv/users/manager/apps/will-custom-fields/public/wp-content/themes/kadence/lifterlms/ (ver: 4.4.0) course/syllabus.php (ver: 4.4.0): /srv/users/manager/apps/will-custom-fields/public/wp-content/themes/kadence/lifterlms/ (ver: 4.4.0) ```

This issue has be recreated:

• [ ] Locally
• [x] On a staging site
• [x] On a production website
• [x] With only LifterLMS and a default theme

No HelpScout ticket, this is something Chris let me know about that he heard in Facebook from a customer.

[chrisbadgett]

Ideally a user should not get enrollment status cancelled from a course via a related membership cancellation if they also have an active enrolled status in the course via another membership.

[thomasplevy]

I haven't setup a reproduction but I don't think I need to. When someone is enrolled into a course through a membership the enrollment trigger is stored as that membership. When they're unenrolled from the membership we remove the user from every course with that trigger.

A check should be added to the unenrollment handler that should lookup if the user should have access to that course from somewhere else and skip the unenrollment if they should / could.

@willmiddleton-lifterlms there's no associated support ticket so is this something you've noticed that you're bringing to our attention or do we have an upset user? I'm trying to gauge priority since currently all the devs are wrapped up in other projects and we'll need to halt something else to work on a bug fix if this is a priority issue.

Thanks

[willmiddleton-lifterlms]

@thomasplevy - I don't think we have any really upset customers at the moment on this. The customer who reported it is pretty mellow and I think she has a workaround for now.

[pondermatic]

I have a customer that has been affected by this issue.

Both unenrolling and deleting the enrollment can trip this issue.

@thomasplevy wrote:

A check should be added to the unenrollment handler that should lookup if the user should have access to that course from somewhere else and skip the unenrollment if they should / could.

How can it be determined if "the user should have access to that course from somewhere else"? Not all enrollment triggers are stored. All possible triggering sources will need to be checked. I propose that we add a filter hook that allows all enrollment triggering types to do their own checks? If a filter callback thinks the enrollment should not change from 'enrolled', then it can say "You shall not pass (change the enrollment status)".

When LLMS_Student:enroll() is called and the user already has an enrollment for the product:

• If the current status is enrolled, the enrollment is not updated.
• Else, a new status and trigger are added to the enrollment.

@gocodebox/engineering, I found these types of enrollment triggers. Am I missing any?

Trigger	Enrolled By
admin_{$current_user_id}	LLMS_Student_Bulk_Enroll::enroll_users_in_product(), LLMS_AJAX_Handler::bulk_enroll_students() , LLMS_AJAX_Handler::update_student_enrollment(), LLMS_REST_Enrollments_Controller::handle_status_update()
membership_{$membership_id}	LLMS_Student::add_membership_level(), LLMS_Processor_Membership_Bulk_Enroll::dispatch_enrollment()
order{$order_id}	LLMS_Meta_Box_Order_Enrollment::save_update_enrollment(), LLMS_Controller_Orders()::complete_order()
voucher	LLMS_Voucher::use_voucher()
group_{$group_id}	LLMS_Groups_Enrollment::do_post_enrollment()
invitation	LLMS_Groups_Invitation_Accept::accept_invitation()
primary_admin	llms_create_group() in LifterLMS Groups
wc_order_{$order_id}	LLMS_WC_Order_Actions::do_order_enrollments(), LLMS_WC_Admin_Order_Item_Meta::update_enrollment_status()

For memberships with auto-enroll courses, the course may have been added to the list after the user was enrolled in the membership. The _llms_auto_enroll post meta array doesn't have a date of when the course was added to the array, so we can't compare to the membership enrollment date. @gocodebox/engineering, should we assume that the user should have been auto-enrolled?

For orders, we will have to search for completed user orders related to courses or related to courses auto-enrolled by memberships. Then check the order status and membership enrollment status.

There are many examples of LLMS_Student::unenroll() and llms_unenroll_student() that use any trigger. Currently, 'any' means change the enrollment no matter what triggered it. @gocodebox/engineering, maybe some of these calls should use a specific trigger? Or should we allow 'any' trigger to be overridden by the new filter?

[pondermatic]

This is what I had in mind for the filter.

/**
 * Determines if the change to an enrollment status, from 'enrolled', is allowed by other triggers.
 *
 * @since [version]
 *
 * @param int    $product_id         WordPress post ID of an enrollable post type, such as course or membership.
 * @param string $new_status         Can be "canceled", "expired", or "deleted".
 * @param string $enrollment_trigger The enrollment trigger for the student's enrollment in a product.
 * @param int    $parent_id          WordPress post ID of the parent that is being deleted.
 * @return bool
 */
private function is_enrollment_change_allowed( $product_id, $new_status, $enrollment_trigger, $parent_id = null ) {

    /**
     * This filter allows things like memberships and orders the ability to prevent the enrollment change if there
     * are un-recorded enrollments.
     *
     * When an additional enrollment is triggered and the current enrollment status is 'enrolled',
     * then the additional enrollment is ignored and no new data is added to the enrollment. If the enrollment is
     * changed with the first trigger, the second un-recorded enrollment trigger is effectively changed as well.
     *
     * @since [version]
     *
     * @param bool         $is_change_allowed  Should the enrollment be changed?
     * @param int          $product_id         WordPress post ID of an enrollable post type, such as course or membership.
     * @param string       $new_status         Can be "canceled", "expired", or "deleted".
     * @param string       $enrollment_trigger The enrollment trigger for the student's enrollment in a product.
     * @param int|null     $parent_id          WordPress post ID of the parent that is being deleted.
     * @param LLMS_Student $student            This student object.
     */
    $is_allowed = apply_filters(
        'llms_enrollment_change_allowed',
        true,
        $product_id,
        $new_status,
        $enrollment_trigger,
        $parent_id,
        $this
    );

    return $is_allowed;
}

[thomasplevy]

I found these types of enrollment triggers. Am I missing any?

I have never taken the time to create an exhaustive list of triggers. Without looking through each codebase line by line and trying to develop the same list you've done I simply do not know.

It looks exhaustive to me but I really don't know.

We'll have to call it good enough for today and if we've missed something we can add later.

[thomasplevy]

How can it be determined if "the user should have access to that course from somewhere else"? Not all enrollment triggers are stored. All possible triggering sources will need to be checked. I propose that we add a filter hook that allows all enrollment triggering types to do their own checks? If a filter callback thinks the enrollment should not change from 'enrolled', then it can say "You shall not pass (change the enrollment status)".

Seems like a good solution to me...

Really we should start storing every single trigger so that we can better track the various entry points into things. What we have today is the result of adding functionality over time without considering how these new entry points could be "undone".

The system we have (storing one trigger) "worked" in LifterLMS 1.x but the software is far more complex today and we need to adjust accordingly... obviously.

[thomasplevy]

For memberships with auto-enroll courses, the course may have been added to the list after the user was enrolled in the membership. The _llms_auto_enroll post meta array doesn't have a date of when the course was added to the array, so we can't compare to the membership enrollment date. https://github.com/orgs/gocodebox/teams/engineering, should we assume that the user should have been auto-enrolled?

One thing to note is that autoenroll isn't the only way to connect a membership with a course...

The whole autoenroll system, I think, is vastly abused. And I don't want to get into that today but we also have to denote relationships through members-only access plans.

So if a user gains access to a course through a members-only access plan they should be removed from the course when enrollment in the membership lapses based on that.

We added a concept of membership "post" associations a while back, I think for usage in Groups. This is something to be aware of with regards to enrollments: https://github.com/gocodebox/lifterlms/blob/604c945a603e53c832dc14d990595bf0cb7870c9/includes/models/model.llms.membership.php#L109-L124

[thomasplevy]

There are many examples of LLMS_Student::unenroll() and llms_unenroll_student() that use any trigger. Currently, 'any' means change the enrollment no matter what triggered it. https://github.com/orgs/gocodebox/teams/engineering, maybe some of these calls should use a specific trigger? Or should we allow 'any' trigger to be overridden by the new filter?

It's hard to give a blanket answer to this question...

In certain areas any should provide the ability to override every other trigger. If an admin wants to explicitly cancel someones enrollment, for example, they should be able to do so. However I don't think we have any place in the UI currently to enable that to happen. They have to go through other channels to handle it.

So I think we need to review the use of 'any' on a case by case base and determine if we're using any for a good reason or if we're being lazy.

[pondermatic]

@thomasplevy wrote:

Really we should start storing every single trigger so that we can better track the various entry points into things. What we have today is the result of adding functionality over time without considering how these new entry points could be "undone".

That would definitely make solving this issue more straightforward. I wasn't sure if there was a historical reason for this line. If you like, I can see if there's a downside to storing all enrollments?

[thomasplevy]

My guess -- I can't remember anything for certain related to this -- is that storing duplicates just seemed unnecessary at some point. Like, why would a user need to be enrolled in the same course twice? That's just adding unnecessary information to a table (which would eventually cause performance issues).

---

If the alternative to preventing hypothetical performance issues is preventing bugs (like this one) by storing multiple enrollments in favor of a single enrollment then I think we'll need to just check on our queries that pull enrollment status data and figure out how to rework them to work correctly given the introduction of multiple enrollment records for each.

[pondermatic]

@thomasplevy wrote:

We added a concept of membership "post" associations a while back, I think for usage in Groups. This is something to be aware of with regards to enrollments.

That's a cool feature. I checked it out and it looks like the association is done through the user's _llms_restricted_levels meta and not enrollments.

[nKittie]

Dear Sir/Madam, I would like to highlight that we (a paid customer with multiple licenses) are experiencing this issue, and its causing extra admin work and a bad customer experience.... We would really appreciate it if you would resolve this bug. Otherwise, very happy with the product. Many thanks!

[brianhogg]

Similar issue reported in a wp.org thread here via calling llms_unenroll_student for a membership with overlapping courses:

https://wordpress.org/support/topic/llms_unenroll_student-unexpected-logic/

This is perhaps exactly the same root cause if cancelling an order for a membership ultimately calls llms_unenroll_student

[brianhogg]

When unenrolling from a membership it ultimately hits LLMS_Student::unenroll() which then calls LLMS_Student::remove_membership_level().

This method finds all the course enrollments triggered by the membership and calls LLMS_Student::unenroll() again or LLMS_Student::delete_enrollment() but with the course ID instead of the membership ID. I think we need to check if they would still have access via another membership before we call either unenroll or delete_enrollment for the course. We could perhaps check that it's a membership_... trigger when deciding whether to unenroll them from the course.

As a note, if the user is already enrolled in the course by some other means when they're enrolled in the membership (ie. enrolled in the course directly or via another membership) there's no _enrollment_trigger key with value membership_<id> lifterlms user postmeta added. This means when they're unenrolled from the membership it doesn't try to unenroll them in the course they were already in. This probably makes sense since if the auto-enroll courses changes on a Membership we would likely still want them to be unenrolled from the courses they got access to before. But the courses they'd retain access to is based on the current auto-enroll course listing. A little confusing :)

[brianhogg]

A general note that in this scenario, the student wouldn't be unenrolled from a course currently:

1. Student is already enrolled in course A
2. Student is enrolled in membership which auto-enrolls in course A
3. Student is unenrolled from the membership

Because they were already enrolled in course A, no additional enrollment trigger was added for the membership when they were enrolled in the membership. So when they are unenrolled it's not in the list of courses to unenroll from.

[brianhogg]

The PR is now adding a new enrollment trigger referencing the membership that's "keeping" them in the course. This seems like a better approach than keeping the enrollment trigger to the previous membership when they've been unenrolled from it, and there's still the previous enrollment trigger reference.