Skip to content

Commit 8fc22cc

Browse files
mgwozdz-uniconCopilot
mgwozdz-unicon
and
Copilot
committed
fix: properly update studio search index when tags are deleted (#38477)
Co-authored-by: Copilot <[email protected]> (cherry picked from commit 8bcdd46)
1 parent 75522f7 commit 8fc22cc

3 files changed

Lines changed: 154 additions & 41 deletions

File tree

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -380,15 +380,23 @@ def searchable_doc_tags(object_id: OpaqueKey) -> dict:
380380
# if we used get_all_object_tags() to load all the tags for the library in a single query rather than loading the
381381
# tags for each component separately.
382382
all_tags = tagging_api.get_object_tags(str(object_id)).all()
383-
if not all_tags:
384-
# Clear out tags in the index when unselecting all tags for the block, otherwise
385-
# it would remain the last value if a cleared Fields.tags field is not included
386-
return {Fields.tags: {}}
387383
result = {
388384
Fields.tags_taxonomy: [],
389385
Fields.tags_level0: [],
390-
# ... other levels added as needed
386+
Fields.tags_level1: [],
387+
Fields.tags_level2: [],
388+
Fields.tags_level3: [],
391389
}
390+
if not all_tags:
391+
# Clear out tags in the index when the block has no tags (anymore)
392+
# Note: due to a bug in Meilisearch, just setting `{Fields.tags: {}}`
393+
# does not properly clear previously-set values within the tags field,
394+
# like tags.level0, so we explicitly set `{Field.tags: { level0: [], ... }}`
395+
# etc. to work around that and ensure tags are removed properly.
396+
# In the future, if Meili's bug is fixed, we can perhaps simplify this
397+
# and go back to just setting {Fields.tags: {}}` when there are no tags.
398+
return {Fields.tags: result}
399+
392400
for obj_tag in all_tags:
393401
# Add the taxonomy name:
394402
if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]:
@@ -594,8 +602,8 @@ def searchable_doc_for_container(
594602
found using faceted search.
595603
596604
If no container is found for the given container key, the returned document
597-
will contain only basic information derived from the container key, and no
598-
Fields.type value will be included in the returned dict.
605+
will contain only basic information derived from the container key, and some
606+
fields like Fields.display_name will be missing from the returned dict.
599607
"""
600608
doc = {
601609
Fields.id: meili_id_from_opaque_key(container_key),

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

Lines changed: 93 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@
3333
SearchAccess = {}
3434

3535
STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/"
36+
EMPTY_TAGS = {
37+
"taxonomy": [],
38+
"level0": [],
39+
"level1": [],
40+
"level2": [],
41+
"level3": [],
42+
}
3643

3744

3845
@ddt.ddt
@@ -342,29 +349,29 @@ def test_reindex_meilisearch(self, mock_meilisearch) -> None:
342349

343350
# Add tags field to doc, since reindex calls includes tags
344351
doc_sequential = copy.deepcopy(self.doc_sequential)
345-
doc_sequential["tags"] = {}
352+
doc_sequential["tags"] = copy.deepcopy(EMPTY_TAGS)
346353
doc_vertical = copy.deepcopy(self.doc_vertical)
347-
doc_vertical["tags"] = {}
354+
doc_vertical["tags"] = copy.deepcopy(EMPTY_TAGS)
348355
doc_problem1 = copy.deepcopy(self.doc_problem1)
349-
doc_problem1["tags"] = {}
356+
doc_problem1["tags"] = copy.deepcopy(EMPTY_TAGS)
350357
doc_problem1["collections"] = {'display_name': [], 'key': []}
351358
doc_problem1["units"] = {'display_name': [], 'key': []}
352359
doc_problem2 = copy.deepcopy(self.doc_problem2)
353-
doc_problem2["tags"] = {}
360+
doc_problem2["tags"] = copy.deepcopy(EMPTY_TAGS)
354361
doc_problem2["collections"] = {'display_name': [], 'key': []}
355362
doc_problem2["units"] = {'display_name': [], 'key': []}
356363
doc_collection = copy.deepcopy(self.collection_dict)
357-
doc_collection["tags"] = {}
364+
doc_collection["tags"] = copy.deepcopy(EMPTY_TAGS)
358365
doc_unit = copy.deepcopy(self.unit_dict)
359-
doc_unit["tags"] = {}
366+
doc_unit["tags"] = copy.deepcopy(EMPTY_TAGS)
360367
doc_unit["collections"] = {'display_name': [], 'key': []}
361368
doc_unit["subsections"] = {'display_name': ['Subsection 1'], 'key': ['lct:org1:lib:subsection:subsection-1']}
362369
doc_subsection = copy.deepcopy(self.subsection_dict)
363-
doc_subsection["tags"] = {}
370+
doc_subsection["tags"] = copy.deepcopy(EMPTY_TAGS)
364371
doc_subsection["collections"] = {'display_name': [], 'key': []}
365372
doc_subsection["sections"] = {'display_name': ['Section 1'], 'key': ['lct:org1:lib:section:section-1']}
366373
doc_section = copy.deepcopy(self.section_dict)
367-
doc_section["tags"] = {}
374+
doc_section["tags"] = copy.deepcopy(EMPTY_TAGS)
368375
doc_section["collections"] = {'display_name': [], 'key': []}
369376

370377
api.rebuild_index()
@@ -384,29 +391,29 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None:
384391

385392
# Add tags field to doc, since reindex calls includes tags
386393
doc_sequential = copy.deepcopy(self.doc_sequential)
387-
doc_sequential["tags"] = {}
394+
doc_sequential["tags"] = copy.deepcopy(EMPTY_TAGS)
388395
doc_vertical = copy.deepcopy(self.doc_vertical)
389-
doc_vertical["tags"] = {}
396+
doc_vertical["tags"] = copy.deepcopy(EMPTY_TAGS)
390397
doc_problem1 = copy.deepcopy(self.doc_problem1)
391-
doc_problem1["tags"] = {}
398+
doc_problem1["tags"] = copy.deepcopy(EMPTY_TAGS)
392399
doc_problem1["collections"] = {"display_name": [], "key": []}
393400
doc_problem1["units"] = {'display_name': [], 'key': []}
394401
doc_problem2 = copy.deepcopy(self.doc_problem2)
395-
doc_problem2["tags"] = {}
402+
doc_problem2["tags"] = copy.deepcopy(EMPTY_TAGS)
396403
doc_problem2["collections"] = {"display_name": [], "key": []}
397404
doc_problem2["units"] = {'display_name': [], 'key': []}
398405
doc_collection = copy.deepcopy(self.collection_dict)
399-
doc_collection["tags"] = {}
406+
doc_collection["tags"] = copy.deepcopy(EMPTY_TAGS)
400407
doc_unit = copy.deepcopy(self.unit_dict)
401-
doc_unit["tags"] = {}
408+
doc_unit["tags"] = copy.deepcopy(EMPTY_TAGS)
402409
doc_unit["collections"] = {"display_name": [], "key": []}
403410
doc_unit["subsections"] = {'display_name': ['Subsection 1'], 'key': ['lct:org1:lib:subsection:subsection-1']}
404411
doc_subsection = copy.deepcopy(self.subsection_dict)
405-
doc_subsection["tags"] = {}
412+
doc_subsection["tags"] = copy.deepcopy(EMPTY_TAGS)
406413
doc_subsection["collections"] = {'display_name': [], 'key': []}
407414
doc_subsection["sections"] = {'display_name': ['Section 1'], 'key': ['lct:org1:lib:section:section-1']}
408415
doc_section = copy.deepcopy(self.section_dict)
409-
doc_section["tags"] = {}
416+
doc_section["tags"] = copy.deepcopy(EMPTY_TAGS)
410417
doc_section["collections"] = {'display_name': [], 'key': []}
411418

412419
api.rebuild_index(incremental=True)
@@ -524,11 +531,11 @@ def test_reindex_meilisearch_library_block_error(self, mock_meilisearch) -> None
524531

525532
# Add tags field to doc, since reindex calls includes tags
526533
doc_sequential = copy.deepcopy(self.doc_sequential)
527-
doc_sequential["tags"] = {}
534+
doc_sequential["tags"] = copy.deepcopy(EMPTY_TAGS)
528535
doc_vertical = copy.deepcopy(self.doc_vertical)
529-
doc_vertical["tags"] = {}
536+
doc_vertical["tags"] = copy.deepcopy(EMPTY_TAGS)
530537
doc_problem2 = copy.deepcopy(self.doc_problem2)
531-
doc_problem2["tags"] = {}
538+
doc_problem2["tags"] = copy.deepcopy(EMPTY_TAGS)
532539
doc_problem2["collections"] = {'display_name': [], 'key': []}
533540
doc_problem2["units"] = {'display_name': [], 'key': []}
534541

@@ -611,14 +618,20 @@ def test_index_xblock_tags(self, mock_meilisearch) -> None:
611618
"id": self.doc_sequential["id"],
612619
"tags": {
613620
'taxonomy': ['A'],
614-
'level0': ['A > one', 'A > two']
621+
'level0': ['A > one', 'A > two'],
622+
'level1': [],
623+
'level2': [],
624+
'level3': [],
615625
}
616626
}
617627
doc_sequential_with_tags2 = {
618628
"id": self.doc_sequential["id"],
619629
"tags": {
620630
'taxonomy': ['A', 'B'],
621-
'level0': ['A > one', 'A > two', 'B > four', 'B > three']
631+
'level0': ['A > one', 'A > two', 'B > four', 'B > three'],
632+
'level1': [],
633+
'level2': [],
634+
'level3': [],
622635
}
623636
}
624637

@@ -631,6 +644,41 @@ def test_index_xblock_tags(self, mock_meilisearch) -> None:
631644
any_order=True,
632645
)
633646

647+
@override_settings(MEILISEARCH_ENABLED=True)
648+
def test_remove_xblock_tag_clears_index_tags(self, mock_meilisearch) -> None:
649+
"""
650+
Test that removing tags from an XBlock clears tag facets in the index.
651+
"""
652+
# Add a tag first so we can verify it is later cleared.
653+
tagging_api.tag_object(str(self.sequential.usage_key), self.taxonomyA, ["one"])
654+
655+
# Remove all tags for taxonomyA from this object.
656+
tagging_api.tag_object(str(self.sequential.usage_key), self.taxonomyA, [])
657+
658+
doc_with_tag = {
659+
"id": self.doc_sequential["id"],
660+
"tags": {
661+
"taxonomy": ["A"],
662+
"level0": ["A > one"],
663+
"level1": [],
664+
"level2": [],
665+
"level3": [],
666+
},
667+
}
668+
doc_without_tags = {
669+
"id": self.doc_sequential["id"],
670+
"tags": copy.deepcopy(EMPTY_TAGS),
671+
}
672+
673+
assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2
674+
mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls(
675+
[
676+
call([doc_with_tag]),
677+
call([doc_without_tags]),
678+
],
679+
any_order=True,
680+
)
681+
634682
@override_settings(MEILISEARCH_ENABLED=True)
635683
def test_delete_index_xblock(self, mock_meilisearch) -> None:
636684
"""
@@ -670,14 +718,20 @@ def test_index_library_block_tags(self, mock_meilisearch) -> None:
670718
"id": self.doc_problem1["id"],
671719
"tags": {
672720
'taxonomy': ['A'],
673-
'level0': ['A > one', 'A > two']
721+
'level0': ['A > one', 'A > two'],
722+
'level1': [],
723+
'level2': [],
724+
'level3': [],
674725
}
675726
}
676727
doc_problem_with_tags2 = {
677728
"id": self.doc_problem1["id"],
678729
"tags": {
679730
'taxonomy': ['A', 'B'],
680-
'level0': ['A > one', 'A > two', 'B > four', 'B > three']
731+
'level0': ['A > one', 'A > two', 'B > four', 'B > three'],
732+
'level1': [],
733+
'level2': [],
734+
'level3': [],
681735
}
682736
}
683737

@@ -876,14 +930,20 @@ def test_index_tags_in_collections(self, mock_meilisearch) -> None:
876930
"id": "lib-collectionorg1libmycol-5b647617",
877931
"tags": {
878932
'taxonomy': ['A'],
879-
'level0': ['A > one', 'A > two']
933+
'level0': ['A > one', 'A > two'],
934+
'level1': [],
935+
'level2': [],
936+
'level3': [],
880937
}
881938
}
882939
doc_collection_with_tags2 = {
883940
"id": "lib-collectionorg1libmycol-5b647617",
884941
"tags": {
885942
'taxonomy': ['A', 'B'],
886-
'level0': ['A > one', 'A > two', 'B > four', 'B > three']
943+
'level0': ['A > one', 'A > two', 'B > four', 'B > three'],
944+
'level1': [],
945+
'level2': [],
946+
'level3': [],
887947
}
888948
}
889949

@@ -1148,14 +1208,20 @@ def test_index_tags_in_containers(self, container_type, container_id, mock_meili
11481208
"id": container_id,
11491209
"tags": {
11501210
'taxonomy': ['A'],
1151-
'level0': ['A > one', 'A > two']
1211+
'level0': ['A > one', 'A > two'],
1212+
'level1': [],
1213+
'level2': [],
1214+
'level3': [],
11521215
}
11531216
}
11541217
doc_unit_with_tags2 = {
11551218
"id": container_id,
11561219
"tags": {
11571220
'taxonomy': ['A', 'B'],
1158-
'level0': ['A > one', 'A > two', 'B > four', 'B > three']
1221+
'level0': ['A > one', 'A > two', 'B > four', 'B > three'],
1222+
'level1': [],
1223+
'level2': [],
1224+
'level3': [],
11591225
}
11601226
}
11611227

0 commit comments

Comments
 (0)