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

Fix unread_comments migration #594

Merged
merged 2 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 68 additions & 33 deletions db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down