Skip to content

Conversation

@abeglova
Copy link
Contributor

@abeglova abeglova commented Jun 17, 2024

What are the relevant tickets?

closes https://github.com/mitodl/hq/issues/4578
makes backend changes needed for https://github.com/mitodl/hq/issues/4620

Description (What does it do?)

This pr adds a new "is_learning_material" boolean to the learning resources search index and api. This is true for Podcast, Podcast Episode, Video, Video Playlist.

It also updates the default sort when there is no text filter to [is_learning_material, -created_on] instead of just -created_on so that courses and programs are shown before "learning materials"

How can this be tested?

recreate your search index by running manage.py recreate_index --all ` this will take a while if you have a lot of content files

go to http://localhost:8063/search
go to http://localhost:8063/search?sortby=new in a different tab

Verify that the default sort ("best match") shows the same resources as sort by new but with everything except for courses and programs excluded.

Try searching for text term. The default sort should sort by relevance

Verify that http://localhost:8063/api/v1/learning_resources_search/?is_learning_material=true, http://localhost:8063/api/v1/learning_resources_search/?is_learning_material=false and http://localhost:8063/api/v1/learning_resources_search/?aggregations=is_learning_material gives the expected results and http://localhost:8063/api/v1/schema/redoc/#tag/learning_resources_search/operation/learning_resources_search_retrieve looks good

@abeglova abeglova marked this pull request as ready for review June 17, 2024 20:09
@abeglova abeglova added the Needs Review An open Pull Request that is ready for review label Jun 17, 2024
@abeglova
Copy link
Contributor Author

In order to not have downtime on the search page while we reindex I will probably break the change to learning_resources_search/api.py into a separate tiny pr to release.

The release process will be :

  1. Release PR minus the change to the default sort to add the new field to the index
  2. Reindex
  3. Release PR that uses the new field for the default sort

@ChristopherChudzicki ChristopherChudzicki self-requested a review June 18, 2024 15:25
@ChristopherChudzicki ChristopherChudzicki self-assigned this Jun 18, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

I left one question.

Comment on lines 82 to 87
"resource_relations": {"name": "resource"},
"created_on": learning_resource_obj.created_on,
"is_learning_material": learning_resource_obj.resource_type
not in [COURSE_TYPE, PROGRAM_TYPE],
**serialized_data,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These three fields:

  1. resource_relations
  2. created_on
  3. is_learning_material

are "extra". None of them show up in http://localhost:8063/api/v1/learning_resources/.

(1) and (2) do not show up in http://localhost:8063/api/v1/learning_resources_search/, but (3) does. Do you know why?

Mostly asking out of curiosity. If removing (3) is easy, that might be worthwhile since then the response formats will match exactly, which has been the goal. That said, having (3) is not a problem—it's "extra" and it's not usable on the frontend since it doesn't show up in the openapi (good).

Copy link
Contributor Author

@abeglova abeglova Jun 18, 2024

Choose a reason for hiding this comment

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

I hid the is_learning_material from the results. It's pretty redundant anyway. I guess it's a bit weird to filter by something that's not a field in the results but keeping the the output the same is good and cluttering the results of both the learning_resources api and learing_resources_search api with a field that's pretty specific to our ui seems unnecessary

@abeglova abeglova merged commit eb0537b into main Jun 18, 2024
@odlbot odlbot mentioned this pull request Jun 18, 2024
6 tasks
@rhysyngsun rhysyngsun deleted the ab/change-sort branch February 7, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants