From 95168d8e4409fab3e8eee0a30e20139641efba7a Mon Sep 17 00:00:00 2001 From: fosterfarrell9 <28628554+fosterfarrell9@users.noreply.github.com> Date: Fri, 16 Feb 2024 11:48:12 +0100 Subject: [PATCH] Fix erroneous media search (#593) * Warn about too long GitHub commit messages (#586) * Fix comment status (#585) * Reapply first fix for Reader/Media See discussion on #574 for further details. Previous PR for this was #576, closed in favor of this one as this directly branches off the new "dev" branch. * Correctly show latest post (might be current_user's comment) * Fix update of unread comments logic in comments controller * Fix update icon logic and latest post comment * Simplify latest comment logic * Improve code comments * Further improve comments * Fix wording in comment * Fix construction of media array & use `.blank?` instead of `.empty?` * Migrate `unread_comments` flag (fix inconsistencies) (#587) * Add dummy migration * Implement migration for unread comment flag * Remove unnecessary comment * Declare migration as not idempotent * Use array.length instead of counting * Throw error to prevent revert of migration * Fix severe flaws in unread comments migration * Simplify Reader retrieval * Use the more explicit `.nil?` method * Update migration date * Fix annoying bug: don't use `.select!` but `.select` * Polish migration e.g. update comment, more suitable name for the method etc. * Rename method according to #585 * Use `warn` log level for migration (#588) * Rename get_statistics.coffee file to statistics.coffee (#591) This is to reflect the corresponding renaming of the route due to rubocop. * fix behaviour of media search * Disable OR/AND buttons if "all" tags is enabled (inside the media search, right now only frontend change) * Use ids autogenerated by Rails This is such that we can still click on the label to select the respective radio button, instead of only being able to click on the radio button itself. * Disable AND/OR initially (until user deselects "all tags") * Don't restrict media search if "all" tags is enabled. If the "all tags" option is enabled in the search, we should allow the results to include *any* tag. This is automatically the case if we just don't add any restriction regarding the tag ids in the SOLR search. This change accompanies the frontend change to disable the AND/OR buttons if the "all" button is selected meaning the user wants to search for all tags. See #593 for more details. --------- Co-authored-by: Splines <37160523+Splines@users.noreply.github.com> Co-authored-by: Splines --- app/assets/javascripts/application.js | 1 + app/assets/javascripts/search_tags.js | 19 +++++++++++++++++++ app/controllers/media_controller.rb | 15 --------------- app/models/medium.rb | 16 +++++----------- app/views/main/start/_media_search.html.erb | 6 ++++-- app/views/media/catalog/_search_form.html.erb | 6 ++++-- 6 files changed, 33 insertions(+), 30 deletions(-) create mode 100644 app/assets/javascripts/search_tags.js diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 9834213e1..31511217e 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -57,3 +57,4 @@ //= require vertices //= require watchlists //= require turbolinks +//= require search_tags \ No newline at end of file diff --git a/app/assets/javascripts/search_tags.js b/app/assets/javascripts/search_tags.js new file mode 100644 index 000000000..2aa798305 --- /dev/null +++ b/app/assets/javascripts/search_tags.js @@ -0,0 +1,19 @@ +$(document).on("turbolinks:load", function () { + $("#search_all_tags").change(evt => toggleSearchAllTags(evt)); +}); + +/** + * Dynamically enable/disable the OR/AND buttons in the media search form. + * If the user has decided to search for media regardless of the tags, + * i.e. they enable the "all" (tags) button, we disable the "OR/AND" buttons + * as it is pointless to search for media that references *all* available tags + * at once. + */ +function toggleSearchAllTags(evt) { + const searchAllTags = evt.target.checked; + if (searchAllTags) { + $("#search_tag_operator_or").prop("checked", true); + } + $("#search_tag_operator_or").prop("disabled", searchAllTags); + $("#search_tag_operator_and").prop("disabled", searchAllTags); +} diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index 23fd99052..a8efdbd2c 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -228,21 +228,6 @@ def search results = search.results @total = search.total - # in the case of a search with tag_operator 'or', we - # execute two searches and merge the results, where media - # with the selected tags are now shown at the front of the list - if (search_params[:tag_operator] == "or") \ - && (search_params[:all_tags] == "0") \ - && (search_params[:fulltext].size >= 2) - params["search"]["all_tags"] = "1" - search_no_tags = Medium.search_by(search_params, params[:page]) - search_no_tags.execute - results_no_tags = search_no_tags.results - results = (results + results_no_tags).uniq - @total = results.size - params["search"]["all_tags"] = "0" - end - if filter_media search_arel = Medium.where(id: results.pluck(:id)) visible_search_results = current_user.filter_visible_media(search_arel) diff --git a/app/models/medium.rb b/app/models/medium.rb index 0fe6faaa5..645371915 100644 --- a/app/models/medium.rb +++ b/app/models/medium.rb @@ -301,9 +301,6 @@ def self.search_by(search_params, _page) search_params[:all_terms] = "1" if search_params[:all_terms].blank? search_params[:all_teachers] = "1" if search_params[:all_teachers].blank? search_params[:term_ids].push("0") if search_params[:term_ids].present? - if search_params[:all_tags] == "1" && search_params[:tag_operator] == "and" - search_params[:tag_ids] = Tag.pluck(:id) - end user = User.find_by(id: search_params[:user_id]) search = Sunspot.new_search(Medium) search.build do @@ -336,15 +333,12 @@ def self.search_by(search_params, _page) with(:release_state, search_params[:access]) end end - if !search_params[:all_tags] == "1" && - !search_params[:tag_operator] == "or" && (search_params[:tag_ids]) - if search_params[:tag_operator] == "or" || search_params[:all_tags] == "1" - search.build do - with(:tag_ids).any_of(search_params[:tag_ids]) - end - else - search.build do + if search_params[:all_tags] == "0" && search_params[:tag_ids].any? + search.build do + if search_params[:tag_operator] == "and" with(:tag_ids).all_of(search_params[:tag_ids]) + else + with(:tag_ids).any_of(search_params[:tag_ids]) end end end diff --git a/app/views/main/start/_media_search.html.erb b/app/views/main/start/_media_search.html.erb index b836981b2..2a0f2dcfa 100644 --- a/app/views/main/start/_media_search.html.erb +++ b/app/views/main/start/_media_search.html.erb @@ -62,7 +62,8 @@ <%= f.radio_button :tag_operator, 'or', checked: true, - class: 'form-check-input' %> + class: 'form-check-input', + disabled: true %> <%= f.label :tag_operator, t('basics.OR'), value: 'or', @@ -71,7 +72,8 @@
<%= f.radio_button :tag_operator, 'and', - class: 'form-check-input' %> + class: 'form-check-input', + disabled: true %> <%= f.label :tag_operator, t('basics.AND'), value: 'and', diff --git a/app/views/media/catalog/_search_form.html.erb b/app/views/media/catalog/_search_form.html.erb index d831fae4e..8d554400b 100644 --- a/app/views/media/catalog/_search_form.html.erb +++ b/app/views/media/catalog/_search_form.html.erb @@ -105,7 +105,8 @@ <%= f.radio_button :tag_operator, 'or', checked: true, - class: 'form-check-input' %> + class: 'form-check-input', + disabled: true %> <%= f.label :tag_operator, t('basics.OR'), value: 'or', @@ -114,7 +115,8 @@
<%= f.radio_button :tag_operator, 'and', - class: 'form-check-input' %> + class: 'form-check-input', + disabled: true %> <%= f.label :tag_operator, t('basics.AND'), value: 'and',