ENT-2230 Add patch method on learner portal enrollment endpoint #565
Conversation
brittneyexline
requested review from
douglashall,
adamstankiewicz and
christopappas
|
|
||
| data = EnterpriseCourseEnrollmentSerializer( | ||
| overviews, | ||
| enterprise_enrollments, |
There was a problem hiding this comment.
Isn't this a deoptimization? Now we will make a query to the course overview table for every enterprise enrollment instead of batching them like the previous code was doing.
There was a problem hiding this comment.
the motivation for this was so that it was easier to serialize one enrollment's data below in what the patch function returned, and to also have access to the enterprise enrollment object itself in the serializer for the status determination. i considered passing the enterprise_enrollments as context, but it seemed awkward to do that considering this serializer is supposed to serialize "enrollments". i'm not sure what the performance implications are though... i would guess the table is indexed by course id?
| enterprise_customer_user=enterprise_customer_user | ||
| ).values_list('course_id', flat=True) | ||
|
|
||
| overviews = get_course_overviews(course_ids_for_ent_enrollments) |
There was a problem hiding this comment.
Maybe just move the get_course_overviews call to the serializer, but keep the ability to pass multiple enterprise enrollments to the serializer.
There was a problem hiding this comment.
By default the serializer will only ever work on 1 object at a time though, so even if we hand multiple course enrollments to the serializer, still would be doing a get_course_overviews call for each item in that list/queryset we hand it
A suggestion: in the view, make 1 call to get all the course overviews that we will ever need to look at, and then pass that to the serializer's context. So instead of calling get_course_overviews in the serializer, you could just lookup the course_overview info in the dict that exists in the context
^ This would get you a) 1 DB call for all course_overviews b) the ability to still serialize enterprise course enrollment objects
| enterprise_customer_id = request.query_params.get('enterprise_id', None) | ||
| course_id = request.query_params.get('course_id', None) | ||
| marked_done = request.query_params.get('marked_done', None) | ||
| if not (enterprise_customer_id and course_id) or marked_done is None: |
There was a problem hiding this comment.
Personal preference, but I think I prefer not mixing ANDs and ORs if it's possible. Something like:
if not enterprise_customer_id or not course_id or marked_done is None:
Feel free to disregard though
| context={'request': request}, | ||
| ).data | ||
|
|
||
| return Response(data) |
There was a problem hiding this comment.
Having the response as the newly updated course enrollment serialized by EnterpriseCourseEnrollmentSerializer matches what the frontend will be looking for :)
brittneyexline
force-pushed
the
bexline/move_to_completed
branch
3 times, most recently
from
459ce81 to
1c2dd30
Compare
brittneyexline
force-pushed
the
bexline/move_to_completed
branch
from
1c2dd30 to
96688bb
Compare
brittneyexline
force-pushed
the
bexline/move_to_completed
branch
from
96688bb to
736a300
Compare
Description: Describe in a couple of sentence what this PR adds
JIRA: Link to JIRA ticket
Dependencies: dependencies on other outstanding PRs, issues, etc.
Merge deadline: List merge deadline (if any)
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
Merge checklist:
base.inif needed in production, but edx-platform doesn't install it.test-master.inif edx-platform pins it, with a matching version.make upgrade && make requirements(and make sure to fix any errors).DO NOT just add dependencies to
requirements/*.txtfiles.make staticfor webpack bundling if any static content was updated.Post merge:
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.