Skip to content

Commit 33a5bce

Browse files
mbertrandclaude
andcommitted
Fix course blocklist exclusion to use __in lookup in index/embed tasks
The recreate-index and embed tasks excluded blocklisted courses with an exact-match lookup against a list (readable_id=blocklisted_ids), which CharField stringifies into a value that matches nothing — so the exclusion was a silent no-op. Use readable_id__in like the other call sites, and add tests that fail against the old lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8f26840 commit 33a5bce

4 files changed

Lines changed: 102 additions & 5 deletions

File tree

learning_resources_search/tasks.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,7 @@ def start_recreate_index(self, indexes, remove_existing_reindexing_tags):
624624
)
625625
for ids in chunks(
626626
Course.objects.filter(learning_resource__published=True)
627-
.exclude(learning_resource__readable_id=blocklisted_ids)
627+
.exclude(learning_resource__readable_id__in=blocklisted_ids)
628628
.order_by("learning_resource_id")
629629
.values_list("learning_resource_id", flat=True),
630630
chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE,
@@ -634,7 +634,7 @@ def start_recreate_index(self, indexes, remove_existing_reindexing_tags):
634634
for course in (
635635
Course.objects.filter(learning_resource__published=True)
636636
.filter(learning_resource__etl_source__in=RESOURCE_FILE_ETL_SOURCES)
637-
.exclude(learning_resource__readable_id=blocklisted_ids)
637+
.exclude(learning_resource__readable_id__in=blocklisted_ids)
638638
.order_by("learning_resource_id")
639639
):
640640
index_tasks = index_tasks + [
@@ -668,7 +668,7 @@ def start_recreate_index(self, indexes, remove_existing_reindexing_tags):
668668
LearningResource.objects.filter(
669669
published=True,
670670
)
671-
.exclude(readable_id=blocklisted_ids)
671+
.exclude(readable_id__in=blocklisted_ids)
672672
.order_by("id")
673673
.values_list("id", flat=True),
674674
chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE,

learning_resources_search/tasks_test.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,59 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9
285285
assert mocked_celery.replace.call_args[0][1] == mocked_celery.chain.return_value
286286

287287

288+
@pytest.mark.parametrize("indexes", [["course"], ["combined_hybrid"]])
289+
def test_start_recreate_index_excludes_blocklisted_courses(
290+
mocker, mocked_celery, indexes
291+
):
292+
"""start_recreate_index should not index courses whose readable_id is blocklisted"""
293+
courses = CourseFactory.create_batch(3, etl_source=ETLSource.ocw.value)
294+
blocked = courses[0]
295+
for course in courses:
296+
ContentFileFactory.create_batch(2, run=course.learning_resource.runs.first())
297+
mocker.patch(
298+
"learning_resources_search.tasks.load_course_blocklist",
299+
return_value=[blocked.learning_resource.readable_id],
300+
)
301+
index_learning_resources_mock = mocker.patch(
302+
"learning_resources_search.tasks.index_learning_resources", autospec=True
303+
)
304+
index_files_mock = mocker.patch(
305+
"learning_resources_search.tasks.index_content_files", autospec=True
306+
)
307+
mocker.patch(
308+
"learning_resources_search.indexing_api.create_backing_index",
309+
autospec=True,
310+
return_value="backing",
311+
)
312+
mocker.patch(
313+
"learning_resources_search.indexing_api.get_existing_reindexing_indexes",
314+
autospec=True,
315+
return_value=[],
316+
)
317+
mocker.patch(
318+
"learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True
319+
)
320+
mocker.patch("learning_resources_search.tasks.finish_recreate_index", autospec=True)
321+
322+
with pytest.raises(mocked_celery.replace_exception_class):
323+
start_recreate_index.delay(indexes, remove_existing_reindexing_tags=False)
324+
325+
indexed_resource_ids = {
326+
resource_id
327+
for call in index_learning_resources_mock.si.call_args_list
328+
for resource_id in call.args[0]
329+
}
330+
assert indexed_resource_ids == {
331+
course.learning_resource_id for course in courses[1:]
332+
}
333+
334+
if COURSE_TYPE in indexes:
335+
file_course_ids = {call.args[1] for call in index_files_mock.si.call_args_list}
336+
assert file_course_ids == {
337+
course.learning_resource_id for course in courses[1:]
338+
}
339+
340+
288341
@pytest.mark.parametrize(
289342
"remove_existing_reindexing_tags",
290343
[True, False],

vector_search/tasks.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ def start_embed_resources(self, indexes, skip_content_files, overwrite): # noqa
248248
generate_embeddings.si(ids, COURSE_TYPE, overwrite)
249249
for ids in chunks(
250250
Course.objects.filter(learning_resource__published=True)
251-
.exclude(learning_resource__readable_id=blocklisted_ids)
251+
.exclude(learning_resource__readable_id__in=blocklisted_ids)
252252
.order_by("learning_resource_id")
253253
.values_list("learning_resource_id", flat=True),
254254
chunk_size=settings.QDRANT_CHUNK_SIZE,
@@ -261,7 +261,7 @@ def start_embed_resources(self, indexes, skip_content_files, overwrite): # noqa
261261
resource_type=COURSE_TYPE,
262262
)
263263
.filter(Q(published=True) | Q(test_mode=True))
264-
.exclude(readable_id=blocklisted_ids)
264+
.exclude(readable_id__in=blocklisted_ids)
265265
.order_by("id")
266266
):
267267
# Embed published content files across all runs of the course

vector_search/tasks_test.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,50 @@ def test_start_embed_resources_without_settings(mocker, mocked_celery, index):
146146
generate_embeddings_mock.si.assert_not_called()
147147

148148

149+
def test_start_embed_resources_excludes_blocklisted_courses(mocker, mocked_celery):
150+
"""start_embed_resources should not embed courses whose readable_id is blocklisted"""
151+
courses = CourseFactory.create_batch(3, etl_source=ETLSource.ocw.value)
152+
blocked = courses[0]
153+
for course in courses:
154+
ContentFileFactory.create_batch(2, run=course.learning_resource.runs.first())
155+
mocker.patch(
156+
"vector_search.tasks.load_course_blocklist",
157+
return_value=[blocked.learning_resource.readable_id],
158+
)
159+
generate_embeddings_mock = mocker.patch(
160+
"vector_search.tasks.generate_embeddings", autospec=True
161+
)
162+
163+
with pytest.raises(mocked_celery.replace_exception_class):
164+
start_embed_resources.delay(
165+
[COURSE_TYPE], skip_content_files=False, overwrite=True
166+
)
167+
168+
embedded_resource_ids = {
169+
resource_id
170+
for call in generate_embeddings_mock.si.call_args_list
171+
if call.args[1] == COURSE_TYPE
172+
for resource_id in call.args[0]
173+
}
174+
assert embedded_resource_ids == {
175+
course.learning_resource_id for course in courses[1:]
176+
}
177+
178+
blocked_file_ids = set(
179+
ContentFile.objects.filter(
180+
run__learning_resource=blocked.learning_resource
181+
).values_list("id", flat=True)
182+
)
183+
embedded_file_ids = {
184+
file_id
185+
for call in generate_embeddings_mock.si.call_args_list
186+
if call.args[1] == CONTENT_FILE_TYPE
187+
for file_id in call.args[0]
188+
}
189+
assert embedded_file_ids
190+
assert not embedded_file_ids & blocked_file_ids
191+
192+
149193
def test_embed_new_learning_resources(mocker, mocked_celery):
150194
"""
151195
embed_new_learning_resources should generate embeddings for new resources

0 commit comments

Comments
 (0)