fixup! refactor: decouple config_id and location

Author: navinkarkera

lti_consumer/api.py

@@ -4,28 +4,31 @@
 Some methods are meant to be used inside the XBlock, so they
 return plaintext to allow easy testing/mocking.
 """
+from uuid import UUID
 
 import json
 
-from opaque_keys.edx.keys import CourseKey
+from opaque_keys.edx.keys import CourseKey, UsageKey
 
 from lti_consumer.lti_1p3.constants import LTI_1P3_ROLE_MAP
-from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem
+
+from .filters import get_external_config_from_filter
+from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem, LtiXBlockConfig
 from .utils import (
     get_cache_key,
     get_data_from_cache,
-    get_lti_1p3_context_types_claim,
-    get_lti_deeplinking_content_url,
+    get_lms_lti_access_token_link,
     get_lms_lti_keyset_link,
     get_lms_lti_launch_link,
-    get_lms_lti_access_token_link,
+    get_lti_1p3_context_types_claim,
+    get_lti_deeplinking_content_url,
 )
-from .filters import get_external_config_from_filter
 
 
-def _get_or_create_local_lti_config(
-    lti_version,
-    config_id,
+def _get_or_create_local_lti_xblock_config(
+    lti_version: str,
+    block_location: UsageKey | str,
+    config_id: str | None = None,
     config_store=LtiConfiguration.CONFIG_ON_XBLOCK,
     external_id=None,
 ):
@@ -39,7 +42,14 @@ def _get_or_create_local_lti_config(
     This allows XBlock users to update the LTI version without needing to update the database.
     """
     # The create operation is only performed when there is no existing configuration for the block
-    lti_config, _ = LtiConfiguration.objects.get_or_create(config_id=config_id)
+    lti_xblock_config, created = LtiXBlockConfig.objects.get_or_create(location=block_location)
+    if created:
+        if config_id:
+            lti_config, _ = LtiConfiguration.objects.get_or_create(config_id=config_id)
+        else:
+            lti_config = LtiConfiguration.objects.create()
+    else:
+        lti_config = lti_xblock_config.lti_configuration
 
     lti_config.config_store = config_store
     lti_config.external_id = external_id
@@ -49,7 +59,7 @@ def _get_or_create_local_lti_config(
 
     lti_config.save()
 
-    return lti_config
+    return lti_xblock_config
 
 
 def _get_config_by_config_id(config_id):
@@ -59,16 +69,24 @@ def _get_config_by_config_id(config_id):
     return LtiConfiguration.objects.get(config_id=config_id)
 
 
+def _get_config_by_location(location: UsageKey | str):
+    """
+    Gets the LTI xblock config by its UUID config ID
+    """
+    return LtiXBlockConfig.objects.get(location=location)
+
+
 def _get_lti_config_for_block(block):
     """
-    Retrieves or creates a LTI Configuration for a block.
+    Retrieves or creates a LTI Xblock Configuration for a block.
 
-    This wraps around `_get_or_create_local_lti_config` and handles the block and modulestore
+    This wraps around `_get_or_create_local_lti_xblock_config` and handles the block and modulestore
     bits of configuration.
     """
     if block.config_type == 'database':
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             block.lti_version,
+            block.scope_ids.usage_id,
             block.config_id,
             LtiConfiguration.CONFIG_ON_DB,
         )
@@ -77,51 +95,54 @@ def _get_lti_config_for_block(block):
             {"course_key": block.scope_ids.usage_id.context_key},
             block.external_config
         )
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             config.get("version"),
+            block.scope_ids.usage_id,
             block.config_id,
             LtiConfiguration.CONFIG_EXTERNAL,
             external_id=block.external_config,
         )
     else:
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             block.lti_version,
+            block.scope_ids.usage_id,
             block.config_id,
             LtiConfiguration.CONFIG_ON_XBLOCK,
         )
-    return lti_config
+    return lti_xblock_config
 
 
-def config_id_for_block(block):
+def config_for_block(block):
     """
     Returns the externally facing config_id of the LTI Configuration used by this block,
     creating one if required. That ID is suitable for use in launch data or get_consumer.
     """
-    config = _get_lti_config_for_block(block)
-    return config.config_id
+    xblock_config = _get_lti_config_for_block(block)
+    return xblock_config
 
 
-def get_lti_consumer(config_id):
+def get_lti_consumer(location: UsageKey):
     """
-    Retrieves an LTI Consumer instance for a given configuration.
+    Retrieves an LTI Consumer instance for a given location.
 
     Returns an instance of LtiConsumer1p1 or LtiConsumer1p3 depending
     on the configuration.
     """
     # Return an instance of LTI 1.1 or 1.3 consumer, depending
     # on the configuration stored in the model.
-    return _get_config_by_config_id(config_id).get_lti_consumer()
+    return _get_config_by_location(location).get_lti_consumer()
 
 
 def get_lti_1p3_launch_info(
     launch_data,
+    location: UsageKey,
 ):
     """
     Retrieves the Client ID, Keyset URL and other urls used to configure a LTI tool.
     """
     # Retrieve LTI Config and consumer
-    lti_config = _get_config_by_config_id(launch_data.config_id)
-    lti_consumer = lti_config.get_lti_consumer()
+    lti_xblock_config = _get_config_by_location(location)
+    lti_consumer = lti_xblock_config.get_lti_consumer()
 
     # Check if deep Linking is available, if so, add some extra context:
     # Deep linking launch URL, and if deep linking is already configured
@@ -138,12 +159,13 @@ def get_lti_1p3_launch_info(
 
         # Retrieve LTI Content Items (if any was set up)
         dl_content_items = LtiDlContentItem.objects.filter(
-            lti_configuration=lti_config
+            lti_xblock_config=lti_xblock_config
         )
         # Add content item attributes to context
         if dl_content_items.exists():
             deep_linking_content_items = [item.attributes for item in dl_content_items]
 
+    lti_config = lti_xblock_config.lti_configuration
     config_id = lti_config.config_id
     client_id = lti_config.lti_1p3_client_id
 
@@ -169,14 +191,15 @@ def get_lti_1p3_launch_info(
 
 def get_lti_1p3_launch_start_url(
     launch_data,
+    location: UsageKey,
     deep_link_launch=False,
     dl_content_id=None,
 ):
     """
     Computes and retrieves the LTI URL that starts the OIDC flow.
     """
     # Retrieve LTI consumer
-    lti_consumer = get_lti_consumer(launch_data.config_id)
+    lti_consumer = get_lti_consumer(location)
 
     # Include a message hint in the launch_data depending on LTI launch type
     # Case 1: Performs Deep Linking configuration flow. Triggered by staff users to
@@ -194,6 +217,7 @@ def get_lti_1p3_launch_start_url(
 
 def get_lti_1p3_content_url(
     launch_data,
+    location: UsageKey,
 ):
     """
     Computes and returns which URL the LTI consumer should launch to.
@@ -213,13 +237,14 @@ def get_lti_1p3_content_url(
 
     # If there's no content items, return normal LTI launch URL
     if not content_items.count():
-        return get_lti_1p3_launch_start_url(launch_data)
+        return get_lti_1p3_launch_start_url(launch_data, location)
 
     # If there's a single `ltiResourceLink` content, return the launch
     # url for that specific deep link
     if content_items.count() == 1 and content_items.get().content_type == LtiDlContentItem.LTI_RESOURCE_LINK:
         return get_lti_1p3_launch_start_url(
             launch_data,
+            location,
             dl_content_id=content_items.get().id,
         )
 

lti_consumer/lti_xblock.py

@@ -715,11 +715,14 @@ def validate(self):
                 validation.add(ValidationMessage(ValidationMessage.WARNING, str(
                     _("The specified LTI ID is not configured in this course's Advanced Settings.")
                 )))
+        return validation
+
+    def save(self):
         if self.lti_version == "lti_1p3" and not self.config_id:
             self.config_id = str(uuid.uuid4())
             # __AUTO_GENERATED_PRINT_VAR_START__
             print(f"""======================================= LtiConsumerXBlock#validate config_id: {self.config_id}""") # __AUTO_GENERATED_PRINT_VAR_END__
-        return validation
+        super().save()
 
     def get_settings(self):
         """
@@ -1126,9 +1129,9 @@ def _get_lti_consumer(self):
         # Runtime import since this will only run in the
         # Open edX LMS/Studio environments.
         # pylint: disable=import-outside-toplevel
-        from lti_consumer.api import config_id_for_block, get_lti_consumer
+        from lti_consumer.api import config_for_block
 
-        return get_lti_consumer(config_id_for_block(self))
+        return config_for_block(self).get_lti_consumer()
 
     def extract_real_user_data(self):
         """
@@ -1654,8 +1657,8 @@ def get_lti_1p3_launch_data(self):
         # Open edX LMS/Studio environments.
         # TODO: Replace this with a more appropriate API function that is intended for public use.
         # pylint: disable=import-outside-toplevel
-        from lti_consumer.api import config_id_for_block
-        config_id = config_id_for_block(self)
+        from lti_consumer.api import config_for_block
+        xblock_config = config_for_block(self)
 
         location = self.scope_ids.usage_id
         course_key = str(location.context_key)
@@ -1678,7 +1681,7 @@ def get_lti_1p3_launch_data(self):
         launch_data = Lti1p3LaunchData(
             user_id=self.lms_user_id,
             user_role=self.role,
-            config_id=config_id,
+            config_id=xblock_config.config.config_id,
             resource_link_id=str(location),
             external_user_id=self.external_user_id,
             preferred_username=username,
@@ -1719,6 +1722,7 @@ def _get_lti_block_launch_handler(self):
             from lti_consumer.api import get_lti_1p3_content_url  # pylint: disable=import-outside-toplevel
             lti_block_launch_handler = get_lti_1p3_content_url(
                 launch_data,
+                self.scope_ids.usage_id,
             )
 
         return lti_block_launch_handler

lti_consumer/tests/unit/test_api.py

@@ -11,8 +11,8 @@
 
 from lti_consumer.api import (
     _get_config_by_config_id,
-    _get_or_create_local_lti_config,
-    config_id_for_block,
+    _get_or_create_local_lti_xblock_config,
+    config_for_block,
     get_end_assessment_return,
     get_lti_1p3_content_url,
     get_deep_linking_data,
@@ -103,13 +103,13 @@ def setUp(self):
 
     def test_double_fetch(self):
         self.xblock.config_type = 'database'
-        config_id = config_id_for_block(self.xblock)
+        config_id = config_for_block(self.xblock)
         self.assertIsNotNone(config_id)
         config = _get_config_by_config_id(config_id)
         self.assertEqual(LtiConfiguration.CONFIG_ON_DB, config.config_store)
 
         # fetch again, shouldn't make a new one
-        second_config_id = config_id_for_block(self.xblock)
+        second_config_id = config_for_block(self.xblock)
         self.assertEqual(config_id, second_config_id)
 
     @ddt.data(
@@ -122,7 +122,7 @@ def test_store_types(self, mapping_pair, mock_external_config):
         mock_external_config.return_value = {"version": LtiConfiguration.LTI_1P3}
         str_store, result_store = mapping_pair
         self.xblock.config_type = str_store
-        config_id = config_id_for_block(self.xblock)
+        config_id = config_for_block(self.xblock)
         self.assertIsNotNone(config_id)
         config = _get_config_by_config_id(config_id)
         self.assertEqual(result_store, config.config_store)
@@ -144,7 +144,7 @@ def test_create_lti_config_if_inexistent(self):
         self.assertEqual(LtiConfiguration.objects.all().count(), 0)
 
         # Call API
-        lti_config = _get_or_create_local_lti_config(
+        lti_config = _get_or_create_local_lti_xblock_config(
             lti_version=lti_version,
             block_location=location
         )
@@ -166,7 +166,7 @@ def test_retrieve_existing(self):
         )
 
         # Call API
-        lti_config_retrieved = _get_or_create_local_lti_config(
+        lti_config_retrieved = _get_or_create_local_lti_xblock_config(
             lti_version=lti_version,
             block_location=location
         )
@@ -187,7 +187,7 @@ def test_update_lti_version(self):
         )
 
         # Call API
-        _get_or_create_local_lti_config(
+        _get_or_create_local_lti_xblock_config(
             lti_version=LtiConfiguration.LTI_1P3,
             block_location=location
         )
@@ -204,7 +204,7 @@ def test_create_lti_config_config_store(self, config_store):
         """
         location = 'block-v1:course+test+2020+type@problem+block@test'
         lti_version = LtiConfiguration.LTI_1P3
-        lti_config = _get_or_create_local_lti_config(
+        lti_config = _get_or_create_local_lti_xblock_config(
             lti_version=lti_version,
             block_location=location,
             config_store=config_store,
@@ -226,7 +226,7 @@ def test_external_config_values_are_cleared(self):
             external_id="test_plugin:test-id"
         )
 
-        _get_or_create_local_lti_config(
+        _get_or_create_local_lti_xblock_config(
             lti_version=lti_version,
             block_location=location,
             external_id=None

lti_consumer/tests/unit/test_lti_xblock.py

@@ -20,7 +20,7 @@
 from jwt.api_jwk import PyJWK, PyJWKSet
 from lti_consumer.exceptions import LtiError
 
-from lti_consumer.api import config_id_for_block
+from lti_consumer.api import config_for_block
 from lti_consumer.data import Lti1p3LaunchData
 from lti_consumer.lti_xblock import LtiConsumerXBlock, parse_handler_suffix, valid_config_type_values
 from lti_consumer.lti_1p3.tests.utils import create_jwt
@@ -1895,7 +1895,7 @@ def test_get_lti_1p3_launch_data(
         expected_launch_data_kwargs = {
             "user_id": 1,
             "user_role": "instructor",
-            "config_id": config_id_for_block(self.xblock),
+            "config_id": config_for_block(self.xblock),
             "resource_link_id": str(self.xblock.scope_ids.usage_id),
             "external_user_id": "external_user_id",
             "launch_presentation_document_target": "iframe",