Skip to content
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

Fix notifications for non-contacts interactions #6498

Closed

Conversation

@cmrd-senya
Copy link
Member

commented Oct 13, 2015

will fix #5522

@@ -99,6 +99,9 @@ def self.federated_class
def initialize(person, target, text)
@text = text
super(person, target)
authors = target.comments_authors
author_people = Person.find(authors)
@dispatcher_opts = {:additional_subscribers => author_people}

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Oct 13, 2015

Use the new Ruby 1.9 hash syntax.

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

I don't like too much that where super is called is important. How about having @dispatcher_opts ||= {} in the parent?

@@ -55,6 +55,10 @@ class Post < ActiveRecord::Base
joins(:likes).where(:likes => {:author_id => person.id})
}

scope :comments_authors_for_post, ->(id) {

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Oct 13, 2015

Use the lambda method for multi-line lambdas.

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

Mmmh, gotta disable this one, it's rather arbitrary.

@@ -55,6 +55,10 @@ class Post < ActiveRecord::Base
joins(:likes).where(:likes => {:author_id => person.id})
}

scope :comments_authors_for_post, ->(id) {
joins(:comments).select("comments.*").where(:id => id)

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Oct 13, 2015

Use the new Ruby 1.9 hash syntax.

@@ -44,6 +44,15 @@ class StatusMessage < Post
joins(:mentions).where(:mentions => {:person_id => person.id})
}

def comments_authors
authors = [];

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Oct 13, 2015

Do not use semicolons to terminate expressions.

@@ -61,5 +61,8 @@ def notify_users
@users.find_each do |user|
Notification.notify(user, @object, @object.author)
end
if @object.target.author.owner

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Oct 13, 2015

Use a guard clause instead of wrapping the code inside a conditional expression.

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:5522-notifications-issues branch 2 times, most recently from f277812 to 3704df7 Oct 13, 2015

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2015

This patch now sends notifications in the all scenarios, listed in #5522.

It is only left to make it beautiful now.

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2015

I probably need some review on this code.

@@ -61,5 +61,12 @@ def notify_users
@users.find_each do |user|
Notification.notify(user, @object, @object.author)
end
additional_subscriber = nil

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

No need for this line.

| text | great post! |
And I press "Comment"
Then I should see "less than a minute ago" within ".comment"
And I sign out

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

When

| text | great post! |
And I press "Comment"
Then I should see "less than a minute ago" within ".comment"
And I sign out

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

When

And I sign in as "carol@carol.carol"
And I follow "Notifications" in the header
Then the notification dropdown should be visible
Then I should see "also commented on"

This comment has been minimized.

Copy link
@jhass
Then I should see "also commented on"
And I should have 2 email delivery

Scenario: connected user comments in reply to my comment to an uncommented user post

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

unconnected?

Then the notification dropdown should be visible
Then I should see "also commented on"
And I should have 3 email delivery

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

Aren't we missing one where Carol comments, Bob comments, Carol comments, Bob should have a notification?

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Oct 13, 2015

Author Member

You mean "Bob comments on Alice's post, Carol also comments on that post -> notification for Bob" that received notification fine before my patch? I'm fine implementing it.

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

I didn't read the surrounding cukes tbh, but yeah if it's not there already it might be a good addition here.

@@ -55,6 +55,10 @@ class Post < ActiveRecord::Base
joins(:likes).where(:likes => {:author_id => person.id})
}

scope :comments_authors_for_post, ->(id) {
joins(:comments).select("comments.*").where(:id => id)
}

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member

This is just reimplementing includes(:comments)

authors.push(record.author_id)
end
authors.uniq
end

This comment has been minimized.

Copy link
@jhass

jhass Oct 13, 2015

Member
Person.where(id: comments.select(:author_id).distinct)

Return the relation so no query is made until access and the caller can decide on the data to fetch without doing more than one query.

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:5522-notifications-issues branch from 3704df7 to 55ccade Oct 14, 2015

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:5522-notifications-issues branch from 55ccade to bcf1ba5 Oct 14, 2015

@cmrd-senya cmrd-senya changed the title [WIP] Fix notifications for non-contacts interactions Fix notifications for non-contacts interactions Oct 14, 2015

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2015

Looks like I've finished, review please.

@@ -98,6 +98,7 @@ def self.federated_class

def initialize(person, target, text)
@text = text
@dispatcher_opts = {additional_subscribers: target.comments_authors.where("id !=#{person.id}")}

This comment has been minimized.

Copy link
@jhass

jhass Oct 14, 2015

Member

where.not(id: person.id)


Notification.notify(additional_subscriber, @object, @object.author) if additional_subscriber &&
additional_subscriber != @object.author.owner &&
!@users.exists?(additional_subscriber.id)

This comment has been minimized.

Copy link
@jhass

jhass Oct 14, 2015

Member

Make this a regular if, don't add newlines to a modifer-if

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Oct 14, 2015

Author Member

Rubocop asked me of opposite :)

This comment has been minimized.

Copy link
@jhass

jhass Oct 14, 2015

Member

Meh, extract the condition to private method perhaps?

additional_subscriber = @object.post.author.owner
end

Notification.notify(additional_subscriber, @object, @object.author) if needs_notification?(additional_subscriber)

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Oct 14, 2015

Author Member

@jhass, how about this?

This comment has been minimized.

Copy link
@jhass

jhass Oct 14, 2015

Member

yes, that's better

@jhass jhass added this to the 0.5.5.0 milestone Oct 14, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

Merged as dc02c53 69b46df

Thanks.

@jhass jhass closed this Oct 14, 2015

@cmrd-senya cmrd-senya deleted the cmrd-senya:5522-notifications-issues branch Oct 14, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

Wow, and even in stable? Awesome, thank you very much @cmrd-senya, updating d-fr right now!

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2015

You are welcome. Happy to work on Diaspora.

@goobertron

This comment has been minimized.

Copy link

commented Oct 15, 2015

Brilliant! Thank you. Looking forward to seeing this in my pod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.