Skip to content
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

fix: get enrollments API #154

Open
shadinaif opened this issue Nov 27, 2024 · 0 comments
Open

fix: get enrollments API #154

shadinaif opened this issue Nov 27, 2024 · 0 comments
Assignees
Labels
test on production Being tested on production server

Comments

@shadinaif
Copy link
Collaborator

We have several things to fix:

  1. Replace this test with the following code, and it'll fail (a serializer issue)
    def test_success(self):
        """Verify that the view returns the correct response"""
        self.login_user(self.staff_user)
        response = self.client.get(self.url)
        self.assertEqual(response.status_code, http_status.HTTP_200_OK)

        user_id = 15
        course_id = 'course-v1:ORG1+5+5'
        response = self.client.get(self.url, data={'course_ids': course_id, 'user_ids': user_id})
        self.assertEqual(response.status_code, http_status.HTTP_200_OK)
        self.assertEqual(response.data['count'], 1)
        self.assertEqual(response.data['results'][0]['user_id'], user_id)
        self.assertEqual(response.data['results'][0]['course_id'], course_id)
  1. In get_learners_enrollments_queryset, make the annotations removable for performance reasons
  2. I found a permission bug in the original get_learners_by_course_queryset function. Same bug in the new get_learners_enrollments_queryset function. We do not want to fix the bug in the original since we're deprecating it soon. Just fix it with the new one. The function does not care about the caller's access. It should receive fx_permission_info and use it to verify that the caller has permission to the selected courses. Create a new function verify_course_access that takes fx_permission_info and course_ids as arguments and do the magic there; raise an FXCodedException if any course is not accessible by the caller.
  3. We also need to verify the access of the requested learners (user_ids). Use get_permitted_learners_queryset to do the verification at the beginning of get_learners_enrollments_queryset
  4. Finally, we have a bug when the caller is not specifying the course_ids. The query will return all courses in the system. It must filter on all accessible courses. fx_permission_info is to be used for that purpose too
@tehreem-sadat tehreem-sadat reopened this Dec 3, 2024
@tehreem-sadat tehreem-sadat added the on staging Task is implemented and deployed to staging, but not to production label Dec 6, 2024
@shadinaif shadinaif added test on production Being tested on production server and removed on staging Task is implemented and deployed to staging, but not to production labels Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test on production Being tested on production server
Projects
None yet
Development

No branches or pull requests

2 participants