From b61b29196b686d628b4297b7696af3fc2a41def1 Mon Sep 17 00:00:00 2001 From: Chris Deery <3932645+cdeery@users.noreply.github.com> Date: Fri, 4 Mar 2022 09:01:02 -0500 Subject: [PATCH] feat: [AA-1206] remove redundant fields from API (#29920) * feat: [AA-1206] remove redundant fields from API Part of a larger effort to clean up the MFE BFF endpoints. Remove the redundant fields username and course_access, both of which are also available in the course home metadata call. --- docs/swagger.yaml | 1 - lms/djangoapps/courseware/access_response.py | 11 ---- .../djangoapps/courseware_api/serializers.py | 2 - .../courseware_api/tests/test_views.py | 37 +++---------- .../core/djangoapps/courseware_api/views.py | 54 +++---------------- 5 files changed, 13 insertions(+), 92 deletions(-) diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 93669aca7a..bced6a24a0 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -2557,7 +2557,6 @@ paths: * pacing: Course pacing. Possible values: instructor, self * tabs: Course tabs * user_timezone: User's chosen timezone setting (or null for browser default) - * can_load_course: Whether the user can view the course (AccessResponse object) * is_staff: Whether the effective user has staff access to the course * original_user_is_staff: Whether the original user has staff access to the course * can_view_legacy_courseware: Indicates whether the user is able to see the legacy courseware view diff --git a/lms/djangoapps/courseware/access_response.py b/lms/djangoapps/courseware/access_response.py index fbb4b9373b..e88b58d205 100644 --- a/lms/djangoapps/courseware/access_response.py +++ b/lms/djangoapps/courseware/access_response.py @@ -236,14 +236,3 @@ class AuthenticationRequiredAccessError(AccessError): developer_message = "User must be authenticated to view the course" user_message = _("You must be logged in to see this course") super().__init__(error_code, developer_message, user_message) - - -class CoursewareMicrofrontendDisabledAccessError(AccessError): - """ - Access denied because the courseware micro-frontend is disabled for this user. - """ - def __init__(self): - error_code = 'microfrontend_disabled' - developer_message = 'Micro-frontend is disabled for this user' - user_message = _('Please view your course in the existing experience') - super().__init__(error_code, developer_message, user_message) diff --git a/openedx/core/djangoapps/courseware_api/serializers.py b/openedx/core/djangoapps/courseware_api/serializers.py index b1e1c24ee3..9d2357d7c2 100644 --- a/openedx/core/djangoapps/courseware_api/serializers.py +++ b/openedx/core/djangoapps/courseware_api/serializers.py @@ -109,7 +109,6 @@ class CourseInfoSerializer(serializers.Serializer): # pylint: disable=abstract- can_view_legacy_courseware = serializers.BooleanField() can_access_proctored_exams = serializers.BooleanField() is_staff = serializers.BooleanField() - course_access = serializers.DictField() notes = serializers.DictField() marketing_url = serializers.CharField() celebrations = serializers.DictField() @@ -121,7 +120,6 @@ class CourseInfoSerializer(serializers.Serializer): # pylint: disable=abstract- linkedin_add_to_profile_url = serializers.URLField() is_integrity_signature_enabled = serializers.BooleanField() user_needs_integrity_signature = serializers.BooleanField() - username = serializers.CharField() def __init__(self, *args, **kwargs): """ diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index b4a0169068..d1cc102c2d 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -129,9 +129,7 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): ) @ddt.unpack @mock.patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) - @mock.patch('openedx.core.djangoapps.courseware_api.views.CoursewareMeta.is_microfrontend_enabled_for_user') - def test_enrolled_course_metadata(self, logged_in, enrollment_mode, is_microfrontend_enabled_for_user): - is_microfrontend_enabled_for_user.return_value = True + def test_enrolled_course_metadata(self, logged_in, enrollment_mode): check_public_access = mock.Mock() check_public_access.return_value = ACCESS_DENIED with mock.patch('lms.djangoapps.courseware.access_utils.check_public_access', check_public_access): @@ -177,7 +175,7 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): if enrollment_mode == 'audit': assert response.data['verify_identity_url'] is None - assert response.data['verification_status'] == 'none' # lint-amnesty, pylint: disable=literal-comparison + assert response.data['verification_status'] == 'none' assert response.data['linkedin_add_to_profile_url'] is None else: assert response.data['certificate_data']['cert_status'] == 'earned_but_not_available' @@ -186,7 +184,7 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): ) # The response contains an absolute URL so this is only checking the path of the final assert expected_verify_identity_url in response.data['verify_identity_url'] - assert response.data['verification_status'] == 'none' # lint-amnesty, pylint: disable=literal-comparison + assert response.data['verification_status'] == 'none' request = RequestFactory().request() cert_url = get_certificate_url(course_id=self.course.id, uuid=cert.verify_uuid) @@ -215,9 +213,7 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): ) @ddt.unpack @mock.patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) - @mock.patch('openedx.core.djangoapps.courseware_api.views.CoursewareMeta.is_microfrontend_enabled_for_user') - def test_unenrolled_course_metadata(self, logged_in, enable_anonymous, is_microfrontend_enabled_for_user): - is_microfrontend_enabled_for_user.return_value = True + def test_unenrolled_course_metadata(self, logged_in, enable_anonymous): check_public_access = mock.Mock() check_public_access.return_value = enable_anonymous with mock.patch('lms.djangoapps.courseware.access_utils.check_public_access', check_public_access): @@ -229,13 +225,9 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): if enable_anonymous and not logged_in: # multiple checks use this handler - check_public_access.assert_called() assert response.data['enrollment']['mode'] is None - assert response.data['course_access']['has_access'] assert response.data['course_goals']['selected_goal'] is None assert response.data['course_goals']['weekly_learning_goal_enabled'] is False - else: - assert not response.data['course_access']['has_access'] @ddt.data( # Who has access to MFE courseware? @@ -245,7 +237,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "student", "enroll_user": True, "masquerade_role": None, - "expect_course_access": True, }, { # Un-enrolled learners should NOT have access. @@ -253,7 +244,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "student", "enroll_user": False, "masquerade_role": None, - "expect_course_access": False, }, { # Un-enrolled instructors should have access. @@ -261,7 +251,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "instructor", "enroll_user": False, "masquerade_role": None, - "expect_course_access": True, }, { # Un-enrolled instructors masquerading as students should have access. @@ -269,7 +258,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "instructor", "enroll_user": False, "masquerade_role": "student", - "expect_course_access": True, }, { # If MFE is not visible, enrolled learners shouldn't have access. @@ -277,7 +265,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "student", "enroll_user": True, "masquerade_role": None, - "expect_course_access": False, }, { # If MFE is not visible, instructors shouldn't have access. @@ -285,7 +272,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "instructor", "enroll_user": False, "masquerade_role": None, - "expect_course_access": False, }, { # If MFE is not visible, masquerading instructors shouldn't have access. @@ -293,7 +279,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): "username": "instructor", "enroll_user": False, "masquerade_role": "student", - "expect_course_access": False, }, ) @ddt.unpack @@ -303,7 +288,6 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): username: str, enroll_user: bool, masquerade_role: Optional[str], - expect_course_access: bool, ): """ Test that course_access is calculated correctly based on @@ -313,22 +297,13 @@ class CourseApiTestViews(BaseCoursewareTests, MasqueradeMixin): if enroll_user: CourseEnrollment.enroll(user, self.course.id, 'audit') - patch_mfe_visible = mock.patch( - 'openedx.core.djangoapps.courseware_api.views.courseware_mfe_is_visible', - return_value=mfe_is_visible, - ) self.client.login(username=user, password='foo') if masquerade_role: self.update_masquerade(role=masquerade_role) - with patch_mfe_visible: - response = self.client.get(self.url) + + response = self.client.get(self.url) assert response.status_code == 200 - assert response.data['username'] == masquerade_role or username - if expect_course_access: - assert response.data['course_access']['has_access'] - else: - assert not response.data['course_access']['has_access'] def test_streak_data_in_response(self): """ Test that metadata endpoint returns data for the streak celebration """ diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index f17a674c9d..e736f5716f 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -18,6 +18,11 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem +from xmodule.modulestore.search import path_to_location +from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW + from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.util.views import expose_header from lms.djangoapps.edxnotes.helpers import is_feature_enabled @@ -27,11 +32,8 @@ from lms.djangoapps.course_api.api import course_detail from lms.djangoapps.course_goals.models import UserActivity from lms.djangoapps.course_goals.api import get_course_goal from lms.djangoapps.courseware.access import has_access -from lms.djangoapps.courseware.access_response import ( - CoursewareMicrofrontendDisabledAccessError, -) + from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs -from lms.djangoapps.courseware.courses import check_course_access from lms.djangoapps.courseware.masquerade import ( is_masquerading_as_specific_student, setup_masquerade, @@ -42,7 +44,6 @@ from lms.djangoapps.courseware.module_render import get_module_by_usage_id from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.courseware.toggles import ( courseware_legacy_is_visible, - courseware_mfe_is_visible, course_exit_page_is_active, ) from lms.djangoapps.courseware.views.views import get_cert_data @@ -64,10 +65,6 @@ from common.djangoapps.student.models import ( CourseEnrollmentCelebration, LinkedInAddToProfileConfiguration ) -from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.search import path_to_location # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW # lint-amnesty, pylint: disable=wrong-import-order from .serializers import CourseInfoSerializer from .utils import serialize_upgrade_info @@ -84,16 +81,7 @@ class CoursewareMeta: username or self.request.user.username, course_key, ) - # We must compute course load access *before* setting up masquerading, - # else course staff (who are not enrolled) will not be able view - # their course from the perspective of a learner. - self.load_access = check_course_access( - self.overview, - self.request.user, - 'load', - check_if_enrolled=True, - check_if_authenticated=True, - ) + self.original_user_is_staff = has_access(self.request.user, 'staff', self.overview).has_access self.original_user_is_global_staff = self.request.user.is_staff self.course_key = course_key @@ -115,16 +103,6 @@ class CoursewareMeta: def __getattr__(self, name): return getattr(self.overview, name) - def is_microfrontend_enabled_for_user(self): - """ - Can this user see the MFE for this course? - """ - return courseware_mfe_is_visible( - course_key=self.course_key, - is_global_staff=self.original_user_is_global_staff, - is_course_staff=self.original_user_is_staff - ) - @property def enrollment(self): """ @@ -161,24 +139,6 @@ class CoursewareMeta: def license(self): return self.course.license - @property - def username(self): - return self.effective_user.username - - @property - def course_access(self) -> dict: - """ - Can the user load this course in the learning micro-frontend? - - Return a JSON-friendly access response. - """ - # Only check whether the MFE is enabled if the user would otherwise be allowed to see it - # This means that if the user was denied access, they'll see a meaningful message first if - # there is one. - if self.load_access and not self.is_microfrontend_enabled_for_user(): - return CoursewareMicrofrontendDisabledAccessError().to_json() - return self.load_access.to_json() - @property def tabs(self): """