-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: create DefaultEnterpriseEnrollmentRealization during bulk enrollment #2276
Conversation
f23d97a
to
a94d8ec
Compare
a94d8ec
to
dbdcb76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, one question about that positive test case.
default_enrollment_intentions_for_customer = models.DefaultEnterpriseEnrollmentIntention.objects.filter( | ||
enterprise_customer=enterprise_customer_uuid, | ||
default_enrollment_intentions_for_customer = ( | ||
models.DefaultEnterpriseEnrollmentIntention.available_objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breadcrumb for future readers on SoftDeletableModel
, TIL!
Note that relying on the default objects manager to filter out not-deleted instances is deprecated. objects will include deleted objects in a future release. Until then, the recommended course of action is to use the manager all_objects when you want to include all instances.
https://django-model-utils.readthedocs.io/en/latest/models.html#softdeletablemodel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the future change to always need to rely on available_objects
was inspired by this issue thread.
if not default_enterprise_enrollment_intention: | ||
LOGGER.info( | ||
"No default enrollment intention found for enterprise course enrollment %s", | ||
enterprise_course_enrollment.id | ||
) | ||
return None | ||
|
||
default_enterprise_enrollment_realization, created = ( | ||
default_enterprise_enrollment_realization_model().objects.get_or_create( | ||
intended_enrollment=default_enterprise_enrollment_intention, | ||
realized_enrollment=enterprise_course_enrollment, | ||
) | ||
) | ||
|
||
if created: | ||
LOGGER.info( | ||
"Created default enterprise enrollment realization for default enrollment " | ||
"intention %s and enterprise course enrollment %s", | ||
default_enterprise_enrollment_intention.uuid, | ||
enterprise_course_enrollment.id | ||
) | ||
|
||
return default_enterprise_enrollment_intention, default_enterprise_enrollment_realization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chefs kiss
] | ||
}, | ||
'fulfillment_source': LicensedEnterpriseCourseEnrollment, | ||
'expected_enrollment_realization_count': 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this test create a new realization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, you're correct! This wasn't working properly as I had not added is_default_auto_enrollment
to EnrollmentsInfoSerializer
, so the field wasn't included during processing, always falling back to is_default_auto_enrollment=False
.
Updated/fixed via 24886e1
24886e1
to
9681789
Compare
9681789
to
19f26d5
Compare
19f26d5
to
7fde730
Compare
Description
Goal: When using the bulk enrollment API to auto-enroll learners in default enterprise enrollment intentions, related
DefaultEnterpriseEnrollmentRealization
records should be created.ENT-9684
CHANGELOG
Extended the
enroll_learners_in_courses
POST API withinEnterpriseCustomerViewSet
to (optionally) accept a new propertyis_default_auto_enrollment: <boolean>
on each bulk enrollment attempt. Whenis_default_auto_enrollment
isTrue
and the enrollment has succeeded:DefaultEnterpriseEnrollmentIntention
records that exist for the enterprise customer.DefaultEnterpriseEnrollmentIntention
to the associatedEnterpriseCourseEnrollment
based on matching course run key for the intention and course id in the enrollment.DefaultEnterpriseEnrollmentIntention
is found, gets/createsDefaultEnterpriseEnrollmentRealization
record for theintended_enrollment
andrealized_enrollment
.Updated
DefaultEnterpriseEnrollmentRealization
model to alter therealized_enrollment
field to be aOneToOneField
instead of aForeignKey
. This change makes it have more parity with the related models likeLicensedEnterpriseCourseEnrollment
andLearnerCreditEnterpriseCourseEnrollment
.Added property
default_enterprise_enrollment_realization
toEnterpriseCourseEnrollment
model, similar to the existinglicense
andlearner_credit_fulfillment
properties.Resolves deprecation warning with
SoftDeleteableModel
related to theDefaultEnterpriseEnrollmentIntention
model.metadata
dictionary returned by theDefaultEnterpriseEnrollmentIntentionViewSet
(learner_status
action).total_default_enterprise_course_enrollments
->total_default_enterprise_enrollment_intentions
Merge checklist:
requirements/*.txt
files)base.in
if needed in production but edx-platform doesn't install ittest-master.in
if edx-platform pins it, with a matching versionmake upgrade && make requirements
have been run to regenerate requirementsmake static
has been run to update webpack bundling if any static content was updated./manage.py makemigrations
has been run./manage.py lms makemigrations
in the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgrade
in edx-platform will look for the latest version in PyPi.