Store content object collections in search index [FC-0062] #35469

pomegranited · 2024-09-11T02:13:01Z

Description

This PR ensures that when a content object is added/removed from a Collection, its search index document is updated to reflect this change. The collection.key values for each collection are stored in a new collections list.

To support this, we added a new event called CONTENT_OBJECT_ASSOCIATIONS_CHANGED, and trigger it when an object's collections change. This event is also now triggered when an object's tags change. The previous CONTENT_OBJECT_TAGS_CHANGED event is still emitted for tag changes, but is now deprecated.

Useful information to include:

Which edX user roles will this change impact? Course Authors who have enabled Content Libraries V2.

Supporting information

Related Tickets:

Depends on / blocked by:

feat: Add Library Collections REST endpoints [FC-0062] #35321 -- see Store content object collections in search index [FC-0062] open-craft/edx-platform#680 for code review.

Testing instructions

Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin, plus ensure you are running with the latest versions of openedx-learning + openedx-events.
Run migrations: tutor dev run cms python manage.py migrate
In the content-authoring MFE, create a library, e.g. lib:SampleTaxonomyOrg1:AL1
Add some blocks to the library, and note their block keys.
Hint: you can retrieve the block keys using the REST API, e.g. http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/blocks/
Run tutor dev run cms ./manage.py cms reindex_studio --experimental to add the new collections field.
Run below snippet to add some collections -- reindexing should happen automatically.

# tutor dev run cms python manage.py cms shell
from opaque_keys.edx.keys import UsageKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx.core.djangoapps.content_libraries import api                                                                                                
lib_key_str = "lib:<your lib key>"
block_key_strs = [
    "lb:<your block key",
    "lb:<your block key",
    # ....
]

library_key = LibraryLocatorV2.from_string(lib_key_str)
block_keys = [
    UsageKey.from_string(key_str) for key_str in block_key_strs
]

api.create_library_collection(library_key, "FAL-3787", title="Collection FAL-3787")
api.update_library_collection_components(library_key, "FAL-3787", usage_keys=block_keys)
# <Collection> (lp:1 FAL-3787:Collection FAL-3787)

Open http://meilisearch.local.edly.io:7700/
Your api key can be found with tutor config printvalue MEILISEARCH_API_KEY
Search for FAL-3787 and verify that each of your blocks has FAL-3787 showing in its collections list.

Deadline

Before Sumac gets cut in October.

…#674) * chore: uses openedx-learning==0.11.3 * feat: add/remove components to/from a collection Sends a CONTENT_OBJECT_TAGS_CHANGED for each component added/removed. * docs: Add warning about unstable REST APIs * refactor: use oel_collections.get_collections as oel_collections.get_learning_package_collections has been removed. * test: fixes flaky collection search test * refactor: simplify the REST API params and validation

For the Collection "create" and "update" views, we call an authoring_api method and emit an event on success. This change simplifies the view code by moving these call to new content_libraries.api methods.

and refactors collection views to use DRF conventions.

They expect a UsageKey object, not the string object_id.

and ensures this new field is searchable and filterable. Serializes the object's collections to a list of collection.key values.

which adds CONTENT_OBJECT_ASSOCIATIONS_CHANGED

whenever a content object's tags or collections have changed, and handle that event in content/search. The deprecated CONTENT_OBJECT_TAGS_CHANGED event is still emitted when tags change; to be removed after Sumac.

…_TAGS_CHANGED in content_tagging app, while CONTENT_OBJECT_TAGS_CHANGED is being deprecated.

Collection.key is stored here, not ID

and re-raise as api.LibraryCollectionAlreadyExists

…tions-crud-rest-api

…ATIONS_CHANGED

and fixes event types documented in the hooks.

openedx-webhooks · 2024-09-11T02:13:05Z

Thanks for the pull request, @pomegranited!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

rpenido

Hi, @pomegranited!
Great work here! I added some comments, and as soon as they are addressed, this will be ready for CC review!

I tested this using the instructions from the PR
I read through the code
~~I checked for accessibility issues~~
Includes documentation

rpenido · 2024-09-11T13:22:34Z

openedx/core/djangoapps/content/search/documents.py

+
+    e.g. for something in Collections "COL_A" and "COL_B", this would return:
+        {
+            "collections": ["COL_A", "COL_B"],


@pomegranited Sorry for the late request (I overlooked this before), but what about changing the structure here?

Suggested change

"collections": ["COL_A", "COL_B"],

"collections": [

{ "display_name": "Collection A", key: "COL_A" },

{ "display_name": "Collection B", key: "COL_B" },

],

That way, we can use the display_name as a searchable attribute and the slug/key to a dev action (like a redirect).

Sure @rpenido , but I decided to store the collections data like we store the tags data, e.g.

"collections": { "display_name": ["Collection One", "Another Collection"], "key": ["collection-one", "another-key"], },

cf a81ea9a

rpenido · 2024-09-11T13:44:30Z

openedx/core/djangoapps/content/search/documents.py

+        ).values_list("key", flat=True)
+    except ObjectDoesNotExist:
+        log.warning(f"No component found for {object_id}")
+
+    if collections:
+        result[Fields.collections] = list(collections)


If the component is not in any collections, we must return an empty list to ensure the index is updated with this information.

I created a small PR here: open-craft#685

Done here: a81ea9a#diff-8c7bc1adff4ab3b98ae2dfca83b4d50407bb2be71d45de93e504552424703fe2R264

rpenido · 2024-09-11T14:10:27Z

openedx/core/djangoapps/content/search/tests/test_api.py

+        mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls(
+            [
+                call([doc_problem_with_collection2]),
+                call([doc_problem_with_collection1]),
+            ],
+            any_order=True,


I think we can have a more deterministic approach here to assert we don't have extra calls

Suggested change

mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls(

[

call([doc_problem_with_collection2]),

call([doc_problem_with_collection1]),

],

any_order=True,

self.assertEqual(

mock_meilisearch.return_value.index.return_value.update_documents.call_count,

2,

)

mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls(

[

call([doc_problem_with_collection2]),

call([doc_problem_with_collection1]),

],

Sure -- we still can't rely on the order these calls are made in, but I've added asserts for the call_count, e.g. a81ea9a#diff-f6238e12f564d80e4a90b6f385c31c1396de25784be34992c1f17d1c9ca557a1R353

rpenido · 2024-09-11T14:47:01Z

openedx/core/djangoapps/content/search/handlers.py

+    if "tags" in content_object.changes:
+        upsert_block_tags_index_docs(usage_key)
+    elif "collections" in content_object.changes:
+        upsert_block_collections_index_docs(usage_key)


Nit: If we have empty changes, we assume everything will change.
Also, we cannot use elif here, or we may skip the collecions update.

Suggested change

if "tags" in content_object.changes:

upsert_block_tags_index_docs(usage_key)

elif "collections" in content_object.changes:

upsert_block_collections_index_docs(usage_key)

if not content_object.changes or "tags" in content_object.changes:

upsert_block_tags_index_docs(usage_key)

if not content_object.changes or "collections" in content_object.changes:

upsert_block_collections_index_docs(usage_key)

See 360ec35

rpenido · 2024-09-11T14:49:35Z

openedx/core/djangoapps/content/search/documents.py

@@ -288,7 +334,7 @@ def searchable_doc_for_collection(collection) -> dict:
    found using faceted search.
    """
    doc = {
-        Fields.id: collection.id,
+        Fields.id: collection.key,


I think we are better with the id here. I fixed it in the other PR and add comments here: 1738447

#35321 is merged, so we can update this

We need the collection.key in order to call the REST APIs though -- I've re-added it in the internal PR: open-craft@5bdcc9e

Thank you for catching this @pomegranited!

ChrisChV

Looks good. Rebase this with the changes of #35321 and I will do another quick review

…llection

so we can index on the collection key or title.

pomegranited · 2024-09-12T06:00:17Z

@ChrisChV I think it's worth merging open-craft#684 into here first before we merge this change -- it contains a number of necessary fixes and additions.

…llection

to update the index before the frontend re-fetches

Store Collection metadata + component count in meilisearch

ChrisChV · 2024-09-13T00:44:40Z

@pomegranited I will merge this tomorrow morning

edx-pipeline-bot · 2024-09-13T14:47:21Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-09-13T17:18:11Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-09-13T17:18:12Z

2U Release Notice: This PR has been deployed to the edX production environment.

yusuf-musleh and others added 30 commits August 30, 2024 09:28

feat: Add Library Collections REST endpoints

ed30ce2

test: Add tests for Collections REST APIs

3d64ea4

chore: Add missing __init__ files

931f688

feat: Verify collection belongs to library

b2b38cb

feat: Add events emitting for Collections

a16b398

chore: fix pylint errors

be39d03

refactor: Use convert_exceptions + update tests

22fa791

refactor: Relocate convert_exceptions, remove utils

0bf0f78

refactor: Remove Collection handlers skeleton code

45cf886

refactor: Move Collections views/tests

366a7e9

refactor: pull view functionality into api

a2da207

For the Collection "create" and "update" views, we call an authoring_api method and emit an event on success. This change simplifies the view code by moving these call to new content_libraries.api methods.

temp: use temporary openedx-learning branch

aa3c1e3

refactor: use collection.key as ID in search records

6ae83ba

refactor: use Collection.key as identifier in content_libraries

abb1eb1

and refactors collection views to use DRF conventions.

test: adds tests for events raised by collections api

0eb52cf

temp: use temporary openedx-events branch

6f39767

fix: pass a ContentKey to the search doc handlers

bcc5cbb

They expect a UsageKey object, not the string object_id.

feat: add tags.collections to content object search index

0e32451

and ensures this new field is searchable and filterable. Serializes the object's collections to a list of collection.key values.

temp: use openedx-events temporary branch

3c6617e

which adds CONTENT_OBJECT_ASSOCIATIONS_CHANGED

feat: emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED

0c29552

whenever a content object's tags or collections have changed, and handle that event in content/search. The deprecated CONTENT_OBJECT_TAGS_CHANGED event is still emitted when tags change; to be removed after Sumac.

temp: use WIP openedx-events branch

e20ee24

test: fix test failing after changes to openedx-learning

9619341

fix: emit both CONTENT_OBJECT_ASSOCIATIONS_CHANGED and CONTENT_OBJECT…

1764e87

…_TAGS_CHANGED in content_tagging app, while CONTENT_OBJECT_TAGS_CHANGED is being deprecated.

docs: minor comment update

ae7926f

Collection.key is stored here, not ID

fix: catch IntegrityError in create_library_collection

007f80a

and re-raise as api.LibraryCollectionAlreadyExists

Merge remote-tracking branch 'origin/master' into yusuf-musleh/collec…

197733b

…tions-crud-rest-api

docs: replaces CONTENT_OBJECT_TAGS_CHANGED with CONTENT_OBJECT_ASSOCI…

436822e

…ATIONS_CHANGED

refactor: store collections outside of tags index

d517d80

chore: updates openedx-events==9.14.0

d312ff3

and fixes event types documented in the hooks.

pomegranited requested a review from ChrisChV September 11, 2024 02:13

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 11, 2024

pomegranited added 3 commits September 11, 2024 12:16

test: fixed failing test after refactor

bd44874

feat: index collections when created or updated

bfd548b

feat: adds num_children to collection index

0037530

pomegranited mentioned this pull request Sep 11, 2024

Store Collection metadata + component count in meilisearch open-craft/edx-platform#684

Merged

rpenido approved these changes Sep 11, 2024

View reviewed changes

ChrisChV reviewed Sep 11, 2024

View reviewed changes

rpenido mentioned this pull request Sep 11, 2024

fix: update index with empty collection list open-craft/edx-platform#685

Closed

pomegranited mentioned this pull request Sep 12, 2024

Store content object collections in search index [FC-0062] open-craft/edx-platform#680

Closed

pomegranited added 5 commits September 12, 2024 13:58

Merge branch 'master' into jill/collection-components-search

8a380a7

Merge branch 'jill/collection-components-search' into jill/reindex-co…

eb78583

…llection

fix: store collection.key in the document block_id field

5bdcc9e

fix: store library block collections as a dict in search index

a81ea9a

so we can index on the collection key or title.

fix: reindex tags + collections if no changes specified

360ec35

pomegranited and others added 3 commits September 12, 2024 15:37

Merge branch 'jill/collection-components-search' into jill/reindex-co…

ab25e2c

…llection

fix: apply library collection changes synchronously

2c6c8cf

to update the index before the frontend re-fetches

Merge pull request #684 from open-craft/jill/reindex-collection

79f83d7

Store Collection metadata + component count in meilisearch

pomegranited requested a review from ChrisChV September 12, 2024 23:57

ChrisChV approved these changes Sep 13, 2024

View reviewed changes

Merge branch 'master' into jill/collection-components-search

8d9f738

ChrisChV merged commit c41fe89 into openedx:master Sep 13, 2024
49 checks passed

ChrisChV deleted the jill/collection-components-search branch September 13, 2024 13:20

ormsbee mentioned this pull request Sep 27, 2024

XBlock asset support for new libraries experience (WIP) #35557

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store content object collections in search index [FC-0062] #35469

Store content object collections in search index [FC-0062] #35469

pomegranited commented Sep 11, 2024 •

edited

Loading

openedx-webhooks commented Sep 11, 2024

rpenido left a comment

rpenido Sep 11, 2024 •

edited

Loading

pomegranited Sep 12, 2024

rpenido Sep 11, 2024

pomegranited Sep 12, 2024

rpenido Sep 11, 2024

pomegranited Sep 12, 2024

rpenido Sep 11, 2024 •

edited

Loading

pomegranited Sep 12, 2024

rpenido Sep 11, 2024 •

edited

Loading

ChrisChV Sep 11, 2024

pomegranited Sep 12, 2024

rpenido Sep 12, 2024

ChrisChV left a comment

pomegranited commented Sep 12, 2024

ChrisChV commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

Store content object collections in search index [FC-0062] #35469

Store content object collections in search index [FC-0062] #35469

Conversation

pomegranited commented Sep 11, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Sep 11, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

rpenido left a comment

Choose a reason for hiding this comment

rpenido Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV left a comment

Choose a reason for hiding this comment

pomegranited commented Sep 12, 2024

ChrisChV commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

edx-pipeline-bot commented Sep 13, 2024

pomegranited commented Sep 11, 2024 •

edited

Loading

rpenido Sep 11, 2024 •

edited

Loading

rpenido Sep 11, 2024 •

edited

Loading

rpenido Sep 11, 2024 •

edited

Loading