-
Notifications
You must be signed in to change notification settings - Fork 26
feat: add Collection model and api #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bd78879
28a96f9
29c0b29
370e73a
d6cdf08
ba04b4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| """ | ||
| Open edX Learning ("Learning Core"). | ||
| """ | ||
| __version__ = "0.10.1" | ||
| __version__ = "0.11.0" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| """ | ||
| Collections API (warning: UNSTABLE, in progress API) | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| from django.db.models import QuerySet | ||
|
|
||
| from .models import Collection | ||
|
|
||
| # The public API that will be re-exported by openedx_learning.apps.authoring.api | ||
| # is listed in the __all__ entries below. Internal helper functions that are | ||
| # private to this module should start with an underscore. If a function does not | ||
| # start with an underscore AND it is not in __all__, that function is considered | ||
| # to be callable only by other apps in the authoring package. | ||
| __all__ = [ | ||
| "create_collection", | ||
| "get_collection", | ||
| "get_learning_package_collections", | ||
| "update_collection", | ||
| ] | ||
|
|
||
|
|
||
| def create_collection( | ||
| learning_package_id: int, | ||
| name: str, | ||
| description: str = "", | ||
| ) -> Collection: | ||
| """ | ||
| Create a new Collection | ||
| """ | ||
| collection = Collection.objects.create( | ||
| learning_package_id=learning_package_id, | ||
| name=name, | ||
| description=description, | ||
| ) | ||
| return collection | ||
|
|
||
|
|
||
| def get_collection(collection_id: int) -> Collection: | ||
| """ | ||
| Get a Collection by ID | ||
| """ | ||
| return Collection.objects.get(id=collection_id) | ||
|
|
||
|
|
||
| def update_collection( | ||
| collection_id: int, | ||
| name: str | None = None, | ||
| description: str | None = None, | ||
| ) -> Collection: | ||
| """ | ||
| Update a Collection | ||
| """ | ||
| lp = 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 name is not None: | ||
| lp.name = name | ||
| if description is not None: | ||
| lp.description = description | ||
|
|
||
| lp.save() | ||
| return lp | ||
|
|
||
|
|
||
| def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]: | ||
| """ | ||
| Get all collections for a given learning package | ||
|
|
||
| Only enabled collections are returned | ||
| """ | ||
| return ( | ||
| Collection.objects | ||
| .filter(learning_package_id=learning_package_id, enabled=True) | ||
| .select_related("learning_package") | ||
|
Comment on lines
+78
to
+79
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'm a little surprised the linter didn't error on this, since I thought it was pretty fussy about indentation, e.g. Collection.objects \
.filter(learning_package_id=learning_package_id, enabled=True) \
.select_related("learning_package") |
||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| """ | ||
| Django metadata for the Collections Django application. | ||
| """ | ||
| from django.apps import AppConfig | ||
|
|
||
|
|
||
| class CollectionsConfig(AppConfig): | ||
| """ | ||
| Configuration for the Collections Django application. | ||
| """ | ||
|
|
||
| name = "openedx_learning.apps.authoring.collections" | ||
| verbose_name = "Learning Core > Authoring > Collections" | ||
| default_auto_field = "django.db.models.BigAutoField" | ||
| label = "oel_collections" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # Generated by Django 4.2.14 on 2024-08-05 20:42 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
|
|
||
| import openedx_learning.lib.fields | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| initial = True | ||
|
|
||
| dependencies = [ | ||
| ('oel_publishing', '0002_alter_learningpackage_key_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='Collection', | ||
| fields=[ | ||
| ('id', models.AutoField(primary_key=True, serialize=False)), | ||
| ('name', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, db_index=True, help_text='The name of the collection.', max_length=255)), | ||
| ('description', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, help_text='Provides extra information for the user about this collection.', max_length=10000)), | ||
| ('enabled', models.BooleanField(default=True, help_text='Whether the collection is enabled or not.')), | ||
| ('created', models.DateTimeField(auto_now_add=True)), | ||
| ('modified', models.DateTimeField(auto_now=True)), | ||
| ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), | ||
| ], | ||
| options={ | ||
| 'verbose_name_plural': 'Collections', | ||
| }, | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| """ | ||
| Core models for Collections | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| from django.db import models | ||
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
| from ....lib.fields import case_insensitive_char_field | ||
| from ..publishing.models import LearningPackage | ||
|
|
||
|
|
||
| class Collection(models.Model): | ||
| """ | ||
| Represents a collection of library components | ||
| """ | ||
|
|
||
| id = models.AutoField(primary_key=True) | ||
|
|
||
| # Each collection belongs to a learning package | ||
| learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) | ||
|
pomegranited marked this conversation as resolved.
|
||
|
|
||
| name = case_insensitive_char_field( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
| null=False, | ||
| max_length=255, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again for consistency, please use the same numbers as LearningPackage for now (500). The historical reason for 255 was because that was the largest indexable value when using |
||
| db_index=True, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please put all indexes in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, while it's fine to have a global index on the Collection titles, the most common sorting case is going to be to order by name, scoped to a particular LearningPackage. So you'll want to make a compound index on the combination of |
||
| help_text=_( | ||
| "The name of the collection." | ||
| ), | ||
| ) | ||
| description = case_insensitive_char_field( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're intending this to be long, it should be a text field, not a char field. There's no helper for this in Also, MySQL indexing limits means that even if you specify 10K as the max length here, only the first 767 characters will be guaranteed to be indexable as a varchar. We can just turn this off altogether and not make the name searchable at the database layer (relying on dedicated search later). |
||
| null=False, | ||
| blank=True, | ||
| max_length=10_000, | ||
| help_text=_( | ||
| "Provides extra information for the user about this collection." | ||
| ), | ||
| ) | ||
| # 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( | ||
| default=True, | ||
| help_text=_( | ||
| "Whether the collection is enabled or not." | ||
| ), | ||
| ) | ||
| created = models.DateTimeField(auto_now_add=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition to when it was created, we need to track who it was created by (which should be nullable in case the system created it or the user is deleted). We have |
||
| modified = models.DateTimeField(auto_now=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of Learning Core uses manually set datetimes (no auto), but I think it's okay to use here because Collections are created differently (e.g. you're not going to have Publish operation that creates a Collections in bulk). But please do add the |
||
|
|
||
| class Meta: | ||
| verbose_name_plural = "Collections" | ||
|
|
||
| def __repr__(self) -> str: | ||
| """ | ||
| Developer-facing representation of a Collection. | ||
| """ | ||
| return str(self) | ||
|
|
||
| def __str__(self) -> str: | ||
| """ | ||
| User-facing string representation of a Collection. | ||
| """ | ||
| return f"<{self.__class__.__name__}> ({self.id}:{self.name})" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| Collections App | ||
| ============== | ||
|
|
||
| The ``collections`` app will enable content authors to group content together for | ||
| organizing and re-using content. Components can be part of several different collections. | ||
|
|
||
| Motivation | ||
| ---------- | ||
|
|
||
| With the Legacy Libraries ("v1"), people didn't have any way to organize content in libraries, so they | ||
| had to create many small libraries. | ||
|
|
||
| For the Libraries Relaunch ("v2"), we want to encourage people to create large libraries with lots of content, | ||
| and organize that content using tags and collections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this variable
lp? LearningPackage?