fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place. #2234
Conversation
pwnage101
force-pushed
the
pwnage101/ENT-9213-2
branch
4 times, most recently
from
95248e6
to
4b57f58
Compare
enterprise/models.py
Outdated
| def fulfillment(self): | ||
| """ | ||
| Find and return the related EnterpriseFulfillmentSource subclass, or None. | ||
| """ | ||
| return self.license or self.learner_credit_fulfillment |
There was a problem hiding this comment.
[curious] Likely edge casey, but is it possible for the same EnterpriseCourseEnrollment to be re-used across multiple fulfillment sources, e.g. both a license and learner credit? For example, if the learner has enrolled previously with subscriptions, unenrolled, and then re-enrolled with learner credit instead (e.g., the customer switched subsidy types), and then unenrolled again. Would we want this to prioritize only revoking the self.license in this case, or would we expect to revoke both fulfillment sources?
There was a problem hiding this comment.
Great querstion! Since EnterpriseFulfillmentSource.enterprise_course_enrollment is a 1-1 field, its not possible for two different fulfillment source records to point at the same ECE record (OneToOneFields are just unique FKs).
There was a problem hiding this comment.
I do think adam might be right because the subclasses of EnterpriseFulfillmentSource become separate physical tables. While the relationship between an ECE <-> LC fulfillment is 1:1, and ECE <-> Licensed fulfillment is also 1:1, that doesn't technically stop an ECE from being related to an LC and Licensed fulfillment simultaneously. I think we rely solely on business logic to protect against this case.
pwnage101
force-pushed
the
pwnage101/ENT-9213-2
branch
from
4b57f58
to
5e02896
Compare
…lace. I had originally tried emitting this event from the `/cancel_enrollment` API endpoint, but in reality the LMS and Enterprise dashboards were calling the `/change_enrollment` endpoint. The former is called by enterprise-subsidy on transaction reversal, i.e. NOT learner-initiated. The latter is learner-initiated, and that's the original goal of this work. Changes in this commit: * Update the event emission to live closer to the fulfillment model itself (inside `revoke()`). * Ensure that when an EnterpriseCourseEnrollment is unenrolled, the related Fulfillment subclass is revoked. This is a best-effort to improve internal consistency during learner-initiated unenrollment. * Consumers of the various enrollment models are now expected to call helper functions to unenroll/reenroll/revoke/reactivate rather than directly set internal fields. ENT-9213
pwnage101
force-pushed
the
pwnage101/ENT-9213-2
branch
from
77e9d35
to
9dc7502
Compare
I had originally tried emitting this event from the
/cancel_enrollmentAPI endpoint, but in reality the LMS and Enterprise dashboards were
calling the
/change_enrollmentendpoint. The former is called byenterprise-subsidy on transaction reversal, i.e. NOT learner-initiated.
The latter is learner-initiated, and that's the original goal of this
work.
Changes in this commit:
model itself (inside
revoke()).related Fulfillment subclass is revoked. This is a best-effort to
improve internal consistency during learner-initiated unenrollment.
helper functions to unenroll/reenroll/revoke/reactivate rather than
directly set internal fields.
ENT-9213