From 61eb886447aeeb7e0568d048d0423022398fea32 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 19 Nov 2024 16:18:49 +0100 Subject: [PATCH] Take course/assignment ID on get_request_XXX methods Instead of trying different locations in secession (matchdict, parsed params...) pass the right location directly form the view. --- lms/services/dashboard.py | 28 +---------- lms/views/dashboard/api/assignment.py | 8 ++- lms/views/dashboard/api/course.py | 4 +- lms/views/dashboard/api/grading.py | 8 ++- lms/views/dashboard/api/user.py | 12 +++-- lms/views/dashboard/views.py | 8 ++- tests/unit/lms/services/dashboard_test.py | 49 ++++--------------- .../views/dashboard/api/assignment_test.py | 8 +-- .../lms/views/dashboard/api/course_test.py | 4 +- .../lms/views/dashboard/api/grading_test.py | 15 ++++-- .../unit/lms/views/dashboard/api/user_test.py | 12 +++-- tests/unit/lms/views/dashboard/views_test.py | 8 +-- 12 files changed, 73 insertions(+), 91 deletions(-) diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 05b97d8ff8..c2d96396e9 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -34,22 +34,8 @@ def __init__( # noqa: PLR0913 self._organization_service = organization_service self._h_authority = h_authority - def get_request_assignment(self, request) -> Assignment: + def get_request_assignment(self, request, assigment_id: int) -> Assignment: """Get and authorize an assignment for the given request.""" - # Requests that are scoped to one assignment on the URL parameter - assigment_id = request.matchdict.get("assignment_id") - if not assigment_id: - # Request that are scoped to a single assignment but as a query parameter - assigment_id = request.parsed_params.get("assignment_id") - - if ( - not assigment_id - and request.parsed_params.get("assignment_ids") - and len(request.parsed_params["assignment_ids"]) == 1 - ): - # Request that take a list of assignments, but we only recieved one, the requests is scoped to that one assignment - assigment_id = request.parsed_params["assignment_ids"][0] - assignment = self._assignment_service.get_by_id(assigment_id) if not assignment: raise HTTPNotFound() @@ -73,18 +59,8 @@ def get_request_assignment(self, request) -> Assignment: return assignment - def get_request_course(self, request): + def get_request_course(self, request, course_id: int): """Get and authorize a course for the given request.""" - # Requests that are scoped to one course on the URL parameter - course_id = request.matchdict.get("course_id") - if ( - not course_id - and request.parsed_params.get("course_ids") - and len(request.parsed_params["course_ids"]) == 1 - ): - # Request that take a list of courses, but we only recieved one, the requests is scoped to that one course - course_id = request.parsed_params["course_ids"][0] - course = self._course_service.get_by_id(course_id) if not course: raise HTTPNotFound() diff --git a/lms/views/dashboard/api/assignment.py b/lms/views/dashboard/api/assignment.py index 7809afa8a6..05d62e1f01 100644 --- a/lms/views/dashboard/api/assignment.py +++ b/lms/views/dashboard/api/assignment.py @@ -99,7 +99,9 @@ def assignments(self) -> APIAssignments: permission=Permissions.DASHBOARD_VIEW, ) def assignment(self) -> APIAssignment: - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, self.request.matchdict["assignment_id"] + ) api_assignment = APIAssignment( id=assignment.id, title=assignment.title, @@ -135,7 +137,9 @@ def course_assignments_metrics(self) -> APIAssignments: self.request ) - course = self.dashboard_service.get_request_course(self.request) + course = self.dashboard_service.get_request_course( + self.request, self.request.matchdict["course_id"] + ) course_students = self.request.db.scalars( self.user_service.get_users( course_ids=[course.id], diff --git a/lms/views/dashboard/api/course.py b/lms/views/dashboard/api/course.py index 67c79df3c3..ae48603d69 100644 --- a/lms/views/dashboard/api/course.py +++ b/lms/views/dashboard/api/course.py @@ -137,7 +137,9 @@ def courses_metrics(self) -> APICourses: permission=Permissions.DASHBOARD_VIEW, ) def course(self) -> APICourse: - course = self.dashboard_service.get_request_course(self.request) + course = self.dashboard_service.get_request_course( + self.request, self.request.matchdict["course_id"] + ) return { "id": course.id, "title": course.lms_name, diff --git a/lms/views/dashboard/api/grading.py b/lms/views/dashboard/api/grading.py index 0eaa257a40..516919501e 100644 --- a/lms/views/dashboard/api/grading.py +++ b/lms/views/dashboard/api/grading.py @@ -45,7 +45,9 @@ def __init__(self, request) -> None: schema=AutoGradeSyncSchema, ) def create_grading_sync(self): - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, self.request.matchdict["assignment_id"] + ) if self.auto_grading_service.get_in_progress_sync(assignment): self.request.response.status_int = 400 @@ -85,7 +87,9 @@ def create_grading_sync(self): permission=Permissions.GRADE_ASSIGNMENT, ) def get_grading_sync(self): - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, self.request.matchdict["assignment_id"] + ) if grading_sync := self.auto_grading_service.get_last_sync(assignment): return self._serialize_grading_sync(grading_sync) diff --git a/lms/views/dashboard/api/user.py b/lms/views/dashboard/api/user.py index 01afa18fb7..9a1842a238 100644 --- a/lms/views/dashboard/api/user.py +++ b/lms/views/dashboard/api/user.py @@ -110,7 +110,9 @@ def students(self) -> APIStudents: ) def students_metrics(self) -> APIStudents: """Fetch the stats for one particular assignment.""" - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, self.request.parsed_params["assignment_id"] + ) request_segment_authority_provided_ids = self.request.parsed_params.get( "segment_authority_provided_ids" @@ -211,7 +213,9 @@ def _students_query( and not segment_authority_provided_ids ): # Fetch the assignment to be sure the current user has access to it. - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, assignment_ids[0] + ) return self.user_service.get_users_for_assignment( role_scope=RoleScope.COURSE, @@ -223,7 +227,9 @@ def _students_query( # Single course fetch if course_ids and len(course_ids) == 1 and not segment_authority_provided_ids: # Fetch the course to be sure the current user has access to it. - course = self.dashboard_service.get_request_course(self.request) + course = self.dashboard_service.get_request_course( + self.request, course_id=course_ids[0] + ) return self.user_service.get_users_for_course( role_scope=RoleScope.COURSE, diff --git a/lms/views/dashboard/views.py b/lms/views/dashboard/views.py index 785c972e9c..4ca671c709 100644 --- a/lms/views/dashboard/views.py +++ b/lms/views/dashboard/views.py @@ -81,7 +81,9 @@ def assignment_show(self): Authenticated via the LTIUser present in a cookie making this endpoint accessible directly in the browser. """ - assignment = self.dashboard_service.get_request_assignment(self.request) + assignment = self.dashboard_service.get_request_assignment( + self.request, self.request.matchdict["assignment_id"] + ) self.request.context.js_config.enable_dashboard_mode( AUTHORIZATION_DURATION_SECONDS ) @@ -105,7 +107,9 @@ def course_show(self): Authenticated via the LTIUser present in a cookie making this endpoint accessible directly in the browser. """ - course = self.dashboard_service.get_request_course(self.request) + course = self.dashboard_service.get_request_course( + self.request, self.request.matchdict["course_id"] + ) self.request.context.js_config.enable_dashboard_mode( AUTHORIZATION_DURATION_SECONDS ) diff --git a/tests/unit/lms/services/dashboard_test.py b/tests/unit/lms/services/dashboard_test.py index b33acf5cf7..7c21f59e8a 100644 --- a/tests/unit/lms/services/dashboard_test.py +++ b/tests/unit/lms/services/dashboard_test.py @@ -12,35 +12,31 @@ class TestDashboardService: def test_get_request_assignment_404(self, pyramid_request, assignment_service, svc): - pyramid_request.matchdict["assignment_id"] = sentinel.id assignment_service.get_by_id.return_value = None with pytest.raises(HTTPNotFound): - svc.get_request_assignment(pyramid_request) + svc.get_request_assignment(pyramid_request, sentinel.id) def test_get_request_assignment_403(self, pyramid_request, course_service, svc): - pyramid_request.matchdict["assignment_id"] = sentinel.id course_service.is_member.return_value = False with pytest.raises(HTTPUnauthorized): - svc.get_request_assignment(pyramid_request) + svc.get_request_assignment(pyramid_request, sentinel.id) def test_get_request_assignment_for_staff( self, pyramid_request, assignment_service, pyramid_config, svc ): pyramid_config.testing_securitypolicy(permissive=True) - pyramid_request.matchdict["assignment_id"] = sentinel.id assignment_service.is_member.return_value = False - assert svc.get_request_assignment(pyramid_request) + assert svc.get_request_assignment(pyramid_request, sentinel.id) def test_get_request_assignment( self, pyramid_request, course_service, svc, assignment_service ): - pyramid_request.matchdict["assignment_id"] = sentinel.id course_service.is_member.return_value = True - assert svc.get_request_assignment(pyramid_request) + assert svc.get_request_assignment(pyramid_request, sentinel.id) course_service.is_member.assert_called_once_with( assignment_service.get_by_id.return_value.course, @@ -62,18 +58,7 @@ def test_get_request_assignment_for_admin( assignment_service.get_by_id.return_value = assignment get_request_admin_organizations.return_value = [organization] - pyramid_request.matchdict["assignment_id"] = sentinel.id - - assert svc.get_request_assignment(pyramid_request) - - def test_get_request_assignment_for_parsed_params_assignment_id( - self, pyramid_request, assignment_service, svc - ): - pyramid_request.parsed_params = {"assignment_ids": [sentinel.parsed_params_id]} - - svc.get_request_assignment(pyramid_request) - - assignment_service.get_by_id.assert_called_once_with(sentinel.parsed_params_id) + assert svc.get_request_assignment(pyramid_request, sentinel.id) def test_get_request_course_404( self, @@ -81,36 +66,24 @@ def test_get_request_course_404( course_service, svc, ): - pyramid_request.matchdict["course_id"] = sentinel.id course_service.get_by_id.return_value = None with pytest.raises(HTTPNotFound): - svc.get_request_course(pyramid_request) + svc.get_request_course(pyramid_request, sentinel.id) def test_get_request_course_403(self, pyramid_request, course_service, svc): - pyramid_request.matchdict["course_id"] = sentinel.id course_service.is_member.return_value = False with pytest.raises(HTTPUnauthorized): - svc.get_request_course(pyramid_request) + svc.get_request_course(pyramid_request, sentinel.id) def test_get_request_course_for_staff( self, pyramid_request, course_service, pyramid_config, svc ): pyramid_config.testing_securitypolicy(permissive=True) - pyramid_request.matchdict["course_id"] = sentinel.id course_service.is_member.return_value = False - assert svc.get_request_course(pyramid_request) - - def test_get_request_for_parsed_params_course_ids( - self, pyramid_request, course_service, svc - ): - pyramid_request.parsed_params = {"course_ids": [sentinel.parsed_params_id]} - - svc.get_request_course(pyramid_request) - - course_service.get_by_id.assert_called_once_with(sentinel.parsed_params_id) + assert svc.get_request_course(pyramid_request, sentinel.id) def test_get_request_course_for_admin( self, @@ -123,15 +96,13 @@ def test_get_request_course_for_admin( ): course_service.get_by_id.return_value = course get_request_admin_organizations.return_value = [organization] - pyramid_request.matchdict["course_id"] = sentinel.id - assert svc.get_request_course(pyramid_request) + assert svc.get_request_course(pyramid_request, sentinel.id) def test_get_request_course(self, pyramid_request, course_service, svc): - pyramid_request.matchdict["course_id"] = sentinel.id course_service.is_member.return_value = True - assert svc.get_request_course(pyramid_request) + assert svc.get_request_course(pyramid_request, sentinel.id) def test_add_dashboard_admin(self, svc, db_session): admin = svc.add_dashboard_admin( diff --git a/tests/unit/lms/views/dashboard/api/assignment_test.py b/tests/unit/lms/views/dashboard/api/assignment_test.py index 6c3c6a80e4..023a58a644 100644 --- a/tests/unit/lms/views/dashboard/api/assignment_test.py +++ b/tests/unit/lms/views/dashboard/api/assignment_test.py @@ -64,7 +64,7 @@ def test_assignment( response = views.assignment() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) assert response == { @@ -91,7 +91,7 @@ def test_assignment_with_auto_grading( response = views.assignment() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) assert response == { @@ -119,7 +119,7 @@ def test_assignment_with_groups( response = views.assignment() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) assignment_service.get_assignment_groups.assert_called_once_with(assignment) @@ -146,7 +146,7 @@ def test_assignment_with_sections( response = views.assignment() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) assignment_service.get_assignment_sections.assert_called_once_with(assignment) diff --git a/tests/unit/lms/views/dashboard/api/course_test.py b/tests/unit/lms/views/dashboard/api/course_test.py index 8474760a89..7b48fd3ed9 100644 --- a/tests/unit/lms/views/dashboard/api/course_test.py +++ b/tests/unit/lms/views/dashboard/api/course_test.py @@ -94,7 +94,9 @@ def test_course(self, views, pyramid_request, dashboard_service): response = views.course() - dashboard_service.get_request_course.assert_called_once_with(pyramid_request) + dashboard_service.get_request_course.assert_called_once_with( + pyramid_request, sentinel.id + ) assert response == { "id": course.id, diff --git a/tests/unit/lms/views/dashboard/api/grading_test.py b/tests/unit/lms/views/dashboard/api/grading_test.py index 7e2430518f..d6e91a6a9e 100644 --- a/tests/unit/lms/views/dashboard/api/grading_test.py +++ b/tests/unit/lms/views/dashboard/api/grading_test.py @@ -1,4 +1,4 @@ -from unittest.mock import Mock +from unittest.mock import Mock, sentinel import pytest from h_matchers import Any @@ -20,6 +20,7 @@ def test_create_grading_sync_with_no_lms_users( dashboard_service, assignment, ): + pyramid_request.matchdict = {"assignment_id": sentinel.id} dashboard_service.get_request_assignment.return_value = assignment auto_grading_service.get_in_progress_sync.return_value = None pyramid_request.parsed_params["grades"] = [ @@ -29,7 +30,7 @@ def test_create_grading_sync_with_no_lms_users( views.create_grading_sync() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) auto_grading_service.get_in_progress_sync.assert_called_once_with( dashboard_service.get_request_assignment.return_value @@ -44,12 +45,13 @@ def test_create_grading_sync_with_existing_sync( dashboard_service, assignment, ): + pyramid_request.matchdict = {"assignment_id": sentinel.id} dashboard_service.get_request_assignment.return_value = assignment views.create_grading_sync() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) auto_grading_service.get_in_progress_sync.assert_called_once_with( dashboard_service.get_request_assignment.return_value @@ -65,6 +67,7 @@ def test_create_grading_sync( assignment, db_session, ): + pyramid_request.matchdict = {"assignment_id": sentinel.id} pyramid_request.parsed_params["grades"] = [ {"h_userid": "STUDENT_1", "grade": 0.5}, {"h_userid": "STUDENT_2", "grade": 1}, @@ -78,7 +81,7 @@ def test_create_grading_sync( response = views.create_grading_sync() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) auto_grading_service.get_in_progress_sync.assert_called_once_with( dashboard_service.get_request_assignment.return_value @@ -105,11 +108,12 @@ def test_get_grading_sync( dashboard_service, grading_sync, ): + pyramid_request.matchdict = {"assignment_id": sentinel.id} auto_grading_service.get_last_sync.return_value = grading_sync response = views.get_grading_sync() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) auto_grading_service.get_last_sync.assert_called_once_with( dashboard_service.get_request_assignment.return_value @@ -132,6 +136,7 @@ def test_get_grading_sync( def test_get_grading_sync_not_found( self, auto_grading_service, views, pyramid_request ): + pyramid_request.matchdict = {"assignment_id": sentinel.id} auto_grading_service.get_last_sync.return_value = None response = views.get_grading_sync() diff --git a/tests/unit/lms/views/dashboard/api/user_test.py b/tests/unit/lms/views/dashboard/api/user_test.py index bba115f5fc..41139657b6 100644 --- a/tests/unit/lms/views/dashboard/api/user_test.py +++ b/tests/unit/lms/views/dashboard/api/user_test.py @@ -119,7 +119,7 @@ def test_students_metrics( response = views.students_metrics() dashboard_service.get_request_assignment.assert_has_calls( - [call(pyramid_request)] + [call(pyramid_request, sentinel.id)] ) h_api.get_annotation_counts.assert_called_once_with( [g.authority_provided_id for g in assignment.groupings], @@ -184,6 +184,7 @@ def test_students_metrics_with_auto_grading( pyramid_request.parsed_params = { "h_userids": sentinel.h_userids, + "assignment_id": sentinel.assignment_id, } assignment = factories.Assignment(course=factories.Course()) assignment.auto_grading_config = AutoGradingConfig( @@ -207,7 +208,10 @@ def test_students_metrics_with_auto_grading( response = views.students_metrics() dashboard_service.get_request_assignment.assert_has_calls( - [call(pyramid_request), call(pyramid_request)] + [ + call(pyramid_request, sentinel.assignment_id), + call(pyramid_request, assignment.id), + ] ) h_api.get_annotation_counts.assert_called_once_with( [g.authority_provided_id for g in assignment.groupings], @@ -277,7 +281,9 @@ def test__students_query_single_course( views._students_query(assignment_ids=None, segment_authority_provided_ids=None) # noqa: SLF001 - dashboard_service.get_request_course.assert_called_once_with(pyramid_request) + dashboard_service.get_request_course.assert_called_once_with( + pyramid_request, sentinel.course_id + ) user_service.get_users_for_course.assert_called_once_with( role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, diff --git a/tests/unit/lms/views/dashboard/views_test.py b/tests/unit/lms/views/dashboard/views_test.py index 29008a4824..03c26759e7 100644 --- a/tests/unit/lms/views/dashboard/views_test.py +++ b/tests/unit/lms/views/dashboard/views_test.py @@ -48,7 +48,7 @@ def test_assignment_show(self, views, pyramid_request, dashboard_service): views.assignment_show() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once() self.assert_cookie_value(pyramid_request.response) @@ -63,7 +63,9 @@ def test_course_show(self, views, pyramid_request, dashboard_service): views.course_show() - dashboard_service.get_request_course.assert_called_once_with(pyramid_request) + dashboard_service.get_request_course.assert_called_once_with( + pyramid_request, sentinel.id + ) pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once() self.assert_cookie_value(pyramid_request.response) @@ -91,7 +93,7 @@ def test_assignment_show_with_no_lti_user( views.assignment_show() dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + pyramid_request, sentinel.id ) pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once()