-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: filter by current site organizations #35500
base: master
Are you sure you want to change the base?
feat: filter by current site organizations #35500
Conversation
Thanks for the pull request, @andrey-canon! This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
Hi @andrey-canon! Just flagging that there's a failing check here. |
53b5ac1
to
2bf833e
Compare
@andrey-canon hi there! Just checking to see if this is still in progress? |
Hi @mphilbrick211, nop, this is ready for review. |
Hi @felipemontoya, @mariajgrimaldi could you please take a look of this ? |
@andrey-canon looks like this needs to be rebased off the latest master to get the correct tests running, it looks like it's not running the new static asset shard and is running one too many JS shards. |
2bf833e
to
fd74df3
Compare
rebased |
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.
@andrey-canon thinking through this a bit more, I think it's worth it to add a test to ensure that the API doesn't break again in the future, can you add a small site-aware test of this endpoint?
5166064
to
3eab7ea
Compare
@feanil Done Previous coverageCurrent coverage |
Description
This change filters out course enrollments whose organization is no available for the current site when the site configuration is enabled
I am proposing this change because when an instance contains multiple sites, each of which has access to only specific organizations, there is an issue where the API returns all courses a user is enrolled in, even if the current site does not have access to certain organizations. Although the API offers a query parameter to filter by organization, this parameter only supports one organization at a time, whereas a site may have access to multiple organizations.
Testing instructions
/api/mobile/v4/users/<username>/course_enrollments/
and check that the result doesn't contain any invalid organization