Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-5208: Fix filtering behavior #3113
Conversation
| @@ -112,8 +113,7 @@ def process_options(opts = {}) | ||
| # TODO: Change this to not rely on WorkSearch | ||
| processed_opts = WorkSearch.new(opts).options | ||
| - processed_opts.merge!(collected: opts[:collected]) if opts[:collected] | ||
| - | ||
| + processed_opts.merge!(collected: opts[:collected], faceted: opts[:faceted]) |
houndci-bot
Oct 22, 2017
Use processed_opts[:collected] = opts[:collected]; processed_opts[:faceted] = opts[:faceted] instead of processed_opts.merge!(collected: opts[:collected], faceted: opts[:faceted]).
elzj
added
Awaiting review
Priority: Broken on Test (High)
labels
Oct 22, 2017
| @@ -17,7 +17,9 @@ def document_type | ||
| # Hopefully someday they'll fix this and we can get the data from a single query | ||
| def search_results | ||
| response = search | ||
| - response['aggregations'].merge!(BookmarkableQuery.filters_for_bookmarks(self)) | ||
| + if response['aggregations'] |
houndci-bot
Oct 22, 2017
Use safe navigation (&.) instead of checking if an object exists before calling the method.
|
My concern with this is that as soon as you, say, exclude a particularly popular pairing using the checkboxes, the frequency of that pairing in your search results will become 0, and so if I'm not mistaken it'll drop off the list of the most popular tags in your search. This means that it won't show up in the checkboxes on the right, so if you then try to exclude a second pairing and press "Sort and Filter" again, it'll revert back to showing works/bookmarks with the first pairing. My suggestion would be to have some extra code on the filters form to display tags that have already been excluded (with no counts, since they'd all be 0). |
|
@tickinginstant Good point, I'll change that. |
| end | ||
| @bookmarks = @search.search_results | ||
| @facets = @bookmarks.facets | ||
| + if @search.options[:excluded_tag_ids].present? |
| @@ -180,6 +178,13 @@ def index | ||
| end | ||
| @facets = @works.facets | ||
| + if @search.options[:excluded_tag_ids].present? | ||
| + tags = Tag.where(id: @search.options[:excluded_tag_ids]) | ||
| + tags.each do |tag| |
elzj
added some commits
Oct 23, 2017
|
This appears to work perfectly for the work filters, but I'm getting a bit of a glitch with excluding bookmarks. The excluded tag shows up twice: once with its original count, and once with a count of (0) afterwards. And in general, the counts don't seem to decrease as I exclude tags. |
| + private | ||
| + | ||
| + def flipped_filter(filter) | ||
| + if filter.has_key?(:term) || filter.has_key?(:terms) |
| + def flipped_filter(filter) | ||
| + if filter.has_key?(:term) || filter.has_key?(:terms) | ||
| + { has_child: { type: "bookmark", filter: filter } } | ||
| + elsif filter.has_key?(:has_parent) |
| + filter[:has_parent][:filter] | ||
| + end | ||
| + end | ||
| + |
elzj
added some commits
Oct 23, 2017
| @@ -224,7 +228,12 @@ def collections_filter | ||
| def tag_exclusion_filter | ||
| if exclusion_ids.present? | ||
| - exclusion_ids.flatten.map { |exclusion_id| term_filter(:filter_ids, exclusion_id) } | ||
| + exclusion_ids.flatten.map { |exclusion_id| |
| @@ -0,0 +1,55 @@ | ||
| +# Shared methods for work and bookmark queries |
| @@ -0,0 +1,55 @@ | ||
| +# Shared methods for work and bookmark queries | ||
| +module TaggableQuery | ||
| + |
| + if options[:excluded_tag_ids] | ||
| + excluded_tags += Tag.where(id: options[:excluded_tag_ids]) | ||
| + end | ||
| + if excluded_tags.find{ |tag| tag.merger_id.present? } |
| + if excluded_tags.present? | ||
| + sub_ids = MetaTagging.where(meta_tag_id: ids).pluck(:sub_tag_id) | ||
| + child_ids = CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). | ||
| + where("filterable_id IN (?) AND tags.type = 'Character'", ids). |
houndci-bot
Oct 23, 2017
Align where with CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). on line 35.
| + sub_ids = MetaTagging.where(meta_tag_id: ids).pluck(:sub_tag_id) | ||
| + child_ids = CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). | ||
| + where("filterable_id IN (?) AND tags.type = 'Character'", ids). | ||
| + pluck(:common_tag_id) |
houndci-bot
Oct 23, 2017
Align pluck with CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). on line 35.
| + end | ||
| + Tag.where(name: names, canonical: true).pluck(:id) | ||
| + end | ||
| + |
| + Tag.where(name: names, canonical: true).pluck(:id) | ||
| + end | ||
| + | ||
| +end |
| @@ -36,7 +36,7 @@ | ||
| it "should not return bookmarks of hidden objects" do | ||
| q = BookmarkQuery.new | ||
| - expect(q.filters).to include({term: { bookmarkable_hidden_by_admin: 'false' }}) | ||
| + expect(q.filters).to include({has_parent:{type: 'bookmarkable', filter:{term: { hidden_by_admin: 'false' }}}}) |
houndci-bot
Oct 23, 2017
Space inside { missing.
Redundant curly braces around a hash parameter.
Space missing after colon.
Space inside } missing.
| + tags = Tag.where(id: @search.options[:excluded_tag_ids]) | ||
| + excluded_bookmark_tag_ids = params.dig(:exclude_bookmark_search, :tag_ids) || [] | ||
| + tags.each do |tag| | ||
| + if excluded_bookmark_tag_ids.include?(tag.id.to_s) |
| + tags = Tag.where(id: @search.options[:excluded_tag_ids]) | ||
| + excluded_bookmark_tag_ids = params.dig(:exclude_bookmark_search, :tag_ids) || [] | ||
| + tags.each do |tag| | ||
| + if excluded_bookmark_tag_ids.include?(tag.id.to_s) |
elzj
added some commits
Oct 24, 2017
| + ids = excluded_tags.pluck(:id) | ||
| + if excluded_tags.present? | ||
| + child_ids = CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). | ||
| + where("filterable_id IN (?) AND tags.type = 'Character'", ids). |
houndci-bot
Oct 24, 2017
Align where with CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). on line 34.
| + if excluded_tags.present? | ||
| + child_ids = CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). | ||
| + where("filterable_id IN (?) AND tags.type = 'Character'", ids). | ||
| + pluck(:common_tag_id) |
houndci-bot
Oct 24, 2017
Align pluck with CommonTagging.joins("JOIN tags ON tags.id = common_taggings.filterable_id"). on line 34.
| + # before(:each) do | ||
| + # child_tag.update(meta_tag_string: parent_tag.name) | ||
| + # parent_tag.update(meta_tag_string: grand_parent_tag.name) | ||
| + # end |
| - end | ||
| + # expect(search.search_results).to include(included_bookmark) | ||
| + # expect(search.search_results).not_to include(excluded_bookmark) | ||
| + # end |
| + | ||
| + def query_term | ||
| + input = (options[:q] || options[:query]) | ||
| + generate_search_text( input || '' ) |
| + { has_child: q[:has_parent]&.merge(type: "bookmark") } | ||
| + end | ||
| + end | ||
| + |
| - end | ||
| + # expect(search.search_results).to include(included_bookmark) | ||
| + # expect(search.search_results).not_to include(excluded_bookmark) | ||
| + # end |
| @@ -5,7 +5,7 @@ | ||
| it "should allow you to perform a simple search" do | ||
| q = BookmarkQuery.new(query: "unicorns") | ||
| search_body = q.generated_query | ||
| - expect(search_body[:query][:bool][:must]).to eq([{:query_string => { :query => "unicorns" }}]) | ||
| + expect(search_body[:query][:bool][:should]).to include({:query_string => { :query => "unicorns" }}) |
houndci-bot
Oct 24, 2017
Space inside { missing.
Redundant curly braces around a hash parameter.
Use the new Ruby 1.9 hash syntax.
Space inside } missing.
| - end | ||
| + # expect(search.search_results).to include(included_bookmark) | ||
| + # expect(search.search_results).not_to include(excluded_bookmark) | ||
| + # end |
elzj
added some commits
Oct 24, 2017
| json_object = object.as_json( | ||
| root: false, | ||
| except: [:notes_sanitizer_version, :delta], | ||
| - methods: [:bookmarker, :collection_ids, :with_notes] | ||
| + methods: [:bookmarker, :collection_ids, :with_notes, :bookmarkable_date] |
| end | ||
| def generate_search_text(query = '') | ||
| search_text = query | ||
| - [:bookmarker].each do |field| | ||
| + [:bookmarker, :notes, :tag].each do |field| |
| + end | ||
| + Tag.where(name: names).pluck(:id) | ||
| + end | ||
| + |
| + Tag.where(name: names).pluck(:id) | ||
| + end | ||
| + | ||
| +end |
| - end | ||
| + # expect(search.search_results).to include(included_bookmark) | ||
| + # expect(search.search_results).not_to include(excluded_bookmark) | ||
| + # end |
| @@ -6,6 +6,7 @@ class BookmarkSearch < Search | ||
| serialized_options :query, | ||
| :rec, | ||
| :notes, | ||
| + :bookmark_notes, |
| - end | ||
| + # expect(search.search_results).to include(included_bookmark) | ||
| + # expect(search.search_results).not_to include(excluded_bookmark) | ||
| + # end |
tickinginstant
approved these changes
Oct 25, 2017
Looks good to me! (And I'm very relieved that the children aren't being added to the query anymore.)
| end | ||
| @bookmarks = @search.search_results | ||
| @facets = @bookmarks.facets | ||
| + if @search.options[:excluded_tag_ids].present? | ||
| + tags = Tag.where(id: @search.options[:excluded_tag_ids]) | ||
| + excluded_bookmark_tag_ids = params.dig(:exclude_bookmark_search, :tag_ids) || [] |
tickinginstant
Oct 25, 2017
Contributor
Bit of an aside, but this line makes me think that the code above for packing all the information from :exclude_bookmark_search into :excluded_tag_ids is kind of overzealous (and I think it makes the include/exclude somewhat asymmetric). Still, I think that's probably outside the scope of this pull request. Maybe something to refactor later.
| @@ -224,7 +247,12 @@ def collections_filter | ||
| def tag_exclusion_filter | ||
| if exclusion_ids.present? | ||
| - exclusion_ids.flatten.map { |exclusion_id| term_filter(:filter_ids, exclusion_id) } | ||
| + exclusion_ids.flatten.map { |exclusion_id| |
tickinginstant
Oct 25, 2017
Contributor
There's a lot of flattens here. Maybe just use a single flat_map and rely on the fact that exclusion_ids will always return a list of IDs?
| + return if options[:excluded_tag_names].blank? && options[:excluded_tag_ids].blank? | ||
| + | ||
| + ids = options[:excluded_tag_ids] || [] | ||
| + names = options[:excluded_tag_names]&.split(",") |
tickinginstant
Oct 25, 2017
Contributor
I'd be tempted to add a .map(&:squish) here (and in named_tags) just in case someone has javascript disabled and is manually typing in tag names. But again, that might be outside the scope of the pull request, particularly since that's not the way other_tag_names is handled on the live website.
| @@ -272,7 +272,7 @@ def tag | ||
| # Index all the filters for pulling works | ||
| def filter_ids | ||
| - filters.pluck :id | ||
| + (tags.pluck(:id) + filters.pluck(:id)).uniq |
tickinginstant
Oct 25, 2017
Contributor
This slightly alters the way that searching for non-canonical tags works (it used to just fall back to searching the :tag field for matching strings), but I actually much prefer this way of handling it.
elzj commentedOct 22, 2017
Issue
https://otwarchive.atlassian.net/browse/AO3-5208
Purpose
Restores previous filter functionality: filters should become more specific to match the results as you drill down. They should also include collections on the appropriate pages. And bookmark filters should not include every tag on the site.
Testing
Filtering should work the way it does on production.