ENT-2310 | Adding a post_save receiver to listen to CourseEnrollments… #589
Conversation
| "Creating EnterpriseCourseEnrollment for user %s " | ||
| "on course %s for enterprise_customer %s" | ||
| ), enterprise_customer_user.user_id, course_id, enterprise_customer) | ||
| EnterpriseCourseEnrollment.objects.create( |
There was a problem hiding this comment.
maybe use a get_or_create here just to be safe? (so we don't create duplicates)
There was a problem hiding this comment.
clicking enroll in devstack did NOT create 2 enterprise course enrollments, so i do not think this is necessary
There was a problem hiding this comment.
Hmm, good question. Even though in testing with devstack, it did not create 2 enterprise course enrollments... I feel that it might be good to be safe here just in case with get_or_create. If there are any edge cases where an enrollment might already exist in stage/production data, might be good to proactively handle that.
There was a problem hiding this comment.
some of the enrollment saves that trigger this will be from existing enterprise enrollment workflows, so it is definitely possible that the enterprise course enrollment already exists. you might want to check for its existence before making the catalog call.
christopappas
requested review from
brittneyexline,
douglashall and
adamstankiewicz
| "Creating EnterpriseCourseEnrollment for user %s " | ||
| "on course %s for enterprise_customer %s" | ||
| ), enterprise_customer_user.user_id, course_id, enterprise_customer) | ||
| EnterpriseCourseEnrollment.objects.create( |
There was a problem hiding this comment.
Hmm, good question. Even though in testing with devstack, it did not create 2 enterprise course enrollments... I feel that it might be good to be safe here just in case with get_or_create. If there are any edge cases where an enrollment might already exist in stage/production data, might be good to proactively handle that.
| """ | ||
| self.user = UserFactory(id=2, email='user@example.com') | ||
| self.enterprise_customer = EnterpriseCustomerFactory( | ||
| name='Team Titans', |
There was a problem hiding this comment.
nice enterprise customer name ;)
There was a problem hiding this comment.
looks good overall, just one comment.
another thing that i was thinking, though it may be overly cautious, is to put this behind a waffle flag so we can control when we turn it on and monitor it more closely. it could be overkill and then we'd have to clean up the flag later, but given the unpredictable nature of edx platform releases, it would allow us to have more control over when this is really getting turned on. food for thought :-)
| "Creating EnterpriseCourseEnrollment for user %s " | ||
| "on course %s for enterprise_customer %s" | ||
| ), enterprise_customer_user.user_id, course_id, enterprise_customer) | ||
| EnterpriseCourseEnrollment.objects.create( |
There was a problem hiding this comment.
some of the enrollment saves that trigger this will be from existing enterprise enrollment workflows, so it is definitely possible that the enterprise course enrollment already exists. you might want to check for its existence before making the catalog call.
|
The task will not be kicked off if Would there ever be a case where there would be more than 1 course enrollment created that maps to a single enterpriseCourseEnrollment? If not, I think this would be fine, right? |
|
i'm saying that if the EnterpriseCourseEnrollment record had already been created at the point that the task has been kicked off, something that is definitely possible for the current enterprise enrollment workflow (see https://github.com/edx/edx-enterprise/blob/master/enterprise/models.py#L760), then we could bypass doing the work of in the task in this case too. |
|
Ah, I see. Update coming |
|
@brittneyexline addressed your recommendation. updated to make sure we check if the EnterpriseCourseEnrollment exists before making the call to discovery (and added a test) |
christopappas
force-pushed
the
cpappas/ENT-2310
branch
from
4d7e133 to
09fd5e0
Compare
… table Adding in a test file I forgot. Updating a few test doctrings Actually adding in the tasks file too Updating task to prevent duplicate recreation of EnterpriseCourseEnrollment Appeasing pylint bumping version
christopappas
force-pushed
the
cpappas/ENT-2310
branch
from
09fd5e0 to
570080f
Compare
… table (openedx#589) Adding in a test file I forgot. Updating a few test doctrings Actually adding in the tasks file too Updating task to prevent duplicate recreation of EnterpriseCourseEnrollment Appeasing pylint bumping version
… table
Before I merged, I definitely want to make sure I can verify that the async element of this will work on Production... will want to mess around a bit more with Devstack to that end.This works in devstack with
CELERY_ALWAYS_EAGER=True, meaning that tasks run synchronously. Production and staging environment configs are different enough from devstack that I'm not 100% convinced testing asynchronous functionality means this code will or will not work in production, though I will still test setting up a broker and worker locally to prove the concept.Barring discovering anything that drastically changes my approach, this should be ready for review!
Merge checklist:
requirements/*.txtfiles)base.inif needed in production but edx-platform doesn't install ittest-master.inif edx-platform pins it, with a matching versionmake upgrade && make requirementshave been run to regenerate requirementsmake statichas been run to update webpack bundling if any static content was updatedPost merge: