Prevent unwanted course enrollment changes

[pondermatic]

Description

Prevent course enrollment changes while removing a user's membership level if the user was auto-enrolled in the course by a different membership.

Fixes #1861

How has this been tested?

Manually and with a new unit test.

1. Enroll user in membership 'm1' that auto-enrolls user in course 'c1'.
2. Enroll user in membership 'm2' that auto-enrolls user in courses 'c1' and 'c2'.
3. Expire the user's enrollment in membership 'm1'. The user's enrollment status in both courses is still 'enrolled'.
4. Delete the user's enrollment in membership 'm1'.
5. Expire the user's enrollment in membership 'm2'. The user's enrollment status in both courses is 'expired'.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code has been tested.
• My code passes all existing automated tests.
• My code follows the LifterLMS Coding & Documentation Standards.

[pondermatic]

@gocodebox/engineering, please forgive the large number of changes to LLMS_Student::remove_membership_level(). In addition to fixing the enrollment issue and some minor code cleanup, I renamed many variables in an attempt to make it easier to understand what they contains (at the expense of longer names).

Are there any test scenarios that you would like me to add?

[eri-trabiccolo]

This looks pretty good, and covers pretty well the unwanted unenrollments when removing a membership level, although there's something we're missing here:
config:
Membership M1 -> courses C1, C2

do:
S1 purchases C2's access plan-> enrolled in C2
enroll S1 into M1 -> auto-enrolled in C1 (already enrolled in C2)

cancel (not delete) the C2 order from the order screen -> unenrolled C2, this is to simulate e.g. an access plan expiration.

Although the student has still a valid access to C2 through M1.
We should prevent this, what do you think?

On the other hand a manual unenrollment of a student from a course performed by an admin should bypass this restriction.

This is getting complicated.
I wonder if we shouldn't take this as an occasion to think how to restructure our enrollment recording. Because ideally we should be able to have "multiple triggers", which leads to multiple enrollments at a time, with the valid one being the oldest active one...

[pondermatic]

@eri-trabiccolo wrote:

This looks pretty good, and covers pretty well the unwanted unenrollments when removing a membership level, although there's something we're missing here: config: Membership M1 -> courses C1, C2

do: S1 purchases C2's access plan-> enrolled in C2 enroll S1 into M1 -> auto-enrolled in C1 (already enrolled in C2)

cancel (not delete) the C2 order from the order screen -> unenrolled C2, this is to simulate e.g. an access plan expiration.

Although the student has still a valid access to C2 through M1. We should prevent this, what do you think?

On the other hand a manual unenrollment of a student from a course performed by an admin should bypass this restriction.

This is getting complicated. I wonder if we shouldn't take this as an occasion to think how to restructure our enrollment recording. Because ideally we should be able to have "multiple triggers", which leads to multiple enrollments at a time, with the valid one being the oldest active one...

I think that if there is at least one valid enrollment, then the user should have access. Storing only one of those triggers makes it difficult to determine the overall enrollment status. This PR only handles membership auto-enroll triggers.

I will open up a new issue with our thoughts about redesigning enrollment recording.

[eri-trabiccolo]

@pondermatic wrote:

I think that if there is at least one valid enrollment, then the user should have access.

I agree.

Storing only one of those triggers makes it difficult to determine the overall enrollment status.

Totally agree, in the future we want to overhaul the enrollment's recording.

This PR only handles membership auto-enroll triggers.

Yeah,
can we treat that edge case I highlighted before in this PR?
Basically the idea would be that if the unenrollment to a course comes from an order () we first check if the course is an auto-enroll course of a student's membership's level. Does it make sense?

[pondermatic]

Yeah,
can we treat that edge case I highlighted before in this PR?
Basically the idea would be that if the unenrollment to a course comes from an order () we first check if the course is an auto-enroll course of a student's membership's level. Does it make sense?

My first thought was that when unenrolling a course triggered by an order is handled elsewhere in the code, but I'll have a look at it. At the very least, I'll open an issue.

[eri-trabiccolo]

My first thought was that when unenrolling a course triggered by an order is handled elsewhere in the code, but I'll have a look at it. At the very least, I'll open an issue.

You mean in a different method? Yeah sure.
It's performed here:

lifterlms/includes/controllers/class.llms.controller.orders.php

Line 420 in 3be2130

llms_unenroll_student( $order->get( 'user_id' ), $order->get( 'product_id' ), $status, 'order_' . $order->get( 'id' ) );

(

lifterlms/includes/models/model.llms.student.php

Line 1489 in 3be2130

public function unenroll( $product_id, $trigger = 'any', $new_status = 'expired' ) {

)

[eri-trabiccolo]

so, to summarize:
#2064 (comment)