Don't send comment email to suspended or inactive users. #1363
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| it "should not notify the user who created the comment" do | ||
| user = create(:user, confirmed_at: Time.now, email: "[email protected]") | ||
| Comment.create!(comment: "Hello", user: user, commentable: entity) |
Check notice
Code scanning / Rubocop
Prefer Ruby 1.9 hash syntax { a: 1, b: 2 } over 1.8 syntax { :a => 1, :b => 2 }. Note test
|
I need to fix failing feature spec before merging |
app/models/polymorphic/comment.rb
Outdated
| @@ -53,7 +53,8 @@ def subscribe_user_to_entity(u = user) | |||
| # Notify subscribed users when a comment is added, unless user created this comment | |||
| def notify_subscribers | |||
| commentable.subscribed_users.reject { |user_id| user_id == user.id }.each do |subscriber_id| | |||
| if subscriber = User.find_by_id(subscriber_id) | |||
| subscriber = User.find_by_id(subscriber_id) | |||
| if subscriber&.confirmed? && !subscriber.awaits_approval? && !subscriber.suspended? && subscriber.email.present? | |||
There was a problem hiding this comment.
Can you refactor this to a method on User, something like emailable?
app/models/polymorphic/comment.rb
Outdated
| @@ -53,7 +53,8 @@ def subscribe_user_to_entity(u = user) | |||
| # Notify subscribed users when a comment is added, unless user created this comment | |||
| def notify_subscribers | |||
| commentable.subscribed_users.reject { |user_id| user_id == user.id }.each do |subscriber_id| | |||
| if subscriber = User.find_by_id(subscriber_id) | |||
| subscriber = User.find_by_id(subscriber_id) | |||
There was a problem hiding this comment.
Assuming this is an activerecord relation; not an array of ids (hard to tell), the below
commentable.subscribed_users.reject { |user_id| user_id == user.id }
should be refactored to a scope or .where.not(user+id: user.id); so that you don't do a loop of user objects you already have.
If its an array of user ids; it should still be refactored to something like
other_users = User.where(id: commentable.subscribed_users.reject { |user_id| user_id == user.id }.collect(&:subscriber_id))
Then you can further add scopes for currently_emailable applying your conditions below.
Avoid N+1 issues
|
Good call, I'll make those changes.
Unfortunately subscribed_users is an array of IDs rather than a relation.
We really need to change that in future. I've popped in an issue just for
that.
…On Sat, Sep 21, 2024, 11:13 AM CloCkWeRX ***@***.***> wrote:
***@***.**** requested changes on this pull request.
N+1 issues if it is likely to be more than 1-2 subscribed users for a
comment; .emailable? or a similar method to consolidate the logic in one
spot.
—
Reply to this email directly, view it on GitHub
<#1363 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABENTQNFXVEBJATRRAKYPDZXTP6FAVCNFSM6AAAAABOTE3PGOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJZGY2DANJZG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@CloCkWeRX ok to merge? let me know if any further thoughts |
Users who are suspended, inactive or not yet confirmed should not receive comment notifications.