diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 5e1beeb5c454..6cb518ffe8b7 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -207,7 +207,6 @@ def _preview_module_system(request, descriptor, field_data): wrappers=wrappers, wrappers_asides=wrappers_asides, error_descriptor_class=ErrorBlock, - get_user_role=lambda: get_user_role(request.user, course_id), # Get the raw DescriptorSystem, not the CombinedSystem descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access services={ @@ -215,7 +214,11 @@ def _preview_module_system(request, descriptor, field_data): "i18n": ModuleI18nService, 'mako': mako_service, "settings": SettingsService(), - "user": DjangoXBlockUserService(request.user, anonymous_user_id='student'), + "user": DjangoXBlockUserService( + request.user, + anonymous_user_id='student', + user_role=get_user_role(request.user, course_id), + ), "partitions": StudioPartitionService(course_id=course_id), "teams_configuration": TeamsConfigurationService(), }, diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index 3a7f29472f2f..91b77523b8f0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -230,6 +230,16 @@ def setUp(self): self.request = RequestFactory().get('/dummy-url') self.request.user = self.user self.request.session = {} + self.descriptor = ItemFactory(category="video", parent=self.course) + self.field_data = mock.Mock() + self.runtime = _preview_module_system( + self.request, + self.descriptor, + self.field_data, + ) + + def test_get_user_role(self): + assert self.runtime.get_user_role() == 'staff' @XBlock.register_temp_plugin(PureXBlock, identifier='pure') def test_render_template(self): diff --git a/common/djangoapps/xblock_django/constants.py b/common/djangoapps/xblock_django/constants.py index 3d44db7877b9..cb528f52222b 100644 --- a/common/djangoapps/xblock_django/constants.py +++ b/common/djangoapps/xblock_django/constants.py @@ -4,8 +4,10 @@ # Optional attributes stored on the XBlockUser ATTR_KEY_ANONYMOUS_USER_ID = 'edx-platform.anonymous_user_id' +ATTR_KEY_REQUEST_COUNTRY_CODE = 'edx-platform.request_country_code' ATTR_KEY_IS_AUTHENTICATED = 'edx-platform.is_authenticated' ATTR_KEY_USER_ID = 'edx-platform.user_id' ATTR_KEY_USERNAME = 'edx-platform.username' ATTR_KEY_USER_IS_STAFF = 'edx-platform.user_is_staff' ATTR_KEY_USER_PREFERENCES = 'edx-platform.user_preferences' +ATTR_KEY_USER_ROLE = 'edx-platform.user_role' diff --git a/common/djangoapps/xblock_django/tests/test_user_service.py b/common/djangoapps/xblock_django/tests/test_user_service.py index 7954a09dc1d0..33cfa9aad400 100644 --- a/common/djangoapps/xblock_django/tests/test_user_service.py +++ b/common/djangoapps/xblock_django/tests/test_user_service.py @@ -14,9 +14,11 @@ from common.djangoapps.xblock_django.user_service import ( ATTR_KEY_IS_AUTHENTICATED, ATTR_KEY_ANONYMOUS_USER_ID, + ATTR_KEY_REQUEST_COUNTRY_CODE, ATTR_KEY_USER_ID, ATTR_KEY_USER_IS_STAFF, ATTR_KEY_USER_PREFERENCES, + ATTR_KEY_USER_ROLE, ATTR_KEY_USERNAME, USER_PREFERENCES_WHITE_LIST, DjangoXBlockUserService @@ -37,15 +39,22 @@ def setUp(self): set_user_preference(self.user, 'not_white_listed', 'hidden_value') self.anon_user = AnonymousUserFactory() - def assert_is_anon_xb_user(self, xb_user): + def assert_is_anon_xb_user(self, xb_user, request_country_code): """ A set of assertions for an anonymous XBlockUser. """ assert not xb_user.opt_attrs[ATTR_KEY_IS_AUTHENTICATED] + assert xb_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] == request_country_code assert xb_user.full_name is None self.assertListEqual(xb_user.emails, []) - def assert_xblock_user_matches_django(self, xb_user, dj_user, user_is_staff=False, anonymous_user_id=None): + def assert_xblock_user_matches_django( + self, xb_user, dj_user, + user_is_staff=False, + user_role=None, + anonymous_user_id=None, + request_country_code=None, + ): """ A set of assertions for comparing a XBlockUser to a django User """ @@ -55,37 +64,49 @@ def assert_xblock_user_matches_django(self, xb_user, dj_user, user_is_staff=Fals assert xb_user.opt_attrs[ATTR_KEY_USERNAME] == dj_user.username assert xb_user.opt_attrs[ATTR_KEY_USER_ID] == dj_user.id assert xb_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] == user_is_staff + assert xb_user.opt_attrs[ATTR_KEY_USER_ROLE] == user_role assert xb_user.opt_attrs[ATTR_KEY_ANONYMOUS_USER_ID] == anonymous_user_id + assert xb_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] == request_country_code assert all((pref in USER_PREFERENCES_WHITE_LIST) for pref in xb_user.opt_attrs[ATTR_KEY_USER_PREFERENCES]) def test_convert_anon_user(self): """ Tests for convert_django_user_to_xblock_user behavior when django user is AnonymousUser. """ - django_user_service = DjangoXBlockUserService(self.anon_user) + country_code = 'UK' + django_user_service = DjangoXBlockUserService(self.anon_user, request_country_code=country_code) xb_user = django_user_service.get_current_user() assert xb_user.is_current_user - self.assert_is_anon_xb_user(xb_user) + self.assert_is_anon_xb_user(xb_user, request_country_code=country_code) @ddt.data( - (False, None), - (True, None), - (False, 'abcdef0123'), - (True, 'abcdef0123'), + (False, None, None, None), + (True, 'instructor', None, None), + (True, 'staff', None, None), + (False, 'student', 'abcdef0123', None), + (True, 'student', 'abcdef0123', 'uk'), ) @ddt.unpack - def test_convert_authenticate_user(self, user_is_staff, anonymous_user_id): + def test_convert_authenticate_user(self, user_is_staff, user_role, anonymous_user_id, request_country_code): """ Tests for convert_django_user_to_xblock_user behavior when django user is User. """ django_user_service = DjangoXBlockUserService( self.user, user_is_staff=user_is_staff, + user_role=user_role, anonymous_user_id=anonymous_user_id, + request_country_code=request_country_code, ) xb_user = django_user_service.get_current_user() assert xb_user.is_current_user - self.assert_xblock_user_matches_django(xb_user, self.user, user_is_staff, anonymous_user_id) + self.assert_xblock_user_matches_django( + xb_user, self.user, + user_is_staff, + user_role, + anonymous_user_id, + request_country_code, + ) def test_get_anonymous_user_id_returns_none_for_non_staff_users(self): """ @@ -126,6 +147,27 @@ def test_get_anonymous_user_id_returns_id_for_existing_users(self): assert anonymous_user_id == anon_user_id + def test_get_user_by_anonymous_id(self): + """ + Tests that get_user_by_anonymous_id returns the expected user. + """ + course_key = CourseKey.from_string('edX/toy/2012_Fall') + anon_user_id = anonymous_id_for_user( + user=self.user, + course_id=course_key + ) + + django_user_service = DjangoXBlockUserService(self.user) + user = django_user_service.get_user_by_anonymous_id(anon_user_id) + assert user == self.user + + def test_get_user_by_anonymous_id_not_found(self): + """ + Tests that get_user_by_anonymous_id returns None for an unassigned anonymous user id. + """ + django_user_service = DjangoXBlockUserService(self.user) + assert django_user_service.get_user_by_anonymous_id('invalid-anon-id') is None + def test_external_id(self): """ Tests that external ids differ based on type. @@ -138,3 +180,17 @@ def test_external_id(self): assert ext_id1 != ext_id2 with pytest.raises(ValueError): django_user_service.get_external_user_id('unknown') + + def test_get_user_by_anonymous_id_assume_id(self): + """ + Tests that get_user_by_anonymous_id uses the anonymous user ID given to the service if none is provided. + """ + course_key = CourseKey.from_string('edX/toy/2012_Fall') + anon_user_id = anonymous_id_for_user( + user=self.user, + course_id=course_key + ) + + django_user_service = DjangoXBlockUserService(self.user, anonymous_user_id=anon_user_id) + user = django_user_service.get_user_by_anonymous_id() + assert user == self.user diff --git a/common/djangoapps/xblock_django/user_service.py b/common/djangoapps/xblock_django/user_service.py index 19eff4d22423..e9d8e74992bd 100644 --- a/common/djangoapps/xblock_django/user_service.py +++ b/common/djangoapps/xblock_django/user_service.py @@ -9,15 +9,17 @@ from openedx.core.djangoapps.external_user_ids.models import ExternalId from openedx.core.djangoapps.user_api.preferences.api import get_user_preferences -from common.djangoapps.student.models import anonymous_id_for_user, get_user_by_username_or_email +from common.djangoapps.student.models import anonymous_id_for_user, get_user_by_username_or_email, user_by_anonymous_id from .constants import ( ATTR_KEY_ANONYMOUS_USER_ID, ATTR_KEY_IS_AUTHENTICATED, + ATTR_KEY_REQUEST_COUNTRY_CODE, ATTR_KEY_USER_ID, ATTR_KEY_USERNAME, ATTR_KEY_USER_IS_STAFF, ATTR_KEY_USER_PREFERENCES, + ATTR_KEY_USER_ROLE, ) @@ -34,12 +36,16 @@ def __init__(self, django_user, **kwargs): Args: user_is_staff(bool): optional - whether the user is staff in the course + user_role(str): optional -- user's role in the course ('staff', 'instructor', or 'student') anonymous_user_id(str): optional - anonymous_user_id for the user in the course + request_country_code(str): optional -- country code determined from the user's request IP address. """ super().__init__(**kwargs) self._django_user = django_user self._user_is_staff = kwargs.get('user_is_staff', False) + self._user_role = kwargs.get('user_role', 'student') self._anonymous_user_id = kwargs.get('anonymous_user_id', None) + self._request_country_code = kwargs.get('request_country_code', None) def get_current_user(self): """ @@ -80,6 +86,16 @@ def get_anonymous_user_id(self, username, course_id): course_id = CourseKey.from_string(course_id) return anonymous_id_for_user(user=user, course_id=course_id) + def get_user_by_anonymous_id(self, uid=None): + """ + Returns the Django User object corresponding to the given anonymous user id. + + Returns None if there is no user with the given anonymous user id. + + If no `uid` is provided, then the current anonymous user ID is used. + """ + return user_by_anonymous_id(uid or self._anonymous_user_id) + def _convert_django_user_to_xblock_user(self, django_user): """ A function that returns an XBlockUser from the current Django request.user @@ -96,9 +112,11 @@ def _convert_django_user_to_xblock_user(self, django_user): xblock_user.emails = [django_user.email] xblock_user.opt_attrs[ATTR_KEY_ANONYMOUS_USER_ID] = self._anonymous_user_id xblock_user.opt_attrs[ATTR_KEY_IS_AUTHENTICATED] = True + xblock_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] = self._request_country_code xblock_user.opt_attrs[ATTR_KEY_USER_ID] = django_user.id xblock_user.opt_attrs[ATTR_KEY_USERNAME] = django_user.username xblock_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] = self._user_is_staff + xblock_user.opt_attrs[ATTR_KEY_USER_ROLE] = self._user_role user_preferences = get_user_preferences(django_user) xblock_user.opt_attrs[ATTR_KEY_USER_PREFERENCES] = { pref: user_preferences.get(pref) @@ -107,5 +125,6 @@ def _convert_django_user_to_xblock_user(self, django_user): } else: xblock_user.opt_attrs[ATTR_KEY_IS_AUTHENTICATED] = False + xblock_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] = self._request_country_code return xblock_user diff --git a/common/lib/xmodule/xmodule/lti_2_util.py b/common/lib/xmodule/xmodule/lti_2_util.py index 82fcc424f857..17dfe680c046 100644 --- a/common/lib/xmodule/xmodule/lti_2_util.py +++ b/common/lib/xmodule/xmodule/lti_2_util.py @@ -78,7 +78,7 @@ def lti_2_0_result_rest_handler(self, request, suffix): except LTIError: return Response(status=401) # Unauthorized in this case. 401 is right - real_user = self.system.get_real_user(anon_id) + real_user = self.system.service(self, 'user').get_user_by_anonymous_id(anon_id) if not real_user: # that means we can't save to database, as we do not have real user id. msg = f"[LTI]: Real user not found against anon_id: {anon_id}" log.info(msg) diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index 29acd2b155d5..ce17fb0f11c9 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -78,7 +78,10 @@ from openedx.core.djangolib.markup import HTML, Text from xmodule.editing_module import EditingMixin -from common.djangoapps.xblock_django.constants import ATTR_KEY_ANONYMOUS_USER_ID +from common.djangoapps.xblock_django.constants import ( + ATTR_KEY_ANONYMOUS_USER_ID, + ATTR_KEY_USER_ROLE, +) from xmodule.lti_2_util import LTI20BlockMixin, LTIError from xmodule.raw_module import EmptyDataRawMixin from xmodule.util.xmodule_django import add_webpack_to_fragment @@ -626,7 +629,8 @@ def role(self): 'staff': 'Administrator', 'instructor': 'Instructor', } - return roles.get(self.system.get_user_role(), 'Student') + user_role = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_ROLE) + return roles.get(user_role, 'Student') def get_icon_class(self): """ Returns the icon class """ @@ -676,17 +680,15 @@ def oauth_params(self, custom_parameters, client_key, client_secret): # Username and email can't be sent in studio mode, because the user object is not defined. # To test functionality test in LMS - if callable(self.runtime.get_real_user): - user_id = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) - real_user_object = self.runtime.get_real_user(user_id) - try: - self.user_email = real_user_object.email # lint-amnesty, pylint: disable=attribute-defined-outside-init - except AttributeError: - self.user_email = "" # lint-amnesty, pylint: disable=attribute-defined-outside-init - try: - self.user_username = real_user_object.username # lint-amnesty, pylint: disable=attribute-defined-outside-init - except AttributeError: - self.user_username = "" # lint-amnesty, pylint: disable=attribute-defined-outside-init + real_user_object = self.runtime.service(self, 'user').get_user_by_anonymous_id() + try: + self.user_email = real_user_object.email # lint-amnesty, pylint: disable=attribute-defined-outside-init + except AttributeError: + self.user_email = "" # lint-amnesty, pylint: disable=attribute-defined-outside-init + try: + self.user_username = real_user_object.username # lint-amnesty, pylint: disable=attribute-defined-outside-init + except AttributeError: + self.user_username = "" # lint-amnesty, pylint: disable=attribute-defined-outside-init if self.ask_to_send_username and self.user_username: body["lis_person_sourcedid"] = self.user_username @@ -837,7 +839,7 @@ def grade_handler(self, request, suffix): # lint-amnesty, pylint: disable=unuse log.debug("[LTI]: " + error_message) # lint-amnesty, pylint: disable=logging-not-lazy return Response(response_xml_template.format(**failure_values), content_type="application/xml") - real_user = self.system.get_real_user(parse.unquote(sourcedId.split(':')[-1])) + real_user = self.runtime.service(self, 'user').get_user_by_anonymous_id(parse.unquote(sourcedId.split(':')[-1])) if not real_user: # that means we can't save to database, as we do not have real user id. failure_values['imsx_messageIdentifier'] = escape(imsx_messageIdentifier) failure_values['imsx_description'] = "User not found." diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 533bff5c6952..2dd1a71326d0 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -92,6 +92,7 @@ def get_test_system( course_id=CourseKey.from_string('/'.join(['org', 'course', 'run'])), user=None, user_is_staff=False, + user_location=None, render_template=None, ): """ @@ -102,10 +103,14 @@ def get_test_system( """ if not user: user = Mock(name='get_test_system.user', is_staff=False) + if not user_location: + user_location = Mock(name='get_test_system.user_location') user_service = StubUserService( user=user, anonymous_user_id='student', user_is_staff=user_is_staff, + user_role='student', + request_country_code=user_location, ) mako_service = StubMakoService(render_template=render_template) @@ -133,7 +138,6 @@ def get_module(descriptor): track_function=Mock(name='get_test_system.track_function'), get_module=get_module, replace_urls=str, - get_real_user=lambda __: user, filestore=Mock(name='get_test_system.filestore', root_path='.'), debug=True, hostname="edx.org", @@ -151,8 +155,6 @@ def get_module(descriptor): node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), course_id=course_id, error_descriptor_class=ErrorBlock, - get_user_role=Mock(name='get_test_system.get_user_role', is_staff=False), - user_location=Mock(name='get_test_system.user_location'), descriptor_runtime=descriptor_system, ) diff --git a/common/lib/xmodule/xmodule/tests/helpers.py b/common/lib/xmodule/xmodule/tests/helpers.py index 22dc86892431..f57f67fe97f7 100644 --- a/common/lib/xmodule/xmodule/tests/helpers.py +++ b/common/lib/xmodule/xmodule/tests/helpers.py @@ -4,7 +4,6 @@ import filecmp -from unittest.mock import Mock import pprint from path import Path as path @@ -61,10 +60,18 @@ class StubUserService(UserService): Stub UserService for testing the sequence module. """ - def __init__(self, user=None, user_is_staff=False, anonymous_user_id=None, **kwargs): - self.user = user or Mock(name='StubUserService.user') + def __init__(self, + user=None, + user_is_staff=False, + user_role=None, + anonymous_user_id=None, + request_country_code=None, + **kwargs): + self.user = user self.user_is_staff = user_is_staff + self.user_role = user_role self.anonymous_user_id = anonymous_user_id + self.request_country_code = request_country_code super().__init__(**kwargs) def get_current_user(self): @@ -72,12 +79,21 @@ def get_current_user(self): Implements abstract method for getting the current user. """ user = XBlockUser() - if self.user.is_authenticated: + if self.user and self.user.is_authenticated: user.opt_attrs['edx-platform.anonymous_user_id'] = self.anonymous_user_id + user.opt_attrs['edx-platform.request_country_code'] = self.request_country_code user.opt_attrs['edx-platform.user_is_staff'] = self.user_is_staff user.opt_attrs['edx-platform.user_id'] = self.user.id + user.opt_attrs['edx-platform.user_role'] = self.user_role user.opt_attrs['edx-platform.username'] = self.user.username else: user.opt_attrs['edx-platform.username'] = 'anonymous' + user.opt_attrs['edx-platform.request_country_code'] = self.request_country_code user.opt_attrs['edx-platform.is_authenticated'] = False return user + + def get_user_by_anonymous_id(self, uid=None): # pylint: disable=unused-argument + """ + Return the original user passed into the service. + """ + return self.user diff --git a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py index 7dc69c9ad3f0..aca04ea95561 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py @@ -11,6 +11,7 @@ from xmodule.lti_2_util import LTIError from xmodule.lti_module import LTIBlock +from xmodule.tests.helpers import StubUserService from . import get_test_system @@ -18,11 +19,13 @@ class LTI20RESTResultServiceTest(unittest.TestCase): """Logic tests for LTI module. LTI2.0 REST ResultService""" + USER_STANDIN = Mock() + USER_STANDIN.id = 999 + def setUp(self): super().setUp() - self.system = get_test_system() + self.system = get_test_system(user=self.USER_STANDIN) self.environ = {'wsgi.url_scheme': 'http', 'REQUEST_METHOD': 'POST'} - self.system.get_real_user = Mock() self.system.publish = Mock() self.system.rebind_noauth_module_to_user = Mock() @@ -223,14 +226,10 @@ def get_signed_lti20_mock_request(self, body, method='PUT'): mock_request.body = body return mock_request - USER_STANDIN = Mock() - USER_STANDIN.id = 999 - def setup_system_xmodule_mocks_for_lti20_request_test(self): """ Helper fn to set up mocking for lti 2.0 request test """ - self.system.get_real_user = Mock(return_value=self.USER_STANDIN) self.xmodule.max_score = Mock(return_value=1.0) self.xmodule.get_client_key_secret = Mock(return_value=('test_client_key', 'test_client_secret')) self.xmodule.verify_oauth_body_sign = Mock() @@ -371,7 +370,7 @@ def test_lti20_request_handler_bad_user(self): Test that we get a 404 when the supplied user does not exist """ self.setup_system_xmodule_mocks_for_lti20_request_test() - self.system.get_real_user = Mock(return_value=None) + self.system._services['user'] = StubUserService(user=None) # pylint: disable=protected-access mock_request = self.get_signed_lti20_mock_request(self.GOOD_JSON_PUT) response = self.xmodule.lti_2_0_result_rest_handler(mock_request, "user/abcd") assert response.status_code == 404 diff --git a/common/lib/xmodule/xmodule/tests/test_lti_unit.py b/common/lib/xmodule/xmodule/tests/test_lti_unit.py index 3f13666e98b8..103cdaeb534c 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti_unit.py @@ -20,6 +20,7 @@ from xmodule.fields import Timedelta from xmodule.lti_2_util import LTIError from xmodule.lti_module import LTIBlock +from xmodule.tests.helpers import StubUserService from . import get_test_system @@ -57,7 +58,6 @@ def setUp(self): """) self.system = get_test_system() - self.system.get_real_user = Mock() self.system.publish = Mock() self.system.rebind_noauth_module_to_user = Mock() @@ -171,9 +171,9 @@ def test_real_user_is_none(self): """ If we have no real user, we should send back failure response. """ + self.system._services['user'] = StubUserService(user=None) # pylint: disable=protected-access self.xmodule.verify_oauth_body_sign = Mock() self.xmodule.has_score = True - self.system.get_real_user = Mock(return_value=None) request = Request(self.environ) request.body = self.get_request_body() response = self.xmodule.grade_handler(request, '') diff --git a/common/lib/xmodule/xmodule/tests/test_sequence.py b/common/lib/xmodule/xmodule/tests/test_sequence.py index c51a642616b4..bf0e16fb1e2b 100644 --- a/common/lib/xmodule/xmodule/tests/test_sequence.py +++ b/common/lib/xmodule/xmodule/tests/test_sequence.py @@ -98,7 +98,7 @@ def _set_up_block(self, parent, index_in_parent): block.xmodule_runtime._services['completion'] = Mock( # pylint: disable=protected-access return_value=Mock(vertical_is_complete=Mock(return_value=True)) ) - block.xmodule_runtime._services['user'] = StubUserService() # pylint: disable=protected-access + block.xmodule_runtime._services['user'] = StubUserService(user=Mock()) # pylint: disable=protected-access block.parent = parent.location return block diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index b24e561429d7..22b06d3548f3 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -190,7 +190,7 @@ def test_render_problem_without_score(self, has_score): Test the rendering of the student and public view. """ self.module_system._services['bookmarks'] = Mock() - self.module_system._services['user'] = StubUserService() + self.module_system._services['user'] = StubUserService(user=Mock()) self.module_system._services['completion'] = StubCompletionService(enabled=True, completion_value=0) now = datetime.now(pytz.UTC) @@ -212,7 +212,7 @@ def test_render_problem_without_score(self, has_score): def test_render_access_denied_blocks(self, node_has_access_error, child_has_access_error): """ Tests access denied blocks are not rendered when hide_access_error_blocks is True """ self.module_system._services['bookmarks'] = Mock() - self.module_system._services['user'] = StubUserService() + self.module_system._services['user'] = StubUserService(user=Mock()) self.vertical.due = datetime.now(pytz.UTC) + timedelta(days=-1) self.problem_block.has_access_error = node_has_access_error self.nested_problem_block.has_access_error = child_has_access_error diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 2216c88488ca..052b6d7e9179 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -29,6 +29,7 @@ from xblock.fields import ScopeIds from xblock.runtime import KvsFieldData +from common.djangoapps.xblock_django.constants import ATTR_KEY_REQUEST_COUNTRY_CODE from openedx.core.djangoapps.video_config.models import HLSPlaybackEnabledFlag, CourseYoutubeBlockedFlag from openedx.core.djangoapps.video_pipeline.config.waffle import DEPRECATE_YOUTUBE, waffle_flags from openedx.core.lib.cache_utils import request_cached @@ -108,7 +109,7 @@ @XBlock.wants('settings', 'completion', 'i18n', 'request_cache') -@XBlock.needs('mako') +@XBlock.needs('mako', 'user') class VideoBlock( VideoFields, VideoTranscriptsMixin, VideoStudioViewHandlers, VideoStudentViewHandlers, TabsEditingMixin, EmptyDataRawMixin, XmlMixin, EditingMixin, @@ -281,7 +282,8 @@ def get_html(self, view=STUDENT_VIEW): # lint-amnesty, pylint: disable=argument # based on user locale. This exists to support cases where # we leverage a geography specific CDN, like China. default_cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get('default') - cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get(self.system.user_location, default_cdn_url) + user_location = self.runtime.service(self, 'user').get_current_user().opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] + cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get(user_location, default_cdn_url) # If we have an edx_video_id, we prefer its values over what we store # internally for download links (source, html5_sources) and the youtube @@ -335,7 +337,7 @@ def get_html(self, view=STUDENT_VIEW): # lint-amnesty, pylint: disable=argument # Video caching is disabled for Studio. User_location is always None in Studio. # CountryMiddleware disabled for Studio. if getattr(self, 'video_speed_optimizations', True) and cdn_url: - branding_info = BrandingInfoConfig.get_config().get(self.system.user_location) + branding_info = BrandingInfoConfig.get_config().get(user_location) if self.edx_video_id and edxval_api and video_status != 'external': for index, source_url in enumerate(sources): diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 056528ebd4ba..607c1b2ecf99 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -44,8 +44,10 @@ from common.djangoapps.xblock_django.constants import ( ATTR_KEY_ANONYMOUS_USER_ID, + ATTR_KEY_REQUEST_COUNTRY_CODE, ATTR_KEY_USER_ID, ATTR_KEY_USER_IS_STAFF, + ATTR_KEY_USER_ROLE, ) @@ -1809,6 +1811,59 @@ def user_is_staff(self): return self._services['user'].get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) return None + @property + def user_location(self): + """ + Returns the "country code" associated with the current user's request IP address. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.user_location is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return self._services['user'].get_current_user().opt_attrs.get(ATTR_KEY_REQUEST_COUNTRY_CODE) + return None + + @property + def get_real_user(self): + """ + Returns a function that takes `anonymous_student_id` and returns the Django User object + associated with `anonymous_student_id`. + + If no `anonymous_student_id` is provided as an argument to this function, then the user service's anonymous user + ID is used instead. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.get_real_user is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return self._services['user'].get_user_by_anonymous_id + return None + + @property + def get_user_role(self): + """ + Returns a function that returns the user's role in the course. + + Implementation is different for LMS and Studio. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.get_user_role is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return partial(self._services['user'].get_current_user().opt_attrs.get, ATTR_KEY_USER_ROLE) + @property def render_template(self): """ @@ -1846,9 +1901,9 @@ def __init__( debug=False, hostname="", xqueue=None, publish=None, node_path="", course_id=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, - replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None, - field_data=None, get_user_role=None, rebind_noauth_module_to_user=None, - user_location=None, get_python_lib_zip=None, **kwargs): + replace_jump_to_id_urls=None, error_descriptor_class=None, + field_data=None, rebind_noauth_module_to_user=None, + get_python_lib_zip=None, **kwargs): """ Create a closure around the system environment. @@ -1895,12 +1950,6 @@ def __init__( error_descriptor_class - The class to use to render XModules with errors - get_real_user - function that takes `anonymous_student_id` and returns real user_id, - associated with `anonymous_student_id`. - - get_user_role - A function that returns user role. Implementation is different - for LMS and Studio. - field_data - the `FieldData` to use for backing XBlock storage. rebind_noauth_module_to_user - rebinds module bound to AnonymousUser to a real user...used in LTI @@ -1935,10 +1984,6 @@ def __init__( self.error_descriptor_class = error_descriptor_class self.xmodule_instance = None - self.get_real_user = get_real_user - self.user_location = user_location - - self.get_user_role = get_user_role self.descriptor_runtime = descriptor_runtime self.rebind_noauth_module_to_user = rebind_noauth_module_to_user diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 6929cea8a8c3..00ffa2a9b8cb 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -84,7 +84,7 @@ from openedx.features.course_duration_limits.access import course_expiration_wrapper from openedx.features.discounts.utils import offer_banner_wrapper from openedx.features.content_type_gating.services import ContentTypeGatingService -from common.djangoapps.student.models import anonymous_id_for_user, user_by_anonymous_id +from common.djangoapps.student.models import anonymous_id_for_user from common.djangoapps.student.roles import CourseBetaTesterRole from common.djangoapps.track import contexts from common.djangoapps.util import milestones_helpers @@ -553,7 +553,9 @@ def get_event_handler(event_type): user_service = DjangoXBlockUserService( user, user_is_staff=user_is_staff, + user_role=get_user_role(user, course_id), anonymous_user_id=anonymous_student_id, + request_country_code=user_location, ) def publish(block, event_type, event): @@ -806,7 +808,6 @@ def rebind_noauth_module_to_user(module, real_user): # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) mixins=descriptor.runtime.mixologist._mixins, # pylint: disable=protected-access wrappers=block_wrappers, - get_real_user=user_by_anonymous_id, services={ 'fs': FSService(), 'field-data': field_data, @@ -822,10 +823,8 @@ def rebind_noauth_module_to_user(module, real_user): 'user_state': UserStateService(), 'content_type_gating': ContentTypeGatingService(), }, - get_user_role=lambda: get_user_role(user, course_id), descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access rebind_noauth_module_to_user=rebind_noauth_module_to_user, - user_location=user_location, request_token=request_token, ) diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index feb57dd065ef..e92326934a8a 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -64,18 +64,18 @@ class BaseTestXmodule(ModuleStoreTestCase): METADATA = {} MODEL_DATA = {'data': ''} - def new_module_runtime(self, render_template=None): + def new_module_runtime(self, **kwargs): """ Generate a new ModuleSystem that is minimally set up for testing """ - return get_test_system(course_id=self.course.id, render_template=render_template) + return get_test_system(course_id=self.course.id, **kwargs) - def new_descriptor_runtime(self): - runtime = get_test_descriptor_system() + def new_descriptor_runtime(self, **kwargs): + runtime = get_test_descriptor_system(**kwargs) runtime.get_block = modulestore().get_item return runtime - def initialize_module(self, **kwargs): # lint-amnesty, pylint: disable=missing-function-docstring + def initialize_module(self, runtime_kwargs=None, **kwargs): # lint-amnesty, pylint: disable=missing-function-docstring kwargs.update({ 'parent_location': self.section.location, 'category': self.CATEGORY @@ -90,7 +90,9 @@ def initialize_module(self, **kwargs): # lint-amnesty, pylint: disable=missing- student_data = DictFieldData(field_data) self.item_descriptor._field_data = LmsFieldData(self.item_descriptor._field_data, student_data) # lint-amnesty, pylint: disable=protected-access - self.item_descriptor.xmodule_runtime = self.new_module_runtime() + if runtime_kwargs is None: + runtime_kwargs = {} + self.item_descriptor.xmodule_runtime = self.new_module_runtime(**runtime_kwargs) self.item_url = str(self.item_descriptor.location) @@ -144,13 +146,13 @@ def get_url(self, dispatch): class XModuleRenderingTestBase(BaseTestXmodule): # lint-amnesty, pylint: disable=missing-class-docstring - def new_module_runtime(self, render_template=None): + def new_module_runtime(self, **kwargs): """ Create a runtime that actually does html rendering """ - if not render_template: - render_template = render_to_string - runtime = super().new_module_runtime(render_template=render_template) + if 'render_template' not in kwargs: + kwargs['render_template'] = render_to_string + runtime = super().new_module_runtime(**kwargs) runtime.modulestore = Mock() return runtime diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 5f76c0580328..bdbf449a12ad 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -2624,6 +2624,21 @@ def test_user_is_staff(self): course=self.course, ) assert runtime.user_is_staff + assert runtime.get_user_role() == 'student' + + @patch('lms.djangoapps.courseware.module_render.get_user_role', Mock(return_value='instructor', autospec=True)) + def test_get_user_role(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + assert runtime.get_user_role() == 'instructor' def test_anonymous_student_id(self): runtime, _ = render.get_module_system_for_user( @@ -2687,6 +2702,27 @@ def test_user_service_with_anonymous_user(self): assert runtime.seed == 0 assert runtime.user_id is None assert not runtime.user_is_staff + assert not runtime.get_user_role() + + def test_get_real_user(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + course_anonymous_student_id = anonymous_id_for_user(self.user, self.course.id) + assert runtime.get_real_user(course_anonymous_student_id) == self.user # pylint: disable=not-callable + + no_course_anonymous_student_id = anonymous_id_for_user(self.user, None) + assert runtime.get_real_user(no_course_anonymous_student_id) == self.user # pylint: disable=not-callable + + # Tests that the default is to use the user service's anonymous_student_id + assert runtime.get_real_user() == self.user # pylint: disable=not-callable def test_render_template(self): runtime, _ = render.get_module_system_for_user( diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index d48dd16811aa..b6724a47606f 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -34,6 +34,7 @@ from path import Path as path from waffle.testutils import override_flag +from common.djangoapps.xblock_django.constants import ATTR_KEY_REQUEST_COUNTRY_CODE from lms.djangoapps.courseware.tests.helpers import get_context_dict_from_string from openedx.core.djangoapps.video_pipeline.config.waffle import DEPRECATE_YOUTUBE, waffle_flags from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel @@ -883,8 +884,12 @@ def side_effect(*args, **kwargs): # lint-amnesty, pylint: disable=unused-argume sources=data['sources'], edx_video_id=data['edx_video_id'], ) - self.initialize_block(data=DATA) - self.item_descriptor.xmodule_runtime.user_location = 'CN' + self.initialize_block(data=DATA, runtime_kwargs={ + 'user_location': 'CN', + }) + user_service = self.item_descriptor.xmodule_runtime.service(self.item_descriptor, 'user') + user_location = user_service.get_current_user().opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] + assert user_location == 'CN' context = self.item_descriptor.render('student_view').content expected_context = dict(initial_context) expected_context['metadata'].update({ diff --git a/lms/djangoapps/edxnotes/decorators.py b/lms/djangoapps/edxnotes/decorators.py index 6c5d0ca6d933..c641bd230c66 100644 --- a/lms/djangoapps/edxnotes/decorators.py +++ b/lms/djangoapps/edxnotes/decorators.py @@ -9,7 +9,6 @@ from xblock.exceptions import NoSuchServiceError from common.djangoapps.edxmako.shortcuts import render_to_string -from common.djangoapps.xblock_django.constants import ATTR_KEY_ANONYMOUS_USER_ID def edxnotes(cls): @@ -43,8 +42,7 @@ def get_html(self, *args, **kwargs): # - the feature flag or `edxnotes` setting of the course is set to False # - the user is not authenticated try: - user_id = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) - user = self.runtime.get_real_user(user_id) + user = self.runtime.service(self, 'user').get_user_by_anonymous_id() except NoSuchServiceError: user = None diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index fc7939bbcd50..a67ba7e0fa2e 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -33,6 +33,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory # lint-amnesty, pylint: disable=wrong-import-order from xmodule.tabs import CourseTab # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.tests.helpers import StubUserService # lint-amnesty, pylint: disable=wrong-import-order from . import helpers from .decorators import edxnotes @@ -81,7 +82,8 @@ def __init__(self, course, user=None): self.system = MagicMock(is_author_mode=False) self.scope_ids = MagicMock(usage_id="test_usage_id") user = user or UserFactory() - self.runtime = MagicMock(course_id=course.id, get_real_user=lambda __: user) + user_service = StubUserService(user) + self.runtime = MagicMock(course_id=course.id, service=lambda _a, _b: user_service) self.descriptor = MagicMock() self.descriptor.runtime.modulestore.get_course.return_value = course diff --git a/lms/djangoapps/lms_xblock/test/test_runtime.py b/lms/djangoapps/lms_xblock/test/test_runtime.py index 5a21fca1f286..eed0279c7184 100644 --- a/lms/djangoapps/lms_xblock/test/test_runtime.py +++ b/lms/djangoapps/lms_xblock/test/test_runtime.py @@ -176,10 +176,6 @@ def create_runtime(self): """ Create the testing runtime. """ - def mock_get_real_user(_anon_id): - """Just returns the test user""" - return self.user - return LmsModuleSystem( static_url='/static', track_function=Mock(), @@ -187,7 +183,6 @@ def mock_get_real_user(_anon_id): replace_urls=str, course_id=self.course_id, user=self.user, - get_real_user=mock_get_real_user, descriptor_runtime=Mock(), )