Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""
Open edX Learning ("Learning Core").
"""
__version__ = "0.11.1"
__version__ = "0.11.2"
14 changes: 13 additions & 1 deletion openedx_learning/apps/authoring/collections/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
__all__ = [
"create_collection",
"get_collection",
"get_collections",
"get_learning_package_collections",
"update_collection",
]
Expand Down Expand Up @@ -77,4 +78,15 @@ def get_learning_package_collections(learning_package_id: int) -> QuerySet[Colle
"""
return Collection.objects \
.filter(learning_package_id=learning_package_id, enabled=True) \
.select_related("learning_package")
.select_related("learning_package") \
.order_by('pk')


def get_collections(enabled: bool | None = None) -> QuerySet[Collection]:
"""
Get all collections, optionally caller can filter by enabled flag
"""
qs = Collection.objects.all()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_components api method sorts its returned results by pk.. should we be doing that here too? It's nice to have APIs return things in a deterministic order (and it makes your tests simpler too).

I can't find an ADR which supports this stance though.. @ormsbee what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I guess if we sort these results, we should sort those from get_learning_package_collections too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pomegranited I agree about making tests simpler, as you can see I had to make use of self.assertQuerySetEqual as the order was not matching in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR to order by pk, we can remove the commit if @ormsbee does not agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally made it sort by pk as a cheap, lazy way of sorting it by creation time. But I think it's a reasonable default sort in a lot of situations, so this is fine by me.

if enabled is not None:
qs = qs.filter(enabled=enabled)
return qs.select_related("learning_package").order_by('pk')
42 changes: 42 additions & 0 deletions tests/openedx_learning/apps/authoring/collections/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class CollectionTestCase(TestCase):
Base-class for setting up commonly used test data.
"""
learning_package: LearningPackage
learning_package_2: LearningPackage
now: datetime

@classmethod
Expand All @@ -29,6 +30,10 @@ def setUpTestData(cls) -> None:
key="ComponentTestCase-test-key",
title="Components Test Case Learning Package",
)
cls.learning_package_2 = publishing_api.create_learning_package(
key="ComponentTestCase-test-key-2",
title="Components Test Case another Learning Package",
)
cls.now = datetime(2024, 8, 5, tzinfo=timezone.utc)


Expand All @@ -38,6 +43,7 @@ class GetCollectionTestCase(CollectionTestCase):
"""
collection1: Collection
collection2: Collection
collection3: Collection
disabled_collection: Collection

@classmethod
Expand All @@ -58,6 +64,12 @@ def setUpTestData(cls) -> None:
title="Collection 2",
description="Description of Collection 2",
)
cls.collection3 = collection_api.create_collection(
cls.learning_package_2.id,
created_by=None,
title="Collection 3",
description="Description of Collection 3",
)
cls.disabled_collection = collection_api.create_collection(
cls.learning_package.id,
created_by=None,
Expand Down Expand Up @@ -98,6 +110,36 @@ def test_get_invalid_learning_package_collections(self):
collections = collection_api.get_learning_package_collections(12345)
assert not list(collections)

def test_get_all_collections(self):
"""
Test getting all collections.
"""
collections = collection_api.get_collections()
self.assertQuerySetEqual(collections, [
self.collection1,
self.collection2,
self.collection3,
self.disabled_collection,
], ordered=True)

def test_get_all_enabled_collections(self):
"""
Test getting all ENABLED collections.
"""
collections = collection_api.get_collections(enabled=True)
self.assertQuerySetEqual(collections, [
self.collection1,
self.collection2,
self.collection3,
], ordered=True)

def test_get_all_disabled_collections(self):
"""
Test getting all DISABLED collections.
"""
collections = collection_api.get_collections(enabled=False)
assert list(collections) == [self.disabled_collection]


class CollectionCreateTestCase(CollectionTestCase):
"""
Expand Down