feat: [FC-0031] Add optional field 'is_enrolled' to course detail view #33646
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @GlugovGrGlib! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
| """ | ||
| response = super().to_representation(instance) | ||
| if can_show_certificate_available_date_field(instance): | ||
| response['certificate_available_date'] = instance.certificate_available_date | ||
|
|
||
| requested_user = self.context['request'].query_params.get('username', None) | ||
| if self.context['request'].user.is_authenticated and requested_user: |
There was a problem hiding this comment.
This would let any user look up the enrollment status for any other user, I think we would only want to allow that if the user is a staff user and otherwise, we should only provide this info in the requested user is the same as the authenticated user.
Alternatively, we could just flag this on if an authenticated user is making the call and provide the authenticated users enrollment status in that case. This might be simpler?
There was a problem hiding this comment.
@feanil The code was updated to follow proposed logic, please review
KyryloKireiev
force-pushed
the
rg/feat/FC0031/update_course_detail_view_endpoint
branch
from
d2cab9a
to
b3e0aea
Compare
|
@feanil I believe your comments have been addressed. |
cms/envs/devstack_docker.py
Outdated
There was a problem hiding this comment.
Why was this added to the PR? is this a mistake? This file is nearly empty because we're trying to get rid of it so if there is a reason for these override, I'd like to understand it so I can help find a better place for the overrides.
There was a problem hiding this comment.
Yes, sorry, this is mistake happened during rebase, will update this, thanks
lms/envs/devstack_docker.py
Outdated
There was a problem hiding this comment.
Same question here as in the other settings file:
Why was this added to the PR? is this a mistake? This file is nearly empty because we're trying to get rid of it so if there is a reason for these override, I'd like to understand it so I can help find a better place for the overrides.
| if requested_username: | ||
| user = self.context['request'].user | ||
| if ((user.is_authenticated and user.username == requested_username) | ||
| or user.is_staff): |
There was a problem hiding this comment.
One more thing: I think this should also be allowed for is_superuser users. Can you add that as well?
There was a problem hiding this comment.
Yes, is_superuser status should be allowed to do this as well, thanks
GlugovGrGlib
force-pushed
the
rg/feat/FC0031/update_course_detail_view_endpoint
branch
from
b3e0aea
to
d344a7e
Compare
|
@GlugovGrGlib 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
In this PR, the
is_enrolledparameter is included in theCourseDetailViewendpoint's response when the optionalusernamequery parameter is provided.Supporting information
This contribution is a part of the FC-0031
Testing instructions
GET /api/mobile/v3/course_info/{course_key}/info
See that "is_enrolled" is present in the response
Other information
The rejected implementation for the mobile endpoint can be found in #33297