diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 658c55a0e4e9..63aaf3d1e7eb 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -395,7 +395,7 @@ def get_library(library_key: LibraryLocatorV2) -> ContentLibraryMetadata: def create_library( - org: str, + org: Organization, slug: str, title: str, description: str = "", @@ -424,20 +424,19 @@ def create_library( validate_unicode_slug(slug) try: with transaction.atomic(): + learning_package = authoring_api.create_learning_package( + key=ContentLibrary.make_library_key(org.short_name, slug), + title=title, + description=description, + ) ref = ContentLibrary.objects.create( org=org, slug=slug, allow_public_learning=allow_public_learning, allow_public_read=allow_public_read, license=library_license, + learning_package=learning_package, ) - learning_package = authoring_api.create_learning_package( - key=str(ref.library_key), - title=title, - description=description, - ) - ref.learning_package = learning_package - ref.save() except IntegrityError: raise LibraryAlreadyExists(slug) # lint-amnesty, pylint: disable=raise-missing-from diff --git a/openedx/core/djangoapps/content_libraries/migrations/0012_alter_contentlibrary_learning_package.py b/openedx/core/djangoapps/content_libraries/migrations/0012_alter_contentlibrary_learning_package.py new file mode 100644 index 000000000000..e439f5b64559 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/migrations/0012_alter_contentlibrary_learning_package.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.23 on 2025-10-17 18:13 + +from django.db import migrations, models +import django.db.models.deletion + + +def remove_blockstore_libraries(apps, schema_editor): + """ + Remove old Blockstore-based libraries + + Content Libraries are always created with a LearningPackage these days, but + this model was once used for Blockstore-based libraries that did not use + LearningPackages at all. Very few instances ever used those libraries, and + those libraries will in a broken state anyway because Blockstore support has + long since been removed from edx-platform. + + We are deleting these ContentLibrary entries so that we can enforce that all + libraries going forward must specify a LearningPackage. + """ + ContentLibrary = apps.get_model("content_libraries", "ContentLibrary") + ContentLibrary.objects.filter(learning_package__isnull=True).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_libraries', '0011_remove_contentlibrary_bundle_uuid_and_more'), + ] + + operations = [ + migrations.RunPython(remove_blockstore_libraries, migrations.RunPython.noop), + migrations.AlterField( + model_name='contentlibrary', + name='learning_package', + field=models.OneToOneField(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.learningpackage'), + ), + ] diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 415821145605..941170e4cc56 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -109,11 +109,7 @@ class ContentLibrary(models.Model): # We can't delete the LearningPackage that holds a Library's content # unless we're deleting both at the same time. on_delete=models.RESTRICT, - # This is nullable mostly for backwards compatibility, though it should - # be possible to have the abstract notion of a Library with no actual - # content in it yet. - null=True, - default=None, + null=False, ) # How is this library going to be used? @@ -154,7 +150,17 @@ def library_key(self): """ Get the LibraryLocatorV2 opaque key for this library """ - return LibraryLocatorV2(org=self.org.short_name, slug=self.slug) + return self.make_library_key(org=self.org.short_name, slug=self.slug) + + @staticmethod + def make_library_key(org, slug): + """ + Generate a LibraryLocatorV2 for any library. + + This method exists mostly so that we can create the correct + LearningPackage key before the ContentLibrary exists. + """ + return LibraryLocatorV2(org=org, slug=slug) @property def allow_lti(self): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_models.py b/openedx/core/djangoapps/content_libraries/tests/test_models.py deleted file mode 100644 index eded47a305d2..000000000000 --- a/openedx/core/djangoapps/content_libraries/tests/test_models.py +++ /dev/null @@ -1,302 +0,0 @@ -""" -Unit tests for Content Libraries models. -""" - - -from unittest import mock - -from django.test import TestCase -from django.test import RequestFactory -from django.contrib.auth import get_user_model - -from pylti1p3.contrib.django.lti1p3_tool_config.models import LtiToolKey - -from organizations.models import Organization -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 - -from ..models import ALL_RIGHTS_RESERVED -from ..models import ContentLibrary -from ..models import LtiGradedResource -from ..models import LtiProfile -from ..models import LtiTool - - -class ContentLibraryTest(TestCase): - """ - Tests for ContentLibrary model. - """ - - def _create_library(self, **kwds): - """ - Create a library model, without a LearningPackage attached to it. - """ - org = Organization.objects.create(name='foo', short_name='foo') - return ContentLibrary.objects.create( - org=org, - slug='foobar', - allow_public_learning=False, - allow_public_read=False, - license=ALL_RIGHTS_RESERVED, - **kwds, - ) - - def test_authorize_lti_launch_when_no_library(self): - """ - Given no library - When authorize_lti_launch is called - Then return False - """ - self.assertFalse(ContentLibrary.objects.exists()) - authorized = ContentLibrary.authorize_lti_launch( - LibraryLocatorV2(org='foo', slug='foobar'), - issuer='http://a-fake-issuer', - client_id='a-fake-client-id') - self.assertFalse(authorized) - - def test_authorize_lti_launch_when_null(self): - """ - Given a library WITHOUT an lti tool set - When authorize_lti_launch is called - Then return False - """ - library = self._create_library() - authorized = ContentLibrary.authorize_lti_launch( - library.library_key, - issuer='http://a-fake-issuer', - client_id='a-fake-client-id') - self.assertFalse(authorized) - - def test_authorize_lti_launch_when_not_null(self): - """ - Given a library WITH an lti tool set - When authorize_lti_launch is called with different issuers - Then return False - """ - issuer = 'http://a-fake-issuer' - client_id = 'a-fake-client-id' - library = self._create_library() - library.authorized_lti_configs.add(LtiTool.objects.create( - issuer=issuer, - client_id=client_id, - tool_key=LtiToolKey.objects.create() - )) - authorized = ContentLibrary.authorize_lti_launch( - library.library_key, - issuer='http://another-fake-issuer', - client_id='another-fake-client-id') - self.assertFalse(authorized) - - def test_authorize_lti_launch_when_not_null_and_inactive(self): - """ - Given a library WITH an lti tool set - When authorize_lti_launch is called with the same issuers - And lti tool is inactive - Then return False - """ - issuer = 'http://a-fake-issuer' - client_id = 'a-fake-client-id' - library = self._create_library() - library.authorized_lti_configs.add(LtiTool.objects.create( - issuer=issuer, - client_id=client_id, - is_active=False, - tool_key=LtiToolKey.objects.create() - )) - authorized = ContentLibrary.authorize_lti_launch( - library.library_key, - issuer='http://another-fake-issuer', - client_id='another-fake-client-id') - self.assertFalse(authorized) - - def test_authorize_lti_launch_when_not_null_and_active(self): - """ - Given a library WITH an lti tool set - When authorize_lti_launch is called with the same issuers - And lti tool is active - Then return True - """ - issuer = 'http://a-fake-issuer' - client_id = 'a-fake-client-id' - library = self._create_library() - library.authorized_lti_configs.add(LtiTool.objects.create( - issuer=issuer, - client_id=client_id, - is_active=True, # redudant since it defaults to True - tool_key=LtiToolKey.objects.create() - )) - authorized = ContentLibrary.authorize_lti_launch( - library.library_key, - issuer=issuer, - client_id=client_id) - self.assertTrue(authorized) - - -class LtiProfileTest(TestCase): - """ - LtiProfile model tests. - """ - - def test_get_from_claims_doesnotexists(self): - with self.assertRaises(LtiProfile.DoesNotExist): - LtiProfile.objects.get_from_claims(iss='iss', aud='aud', sub='sub') - - def test_get_from_claims_exists(self): - """ - Given a LtiProfile with iss and sub, - When get_from_claims() - Then return the same object. - """ - - iss = 'http://foo.example.com/' - sub = 'randomly-selected-sub-for-testing' - aud = 'randomly-selected-aud-for-testing' - profile = LtiProfile.objects.create( - platform_id=iss, - client_id=aud, - subject_id=sub) - - queried_profile = LtiProfile.objects.get_from_claims( - iss=iss, aud=aud, sub=sub) - - self.assertEqual( - queried_profile, - profile, - 'The queried profile is equal to the profile created.') - - def test_subject_url(self): - """ - Given a profile - Then has a valid subject_url. - """ - iss = 'http://foo.example.com' - sub = 'randomly-selected-sub-for-testing' - aud = 'randomly-selected-aud-for-testing' - expected_url = 'http://foo.example.com/randomly-selected-aud-for-testing/randomly-selected-sub-for-testing' - profile = LtiProfile.objects.create( - platform_id=iss, - client_id=aud, - subject_id=sub) - self.assertEqual(expected_url, profile.subject_url) - - def test_create_with_user(self): - """ - Given a profile without a user - When save is called - Then a user is created. - """ - - iss = 'http://foo.example.com/' - sub = 'randomly-selected-sub-for-testing' - aud = 'randomly-selected-aud-for-testing' - profile = LtiProfile.objects.create( - platform_id=iss, - client_id=aud, - subject_id=sub) - self.assertIsNotNone(profile.user) - self.assertTrue( - profile.user.username.startswith('urn:openedx:content_libraries:username:')) - - def test_get_or_create_from_claims(self): - """ - Given a profile does not exist - When get or create - And get or create again - Then the same profile is returned. - """ - iss = 'http://foo.example.com/' - sub = 'randomly-selected-sub-for-testing' - aud = 'randomly-selected-aud-for-testing' - self.assertFalse(LtiProfile.objects.exists()) - profile = LtiProfile.objects.get_or_create_from_claims(iss=iss, aud=aud, sub=sub) - self.assertIsNotNone(profile.user) - self.assertEqual(iss, profile.platform_id) - self.assertEqual(sub, profile.subject_id) - - profile_two = LtiProfile.objects.get_or_create_from_claims(iss=iss, aud=aud, sub=sub) - self.assertEqual(profile_two, profile) - - def test_get_or_create_from_claims_twice(self): - """ - Given a profile - When another profile is created - Then success - """ - iss = 'http://foo.example.com/' - aud = 'randomly-selected-aud-for-testing' - sub_one = 'randomly-selected-sub-for-testing' - sub_two = 'another-randomly-sub-for-testing' - self.assertFalse(LtiProfile.objects.exists()) - LtiProfile.objects.get_or_create_from_claims(iss=iss, aud=aud, sub=sub_one) - LtiProfile.objects.get_or_create_from_claims(iss=iss, aud=aud, sub=sub_two) - - -class LtiResourceTest(TestCase): - """ - LtiGradedResource model tests. - """ - - iss = 'fake-iss-for-test' - - sub = 'fake-sub-for-test' - - aud = 'fake-aud-for-test' - - def setUp(self): - super().setUp() - self.request_factory = RequestFactory() - - def test_get_from_user_id_when_no_user_then_not_found(self): - user_id = 0 - with self.assertRaises(LtiGradedResource.DoesNotExist): - LtiGradedResource.objects.get_from_user_id(user_id) - - def test_get_from_user_id_when_no_profile_then_not_found(self): - user = get_user_model().objects.create(username='foobar') - with self.assertRaises(LtiGradedResource.DoesNotExist): - LtiGradedResource.objects.get_from_user_id(user.pk) - - def test_get_from_user_id_when_profile_then_found(self): - profile = LtiProfile.objects.get_or_create_from_claims( - iss=self.iss, aud=self.aud, sub=self.sub) - LtiGradedResource.objects.create(profile=profile) - resource = LtiGradedResource.objects.get_from_user_id(profile.user.pk) - self.assertEqual(profile, resource.profile) - - def test_upsert_from_ags_launch(self): - """ - Give no graded resource - When get_or_create_from_launch twice - Then created at first, retrieved at second. - """ - - resource_id = 'resource-foobar' - usage_key = 'lb:foo:bar:fooz:barz' - lineitem = 'http://canvas.docker/api/lti/courses/1/line_items/7' - resource_endpoint = { - "lineitem": lineitem, - "scope": [ - "https://purl.imsglobal.org/spec/lti-ags/scope/lineitem", - "https://purl.imsglobal.org/spec/lti-ags/scope/score" - ], - } - resource_link = { - "id": resource_id, - "title": "A custom title", - } - - profile = LtiProfile.objects.get_or_create_from_claims( - iss=self.iss, aud=self.aud, sub=self.sub) - block_mock = mock.Mock() - block_mock.scope_ids.usage_id = LibraryUsageLocatorV2.from_string(usage_key) - res = LtiGradedResource.objects.upsert_from_ags_launch( - profile.user, block_mock, resource_endpoint, resource_link) - - self.assertEqual(resource_id, res.resource_id) - self.assertEqual(lineitem, res.ags_lineitem) - self.assertEqual(usage_key, str(res.usage_key)) - self.assertEqual(profile, res.profile) - - res2 = LtiGradedResource.objects.upsert_from_ags_launch( - profile.user, block_mock, resource_endpoint, resource_link) - - self.assertEqual(res, res2)