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

@@ -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

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

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

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(
         /(?<pre>\[quote\s*=\s*["'']?.*username:)#{old_username}(?<post>\,?[^\]]*\])/i,
@@ -18,7 +15,7 @@ class QuoteRewriter
     raw.gsub(pattern, "\\k<pre>#{new_username}\\k<post>")
   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 = /(?<pre>\[quote\s*=\s*["'']?)#{old_display_name}(?<post>\,[^\]]*username[^\]]*\])/i
+
+    raw.gsub(pattern, "\\k<pre>#{new_display_name}\\k<post>")
+  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)

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),
+          <aside class="quote no-group" data-username="codinghorror" data-post="1" data-topic="#{quoted_post.topic.id}">
+          <div class="title">
+          <div class="quote-controls"></div>
+          <img loading="lazy" alt="" width="24" height="24" src="#{avatar_url}" class="avatar"> Mr. Atwood:</div>
+          <blockquote>
+          <p>quoted post</p>
+          </blockquote>
+          </aside>
+        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

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)
+          <aside class="quote no-group" data-username="codinghorror" data-post="1" data-topic="#{quoted_post.topic.id}">
+          <div class="title">
+          <div class="quote-controls"></div>
+          <img loading="lazy" alt="" width="24" height="24" src="#{avatar_url}" class="avatar"> Mr. Atwood:</div>
+          <blockquote>
+          <p>quoted post</p>
+          </blockquote>
+          </aside>
+        HTML
+      end
+    end
+  end
+end

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