From b1b3c646ad52777d76b0e6d1cc5211bdc3ba8a44 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 31 Aug 2021 16:26:44 +0930 Subject: [PATCH] refactor: fixes tests * UserStubService now takes user, user_is_staff, and anonymous_user_id * get_test_system() creates a UserStubService with an anonymous_user_id of 'student' * Removes references to deprecated ModuleSystem attributes from test code * Fixes and simplifies the ConditionalBlock tests, using get_module provided by TestModuleSystem instead of trying to mock out all the pieces. (cherry picked from commit 927016a8df7a0efffb95493245e01e12375f914b) --- common/lib/xmodule/xmodule/tests/__init__.py | 13 +++- common/lib/xmodule/xmodule/tests/helpers.py | 16 +++-- .../xmodule/xmodule/tests/test_capa_module.py | 3 +- .../xmodule/xmodule/tests/test_conditional.py | 63 +++---------------- .../xmodule/xmodule/tests/test_html_module.py | 4 +- .../xmodule/xmodule/tests/test_lti20_unit.py | 3 +- .../xmodule/xmodule/tests/test_lti_unit.py | 4 +- .../xmodule/xmodule/tests/test_vertical.py | 5 +- .../courseware/tests/test_lti_integration.py | 4 +- 9 files changed, 44 insertions(+), 71 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 2ebc9787a6..c91da247d8 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -34,8 +34,10 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.modulestore.xml import CourseLocationManager +from xmodule.tests.helpers import StubUserService from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin + MODULE_DIR = path(__file__).dirname() # Location of common test DATA directory # '../../../../edx-platform/common/test/data/' @@ -90,6 +92,7 @@ class TestModuleSystem(ModuleSystem): # pylint: disable=abstract-method def get_test_system( course_id=CourseKey.from_string('/'.join(['org', 'course', 'run'])), user=None, + user_is_staff=False, ): """ Construct a test ModuleSystem instance. @@ -105,6 +108,11 @@ def get_test_system( """ if not user: user = Mock(name='get_test_system.user', is_staff=False) + user_service = StubUserService( + user=user, + anonymous_user_id='student', + user_is_staff=user_is_staff, + ) descriptor_system = get_test_descriptor_system() @@ -130,11 +138,13 @@ def get_test_system( get_module=get_module, render_template=mock_render_template, replace_urls=str, - user=user, get_real_user=lambda __: user, filestore=Mock(name='get_test_system.filestore', root_path='.'), debug=True, hostname="edx.org", + services={ + 'user': user_service, + }, xqueue={ 'interface': None, 'callback_url': '/', @@ -143,7 +153,6 @@ def get_test_system( 'construct_callback': Mock(name='get_test_system.xqueue.construct_callback', side_effect="/"), }, node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), - anonymous_student_id='student', course_id=course_id, error_descriptor_class=ErrorBlock, get_user_role=Mock(name='get_test_system.get_user_role', is_staff=False), diff --git a/common/lib/xmodule/xmodule/tests/helpers.py b/common/lib/xmodule/xmodule/tests/helpers.py index 42156049fd..4e9e9d1d14 100644 --- a/common/lib/xmodule/xmodule/tests/helpers.py +++ b/common/lib/xmodule/xmodule/tests/helpers.py @@ -4,6 +4,7 @@ Utility methods for unit tests. import filecmp +from unittest.mock import Mock from path import Path as path from xblock.reference.user_service import UserService, XBlockUser @@ -34,8 +35,10 @@ class StubUserService(UserService): Stub UserService for testing the sequence module. """ - def __init__(self, is_anonymous=False, **kwargs): - self.is_anonymous = is_anonymous + def __init__(self, user=None, user_is_staff=False, anonymous_user_id=None, **kwargs): + self.user = user or Mock(name='StubUserService.user') + self.user_is_staff = user_is_staff + self.anonymous_user_id = anonymous_user_id super().__init__(**kwargs) def get_current_user(self): @@ -43,9 +46,12 @@ class StubUserService(UserService): Implements abstract method for getting the current user. """ user = XBlockUser() - if self.is_anonymous: + if self.user.is_authenticated: + user.opt_attrs['edx-platform.anonymous_user_id'] = self.anonymous_user_id + 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.username'] = self.user.username + else: user.opt_attrs['edx-platform.username'] = 'anonymous' user.opt_attrs['edx-platform.is_authenticated'] = False - else: - user.opt_attrs['edx-platform.username'] = 'bilbo' return user diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 30f2e5cd4b..58de4adc26 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -113,8 +113,7 @@ class CapaFactory: # since everything else is a string. field_data['attempts'] = int(attempts) - system = get_test_system(course_id=location.course_key) - system.user_is_staff = kwargs.get('user_is_staff', False) + system = get_test_system(course_id=location.course_key, user_is_staff=kwargs.get('user_is_staff', False)) system.render_template = Mock(return_value="
Test Template HTML
") module = ProblemBlock( system, diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 49c8603be3..13f2dc6a95 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -178,7 +178,6 @@ class ConditionalBlockBasicTest(unittest.TestCase): modules['cond_module'].save() modules['source_module'].is_attempted = "false" ajax = json.loads(modules['cond_module'].handle_ajax('', '')) - print("ajax: ", ajax) fragments = ajax['fragments'] assert not any(('This is a secret' in item['content']) for item in fragments) @@ -186,7 +185,6 @@ class ConditionalBlockBasicTest(unittest.TestCase): modules['source_module'].is_attempted = "true" ajax = json.loads(modules['cond_module'].handle_ajax('', '')) modules['cond_module'].save() - print("post-attempt ajax: ", ajax) fragments = ajax['fragments'] assert any(('This is a secret' in item['content']) for item in fragments) @@ -220,63 +218,29 @@ class ConditionalBlockXmlTest(unittest.TestCase): Make sure ConditionalBlock works, by loading data in from an XML-defined course. """ - @staticmethod - def get_system(load_error_modules=True): - '''Get a dummy system''' - return DummySystem(load_error_modules) - def setUp(self): super().setUp() self.test_system = get_test_system() - - def get_course(self, name): - """Get a test course by directory name. If there's more than one, error.""" - print(f"Importing {name}") - - modulestore = XMLModuleStore(DATA_DIR, source_dirs=[name]) - courses = modulestore.get_courses() - self.modulestore = modulestore # lint-amnesty, pylint: disable=attribute-defined-outside-init + self.modulestore = XMLModuleStore(DATA_DIR, source_dirs=['conditional_and_poll']) + courses = self.modulestore.get_courses() assert len(courses) == 1 - return courses[0] + self.course = courses[0] + + def get_module_for_location(self, location): + descriptor = self.modulestore.get_item(location, depth=None) + return self.test_system.get_module(descriptor) @patch('xmodule.x_module.descriptor_global_local_resource_url') @patch.dict(settings.FEATURES, {'ENABLE_EDXNOTES': False}) def test_conditional_module(self, _): """Make sure that conditional module works""" - - print("Starting import") - course = self.get_course('conditional_and_poll') - - print("Course: ", course) - print("id: ", course.id) - - def inner_get_module(descriptor): - if isinstance(descriptor, BlockUsageLocator): - location = descriptor - descriptor = self.modulestore.get_item(location, depth=None) - descriptor.xmodule_runtime = get_test_system() - descriptor.xmodule_runtime.descriptor_runtime = descriptor._runtime # pylint: disable=protected-access - descriptor.xmodule_runtime.get_module = inner_get_module - return descriptor - # edx - HarvardX # cond_test - ER22x location = BlockUsageLocator(CourseLocator("HarvardX", "ER22x", "2013_Spring", deprecated=True), "conditional", "condone", deprecated=True) - def replace_urls(text, staticfiles_prefix=None, replace_prefix='/static/', course_namespace=None): # lint-amnesty, pylint: disable=unused-argument - return text - self.test_system.replace_urls = replace_urls - self.test_system.get_module = inner_get_module - - module = inner_get_module(location) - print("module: ", module) - print("module children: ", module.get_children()) - print("module display items (children): ", module.get_display_items()) - + module = self.get_module_for_location(location) html = module.render(STUDENT_VIEW).content - print("html type: ", type(html)) - print("html: ", html) html_expect = module.xmodule_runtime.render_template( 'conditional_ajax.html', { @@ -288,29 +252,20 @@ class ConditionalBlockXmlTest(unittest.TestCase): ) assert html == html_expect - gdi = module.get_display_items() - print("gdi=", gdi) - ajax = json.loads(module.handle_ajax('', '')) - module.save() - print("ajax: ", ajax) fragments = ajax['fragments'] assert not any(('This is a secret' in item['content']) for item in fragments) # Now change state of the capa problem to make it completed - inner_module = inner_get_module(location.replace(category="problem", name='choiceprob')) + inner_module = self.get_module_for_location(location.replace(category="problem", name='choiceprob')) inner_module.attempts = 1 # Save our modifications to the underlying KeyValueStore so they can be persisted inner_module.save() ajax = json.loads(module.handle_ajax('', '')) - module.save() - print("post-attempt ajax: ", ajax) fragments = ajax['fragments'] assert any(('This is a secret' in item['content']) for item in fragments) - maxDiff = None - def test_conditional_module_with_empty_sources_list(self): """ If a ConditionalBlock is initialized with an empty sources_list, we assert that the sources_list is set diff --git a/common/lib/xmodule/xmodule/tests/test_html_module.py b/common/lib/xmodule/xmodule/tests/test_html_module.py index 38f6be9772..5695941d0b 100644 --- a/common/lib/xmodule/xmodule/tests/test_html_module.py +++ b/common/lib/xmodule/xmodule/tests/test_html_module.py @@ -4,6 +4,7 @@ import unittest from unittest.mock import Mock import ddt +from django.contrib.auth.models import AnonymousUser from django.test.utils import override_settings from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from xblock.field_data import DictFieldData @@ -135,8 +136,7 @@ class HtmlBlockSubstitutionTestCase(unittest.TestCase): # lint-amnesty, pylint: def test_substitution_without_anonymous_student_id(self): sample_xml = '''%%USER_ID%%''' field_data = DictFieldData({'data': sample_xml}) - module_system = get_test_system() - module_system.anonymous_student_id = None + module_system = get_test_system(user=AnonymousUser()) module = HtmlBlock(module_system, field_data, Mock()) assert module.get_html() == sample_xml diff --git a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py index c1c43fd17f..7dc69c9ad3 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py @@ -37,8 +37,7 @@ class LTI20RESTResultServiceTest(unittest.TestCase): mocked_course = Mock(name='mocked_course', lti_passports=['lti_id:test_client:test_secret']) modulestore = Mock(name='modulestore') modulestore.get_course.return_value = mocked_course - runtime = Mock(name='runtime', modulestore=modulestore, anonymous_student_id='student') - self.xmodule.runtime = runtime + self.xmodule.runtime.modulestore = modulestore self.xmodule.lti_id = "lti_id" test_cases = ( # (before sanitize, after sanitize) diff --git a/common/lib/xmodule/xmodule/tests/test_lti_unit.py b/common/lib/xmodule/xmodule/tests/test_lti_unit.py index 53d173bf3d..3f13666e98 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti_unit.py @@ -16,6 +16,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 from xmodule.lti_module import LTIBlock @@ -59,13 +60,14 @@ class LTIBlockTest(unittest.TestCase): self.system.get_real_user = Mock() self.system.publish = Mock() self.system.rebind_noauth_module_to_user = Mock() - self.user_id = self.system.anonymous_student_id self.xmodule = LTIBlock( self.system, DictFieldData({}), ScopeIds(None, None, None, BlockUsageLocator(self.system.course_id, 'lti', 'name')) ) + 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 diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index 5a31c16fbc..b24e561429 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -13,6 +13,7 @@ from unittest.mock import Mock, patch import pytz import ddt from fs.memoryfs import MemoryFS +from django.contrib.auth.models import AnonymousUser from . import get_test_system from .helpers import StubUserService @@ -157,11 +158,11 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): now = datetime.now(pytz.UTC) self.vertical.due = now + timedelta(days=days) if view == STUDENT_VIEW: - self.module_system._services['user'] = StubUserService() + self.module_system._services['user'] = StubUserService(user=Mock(username=self.username)) self.module_system._services['completion'] = StubCompletionService(enabled=True, completion_value=completion_value) elif view == PUBLIC_VIEW: - self.module_system._services['user'] = StubUserService(is_anonymous=True) + self.module_system._services['user'] = StubUserService(user=AnonymousUser()) html = self.module_system.render( self.vertical, view, self.default_context if context is None else context diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index e96386b062..70ac82167e 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -10,6 +10,7 @@ import oauthlib from django.conf import settings from django.urls import reverse +from common.djangoapps.xblock_django.constants import ATTR_KEY_ANONYMOUS_USER_ID from lms.djangoapps.courseware.tests.helpers import BaseTestXmodule from lms.djangoapps.courseware.views.views import get_course_lti_endpoints from openedx.core.lib.url_utils import quote_slashes @@ -40,7 +41,8 @@ class TestLTI(BaseTestXmodule): # Note: this course_id is actually a course_key context_id = str(self.item_descriptor.course_id) - user_id = str(self.item_descriptor.xmodule_runtime.anonymous_student_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 resource_link_id = str(urllib.parse.quote(f'{hostname}-{self.item_descriptor.location.html_id()}'))