Mentions in comments backend changes #6818

cmrd-senya · 2016-04-29T19:00:47Z

This is an early preliminary PR with backend-changes to support mentions in comments (#1851).

It introduces changes to the DB schema making Mention model polymorphic and allowing a mention to belong to a comment. It also creates module Diaspora::MentionsContainer which incapsulates the logic of an object that supports keeping mentions. Now it includes logic I picked from the StatusMessage model.

ATM the mentions in comments are supported to the extent of placing user profile links instead of mention code in the mobile version (it is so because the mobile version doesn't require javascript code changes).

Changes summary:

Make Mention model polymorphic;
Introduce Diaspora::MentionsContainer which incapsulates the logic of an object that supports keeping mentions;
We don't filter mentions anymore, instead we send notification/show in streams only that mentions, which are visible to the recipient;
Create Notifications::Mentioned module which incapsulates common logic of mentioned notification;
Add Notifications::MentionedInComment and move old Notifications::Mentioned to Notifications::MentionedInPost;
Update the UI code to render new type of notification;
Hack comments notifications to avoid having double notification when mentioned in a comment;

Drive-by refactorings:

Move Notification.types out of the model to the controller;
Move StatusMessage#mentioned_people_names out of the model. Create a PersonPresenter.people_names method instead;
DRY UserController to use UserPreference::VALID_EMAIL_TYPES in the user_params method;

TODO:

Implement notifications for mentioning in comments
Implement restrictions of mentioning some kind of people (see comment)
Ensure comment is federated to new discussion participants
Ensure mentions in comment are filtered on federation receive
~~Ensure the filtered out mentions are replaced with links properly~~
Don't send redundant second notification when mentioned in comment.
Ensure email notifications work properly
~~Implement frontend javascript changes for comments publisher~~

SuperTux88 · 2016-04-29T19:32:39Z

app/models/person.rb

@@ -172,6 +172,10 @@ def self.search_order
    }.call
  end

+  def self.people_names(people)


## TODO ---- # don't put presentation logic in the model!

Also, this is only used once, why don't you just move .map(&:name).join(", ") to where its used? Maybe the controller is also not the best place, but it's better than split this and add it to the model.

Well, I like to have it as a separate method, if possible. I don't know of a better place for that though. Any ideas?

cmrd-senya · 2016-07-08T17:55:15Z

Ok, I have completed backend support for mentions in comments here. Front end changes are completely separate thing and I suppose they can be implemented&reviewed separetely. This PR can be tested by using the markdown syntax of mention directly: @{alice; alice@example.tld}.

Can be reviewed now.

jhass · 2016-07-11T09:27:41Z

app/helpers/notifications_helper.rb

+      post_link: link_to(post_page_title(post), post_path(post)).html_safe
+    }.tap {|opts|
+      if note.linked_object.instance_of?(Comment)
+        opts.merge!(comment_path: "#{post_path(post)}##{note.linked_object.guid}".html_safe)


IIRC path helpers take an anchor option, post_path(post, anchor: note.linked_object.guid).

jhass · 2016-07-11T09:42:21Z

Sorry I don't remember, since we allow to bring new people into the conversation by allowing the commenter to mention anyone of their contacts, did we decide how to federate the comment in case the mentioned person never got the post initially?

cmrd-senya · 2016-07-11T12:39:28Z

I walked quickly through the discussions and as far as I can see there was no decision. We allow to bring new people into the conversation for the public posts only. In this case we can add a mentioned person to the list of the comments subscribers and the comment will be federated to a new pod. After that, the new receiver pod may fetch the post if it's yet unknown. AFAIK, with the new federation it is nothing that we can't do.

Also I'm a bit concerned with the way notifications work in this PR. In cases when we mention a participant, the participant received two notifications (e.g. AlsoCommented and MentionedInComment). That's not too elegant. I'll seek for a way to mute CommentOnPost and AlsoCommented in case when we send MentionedInComment to a person.

Since these two issues are unresolved in the PR I'll set the status to [WIP] again.

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

SuperTux88 · 2016-07-11T19:03:09Z

app/models/notifications/mentioned_in_comment.rb

+      # mentioned in comment.
+      #
+      # I left intact the Notifications::Mentioned.notify and didn't investigate the message dispatcher
+      # but here I add parent_owner_id to the list so the filtering passes well.


This doesn't work, because private posts are received multiple times (to create the ShareVisibilities), so the mention-notification is only sent, if the user is in the recipient_ids (the user who received the visibility). But comments are only received once (all others are ignored), because all visibilities are already there. See AlsoCommented.

Hmm, I just tried to remove this method and everything seems to work fine. I made a mistake, thanks for pointing!

No, that will not work either. If you use the parent-method, it will work for public posts, but for private-posts it will only send a notification, if the mentioned person is the first who receives it.

I'm not even sure if we should extend from Mentioned, because the notify-method would be simpler (than the parent) if you completely write the logic, instead of trying to use some parent logic.

No, that will not work either. If you use the parent-method, it will work for public posts, but for private-posts it will only send a notification, if the mentioned person is the first who receives it.

Ah, ok, that means my tests don't cover that :)

I'm not even sure if we should extend from Mentioned, because the notify-method would be simpler (than the parent) if you completely write the logic, instead of trying to use some parent logic.

Yes, that what I probably should do.

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

cmrd-senya · 2016-07-26T18:04:04Z

@SuperTux88, I ended up with keeping inheritance. Please check 048d6b0 and tell me if you have any objections on the approach.

SuperTux88 · 2016-07-27T00:06:56Z

app/models/notifications/mentioned.rb

        create_notification(recipient.owner_id, mention, actor).email_the_user(mention, actor)
      end
    end
+
+    def self.get_mentions(mentionable, recipient_user_ids)


I think we shouldn't use get_, rubocop has a cop for this, but only for instance methods.

Flaburgan · 2016-11-23T19:37:55Z

No there is no front-end at the moment. I'm also open to merge this on d-fr if needed, though I agree with @SuperTux88 that it doesn't bring anything more than the local testing as it brings nothing new in the UI.

ghost · 2016-11-23T20:05:05Z

I think we can write a quick UI then. The code is already available. We can reuse the mention system used in the publisher. I won't be a big deal.

cmrd-senya · 2016-11-23T21:34:38Z

I think we can write a quick UI then.

It's not necessary. We can test mentions writing mention markdown clause in comments directly: @{name; id@pod.tld}. If we have a couple of pods running this PR, we can test any imaginable usecase live. Then, when this is merged, we can make the UI without rush.

ghost · 2016-11-24T13:23:54Z

I'm not sure our users will use it with the @{name;handle} syntax. I will merge this in diaspote and add the UI. Does this PR have a path to query the users that can be mentionned? Like there is for the publisher?

cmrd-senya · 2016-11-24T13:50:25Z

Does this PR have a path to query the users that can be mentionned?

No, there isn't since I haven't ever started the UI part.

I'm not sure our users will use it with the @{name;handle} syntax.

The syntax is @{name; handle}, the space after ; is important (that is a hardcoded regexp we have there). And I don't think it is necessary that every user of your pod will test this feature. It is fine to test it among a little group of people, even only ourselves. Again, testing it on "usual users" massively is not how I think we should do. It's better if manual testing is conducted by a person who have some technical skills and understanding of technologies. Most probably, that if a user fails to use mention markdown syntax correctly, then he would also fail to give a decent feedback. Then, if we rush with UI, it could make things more complicated, because you'll have to introduce even more changes, some of them to controllers. If there are any issues with the quickly made UI, it'll get harder to find what is broken.

It's enough even if one person (except of me) conducts testing if it is done well. If it is fine to test changes on the end-users right away, then why don't we just merge the PR to the develop branch right away without any more manual tests?

SuperTux88 · 2016-11-24T16:09:27Z

It's enough even if one person (except of me) conducts testing if it is done well.

Yes. I don't think that we have much benefit if we deploy this to a prod-pod and ask the users to test it. It is better when one person does a careful local testing with different users and as many edge cases the person can think of. This person can then also check the logs and check the expected behavior on every involved user. If you let multiple users test this in production, then they probably test it with their best friends (and always have a mutual connection), and that doesn't help much. But I can't stop somebody if he wants to test it in production, but I think local testing is easier and better.

ghost · 2016-11-24T16:10:22Z

And I don't think it is necessary that every user of your pod will test this feature. It is fine to test it among a little group of people, even only ourselves.

The users of our pod is already a small group. We are talking about less than 50 persons here ;)

Then, if we rush with UI, it could make things more complicated, because you'll have to introduce even more changes, some of them to controllers.

Really, the UI is a one-liner. mention_view.js can be used as is, as long as there's a path to query mentionnable people. See, I'm not kidding, it's really a one-liner:

this.mention = new app.views.PublisherMention({ el: this.$("#publisher_textarea_wrapper") });

That's it.

SuperTux88

Temporary review (but I wanted to give some quick feedback, so you can already fix some things, so we can merge this faster :) ).

I'll do a more careful review again on the weekend and also some testing.

SuperTux88 · 2016-11-23T20:28:44Z

spec/helper_methods.rb

+  def build_relayable_federation_entity(type, data={}, additional_xml_elements={})
+    attributes = FactoryGirl.attributes_for("#{type}_entity".to_sym, data)
+    entity_class = "DiasporaFederation::Entities::#{type.capitalize}".constantize
+    singlable_fields = attributes.keys - [:author_signature]


still: signable_fields

SuperTux88 · 2016-11-24T02:49:14Z

spec/integration/mentioning_spec.rb

+        false
+      else # post
+        object.author.local?
+      end


You can do:

object.is_a?(Post) && object.author.local?

Nit: Add a ? to the method-name: do_check_subscribers? or maybe only check_subscribers?

SuperTux88 · 2016-11-24T02:52:07Z

spec/integration/mentioning_spec.rb

  end

-  def text_mentioning(user)
-    "this is a text mentioning @{#{user.name}; #{user.diaspora_handle}} ... have fun testing!"
+  def user_is_notified?(user, object)


You can move this to the :be_mentioned_in matcher.

SuperTux88 · 2016-11-24T03:43:18Z

spec/integration/mentioning_spec.rb

+          before do
+            allow(DiasporaFederation.callbacks).to receive(:trigger).and_call_original
+            allow(DiasporaFederation.callbacks).to receive(:trigger).with(
+              :fetch_private_key, remote_raphael.diaspora_handle


You don't need the private key of remote_raphael to receive it.

SuperTux88 · 2016-11-24T03:48:41Z

spec/integration/mentioning_spec.rb

+          end
+
+          let(:mentioned_user) {
+            FactoryGirl.build(:user).tap {|user|


text_mentioning can also handle a person, so you don't need to create a user-object for this.

Also, if you are using it only in one test, you can assign it to a local variable in the test.

SuperTux88 · 2016-11-24T04:12:07Z

spec/helper_methods.rb

@@ -42,4 +44,18 @@ def get_response_for_user_agent(app, userAgent)
    body.close if body.respond_to?(:close) # avoids deadlock after 3 tests
    ActionDispatch::TestResponse.new(status, headers, body)
  end
+
+  def text_mentioning(*users)


Nit: I would rename the parameter to people, it works for both, users and people (because the user delegates to the person entity), but "people" is more generic and a user is also a person.

SuperTux88 · 2016-11-25T02:08:00Z

app/models/comment.rb

+  def subscribers
+    super.tap {|subscribers|
+      if parent.public? && parent.author.local?
+        subscribers.concat(legitimate_mention_recipients.select(&:remote?))


This is now somehow redundant. You check public? here and also check public in legitimate_mention_recipients (which calls people_allowed_to_be_mentioned). Also we never need to add subscribers for private posts (status messages and comments)

I would rename legitimate_mention_recipients to add_mention_recipients, then you can do something like this:

def add_mention_recipients(subscribers) subscribers.concat(mentions.map(&:person).select(&:remote?)) if public? end

And you can use this for comments and status messages.

Just some quick thoughts, maybe you have a better idea how to simplify this? :)

see 39cd3ca

SuperTux88 · 2016-11-25T02:11:49Z

spec/models/notifications/mentioned_spec.rb

+      expect(TestNotification).to receive(:filter_mentions).with(
+        Mention.where(mentions_container: status_message, person: [alice, bob].map(&:person)),
+        status_message,
+        :passthrough_argument


This confused me, what about [alice.id, bob.id]?

SuperTux88 · 2016-11-25T02:15:08Z

spec/models/notifications/mentioned_spec.rb


-        Notifications::Mentioned.notify(private_sm, [alice.id])
-      end
+    it "doesnt't create notification if it was filtered out by filter_mentions" do


"doesn't ..."

SuperTux88 · 2016-11-25T02:16:13Z

spec/models/notifications/mentioned_spec.rb

-      expect(bob).to receive(:mail).with(Workers::Mail::Mentioned, bob.id, sm.author.id, sm.mentions.first.id)
-
-      Notifications::Mentioned.notify(sm, [])
+    def email_the_user(*)


You can remove this.

SuperTux88 · 2016-11-25T13:27:05Z

app/models/status_message.rb

-      subscribers.concat(legitimate_mention_recipients)
-    }
+  def add_mention_subscribers?
+    super && author.local?


Do we really need the author.local? here? When sending a StatusMessage the author is always local, and the subscribers of the StatusMessage are only added to the relayable-subscribers if the author is local.

ghost · 2016-11-25T20:54:00Z

Keep up the good work @cmrd-senya!

SuperTux88

Looks really good so far, I think we can merge this soon :)

I also did some local testing now with different scenarios, and everything works as expected 👍

Can you please fix these last remarks and then squash your commits? :)

SuperTux88 · 2016-11-27T13:29:49Z

app/helpers/notifications_helper.rb

+    {
+      post_link: link_to(post_page_title(post), post_path(post)).html_safe
+    }.tap {|opts|
+      opts.merge!(comment_path: post_path(post, anchor: mentioned.guid).html_safe) if mentioned.instance_of?(Comment)


Nit: while thinking about it, I think that would be better readable:

opts[:comment_path] = post_path(post, anchor: mentioned.guid).html_safe if mentioned.instance_of?(Comment)

SuperTux88 · 2016-11-27T15:21:11Z

config/locales/diaspora/en.yml

      one: "%{actors} has mentioned you in the post %{post_link}."
      other: "%{actors} have mentioned you in the post %{post_link}."
+    mentioned_in_comment:
+      one: "%{actors} has mentioned you in the <a href='%{comment_path}'>comment</a> to the post %{post_link}."


Nit: I think "... in a comment ..." is better here.

SuperTux88 · 2016-11-27T15:38:13Z

spec/helpers/notifications_helper_spec.rb

+        link = object_link(notification, notification_people_link(notification))
+        expect(link).to include("mentioned you in the")
+        expect(link).to include("comment")
+        expect(link).to include(post_path(status_message))


This line is redundant with the line below. If you want to check if it contains a link to the post without comment-guid, you need to change this, otherwise remove it.

SuperTux88 · 2016-11-27T20:45:15Z

I have just merged #7223 and that causes some conflicts with this PR (sorry about that, but they should be easy to resolve :) ). When rebasing, can you please also remove require "spec_helper" from any new specs you added (where you don't have a conflict).

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

Introduce Workers::Mail::NotifierBase to be a base for all appropriate mail workers to reduce code duplication

cmrd-senya · 2016-11-28T14:42:01Z

Remarks corrected, squashed, rebased, signed. Tests are passing, except of random cukes.

Mentions in comments backend changes

SuperTux88 · 2016-11-29T18:35:10Z

Merged to develop branch, thanks @cmrd-senya :)

cmrd-senya · 2016-11-29T18:42:32Z

Great! Thanks for your work either. I know it required many much efforts to review it multiple times. I'll do my best to produce a better code in my next contributions :)

We also need UI to get #1851 fixed. I'm going to work on it soon. I don't really think it is a one-liner, as @AugierLe42e says, when I looked at it last time I didn't quite get it. But that's on my list anyway.

goobertron · 2016-11-29T19:02:22Z

Whoo whoo whoo! Thanks so much, @cmrd-senya, and thanks to all those who helped and reviewed.

Flaburgan · 2016-11-29T21:18:23Z

A massive thank you for this very needed work, this was definitely a missing key feature on diaspora.

ghost · 2016-11-30T07:57:44Z

Same as @Flaburgan : huge thanks to @cmrd-senya and @SuperTux88.

when I looked at it last time I didn't quite get it. But that's on my list anyway.

As explained, as soon as there's a route that answers mentionnable people for a particular post, it's easy. Maybe not a one-liner, but 3 or 4 lines at most. As I refactored the mention system, I can help you or even write it myself. Just ping me on IRC if you need it.

SuperTux88 reviewed Apr 29, 2016
View reviewed changes

cmrd-senya mentioned this pull request May 15, 2016

Refactor notification delivery code #6834

Closed

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch 3 times, most recently from a5d6316 to de16d09 Compare July 1, 2016 20:08

cmrd-senya mentioned this pull request Jul 1, 2016

Post visible for mentioned people - Post was NOT shared with them! #4160

Closed

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from de16d09 to 4025abc Compare July 8, 2016 17:05

cmrd-senya changed the title ~~[WIP] Mentions in comments backend changes~~ Mentions in comments backend changes Jul 8, 2016

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from 4025abc to a2e010b Compare July 8, 2016 17:52

jhass reviewed Jul 11, 2016
View reviewed changes

cmrd-senya changed the title ~~Mentions in comments backend changes~~ [WIP] Mentions in comments backend changes Jul 11, 2016

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from a2e010b to e02a8e9 Compare July 11, 2016 15:16

cmrd-senya added a commit to cmrd-senya/diaspora that referenced this pull request Jul 11, 2016

Remove the photo comment spec

e02a8e9

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

SuperTux88 reviewed Jul 11, 2016
View reviewed changes

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from e02a8e9 to 8ed5bd4 Compare July 11, 2016 20:06

cmrd-senya added a commit to cmrd-senya/diaspora that referenced this pull request Jul 11, 2016

Remove the photo comment spec

5cd385c

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

denschub added this to the 0.7.0.0 milestone Jul 11, 2016

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from 8ed5bd4 to 048d6b0 Compare July 26, 2016 18:04

cmrd-senya added a commit to cmrd-senya/diaspora that referenced this pull request Jul 26, 2016

Remove the photo comment spec

0ab0a87

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

SuperTux88 reviewed Jul 27, 2016
View reviewed changes

SuperTux88 requested changes Nov 25, 2016

View reviewed changes

SuperTux88 reviewed Nov 25, 2016

View reviewed changes

SuperTux88 requested changes Nov 27, 2016

View reviewed changes

cmrd-senya added 2 commits November 28, 2016 15:35

Remove the photo comment spec

0ee34f8

This functionality is not something we support and it doesn't look it will be reintroduced soon. see discussion diaspora#6818 (comment)

Refactor mail workers to use common base

ef5751b

Introduce Workers::Mail::NotifierBase to be a base for all appropriate mail workers to reduce code duplication

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from 79d4924 to 1ddcf37 Compare November 28, 2016 13:36

Mentions in comments backend changes

33ad411

cmrd-senya force-pushed the 1851-mentions-in-comments.new branch from 1ddcf37 to 33ad411 Compare November 28, 2016 13:57

SuperTux88 approved these changes Nov 29, 2016

View reviewed changes

SuperTux88 merged commit 33ad411 into diaspora:develop Nov 29, 2016

SuperTux88 added a commit that referenced this pull request Nov 29, 2016

Merge pull request #6818 from cmrd-senya/1851-mentions-in-comments.new

3b0c102

Mentions in comments backend changes

cmrd-senya deleted the 1851-mentions-in-comments.new branch November 29, 2016 18:34

Flaburgan mentioned this pull request Nov 30, 2016

User should be able to mention other users in comments #1851

Closed

cmrd-senya mentioned this pull request Dec 7, 2016

new PostController#mentionable_in_comment action #7242

Merged

SuperTux88 mentioned this pull request Jan 31, 2017

Improve mention syntax #7305

Merged

Mentions in comments backend changes #6818

Mentions in comments backend changes #6818

Conversation

cmrd-senya commented Apr 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmrd-senya commented Jul 8, 2016 • edited

Choose a reason for hiding this comment

jhass commented Jul 11, 2016

cmrd-senya commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmrd-senya commented Jul 26, 2016

Choose a reason for hiding this comment

Flaburgan commented Nov 23, 2016 • edited

ghost commented Nov 23, 2016

cmrd-senya commented Nov 23, 2016

ghost commented Nov 24, 2016

cmrd-senya commented Nov 24, 2016

SuperTux88 commented Nov 24, 2016

ghost commented Nov 24, 2016

SuperTux88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 25, 2016

SuperTux88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperTux88 commented Nov 27, 2016

cmrd-senya commented Nov 28, 2016

SuperTux88 commented Nov 29, 2016

cmrd-senya commented Nov 29, 2016

goobertron commented Nov 29, 2016

Flaburgan commented Nov 29, 2016

ghost commented Nov 30, 2016

cmrd-senya commented Apr 29, 2016 •

edited

cmrd-senya commented Jul 8, 2016 •

edited

Flaburgan commented Nov 23, 2016 •

edited