-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEV: Redesign chat mentions #24752
DEV: Redesign chat mentions #24752
Changes from all commits
524564d
56fb36a
6a2be2b
b03067c
3be898a
8fc3dac
b4f8c99
bb5016e
956b934
fd16607
18c14c1
8400b4b
eeefa7f
b53db84
6722c7c
87e8de1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# frozen_string_literal: true | ||
|
||
module Chat | ||
class AllMention < Mention | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
module Chat | ||
class GroupMention < Mention | ||
belongs_to :group, foreign_key: :target_id | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# frozen_string_literal: true | ||
|
||
module Chat | ||
class HereMention < Mention | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,22 @@ class Message < ActiveRecord::Base | |
dependent: :destroy, | ||
class_name: "Chat::Mention", | ||
foreign_key: :chat_message_id | ||
has_many :user_mentions, | ||
dependent: :destroy, | ||
class_name: "Chat::UserMention", | ||
foreign_key: :chat_message_id | ||
has_many :group_mentions, | ||
dependent: :destroy, | ||
class_name: "Chat::GroupMention", | ||
foreign_key: :chat_message_id | ||
has_one :all_mention, | ||
dependent: :destroy, | ||
class_name: "Chat::AllMention", | ||
foreign_key: :chat_message_id | ||
has_one :here_mention, | ||
dependent: :destroy, | ||
class_name: "Chat::HereMention", | ||
foreign_key: :chat_message_id | ||
|
||
scope :in_public_channel, | ||
-> do | ||
|
@@ -248,14 +264,10 @@ def url | |
end | ||
|
||
def upsert_mentions | ||
mentioned_user_ids = parsed_mentions.all_mentioned_users_ids | ||
old_mentions = chat_mentions.pluck(:user_id) | ||
|
||
mentioned_user_ids_to_drop = old_mentions - mentioned_user_ids | ||
delete_mentions(mentioned_user_ids_to_drop) | ||
|
||
mentioned_user_ids_to_add = mentioned_user_ids - old_mentions | ||
insert_mentions(mentioned_user_ids_to_add) | ||
upsert_user_mentions | ||
upsert_group_mentions | ||
create_or_delete_all_mention | ||
create_or_delete_here_mention | ||
end | ||
|
||
def in_thread? | ||
|
@@ -280,24 +292,16 @@ def invalidate_parsed_mentions | |
|
||
private | ||
|
||
def delete_mentions(user_ids) | ||
chat_mentions.where(user_id: user_ids).destroy_all | ||
def delete_mentions(mention_type, target_ids) | ||
chat_mentions.where(type: mention_type, target_id: target_ids).destroy_all | ||
end | ||
|
||
def insert_mentions(user_ids) | ||
return if user_ids.empty? | ||
|
||
now = Time.zone.now | ||
mentions = [] | ||
User | ||
.where(id: user_ids) | ||
.find_each do |user| | ||
mentions << { | ||
chat_message_id: self.id, | ||
user_id: user.id, | ||
created_at: now, | ||
updated_at: now, | ||
} | ||
def insert_mentions(type, target_ids) | ||
return if target_ids.empty? | ||
|
||
mentions = | ||
target_ids.map do |target_id| | ||
{ chat_message_id: self.id, target_id: target_id, type: type } | ||
end | ||
|
||
Chat::Mention.insert_all(mentions) | ||
|
@@ -314,6 +318,38 @@ def message_too_long? | |
def ensure_last_editor_id | ||
self.last_editor_id ||= self.user_id | ||
end | ||
|
||
def create_or_delete_all_mention | ||
if !parsed_mentions.has_global_mention && all_mention.present? | ||
all_mention.destroy! | ||
association(:all_mention).reload | ||
elsif parsed_mentions.has_global_mention && all_mention.blank? | ||
build_all_mention.save! | ||
end | ||
end | ||
|
||
def create_or_delete_here_mention | ||
if !parsed_mentions.has_here_mention && here_mention.present? | ||
here_mention.destroy! | ||
association(:here_mention).reload | ||
elsif parsed_mentions.has_here_mention && here_mention.blank? | ||
build_here_mention.save! | ||
end | ||
end | ||
|
||
def upsert_group_mentions | ||
old_mentions = group_mentions.pluck(:target_id) | ||
new_mentions = parsed_mentions.groups_to_mention.pluck(:id) | ||
delete_mentions("Chat::GroupMention", old_mentions - new_mentions) | ||
insert_mentions("Chat::GroupMention", new_mentions - old_mentions) | ||
end | ||
|
||
def upsert_user_mentions | ||
old_mentions = user_mentions.pluck(:target_id) | ||
new_mentions = parsed_mentions.direct_mentions.pluck(:id) | ||
delete_mentions("Chat::UserMention", old_mentions - new_mentions) | ||
insert_mentions("Chat::UserMention", new_mentions - old_mentions) | ||
end | ||
Comment on lines
+322
to
+352
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if I follow this correctly we will in final call: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct. One thing though is that "multiple" is actually from one to two inserts (and maybe deletes) in normal cases. For example, if you mention several users and several groups like this that'll be two inserts:
The worst case is 4 inserts, but this should happen rarely:
We can do that, but is it really worth optimizing? Those inserts and deletes are lightweight, for example this is an insert that adds two user mentions: There is no much to win here. At the same time, I'm not quite happy with how this code looks now, and I believe we can make it more concise at some point and If we refactor it to make all inserts / deletes in one query, it'll become even worse, and it'll be harder to improve it in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, if you are comfortable with this, let's keep it that way. We can always reconsider if necessary. 👍 |
||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
module Chat | ||
class UserMention < Mention | ||
belongs_to :user, foreign_key: :target_id | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
AndrewPrigorshnev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
class AddTypeAndTargetIdToChatMentions < ActiveRecord::Migration[7.0] | ||
def up | ||
add_column :chat_mentions, :type, :string, null: true | ||
add_column :chat_mentions, :target_id, :integer, null: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the
|
||
change_column_null :chat_mentions, :user_id, true | ||
end | ||
|
||
def down | ||
change_column_null :chat_mentions, :user_id, false | ||
remove_column :chat_mentions, :target_id | ||
remove_column :chat_mentions, :type | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# frozen_string_literal: true | ||
|
||
class SetTypeAndTargetIdOnChatMentions < ActiveRecord::Migration[7.0] | ||
disable_ddl_transaction! | ||
BATCH_SIZE = 5000 | ||
|
||
def up | ||
begin | ||
updated_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) | ||
WITH cte AS (SELECT id, user_id | ||
FROM chat_mentions | ||
WHERE type IS NULL AND target_id IS NULL | ||
LIMIT :batch_size) | ||
UPDATE chat_mentions | ||
SET type = 'Chat::UserMention', target_id = cte.user_id | ||
FROM cte | ||
WHERE chat_mentions.id = cte.id; | ||
SQL | ||
end while updated_count > 0 | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this migration just sets the type for all old mentions to The fully correct way of migrating the data would be to figure out which mentions were At the same time, It seems we can get away with this simple migration instead. We still may decide to fully migrate old mentions correctly, but even if we decide so, it's better to do that in a follow-up. |
||
|
||
def down | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# frozen_string_literal: true | ||
|
||
class AddAndRemoveIndexesOnChatMentions < ActiveRecord::Migration[7.0] | ||
disable_ddl_transaction! | ||
def up | ||
remove_index :chat_mentions, | ||
name: :chat_mentions_index, | ||
algorithm: :concurrently, | ||
if_exists: true | ||
add_index :chat_mentions, %i[chat_message_id], algorithm: :concurrently | ||
add_index :chat_mentions, %i[target_id], algorithm: :concurrently | ||
end | ||
|
||
def down | ||
remove_index :chat_mentions, %i[target_id], algorithm: :concurrently, if_exists: true | ||
remove_index :chat_mentions, %i[chat_message_id], algorithm: :concurrently, if_exists: true | ||
add_index :chat_mentions, | ||
%i[chat_message_id user_id notification_id], | ||
unique: true, | ||
name: "chat_mentions_index", | ||
algorithm: :concurrently | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# frozen_string_literal: true | ||
|
||
class SetTypeAndTargetIdOnChatMentionsPostMigrate < ActiveRecord::Migration[7.0] | ||
disable_ddl_transaction! | ||
BATCH_SIZE = 5000 | ||
|
||
def up | ||
# we're setting it again in post-migration | ||
# in case some mentions have been created after we run | ||
# this query the first time in the regular migration | ||
begin | ||
updated_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) | ||
WITH cte AS (SELECT id, user_id | ||
FROM chat_mentions | ||
WHERE type IS NULL AND target_id IS NULL | ||
LIMIT :batch_size) | ||
UPDATE chat_mentions | ||
SET type = 'Chat::UserMention', target_id = cte.user_id | ||
FROM cte | ||
WHERE chat_mentions.id = cte.id; | ||
SQL | ||
end while updated_count > 0 | ||
end | ||
|
||
def down | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# frozen_string_literal: true | ||
|
||
class MakeTypeOnChatMentionsNonNullable < ActiveRecord::Migration[7.0] | ||
def up | ||
change_column_null :chat_mentions, :type, false | ||
end | ||
|
||
def down | ||
change_column_null :chat_mentions, :type, true | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# frozen_string_literal: true | ||
|
||
module Chat | ||
module GroupExtension | ||
extend ActiveSupport::Concern | ||
|
||
prepended do | ||
has_many :chat_mentions, class_name: "Chat::GroupMention", foreign_key: "target_id" | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: if you only care about some fields about 1 record, you can use
#pick
and list the fields you want.This avoids loading the whole record in memory (due to the
.first
) when we only want theid
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, merged the PR before I saw this. I've fixed it in a follow-up – #25301.