ENT-2117 | Adding enterprise_learner_portal djangoapp to house endpoi… #544
Conversation
|
|
||
|
|
||
|
|
||
| # course_run_status |
There was a problem hiding this comment.
need to write the logic for this still (porting it over from program_enrollment code in the platform)
| 'installed in an Open edX environment.') | ||
| ) | ||
|
|
||
| user = get_object_or_404(User, email=user_email) |
There was a problem hiding this comment.
I think we want this endpoint to return the enrollments for the authenticated user. The client should not be able to request a specific user with a query param.
There was a problem hiding this comment.
Is your concern that someone viewing a page on the frontend could get information about other learners?
There was a problem hiding this comment.
Yes, that would be a concern. We'd need to add an is_staff check in here to allow someone to request another user's enrollments. In general, though, I think this endpoint is designed to service the learner portal and in that context you would never need to request another user's enrollments.
There was a problem hiding this comment.
Okay. I can definitely make sure is_staff is a requirement.
As for the resource the endpoint uses to return data, the endpoint itself is more flexible if we don't limit it to the authenticated user. You could imagine a staff user having the option of checking data for number of users other than one's own. Especially where this is a new endpoint, I don't think we need to add this restriction just yet
| from lms.djangoapps.program_enrollments.api.api import ( | ||
| get_due_dates, | ||
| get_course_run_url, | ||
| get_emails_enabled, |
There was a problem hiding this comment.
How many of these helper functions make DB calls or external API calls? I'm a bit worried about performance, but perhaps those concerns are premature at this point.
There was a problem hiding this comment.
Some, but not all. https://github.com/edx/edx-platform/pull/21258/files in api/api.py. get_due_dates looks to be one that does
| authentication_classes = (JwtAuthentication, BearerAuthentication, SessionAuthentication,) | ||
|
|
||
| def get(self, request, user_email): | ||
| if not request.user.is_staff: |
There was a problem hiding this comment.
This will prevent non-staff users from using this endpoint. Keep it simple for now, just return enrollments for the request user. We can make this endpoint more flexible in the future when/if we need to.
There was a problem hiding this comment.
Can you help me to understand the value of changing what I've already written here? This seems to be more of a matter of preference
There was a problem hiding this comment.
Actually @brittneyexline , do you have any sense of future product requirements that could guide this decision?
If we don't think we'll ever benefit from the way I've written this endpoint (specifically: being able to provide the email for any user), then I can change it
There was a problem hiding this comment.
The way you have written it here will prevent this endpoint from being useable by the learner portal. Most enterprise learners will not be staff so when the frontend app makes a request to this endpoint they will get a 403 response.
There was a problem hiding this comment.
i think you will have to modify setup.py to make sure this new package is getting exported. i would do a spot check to see if this app will be available when this branch is installed as the version of edx-enterprise to be sure.
other than that, this looks pretty good. i have one follow up i'd like to ask jennifer about around supporting multiple enterprises; but i would not worry about that in the scope of this ticket and PR.
| ) | ||
|
|
||
| user = request.user | ||
| enterprise_customer_user = get_object_or_404( |
There was a problem hiding this comment.
i'm wondering if we should be mindful of having multiple enterprise's linked to the learner here. probably it would be best to follow up with separate work if we are trying to support this case... i will bring it up with jennifer to see her thoughts.
There was a problem hiding this comment.
we would like to have whatever we build support users linked to multiple enterprises, though we are not going to change any existing code unaffected by the scope of our project. i would recommend adding an enterprise id as a parameter passed into the path so that the endpoint will only return enrollments scoped to the enterprise. i leave it up to you to file a new story or complete the work with this story... i just don't want to forget about it 😄
| overviews, | ||
| many=True, | ||
| context={ | ||
| 'user': enterprise_customer_user.user, |
There was a problem hiding this comment.
why add user here separately when the user is in the request?
There was a problem hiding this comment.
good catch, i think this is a vestige of the previous implementation (where we would want to serialize the data of the user whose email was handed to us)
christopappas
force-pushed
the
cpappas/ENT-2117
branch
from
daedc46
to
977b7e4
Compare
christopappas
force-pushed
the
cpappas/ENT-2117
branch
2 times, most recently
from
0c8918c
to
6c6ed85
Compare
| ) | ||
|
|
||
| user = request.user | ||
| enterprise_customer_user = get_object_or_404( |
There was a problem hiding this comment.
we would like to have whatever we build support users linked to multiple enterprises, though we are not going to change any existing code unaffected by the scope of our project. i would recommend adding an enterprise id as a parameter passed into the path so that the endpoint will only return enrollments scoped to the enterprise. i leave it up to you to file a new story or complete the work with this story... i just don't want to forget about it 😄
…nts needed for enterprise learner portal frontend Fixing indent Removing some whitespace Adding logic to serializer to make sure it is installed into openedx instance cleaning up imports; Adding tests Adding staff check on new endpoint. Also removing print statements Attempting to fix travis build Adding quality fixes more quality fixes Folding in refactors to get_course_run_status similar to what was done in edx-platform Changing user to query for based on discussion. changing tests Oops. missed a name change Tweaks around serializer request/user adding package to setup.py Updating readme and changelog Fixing changelog
christopappas
force-pushed
the
cpappas/ENT-2117
branch
from
6c6ed85
to
68dd73f
Compare
…nts-07dfeb0 Python Requirements Update
…nts needed for enterprise learner portal frontend
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.