DEV: Redesign chat mentions #24752
Merged
DEV: Redesign chat mentions #24752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
7 times, most recently
from
b2cb27f
to
2887a5e
Compare
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
4 times, most recently
from
7c338d0
to
1c8f343
Compare
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
2 times, most recently
from
ad23354
to
d9ba3a7
Compare
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
15 times, most recently
from
e839fb4
to
542a1b0
Compare
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
2 times, most recently
from
fa0534e
to
a06deea
Compare
jjaffeux
reviewed
Jan 8, 2024
plugins/chat/db/post_migrate/20231227160005_make_type_on_chat_mentions_non_nullable.rb
Outdated
Show resolved
Hide resolved
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
2 times, most recently
from
cc139ae
to
aef0bfd
Compare
|
@jjaffeux, I've rebased the branch and also fixed test failures (the most of them were because of a typo). I've also addressed your recent comment regarding |
jjaffeux
approved these changes
Jan 16, 2024
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
2 times, most recently
from
fccece3
to
c687487
Compare
rather than calling an overarching service that does other stuff
… true when altering indexes
With the previous condition we risked to accidentally corrupt here and all mentions, since they don't have target_id
AndrewPrigorshnev
force-pushed
the
dev/rethink-mass-mentions-polymorphic
branch
from
c687487
to
87e8de1
Compare
ZogStriP
reviewed
Jan 17, 2024
| target_id = user_id | ||
| elsif mention_klass == ::Chat::GroupMention | ||
| begin | ||
| target_id = Group.where("LOWER(name) = ?", "#{mention_type}").first.id |
There was a problem hiding this comment.
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 the id.
Suggested change
| target_id = Group.where("LOWER(name) = ?", "#{mention_type}").first.id | |
| target_id = Group.where("LOWER(name) = ?", "#{mention_type}").pick(:id) |
There was a problem hiding this comment.
Sorry, merged the PR before I saw this. I've fixed it in a follow-up – #25301.
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/query-causing-400-cpu-load/285542/27 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment, when someone is mentioning a group, or using
hereorallmention, we create achat_mentionrecord per user. What we want instead is to have special kinds of mentions, so we can create only onechat_mentionrecord in such cases. This PR implements that.Note, that such mentions will still have N related notifications, one notification per a user. We don't expect we'll have performance problems on the notifications side, but if at some point we do, we should be able to solve them on the side of notifications (notifications are handled in jobs, also some little delays with the notifications are acceptable, so we can make sure notifications are properly queued, and that processing of every notification is fast enough to make delays small enough).
The preparation work for this PR was done in fbd24fa, where we make it possible for one mention to have several related notifications.
A pretty tricky part of this PR is schema and data migration, I've explained related details inline on the migration files.