From aedcd7910db55c9a2b924313cbfcadd168fa10a6 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 9 Jun 2022 03:49:58 +1000 Subject: [PATCH] refactor: deprecate `hostname` attribute from `ModuleSystem` (#30308) The hostname constructor argument of the XModule ModuleSystem is deprecated in favour of directly accessing the LMS_BASE value from django.conf.settings. --- common/lib/xmodule/xmodule/lti_module.py | 3 +- common/lib/xmodule/xmodule/tests/__init__.py | 1 - .../xmodule/xmodule/tests/test_lti_unit.py | 11 +++++-- common/lib/xmodule/xmodule/x_module.py | 31 +++++++++++++++---- lms/djangoapps/courseware/module_render.py | 1 - .../courseware/tests/test_lti_integration.py | 2 +- 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index a2174f1f67..583cf54d65 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -65,6 +65,7 @@ from urllib import parse import bleach import oauthlib.oauth1 +from django.conf import settings from lxml import etree from oauthlib.oauth1.rfc5849 import signature from pkg_resources import resource_string @@ -584,7 +585,7 @@ class LTIBlock( i4x-2-3-lti-31de800015cf4afb973356dbe81496df this part of resource_link_id: makes resource_link_id to be unique among courses inside same system. """ - return str(parse.quote(f"{self.system.hostname}-{self.location.html_id()}")) # lint-amnesty, pylint: disable=line-too-long + return str(parse.quote(f"{settings.LMS_BASE}-{self.location.html_id()}")) def get_lis_result_sourcedid(self): """ diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 0925073099..6b5f903553 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -153,7 +153,6 @@ def get_test_system( static_url='/static', track_function=Mock(name='get_test_system.track_function'), get_module=get_module, - hostname="edx.org", services={ 'user': user_service, 'mako': mako_service, diff --git a/common/lib/xmodule/xmodule/tests/test_lti_unit.py b/common/lib/xmodule/xmodule/tests/test_lti_unit.py index 103cdaeb53..cfda18a62f 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti_unit.py @@ -3,12 +3,14 @@ import datetime import textwrap -import unittest from copy import copy from unittest.mock import Mock, PropertyMock, patch from urllib import parse + import pytest +from django.conf import settings +from django.test import TestCase, override_settings from lxml import etree from opaque_keys.edx.locator import BlockUsageLocator from pytz import UTC @@ -16,6 +18,7 @@ from webob.request import Request from xblock.field_data import DictFieldData from xblock.fields import ScopeIds + from common.djangoapps.xblock_django.constants import ATTR_KEY_ANONYMOUS_USER_ID from xmodule.fields import Timedelta from xmodule.lti_2_util import LTIError @@ -25,7 +28,8 @@ from xmodule.tests.helpers import StubUserService from . import get_test_system -class LTIBlockTest(unittest.TestCase): +@override_settings(LMS_BASE="edx.org") +class LTIBlockTest(TestCase): """Logic tests for LTI module.""" def setUp(self): @@ -69,8 +73,9 @@ class LTIBlockTest(unittest.TestCase): current_user = self.system.service(self.xmodule, 'user').get_current_user() self.user_id = current_user.opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) self.lti_id = self.xmodule.lti_id + self.unquoted_resource_link_id = '{}-i4x-2-3-lti-31de800015cf4afb973356dbe81496df'.format( - self.xmodule.runtime.hostname + settings.LMS_BASE ) sourced_id = ':'.join(parse.quote(i) for i in (self.lti_id, self.unquoted_resource_link_id, self.user_id)) # lint-amnesty, pylint: disable=line-too-long diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ef5072c9bb..7ec451ce54 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1621,6 +1621,19 @@ class ModuleSystemShim: DeprecationWarning, stacklevel=3 ) + @property + def hostname(self): + """ + Hostname of the site as set in the Django settings `LMS_BASE` + Deprecated in favour of direct import of `django.conf.settings` + """ + warnings.warn( + 'runtime.hostname is deprecated. Please use `LMS_BASE` from `django.conf.settings`.', + DeprecationWarning, stacklevel=3, + ) + from django.conf import settings + return settings.LMS_BASE + class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime): """ @@ -1636,11 +1649,18 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, """ def __init__( - self, static_url, track_function, get_module, - descriptor_runtime, hostname="", publish=None, - course_id=None, error_descriptor_class=None, - field_data=None, rebind_noauth_module_to_user=None, - **kwargs): + self, + static_url, + track_function, + get_module, + descriptor_runtime, + publish=None, + course_id=None, + error_descriptor_class=None, + field_data=None, + rebind_noauth_module_to_user=None, + **kwargs, + ): """ Create a closure around the system environment. @@ -1678,7 +1698,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, self.STATIC_URL = static_url self.track_function = track_function self.get_module = get_module - self.HOSTNAME = self.hostname = hostname self.course_id = course_id if publish: diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 9b1e7bc618..6345011358 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -729,7 +729,6 @@ def get_module_system_for_user( static_url=settings.STATIC_URL, get_module=inner_get_module, user=user, - hostname=settings.SITE_NAME, publish=publish, course_id=course_id, # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index c4a2d8a7db..da4805774e 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -43,7 +43,7 @@ class TestLTI(BaseTestXmodule): context_id = str(self.item_descriptor.course_id) user_service = self.item_descriptor.xmodule_runtime.service(self.item_descriptor, 'user') user_id = str(user_service.get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID)) - hostname = self.item_descriptor.xmodule_runtime.hostname + hostname = settings.LMS_BASE resource_link_id = str(urllib.parse.quote(f'{hostname}-{self.item_descriptor.location.html_id()}')) sourcedId = "{context}:{resource_link}:{user_id}".format(