Skip to content

Conversation

@abeglova
Copy link
Contributor

@abeglova abeglova commented Oct 25, 2024

What are the relevant tickets?

None

Description (What does it do?)

This pr removes learning paths from the search results. Currently we do not want learning paths to be displayed in the search index. We asked people to set any learning paths they create as private but we should just not index them regardless of setting

Screenshots (if appropriate):

None

How can this be tested?

In the main branch create a public learning path if you don't already have any in the index.
Go to http://open.odl.local:8062/search?resource_category=learning_material and verify that "learning path" is a facet

Switch to this branch
Go to http://open.odl.local:8062/search?resource_category=learning_material and verify that "learning path" is not a facet

Create a new public learning path and verify that there isn't an error

@abeglova abeglova marked this pull request as ready for review October 25, 2024 19:30
@abeglova abeglova added the Needs Review An open Pull Request that is ready for review label Oct 25, 2024
@mbertrand mbertrand self-assigned this Oct 28, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Works great! One thing I noticed though is that if I create/edit a new learning path and set it to "Public" in the UI, a request is still made to index it - and the request succeeds even after a reindex, I guess the old learningpath index is still around?

web-1 | [2024-10-28 13:30:11] INFO 49 [opensearch] base.py:258 - [21a92b851f32] - HEAD http://opensearch-node-mitopen-1:9200/discussions_local_learning_path_default [status:200 request:0.004s]
web-1 | [2024-10-28 13:30:11] INFO 49 [opensearch] base.py:258 - [21a92b851f32] - POST http://opensearch-node-mitopen-1:9200/discussions_local_learning_path_default/_update/13515?retry_on_conflict=1 [status:200 request:0.014s]

Then I deleted the opensearch container and its volume, ran recreate_index again to make a new index from scratch, and after that the request to opensearch fails with a 404, but there isn't any error on the frontend:

celery-1 | [2024-10-28 13:41:24,841: WARNING/ForkPoolWorker-9] POST http://opensearch-node-mitopen-1:9200/_search [status:404 request:0.007s]
celery-1 | [2024-10-28 13:41:24,841: INFO/ForkPoolWorker-9] document 13548 not found in index

Not sure if we still need the "Private" vs "Public" UI option for learning paths anymore either, unless we still want learning paths hidden from the standard non-search API results if they are "private" (published=False). But if so, that can be for a subsequent PR.

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Oct 28, 2024
@abeglova
Copy link
Contributor Author

I removed the index update from the serializer for creating/updating learning paths. I think we should keep the "Private" vs "Public" UI option for now. We are in the process of figuring out how learning paths should be displayed and people are actively creating new learning paths. While we should not display learning paths in the search results until we figure out how then will be used, I don't think it makes sense to make more changes to learning paths until we have some product input and letting people set paths to "private" until they are ready to be displayed is something we will likely want in the future.

Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

👍

@abeglova abeglova force-pushed the ab/remove-learning-paths-from-index branch from 764eace to fc20e1e Compare October 28, 2024 15:55
@abeglova abeglova merged commit f978b16 into main Oct 28, 2024
11 checks passed
This was referenced Oct 28, 2024
@rhysyngsun rhysyngsun deleted the ab/remove-learning-paths-from-index branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants