From 4e388eac0a92ac683182677d8c0a6b45e9b4a3e8 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Mon, 16 May 2022 13:13:14 +1000 Subject: [PATCH 1/7] refactor: replace oauth param fetching from client to signature method The OAuthlib 1.0 Client's get_oauth_params fails when processing Webob request object with the body stored as a binary instead of string. This commit replaces the client function with a different one which doesn't involve body hashing, as the body hash is calculated explicitly. --- lti_consumer/lti_1p1/oauth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/lti_1p1/oauth.py b/lti_consumer/lti_1p1/oauth.py index c1a127f2..4da150ef 100644 --- a/lti_consumer/lti_1p1/oauth.py +++ b/lti_consumer/lti_1p1/oauth.py @@ -147,7 +147,7 @@ def log_authorization_header(request, client_key, client_secret): oauth_body_hash = str(base64.b64encode(sha1.digest())) log.debug("[LTI] oauth_body_hash = %s", oauth_body_hash) client = oauth1.Client(client_key, client_secret) - params = client.get_oauth_params(request) + params = oauth1.rfc5849.signature.collect_parameters(headers=request.headers, exclude_oauth_signature=False) params.append(('oauth_body_hash', oauth_body_hash)) mock_request = SignedRequest( uri=str(urllib.parse.unquote(request.url)), From 23d75b691206e38865773daa65830414ed61087c Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Tue, 10 May 2022 18:25:19 +1000 Subject: [PATCH 2/7] refactor: replace user rebinding runtime function with service The `rebind_noauth_module_to_user` function is deprecated in the core edx-platform [1]. This is now replaced with a "rebind_user" service. This commit brings this change to the LTI Consumer XBlock. [1] - https://github.com/openedx/edx-platform/pull/30320/ --- lti_consumer/lti_xblock.py | 5 +++-- lti_consumer/tests/unit/test_lti_xblock.py | 11 ++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index a3b1fef9..3f60d03c 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -161,6 +161,7 @@ class LaunchTarget: @XBlock.needs('i18n') +@XBlock.needs('rebind_user') @XBlock.wants('user') @XBlock.wants('settings') @XBlock.wants('lti-configuration') @@ -1332,7 +1333,7 @@ def _result_service_get(self, lti_consumer, user): Returns: dict: response to this request as dictated by the LtiConsumer """ - self.runtime.rebind_noauth_module_to_user(self, user) + self.runtime.service(self, 'rebind_user').rebind_noauth_module_to_user(self, user) args = [] if self.module_score: args.extend([self.module_score, self.score_comment]) @@ -1420,7 +1421,7 @@ def set_user_module_score(self, user, score, max_score, comment=''): else: scaled_score = None - self.runtime.rebind_noauth_module_to_user(self, user) + self.runtime.service(self, 'rebind_user').rebind_noauth_module_to_user(self, user) # have to publish for the progress page... self.runtime.publish( diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 81178dfe..604f3ab4 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -1074,10 +1074,12 @@ def test_consumer_get_result_called(self): mock_runtime = self.xblock.runtime = Mock() mock_lti_consumer = Mock() mock_user = Mock() + mock_rebind_user_service = Mock() + mock_runtime.service.return_value = mock_rebind_user_service self.xblock._result_service_get(mock_lti_consumer, mock_user) # pylint: disable=protected-access - mock_runtime.rebind_noauth_module_to_user.assert_called_with(self.xblock, mock_user) + mock_rebind_user_service.rebind_noauth_module_to_user.assert_called_with(self.xblock, mock_user) mock_lti_consumer.get_result.assert_called_with() @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.module_score', PropertyMock(return_value=0.5)) @@ -1210,12 +1212,15 @@ class TestSetScore(TestLtiConsumerXBlock): def test_rebind_called(self): """ - Test that `runtime.rebind_noauth_module_to_user` is called + Test that `rebind_noauth_module_to_user` service is called """ + mock_rebind_user_service = Mock() + self.xblock.runtime.service = Mock() + self.xblock.runtime.service.return_value = mock_rebind_user_service user = Mock(user_id=FAKE_USER_ID) self.xblock.set_user_module_score(user, 0.92, 1.0, 'Great Job!') - self.xblock.runtime.rebind_noauth_module_to_user.assert_called_with(self.xblock, user) + mock_rebind_user_service.rebind_noauth_module_to_user.assert_called_with(self.xblock, user) def test_publish_grade_event_called(self): """ From b6939d4eb19e41261f45806e02e152bdce09d76a Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Tue, 17 May 2022 16:27:59 +1000 Subject: [PATCH 3/7] refactor: replace getting hostname from to settings.LMS_BASE The hostname used to construct the resource link ID is moved from using a runtime attribute to the LMS_BASE setting. --- lti_consumer/lti_xblock.py | 4 +--- lti_consumer/tests/unit/test_lti_xblock.py | 4 +++- test_settings.py | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 3f60d03c..86620af6 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -881,9 +881,7 @@ def resource_link_id(self): 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(urllib.parse.quote( - f"{self.runtime.hostname}-{self.location.html_id()}" # pylint: disable=no-member - )) + return str(urllib.parse.quote(f"{settings.LMS_BASE}-{self.location.html_id()}")) # pylint: disable=no-member @property def lis_result_sourcedid(self): diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 604f3ab4..2d3561e2 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -292,13 +292,15 @@ def test_external_user_id_none(self): with self.assertRaises(LtiError): __ = self.xblock.external_user_id + @override_settings(LMS_BASE="edx.org") def test_resource_link_id(self): """ Test `resource_link_id` returns appropriate string """ + hostname = "edx.org" self.assertEqual( self.xblock.resource_link_id, - f"{self.xblock.runtime.hostname}-{self.xblock.location.html_id()}" # pylint: disable=no-member + f"{hostname}-{self.xblock.location.html_id()}" # pylint: disable=no-member ) @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.context_id') diff --git a/test_settings.py b/test_settings.py index c1b33d91..ec88f573 100644 --- a/test_settings.py +++ b/test_settings.py @@ -12,6 +12,7 @@ # LMS Urls - for LTI 1.3 testing LMS_ROOT_URL = "https://example.com" +LMS_BASE = "example.com" # Dummy FEATURES dict FEATURES = {} From 886674c9509fd1ca08b9201cf121dc01da131848 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 5 Oct 2022 16:48:53 +0530 Subject: [PATCH 4/7] refactor: replace usage of get_real_user with UserService --- lti_consumer/lti_xblock.py | 2 +- lti_consumer/outcomes.py | 5 +++-- lti_consumer/tests/unit/test_lti_xblock.py | 6 +++++- lti_consumer/tests/unit/test_outcomes.py | 6 +++++- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 86620af6..077c27b6 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -1293,7 +1293,7 @@ def result_service_handler(self, request, suffix=''): except LtiError: return Response(status=401) # Unauthorized in this case. 401 is right - user = self.runtime.get_real_user(anon_id) + user = self.runtime.service(self, 'user').get_user_by_anonymous_id(anon_id) if not user: # that means we can't save to database, as we do not have real user id. msg = _("[LTI]: Real user not found against anon_id: {}").format(anon_id) log.info(msg) diff --git a/lti_consumer/outcomes.py b/lti_consumer/outcomes.py index d715ff24..b05ae0db 100644 --- a/lti_consumer/outcomes.py +++ b/lti_consumer/outcomes.py @@ -6,8 +6,8 @@ """ import logging -import urllib.parse from xml.sax.saxutils import escape +from urllib.parse import unquote from lxml import etree from xblockutils.resources import ResourceLoader @@ -183,7 +183,8 @@ def handle_request(self, request): log.debug("[LTI]: %s", error_message) return response_xml_template.format(**failure_values) - real_user = self.xblock.runtime.get_real_user(urllib.parse.unquote(sourced_id.split(':')[-1])) + anon_id = unquote(sourced_id.split(':')[-1]) + real_user = self.xblock.runtime.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. failure_values['imsx_messageIdentifier'] = escape(imsx_message_identifier) failure_values['imsx_description'] = "User not found." diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 2d3561e2..7b105cb6 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -998,7 +998,11 @@ def test_bad_user_id(self, mock_parse_suffix): Test 404 response returned when a user cannot be found """ mock_parse_suffix.return_value = FAKE_USER_ID - self.xblock.runtime.get_real_user.return_value = None + self.xblock.runtime.service = Mock( + return_value=Mock( + get_user_by_anonymous_id=Mock(return_value=None) + ) + ) response = self.xblock.result_service_handler(make_request('', 'GET')) self.assertEqual(response.status_code, 404) diff --git a/lti_consumer/tests/unit/test_outcomes.py b/lti_consumer/tests/unit/test_outcomes.py index d97d926f..dcc1e5de 100644 --- a/lti_consumer/tests/unit/test_outcomes.py +++ b/lti_consumer/tests/unit/test_outcomes.py @@ -416,7 +416,11 @@ def test_user_not_found(self): Test user not found returns failure response """ request = make_request('') - self.xblock.runtime.get_real_user.return_value = None + self.xblock.runtime.service = Mock( + return_value=Mock( + get_user_by_anonymous_id=Mock(return_value=None) + ) + ) response = self.outcome_servce.handle_request(request) self.assertIn('failure', response) From 98a16ad7e3cab2fea30f5c46420fabdbc3af6fb5 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 6 Oct 2022 07:58:25 +0530 Subject: [PATCH 5/7] refactor: Replace deprecated usage of runtime.course_id Replaces usages of runtime.course_id with runtime.scope_ids.usage_id.context_key. --- lti_consumer/lti_xblock.py | 6 +++--- lti_consumer/tests/unit/test_lti_xblock.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 077c27b6..f877254a 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -714,7 +714,7 @@ def editable_fields(self): if config_service: is_already_sharing_learner_info = self.ask_to_send_email or self.ask_to_send_username if not config_service.configuration.lti_access_to_learners_editable( - self.course_id, + self.scope_ids.usage_id.context_key, is_already_sharing_learner_info, ): noneditable_fields.extend(['ask_to_send_username', 'ask_to_send_email']) @@ -746,7 +746,7 @@ def context_id(self): context_id is an opaque identifier that uniquely identifies the context (e.g., a course) that contains the link being launched. """ - return str(self.course_id) + return str(self.scope_ids.usage_id.context_key) @property def role(self): @@ -764,7 +764,7 @@ def course(self): """ Return course by course id. """ - return self.runtime.modulestore.get_course(self.runtime.course_id) + return self.runtime.modulestore.get_course(self.scope_ids.usage_id.context_key) @property def lti_provider_key_secret(self): diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 7b105cb6..556971fb 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -130,7 +130,7 @@ def test_context_id(self): """ Test `context_id` returns unicode course id """ - self.assertEqual(self.xblock.context_id, str(self.xblock.course_id)) + self.assertEqual(self.xblock.context_id, str(self.xblock.runtime.scope_ids.usage_id.context_key)) def test_validate(self): """ From 7178d4980cb222114823364ff17f61d3f9467422 Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Tue, 29 Nov 2022 11:13:02 +0100 Subject: [PATCH 6/7] chore: update pylint configuration --- pylintrc | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/pylintrc b/pylintrc index 6830c868..abec8fa6 100644 --- a/pylintrc +++ b/pylintrc @@ -64,18 +64,18 @@ # SERIOUSLY. # # ------------------------------ -# Generated by edx-lint version: 5.2.5 +# Generated by edx-lint version: 5.3.0 # ------------------------------ [MASTER] -ignore = +ignore = persistent = yes load-plugins = edx_lint.pylint,pylint_django,pylint_celery [MESSAGES CONTROL] -enable = +enable = blacklisted-name, line-too-long, - + abstract-class-instantiated, abstract-method, access-member-before-definition, @@ -184,26 +184,26 @@ enable = used-before-assignment, using-constant-test, yield-outside-function, - + astroid-error, fatal, method-check-failed, parse-error, raw-checker-failed, - + empty-docstring, invalid-characters-in-docstring, missing-docstring, wrong-spelling-in-comment, wrong-spelling-in-docstring, - + unused-argument, unused-import, unused-variable, - + eval-used, exec-used, - + bad-classmethod-argument, bad-mcs-classmethod-argument, bad-mcs-method-argument, @@ -234,30 +234,30 @@ enable = unneeded-not, useless-else-on-loop, wrong-assert-type, - + deprecated-method, deprecated-module, - + too-many-boolean-expressions, too-many-nested-blocks, too-many-statements, - + wildcard-import, wrong-import-order, wrong-import-position, - + missing-final-newline, mixed-line-endings, trailing-newlines, trailing-whitespace, unexpected-line-ending-format, - + bad-inline-option, bad-option-value, deprecated-pragma, unrecognized-inline-option, useless-suppression, -disable = +disable = bad-indentation, consider-using-f-string, duplicate-code, @@ -281,10 +281,10 @@ disable = unspecified-encoding, unused-wildcard-import, use-maxsplit-arg, - + feature-toggle-needs-doc, illegal-waffle-usage, - + logging-fstring-interpolation, django-not-configured, @@ -329,7 +329,7 @@ ignore-imports = no ignore-mixin-members = yes ignored-classes = SQLObject unsafe-load-any-extension = yes -generated-members = +generated-members = REQUEST, acl_users, aq_parent, @@ -355,7 +355,7 @@ generated-members = [VARIABLES] init-import = no dummy-variables-rgx = _|dummy|unused|.*_unused -additional-builtins = +additional-builtins = [CLASSES] defining-attr-methods = __init__,__new__,setUp @@ -376,11 +376,11 @@ max-public-methods = 20 [IMPORTS] deprecated-modules = regsub,TERMIOS,Bastion,rexec -import-graph = -ext-import-graph = -int-import-graph = +import-graph = +ext-import-graph = +int-import-graph = [EXCEPTIONS] overgeneral-exceptions = Exception -# d1da7b0099fad5ecd841f40012c2a0bc326a4c1b +# acb0bd5fab1bee88c0721bdd68196a478007223c From f647c5fb7a52bcbd13d8eaaba8bdca8419831c6b Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 4 Nov 2022 16:53:29 +0530 Subject: [PATCH 7/7] refactor: replace block.location with block.scope_ids.usage_id --- CHANGELOG.rst | 9 ++++++++ lti_consumer/__init__.py | 2 +- lti_consumer/api.py | 8 +++---- lti_consumer/lti_xblock.py | 22 +++++++++---------- lti_consumer/models.py | 2 +- lti_consumer/signals/signals.py | 2 +- lti_consumer/tests/test_utils.py | 20 ++++++++--------- lti_consumer/tests/unit/plugin/test_views.py | 2 +- .../tests/unit/plugin/test_views_lti_ags.py | 20 ++++++++--------- .../plugin/test_views_lti_deep_linking.py | 2 +- .../tests/unit/plugin/test_views_lti_nrps.py | 2 +- lti_consumer/tests/unit/test_api.py | 8 ++----- lti_consumer/tests/unit/test_lti_xblock.py | 8 +++---- lti_consumer/tests/unit/test_models.py | 12 +++++----- 14 files changed, 61 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ba8cca09..e8af8568 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -15,6 +15,15 @@ Please See the [releases tab](https://github.com/openedx/xblock-lti-consumer/rel Unreleased ~~~~~~~~~~ + +7.0.0 - 2022-11-29 +------------------ +* Refactor anonymous user to real user rebinding function to use `rebind_user` service. +* Refactor accessing hostname from runtime attribute to using `settings.LMS_BASE`. +* Refactor usage of `get_real_user` with `UserService`. +* Refactor deprecated usage of `runtime.course_id` and replace with `runtime.scope_ids.usage_id.context_key`. +* Refactor deprecated usage of `block.location` with `block.scope_ids.usage_id`. + 6.4.0 - 2022-11-18 ------------------ Adds support for sending an external_user_id in LTI 1.1 XBlock launches. When the diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py index 35120e4e..df365c47 100644 --- a/lti_consumer/__init__.py +++ b/lti_consumer/__init__.py @@ -4,4 +4,4 @@ from .apps import LTIConsumerApp from .lti_xblock import LtiConsumerXBlock -__version__ = '6.4.0' +__version__ = '7.0.0' diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 34290d57..dc1b29d2 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -65,24 +65,24 @@ def _get_lti_config_for_block(block): if block.config_type == 'database': lti_config = _get_or_create_local_lti_config( block.lti_version, - block.location, + block.scope_ids.usage_id, LtiConfiguration.CONFIG_ON_DB, ) elif block.config_type == 'external': config = get_external_config_from_filter( - {"course_key": block.location.course_key}, + {"course_key": block.scope_ids.usage_id.context_key}, block.external_config ) lti_config = _get_or_create_local_lti_config( config.get("version"), - block.location, + block.scope_ids.usage_id, LtiConfiguration.CONFIG_EXTERNAL, external_id=block.external_config, ) else: lti_config = _get_or_create_local_lti_config( block.lti_version, - block.location, + block.scope_ids.usage_id, LtiConfiguration.CONFIG_ON_XBLOCK, ) return lti_config diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index f877254a..fb3e1d96 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -139,10 +139,10 @@ def valid_config_type_values(block): {"display_name": _("Configuration on block"), "value": "new"} ] - if database_config_enabled(block.location.course_key): + if database_config_enabled(block.scope_ids.usage_id.context_key): values.append({"display_name": _("Database Configuration"), "value": "database"}) - if external_config_filter_enabled(block.location.course_key): + if external_config_filter_enabled(block.scope_ids.usage_id.context_key): values.append({"display_name": _("Reusable Configuration"), "value": "external"}) return values @@ -696,8 +696,8 @@ def editable_fields(self): editable_fields = self.editable_field_names noneditable_fields = [] - is_database_config_enabled = database_config_enabled(self.location.course_key) # pylint: disable=no-member - is_external_config_filter_enabled = external_config_filter_enabled(self.location.course_key) # pylint: disable=no-member + is_database_config_enabled = database_config_enabled(self.scope_ids.usage_id.context_key) + is_external_config_filter_enabled = external_config_filter_enabled(self.scope_ids.usage_id.context_key) # If neither additional config_types are enabled, do not display the "config_type" field to users, as "new" is # the only option and does not make sense without other options. @@ -844,7 +844,7 @@ def get_lti_1p1_user_id(self): toggling this flag in a running course carries the risk of breaking the LTI integrations in the course. This flag should also only be enabled for new courses in which no LTI attempts have been made. """ - if external_user_id_1p1_launches_enabled(self.location.course_key): # pylint: disable=no-member + if external_user_id_1p1_launches_enabled(self.scope_ids.usage_id.context_key): return self.external_user_id return self.anonymous_user_id @@ -881,7 +881,7 @@ def resource_link_id(self): 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(urllib.parse.quote(f"{settings.LMS_BASE}-{self.location.html_id()}")) # pylint: disable=no-member + return str(urllib.parse.quote(f"{settings.LMS_BASE}-{self.scope_ids.usage_id.html_id()}")) @property def lis_result_sourcedid(self): @@ -1222,7 +1222,7 @@ def lti_1p3_access_token(self, request, suffix=''): # pylint: disable=unused-ar # Runtime import because this can only be run in the LMS/Studio Django # environments. Importing the views on the top level will cause RuntimeErorr from lti_consumer.plugin.views import access_token_endpoint # pylint: disable=import-outside-toplevel - return access_token_endpoint(request, usage_id=str(self.location)) # pylint: disable=no-member + return access_token_endpoint(request, usage_id=str(self.scope_ids.usage_id)) @XBlock.handler def outcome_service_handler(self, request, suffix=''): # pylint: disable=unused-argument @@ -1458,8 +1458,8 @@ def get_lti_1p3_launch_data(self): from lti_consumer.api import config_id_for_block config_id = config_id_for_block(self) - location = self.location # pylint: disable=no-member - course_key = str(location.course_key) + location = self.scope_ids.usage_id + course_key = str(location.context_key) launch_data = Lti1p3LaunchData( user_id=self.lms_user_id, @@ -1481,7 +1481,7 @@ def get_context_title(self): Return the title attribute of the context_claim for LTI 1.3 launches. This information is included in the launch_data query or form parameter of the LTI 1.3 third-party login initiation request. """ - course_key = self.location.course_key # pylint: disable=no-member + course_key = self.scope_ids.usage_id.context_key course = compat.get_course_by_id(course_key) return " - ".join([ @@ -1554,7 +1554,7 @@ def _get_context_for_template(self): return { 'launch_url': launch_url.strip(), 'lti_1p3_launch_url': lti_1p3_launch_url, - 'element_id': self.location.html_id(), # pylint: disable=no-member + 'element_id': self.scope_ids.usage_id.html_id(), 'element_class': self.category, 'launch_target': self.launch_target, 'display_name': self.display_name, diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 1b83b0c2..f96f5f71 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -241,7 +241,7 @@ def clean(self): }) if self.version == self.LTI_1P3 and self.config_store == self.CONFIG_ON_DB: block = compat.load_enough_xblock(self.location) - if not database_config_enabled(block.location.course_key): + if not database_config_enabled(block.scope_ids.usage_id.context_key): raise ValidationError({ "config_store": _("LTI Configuration stores on database is not enabled."), }) diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index ccf1194f..593d5abd 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -46,7 +46,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl # the LMS database. log.info( "Publishing LTI grade from block %s to LMS. User: %s (score: %s)", - block.location, + block.scope_ids.usage_id, user, score, ) diff --git a/lti_consumer/tests/test_utils.py b/lti_consumer/tests/test_utils.py index 192505b9..f31c0781 100644 --- a/lti_consumer/tests/test_utils.py +++ b/lti_consumer/tests/test_utils.py @@ -5,7 +5,8 @@ from unittest.mock import Mock import urllib -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LocalId from webob import Request from workbench.runtime import WorkbenchRuntime from xblock.fields import ScopeIds @@ -22,30 +23,27 @@ def make_xblock(xblock_name, xblock_cls, attributes): runtime = WorkbenchRuntime() key_store = DictKeyValueStore() db_model = KvsFieldData(key_store) - ids = generate_scope_ids(runtime, xblock_name) + course_id = 'course-v1:edX+DemoX+Demo_Course' + course_key = CourseKey.from_string(course_id) + ids = generate_scope_ids(course_key, xblock_name) + xblock = xblock_cls(runtime, db_model, scope_ids=ids) xblock.category = Mock() - xblock.location = UsageKey.from_string( - 'block-v1:edX+DemoX+Demo_Course+type@problem+block@466f474fa4d045a8b7bde1b911e095ca' - ) - xblock.runtime = Mock( hostname='localhost', ) - xblock.course_id = 'course-v1:edX+DemoX+Demo_Course' for key, value in attributes.items(): setattr(xblock, key, value) return xblock -def generate_scope_ids(runtime, block_type): +def generate_scope_ids(course_key, block_type): """ Helper to generate scope IDs for an XBlock """ - def_id = runtime.id_generator.create_definition(block_type) - usage_id = runtime.id_generator.create_usage(def_id) - return ScopeIds('user', block_type, def_id, usage_id) + usage_key = course_key.make_usage_key(block_type, str(LocalId())) + return ScopeIds('user', block_type, usage_key, usage_key) def make_request(body, method='POST'): diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 7c674032..543c24c7 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -333,7 +333,7 @@ def test_launch_callback_endpoint_deep_linking_database_config(self, dl_enabled) self.xblock.config_type = 'database' LtiConfiguration.objects.filter(id=self.config.id).update( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_DB, lti_advantage_deep_linking_enabled=dl_enabled, diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index a80a142f..7a9e850f 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -51,7 +51,7 @@ def setUp(self): # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, ) @@ -171,7 +171,7 @@ def test_lti_ags_list(self): line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, # pylint: disable=no-member + resource_link_id=self.xblock.scope_ids.usage_id, label="test label", score_maximum=100 ) @@ -192,7 +192,7 @@ def test_lti_ags_list(self): 'scoreMaximum': 100, 'label': 'test label', 'tag': '', - 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member + 'resourceLinkId': str(self.xblock.scope_ids.usage_id), 'startDateTime': None, 'endDateTime': None, } @@ -209,7 +209,7 @@ def test_lti_ags_retrieve(self): line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, # pylint: disable=no-member + resource_link_id=self.xblock.scope_ids.usage_id, label="test label", score_maximum=100 ) @@ -236,7 +236,7 @@ def test_lti_ags_retrieve(self): 'scoreMaximum': 100, 'label': 'test label', 'tag': '', - 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member + 'resourceLinkId': str(self.xblock.scope_ids.usage_id), 'startDateTime': None, 'endDateTime': None, } @@ -256,7 +256,7 @@ def test_create_lineitem(self): 'scoreMaximum': 100, 'label': 'test', 'tag': 'score', - 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member + 'resourceLinkId': str(self.xblock.scope_ids.usage_id), }), content_type="application/vnd.ims.lis.v2.lineitem+json", ) @@ -270,7 +270,7 @@ def test_create_lineitem(self): 'scoreMaximum': 100, 'label': 'test', 'tag': 'score', - 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member + 'resourceLinkId': str(self.xblock.scope_ids.usage_id), 'startDateTime': None, 'endDateTime': None, } @@ -281,7 +281,7 @@ def test_create_lineitem(self): self.assertEqual(line_item.score_maximum, 100) self.assertEqual(line_item.label, 'test') self.assertEqual(line_item.tag, 'score') - self.assertEqual(str(line_item.resource_link_id), str(self.xblock.location)) # pylint: disable=no-member + self.assertEqual(str(line_item.resource_link_id), str(self.xblock.scope_ids.usage_id)) def test_create_lineitem_invalid_resource_link_id(self): """ @@ -318,7 +318,7 @@ def setUp(self): self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, # pylint: disable=no-member + resource_link_id=self.xblock.scope_ids.usage_id, label="test label", score_maximum=100 ) @@ -850,7 +850,7 @@ def setUp(self): self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, # pylint: disable=no-member + resource_link_id=self.xblock.scope_ids.usage_id, label="test label", score_maximum=100 ) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py index 5d29d12d..a0ddc8b4 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py @@ -31,7 +31,7 @@ def setUp(self): # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, ) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py index a9d3e1e2..352e4533 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py @@ -137,7 +137,7 @@ def setUp(self): # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, ) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 19ff9f2f..083f850e 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -72,7 +72,7 @@ def _setup_lti_block(self): # Create lti configuration self.lti_config = LtiConfiguration.objects.create( config_id=_test_config_id, - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_XBLOCK, ) @@ -464,15 +464,11 @@ def test_retrieve_with_block(self): """ Check if the API creates the model and retrieved correct info. """ - block = Mock() - block.location = 'block-v1:course+test+2020+type@problem+block@test' - block.lti_version = LtiConfiguration.LTI_1P3 - launch_data = self._get_lti_1p3_launch_data() # Create LTI Config and Deep linking object lti_config = LtiConfiguration.objects.create( - location=block.location, + location='block-v1:course+test+2020+type@problem+block@test', config_id=_test_config_id, ) LtiDlContentItem.objects.create( diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 556971fb..0b8631cd 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -130,7 +130,7 @@ def test_context_id(self): """ Test `context_id` returns unicode course id """ - self.assertEqual(self.xblock.context_id, str(self.xblock.runtime.scope_ids.usage_id.context_key)) + self.assertEqual(self.xblock.context_id, str(self.xblock.scope_ids.usage_id.context_key)) def test_validate(self): """ @@ -300,7 +300,7 @@ def test_resource_link_id(self): hostname = "edx.org" self.assertEqual( self.xblock.resource_link_id, - f"{hostname}-{self.xblock.location.html_id()}" # pylint: disable=no-member + f"{hostname}-{self.xblock.scope_ids.usage_id.html_id()}" ) @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.context_id') @@ -1529,12 +1529,12 @@ def test_get_lti_1p3_launch_data(self): launch_data = self.xblock.get_lti_1p3_launch_data() - course_key = str(self.xblock.location.course_key) # pylint: disable=no-member + course_key = str(self.xblock.scope_ids.usage_id.course_key) expected_launch_data = Lti1p3LaunchData( user_id=1, user_role="instructor", config_id=config_id_for_block(self.xblock), - resource_link_id=str(self.xblock.location), # pylint: disable=no-member + resource_link_id=str(self.xblock.scope_ids.usage_id), external_user_id="external_user_id", launch_presentation_document_target="iframe", message_type="LtiResourceLinkRequest", diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index f9b189c9..9f9a37e0 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -59,17 +59,17 @@ def setUp(self): # Creates an LTI configuration objects for testing self.lti_1p1_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P1 ) self.lti_1p3_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3 ) self.lti_1p3_config_db = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_DB, lti_advantage_ags_mode='programmatic', @@ -92,7 +92,7 @@ def _get_1p3_config(self, **kwargs): Helper function to create a LtiConfiguration object with specific attributes """ return LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, **kwargs ) @@ -472,7 +472,7 @@ def setUp(self): self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) self.lti_1p3_config = LtiConfiguration.objects.create( - location=self.xblock.location, # pylint: disable=no-member + location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3 ) @@ -489,7 +489,7 @@ def test_repr(self): self.assertEqual( str(content_item), "[CONFIG_ON_XBLOCK] lti_1p3 - " - "block-v1:edX+DemoX+Demo_Course+type@problem+block@466f474fa4d045a8b7bde1b911e095ca: image" + f"{content_item.lti_configuration.location}: image" )