From 403c34e4f6c92574d0aa66951d24c29eeac30fb3 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Wed, 24 Jul 2024 15:23:07 +0200 Subject: [PATCH] Don't scope dashboard URLs and cookies to organizations --- lms/js_config_types.py | 2 +- lms/resources/_js_config/__init__.py | 4 +- lms/routes.py | 21 +++------ lms/security.py | 2 +- lms/services/course.py | 6 +-- lms/services/dashboard.py | 42 ++++++++++++------ lms/static/scripts/frontend_apps/api-types.ts | 2 +- .../frontend_apps/components/AppRoot.tsx | 2 +- .../dashboard/OrganizationActivity.tsx | 21 +++------ .../test/OrganizationActivity-test.js | 4 +- lms/static/scripts/frontend_apps/config.ts | 2 +- lms/views/admin/assignment.py | 6 +-- lms/views/dashboard/api/course.py | 11 +++-- lms/views/dashboard/views.py | 16 +++---- .../lms/resources/_js_config/__init___test.py | 2 +- tests/unit/lms/services/course_test.py | 30 +++++++------ tests/unit/lms/services/dashboard_test.py | 43 +++++++++---------- tests/unit/lms/views/admin/assignment_test.py | 10 +---- tests/unit/lms/views/admin/course_test.py | 10 +---- .../lms/views/dashboard/api/course_test.py | 27 +++++++----- tests/unit/lms/views/dashboard/views_test.py | 32 +++++--------- 21 files changed, 141 insertions(+), 154 deletions(-) diff --git a/lms/js_config_types.py b/lms/js_config_types.py index 46ffb9bc34..b10866bbc6 100644 --- a/lms/js_config_types.py +++ b/lms/js_config_types.py @@ -83,7 +83,7 @@ class DashboardRoutes(TypedDict): course_assignments_metrics: str - organization_courses: str + organization_metrics: str courses: str """Paginated endpoint to fetch courses""" diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 221e729c7f..7a6e0e42e1 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -268,8 +268,8 @@ def enable_dashboard_mode(self) -> None: course_assignments_metrics=self._to_frontend_template( "api.dashboard.course.assignments.metrics" ), - organization_courses=self._to_frontend_template( - "api.dashboard.organizations.courses" + courses_metrics=self._to_frontend_template( + "api.dashboard.courses.metrics" ), courses=self._to_frontend_template("api.dashboard.courses"), assignments=self._to_frontend_template( diff --git a/lms/routes.py b/lms/routes.py index 68ed4a28e0..ea70b9b107 100644 --- a/lms/routes.py +++ b/lms/routes.py @@ -247,37 +247,30 @@ def includeme(config): # noqa: PLR0915 config.add_route( "dashboard.assignment", - "/dashboard/organizations/{public_id}/assignments/{assignment_id}", + "/dashboard/assignments/{assignment_id}", factory="lms.resources.dashboard.DashboardResource", ) config.add_route( "dashboard.course", - "/dashboard/organizations/{public_id}/courses/{course_id}", + "/dashboard/courses/{course_id}", factory="lms.resources.dashboard.DashboardResource", ) config.add_route( - "dashboard.organization", - "/dashboard/organizations/{organization_public_id}", - factory="lms.resources.dashboard.DashboardResource", + "dashboard", "/dashboard", factory="lms.resources.dashboard.DashboardResource" ) + config.add_route("api.dashboard.courses.metrics", "/api/dashboard/courses/metrics") config.add_route("api.dashboard.courses", "/api/dashboard/courses") + config.add_route("api.dashboard.course", r"/api/dashboard/courses/{course_id}") config.add_route( - "api.dashboard.organizations.courses", - "/api/dashboard/organizations/{organization_public_id}/courses", + "api.dashboard.assignment", r"/api/dashboard/assignments/{assignment_id}" ) - - config.add_route( - "api.dashboard.assignment", "/api/dashboard/assignments/{assignment_id}" - ) - config.add_route("api.dashboard.course", "/api/dashboard/courses/{course_id}") - config.add_route("api.dashboard.assignments", "/api/dashboard/assignments") config.add_route( "api.dashboard.course.assignments.metrics", - "/api/dashboard/courses/{course_id}/assignments/metrics", + r"/api/dashboard/courses/{course_id}/assignments/metrics", ) config.add_route("api.dashboard.students", "/api/dashboard/students") diff --git a/lms/security.py b/lms/security.py index 605537a13a..488db66a4b 100644 --- a/lms/security.py +++ b/lms/security.py @@ -103,7 +103,7 @@ def get_policy(request: Request): # noqa: PLR0911 # Routes that require the Google auth policy return LMSGoogleSecurityPolicy() - if path.startswith(("/dashboard/organization", "/api/dashboard")): + if path.startswith(("/dashboard", "/api/dashboard")): # For certain routes we only use the google policy in case it resulted # non empty identity. # This is useful for routes that can be used by admin pages users on top of diff --git a/lms/services/course.py b/lms/services/course.py index 3e9114efe8..809f4ec108 100644 --- a/lms/services/course.py +++ b/lms/services/course.py @@ -141,21 +141,21 @@ def search( # noqa: PLR0913, PLR0917 def get_courses( self, + organization_ids: list[int], instructor_h_userid: str | None = None, - organization: Organization | None = None, h_userids: list[str] | None = None, assignment_ids: list[str] | None = None, ) -> Select[tuple[Course]]: """Get a list of unique courses. - :param organization: organization the courses belong to. + :param organization_ids: organizations the courses belong to. :param instructor_h_userid: return only courses where instructor_h_userid is an instructor. :param h_userids: return only courses where these users are members. :param assignment_ids: return only the courses these assignments belong to. """ courses_query = ( self._search_query( - organization_ids=[organization.id] if organization else None, + organization_ids=organization_ids, h_userids=h_userids, limit=None, ) diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 7277732e42..1166352a7b 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -1,4 +1,5 @@ from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized +from sqlalchemy import select from lms.models.dashboard_admin import DashboardAdmin from lms.models.organization import Organization @@ -47,22 +48,39 @@ def get_request_course(self, request): return course - def get_request_organization(self, request): - """Get and authorize an organization for the given request.""" - organization = self._organization_service.get_by_public_id( - public_id=request.matchdict["organization_public_id"] - ) - if not organization: - raise HTTPNotFound() + def get_request_organizations(self, request) -> list[Organization]: + """Get the relevant organizations for the current requests.""" + organizations = [] - if request.has_permission(Permissions.STAFF): - # STAFF members in our admin pages can access all organizations - return organization + # If we have an user, include the organization it belongs to + if request.lti_user: + lti_user_organization = request.lti_user.application_instance.organization + + if not self._organization_service.is_member( + lti_user_organization, request.user + ): + raise HTTPUnauthorized() - if not self._organization_service.is_member(organization, request.user): + organizations.append(lti_user_organization) + + # Include any other organizations we are an admin in + admin_organizations = self.get_organizations_by_admin_email( + request.lti_user.email if request.lti_user else request.identity.userid + ) + organizations.extend(admin_organizations) + + if not organizations: raise HTTPUnauthorized() - return organization + return organizations + + def get_organizations_by_admin_email(self, email: str) -> list[Organization]: + return self._db.scalars( + select(Organization) + .join(DashboardAdmin) + .where(DashboardAdmin.email == email) + .distinct() + ).all() def add_dashboard_admin( self, organization: Organization, email: str, created_by: str diff --git a/lms/static/scripts/frontend_apps/api-types.ts b/lms/static/scripts/frontend_apps/api-types.ts index 2e6f5e2e81..1506d35e78 100644 --- a/lms/static/scripts/frontend_apps/api-types.ts +++ b/lms/static/scripts/frontend_apps/api-types.ts @@ -171,7 +171,7 @@ export type CourseWithMetrics = Course & { }; /** - * Response for `/api/dashboard/organizations/{organization_public_id}` call. + * Response for `/api/dashboard/course/metrics` call. */ export type CoursesResponse = { courses: CourseWithMetrics[]; diff --git a/lms/static/scripts/frontend_apps/components/AppRoot.tsx b/lms/static/scripts/frontend_apps/components/AppRoot.tsx index e65cc54e2e..662eecf6a1 100644 --- a/lms/static/scripts/frontend_apps/components/AppRoot.tsx +++ b/lms/static/scripts/frontend_apps/components/AppRoot.tsx @@ -41,7 +41,7 @@ export default function AppRoot({ initialConfig, services }: AppRootProps) { - + diff --git a/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx b/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx index 988ed4d21b..94f28c8d70 100644 --- a/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx +++ b/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx @@ -1,6 +1,6 @@ import { Link } from '@hypothesis/frontend-shared'; import { useMemo, useState } from 'preact/hooks'; -import { Link as RouterLink, useParams } from 'wouter-preact'; +import { Link as RouterLink } from 'wouter-preact'; import type { Assignment, @@ -10,7 +10,6 @@ import type { } from '../../api-types'; import { useConfig } from '../../config'; import { urlPath, useAPIFetch } from '../../utils/api'; -import { replaceURLParams } from '../../utils/url'; import DashboardActivityFilters from './DashboardActivityFilters'; import FormattedDate from './FormattedDate'; import OrderableActivityTable from './OrderableActivityTable'; @@ -30,9 +29,6 @@ const courseURL = (id: number) => urlPath`/courses/${String(id)}`; export default function OrganizationActivity() { const { dashboard } = useConfig(['dashboard']); const { routes } = dashboard; - const { organizationPublicId } = useParams<{ - organizationPublicId: string; - }>(); const [selectedStudents, setSelectedStudents] = useState([]); const [selectedAssignments, setSelectedAssignments] = useState( @@ -52,16 +48,11 @@ export default function OrganizationActivity() { [selectedCourses], ); - const courses = useAPIFetch( - replaceURLParams(routes.organization_courses, { - organization_public_id: organizationPublicId, - }), - { - h_userid: studentIds, - assignment_id: assignmentIds, - course_id: courseIds, - }, - ); + const courses = useAPIFetch(routes.courses_metrics, { + h_userid: studentIds, + assignment_id: assignmentIds, + course_id: courseIds, + }); const rows: CoursesTableRow[] = useMemo( () => courses.data?.courses.map(({ id, title, course_metrics }) => ({ diff --git a/lms/static/scripts/frontend_apps/components/dashboard/test/OrganizationActivity-test.js b/lms/static/scripts/frontend_apps/components/dashboard/test/OrganizationActivity-test.js index 5c73476577..4365982349 100644 --- a/lms/static/scripts/frontend_apps/components/dashboard/test/OrganizationActivity-test.js +++ b/lms/static/scripts/frontend_apps/components/dashboard/test/OrganizationActivity-test.js @@ -41,8 +41,8 @@ describe('OrganizationActivity', () => { fakeConfig = { dashboard: { routes: { - organization_courses: - '/api/dashboard/organizations/:organization_public_id', + courses_metrics: + '/api/courses/metrics', }, }, }; diff --git a/lms/static/scripts/frontend_apps/config.ts b/lms/static/scripts/frontend_apps/config.ts index b31a90741c..60027f463e 100644 --- a/lms/static/scripts/frontend_apps/config.ts +++ b/lms/static/scripts/frontend_apps/config.ts @@ -260,7 +260,7 @@ export type DashboardRoutes = { course_assignments_metrics: string; /** Fetch courses stats */ - organization_courses: string; + courses_metrics: string; /** Fetch list of courses */ courses: string; diff --git a/lms/views/admin/assignment.py b/lms/views/admin/assignment.py index 613c311fb3..c6e2434de5 100644 --- a/lms/views/admin/assignment.py +++ b/lms/views/admin/assignment.py @@ -52,11 +52,7 @@ def assignment_dashboard(self): response = HTTPFound( location=self.request.route_url( - "dashboard.assignment", - public_id=assignment.groupings[ - 0 - ].application_instance.organization.public_id, # type: ignore - assignment_id=assignment.id, + "dashboard.assignment", assignment_id=assignment.id ), ) return response diff --git a/lms/views/dashboard/api/course.py b/lms/views/dashboard/api/course.py index 898ce85b43..035fd4ea31 100644 --- a/lms/views/dashboard/api/course.py +++ b/lms/views/dashboard/api/course.py @@ -55,7 +55,10 @@ def courses(self) -> APICourses: filter_by_h_userids = self.request.parsed_params.get("h_userids") filter_by_assignment_ids = self.request.parsed_params.get("assignment_ids") + organizations = self.dashboard_service.get_request_organizations(self.request) + courses = self.course_service.get_courses( + organization_ids=[org.id for org in organizations], instructor_h_userid=self.request.user.h_userid if self.request.user else None, @@ -73,19 +76,19 @@ def courses(self) -> APICourses: } @view_config( - route_name="api.dashboard.organizations.courses", + route_name="api.dashboard.courses.metrics", request_method="GET", renderer="json", permission=Permissions.DASHBOARD_VIEW, schema=CoursesMetricsSchema, ) - def organization_courses(self) -> APICourses: + def courses_metrics(self) -> APICourses: filter_by_h_userids = self.request.parsed_params.get("h_userids") filter_by_assignment_ids = self.request.parsed_params.get("assignment_ids") filter_by_course_ids = self.request.parsed_params.get("course_ids") - org = self.dashboard_service.get_request_organization(self.request) + organizations = self.dashboard_service.get_request_organizations(self.request) courses_query = self.course_service.get_courses( - organization=org, + organization_ids=[org.id for org in organizations], instructor_h_userid=self.request.user.h_userid if self.request.user else None, diff --git a/lms/views/dashboard/views.py b/lms/views/dashboard/views.py index e66224396d..05fda90372 100644 --- a/lms/views/dashboard/views.py +++ b/lms/views/dashboard/views.py @@ -22,7 +22,7 @@ renderer="lms:templates/dashboard/forbidden.html.jinja2", ) @forbidden_view_config( - route_name="dashboard.organization", + route_name="dashboard", request_method="GET", renderer="lms:templates/dashboard/forbidden.html.jinja2", ) @@ -54,9 +54,7 @@ def assignment_redirect_from_launch(self): assignment_id = self.request.matchdict["assignment_id"] response = HTTPFound( location=self.request.route_url( - "dashboard.assignment", - public_id=self.request.lti_user.application_instance.organization.public_id, - assignment_id=assignment_id, + "dashboard.assignment", assignment_id=assignment_id ), ) self._set_lti_user_cookie(response) @@ -95,18 +93,18 @@ def course_show(self): return {"title": course.lms_name} @view_config( - route_name="dashboard.organization", + route_name="dashboard", permission=Permissions.DASHBOARD_VIEW, request_method="GET", renderer="lms:templates/dashboard/index.html.jinja2", ) - def organization_show(self): + def courses(self): """Start the dashboard miniapp in the frontend scoped to an organization. Authenticated via the LTIUser present in a cookie making this endpoint accessible directly in the browser. """ # Just get the org for the side effect of checking permissions. - _ = self.dashboard_service.get_request_organization(self.request) + _ = self.dashboard_service.get_request_organizations(self.request) self.request.context.js_config.enable_dashboard_mode() self._set_lti_user_cookie(self.request.response) # Org names are not 100% ready for public consumption, let's hardcode a title for now. @@ -128,8 +126,8 @@ def _set_lti_user_cookie(self, response): value=auth_token, secure=not self.request.registry.settings["dev"], httponly=True, - # Scope the cookie to all the org endpoints - path=f"/dashboard/organizations/{lti_user.application_instance.organization.public_id}", + # Scope the cookie to all dashboard views + path="/dashboard", max_age=60 * 60 * 24, # 24 hours, matches the lifetime of the auth_token ) return response diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index 17aedccd22..d1d71bcfe8 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -706,7 +706,7 @@ def test_it(self, js_config, lti_user): "students_metrics": "/api/dashboard/students/metrics", "course": "/api/dashboard/courses/:course_id", "course_assignments_metrics": "/api/dashboard/courses/:course_id/assignments/metrics", - "organization_courses": "/api/dashboard/organizations/:organization_public_id/courses", + "courses_metrics": "/api/dashboard/courses/metrics", "courses": "/api/dashboard/courses", "assignments": "/api/dashboard/assignments", "students": "/api/dashboard/students", diff --git a/tests/unit/lms/services/course_test.py b/tests/unit/lms/services/course_test.py index 1cc8b4c04b..53ce8efa57 100644 --- a/tests/unit/lms/services/course_test.py +++ b/tests/unit/lms/services/course_test.py @@ -344,10 +344,8 @@ def test_is_member(self, svc, db_session): assert svc.is_member(course, user.h_userid) assert not svc.is_member(course, other_user.h_userid) - def test_get_courses_deduplicates(self, db_session, svc): - org = factories.Organization() - - ai = factories.ApplicationInstance(organization=org) + def test_get_courses_deduplicates(self, db_session, svc, application_instance): + org = application_instance.organization other_ai = factories.ApplicationInstance(organization=org) older_course = factories.Course( @@ -357,7 +355,7 @@ def test_get_courses_deduplicates(self, db_session, svc): ) # Most recent group, same authority_provided_id, more recent update date course = factories.Course( - application_instance=ai, + application_instance=application_instance, updated=date(2022, 1, 1), authority_provided_id="COURSE", ) @@ -367,12 +365,14 @@ def test_get_courses_deduplicates(self, db_session, svc): # But organization deduplicate, We only get the most recent course assert db_session.scalars( - svc.get_courses(organization=org, instructor_h_userid=None) + svc.get_courses(organization_ids=[org.id], instructor_h_userid=None) ).all() == [course] - def test_get_courses_by_instructor_h_userid(self, svc, db_session): + def test_get_courses_by_instructor_h_userid( + self, svc, db_session, application_instance + ): factories.User() # User not in course - course = factories.Course() + course = factories.Course(application_instance=application_instance) assignment = factories.Assignment() user = factories.User() lti_role = factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.INSTRUCTOR) @@ -388,11 +388,14 @@ def test_get_courses_by_instructor_h_userid(self, svc, db_session): db_session.flush() assert db_session.scalars( - svc.get_courses(instructor_h_userid=user.h_userid) + svc.get_courses( + organization_ids=[application_instance.organization.id], + instructor_h_userid=user.h_userid, + ) ).all() == [course] - def test_get_courses_by_assignment_ids(self, svc, db_session): - course = factories.Course() + def test_get_courses_by_assignment_ids(self, svc, db_session, application_instance): + course = factories.Course(application_instance=application_instance) assignment = factories.Assignment() user = factories.User() factories.AssignmentMembership.create( @@ -403,7 +406,10 @@ def test_get_courses_by_assignment_ids(self, svc, db_session): db_session.flush() assert db_session.scalars( - svc.get_courses(assignment_ids=[assignment.id]) + svc.get_courses( + organization_ids=[application_instance.organization.id], + assignment_ids=[assignment.id], + ) ).all() == [course] @pytest.fixture diff --git a/tests/unit/lms/services/dashboard_test.py b/tests/unit/lms/services/dashboard_test.py index 35ab8a0b00..86a4412d70 100644 --- a/tests/unit/lms/services/dashboard_test.py +++ b/tests/unit/lms/services/dashboard_test.py @@ -4,6 +4,7 @@ from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized from lms.models.dashboard_admin import DashboardAdmin +from lms.security import Identity from lms.services.dashboard import DashboardService, factory from tests import factories @@ -88,41 +89,39 @@ def test_get_request_course(self, pyramid_request, course_service, svc): assert svc.get_request_course(pyramid_request) - def test_get_request_organization_404( - self, pyramid_request, organization_service, svc - ): - pyramid_request.matchdict["organization_public_id"] = sentinel.id - organization_service.get_by_public_id.return_value = None - - with pytest.raises(HTTPNotFound): - svc.get_request_organization(pyramid_request) - - def test_get_request_organization_403( + def test_get_request_organizations_403( self, pyramid_request, organization_service, svc, ): - pyramid_request.matchdict["organization_public_id"] = sentinel.id organization_service.is_member.return_value = False with pytest.raises(HTTPUnauthorized): - svc.get_request_organization(pyramid_request) + svc.get_request_organizations(pyramid_request) - def test_get_request_organization_for_staff( - self, pyramid_request, organization_service, pyramid_config, svc + def test_get_request_organizations_for_staff( + self, pyramid_request, pyramid_config, svc, db_session ): - pyramid_config.testing_securitypolicy(permissive=True) - pyramid_request.matchdict["organization_public_id"] = sentinel.id - organization_service.is_member.return_value = False - - assert svc.get_request_organization(pyramid_request) + org = factories.Organization() + svc.add_dashboard_admin(org, "test@example.com", "creator") + db_session.flush() + pyramid_request.lti_user = None + pyramid_config.testing_securitypolicy( + permissive=False, + identity=Identity(userid="test@example.com", permissions=[]), + ) + assert svc.get_request_organizations(pyramid_request) == [org] - def test_get_request_organization(self, pyramid_request, organization_service, svc): - pyramid_request.matchdict["organization_public_id"] = sentinel.id + def test_get_request_organizations( + self, pyramid_request, organization_service, svc, lti_user + ): + pyramid_request.lti_user = lti_user organization_service.is_member.return_value = True - assert svc.get_request_organization(pyramid_request) + assert svc.get_request_organizations(pyramid_request) == [ + pyramid_request.lti_user.application_instance.organization + ] def test_add_dashboard_admin(self, svc, db_session): admin = svc.add_dashboard_admin( diff --git a/tests/unit/lms/views/admin/assignment_test.py b/tests/unit/lms/views/admin/assignment_test.py index da03d31d69..7f81a76afe 100644 --- a/tests/unit/lms/views/admin/assignment_test.py +++ b/tests/unit/lms/views/admin/assignment_test.py @@ -29,13 +29,7 @@ def test_show_not_found(self, pyramid_request, assignment_service, views): views.show() def test_assignment_dashboard( - self, - pyramid_request, - assignment_service, - views, - AuditTrailEvent, - assignment, - organization, + self, pyramid_request, assignment_service, views, AuditTrailEvent, assignment ): pyramid_request.matchdict["id_"] = sentinel.id assignment_service.get_by_id.return_value = assignment @@ -57,7 +51,7 @@ def test_assignment_dashboard( pyramid_request.registry.notify.has_call_with(AuditTrailEvent.return_value) assert response == Any.instance_of(HTTPFound).with_attrs( { - "location": f"http://example.com/dashboard/organizations/{organization.public_id}/assignments/{assignment.id}", + "location": f"http://example.com/dashboard/assignments/{assignment.id}", } ) diff --git a/tests/unit/lms/views/admin/course_test.py b/tests/unit/lms/views/admin/course_test.py index cea54f4a0d..4b2597ca23 100644 --- a/tests/unit/lms/views/admin/course_test.py +++ b/tests/unit/lms/views/admin/course_test.py @@ -31,13 +31,7 @@ def test_show_not_found(self, pyramid_request, course_service, views): views.show() def test_course_dashboard( - self, - pyramid_request, - course_service, - views, - AuditTrailEvent, - organization, - course, + self, pyramid_request, course_service, views, AuditTrailEvent, course ): pyramid_request.matchdict["id_"] = sentinel.id course_service.get_by_id.return_value = course @@ -59,7 +53,7 @@ def test_course_dashboard( pyramid_request.registry.notify.has_call_with(AuditTrailEvent.return_value) assert response == Any.instance_of(HTTPFound).with_attrs( { - "location": f"http://example.com/dashboard/organizations/{organization.public_id}/courses/{course.id}", + "location": f"http://example.com/dashboard/courses/{course.id}", } ) diff --git a/tests/unit/lms/views/dashboard/api/course_test.py b/tests/unit/lms/views/dashboard/api/course_test.py index 4f8ca38fee..eecda68f9e 100644 --- a/tests/unit/lms/views/dashboard/api/course_test.py +++ b/tests/unit/lms/views/dashboard/api/course_test.py @@ -17,17 +17,22 @@ class TestCourseViews: - def test_get_courses(self, course_service, pyramid_request, views, get_page): + def test_get_courses( + self, course_service, pyramid_request, views, get_page, dashboard_service + ): + org = factories.Organization() courses = factories.Course.create_batch(5) get_page.return_value = courses, sentinel.pagination pyramid_request.parsed_params = { "h_userids": sentinel.h_userids, "assignment_ids": sentinel.assignment_ids, } + dashboard_service.get_request_organizations.return_value = [org] response = views.courses() course_service.get_courses.assert_called_once_with( + organization_ids=[org.id], instructor_h_userid=pyramid_request.user.h_userid, h_userids=sentinel.h_userids, assignment_ids=sentinel.assignment_ids, @@ -42,7 +47,7 @@ def test_get_courses(self, course_service, pyramid_request, views, get_page): "pagination": sentinel.pagination, } - def test_get_organization_courses( + def test_course_metrics( self, course_service, pyramid_request, @@ -53,7 +58,7 @@ def test_get_organization_courses( ): org = factories.Organization() courses = factories.Course.create_batch(5) - dashboard_service.get_request_organization.return_value = org + dashboard_service.get_request_organizations.return_value = [org] course_service.get_courses.return_value = select(Course).order_by(Course.id) pyramid_request.matchdict["organization_public_id"] = sentinel.public_id pyramid_request.parsed_params = { @@ -62,13 +67,13 @@ def test_get_organization_courses( } db_session.flush() - response = views.organization_courses() + response = views.courses_metrics() - dashboard_service.get_request_organization.assert_called_once_with( + dashboard_service.get_request_organizations.assert_called_once_with( pyramid_request ) course_service.get_courses.assert_called_once_with( - organization=org, + organization_ids=[org.id], instructor_h_userid=pyramid_request.user.h_userid, h_userids=sentinel.h_userids, assignment_ids=sentinel.assignment_ids, @@ -91,7 +96,7 @@ def test_get_organization_courses( ] } - def test_get_organization_courses_filter_by_courses( + def test_courses_metrics_by_courses( self, course_service, pyramid_request, @@ -102,19 +107,19 @@ def test_get_organization_courses_filter_by_courses( ): org = factories.Organization() courses = factories.Course.create_batch(5) - dashboard_service.get_request_organization.return_value = org + dashboard_service.get_request_organizations.return_value = [org] course_service.get_courses.return_value = select(Course).order_by(Course.id) pyramid_request.matchdict["organization_public_id"] = sentinel.public_id db_session.flush() pyramid_request.parsed_params = {"course_ids": [courses[0].id]} - response = views.organization_courses() + response = views.courses_metrics() - dashboard_service.get_request_organization.assert_called_once_with( + dashboard_service.get_request_organizations.assert_called_once_with( pyramid_request ) course_service.get_courses.assert_called_once_with( - organization=org, + organization_ids=[org.id], instructor_h_userid=pyramid_request.user.h_userid, h_userids=None, assignment_ids=None, diff --git a/tests/unit/lms/views/dashboard/views_test.py b/tests/unit/lms/views/dashboard/views_test.py index 6f0f5b0b13..435e263dad 100644 --- a/tests/unit/lms/views/dashboard/views_test.py +++ b/tests/unit/lms/views/dashboard/views_test.py @@ -21,7 +21,7 @@ class TestDashboardViews: @freeze_time("2024-04-01 12:00:00") def test_assignment_redirect_from_launch( - self, views, pyramid_request, BearerTokenSchema, organization + self, views, pyramid_request, BearerTokenSchema ): pyramid_request.matchdict["assignment_id"] = sentinel.id @@ -31,24 +31,16 @@ def test_assignment_redirect_from_launch( pyramid_request.lti_user ) assert response == Any.instance_of(HTTPFound).with_attrs( - { - "location": f"http://example.com/dashboard/organizations/{organization.public_id}/assignments/sentinel.id", - } + {"location": "http://example.com/dashboard/assignments/sentinel.id"} ) assert ( response.headers["Set-Cookie"] - == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organizations/{organization.public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == "authorization=TOKEN; Max-Age=86400; Path=/dashboard; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" ) @freeze_time("2024-04-01 12:00:00") @pytest.mark.usefixtures("BearerTokenSchema") - def test_assignment_show( - self, - views, - pyramid_request, - organization, - dashboard_service, - ): + def test_assignment_show(self, views, pyramid_request, dashboard_service): context = DashboardResource(pyramid_request) context.js_config = create_autospec(JSConfig, spec_set=True, instance=True) pyramid_request.context = context @@ -62,12 +54,12 @@ def test_assignment_show( pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once() assert ( pyramid_request.response.headers["Set-Cookie"] - == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organizations/{organization.public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == "authorization=TOKEN; Max-Age=86400; Path=/dashboard; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" ) @freeze_time("2024-04-01 12:00:00") @pytest.mark.usefixtures("BearerTokenSchema") - def test_course_show(self, views, pyramid_request, organization, dashboard_service): + def test_course_show(self, views, pyramid_request, dashboard_service): context = DashboardResource(pyramid_request) context.js_config = create_autospec(JSConfig, spec_set=True, instance=True) pyramid_request.context = context @@ -79,27 +71,25 @@ def test_course_show(self, views, pyramid_request, organization, dashboard_servi pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once() assert ( pyramid_request.response.headers["Set-Cookie"] - == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organizations/{organization.public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == "authorization=TOKEN; Max-Age=86400; Path=/dashboard; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" ) @freeze_time("2024-04-01 12:00:00") @pytest.mark.usefixtures("BearerTokenSchema") - def test_organization_show( - self, views, pyramid_request, organization, dashboard_service - ): + def test_organization_show(self, views, pyramid_request, dashboard_service): context = DashboardResource(pyramid_request) context.js_config = create_autospec(JSConfig, spec_set=True, instance=True) pyramid_request.context = context - views.organization_show() + views.courses() - dashboard_service.get_request_organization.assert_called_once_with( + dashboard_service.get_request_organizations.assert_called_once_with( pyramid_request ) pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once() assert ( pyramid_request.response.headers["Set-Cookie"] - == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organizations/{organization.public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == "authorization=TOKEN; Max-Age=86400; Path=/dashboard; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" ) def test_assignment_show_with_no_lti_user(