feat: process expired licenses and unlink enterprise learners #733
Conversation
muhammad-ammar
changed the title
|
|
||
| queryset = License.objects.filter( | ||
| status__in=[ASSIGNED, ACTIVATED], | ||
| renewed_to=None, |
There was a problem hiding this comment.
Note to Reviewers: I am not sure if renewed_to=None should be set or not? I got this from
muhammad-ammar
force-pushed
the
ammar/process-expired-licenses
branch
from
d15a9c8 to
8acde68
Compare
| expired_subscription_plans = list( | ||
| SubscriptionPlan.objects.filter( | ||
| customer_agreement=customer_agreement, | ||
| expiration_date__lt=now, |
There was a problem hiding this comment.
Note to Reviewers: In expire_subscriptions.py we are using expiration_date__lte. Shouldn't we be using expiration_date__lt?
There was a problem hiding this comment.
Hmm, good question. I think from a business perspective, the expiration date configured on subscription plans is exclusive of the active period for the plan. For example, an expiration date of 2025-01-01 means the plan is valid through 2024-12-31 at midnight.
| expired_subscription_plans = list( | ||
| SubscriptionPlan.objects.filter( | ||
| customer_agreement=customer_agreement, | ||
| expiration_date__lt=now, |
There was a problem hiding this comment.
Hmm, good question. I think from a business perspective, the expiration date configured on subscription plans is exclusive of the active period for the plan. For example, an expiration date of 2025-01-01 means the plan is valid through 2024-12-31 at midnight.
| enterprise_customer_uuids = settings.CUSTOMERS_WITH_EXPIRED_LICENSES_UNLINKING_ENABLED | ||
| for enterprise_customer_uuid in enterprise_customer_uuids: | ||
| logger.info('%s Processing started for licenses. Enterprise: [%s]', log_prefix, enterprise_customer_uuid) | ||
| self.process_expired_licenses(enterprise_customer_uuid, log_prefix, unlink) | ||
| logger.info('%s Processing completed for licenses. Enterprise: [%s]', log_prefix, enterprise_customer_uuid) | ||
|
|
||
| logger.info('%s Command completed.', log_prefix) |
There was a problem hiding this comment.
Can we make this logic part of the existing expire_subscriptions management command? If we don't, we might be in a situation where we have two crons running around the same time, where expire_subscriptions tries to downgrade all the course enrollments for licensed learners to audit mode, and this new process_expired_licenses command tries to unlink learners from an enterprise. If the second one executes first, it might cause the first one to fail.
Inside the _expire_subscription_plan() method would be a good place for it.
There was a problem hiding this comment.
- So you are saying, we should move the unlink logic into expire_subscriptions management command, correct?
- Would it be ok if I trigger a new async task right after license_expiration_task? Any chance that unlinking happens before downgrading the enrollments? OR should I do unlinking without an async task?
There was a problem hiding this comment.
The expire_subscriptions command currently calls the license_expiration_task() synchronously (i.e. without .apply_async()). I would also do the unlinking synchronously from the management command, since we're not in the context of a web request and don't really care if this management command takes several minutes to complete. I would generally only invoke async tasks from a management command if there's lots and lots of work to do and the work is inherently parallelizable (for example, when we re-sync data in enterprise-catalog).
There was a problem hiding this comment.
And yes, correct on your first question.
license_manager/settings/base.py
Outdated
| @@ -477,3 +477,5 @@ | |||
| ] | |||
|
|
|||
| CUSTOMERS_WITH_CUSTOM_LICENSE_EVENTS = ['00000000-1111-2222-3333-444444444444'] | |||
| CUSTOMERS_WITH_EXPIRED_LICENSES_UNLINKING_ENABLED = ['76b933cb-bf2a-4c1e-bf44-4e8a58cc37ae'] | |||
There was a problem hiding this comment.
This should be an empty list in base settings.
muhammad-ammar
force-pushed
the
ammar/process-expired-licenses
branch
3 times, most recently
from
e8396ff to
69aabec
Compare
| queryset = License.objects.filter( | ||
| status__in=[ASSIGNED, ACTIVATED], | ||
| renewed_to=None, | ||
| subscription_plan__uuid__in=expired_subscription_plan_uuids, | ||
| ).select_related( |
There was a problem hiding this comment.
Note to Reviewers: Is the above filtering correct? Specifically asking regarding status and renewed_to.
| for expired_subscription_plan in expired_subscription_plans: | ||
| # exclude subscription plan if there is a renewal | ||
| if expired_subscription_plan.get_renewal(): | ||
| continue | ||
|
|
||
| expired_subscription_plan_uuids.append(expired_subscription_plan.uuid) | ||
|
|
||
| # include prior renewals | ||
| for prior_renewal in expired_subscription_plan.prior_renewals: | ||
| expired_subscription_plan_uuids.append(prior_renewal.prior_subscription_plan.uuid) |
There was a problem hiding this comment.
Note to Reviewers: Do we still need this logic? Now that we are only picking plans expired 90 days ago?
There was a problem hiding this comment.
No you shouldn't need this anymore.
| for expired_subscription_plan in expired_subscription_plans: | ||
| # exclude subscription plan if there is a renewal | ||
| if expired_subscription_plan.get_renewal(): | ||
| continue | ||
|
|
||
| expired_subscription_plan_uuids.append(expired_subscription_plan.uuid) | ||
|
|
||
| # include prior renewals | ||
| for prior_renewal in expired_subscription_plan.prior_renewals: | ||
| expired_subscription_plan_uuids.append(prior_renewal.prior_subscription_plan.uuid) |
There was a problem hiding this comment.
No you shouldn't need this anymore.
| queryset = License.objects.filter( | ||
| status__in=[ASSIGNED, ACTIVATED], | ||
| renewed_to=None, | ||
| subscription_plan__uuid__in=expired_subscription_plan_uuids, | ||
| ).select_related( |
| other_active_licenses = License.for_user_and_customer( | ||
| user_email=license.user_email, | ||
| lms_user_id=license.lms_user_id, | ||
| enterprise_customer_uuid=enterprise_customer_uuid, | ||
| active_plans_only=True, | ||
| current_plans_only=True, | ||
| ).exists() | ||
| if other_active_licenses: | ||
| continue |
| enterprise_customer_uuid, | ||
| { | ||
| 'user_emails': user_emails, | ||
| 'is_relinkable': False |
There was a problem hiding this comment.
We had a change of requirements from Joe on this. Decision was made here: https://twou.slack.com/archives/C07UVDDRRUZ/p1730470547546569
is_relinkable should be set to true.
muhammad-ammar
force-pushed
the
ammar/process-expired-licenses
branch
2 times, most recently
from
0cee565 to
786a8fc
Compare
|
@iloveagent57 @macdiesel Thank you for the review. I have addressed your feedback and also added unit tests. I will do some more manual testing and then merge. |
muhammad-ammar
force-pushed
the
ammar/process-expired-licenses
branch
5 times, most recently
from
5c67922 to
b7238bf
Compare
muhammad-ammar
force-pushed
the
ammar/process-expired-licenses
branch
from
b7238bf to
00485c3
Compare
Description: Add a management command to iterate through an enterprise's expired licenses and, for each license, call the LMS to unlink the learner from the enterprise.
JIRA: https://2u-internal.atlassian.net/browse/ENT-9667
Testing considerations
already been performed.
Post-review
Squash commits into discrete sets of changes