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

Have better section filtering #216

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sebwittr
Copy link
Contributor

Purpose

We have a bug where if you filter by "honors" and "campus", you won't guarantee that there will be one section with the given honors and campus. This will guarantee each class returned has one section matching both filters, and it filters out the rest of the sections as well.

Tickets

https://trello.com/c/v6dY3aCt/416-section-filters-should-only-return-classes-that-have-sections-that-match-both-filters

Contributors

@ananyaspatil @sebwittr

Feature List

  • Changed the structure of our ES mapping to make sections a nested property. This allows to us to have nested queries and maps the sections directly to the classes withing Elasticsearch. This is necessary for doing multiple section filters.
  • Changed the current query to use a nested query for any section filters.
  • Added some code to filter out any sections from the results that don't match all the filters.

Notes (Optional)

The aggregations are still inaccurate (if you turn on the honors filter, and then click the campus filter you'll see that there are a lot of campus options that lead to no results after clicking it). We'll need to make a separate ticket for this!

Reviewers

Primary reviewer: @pranavphadke1 @soulwa

Secondary reviewers:
@ananyaspatil @Anzhuo-W

Original results:
image
With the new filtering:
image

@sebwittr
Copy link
Contributor Author

I'll fix the tests later.. oops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant