Mentions in comments backend changes

app/controllers/notifications_controller.rb

@@ -23,8 +23,8 @@ class NotificationsController < ApplicationController
 
   def index
     conditions = {:recipient_id => current_user.id}
-    if params[:type] && Notification.types.has_key?(params[:type])
-      conditions[:type] = Notification.types[params[:type]]
+    if params[:type] && types.has_key?(params[:type])
+      conditions[:type] = types[params[:type]]
     end
     if params[:show] == "unread" then conditions[:unread] = true end
     page = params[:page] || 1
@@ -44,7 +44,7 @@ class NotificationsController < ApplicationController
 
     @grouped_unread_notification_counts = {}
 
-    Notification.types.each_with_object(current_user.unread_notifications.group_by(&:type)) {|(name, type), notifications|
+    types.each_with_object(current_user.unread_notifications.group_by(&:type)) {|(name, type), notifications|
       @grouped_unread_notification_counts[name] = notifications.has_key?(type) ? notifications[type].count : 0
     }
 
@@ -65,7 +65,7 @@ class NotificationsController < ApplicationController
   end
 
   def read_all
-    current_type = Notification.types[params[:type]]
+    current_type = types[params[:type]]
     notifications = Notification.where(recipient_id: current_user.id, unread: true)
     notifications = notifications.where(type: current_type) if params[:type]
     notifications.update_all(unread: false)
@@ -93,4 +93,17 @@ class NotificationsController < ApplicationController
       }
     }.as_json
   end
+
+  def types
+    {
+      "also_commented"       => "Notifications::AlsoCommented",
+      "comment_on_post"      => "Notifications::CommentOnPost",
+      "liked"                => "Notifications::Liked",
+      "mentioned"            => "Notifications::MentionedInPost",
+      "mentioned_in_comment" => "Notifications::MentionedInComment",
+      "reshared"             => "Notifications::Reshared",
+      "started_sharing"      => "Notifications::StartedSharing"
+    }
+  end
+  helper_method :types
 end

app/controllers/status_messages_controller.rb

@@ -80,7 +80,10 @@ class StatusMessagesController < ApplicationController
 
   def handle_mention_feedback(status_message)
     return unless comes_from_others_profile_page?
-    flash[:notice] = t("status_messages.create.success", names: status_message.mentioned_people_names)
+    flash[:notice] = t(
+      "status_messages.create.success",
+      names: PersonPresenter.people_names(status_message.mentioned_people)
+    )
   end
 
   def comes_from_others_profile_page?

app/controllers/users_controller.rb

@@ -150,16 +150,7 @@ class UsersController < ApplicationController
       :auto_follow_back_aspect_id,
       :getting_started,
       :post_default_public,
-      email_preferences: %i(
-        someone_reported
-        also_commented
-        mentioned
-        comment_on_post
-        private_message
-        started_sharing
-        liked
-        reshared
-      )
+      email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym)
     )
   end
   # rubocop:enable Metrics/MethodLength

app/helpers/notifications_helper.rb

@@ -2,42 +2,46 @@ module NotificationsHelper
   include PeopleHelper
   include PostsHelper
 
-  def object_link(note, actors)
+  def object_link(note, actors_html)
     target_type = note.popup_translation_key
-    actors_count = note.actors.size
+    opts = {actors: actors_html, count: note.actors.size}
 
-    if note.instance_of?(Notifications::Mentioned)
-      if post = note.linked_object
-        translation(target_type,
-                    actors:    actors,
-                    count:     actors_count,
-                    post_link: link_to(post_page_title(post), post_path(post)).html_safe)
-      else
-        t(note.deleted_translation_key, :actors => actors, :count => actors_count).html_safe
+    if note.respond_to?(:linked_object)
+      if note.linked_object.nil? && note.respond_to?(:deleted_translation_key)
+        target_type = note.deleted_translation_key
+      elsif note.is_a?(Notifications::Mentioned)
+        opts.merge!(opts_for_mentioned(note.linked_object))
+      elsif %w(Notifications::CommentOnPost Notifications::AlsoCommented Notifications::Reshared Notifications::Liked)
+            .include?(note.type)
+        opts.merge!(opts_for_post(note.linked_object))
       end
-    elsif note.instance_of?(Notifications::CommentOnPost) || note.instance_of?(Notifications::AlsoCommented) || note.instance_of?(Notifications::Reshared) || note.instance_of?(Notifications::Liked)
-      if post = note.linked_object
-        translation(target_type,
-                    actors:      actors,
-                    count:       actors_count,
-                    post_author: h(post.author_name),
-                    post_link:   link_to(post_page_title(post),
-                                         post_path(post),
-                                         data:  {ref: post.id},
-                                         class: "hard_object_link").html_safe)
-      else
-        t(note.deleted_translation_key, :actors => actors, :count => actors_count).html_safe
-      end
-    else #Notifications:StartedSharing, etc.
-      translation(target_type, :actors => actors, :count => actors_count)
     end
+    translation(target_type, opts)
   end
 
   def translation(target_type, opts = {})
-    {:post_author => nil}.merge!(opts)
     t("#{target_type}", opts).html_safe
   end
 
+  def opts_for_post(post)
+    {
+      post_author: html_escape(post.author_name),
+      post_link:   link_to(post_page_title(post),
+                           post_path(post),
+                           data:  {ref: post.id},
+                           class: "hard_object_link").html_safe
+    }
+  end
+
+  def opts_for_mentioned(mentioned)
+    post = mentioned.instance_of?(Comment) ? mentioned.parent : mentioned
+    {
+      post_link: link_to(post_page_title(post), post_path(post)).html_safe
+    }.tap {|opts|
+      opts[:comment_path] = post_path(post, anchor: mentioned.guid).html_safe if mentioned.instance_of?(Comment)
+    }
+  end
+
   def notification_people_link(note, people=nil)
     actors =people || note.actors
     number_of_actors = actors.size

app/mailers/notification_mailers/mentioned.rb

@@ -4,7 +4,7 @@ module NotificationMailers
     delegate :author_name, to: :post, prefix: true
 
     def set_headers(target_id)
-      @post = Mention.find_by_id(target_id).post
+      @post = Mention.find_by_id(target_id).mentions_container
 
       @headers[:subject] = I18n.t('notifier.mentioned.subject', :name => @sender.name)
       @headers[:in_reply_to] = @headers[:references] = "<#{@post.guid}@#{AppConfig.pod_uri.host}>"

app/mailers/notification_mailers/mentioned_in_comment.rb

+module NotificationMailers
+  class MentionedInComment < NotificationMailers::Base
+    attr_reader :comment
+
+    def set_headers(target_id) # rubocop:disable Style/AccessorMethodName
+      @comment = Mention.find_by_id(target_id).mentions_container
+
+      @headers[:subject] = I18n.t("notifier.mentioned.subject", name: @sender.name)
+    end
+  end
+end

app/models/comment.rb

@@ -11,6 +11,7 @@ class Comment < ActiveRecord::Base
 
   include Diaspora::Taggable
   include Diaspora::Likeable
+  include Diaspora::MentionsContainer
 
   acts_as_taggable_on :tags
   extract_tags_from :text
@@ -49,14 +50,22 @@ class Comment < ActiveRecord::Base
     participation.unparticipate! if participation.present?
   end
 
-  def message
-    @message ||= Diaspora::MessageRenderer.new text
-  end
-
   def text= text
      self[:text] = text.to_s.strip #to_s if for nil, for whatever reason
   end
 
+  def people_allowed_to_be_mentioned
+    if parent.public?
+      :all
+    else
+      [*parent.comments.pluck(:author_id), *parent.likes.pluck(:author_id), parent.author_id].uniq
+    end
+  end
+
+  def add_mention_subscribers?
+    super && parent.author.local?
+  end
+
   class Generator < Diaspora::Federated::Generator
     def self.federated_class
       Comment
@@ -68,7 +77,7 @@ class Comment < ActiveRecord::Base
     end
 
     def relayable_options
-      {:post => @target, :text => @text}
+      {post: @target, text: @text}
     end
   end
 end

app/models/mention.rb

@@ -3,11 +3,15 @@
 #   the COPYRIGHT file.
 
 class Mention < ActiveRecord::Base
-  belongs_to :post
+  belongs_to :mentions_container, polymorphic: true
   belongs_to :person
-  validates :post, presence: true
+  validates :mentions_container, presence: true
   validates :person, presence: true
 
+  scope :local, -> {
+    joins(:person).where.not(people: {owner_id: nil})
+  }
+
   after_destroy :delete_notification
 
   def delete_notification

app/models/notification.rb

@@ -51,15 +51,4 @@ class Notification < ActiveRecord::Base
   private_class_method def self.suppress_notification?(recipient, actor)
     recipient.blocks.where(person: actor).exists?
   end
-
-  def self.types
-    {
-      "also_commented" => "Notifications::AlsoCommented",
-      "comment_on_post" => "Notifications::CommentOnPost",
-      "liked" => "Notifications::Liked",
-      "mentioned" => "Notifications::Mentioned",
-      "reshared" => "Notifications::Reshared",
-      "started_sharing" => "Notifications::StartedSharing"
-    }
-  end
 end

app/models/notifications/also_commented.rb

 module Notifications
   class AlsoCommented < Notification
+    include Notifications::Commented
+
     def mail_job
       Workers::Mail::AlsoCommented
     end
@@ -8,17 +10,13 @@ module Notifications
       "notifications.also_commented"
     end
 
-    def deleted_translation_key
-      "notifications.also_commented_deleted"
-    end
-
     def self.notify(comment, _recipient_user_ids)
       actor = comment.author
       commentable = comment.commentable
       recipient_ids = commentable.participants.local.where.not(id: [commentable.author_id, actor.id]).pluck(:owner_id)
 
       User.where(id: recipient_ids).find_each do |recipient|
-        next if recipient.is_shareable_hidden?(commentable)
+        next if recipient.is_shareable_hidden?(commentable) || mention_notification_exists?(comment, recipient.person)
 
         concatenate_or_create(recipient, commentable, actor).try(:email_the_user, comment, actor)
       end

app/models/notifications/comment_on_post.rb

 module Notifications
   class CommentOnPost < Notification
+    include Notifications::Commented
+
     def mail_job
       Workers::Mail::CommentOnPost
     end
@@ -8,15 +10,12 @@ module Notifications
       "notifications.comment_on_post"
     end
 
-    def deleted_translation_key
-      "notifications.also_commented_deleted"
-    end
-
     def self.notify(comment, _recipient_user_ids)
       actor = comment.author
       commentable_author = comment.commentable.author
 
       return unless commentable_author.local? && actor != commentable_author
+      return if mention_notification_exists?(comment, commentable_author)
 
       concatenate_or_create(commentable_author.owner, comment.commentable, actor).email_the_user(comment, actor)
     end

app/models/notifications/commented.rb

+module Notifications
+  module Commented
+    extend ActiveSupport::Concern
+
+    def deleted_translation_key
+      "notifications.also_commented_deleted"
+    end
+
+    module ClassMethods
+      def mention_notification_exists?(comment, recipient_person)
+        Notifications::MentionedInComment.exists?(target: comment.mentions.where(person: recipient_person))
+      end
+    end
+  end
+end

app/models/notifications/mentioned.rb

 module Notifications
-  class Mentioned < Notification
-    def mail_job
-      Workers::Mail::Mentioned
-    end
-
-    def popup_translation_key
-      "notifications.mentioned"
-    end
-
-    def deleted_translation_key
-      "notifications.mentioned_deleted"
-    end
+  module Mentioned
+    extend ActiveSupport::Concern
 
     def linked_object
-      target.post
+      target.mentions_container
     end
 
-    def self.notify(mentionable, recipient_user_ids)
-      actor = mentionable.author
-
-      mentionable.mentions.select {|mention| mention.person.local? }.each do |mention|
-        recipient = mention.person
-
-        next if recipient == actor || !(mentionable.public || recipient_user_ids.include?(recipient.owner_id))
+    module ClassMethods
+      def notify(mentionable, recipient_user_ids)
+        actor = mentionable.author
+        relevant_mentions = filter_mentions(
+          mentionable.mentions.local.where.not(person: actor),
+          mentionable,
+          recipient_user_ids
+        )
 
-        create_notification(recipient.owner, mention, actor).try(:email_the_user, mention, actor)
+        relevant_mentions.each do |mention|
+          create_notification(mention.person.owner, mention, actor).try(:email_the_user, mention, actor)
+        end
       end
     end
   end

app/models/notifications/mentioned_in_comment.rb

+module Notifications
+  class MentionedInComment < Notification
+    include Notifications::Mentioned
+
+    def popup_translation_key
+      "notifications.mentioned_in_comment"
+    end
+
+    def deleted_translation_key
+      "notifications.mentioned_in_comment_deleted"
+    end
+
+    def self.filter_mentions(mentions, mentionable, _recipient_user_ids)
+      people = mentionable.people_allowed_to_be_mentioned
+      if people == :all
+        mentions
+      else
+        mentions.where(person_id: people)
+      end
+    end
+
+    def mail_job
+      if !recipient.user_preferences.exists?(email_type: "mentioned_in_comment")
+        Workers::Mail::MentionedInComment
+      elsif shareable.author.owner_id == recipient_id
+        Workers::Mail::CommentOnPost
+      elsif shareable.participants.local.where(owner_id: recipient_id)
+        Workers::Mail::AlsoCommented
+      end
+    end
+
+    private
+
+    def shareable
+      linked_object.parent
+    end
+  end
+end

app/models/notifications/mentioned_in_post.rb

+module Notifications
+  class MentionedInPost < Notification
+    include Notifications::Mentioned
+
+    def mail_job
+      Workers::Mail::Mentioned
+    end
+
+    def popup_translation_key
+      "notifications.mentioned"
+    end
+
+    def deleted_translation_key
+      "notifications.mentioned_deleted"
+    end
+
+    def self.filter_mentions(mentions, mentionable, recipient_user_ids)
+      return mentions if mentionable.public
+      mentions.where(person: Person.where(owner_id: recipient_user_ids).ids)
+    end
+  end
+end

app/models/post.rb

@@ -12,16 +12,15 @@ class Post < ActiveRecord::Base
   include Diaspora::Likeable
   include Diaspora::Commentable
   include Diaspora::Shareable
+  include Diaspora::MentionsContainer
 
   has_many :participations, dependent: :delete_all, as: :target, inverse_of: :target
-  has_many :participants, class_name: "Person", through: :participations, source: :author
+  has_many :participants, through: :participations, source: :author
 
   attr_accessor :user_like
 
   has_many :reports, as: :item
 
-  has_many :mentions, dependent: :destroy
-
   has_many :reshares, class_name: "Reshare", foreign_key: :root_guid, primary_key: :guid
   has_many :resharers, class_name: "Person", through: :reshares, source: :author
 
@@ -60,7 +59,6 @@ class Post < ActiveRecord::Base
   end
 
   def root; end
-  def mentioned_people; []; end
   def photos; []; end
 
   #prevents error when trying to access @post.address in a post different than Reshare and StatusMessage types;

app/models/status_message.rb

@@ -23,13 +23,12 @@ class StatusMessage < Post
   attr_accessor :oembed_url
   attr_accessor :open_graph_url
 
-  after_create :create_mentions
   after_commit :queue_gather_oembed_data, :on => :create, :if => :contains_oembed_url_in_text?
   after_commit :queue_gather_open_graph_data, :on => :create, :if => :contains_open_graph_url_in_text?
 
   #scopes
   scope :where_person_is_mentioned, ->(person) {
-    joins(:mentions).where(:mentions => {:person_id => person.id})
+    owned_or_visible_by_user(person.owner).joins(:mentions).where(mentions: {person_id: person.id})
   }
 
   def self.guids_for_author(person)
@@ -52,36 +51,6 @@ class StatusMessage < Post
     text.try(:match, /#nsfw/i) || super
   end
 
-  def message
-    @message ||= Diaspora::MessageRenderer.new(text, mentioned_people: mentioned_people)
-  end
-
-  def mentioned_people
-    if self.persisted?
-      self.mentions.includes(:person => :profile).map{ |mention| mention.person }
-    else
-      Diaspora::Mentionable.people_from_string(text)
-    end
-  end
-
-  ## TODO ----
-  # don't put presentation logic in the model!
-  def mentioned_people_names
-    self.mentioned_people.map(&:name).join(', ')
-  end
-  ## ---- ----
-
-  def create_mentions
-    ppl = Diaspora::Mentionable.people_from_string(text)
-    ppl.each do |person|
-      self.mentions.find_or_create_by(person_id: person.id)
-    end
-  end
-
-  def mentions?(person)
-    mentioned_people.include? person
-  end
-
   def comment_email_subject
     message.title
   end
@@ -126,6 +95,22 @@ class StatusMessage < Post
     photos.each {|photo| photo.receive(recipient_user_ids) }
   end
 
+  # Only includes those people, to whom we're going to send a federation entity
+  # (and doesn't define exhaustive list of people who can receive it)
+  def people_allowed_to_be_mentioned
+    @aspects_ppl ||=
+      if public?
+        :all
+      else
+        Contact.joins(:aspect_memberships).where(aspect_memberships: {aspect: aspects}).distinct.pluck(:person_id)
+      end
+  end
+
+  def filter_mentions
+    return if people_allowed_to_be_mentioned == :all
+    update(text: Diaspora::Mentionable.filter_people(text, people_allowed_to_be_mentioned))
+  end
+
   private
 
   def presence_of_content

app/models/user.rb

@@ -333,6 +333,7 @@ class User < ActiveRecord::Base
 
   ######### Mailer #######################
   def mail(job, *args)
+    return unless job.present?
     pref = job.to_s.gsub('Workers::Mail::', '').underscore
     if(self.disable_mail == false && !self.user_preferences.exists?(:email_type => pref))
       job.perform_async(*args)

app/models/user_preference.rb

@@ -5,13 +5,14 @@ class UserPreference < ActiveRecord::Base
 
   VALID_EMAIL_TYPES =
     ["someone_reported",
-   "mentioned",
-   "comment_on_post",
-   "private_message",
-   "started_sharing",
-   "also_commented",
-   "liked",
-   "reshared"]
+     "mentioned",
+     "mentioned_in_comment",
+     "comment_on_post",
+     "private_message",
+     "started_sharing",
+     "also_commented",
+     "liked",
+     "reshared"]
 
   def must_be_valid_email_type
     unless VALID_EMAIL_TYPES.include?(self.email_type)

app/presenters/person_presenter.rb

@@ -40,6 +40,10 @@ class PersonPresenter < BasePresenter
     }
   end
 
+  def self.people_names(people)
+    people.map(&:name).join(", ")
+  end
+
   protected
 
   def own_profile?

app/services/notification_service.rb

 class NotificationService
   NOTIFICATION_TYPES = {
-    Comment       => [Notifications::CommentOnPost, Notifications::AlsoCommented],
+    Comment       => [Notifications::MentionedInComment, Notifications::CommentOnPost, Notifications::AlsoCommented],
     Like          => [Notifications::Liked],
-    StatusMessage => [Notifications::Mentioned],
+    StatusMessage => [Notifications::MentionedInPost],
     Conversation  => [Notifications::PrivateMessage],
     Message       => [Notifications::PrivateMessage],
     Reshare       => [Notifications::Reshared],

app/services/post_service.rb

@@ -59,8 +59,20 @@ class PostService
   end
 
   def mark_mention_notifications_read(post_id)
-    mention_id = Mention.where(post_id: post_id, person_id: user.person_id).pluck(:id)
-    Notification.where(recipient_id: user.id, target_type: "Mention", target_id: mention_id, unread: true)
-      .update_all(unread: false) if mention_id
+    mention_ids = Mention.where(
+      mentions_container_id:   post_id,
+      mentions_container_type: "Post",
+      person_id:               user.person_id
+    ).ids
+    mention_ids.concat(mentions_in_comments_for_post(post_id).pluck(:id))
+
+    Notification.where(recipient_id: user.id, target_type: "Mention", target_id: mention_ids, unread: true)
+                .update_all(unread: false) if mention_ids.any?
+  end
+
+  def mentions_in_comments_for_post(post_id)
+    Mention
+      .joins("INNER JOIN comments ON mentions_container_id = comments.id AND mentions_container_type = 'Comment'")
+      .where(comments: {commentable_id: post_id, commentable_type: "Post"})
   end
 end

app/services/status_message_creation_service.rb

@@ -19,20 +19,9 @@ class StatusMessageCreationService
 
   def build_status_message(params)
     public = params[:public] || false
-    filter_mentions params
     user.build_post(:status_message, params[:status_message].merge(public: public))
   end
 
-  def filter_mentions(params)
-    unless params[:public]
-      params[:status_message][:text] = Diaspora::Mentionable.filter_for_aspects(
-        params[:status_message][:text],
-        user,
-        *params[:aspect_ids]
-      )
-    end
-  end
-
   def add_attachments(status_message, params)
     add_location(status_message, params[:location_address], params[:location_coords])
     add_poll(status_message, params)
@@ -75,6 +64,7 @@ class StatusMessageCreationService
 
   def dispatch(status_message, services)
     receiving_services = services ? Service.titles(services) : []
+    status_message.filter_mentions # this is only required until changes from #6818 are deployed on every pod
     user.dispatch_post(status_message,
                        url:           short_post_url(status_message.guid, host: AppConfig.environment.url),
                        service_types: receiving_services)

app/views/notifications/_notification.haml

-.media.stream-element{data: {guid: note.id, type: (Notification.types.key(note.type) || "")},
-                      class: (note.unread ? "unread" : "read")}
+.media.stream-element{data: {guid: note.id, type: (types.key(note.type) || "")},
+  class: (note.unread ? "unread" : "read")}
   .unread-toggle.pull-right
     %i.entypo-eye{title: (note.unread ? t("notifications.index.mark_read") : t("notifications.index.mark_unread"))}
   - if note.type == "Notifications::StartedSharing" && (!defined?(no_aspect_dropdown) || !no_aspect_dropdown)

app/views/notifications/_notification.mobile.haml

-.notification_element{:data=>{:guid => note.id, :type => (Notification.types.key(note.type) || '')}, :class => (note.unread ? "unread" : "read")}
+.notification_element{data: {guid: note.id, type: (types.key(note.type) || "")},
+  class: (note.unread ? "unread" : "read")}
   .pull-right.unread-toggle
     %i.entypo-eye{title: (note.unread ? t("notifications.index.mark_read") : t("notifications.index.mark_unread"))}
   = person_image_tag note.actors.first, :thumb_small

app/views/notifications/index.html.haml

@@ -23,7 +23,7 @@
                   %i.entypo-comment
                 - when "liked"
                   %i.entypo-heart
-                - when "mentioned"
+                - when "mentioned", "mentioned_in_comment"
                   %span.mentionIcon
                     @
                 - when "reshared"

app/views/notifier/mentioned.markerb

@@ -4,6 +4,6 @@
 <%= t('notifier.mentioned.limited_post') %>
 <% end %>
 
-[<%= t('notifier.comment_on_post.reply', :name => @notification.post_author_name) %>][1]
+[<%= t("notifier.comment_on_post.reply", name: @notification.post_author_name) %>][1]
 
 [1]: <%= post_url(@notification.post) %>

app/views/notifier/mentioned_in_comment.markerb

+<% if @notification.comment.public? %>
+<%= @notification.comment.message.plain_text_without_markdown %>
+<% else %>
+<%= t("notifier.mentioned_in_comment.limited_post") %>
+<% end %>
+
+[<%= t("notifier.mentioned_in_comment.reply") %>][1]
+
+[1]: <%= post_url(@notification.comment.parent, anchor: @notification.comment.guid) %>

app/views/users/_edit.haml

@@ -142,6 +142,11 @@
                   = t(".mentioned")
                 .small-horizontal-spacer
 
+                = type.label :mentioned_in_comment, class: "checkbox-inline" do
+                  = type.check_box :mentioned_in_comment, {checked: @email_prefs["mentioned_in_comment"]}, false, true
+                  = t(".mentioned_in_comment")
+                .small-horizontal-spacer
+
                 = type.label :liked, class: "checkbox-inline" do
                   = type.check_box :liked, {checked: @email_prefs["liked"]}, false, true
                   = t(".liked")