Continuous Release v1.9.3

Merge pull request #595 from MaMpf-HD/dev

app/assets/javascripts/application.js

@@ -57,3 +57,4 @@
 //= require vertices
 //= require watchlists
 //= require turbolinks
+//= require search_tags

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);
+}

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)

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

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 @@
       <div class="form-check form-check-inline">
         <%= 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',

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 @@
       <div class="form-check form-check-inline">
         <%= 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',

db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb

@@ -10,45 +10,80 @@
 # the unread_comments flag.
 class FixUnreadCommentsInconsistencies < ActiveRecord::Migration[7.0]
   def up
-    num_fixed_users = 0
+    # 🛑 This migration contains a bug, please don't use it again. Instead,
+    # refer to the migration in 20240215100000_fix_unread_comments_inconsistencies_again.rb.
+    #
+    # 💡 Explanation of the bug:
+    # The bug resides within the method `user_unread_comments?`. It was supposed
+    # to reflect the logic of the method `comments` in app/controllers/main_controller.rb:
+    #
+    #  def comments
+    #    @media_comments = current_user.subscribed_media_with_latest_comments_not_by_creator
+    #    @media_comments.select! do |m|
+    #      (Reader.find_by(user: current_user, thread: m[:thread])
+    #            &.updated_at || 1000.years.ago) < m[:latest_comment].created_at &&
+    #        m[:medium].visible_for_user?(current_user)
+    #    end
+    #    @media_array = Kaminari.paginate_array(@media_comments)
+    #                           .page(params[:page]).per(10)
+    #  end
+    #
+    # The method `user_unread_comments?` in this migration, however, does not
+    # reflect the logic of the method `comments` in app/controllers/main_controller.rb
+    # precisely. Consider what happens when for each reader instance, the
+    # check reader.updated_at < latest_thread_comment_time is false. In this case,
+    # we don't encounter an early "return true" and therefore go on with the second
+    # part. There, we want to check if there are any *unseen* media. What we actually do
+    # is to check if there are any media with comments (that are not by the creator
+    # and that are visible to the user). But we missed the part where we check if
+    # the user has already seen these comments, i.e. an additional reader query
+    # is missing. This is why suddenly, many more people encounter the original
+    # issue after having run this migration.
+    #
+    # This migration took ~40 minutes to run for ~7000 users at 2024-02-15, 0:40.
+    raise ActiveRecord::IrreversibleMigration
+
+    # For archive reasons, here is the original code:
 
-    User.find_each do |user|
-      had_user_unread_comments = user.unread_comments # boolean
-      has_user_unread_comments = user_unread_comments?(user)
+    # num_fixed_users = 0
 
-      has_flag_changed = (had_user_unread_comments != has_user_unread_comments)
-      user.update(unread_comments: has_user_unread_comments) if has_flag_changed
-      num_fixed_users += 1 if has_flag_changed
-    end
+    # User.find_each do |user|
+    #   had_user_unread_comments = user.unread_comments # boolean
+    #   has_user_unread_comments = user_unread_comments?(user)
 
-    Rails.logger.warn { "Ran through #{User.count} users (unread comments flag)" }
-    Rails.logger.warn { "Fixed #{num_fixed_users} users (unread comments flag)" }
+    #   has_flag_changed = (had_user_unread_comments != has_user_unread_comments)
+    #   user.update(unread_comments: has_user_unread_comments) if has_flag_changed
+    #   num_fixed_users += 1 if has_flag_changed
+    # end
+
+    # Rails.logger.warn { "Ran through #{User.count} users (unread comments flag)" }
+    # Rails.logger.warn { "Fixed #{num_fixed_users} users (unread comments flag)" }
   end
 
   # Checks and returns whether the user has unread comments.
-  def user_unread_comments?(user)
-    # Check for unread comments -- directly via Reader
-    readers = Reader.where(user: user)
-    readers.each do |reader|
-      thread = Commontator::Thread.find_by(id: reader.thread_id)
-      next if thread.nil?
-
-      latest_thread_comment_by_any_user = thread.comments.max_by(&:created_at)
-      next if latest_thread_comment_by_any_user.blank?
-
-      latest_thread_comment_time = latest_thread_comment_by_any_user.created_at
-      has_user_unread_comments = reader.updated_at < latest_thread_comment_time
-
-      return true if has_user_unread_comments
-    end
-
-    # User might still have unread comments but no related Reader objects
-    # -> Check for unread comments -- via Media
-    unseen_media = user.subscribed_media_with_latest_comments_not_by_creator.select do |m|
-      m[:medium].visible_for_user?(user)
-    end
-    unseen_media.present?
-  end
+  # def user_unread_comments?(user)
+  #   # Check for unread comments -- directly via Reader
+  #   readers = Reader.where(user: user)
+  #   readers.each do |reader|
+  #     thread = Commontator::Thread.find_by(id: reader.thread_id)
+  #     next if thread.nil?
+
+  #     latest_thread_comment_by_any_user = thread.comments.max_by(&:created_at)
+  #     next if latest_thread_comment_by_any_user.blank?
+
+  #     latest_thread_comment_time = latest_thread_comment_by_any_user.created_at
+  #     has_user_unread_comments = reader.updated_at < latest_thread_comment_time
+
+  #     return true if has_user_unread_comments
+  #   end
+
+  #   # User might still have unread comments but no related Reader objects
+  #   # -> Check for unread comments -- via Media
+  #   unseen_media = user.subscribed_media_with_latest_comments_not_by_creator.select do |m|
+  #     m[:medium].visible_for_user?(user)
+  #   end
+  #   unseen_media.present?
+  # end
 
   def down
     raise ActiveRecord::IrreversibleMigration

db/migrate/20240215100000_fix_unread_comments_inconsistencies_again.rb

@@ -0,0 +1,45 @@
+# Fixes the unread_comments flag for all users. Unintended behavior was
+# introduced in pull request #515. Behavior fixed in #585
+# A migration was introduced in #587, but it turned out it contained a bug.
+# This migration here is a fix for the migration in #587.
+#
+# This migration is generally *not* idempotent since users might have interacted
+# with the website since the migration was run and thus they will probably have
+# different unread comments flags as the ones at the time of the migration.
+#
+# This migration is not reversible as we don't store the previous state of
+# the unread_comments flag.
+class FixUnreadCommentsInconsistenciesAgain < ActiveRecord::Migration[7.0]
+  def up
+    num_fixed_users = 0
+
+    User.find_each do |user|
+      had_user_unread_comments = user.unread_comments # boolean
+      has_user_unread_comments = user_unread_comments?(user)
+
+      has_flag_changed = (had_user_unread_comments != has_user_unread_comments)
+      user.update(unread_comments: has_user_unread_comments) if has_flag_changed
+      num_fixed_users += 1 if has_flag_changed
+    end
+
+    Rails.logger.warn { "Ran through #{User.count} users (unread comments flag again)" }
+    Rails.logger.warn { "Fixed #{num_fixed_users} users (unread comments flag again)" }
+  end
+
+  # Checks and returns whether the user has unread comments.
+  def user_unread_comments?(user)
+    # see the method "comments" in app/controllers/main_controller.rb
+    unseen_media = user.subscribed_media_with_latest_comments_not_by_creator
+    unseen_media.select! do |m|
+      (Reader.find_by(user: user, thread: m[:thread])
+            &.updated_at || 1000.years.ago) < m[:latest_comment].created_at &&
+        m[:medium].visible_for_user?(user)
+    end
+
+    unseen_media.present?
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_01_16_180000) do
+ActiveRecord::Schema[7.0].define(version: 2024_02_15_100000) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"