-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Handle Reactions Without Users When Sending Notifications #5820
Handle Reactions Without Users When Sending Notifications #5820
Conversation
Thanks @abdellani for taking this on! The first thing that pops in my mind is: we should write a test that makes sure this solves the problem for good, what do you think? You also mention the query becomes slower, can you post some data about it? |
I used the following code before the after the new code : u=User.first
rxn = Reaction.create(
user_id: User.last.id,
category: "like",
reactable: u.articles.first
)
Notification.send_reaction_notification_without_delay(rxn, u) Form the last instruction, I got the following results using the original code: (byebug) reaction_siblings
Reaction Load (3.4ms) SELECT "reactions".* FROM "reactions" WHERE "reactions"."reactable_id" = $1 AND "reactions"."reactable_type" = $2 AND "reactions"."user_id" != $3 ORDER BY created_at DESC LIMIT $4 [["reactable_id", 3], ["reactable_type", "Article"], ["user_id", 1], ["LIMIT", 11]]
Article Load (1.2ms) SELECT "articles".* FROM "articles" WHERE "articles"."id" = $1 [["id", 3]]
User Load (0.9ms) SELECT "users".* FROM "users" WHERE "users"."id" = $1 [["id", 10]] After the update, I had : (byebug) reaction_siblings
SQL (6.1ms) ...
Article Load (0.8ms) SELECT "articles".* FROM "articles" WHERE "articles"."id" = $1 [["id", 3]]
#<ActiveRecord::Relation [#<Reaction id: 1, category: "like", created_at: "2020-01-31 12:52:53", points: 1.0, reactable_id: 3, reactable_type: "Article", status: "valid", updated_at: "2020-01-31 12:52:53", user_id: 10>]>
|
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.
I second @rhymes, let's write a test for this otherwise I think the solution LGTM!
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.
LGTM! Thanks for adding the spec! 🚀
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.
This looks good to me. We'll see in prod if it's meaningfully slower.
What type of PR is this? (check all applicable)
Description
I tried to fix #5508 by checking the reaction owners' existence in the database when creating
reaction_siblings
. The problem is that the query becomes slower compared to the original code.If this solution is not acceptable, could you please give me some guidance to improve it?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?