-
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: adds openedx-filter hook to the course enrollments site #31331
feat: adds openedx-filter hook to the course enrollments site #31331
Conversation
Thanks for the pull request, @JuanDavidBuitrago! 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. This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate. |
Hi @mariajgrimaldi and @felipemontoya, could you help me review this? |
Hi @JuanDavidBuitrago! Just checking in on this. Are you planning to pursue this pull request? It looks like some failing tests need to be re-run. |
Hi @mphilbrick211! Yes, I'm planning to pursue this. I just need to make changes in openedx/openedx-filters#47 to update some requirements and solve tests in this PR. |
Hi @JuanDavidBuitrago! Just checking in on this! |
try: | ||
## .. filter_implemented_name: CourseEnrollmentQuerysetRequested | ||
## .. filter_type: org.openedx.learning.course_enrollment_queryset.requested.v1 | ||
enrollments = CourseEnrollmentQuerysetRequested.run_filter(enrollments=enrollments) | ||
except CourseEnrollmentQuerysetRequested.PreventEnrollmentQuerysetRequest as exc: | ||
raise EnrollmentRequestNotAllowed(str(exc)) from exc |
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.
Remember to add tests for this implementation. You could use these tests as guidance.
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.
Can you add a test testing this line?
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.
^ I second this comment again
Hi @JuanDavidBuitrago - just checking in to see if you have an update? |
Hi @mphilbrick211 and @mariajgrimaldi I need to do some tests with everything integrated to verify that everything continues to work well, I need a little more time. |
056638c
to
14c4df2
Compare
de032b6
to
54866f8
Compare
Hi, we are studying where else we should add the filter to make it as complete as possible |
Addressing @JuanDavidBuitrago comment: IMO, we have two options for this implementation:
I like option one, I'm just hesitant about how aggressive it is and how much it affects the whole service. It's not just getting the queryset when making an API call, it's related to all the models queries. Option two is good as well, but we must clarify the cases we cover. I'd like to know what you both think, including @felipemontoya. |
At the moment, we only have included the use of the filter when we make a request with a user without staff permissions. @mariajgrimaldi have you identified another connection point? |
@JuanDavidBuitrago: there are many places where this could go. |
With option two that you put above, I think we can cover what is necessary to obtain the course enrollments that we want, we have already tested it in the edx-platform fork that we manage eduNEXT/edunext-platform#696 |
Hi @JuanDavidBuitrago - just following up on this :) |
Hi @JuanDavidBuitrago! Looks like some tests are failing and need to be re-run. |
7477128
to
14df646
Compare
14df646
to
11e386d
Compare
dd5deb0
to
c470e52
Compare
c470e52
to
c0c68c8
Compare
Test filter enrollment queryset when a request is made. | ||
|
||
Expected result: | ||
- CourseEnrollmentQuerysetRequested is triggered and executes TestCourseEnrollmentsPipelineStep. | ||
- The result is a list of course enrollments queryset filter by org | ||
""" | ||
enrollments = get_course_enrollments(self.user) | ||
|
||
self.assertListEqual(self.enrollment, enrollments) |
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.
This is not true, is it? This tests that when the filter is not turned on, then the filtering doesn't occur. Or am I missing something?
self.assertEqual(expected_enrollment, enrollments) | ||
self.assertAlmostEqual(len(enrollments), len(expected_enrollment), 1) |
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.
Please, check that all the courses associated with the enrollment have the demo org
Expected result: | ||
- CourseEnrollmentQuerysetRequested is triggered and executes TestCourseEnrollmentsPipelineStep. | ||
- The result is a list of course enrollments queryset filter by org |
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.
You'll need to update the docstring here too.
view.request = self.mock_request | ||
enrollments = view.get_queryset() | ||
|
||
self.assertEqual(self.enrollment, enrollments) |
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.
can we check that both orgs demo
and test
are included here?
enrollments = view.get_queryset() | ||
|
||
self.assertAlmostEqual(len(enrollments), len(expected_enrollment), 1) | ||
self.assertEqual(expected_enrollment.course.org, "helo") |
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.
Not sure what's this. Isn't the org supposed to be demo
?
I left a few more comments. Also, can we make the tests pass? |
@mphilbrick211: I spoke internally with @JuanDavidBuitrago's team, and they told me they didn't have the capacity to take this on right now. I'll move this PR to draft to give them time to work on it. Thanks! |
Hi @mphilbrick211 @mariajgrimaldi, thank you for your patience. |
Closing this for now. We can reopen in the future if needed. Thanks! |
@JuanDavidBuitrago Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR introduces a new openedx-filters event
org.openedx.learning.course_enrollment_queryset.requested.v1
. This is implemented to guaranty multi tenancy in eox-tenant in course enrollments request, but eox-tenant is only an example would consider use.Impact by the change on GET request:
Supporting information
There are not openedx related tickets. The related PR are:
Testing instructions
The change can be tested with testing instructions in eox-tenant plugin.
Deadline
"None"
Other information
Warning: Once accepted this PR, it has to be merged only after openedx/openedx-filters#47 is merged and the requirements in this PR are updated.
Settings