From a183f14d0936bd272bcc67d381ffad2b1c8d6bac Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Mon, 26 Jun 2023 11:01:59 +0800 Subject: [PATCH] DEV: Update display name in new quote format - Part 2 (#22104) This change adds support retroactively updating display names in the new quote format when the user's name is changed. It happens through a background job that is triggered by a callback when a user is saved with a new name. --- app/jobs/regular/change_display_name.rb | 89 ++++++++++++++++ app/jobs/regular/update_username.rb | 20 ++-- app/models/user.rb | 5 + lib/quote_rewriter.rb | 35 +++++-- spec/jobs/change_display_name_spec.rb | 68 ++++++++++++ spec/lib/quote_rewriter_spec.rb | 131 ++++++++++++++++++++++++ spec/models/user_spec.rb | 13 +++ 7 files changed, 342 insertions(+), 19 deletions(-) create mode 100644 app/jobs/regular/change_display_name.rb create mode 100644 spec/jobs/change_display_name_spec.rb create mode 100644 spec/lib/quote_rewriter_spec.rb diff --git a/app/jobs/regular/change_display_name.rb b/app/jobs/regular/change_display_name.rb new file mode 100644 index 00000000000..2c1aea46f80 --- /dev/null +++ b/app/jobs/regular/change_display_name.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Jobs + class ChangeDisplayName < ::Jobs::Base + sidekiq_options queue: "low" + + # Avoid race conditions if a user's name is updated several times + # in quick succession. + cluster_concurrency 1 + + def execute(args) + @user = User.find_by(id: args[:user_id]) + + return unless user.present? + + # We need to account for the case where the instance allows + # name to be empty by falling back to username. + @old_display_name = (args[:old_name].presence || user.username).unicode_normalize + @new_display_name = (args[:new_name].presence || user.username).unicode_normalize + + @quote_rewriter = QuoteRewriter.new(user.id) + + update_posts + update_revisions + end + + private + + attr_reader :user, :old_display_name, :new_display_name, :quote_rewriter + + def update_posts + Post + .with_deleted + .joins(quoted("posts.id")) + .where("p.user_id = :user_id", user_id: user.id) + .find_each { |post| update_post(post) } + end + + def update_revisions + PostRevision + .joins(quoted("post_revisions.post_id")) + .where("p.user_id = :user_id", user_id: user.id) + .find_each { |revision| update_revision(revision) } + end + + def quoted(post_id_column) + <<~SQL + JOIN quoted_posts AS q ON (q.post_id = #{post_id_column}) + JOIN posts AS p ON (q.quoted_post_id = p.id) + SQL + end + + def update_post(post) + post.raw = update_raw(post.raw) + post.cooked = update_cooked(post.cooked) + + post.update_columns(raw: post.raw, cooked: post.cooked) + + SearchIndexer.index(post, force: true) if post.topic + rescue => e + Discourse.warn_exception(e, message: "Failed to update post with id #{post.id}") + end + + def update_revision(revision) + if revision.modifications["raw"] || revision.modifications["cooked"] + revision.modifications["raw"].map! { |raw| update_raw(raw) } + revision.modifications["cooked"].map! { |cooked| update_cooked(cooked) } + revision.save! + end + rescue => e + Discourse.warn_exception(e, message: "Failed to update post revision with id #{revision.id}") + end + + def update_raw(raw) + @quote_rewriter.rewrite_raw_display_name(raw, old_display_name, new_display_name) + end + + # Uses Nokogiri instead of rebake, because it works for posts and revisions + # and there is no reason to invalidate oneboxes, run the post analyzer etc. + # when only the display name changes. + def update_cooked(cooked) + doc = Nokogiri::HTML5.fragment(cooked) + + @quote_rewriter.rewrite_cooked_display_name(doc, old_display_name, new_display_name) + + doc.to_html + end + end +end diff --git a/app/jobs/regular/update_username.rb b/app/jobs/regular/update_username.rb index c9e1367e479..df7cd667b11 100644 --- a/app/jobs/regular/update_username.rb +++ b/app/jobs/regular/update_username.rb @@ -12,9 +12,9 @@ module Jobs @old_username = args[:old_username].unicode_normalize @new_username = args[:new_username].unicode_normalize - avatar_img = PrettyText.avatar_img(args[:avatar_template], "tiny") + @avatar_img = PrettyText.avatar_img(args[:avatar_template], "tiny") - @quote_rewriter = QuoteRewriter.new(@user_id, @old_username, @new_username, avatar_img) + @quote_rewriter = QuoteRewriter.new(@user_id) @raw_mention_regex = / @@ -160,7 +160,11 @@ module Jobs end def update_raw(raw) - @quote_rewriter.rewrite_raw(raw.gsub(@raw_mention_regex, "@#{@new_username}")) + @quote_rewriter.rewrite_raw_username( + raw.gsub(@raw_mention_regex, "@#{@new_username}"), + @old_username, + @new_username, + ) end # Uses Nokogiri instead of rebake, because it works for posts and revisions @@ -179,17 +183,9 @@ module Jobs ) if a["href"] end - @quote_rewriter.rewrite_cooked(doc) + @quote_rewriter.rewrite_cooked_username(doc, @old_username, @new_username, @avatar_img) doc.to_html end - - def quotes_correct_user?(aside) - Post.exists?( - topic_id: aside["data-topic"], - post_number: aside["data-post"], - user_id: @user_id, - ) - end end end diff --git a/app/models/user.rb b/app/models/user.rb index 17a770bb97e..1810f3f024d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -180,6 +180,7 @@ class User < ActiveRecord::Base if: Proc.new { self.human? && self.saved_change_to_uploaded_avatar_id? } after_update :trigger_user_automatic_group_refresh, if: :saved_change_to_staged? + after_update :change_display_name, if: :saved_change_to_name? before_save :update_usernames before_save :ensure_password_is_hashed @@ -2147,6 +2148,10 @@ class User < ActiveRecord::Base update_column(:previous_visit_at, last_seen_at) if previous_visit_at_update_required?(timestamp) end + def change_display_name + Jobs.enqueue(:change_display_name, user_id: id, old_name: name_before_last_save, new_name: name) + end + def trigger_user_created_event DiscourseEvent.trigger(:user_created, self) true diff --git a/lib/quote_rewriter.rb b/lib/quote_rewriter.rb index abc76504d2c..a316b81ca2d 100644 --- a/lib/quote_rewriter.rb +++ b/lib/quote_rewriter.rb @@ -1,14 +1,11 @@ # frozen_string_literal: true class QuoteRewriter - def initialize(user_id, old_username, new_username, avatar_img) + def initialize(user_id) @user_id = user_id - @old_username = old_username - @new_username = new_username - @avatar_img = avatar_img end - def rewrite_raw(raw) + def rewrite_raw_username(raw, old_username, new_username) pattern = Regexp.union( /(?
\[quote\s*=\s*["'']?.*username:)#{old_username}(?\,?[^\]]*\])/i,
@@ -18,7 +15,7 @@ class QuoteRewriter
     raw.gsub(pattern, "\\k
#{new_username}\\k")
   end
 
-  def rewrite_cooked(cooked)
+  def rewrite_cooked_username(cooked, old_username, new_username, avatar_img)
     pattern = /(?<=\s)#{PrettyText::Helpers.format_username(old_username)}(?=:)/i
 
     cooked
@@ -44,9 +41,33 @@ class QuoteRewriter
       end
   end
 
+  def rewrite_raw_display_name(raw, old_display_name, new_display_name)
+    pattern = /(?
\[quote\s*=\s*["'']?)#{old_display_name}(?\,[^\]]*username[^\]]*\])/i
+
+    raw.gsub(pattern, "\\k
#{new_display_name}\\k")
+  end
+
+  def rewrite_cooked_display_name(cooked, old_display_name, new_display_name)
+    pattern = /(?<=\s)#{PrettyText::Helpers.format_username(old_display_name)}(?=:)/i
+
+    cooked
+      .css("aside.quote")
+      .each do |aside|
+        next unless div = aside.at_css("div.title")
+
+        div.children.each do |child|
+          if child.text?
+            content = child.content
+            display_name_replaced = content.gsub!(pattern, new_display_name).present?
+            child.content = content if display_name_replaced
+          end
+        end
+      end
+  end
+
   private
 
-  attr_reader :user_id, :old_username, :new_username, :avatar_img
+  attr_reader :user_id
 
   def quotes_correct_user?(aside)
     Post.exists?(topic_id: aside["data-topic"], post_number: aside["data-post"], user_id: user_id)
diff --git a/spec/jobs/change_display_name_spec.rb b/spec/jobs/change_display_name_spec.rb
new file mode 100644
index 00000000000..e18db2cf346
--- /dev/null
+++ b/spec/jobs/change_display_name_spec.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::ChangeDisplayName do
+  before { stub_image_size }
+
+  let(:user) { Fabricate(:user, username: "codinghorror", name: "Jeff") }
+  let(:topic) { Fabricate(:topic, user: user) }
+  let!(:post) { create_post(post_attributes.merge(topic_id: topic.id)) }
+
+  let!(:quoted_post) { create_post(user: user, topic: topic, post_number: 1, raw: "quoted post") }
+  let(:avatar_url) { user.avatar_template_url.gsub("{size}", "48") }
+
+  let(:post_attributes) { { raw: <<~RAW } }
+    [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+    quoted post
+    [/quote]
+  RAW
+
+  let(:revised_post_attributes) { { raw: <<~RAW } }
+    [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+    quoted post
+    [/quote]
+    Forgot something.
+  RAW
+
+  let(:args) { { user_id: user.id, old_name: "Jeff", new_name: "Mr. Atwood" } }
+
+  describe "#execute" do
+    context "when the renamed user has been quoted" do
+      it "rewrites the raw quote display name" do
+        expect { described_class.new.execute(args) }.to change { post.reload.raw }.to(<<~RAW.strip)
+          [quote="Mr. Atwood, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+          quoted post
+          [/quote]
+        RAW
+      end
+
+      it "rewrites the cooked quote display name" do
+        expect { described_class.new.execute(args) }.to change { post.reload.cooked }.to(
+          match_html(<<~HTML.strip),
+          
+        HTML
+        )
+      end
+    end
+
+    context "when the user has been quoted in revisions" do
+      before { post.revise(post.user, revised_post_attributes, force_new_version: true) }
+
+      it "rewrites the quote in revisions" do
+        expect { described_class.new.execute(args) }.to change {
+          post.reload.revisions[0].modifications["raw"][0]
+        }.to(<<~RAW.strip)
+          [quote="Mr. Atwood, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+          quoted post
+          [/quote]
+          RAW
+      end
+    end
+  end
+end
diff --git a/spec/lib/quote_rewriter_spec.rb b/spec/lib/quote_rewriter_spec.rb
new file mode 100644
index 00000000000..52e9dd5d693
--- /dev/null
+++ b/spec/lib/quote_rewriter_spec.rb
@@ -0,0 +1,131 @@
+# frozen_string_literal: true
+
+RSpec.describe QuoteRewriter do
+  subject(:quote_rewriter) { described_class.new(post.id) }
+
+  before { stub_image_size }
+
+  let(:user) { Fabricate(:user, username: "codinghorror") }
+  let(:topic) { Fabricate(:topic, user: user) }
+  let(:post) { create_post(post_attributes.merge(topic_id: topic.id)) }
+
+  let(:quoted_post) { create_post(user: user, topic: topic, post_number: 1, raw: "quoted post") }
+  let(:avatar_url) { user.avatar_template_url.gsub("{size}", "48") }
+
+  describe "#rewrite_raw_username" do
+    context "when using the old quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="codinghorror, post:1, topic:#{quoted_post.topic.id}"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "rewrites the username" do
+        expect(quote_rewriter.rewrite_raw_username(post.raw, "codinghorror", "codingterror")).to eq(
+          <<~RAW.strip,
+          [quote="codingterror, post:1, topic:#{quoted_post.topic.id}"]
+          quoted post
+          [/quote]
+        RAW
+        )
+      end
+    end
+
+    context "when using the new quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "rewrites the username" do
+        expect(quote_rewriter.rewrite_raw_username(post.raw, "codinghorror", "codingterror")).to eq(
+          <<~RAW.strip,
+          [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codingterror"]
+          quoted post
+          [/quote]
+        RAW
+        )
+      end
+    end
+  end
+
+  describe "#rewrite_raw_display_name" do
+    context "when using the old quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="codinghorror, post:1, topic:#{quoted_post.topic.id}"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "does nothing because the username hasn't changed" do
+        expect(quote_rewriter.rewrite_raw_display_name(post.raw, "Jeff", "Mr. Atwood")).to eq(
+          <<~RAW.strip,
+          [quote="codinghorror, post:1, topic:#{quoted_post.topic.id}"]
+          quoted post
+          [/quote]
+        RAW
+        )
+      end
+    end
+
+    context "when using the new quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "rewrites the display name" do
+        expect(quote_rewriter.rewrite_raw_display_name(post.raw, "Jeff", "Mr. Atwood")).to eq(
+          <<~RAW.strip,
+          [quote="Mr. Atwood, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+          quoted post
+          [/quote]
+        RAW
+        )
+      end
+    end
+  end
+
+  describe "#rewrite_cooked_display_name" do
+    let(:doc) { Nokogiri::HTML5.fragment(post.cooked) }
+
+    context "when using the old quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="codinghorror, post:1, topic:#{quoted_post.topic.id}"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "does nothing because the display name is the username" do
+        expect(quote_rewriter.rewrite_cooked_display_name(doc, "Jeff", "Mr. Atwood").to_html).to eq(
+          post.cooked.strip,
+        )
+      end
+    end
+
+    context "when using the new quote format" do
+      let(:post_attributes) { { raw: <<~RAW } }
+            [quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
+            quoted post
+            [/quote]
+          RAW
+
+      it "rewrites the display name" do
+        expect(
+          quote_rewriter.rewrite_cooked_display_name(doc, "Jeff", "Mr. Atwood").to_html,
+        ).to match_html(<<~HTML.strip)
+          
+        HTML
+      end
+    end
+  end
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 7c0075e8c45..892fafc04fd 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -159,6 +159,19 @@ RSpec.describe User do
         expect(SidebarSectionLink.exists?(linkable_type: "Tag", user_id: user.id)).to eq(false)
       end
     end
+
+    describe "#change_display_name" do
+      it "enqueues a job to retroactively update display name in quotes, etc." do
+        expect_enqueued_with(
+          job: :change_display_name,
+          args: {
+            user_id: user.id,
+            old_name: "Bruce Wayne",
+            new_name: "Batman",
+          },
+        ) { user.update(name: "Batman") }
+      end
+    end
   end
 
   describe "Validations" do