From ef09229a7c7fb5b39507bbaefa379637cc5ea947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 9 Aug 2024 13:07:12 -0300 Subject: [PATCH 1/7] fix: improve collections models and api --- .../apps/authoring/collections/api.py | 30 +++++------ ...ion_name_collection_created_by_and_more.py | 53 +++++++++++++++++++ .../apps/authoring/collections/models.py | 51 ++++++++++++++---- .../apps/authoring/collections/test_api.py | 32 +++++------ 4 files changed, 124 insertions(+), 42 deletions(-) create mode 100644 openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index 229740a4c..79eb4d0b4 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -22,7 +22,7 @@ def create_collection( learning_package_id: int, - name: str, + title: str, description: str = "", ) -> Collection: """ @@ -30,7 +30,7 @@ def create_collection( """ collection = Collection.objects.create( learning_package_id=learning_package_id, - name=name, + title=title, description=description, ) return collection @@ -45,26 +45,26 @@ def get_collection(collection_id: int) -> Collection: def update_collection( collection_id: int, - name: str | None = None, + title: str | None = None, description: str | None = None, ) -> Collection: """ Update a Collection """ - lp = Collection.objects.get(id=collection_id) + collection = Collection.objects.get(id=collection_id) # If no changes were requested, there's nothing to update, so just return # the Collection as-is - if all(field is None for field in [name, description]): - return lp + if all(field is None for field in [title, description]): + return collection - if name is not None: - lp.name = name + if title is not None: + collection.title = title if description is not None: - lp.description = description + collection.description = description - lp.save() - return lp + collection.save() + return collection def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]: @@ -73,8 +73,6 @@ def get_learning_package_collections(learning_package_id: int) -> QuerySet[Colle Only enabled collections are returned """ - return ( - Collection.objects - .filter(learning_package_id=learning_package_id, enabled=True) - .select_related("learning_package") - ) + return Collection.objects \ + .filter(learning_package_id=learning_package_id, enabled=True) \ + .select_related("learning_package") diff --git a/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py b/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py new file mode 100644 index 000000000..2796623f3 --- /dev/null +++ b/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py @@ -0,0 +1,53 @@ +# Generated by Django 4.2.14 on 2024-08-09 16:05 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + +import openedx_learning.lib.fields +import openedx_learning.lib.validators + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('oel_collections', '0001_initial'), + ] + + operations = [ + migrations.RemoveField( + model_name='collection', + name='name', + ), + migrations.AddField( + model_name='collection', + name='created_by', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL), + ), + migrations.AddField( + model_name='collection', + name='title', + field=openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='Collection', help_text='The title of the collection.', max_length=500), + preserve_default=False, + ), + migrations.AlterField( + model_name='collection', + name='created', + field=models.DateTimeField(auto_now_add=True, validators=[openedx_learning.lib.validators.validate_utc_datetime]), + ), + migrations.AlterField( + model_name='collection', + name='description', + field=openedx_learning.lib.fields.MultiCollationTextField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', help_text='Provides extra information for the user about this collection.', max_length=10000), + ), + migrations.AlterField( + model_name='collection', + name='modified', + field=models.DateTimeField(auto_now=True, validators=[openedx_learning.lib.validators.validate_utc_datetime]), + ), + migrations.AddIndex( + model_name='collection', + index=models.Index(fields=['learning_package_id', 'title'], name='oel_collect_learnin_dfaf89_idx'), + ), + ] diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py index 412ce9d3d..4c032d3e1 100644 --- a/openedx_learning/apps/authoring/collections/models.py +++ b/openedx_learning/apps/authoring/collections/models.py @@ -3,10 +3,12 @@ """ from __future__ import annotations +from django.conf import settings from django.db import models from django.utils.translation import gettext_lazy as _ -from ....lib.fields import case_insensitive_char_field +from ....lib.fields import MultiCollationTextField, case_insensitive_char_field +from ....lib.validators import validate_utc_datetime from ..publishing.models import LearningPackage @@ -20,22 +22,29 @@ class Collection(models.Model): # Each collection belongs to a learning package learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) - name = case_insensitive_char_field( + title = case_insensitive_char_field( null=False, - max_length=255, - db_index=True, + blank=False, + max_length=500, help_text=_( - "The name of the collection." + "The title of the collection." ), ) - description = case_insensitive_char_field( - null=False, + + description = MultiCollationTextField( blank=True, + null=False, + default="", max_length=10_000, help_text=_( "Provides extra information for the user about this collection." ), + db_collations={ + "sqlite": "NOCASE", + "mysql": "utf8mb4_unicode_ci", + } ) + # We don't have api functions to handle the enabled field. This is a placeholder for future use and # a way to "soft delete" collections. enabled = models.BooleanField( @@ -44,11 +53,33 @@ class Collection(models.Model): "Whether the collection is enabled or not." ), ) - created = models.DateTimeField(auto_now_add=True) - modified = models.DateTimeField(auto_now=True) + + created_by = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.SET_NULL, + null=True, + blank=True, + ) + + created = models.DateTimeField( + auto_now_add=True, + validators=[ + validate_utc_datetime, + ], + ) + + modified = models.DateTimeField( + auto_now=True, + validators=[ + validate_utc_datetime, + ], + ) class Meta: verbose_name_plural = "Collections" + indexes = [ + models.Index(fields=["learning_package_id", "title"]), + ] def __repr__(self) -> str: """ @@ -60,4 +91,4 @@ def __str__(self) -> str: """ User-facing string representation of a Collection. """ - return f"<{self.__class__.__name__}> ({self.id}:{self.name})" + return f"<{self.__class__.__name__}> ({self.id}:{self.title})" diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index 893760fc9..9d0e2f7a0 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -45,17 +45,17 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection1 = collection_api.create_collection( cls.learning_package.id, - name="Collection 1", + title="Collection 1", description="Description of Collection 1", ) cls.collection2 = collection_api.create_collection( cls.learning_package.id, - name="Collection 2", + title="Collection 2", description="Description of Collection 2", ) cls.disabled_collection = collection_api.create_collection( cls.learning_package.id, - name="Disabled Collection", + title="Disabled Collection", description="Description of Disabled Collection", ) cls.disabled_collection.enabled = False @@ -106,11 +106,11 @@ def test_create_collection(self): with freeze_time(created_time): collection = collection_api.create_collection( self.learning_package.id, - name="My Collection", + title="My Collection", description="This is my collection", ) - assert collection.name == "My Collection" + assert collection.title == "My Collection" assert collection.description == "This is my collection" assert collection.enabled assert collection.created == created_time @@ -122,9 +122,9 @@ def test_create_collection_without_description(self): """ collection = collection_api.create_collection( self.learning_package.id, - name="My Collection", + title="My Collection", ) - assert collection.name == "My Collection" + assert collection.title == "My Collection" assert collection.description == "" assert collection.enabled @@ -142,37 +142,37 @@ def setUp(self) -> None: super().setUp() self.collection = collection_api.create_collection( self.learning_package.id, - name="Collection", + title="Collection", description="Description of Collection", ) def test_update_collection(self): """ - Test updating a collection's name and description. + Test updating a collection's title and description. """ modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = collection_api.update_collection( self.collection.pk, - name="New Name", + title="New Title", description="", ) - assert collection.name == "New Name" + assert collection.title == "New Title" assert collection.description == "" assert collection.modified == modified_time assert collection.created == self.collection.created # unchanged def test_update_collection_partial(self): """ - Test updating a collection's name. + Test updating a collection's title. """ collection = collection_api.update_collection( self.collection.pk, - name="New Name", + title="New Title", ) - assert collection.name == "New Name" + assert collection.title == "New Title" assert collection.description == self.collection.description # unchanged def test_update_collection_empty(self): @@ -185,7 +185,7 @@ def test_update_collection_empty(self): self.collection.pk, ) - assert collection.name == self.collection.name # unchanged + assert collection.title == self.collection.title # unchanged assert collection.description == self.collection.description # unchanged assert collection.modified == self.collection.modified # unchanged @@ -194,4 +194,4 @@ def test_update_collection_not_found(self): Test updating a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - collection_api.update_collection(12345, name="New Name") + collection_api.update_collection(12345, title="New Title") From fa03042afeec0d0b635ed2e37b22201a81a59dad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 14 Aug 2024 11:00:08 -0300 Subject: [PATCH 2/7] feat: export models --- openedx_learning/api/authoring_models.py | 1 + openedx_learning/apps/authoring/collections/models.py | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py index 142fe6a03..fb0ab669a 100644 --- a/openedx_learning/api/authoring_models.py +++ b/openedx_learning/api/authoring_models.py @@ -7,6 +7,7 @@ """ # These wildcard imports are okay because these modules declare __all__. # pylint: disable=wildcard-import +from ..apps.authoring.collections.models import * from ..apps.authoring.components.models import * from ..apps.authoring.contents.models import * from ..apps.authoring.publishing.model_mixins import * diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py index 4c032d3e1..e2ff1e83b 100644 --- a/openedx_learning/apps/authoring/collections/models.py +++ b/openedx_learning/apps/authoring/collections/models.py @@ -11,6 +11,10 @@ from ....lib.validators import validate_utc_datetime from ..publishing.models import LearningPackage +__all__ = [ + "Collection", +] + class Collection(models.Model): """ From 9417609aa58c94f5226ec8bd3722148d4c5d0445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 14 Aug 2024 11:21:54 -0300 Subject: [PATCH 3/7] fix: index field --- ...2_remove_collection_name_collection_created_by_and_more.py | 4 ++-- openedx_learning/apps/authoring/collections/models.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py b/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py index 2796623f3..eee95e509 100644 --- a/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py +++ b/openedx_learning/apps/authoring/collections/migrations/0002_remove_collection_name_collection_created_by_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.14 on 2024-08-09 16:05 +# Generated by Django 4.2.14 on 2024-08-14 14:20 import django.db.models.deletion from django.conf import settings @@ -48,6 +48,6 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='collection', - index=models.Index(fields=['learning_package_id', 'title'], name='oel_collect_learnin_dfaf89_idx'), + index=models.Index(fields=['learning_package', 'title'], name='oel_collect_learnin_dfaf89_idx'), ), ] diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py index e2ff1e83b..d52d11097 100644 --- a/openedx_learning/apps/authoring/collections/models.py +++ b/openedx_learning/apps/authoring/collections/models.py @@ -82,7 +82,7 @@ class Collection(models.Model): class Meta: verbose_name_plural = "Collections" indexes = [ - models.Index(fields=["learning_package_id", "title"]), + models.Index(fields=["learning_package", "title"]), ] def __repr__(self) -> str: From a30e04a39de540d9c094ce2b4932823e1e8fd78b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 19 Aug 2024 05:52:50 -0300 Subject: [PATCH 4/7] fix: add created_by parameter --- openedx_learning/apps/authoring/collections/api.py | 2 ++ .../openedx_learning/apps/authoring/collections/test_api.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index 79eb4d0b4..c7976f27c 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -23,6 +23,7 @@ def create_collection( learning_package_id: int, title: str, + created_by: int | None, description: str = "", ) -> Collection: """ @@ -31,6 +32,7 @@ def create_collection( collection = Collection.objects.create( learning_package_id=learning_package_id, title=title, + created_by=created_by, description=description, ) return collection diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index 9d0e2f7a0..feea898a6 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -45,16 +45,19 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection1 = collection_api.create_collection( cls.learning_package.id, + created_by=None, title="Collection 1", description="Description of Collection 1", ) cls.collection2 = collection_api.create_collection( cls.learning_package.id, + created_by=None, title="Collection 2", description="Description of Collection 2", ) cls.disabled_collection = collection_api.create_collection( cls.learning_package.id, + created_by=None, title="Disabled Collection", description="Description of Disabled Collection", ) @@ -107,6 +110,7 @@ def test_create_collection(self): collection = collection_api.create_collection( self.learning_package.id, title="My Collection", + created_by=None, description="This is my collection", ) @@ -122,6 +126,7 @@ def test_create_collection_without_description(self): """ collection = collection_api.create_collection( self.learning_package.id, + created_by=None, title="My Collection", ) assert collection.title == "My Collection" @@ -143,6 +148,7 @@ def setUp(self) -> None: self.collection = collection_api.create_collection( self.learning_package.id, title="Collection", + created_by=None, description="Description of Collection", ) From 87defd81601bff11c522747e6dbc3c5a837df29f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 20 Aug 2024 14:19:11 +0930 Subject: [PATCH 5/7] fix: assign to created_by_id instead of created_by because we're passing an int ID. Also adds a test. --- openedx_learning/apps/authoring/collections/api.py | 2 +- .../apps/authoring/collections/test_api.py | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index c7976f27c..0539dc336 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -32,7 +32,7 @@ def create_collection( collection = Collection.objects.create( learning_package_id=learning_package_id, title=title, - created_by=created_by, + created_by_id=created_by, description=description, ) return collection diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index feea898a6..d935e5b97 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -3,6 +3,7 @@ """ from datetime import datetime, timezone +from django.contrib.auth import get_user_model from django.core.exceptions import ObjectDoesNotExist from freezegun import freeze_time @@ -12,6 +13,8 @@ from openedx_learning.apps.authoring.publishing.models import LearningPackage from openedx_learning.lib.test_utils import TestCase +User = get_user_model() + class CollectionTestCase(TestCase): """ @@ -105,12 +108,16 @@ def test_create_collection(self): """ Test creating a collection. """ + user = User.objects.create( + username="user", + email="user@example.com", + ) created_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(created_time): collection = collection_api.create_collection( self.learning_package.id, title="My Collection", - created_by=None, + created_by=user.id, description="This is my collection", ) @@ -119,6 +126,7 @@ def test_create_collection(self): assert collection.enabled assert collection.created == created_time assert collection.modified == created_time + assert collection.created_by == user def test_create_collection_without_description(self): """ From a4540ecc65339932a4ecfd2f195abda53430e038 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 20 Aug 2024 14:23:15 +0930 Subject: [PATCH 6/7] test: small changes to improve test coverage for collections --- .../apps/authoring/collections/test_api.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index d935e5b97..cf3555412 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -188,6 +188,15 @@ def test_update_collection_partial(self): assert collection.title == "New Title" assert collection.description == self.collection.description # unchanged + assert f"{collection}" == f" ({self.collection.pk}:New Title)" + + collection = collection_api.update_collection( + self.collection.pk, + description="New description", + ) + + assert collection.title == "New Title" # unchanged + assert collection.description == "New description" def test_update_collection_empty(self): """ From 1c3fdece69d7892d70ce66c36ec2ed1ea220b73a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 20 Aug 2024 14:24:39 +0930 Subject: [PATCH 7/7] chore: bumps version to 0.11.1 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 9bf5b684e..354ad3724 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.11.0" +__version__ = "0.11.1"