Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

show courses and programs first in default sort #1104

Merged
merged 1 commit into from
Jun 18, 2024
Merged

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

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
12 checks passed
@odlbot odlbot mentioned this pull request Jun 18, 2024
6 tasks
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.

2 participants