diff --git a/.rubocop.yml b/.rubocop.yml index 7c85865b5..86ff8fdc5 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -58,7 +58,7 @@ Naming/BlockForwarding: EnforcedStyle: explicit Naming/PredicateMethod: Mode: conservative - AllowedPatterns: ['verify_.*', 'check_.*', 'enforce_.*', 'setup_.*'] + AllowedPatterns: ['verify_.*', 'check_.*', 'enforce_.*', 'setup_.*','add_.*', 'remove_.*'] AllowedMethods: - post_sign_in # returns a boolean but is a necessary after-sign-in method for both normal and SAML flows - comment_rate_limited? # returns a tuple with the boolean as the first value diff --git a/app/assets/javascripts/comments.js b/app/assets/javascripts/comments.js index e56a4b428..e520674c7 100644 --- a/app/assets/javascripts/comments.js +++ b/app/assets/javascripts/comments.js @@ -250,11 +250,32 @@ $(() => { QPixel.handleJSONResponse(data, () => { const wrapper = getCommentThreadWrapper($tgt); - const inline = isInlineCommentThread(wrapper); - openThread(wrapper, threadID, { inline }); + + if (wrapper) { + const inline = isInlineCommentThread(wrapper); + openThread(wrapper, threadID, { inline }); + } }); }); + $(document).on('click', '.js--unfollow-thread', async (ev) => { + ev.preventDefault(); + + const $tgt = $(ev.target); + const threadID = $tgt.data('thread'); + + const data = await QPixel.unfollowThread(threadID); + + QPixel.handleJSONResponse(data, () => { + const wrapper = getCommentThreadWrapper($tgt); + + if (wrapper) { + const inline = isInlineCommentThread(wrapper); + openThread(wrapper, threadID, { inline }); + } + }); + }) + /** * @param {Element} target */ diff --git a/app/assets/javascripts/qpixel_api.js b/app/assets/javascripts/qpixel_api.js index 51a9336e6..b9d0d4a08 100644 --- a/app/assets/javascripts/qpixel_api.js +++ b/app/assets/javascripts/qpixel_api.js @@ -506,6 +506,14 @@ window.QPixel = { return QPixel.parseJSONResponse(resp, 'Failed to follow thread'); }, + unfollowThread: async (id) => { + const resp = await QPixel.fetchJSON(`/comments/thread/${id}/unfollow`, {}, { + headers: { 'Accept': 'application/json' }, + }); + + return QPixel.parseJSONResponse(resp, 'Failed to unfollow thread'); + }, + lockThread: async (id, duration) => { const resp = await QPixel.fetchJSON(`/comments/thread/${id}/lock`, { duration, diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 6c37822a4..6cc1078b7 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -12,6 +12,7 @@ class CommentsController < ApplicationController :archive_thread, :delete_thread, :follow_thread, + :unfollow_thread, :lock_thread, :thread_unrestrict, :thread_followers] @@ -188,14 +189,16 @@ def show end def thread - respond_to do |format| - format.html { render 'comments/thread' } - format.json { render json: @comment_thread } + if stale?(last_modified: @comment_thread.last_activity.utc) + respond_to do |format| + format.html { render 'comments/thread' } + format.json { render json: @comment_thread } + end end end def thread_content - if stale?(last_modified: @comment_thread.last_activity_at.utc) + if stale?(last_modified: @comment_thread.last_activity.utc) render partial: 'comment_threads/expanded', locals: { inline: params[:inline] == 'true', show_deleted: params[:show_deleted_comments] == '1', @@ -264,7 +267,7 @@ def delete_thread end def follow_thread - status = ThreadFollower.create(comment_thread: @comment_thread, user: current_user) + status = @comment_thread.add_follower(current_user) restrict_thread_response(@comment_thread, status) end @@ -293,7 +296,7 @@ def undelete_thread end def unfollow_thread - status = ThreadFollower.find_by(comment_thread: @comment_thread, user: current_user)&.destroy + status = @comment_thread.remove_follower(current_user) restrict_thread_response(@comment_thread, status) end @@ -311,8 +314,6 @@ def thread_unrestrict unarchive_thread when 'delete' undelete_thread - when 'follow' - unfollow_thread else not_found! end diff --git a/app/jobs/clean_up_thread_followers_job.rb b/app/jobs/clean_up_thread_followers_job.rb new file mode 100644 index 000000000..1f3c23945 --- /dev/null +++ b/app/jobs/clean_up_thread_followers_job.rb @@ -0,0 +1,24 @@ +class CleanUpThreadFollowersJob < ApplicationJob + queue_as :default + + def perform + sql = File.read(Rails.root.join('db/scripts/threads_with_duplicate_followers.sql')) + threads = ActiveRecord::Base.connection.execute(sql).to_a + + threads.each do |thread| + user_id, thread_id = thread + + followers = ThreadFollower.where(comment_thread_id: thread_id, user_id: user_id) + + next unless followers.many? + + duplicate = followers.first + result = duplicate.destroy + + unless result + puts "failed to destroy thread follower duplicate \"#{duplicate.id}\"" + duplicate.errors.each { |e| puts e.full_message } + end + end + end +end diff --git a/app/models/comment.rb b/app/models/comment.rb index 66ac1af5b..2ecb237b3 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -17,16 +17,12 @@ class Comment < ApplicationRecord after_create :create_follower after_update :delete_thread + before_save :bump_last_activity + counter_culture :comment_thread, column_name: proc { |model| model.deleted? ? nil : 'reply_count' }, touch: true validate :content_length - # Gets last activity date and time on the comment - # @return [DateTime] last activity date and time - def last_activity_at - [created_at, updated_at].compact.max - end - def root # If parent_question is nil, the comment is already on a question, so we can just return post. parent_question || post @@ -41,12 +37,28 @@ def content_length end end + # Gets last activity date and time on the comment + # @return [DateTime] last activity date and time + def last_activity + [created_at, updated_at, last_activity_at].compact.max + end + def pings pingable = comment_thread.pingable matches = content.scan(USER_PING_REG_EXP) matches.flatten.select { |m| pingable.include?(m.to_i) }.map(&:to_i) end + # Directly bumps the comment's last activity date & time + # @param persist_changes [Boolean] if set to +true+, will persist the changes + def bump_last_activity(persist_changes: false) + self.last_activity_at = DateTime.now + + if persist_changes + save + end + end + private def create_follower diff --git a/app/models/comment_thread.rb b/app/models/comment_thread.rb index f33af6735..68d40af67 100644 --- a/app/models/comment_thread.rb +++ b/app/models/comment_thread.rb @@ -16,6 +16,8 @@ class CommentThread < ApplicationRecord after_create :create_follower + before_save :bump_last_activity + # Gets threads appropriately scoped for a given user & post # @param user [User, nil] user to check # @para post [Post] post to check @@ -51,9 +53,9 @@ def can_access?(user) # Gets last activity date and time on the thread # @return [DateTime] last activity date and time - def last_activity_at - last_comment_activity_at = comments.map(&:last_activity_at).max - [created_at, locked_at, updated_at, last_comment_activity_at].compact.max + def last_activity + last_comment_activity = comments.map(&:last_activity).compact.max + [created_at, updated_at, last_activity_at, last_comment_activity].compact.max end # Gets a list of user IDs who should be pingable in the thread. @@ -86,6 +88,40 @@ def maximum_title_length end end + # Registers a given user as a follower of the thread + # @param user [User] user to register as a follower + # @return [Boolean] status of the operation + def add_follower(user) + if ThreadFollower.where(comment_thread: self, user: user).any? + bump_last_activity(persist_changes: true) + return true + end + + ThreadFollower.create(comment_thread: self, user: user) + end + + # Directly bumps the thread's last activity date & time + # @param persist_changes [Boolean] if set to +true+, will persist the changes + def bump_last_activity(persist_changes: false) + self.last_activity_at = DateTime.now + + if persist_changes + save + end + end + + # Removes a given user from the thread's followers + # @param user [User] user to remove from followers + # @return [Boolean] status of the operation + def remove_follower(user) + if ThreadFollower.where(comment_thread: self, user: user).none? + bump_last_activity(persist_changes: true) + return true + end + + ThreadFollower.where(comment_thread: self, user: user).destroy_all.any? + end + private # Comment author and post author are automatically followed to the thread. Question author is NOT diff --git a/app/models/thread_follower.rb b/app/models/thread_follower.rb index 426e1f394..562dbc53d 100644 --- a/app/models/thread_follower.rb +++ b/app/models/thread_follower.rb @@ -3,10 +3,17 @@ class ThreadFollower < ApplicationRecord belongs_to :post, optional: true belongs_to :user + after_create :bump_thread_last_activity + before_destroy :bump_thread_last_activity + validate :thread_or_post private + def bump_thread_last_activity + comment_thread&.bump_last_activity(persist_changes: true) + end + def thread_or_post if comment_thread.nil? && post.nil? errors.add(:base, 'Must refer to either a comment thread or a post.') diff --git a/app/models/user.rb b/app/models/user.rb index 5ad2aeb2c..5e1607028 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -483,13 +483,13 @@ def active_flags(post) end # Anonymizes the user (f.e., for the purpose of soft deletion) - # @param persist_changes [Boolean] if set to +false+, will persist the changes + # @param persist_changes [Boolean] if set to +true+, will persist the changes def anonymize(persist_changes: false) assign_attributes(username: "user#{id}", email: "#{id}@deleted.localhost", password: SecureRandom.hex(32)) - unless persist_changes + if persist_changes skip_reconfirmation! save end @@ -505,7 +505,7 @@ def soft_delete(attribute_to) deleted_by_id: attribute_to.id, deleted_at: DateTime.now) - anonymize(persist_changes: true) + anonymize(persist_changes: false) skip_reconfirmation! save diff --git a/app/views/comments/_thread_follow_link.html.erb b/app/views/comments/_thread_follow_link.html.erb index 1a2f46f4e..e99ac05b3 100644 --- a/app/views/comments/_thread_follow_link.html.erb +++ b/app/views/comments/_thread_follow_link.html.erb @@ -8,8 +8,7 @@ <% if thread.followed_by?(user) %> 1; diff --git a/global.d.ts b/global.d.ts index 0153e8907..f66e512a2 100644 --- a/global.d.ts +++ b/global.d.ts @@ -587,6 +587,13 @@ interface QPixel { */ followThread?: (id: string) => Promise + /** + * Attempts to unfollow a comment thread + * @param id id of the thread to unfollow + * @returns result of the operation + */ + unfollowThread?: (id: string) => Promise + /** * Attempts to start following comments on a given post * @param postId id of the post to follow comments on diff --git a/scripts/run_thread_followers_cleanup.rb b/scripts/run_thread_followers_cleanup.rb new file mode 100644 index 000000000..06bbee308 --- /dev/null +++ b/scripts/run_thread_followers_cleanup.rb @@ -0,0 +1 @@ +CleanUpThreadFollowersJob.perform_later diff --git a/test/comments_test_helpers.rb b/test/comments_test_helpers.rb index 8f7a14ae5..4ca0e1da1 100644 --- a/test/comments_test_helpers.rb +++ b/test/comments_test_helpers.rb @@ -68,7 +68,7 @@ def try_follow_thread(thread) # Attempts to unfollow a given comment thread # @param thread [CommentThread] thread to unfollow def try_unfollow_thread(thread) - post :thread_unrestrict, params: { id: thread.id, type: 'follow' } + post :unfollow_thread, params: { id: thread.id } end # Attempts to follow new threads on a given post diff --git a/test/fixtures/comment_threads.yml b/test/fixtures/comment_threads.yml index b4bb74dfb..6a150cbf3 100644 --- a/test/fixtures/comment_threads.yml +++ b/test/fixtures/comment_threads.yml @@ -74,3 +74,18 @@ with_user_pings: reply_count: 1 post: question_one community: sample + +without_activity: + title: Comment thread without activity + reply_count: 0 + post: question_one + community: sample + created_at: 2020-01-01T00:00:00.000000Z + updated_at: 2020-01-01T00:00:00.000000Z + +without_followers: + title: Comment thread without any followers + title: Comment thread without activity + reply_count: 0 + post: question_one + community: sample diff --git a/test/fixtures/comments.yml b/test/fixtures/comments.yml index 5f1ecbd47..709d6f9ca 100644 --- a/test/fixtures/comments.yml +++ b/test/fixtures/comments.yml @@ -81,3 +81,12 @@ with_user_pings: comment_thread: with_user_pings created_at: 2020-01-01T00:00:00.000000Z updated_at: 2021-01-01T00:00:00.000000Z + +without_activity: + user: standard_user + post: question_one + content: This comment must be without any initial activity + community: sample + comment_thread: normal + created_at: 2020-01-01T00:00:00.000000Z + updated_at: 2020-01-01T00:00:00.000000Z diff --git a/test/jobs/clean_up_thread_followers_job_test.rb b/test/jobs/clean_up_thread_followers_job_test.rb new file mode 100644 index 000000000..79d1a7993 --- /dev/null +++ b/test/jobs/clean_up_thread_followers_job_test.rb @@ -0,0 +1,19 @@ +require 'test_helper' + +class CleanUpThreadFollowersJobTest < ActiveJob::TestCase + test 'should correctly clean up thread followers' do + thread = comment_threads(:without_followers) + std_usr = users(:standard_user) + + ThreadFollower.create(comment_thread: thread, user: std_usr) + ThreadFollower.create(comment_thread: thread, user: std_usr) + assert_equal 2, ThreadFollower.where(comment_thread: thread, user: std_usr).size + + perform_enqueued_jobs do + CleanUpThreadFollowersJob.perform_later + end + + assert_performed_jobs 1 + assert_equal 1, ThreadFollower.where(comment_thread: thread, user: std_usr).size + end +end diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index 72a28d95f..bab89bf8d 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -34,7 +34,7 @@ class CommentTest < ActiveSupport::TestCase end end - test 'pings should correctly' do + test 'pings should correctly get pinged user ids' do std_user = users(:standard_user) with_pings = comments(:with_user_pings) @@ -43,4 +43,19 @@ class CommentTest < ActiveSupport::TestCase assert pings.include?(std_user.id) assert_not pings.include?(User.system.id) end + + test 'last_activity should correctly get the comment\'s last activity date & time' do + comment = comments(:without_activity) + + assert_equal comment.created_at, comment.last_activity + + comment.update!(content: 'this should bump last_activity') + comment.reload + last_activity_after_update = comment.last_activity + assert_operator comment.updated_at, '<', last_activity_after_update + + comment.bump_last_activity(persist_changes: true) + comment.reload + assert_operator last_activity_after_update, '<', comment.last_activity + end end diff --git a/test/models/comment_thread_test.rb b/test/models/comment_thread_test.rb index aafddaf42..721543def 100644 --- a/test/models/comment_thread_test.rb +++ b/test/models/comment_thread_test.rb @@ -16,16 +16,47 @@ class CommentThreadTest < ActiveSupport::TestCase end end - test 'last_activity_at should correctly get last activity' do - normal = comment_threads(:normal) - normal_two = comment_threads(:normal_two) - locked = comment_threads(:locked) + test 'remove_follower should correctly bump last_activity' do + std_usr = users(:standard_user) + thread = comment_threads(:without_activity) + assert_equal thread.created_at, thread.last_activity - assert_not_equal locked.last_activity_at, locked.created_at - assert_not_equal normal_two.last_activity_at, normal_two.created_at + thread.add_follower(std_usr) + thread.reload + last_activity_after_follow = thread.last_activity - assert_equal locked.last_activity_at, locked.locked_at - assert_equal normal.last_activity_at, normal.created_at - assert_equal normal_two.last_activity_at, normal_two.updated_at + thread.remove_follower(std_usr) + thread.reload + last_activity_after_unfollow = thread.last_activity + assert_operator last_activity_after_follow, '<', last_activity_after_unfollow + + thread.remove_follower(std_usr) + thread.reload + last_activity_after_noop = thread.last_activity + assert_operator last_activity_after_unfollow, '<', last_activity_after_noop + end + + test 'last_activity should correctly get the thread\'s last activity date & time' do + thread = comment_threads(:without_activity) + assert_equal thread.created_at, thread.last_activity + + thread.update!(title: 'this should bump last_activity') + thread.reload + last_activity_after_update = thread.last_activity + assert_operator thread.updated_at, '<=', last_activity_after_update + + thread.bump_last_activity(persist_changes: true) + thread.reload + last_activity_after_bump = thread.last_activity + assert_operator last_activity_after_update, '<', last_activity_after_bump + + thread.add_follower(users(:editor)) + thread.reload + last_activity_after_follow = thread.last_activity + assert_operator last_activity_after_bump, '<', last_activity_after_follow + + thread.remove_follower(users(:editor)) + thread.reload + assert_operator last_activity_after_follow, '<', thread.last_activity end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 8ea34b2d9..c92638738 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -321,7 +321,7 @@ class UserTest < ActiveSupport::TestCase anonymized_name = "user#{std.id}" anonymized_email = "#{std.id}@deleted.localhost" - [true, false].each do |persist| + [false, true].each do |persist| std.anonymize(persist_changes: persist) assert_equal std.username, anonymized_name @@ -330,11 +330,11 @@ class UserTest < ActiveSupport::TestCase std.reload if persist - assert_not_equal std.username, anonymized_name - assert_not_equal std.email, anonymized_email - else assert_equal std.username, anonymized_name assert_equal std.email, anonymized_email + else + assert_not_equal std.username, anonymized_name + assert_not_equal std.email, anonymized_email end end end