From 7e7d49d60f0c15f5699037206cb50ee065750adb Mon Sep 17 00:00:00 2001 From: Splines <37160523+Splines@users.noreply.github.com> Date: Thu, 15 Feb 2024 16:53:25 +0100 Subject: [PATCH] Fix `unread_comments` migration (#594) * Add fix for migration #587 * Mark flawed unread comments migration as stale --- ...000_fix_unread_comments_inconsistencies.rb | 101 ++++++++++++------ ...x_unread_comments_inconsistencies_again.rb | 45 ++++++++ db/schema.rb | 2 +- 3 files changed, 114 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20240215100000_fix_unread_comments_inconsistencies_again.rb diff --git a/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb b/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb index 74ef88755..9d1a0eb19 100644 --- a/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb +++ b/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 diff --git a/db/migrate/20240215100000_fix_unread_comments_inconsistencies_again.rb b/db/migrate/20240215100000_fix_unread_comments_inconsistencies_again.rb new file mode 100644 index 000000000..02420a74d --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 0eb8c6842..377a05a4f 100644 --- a/db/schema.rb +++ b/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"