From cf9ab01300a8ba30ab037355af46be1bdf7597b1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 8 Jul 2024 00:57:12 +0930 Subject: [PATCH 1/6] feat: adds sortable fields to studio content search index * new datetime fields added to library component serializer: created, modified, and last_published * new datetime fields are stored in Meilisearch as POSIX timestamps for sorting * Meilisearch search ranking updated to allow sorting by display_name or these new datetime fields * Updates tests to verify these new properties and client settings --- openedx/core/djangoapps/content/search/api.py | 18 ++++++++++ .../djangoapps/content/search/documents.py | 11 ++++++- .../djangoapps/content/search/handlers.py | 2 ++ .../content/search/tests/test_api.py | 33 +++++++++++++++++-- .../content/search/tests/test_handlers.py | 26 +++++++++++++-- .../core/djangoapps/content_libraries/api.py | 18 ++++++---- 6 files changed, 97 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 17824dbc2a7e..499eb2abd47a 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -340,6 +340,24 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, ]) + # Mark which attributes can be used for sorting search results: + client.index(temp_index_name).update_sortable_attributes([ + Fields.display_name, + Fields.created, + Fields.modified, + Fields.last_published, + ]) + + # Update the search ranking rules to let the (optional) "sort" parameter take precedence over keyword relevance. + # cf https://www.meilisearch.com/docs/learn/core_concepts/relevancy + client.index(temp_index_name).update_ranking_rules([ + "sort", + "words", + "typo", + "proximity", + "attribute", + "exactness", + ]) ############## Libraries ############## status_cb("Indexing libraries...") diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index dea494f312f0..032023f97c60 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -27,6 +27,9 @@ class Fields: type = "type" # DocType.course_block or DocType.library_block (see below) block_id = "block_id" # The block_id part of the usage key. Sometimes human-readable, sometimes a random hex ID display_name = "display_name" + modified = "modified" + created = "created" + last_published = "last_published" block_type = "block_type" context_key = "context_key" org = "org" @@ -221,6 +224,9 @@ def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetad Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, so that the given library block can be found using faceted search. + + Datetime fields (created, modified, last_published) are serialized to POSIX timestamps so that they can be used to + sort the search results. """ library_name = lib_api.get_library(xblock_metadata.usage_key.context_key).title block = xblock_api.load_block(xblock_metadata.usage_key, user=None) @@ -228,7 +234,10 @@ def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetad doc = { Fields.id: meili_id_from_opaque_key(xblock_metadata.usage_key), Fields.type: DocType.library_block, - Fields.breadcrumbs: [] + Fields.breadcrumbs: [], + Fields.created: xblock_metadata.created.timestamp(), + Fields.modified: xblock_metadata.modified.timestamp(), + Fields.last_published: xblock_metadata.last_published.timestamp() if xblock_metadata.last_published else None, } doc.update(_fields_from_block(block)) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 1a80b2215781..ba0e8c1a1680 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -12,6 +12,7 @@ CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, + LIBRARY_BLOCK_UPDATED, XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED, @@ -96,6 +97,7 @@ def xblock_deleted_handler(**kwargs) -> None: @receiver(LIBRARY_BLOCK_CREATED) +@receiver(LIBRARY_BLOCK_UPDATED) @only_if_meilisearch_enabled def library_block_updated_handler(**kwargs) -> None: """ diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 1c78b28506fe..ab697caf3740 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -5,11 +5,13 @@ import copy +from datetime import datetime, timezone from unittest.mock import MagicMock, call, patch from opaque_keys.edx.keys import UsageKey import ddt from django.test import override_settings +from freezegun import freeze_time from organizations.tests.factories import OrganizationFactory from common.djangoapps.student.tests.factories import UserFactory @@ -118,8 +120,16 @@ def setUp(self): title="Library", ) lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) - # Populate it with a problem: - self.problem = library_api.create_library_block(self.library.key, "problem", "p1") + + # Populate it with a problem, freezing the date so we can verify created date serializes correctly. + created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(created_date): + self.problem = library_api.create_library_block(self.library.key, "problem", "p1") + # Update the problem, freezing the date so we can verify modified date serializes correctly. + modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) + with freeze_time(modified_date): + library_api.set_library_block_olx(self.problem.usage_key, "") + self.doc_problem = { "id": "lborg1libproblemp1-a698218e", "usage_key": "lb:org1:lib:problem:p1", @@ -132,6 +142,9 @@ def setUp(self): "content": {"problem_types": [], "capa_content": " "}, "type": "library_block", "access_id": lib_access.id, + "last_published": None, + "created": created_date.timestamp(), + "modified": modified_date.timestamp(), } # Create a couple of taxonomies with tags @@ -171,6 +184,22 @@ def test_reindex_meilisearch(self, mock_meilisearch): any_order=True, ) + # Check that the sorting-related settings were updated to support sorting on the expected fields + mock_meilisearch.return_value.index.return_value.update_sortable_attributes.assert_called_with([ + "display_name", + "created", + "modified", + "last_published", + ]) + mock_meilisearch.return_value.index.return_value.update_ranking_rules.assert_called_with([ + "sort", + "words", + "typo", + "proximity", + "attribute", + "exactness", + ]) + @ddt.data( True, False diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 1ce9c57a1ab9..8a6627e3902d 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -1,9 +1,11 @@ """ Tests for the search index update handlers """ +from datetime import datetime, timezone from unittest.mock import MagicMock, patch from django.test import LiveServerTestCase, override_settings +from freezegun import freeze_time from organizations.tests.factories import OrganizationFactory from common.djangoapps.student.tests.factories import UserFactory @@ -132,7 +134,10 @@ def test_create_delete_library_block(self, meilisearch_client): ) lib_access, _ = SearchAccess.objects.get_or_create(context_key=library.key) - problem = library_api.create_library_block(library.key, "problem", "Problem1") + # Populate it with a problem, freezing the date so we can verify created date serializes correctly. + created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(created_date): + problem = library_api.create_library_block(library.key, "problem", "Problem1") doc_problem = { "id": "lborgalib_aproblemproblem1-ca3186e9", "type": "library_block", @@ -145,6 +150,9 @@ def test_create_delete_library_block(self, meilisearch_client): "breadcrumbs": [{"display_name": "Library Org A"}], "content": {"problem_types": [], "capa_content": " "}, "access_id": lib_access.id, + "last_published": None, + "created": created_date.timestamp(), + "modified": created_date.timestamp(), } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) @@ -152,10 +160,24 @@ def test_create_delete_library_block(self, meilisearch_client): # Rename the content library library_api.update_library(library.key, title="Updated Library Org A") - # The breadcrumbs should be updated + # The breadcrumbs should be updated (but nothing else) doc_problem["breadcrumbs"][0]["display_name"] = "Updated Library Org A" meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) + # Edit the problem block, freezing the date so we can verify modified date serializes correctly + modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) + with freeze_time(modified_date): + library_api.set_library_block_olx(problem.usage_key, "") + doc_problem["modified"] = modified_date.timestamp() + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) + + # Publish the content library, freezing the date so we can verify last_published date serializes correctly + published_date = datetime(2024, 6, 7, 8, 9, 10, tzinfo=timezone.utc) + with freeze_time(published_date): + library_api.publish_changes(library.key) + doc_problem["last_published"] = published_date.timestamp() + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) + # Delete the Library Block library_api.delete_library_block(problem.usage_key) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 3800fdb7f4f4..bf3ba78e1fa0 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -194,7 +194,10 @@ class LibraryXBlockMetadata: Class that represents the metadata about an XBlock in a content library. """ usage_key = attr.ib(type=LibraryUsageLocatorV2) + created = attr.ib(type=datetime) + modified = attr.ib(type=datetime) display_name = attr.ib("") + last_published = attr.ib(default=None, type=datetime) has_unpublished_changes = attr.ib(False) tags_count = attr.ib(0) @@ -203,6 +206,8 @@ def from_component(cls, library_key, component): """ Construct a LibraryXBlockMetadata from a Component object. """ + last_publish_log = authoring_api.get_last_publish(component.pk) + return cls( usage_key=LibraryUsageLocatorV2( library_key, @@ -210,6 +215,9 @@ def from_component(cls, library_key, component): component.local_key, ), display_name=component.versioning.draft.title, + created=component.created, + modified=component.versioning.latest.created, + last_published=None if last_publish_log is None else last_publish_log.published_at, has_unpublished_changes=component.versioning.has_unpublished_changes ) @@ -660,13 +668,11 @@ def get_library_block(usage_key) -> LibraryXBlockMetadata: if not draft_version: raise ContentLibraryBlockNotFound(usage_key) - published_version = component.versioning.published - - return LibraryXBlockMetadata( - usage_key=usage_key, - display_name=draft_version.title, - has_unpublished_changes=(draft_version != published_version), + xblock_metadata = LibraryXBlockMetadata.from_component( + library_key=usage_key.context_key, + component=component, ) + return xblock_metadata def set_library_block_olx(usage_key, new_olx_str): From 5f71cc3074dfd6024079b06a2279ab6208065a0d Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 9 Jul 2024 21:35:53 +0930 Subject: [PATCH 2/6] fix: reduce queries made when serializing library block metadata * Instead of using `latest.modified` (which always queries the database), use `draft.latest` * However, we have to increase the query count by 1 query per block, because the published_at date can only be found in the publish log, which must be fetched individually for each block. --- openedx/core/djangoapps/content_libraries/api.py | 2 +- .../content_tagging/tests/test_objecttag_export_helpers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index bf3ba78e1fa0..888452c89028 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -216,7 +216,7 @@ def from_component(cls, library_key, component): ), display_name=component.versioning.draft.title, created=component.created, - modified=component.versioning.latest.created, + modified=component.versioning.draft.created, last_published=None if last_publish_log is None else last_publish_log.published_at, has_unpublished_changes=component.versioning.has_unpublished_changes ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py index d3306844ac40..f84ad4e6df72 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py @@ -441,7 +441,7 @@ def test_build_library_object_tree(self) -> None: """ Test if we can export a library """ - with self.assertNumQueries(8): + with self.assertNumQueries(11): tagged_library = build_object_tree_with_objecttags(self.library.key, self.all_library_object_tags) assert tagged_library == self.expected_library_tagged_xblock From b7f8f8e69916a11420de7bdfaea6f88258be2710 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jul 2024 22:55:21 +0930 Subject: [PATCH 3/6] feat: raise LIBRARY_BLOCK_UPDATED in the xblock edit REST API --- .../core/djangoapps/xblock/rest_api/views.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 3722d9d8ab15..a9059ec30b29 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -21,6 +21,10 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from openedx_events.content_authoring.data import LibraryBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED + from openedx.core.lib.api.view_utils import view_auth_classes from ..api import ( get_block_metadata, @@ -254,6 +258,20 @@ def post(self, request, usage_key_str): # Save after the callback so any changes made in the callback will get persisted. block.save() + # Raise a signal to indicate the library block was updated + try: + library_block_key = LibraryUsageLocatorV2.from_string(usage_key_str) + LIBRARY_BLOCK_UPDATED.send_event( + library_block=LibraryBlockData( + library_key=library_block_key.context_key, + usage_key=library_block_key, + ) + ) + + except InvalidKeyError: + # TODO raise XBLOCK_UPDATED if this API is ever used for course blocks. + pass + return Response({ "id": str(block.location), "data": data, From 74a7ac82a5ebcb11368bf4cb30bb4a922e0d6c85 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 24 Jul 2024 00:53:22 +0930 Subject: [PATCH 4/6] fix: allow content search to filter on last_modified --- openedx/core/djangoapps/content/search/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 499eb2abd47a..ee0e5811c8b6 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -322,6 +322,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level3, Fields.type, Fields.access_id, + Fields.last_published, ]) # Mark which attributes are used for keyword search, in order of importance: client.index(temp_index_name).update_searchable_attributes([ From cc6739581ee7fc54aa78c13127dad291dfaec1c1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 26 Jul 2024 02:21:11 +0930 Subject: [PATCH 5/6] refactor: use LearningContext to send "block updated" event --- .../content_libraries/library_context.py | 16 ++++++++++++++ .../learning_context/learning_context.py | 7 ++++++ .../core/djangoapps/xblock/rest_api/views.py | 22 +++++-------------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index 2607c18df7e4..6ff426e73560 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -6,6 +6,9 @@ from django.core.exceptions import PermissionDenied +from openedx_events.content_authoring.data import LibraryBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED + from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.models import ContentLibrary from openedx.core.djangoapps.xblock.api import LearningContext @@ -93,3 +96,16 @@ def block_exists(self, usage_key): type_name=usage_key.block_type, local_key=usage_key.block_id, ) + + def send_block_updated_event(self, usage_key): + """ + Send a "block updated" event for the library block with the given usage_key. + + usage_key: the UsageKeyV2 subclass used for this learning context + """ + LIBRARY_BLOCK_UPDATED.send_event( + library_block=LibraryBlockData( + library_key=usage_key.lib_key, + usage_key=usage_key, + ) + ) diff --git a/openedx/core/djangoapps/xblock/learning_context/learning_context.py b/openedx/core/djangoapps/xblock/learning_context/learning_context.py index 1ac621ef244f..2dc5155dc4e2 100644 --- a/openedx/core/djangoapps/xblock/learning_context/learning_context.py +++ b/openedx/core/djangoapps/xblock/learning_context/learning_context.py @@ -58,3 +58,10 @@ def definition_for_usage(self, usage_key, **kwargs): Retuns None if the usage key doesn't exist in this context. """ raise NotImplementedError + + def send_block_updated_event(self, usage_key): + """ + Send a "block updated" event for the block with the given usage_key in this context. + + usage_key: the UsageKeyV2 subclass used for this learning context + """ diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index a9059ec30b29..7741d4144e4c 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -21,10 +21,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryUsageLocatorV2 -from openedx_events.content_authoring.data import LibraryBlockData -from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED - +from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.lib.api.view_utils import view_auth_classes from ..api import ( get_block_metadata, @@ -258,19 +255,10 @@ def post(self, request, usage_key_str): # Save after the callback so any changes made in the callback will get persisted. block.save() - # Raise a signal to indicate the library block was updated - try: - library_block_key = LibraryUsageLocatorV2.from_string(usage_key_str) - LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=library_block_key.context_key, - usage_key=library_block_key, - ) - ) - - except InvalidKeyError: - # TODO raise XBLOCK_UPDATED if this API is ever used for course blocks. - pass + # Signal that we've modified this block + usage_key = UsageKey.from_string(usage_key_str) + context_impl = get_learning_context_impl(usage_key) + context_impl.send_updated_event(usage_key) return Response({ "id": str(block.location), From ceaf3142d5599796b87505ac418d69283fd9fe68 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 26 Jul 2024 03:10:36 +0930 Subject: [PATCH 6/6] fix: nit, don't redeclare usage_key --- openedx/core/djangoapps/xblock/rest_api/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 7741d4144e4c..501386efba38 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -256,7 +256,6 @@ def post(self, request, usage_key_str): block.save() # Signal that we've modified this block - usage_key = UsageKey.from_string(usage_key_str) context_impl = get_learning_context_impl(usage_key) context_impl.send_updated_event(usage_key)