Improvements for user renaming (#5810)

* FEATURE: Update avatars in posts and revisions when user gets renamed

* FIX: Replace username in deleted posts when user gets renamed

* FEATURE: Replace username in notifications when user gets renamed

FEATURE: Update mentions and quotes when user gets merged

app/jobs/regular/update_username.rb

@@ -3,21 +3,22 @@ module Jobs
 
     def execute(args)
       @user_id = args[:user_id]
-
+      @old_username = args[:old_username]
-      username = args[:old_username]
-      @raw_mention_regex = /(?:(?<![\w`_])|(?<=_))@#{username}(?:(?![\w\-\.])|(?=[\-\.](?:\s|$)))/i
-      @raw_quote_regex = /(\[quote\s*=\s*["'']?)#{username}(\,?[^\]]*\])/i
-      @cooked_mention_username_regex = /^@#{username}$/i
-      @cooked_mention_user_path_regex = /^\/u(?:sers)?\/#{username}$/i
-      @cooked_quote_username_regex = /(?<=\s)#{username}(?=:)/i
       @new_username = args[:new_username]
 
+      @raw_mention_regex = /(?:(?<![\w`_])|(?<=_))@#{@old_username}(?:(?![\w\-\.])|(?=[\-\.](?:\s|$)))/i
+      @raw_quote_regex = /(\[quote\s*=\s*["'']?)#{@old_username}(\,?[^\]]*\])/i
+      @cooked_mention_username_regex = /^@#{@old_username}$/i
+      @cooked_mention_user_path_regex = /^\/u(?:sers)?\/#{@old_username}$/i
+      @cooked_quote_username_regex = /(?<=\s)#{@old_username}(?=:)/i
+
       update_posts
       update_revisions
+      update_notifications
     end
 
     def update_posts
-      Post.where(post_conditions("posts.id"), post_condition_args).find_each do |post|
+      Post.with_deleted.where(post_conditions("posts.id"), post_condition_args).find_each do |post|
         if update_raw!(post.raw)
           post.update_columns(raw: post.raw, cooked: update_cooked(post.cooked))
         end
@@ -39,6 +40,54 @@ module Jobs
       end
     end
 
+    def update_notifications
+      params = {
+        user_id: @user_id,
+        old_username: @old_username,
+        new_username: @new_username,
+        notification_types_with_correct_user_id: [
+          Notification.types[:granted_badge],
+          Notification.types[:group_message_summary]
+        ],
+        invitee_accepted_notification_type: Notification.types[:invitee_accepted]
+      }
+
+      Notification.exec_sql(<<~SQL, params)
+        UPDATE notifications AS n
+        SET data = (data :: JSONB ||
+                    jsonb_strip_nulls(
+                        jsonb_build_object(
+                            'original_username', CASE data :: JSONB ->> 'original_username'
+                                                 WHEN :old_username
+                                                   THEN :new_username
+                                                 ELSE NULL END,
+                            'display_username', CASE data :: JSONB ->> 'display_username'
+                                                WHEN :old_username
+                                                  THEN :new_username
+                                                ELSE NULL END,
+                            'username', CASE data :: JSONB ->> 'username'
+                                        WHEN :old_username
+                                          THEN :new_username
+                                        ELSE NULL END
+                        )
+                    )) :: JSON
+        WHERE EXISTS(
+                  SELECT 1
+                  FROM posts AS p
+                  WHERE p.topic_id = n.topic_id
+                        AND p.post_number = n.post_number
+                        AND p.user_id = :user_id)
+              OR (n.notification_type IN (:notification_types_with_correct_user_id) AND n.user_id = :user_id)
+              OR (n.notification_type = :invitee_accepted_notification_type
+                  AND EXISTS(
+                      SELECT 1
+                      FROM invites i
+                      WHERE i.user_id = :user_id AND n.user_id = i.invited_by_id
+                  )
+              )
+      SQL
+    end
+
   protected
 
     def post_conditions(post_id_column)
@@ -82,13 +131,17 @@ module Jobs
       end
 
       doc.css("aside.quote > div.title").each do |div|
-        # TODO Update avatar URL
         div.children.each do |child|
           child.content = child.content.gsub(@cooked_quote_username_regex, @new_username) if child.text?
         end
+        div.at_css("img.avatar")&.replace(avatar_img)
       end
 
       doc.to_html
     end
+
+    def avatar_img
+      @avatar_img ||= PrettyText.avatar_img(User.find_by_id(@user_id).avatar_template, "tiny")
+    end
   end
 end

app/services/user_merger.rb

@@ -5,7 +5,7 @@ class UserMerger
   end
 
   def merge!
-    update_notifications
+    update_username
     move_posts
     update_user_ids
     merge_given_daily_likes
@@ -23,52 +23,10 @@ class UserMerger
 
   protected
 
-  def update_notifications
+  def update_username
-    params = {
+    Jobs::UpdateUsername.new.execute(user_id: @source_user.id,
-      source_user_id: @source_user.id,
+                                     old_username: @source_user.username,
-      source_username: @source_user.username,
+                                     new_username: @target_user.username)
-      target_username: @target_user.username,
-      notification_types_with_correct_user_id: [
-        Notification.types[:granted_badge],
-        Notification.types[:group_message_summary]
-      ],
-      invitee_accepted_notification_type: Notification.types[:invitee_accepted]
-    }
-
-    Notification.exec_sql(<<~SQL, params)
-      UPDATE notifications AS n
-      SET data = (data :: JSONB ||
-                  jsonb_strip_nulls(
-                      jsonb_build_object(
-                          'original_username', CASE data :: JSONB ->> 'original_username'
-                                               WHEN :source_username
-                                                 THEN :target_username
-                                               ELSE NULL END,
-                          'display_username', CASE data :: JSONB ->> 'display_username'
-                                              WHEN :source_username
-                                                THEN :target_username
-                                              ELSE NULL END,
-                          'username', CASE data :: JSONB ->> 'username'
-                                      WHEN :source_username
-                                        THEN :target_username
-                                      ELSE NULL END
-                      )
-                  )) :: JSON
-      WHERE EXISTS(
-                SELECT 1
-                FROM posts AS p
-                WHERE p.topic_id = n.topic_id
-                      AND p.post_number = n.post_number
-                      AND p.user_id = :source_user_id)
-            OR (n.notification_type IN (:notification_types_with_correct_user_id) AND n.user_id = :source_user_id)
-            OR (n.notification_type = :invitee_accepted_notification_type
-                AND EXISTS(
-                    SELECT 1
-                    FROM invites i
-                    WHERE i.user_id = :source_user_id AND n.user_id = i.invited_by_id
-                )
-            )
-    SQL
   end
 
   def move_posts

spec/services/user_merger_spec.rb

@@ -289,69 +289,6 @@ describe UserMerger do
       expect(Notification.where(user_id: target_user.id).count).to eq(2)
       expect(Notification.where(user_id: source_user.id).count).to eq(0)
     end
-
-    def create_notification(type, notified_user, post, data = {})
-      Fabricate(
-        :notification,
-        notification_type: Notification.types[type],
-        user: notified_user,
-        data: data.to_json,
-        topic: post&.topic,
-        post_number: post&.post_number
-      )
-    end
-
-    def notification_data(notification)
-      JSON.parse(notification.reload.data, symbolize_names: true)
-    end
-
-    def original_and_display_username(user)
-      { original_username: user.username, display_username: user.username, foo: "bar" }
-    end
-
-    def original_username_and_some_text_as_display_username(user)
-      { original_username: user.username, display_username: "some text", foo: "bar" }
-    end
-
-    def only_display_username(user)
-      { display_username: user.username }
-    end
-
-    def username_and_something_else(user)
-      { username: user.username, foo: "bar" }
-    end
-
-    it "updates notification data" do
-      notified_user = Fabricate(:user)
-      p1 = Fabricate(:post, post_number: 1, user: source_user)
-      p2 = Fabricate(:post, post_number: 1, user: walter)
-      Fabricate(:invite, invited_by: notified_user, user: source_user)
-      Fabricate(:invite, invited_by: notified_user, user: walter)
-
-      n01 = create_notification(:mentioned, notified_user, p1, original_and_display_username(source_user))
-      n02 = create_notification(:mentioned, notified_user, p2, original_and_display_username(walter))
-      n03 = create_notification(:mentioned, notified_user, p1, original_username_and_some_text_as_display_username(source_user))
-      n04 = create_notification(:mentioned, notified_user, p1, only_display_username(source_user))
-      n05 = create_notification(:invitee_accepted, notified_user, nil, only_display_username(source_user))
-      n06 = create_notification(:invitee_accepted, notified_user, nil, only_display_username(walter))
-      n07 = create_notification(:granted_badge, source_user, nil, username_and_something_else(source_user))
-      n08 = create_notification(:granted_badge, walter, nil, username_and_something_else(walter))
-      n09 = create_notification(:group_message_summary, source_user, nil, username_and_something_else(source_user))
-      n10 = create_notification(:group_message_summary, walter, nil, username_and_something_else(walter))
-
-      merge_users!
-
-      expect(notification_data(n01)).to eq(original_and_display_username(target_user))
-      expect(notification_data(n02)).to eq(original_and_display_username(walter))
-      expect(notification_data(n03)).to eq(original_username_and_some_text_as_display_username(target_user))
-      expect(notification_data(n04)).to eq(only_display_username(target_user))
-      expect(notification_data(n05)).to eq(only_display_username(target_user))
-      expect(notification_data(n06)).to eq(only_display_username(walter))
-      expect(notification_data(n07)).to eq(username_and_something_else(target_user))
-      expect(notification_data(n08)).to eq(username_and_something_else(walter))
-      expect(notification_data(n09)).to eq(username_and_something_else(target_user))
-      expect(notification_data(n10)).to eq(username_and_something_else(walter))
-    end
   end
 
   context "post actions" do
@@ -1068,4 +1005,13 @@ describe UserMerger do
 
     expect(User.find_by_username(source_user.username)).to be_nil
   end
+
+  it "updates the username" do
+    Jobs::UpdateUsername.any_instance
+      .expects(:execute)
+      .with(user_id: source_user.id, old_username: 'alice1', new_username: 'alice')
+      .once
+
+    merge_users!
+  end
 end

spec/services/username_changer_spec.rb

@@ -98,13 +98,15 @@ describe UsernameChanger do
       before { UserActionCreator.enable }
       after { UserActionCreator.disable }
 
-      def create_post_and_change_username(args = {})
+      def create_post_and_change_username(args = {}, &block)
         post = create_post(args.merge(topic_id: topic.id))
 
         args.delete(:revisions)&.each do |revision|
           post.revise(post.user, revision, force_new_version: true)
         end
 
+        block.call(post) if block
+
         UsernameChanger.change(user, 'bar')
         post.reload
       end
@@ -231,14 +233,26 @@ describe UsernameChanger do
           expect(post.revisions[2].modifications["cooked"][0]).to eq(%Q(<p>Hello <a class="mention" href="/u/bar">@bar</a>!</p>))
           expect(post.revisions[2].modifications["cooked"][1]).to eq(%Q(<p>Hello <a class="mention" href="/u/bar">@bar</a>!!</p>))
         end
+
+        it 'replaces mentions in posts marked for deletion' do
+          post = create_post_and_change_username(raw: "Hello @foo") do |p|
+            PostDestroyer.new(p.user, p).destroy
+          end
+
+          expect(post.raw).to_not include("@foo")
+          expect(post.cooked).to_not include("foo")
+          expect(post.revisions.count).to eq(1)
+
+          expect(post.revisions[0].modifications["raw"][0]).to eq("Hello @bar")
+          expect(post.revisions[0].modifications["cooked"][0]).to eq(%Q(<p>Hello <a class="mention" href="/u/bar">@bar</a></p>))
+        end
       end
 
       context 'quotes' do
         let(:quoted_post) { create_post(user: user, topic: topic, post_number: 1, raw: "quoted post") }
+        let(:avatar_url) { user.avatar_template.gsub("{size}", "40") }
 
         it 'replaces the username in quote tags' do
-          avatar_url = user.avatar_template_url.gsub("{size}", "40")
-
           post = create_post_and_change_username(raw: <<~RAW)
             Lorem ipsum
 
@@ -280,7 +294,7 @@ describe UsernameChanger do
             <aside class="quote no-group" data-post="1" data-topic="#{quoted_post.topic.id}">
             <div class="title">
             <div class="quote-controls"></div>
-            <img alt width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
+            <img alt='' width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
             <blockquote>
             <p>quoted post</p>
             </blockquote>
@@ -288,7 +302,7 @@ describe UsernameChanger do
             <aside class="quote no-group">
             <div class="title">
             <div class="quote-controls"></div>
-            <img alt width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
+            <img alt='' width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
             <blockquote>
             <p>quoted post</p>
             </blockquote>
@@ -296,7 +310,7 @@ describe UsernameChanger do
             <aside class="quote no-group" data-post="1" data-topic="#{quoted_post.topic.id}">
             <div class="title">
             <div class="quote-controls"></div>
-            <img alt width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
+            <img alt='' width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
             <blockquote>
             <p>quoted post</p>
             </blockquote>
@@ -305,10 +319,133 @@ describe UsernameChanger do
           HTML
         end
 
+        context 'simple quote' do
+          let(:raw) do <<~RAW
+              Lorem ipsum
+
+              [quote="foo, post:1, topic:#{quoted_post.topic.id}"]
+              quoted post
+              [/quote]
+            RAW
+          end
+
+          let(:expected_raw) do
+            <<~RAW.strip
+              Lorem ipsum
+
+              [quote="bar, post:1, topic:#{quoted_post.topic.id}"]
+              quoted post
+              [/quote]
+            RAW
+          end
+
+          let(:expected_cooked) do
+            <<~HTML
+              <p>Lorem ipsum</p>
+              <aside class="quote no-group" data-post="1" data-topic="#{quoted_post.topic.id}">
+              <div class="title">
+              <div class="quote-controls"></div>
+              <img alt='' width="20" height="20" src="#{avatar_url}" class="avatar"> bar:</div>
+              <blockquote>
+              <p>quoted post</p>
+              </blockquote>
+              </aside>
+            HTML
+          end
+
+          it 'replaces the username in quote tags when the post is deleted' do
+            post = create_post_and_change_username(raw: raw) do |p|
+              PostDestroyer.new(Discourse.system_user, p).destroy
+            end
+
+            expect(post.raw).to eq(expected_raw)
+            expect(post.cooked).to match_html(expected_cooked)
+          end
+
+          it 'replaces the username in quote tags when the post is marked as deleted' do
+            post = create_post_and_change_username(raw: raw) do |p|
+              PostDestroyer.new(p.user, p).destroy
+            end
+
+            expect(post.raw).to_not include("foo")
+            expect(post.cooked).to_not include("foo")
+            expect(post.revisions.count).to eq(1)
+
+            expect(post.revisions[0].modifications["raw"][0]).to eq(expected_raw)
+            expect(post.revisions[0].modifications["cooked"][0]).to match_html(expected_cooked)
+          end
+        end
+
         # TODO spec for quotes in revisions
       end
     end
 
+    context 'notifications' do
+      def create_notification(type, notified_user, post, data = {})
+        Fabricate(
+          :notification,
+          notification_type: Notification.types[type],
+          user: notified_user,
+          data: data.to_json,
+          topic: post&.topic,
+          post_number: post&.post_number
+        )
+      end
+
+      def notification_data(notification)
+        JSON.parse(notification.reload.data, symbolize_names: true)
+      end
+
+      def original_and_display_username(username)
+        { original_username: username, display_username: username, foo: "bar" }
+      end
+
+      def original_username_and_some_text_as_display_username(username)
+        { original_username: username, display_username: "some text", foo: "bar" }
+      end
+
+      def only_display_username(username)
+        { display_username: username }
+      end
+
+      def username_and_something_else(username)
+        { username: username, foo: "bar" }
+      end
+
+      it 'replaces usernames in notifications' do
+        renamed_user = Fabricate(:user, username: "alice")
+        another_user = Fabricate(:user, username: "another_user")
+        notified_user = Fabricate(:user)
+        p1 = Fabricate(:post, post_number: 1, user: renamed_user)
+        p2 = Fabricate(:post, post_number: 1, user: another_user)
+        Fabricate(:invite, invited_by: notified_user, user: renamed_user)
+        Fabricate(:invite, invited_by: notified_user, user: another_user)
+
+        n01 = create_notification(:mentioned, notified_user, p1, original_and_display_username("alice"))
+        n02 = create_notification(:mentioned, notified_user, p2, original_and_display_username("another_user"))
+        n03 = create_notification(:mentioned, notified_user, p1, original_username_and_some_text_as_display_username("alice"))
+        n04 = create_notification(:mentioned, notified_user, p1, only_display_username("alice"))
+        n05 = create_notification(:invitee_accepted, notified_user, nil, only_display_username("alice"))
+        n06 = create_notification(:invitee_accepted, notified_user, nil, only_display_username("another_user"))
+        n07 = create_notification(:granted_badge, renamed_user, nil, username_and_something_else("alice"))
+        n08 = create_notification(:granted_badge, another_user, nil, username_and_something_else("another_user"))
+        n09 = create_notification(:group_message_summary, renamed_user, nil, username_and_something_else("alice"))
+        n10 = create_notification(:group_message_summary, another_user, nil, username_and_something_else("another_user"))
+
+        UsernameChanger.change(renamed_user, "bob")
+
+        expect(notification_data(n01)).to eq(original_and_display_username("bob"))
+        expect(notification_data(n02)).to eq(original_and_display_username("another_user"))
+        expect(notification_data(n03)).to eq(original_username_and_some_text_as_display_username("bob"))
+        expect(notification_data(n04)).to eq(only_display_username("bob"))
+        expect(notification_data(n05)).to eq(only_display_username("bob"))
+        expect(notification_data(n06)).to eq(only_display_username("another_user"))
+        expect(notification_data(n07)).to eq(username_and_something_else("bob"))
+        expect(notification_data(n08)).to eq(username_and_something_else("another_user"))
+        expect(notification_data(n09)).to eq(username_and_something_else("bob"))
+        expect(notification_data(n10)).to eq(username_and_something_else("another_user"))
+      end
+    end
   end
 
 end