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