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
Mark Likes on comment notifications as read when visiting a post #8442
base: develop
Are you sure you want to change the base?
Mark Likes on comment notifications as read when visiting a post #8442
Conversation
def mark_like_on_comment_notifications_read(post_id) | ||
comment_ids = Comment.where(commentable_id: post_id) | ||
|
||
Notification.where(recipient_id: user.id, target_type: "Comment", target_id: comment_ids, unread: true) |
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.
Style/MultilineIfModifier: Favor a normal if-statement over a modifier clause in a multiline statement.
comment_ids = Comment.where(commentable_id: post_id) | ||
|
||
Notification.where(recipient_id: user.id, target_type: "Comment", target_id: comment_ids, unread: true) | ||
.update_all(unread: false) if comment_ids.any? |
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.
Rails/SkipsModelValidations: Avoid using update_all
because it skips validations.
While it's easy, it might not be the best approach. Just looping over all comments in a given thread is kinda bad, as there could be an unlimited number of comments to iterate on. You're fetching a potentially unlimited number of comments, and you're passing that into the lookup for the notification. This is bound to be slow. A better solution here is to adjust the notification itself to include a link that contains a get parameter containing the actual comment id. That way, you can fetch only notifications for that comment, which will be much quicker (and constant). It'll also limit the number of notifications you need to update to always exactly one, removing any other performance problem. Of course, that means that if a user has multiple of their comments liked in the same thread, it would only mark one notification as read, leaving the other one as unread. I'm not sure if this would be a bug or a feature, I don't actually know what our users would expect to happen in this case. Ultimately, I'd go with your initial implementation and only mark one notification as read. If we get complaints about this, we can move the the other (slower) impl. |
Yes, looping through all comments every time a post gets loaded is a really bad idea. We have some posts with a lot of comments, where loading the comments is already slow (which happens async at the moment, so at least the post loads fast). But now this would also slow down the loading of the post itself.
I think it's fine to only mark the notification as read when looking at the specific comment (so clicking the notification). It's the same as it works for posts, where it only gets marked as read when you visit the specific post, but when you scroll over the post on a stream, and maybe read it, it doesn't mark it as read. So I would even consider marking all notifications of comments on the same post (which would happen with the loop over all comments method) as a "bug", given our current behavior for posts. |
The problem is that our current impls are inconsistent, too. If you get mentioned two times in the same comment thread, accessing the post will mark both "you have been mentioned" notifications as read. Performance-wise, it's a bit less severe as we're joining over the comments in the database, but we could to the same, here, too. |
Just to be sure, as I'm not familiar with ActiveRecord. My actual code is not looping, is it? About the behavior, at the moment when a user visits a post, every notification linked to that post is marked as read: mention in the post, comment on the post, mention in a comment on the post, like on the post. That's why as said, to be consistent, I also added "like on a comment on the post" to this list. To me in the end it makes sense to assume that if users visited a post, they read the comments thread. |
Oh, you are right. But it's also kinda consistent, as the mentioned notification replaces/counts as the normal "commented" notification (so you don't get one of these if you also get mentioned, no double notification). And the normal "commented" notification get marked as read with the post. if you would want it completely consistent you would need to send 2 notifications, and attach the mention notification to the comment, which then can be marked as read only with this specific comment. But I think that would be worse UX.
Ok, it's not "looping", but it's still loading all the comments on a post.
The problem with likes on comments is, it's linked to the comment, not to the post, that's why at the moment it doesn't get marked as read. And as mentioned above, the "mention in a comment on the post" has kind of a special role, as it's a two in one notification and also replaces the "comment on the post" in that case, that's why it gets marked as read with the post. |
So this is fixing the fact that "Like on comment" notification is not marked as read when one visits a post.
Initially, I was thinking about marking it read only when the actual comment is visited, like:
But then I saw in the code that everything is marked as read when a post is visited (mentions in comment too for example) so I kept this consistent.
I need help with rubocop: