From 33ad411bbd0d48652d161a60627657f75d467292 Mon Sep 17 00:00:00 2001 From: cmrd Senya Date: Mon, 22 Aug 2016 15:12:35 +0300 Subject: [PATCH] Mentions in comments backend changes --- app/controllers/notifications_controller.rb | 21 +- app/controllers/status_messages_controller.rb | 5 +- app/controllers/users_controller.rb | 11 +- app/helpers/notifications_helper.rb | 56 +-- app/mailers/notification_mailers/mentioned.rb | 2 +- .../mentioned_in_comment.rb | 11 + app/models/comment.rb | 19 +- app/models/mention.rb | 8 +- app/models/notification.rb | 11 - app/models/notifications/also_commented.rb | 8 +- app/models/notifications/comment_on_post.rb | 7 +- app/models/notifications/commented.rb | 15 + app/models/notifications/mentioned.rb | 35 +- .../notifications/mentioned_in_comment.rb | 38 ++ app/models/notifications/mentioned_in_post.rb | 22 ++ app/models/post.rb | 6 +- app/models/status_message.rb | 49 +-- app/models/user.rb | 1 + app/models/user_preference.rb | 15 +- app/presenters/person_presenter.rb | 4 + app/services/notification_service.rb | 4 +- app/services/post_service.rb | 18 +- .../status_message_creation_service.rb | 12 +- app/views/notifications/_notification.haml | 4 +- .../notifications/_notification.mobile.haml | 3 +- app/views/notifications/index.html.haml | 2 +- app/views/notifier/mentioned.markerb | 2 +- .../notifier/mentioned_in_comment.markerb | 9 + app/views/users/_edit.haml | 5 + app/workers/mail/mentioned_in_comment.rb | 6 + config/locales/diaspora/en.yml | 15 +- .../20161107100840_polymorphic_mentions.rb | 38 ++ db/schema.rb | 11 +- features/desktop/change_settings.feature | 4 + features/desktop/notifications.feature | 20 + features/step_definitions/comment_steps.rb | 6 + features/step_definitions/custom_web_steps.rb | 2 +- .../step_definitions/notifications_steps.rb | 2 +- lib/diaspora/mentionable.rb | 34 +- lib/diaspora/mentions_container.rb | 38 ++ lib/diaspora/message_renderer.rb | 2 +- .../notifications_controller_spec.rb | 13 +- spec/controllers/users_controller_spec.rb | 30 +- spec/factories.rb | 38 +- spec/helper_methods.rb | 20 +- spec/helpers/notifications_helper_spec.rb | 31 ++ spec/integration/mentioning_spec.rb | 366 ++++++++++++++++-- spec/lib/diaspora/federation/receive_spec.rb | 20 +- spec/lib/diaspora/mentionable_spec.rb | 21 +- spec/mailers/notifier_spec.rb | 36 +- spec/models/comment_spec.rb | 51 +++ spec/models/mention_spec.rb | 4 +- .../notifications/mentioned_in_post_spec.rb | 72 ++++ spec/models/notifications/mentioned_spec.rb | 99 ++--- spec/models/status_message_spec.rb | 72 +--- spec/presenters/post_presenter_spec.rb | 4 +- spec/services/notification_service_spec.rb | 74 ++++ spec/services/post_service_spec.rb | 14 +- .../status_message_creation_service_spec.rb | 48 +-- spec/shared_behaviors/mentions_container.rb | 44 +++ spec/workers/mail/mentioned_spec.rb | 5 +- 61 files changed, 1192 insertions(+), 451 deletions(-) create mode 100644 app/mailers/notification_mailers/mentioned_in_comment.rb create mode 100644 app/models/notifications/commented.rb create mode 100644 app/models/notifications/mentioned_in_comment.rb create mode 100644 app/models/notifications/mentioned_in_post.rb create mode 100644 app/views/notifier/mentioned_in_comment.markerb create mode 100644 app/workers/mail/mentioned_in_comment.rb create mode 100644 db/migrate/20161107100840_polymorphic_mentions.rb create mode 100644 lib/diaspora/mentions_container.rb create mode 100644 spec/models/notifications/mentioned_in_post_spec.rb create mode 100644 spec/services/notification_service_spec.rb create mode 100644 spec/shared_behaviors/mentions_container.rb diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index aae218b69..0d1714ea0 100644 --- a/app/controllers/notifications_controller.rb +++ b/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 diff --git a/app/controllers/status_messages_controller.rb b/app/controllers/status_messages_controller.rb index 87db1237f..93e3e6958 100644 --- a/app/controllers/status_messages_controller.rb +++ b/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? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 958353e28..775f6f27e 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index a5bca4a24..a8c1a533f 100644 --- a/app/helpers/notifications_helper.rb +++ b/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 diff --git a/app/mailers/notification_mailers/mentioned.rb b/app/mailers/notification_mailers/mentioned.rb index a06c82e75..e72c2074d 100644 --- a/app/mailers/notification_mailers/mentioned.rb +++ b/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}>" diff --git a/app/mailers/notification_mailers/mentioned_in_comment.rb b/app/mailers/notification_mailers/mentioned_in_comment.rb new file mode 100644 index 000000000..abb346ea8 --- /dev/null +++ b/app/mailers/notification_mailers/mentioned_in_comment.rb @@ -0,0 +1,11 @@ +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 diff --git a/app/models/comment.rb b/app/models/comment.rb index 8d98e52ec..483215f03 100644 --- a/app/models/comment.rb +++ b/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 diff --git a/app/models/mention.rb b/app/models/mention.rb index 9ddfd8dad..597cebe94 100644 --- a/app/models/mention.rb +++ b/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 diff --git a/app/models/notification.rb b/app/models/notification.rb index aaa56d53d..c4b557045 100644 --- a/app/models/notification.rb +++ b/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 diff --git a/app/models/notifications/also_commented.rb b/app/models/notifications/also_commented.rb index a345566f3..cc28c047c 100644 --- a/app/models/notifications/also_commented.rb +++ b/app/models/notifications/also_commented.rb @@ -1,5 +1,7 @@ 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 diff --git a/app/models/notifications/comment_on_post.rb b/app/models/notifications/comment_on_post.rb index df23b558e..ee3320e39 100644 --- a/app/models/notifications/comment_on_post.rb +++ b/app/models/notifications/comment_on_post.rb @@ -1,5 +1,7 @@ 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 diff --git a/app/models/notifications/commented.rb b/app/models/notifications/commented.rb new file mode 100644 index 000000000..e5b1b7d48 --- /dev/null +++ b/app/models/notifications/commented.rb @@ -0,0 +1,15 @@ +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 diff --git a/app/models/notifications/mentioned.rb b/app/models/notifications/mentioned.rb index bede65ee4..b128791bb 100644 --- a/app/models/notifications/mentioned.rb +++ b/app/models/notifications/mentioned.rb @@ -1,30 +1,23 @@ 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 + 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 + ) - 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)) - - 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 diff --git a/app/models/notifications/mentioned_in_comment.rb b/app/models/notifications/mentioned_in_comment.rb new file mode 100644 index 000000000..77038864a --- /dev/null +++ b/app/models/notifications/mentioned_in_comment.rb @@ -0,0 +1,38 @@ +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 diff --git a/app/models/notifications/mentioned_in_post.rb b/app/models/notifications/mentioned_in_post.rb new file mode 100644 index 000000000..35ec36c45 --- /dev/null +++ b/app/models/notifications/mentioned_in_post.rb @@ -0,0 +1,22 @@ +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 diff --git a/app/models/post.rb b/app/models/post.rb index 3ad235f0f..576717f79 100644 --- a/app/models/post.rb +++ b/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; diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 6ee7d388e..28d838536 100644 --- a/app/models/status_message.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index e8a09a322..85ea59cb6 100644 --- a/app/models/user.rb +++ b/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) diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb index 354a48400..ea4f92425 100644 --- a/app/models/user_preference.rb +++ b/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) diff --git a/app/presenters/person_presenter.rb b/app/presenters/person_presenter.rb index 63685cb51..8149acbc1 100644 --- a/app/presenters/person_presenter.rb +++ b/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? diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d356a0211..b481f1b4c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -1,8 +1,8 @@ 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], diff --git a/app/services/post_service.rb b/app/services/post_service.rb index 140c33a0d..99ce0d28e 100644 --- a/app/services/post_service.rb +++ b/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 diff --git a/app/services/status_message_creation_service.rb b/app/services/status_message_creation_service.rb index b541d194d..94a16d795 100644 --- a/app/services/status_message_creation_service.rb +++ b/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) diff --git a/app/views/notifications/_notification.haml b/app/views/notifications/_notification.haml index 8c537bdc0..23ff76fd3 100644 --- a/app/views/notifications/_notification.haml +++ b/app/views/notifications/_notification.haml @@ -1,5 +1,5 @@ -.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) diff --git a/app/views/notifications/_notification.mobile.haml b/app/views/notifications/_notification.mobile.haml index 2b8825505..b38977152 100644 --- a/app/views/notifications/_notification.mobile.haml +++ b/app/views/notifications/_notification.mobile.haml @@ -1,4 +1,5 @@ -.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 diff --git a/app/views/notifications/index.html.haml b/app/views/notifications/index.html.haml index 22e647c05..61a70fe14 100644 --- a/app/views/notifications/index.html.haml +++ b/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" diff --git a/app/views/notifier/mentioned.markerb b/app/views/notifier/mentioned.markerb index a2469a0e5..414739665 100644 --- a/app/views/notifier/mentioned.markerb +++ b/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) %> diff --git a/app/views/notifier/mentioned_in_comment.markerb b/app/views/notifier/mentioned_in_comment.markerb new file mode 100644 index 000000000..3b86f409b --- /dev/null +++ b/app/views/notifier/mentioned_in_comment.markerb @@ -0,0 +1,9 @@ +<% 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) %> diff --git a/app/views/users/_edit.haml b/app/views/users/_edit.haml index 9a171a923..060778b95 100644 --- a/app/views/users/_edit.haml +++ b/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") diff --git a/app/workers/mail/mentioned_in_comment.rb b/app/workers/mail/mentioned_in_comment.rb new file mode 100644 index 000000000..5e8d7f91f --- /dev/null +++ b/app/workers/mail/mentioned_in_comment.rb @@ -0,0 +1,6 @@ +module Workers + module Mail + class MentionedInComment < NotifierBase + end + end +end diff --git a/config/locales/diaspora/en.yml b/config/locales/diaspora/en.yml index 62c2ccfff..65030b224 100644 --- a/config/locales/diaspora/en.yml +++ b/config/locales/diaspora/en.yml @@ -615,9 +615,11 @@ en: one: "%{actors} also commented on %{post_author}’s post %{post_link}." other: "%{actors} also commented on %{post_author}’s post %{post_link}." mentioned: - zero: "%{actors} have mentioned you in the post %{post_link}." 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 a comment to the post %{post_link}." + other: "%{actors} have mentioned you in a comment to the post %{post_link}." liked: zero: "%{actors} have liked your post %{post_link}." one: "%{actors} has liked your post %{post_link}." @@ -640,9 +642,11 @@ en: one: "%{actors} reshared your deleted post." other: "%{actors} reshared your deleted post." mentioned_deleted: - zero: "%{actors} mentioned you in a deleted post." one: "%{actors} mentioned you in a deleted post." other: "%{actors} mentioned you in a deleted post." + mentioned_in_comment_deleted: + one: "%{actors} mentioned you in a deleted comment." + other: "%{actors} mentioned you in a deleted comment." index: notifications: "Notifications" mark_all_as_read: "Mark all as read" @@ -655,7 +659,8 @@ en: also_commented: "Also commented" comment_on_post: "Comment on post" liked: "Liked" - mentioned: "Mentioned" + mentioned: "Mentioned in post" + mentioned_in_comment: "Mentioned in comment" reshared: "Reshared" started_sharing: "Started sharing" no_notifications: "You don't have any notifications yet." @@ -689,6 +694,9 @@ en: mentioned: subject: "%{name} has mentioned you on diaspora*" limited_post: "You were mentioned in a limited post." + mentioned_in_comment: + limited_post: "You were mentioned in a comment to a limited post." + reply: "Reply to or view this conversation >" private_message: subject: "There’s a new private message for you" reply_to_or_view: "Reply to or view this conversation >" @@ -1167,6 +1175,7 @@ en: started_sharing: "someone starts sharing with you" someone_reported: "someone sends a report" mentioned: "you are mentioned in a post" + mentioned_in_comment: "you are mentioned in a comment" liked: "someone likes your post" reshared: "someone reshares your post" comment_on_post: "someone comments on your post" diff --git a/db/migrate/20161107100840_polymorphic_mentions.rb b/db/migrate/20161107100840_polymorphic_mentions.rb new file mode 100644 index 000000000..07b096394 --- /dev/null +++ b/db/migrate/20161107100840_polymorphic_mentions.rb @@ -0,0 +1,38 @@ +class PolymorphicMentions < ActiveRecord::Migration + def change + remove_index :mentions, column: %i(post_id) + remove_index :mentions, column: %i(person_id post_id), unique: true + rename_column :mentions, :post_id, :mentions_container_id + add_column :mentions, :mentions_container_type, :string, null: false + add_index :mentions, + %i(mentions_container_id mentions_container_type), + name: "index_mentions_on_mc_id_and_mc_type", + length: {mentions_container_type: 191} + add_index :mentions, + %i(person_id mentions_container_id mentions_container_type), + name: "index_mentions_on_person_and_mc_id_and_mc_type", + length: {mentions_container_type: 191}, + unique: true + + reversible(&method(:up_down)) + end + + class Mention < ActiveRecord::Base + end + + class Notification < ActiveRecord::Base + end + + def up_down(change) + change.up do + Mention.update_all(mentions_container_type: "Post") + Notification.where(type: "Notifications::Mentioned").update_all(type: "Notifications::MentionedInPost") + end + + change.down do + Notification.where(type: "Notifications::MentionedInPost").update_all(type: "Notifications::Mentioned") + Mention.where(mentions_container_type: "Comment").destroy_all + Notification.where(type: "Notifications::MentionedInComment").destroy_all + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 402f53db2..7e0e55ec0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161024231443) do +ActiveRecord::Schema.define(version: 20161107100840) do create_table "account_deletions", force: :cascade do |t| t.string "diaspora_handle", limit: 255 @@ -204,13 +204,14 @@ ActiveRecord::Schema.define(version: 20161024231443) do end create_table "mentions", force: :cascade do |t| - t.integer "post_id", limit: 4, null: false - t.integer "person_id", limit: 4, null: false + t.integer "mentions_container_id", limit: 4, null: false + t.integer "person_id", limit: 4, null: false + t.string "mentions_container_type", limit: 255, null: false end - add_index "mentions", ["person_id", "post_id"], name: "index_mentions_on_person_id_and_post_id", unique: true, using: :btree + add_index "mentions", ["mentions_container_id", "mentions_container_type"], name: "index_mentions_on_mc_id_and_mc_type", length: {"mentions_container_id"=>nil, "mentions_container_type"=>191}, using: :btree + add_index "mentions", ["person_id", "mentions_container_id", "mentions_container_type"], name: "index_mentions_on_person_and_mc_id_and_mc_type", unique: true, length: {"person_id"=>nil, "mentions_container_id"=>nil, "mentions_container_type"=>191}, using: :btree add_index "mentions", ["person_id"], name: "index_mentions_on_person_id", using: :btree - add_index "mentions", ["post_id"], name: "index_mentions_on_post_id", using: :btree create_table "messages", force: :cascade do |t| t.integer "conversation_id", limit: 4, null: false diff --git a/features/desktop/change_settings.feature b/features/desktop/change_settings.feature index ba0543908..21c473930 100644 --- a/features/desktop/change_settings.feature +++ b/features/desktop/change_settings.feature @@ -22,6 +22,10 @@ Feature: Change settings And I press "change_email_preferences" Then I should see "Email notifications changed" And the "user_email_preferences_mentioned" checkbox should not be checked + When I uncheck "user_email_preferences_mentioned_in_comment" + And I press "change_email_preferences" + Then I should see "Email notifications changed" + And the "user_email_preferences_mentioned_in_comment" checkbox should not be checked Scenario: Change my preferred language When I select "polski" from "user_language" diff --git a/features/desktop/notifications.feature b/features/desktop/notifications.feature index 2dd5eaee4..13fc31a6b 100644 --- a/features/desktop/notifications.feature +++ b/features/desktop/notifications.feature @@ -101,6 +101,26 @@ Feature: Notifications Then I should see "mentioned you in the post" And I should have 1 email delivery + Scenario: someone mentioned me in a comment + Given "alice@alice.alice" has a public post with text "check this out!" + And "bob@bob.bob" has commented mentioning "alice@alice.alice" on "check this out!" + When I sign in as "alice@alice.alice" + And I follow "Notifications" in the header + Then the notification dropdown should be visible + And I should see "mentioned you in a comment" + And I should have 1 email delivery + + Scenario: I mark a notification as read + Given a user with email "bob@bob.bob" is connected with "alice@alice.alice" + And Alice has a post mentioning Bob + When I sign in as "bob@bob.bob" + And I follow "Notifications" in the header + Then the notification dropdown should be visible + And I wait for notifications to load + And I should see a ".unread .unread-toggle .entypo-eye" + When I click on selector ".unread .unread-toggle .entypo-eye" + Then I should see a ".read .unread-toggle" + Scenario: filter notifications Given a user with email "bob@bob.bob" is connected with "alice@alice.alice" And Alice has a post mentioning Bob diff --git a/features/step_definitions/comment_steps.rb b/features/step_definitions/comment_steps.rb index 0d669f1ac..74589dddb 100644 --- a/features/step_definitions/comment_steps.rb +++ b/features/step_definitions/comment_steps.rb @@ -21,6 +21,12 @@ Given /^"([^"]*)" has commented "([^"]*)" on "([^"]*)"$/ do |email, comment_text user.comment!(post, comment_text) end +Given /^"([^"]*)" has commented mentioning "([^"]*)" on "([^"]*)"$/ do |email, mentionee_email, post_text| + user = User.find_by(email: email) + post = StatusMessage.find_by(text: post_text) + user.comment!(post, text_mentioning(User.find_by(email: mentionee_email))) +end + Given /^"([^"]*)" has commented a lot on "([^"]*)"$/ do |email, post_text| user = User.find_by(email: email) post = StatusMessage.find_by(text: post_text) diff --git a/features/step_definitions/custom_web_steps.rb b/features/step_definitions/custom_web_steps.rb index a84f2d337..be4e29555 100644 --- a/features/step_definitions/custom_web_steps.rb +++ b/features/step_definitions/custom_web_steps.rb @@ -176,7 +176,7 @@ end Then /^(?:|I )should see a "([^\"]*)"(?: within "([^\"]*)")?$/ do |selector, scope_selector| with_scope(scope_selector) do - current_scope.should have_css selector + expect(current_scope).to have_css(selector) end end diff --git a/features/step_definitions/notifications_steps.rb b/features/step_definitions/notifications_steps.rb index 87c27ceff..65aa57ad0 100644 --- a/features/step_definitions/notifications_steps.rb +++ b/features/step_definitions/notifications_steps.rb @@ -3,7 +3,7 @@ When "I filter notifications by likes" do end When "I filter notifications by mentions" do - step %(I follow "Mentioned" within "#notifications_container .list-group") + step %(I follow "Mentioned in post" within "#notifications_container .list-group") end Then /^I should( not)? have activated notifications for the post( in the single post view)?$/ do |negate, spv| diff --git a/lib/diaspora/mentionable.rb b/lib/diaspora/mentionable.rb index 8645d9020..c922096af 100644 --- a/lib/diaspora/mentionable.rb +++ b/lib/diaspora/mentionable.rb @@ -54,25 +54,19 @@ module Diaspora::Mentionable end # takes a message text and converts mentions for people that are not in the - # given aspects to simple markdown links, leaving only mentions for people who + # given array to simple markdown links, leaving only mentions for people who # will actually be able to receive notifications for being mentioned. # # @param [String] message text - # @param [User] aspect owner - # @param [Mixed] array containing aspect ids or "all" + # @param [Array] allowed_people ids of people that are allowed to stay # @return [String] message text with filtered mentions - def self.filter_for_aspects(msg_text, user, *aspects) - aspect_ids = MentionsInternal.get_aspect_ids(user, *aspects) - + def self.filter_people(msg_text, allowed_people) mentioned_ppl = people_from_string(msg_text) - aspects_ppl = AspectMembership.where(aspect_id: aspect_ids) - .includes(:contact => :person) - .map(&:person) msg_text.to_s.gsub(REGEX) {|match_str| name, handle = mention_attrs(match_str) person = mentioned_ppl.find {|p| p.diaspora_handle == handle } - mention = MentionsInternal.profile_link(person, name) unless aspects_ppl.include?(person) + mention = MentionsInternal.profile_link(person, name) unless allowed_people.include?(person.id) mention || match_str } @@ -118,26 +112,6 @@ module Diaspora::Mentionable "[#{display_name.presence || person.name}](#{local_or_remote_person_path(person)})" end - - # takes a user and an array of aspect ids or an array containing "all" as - # the first element. will do some checking on ids and return them in an array - # in case of "all", returns an array with all the users aspect ids - # - # @param [User] owner of the aspects - # @param [Array] aspect ids or "all" - # @return [Array] aspect ids - def self.get_aspect_ids(user, *aspects) - return [] if aspects.empty? - - if (!aspects.first.is_a?(Integer)) && aspects.first.to_s == 'all' - return user.aspects.pluck(:id) - end - - ids = aspects.reject {|id| Integer(id) == nil } # only numeric - - #make sure they really belong to the user - user.aspects.where(id: ids).pluck(:id) - end end end diff --git a/lib/diaspora/mentions_container.rb b/lib/diaspora/mentions_container.rb new file mode 100644 index 000000000..5364aa5ae --- /dev/null +++ b/lib/diaspora/mentions_container.rb @@ -0,0 +1,38 @@ +module Diaspora + module MentionsContainer + extend ActiveSupport::Concern + + included do + after_create :create_mentions + has_many :mentions, as: :mentions_container, dependent: :destroy + end + + def mentioned_people + if persisted? + mentions.includes(person: :profile).map(&:person) + else + Diaspora::Mentionable.people_from_string(text) + end + end + + def add_mention_subscribers? + public? + end + + def subscribers + super.tap {|subscribers| + subscribers.concat(mentions.map(&:person).select(&:remote?)) if add_mention_subscribers? + } + end + + def create_mentions + Diaspora::Mentionable.people_from_string(text).each do |person| + mentions.find_or_create_by(person_id: person.id) + end + end + + def message + @message ||= Diaspora::MessageRenderer.new text, mentioned_people: mentioned_people + end + end +end diff --git a/lib/diaspora/message_renderer.rb b/lib/diaspora/message_renderer.rb index 7c39470fb..de616f3d9 100644 --- a/lib/diaspora/message_renderer.rb +++ b/lib/diaspora/message_renderer.rb @@ -72,7 +72,7 @@ module Diaspora end if options[:disable_hovercards] || options[:link_all_mentions] - @message = Diaspora::Mentionable.filter_for_aspects message, nil + @message = Diaspora::Mentionable.filter_people message, [] else make_mentions_plain_text end diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index e64b45bb4..d1f969e7a 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -77,12 +77,13 @@ describe NotificationsController, :type => :controller do timeago_content = note_html.css("time")[0]["data-time-ago"] expect(response_json["unread_count"]).to be(1) expect(response_json["unread_count_by_type"]).to eq( - "also_commented" => 1, - "comment_on_post" => 0, - "liked" => 0, - "mentioned" => 0, - "reshared" => 0, - "started_sharing" => 0 + "also_commented" => 1, + "comment_on_post" => 0, + "liked" => 0, + "mentioned" => 0, + "mentioned_in_comment" => 0, + "reshared" => 0, + "started_sharing" => 0 ) expect(timeago_content).to include(@notification.updated_at.iso8601) expect(response.body).to match(/note_html/) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index a80f9f37e..0ab83e120 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -184,20 +184,24 @@ describe UsersController, :type => :controller do end end - describe 'email settings' do - it 'lets the user turn off mail' do - par = {:id => @user.id, :user => {:email_preferences => {'mentioned' => 'true'}}} - expect{ - put :update, par - }.to change(@user.user_preferences, :count).by(1) - end + describe "email settings" do + UserPreference::VALID_EMAIL_TYPES.each do |email_type| + context "for #{email_type}" do + it "lets the user turn off mail" do + par = {id: @user.id, user: {email_preferences: {email_type => "true"}}} + expect { + put :update, par + }.to change(@user.user_preferences, :count).by(1) + end - it 'lets the user get mail again' do - @user.user_preferences.create(:email_type => 'mentioned') - par = {:id => @user.id, :user => {:email_preferences => {'mentioned' => 'false'}}} - expect{ - put :update, par - }.to change(@user.user_preferences, :count).by(-1) + it "lets the user get mail again" do + @user.user_preferences.create(email_type: email_type) + par = {id: @user.id, user: {email_preferences: {email_type => "false"}}} + expect { + put :update, par + }.to change(@user.user_preferences, :count).by(-1) + end + end end end diff --git a/spec/factories.rb b/spec/factories.rb index 2f8b37d30..ec8cec960 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -76,8 +76,17 @@ FactoryGirl.define do end end - factory :user_with_aspect, :parent => :user do - after(:create) { |u| FactoryGirl.create(:aspect, :user => u) } + factory :user_with_aspect, parent: :user do + transient do + friends [] + end + + after(:create) do |user, evaluator| + FactoryGirl.create(:aspect, user: user) + evaluator.friends.each do |friend| + connect_users_with_aspects(user, friend) + end + end end factory :aspect do @@ -116,8 +125,8 @@ FactoryGirl.define do factory(:status_message_in_aspect) do public false + author { FactoryGirl.create(:user_with_aspect).person } after(:build) do |sm| - sm.author = FactoryGirl.create(:user_with_aspect).person sm.aspects << sm.author.owner.aspects.first end end @@ -228,8 +237,8 @@ FactoryGirl.define do factory(:comment) do sequence(:text) {|n| "#{n} cats"} - association(:author, :factory => :person) - association(:post, :factory => :status_message) + association(:author, factory: :person) + association(:post, factory: :status_message) end factory(:notification) do @@ -242,6 +251,16 @@ FactoryGirl.define do end end + factory(:notification_mentioned_in_comment, class: Notification) do + association :recipient, factory: :user + type "Notifications::MentionedInComment" + + after(:build) do |note| + note.actors << FactoryGirl.build(:person) + note.target = FactoryGirl.create :mention_in_comment, person: note.recipient.person + end + end + factory(:tag, :class => ActsAsTaggableOn::Tag) do name "partytimeexcellent" end @@ -271,8 +290,13 @@ FactoryGirl.define do end factory(:mention) do - association(:person, :factory => :person) - association(:post, :factory => :status_message) + association(:person, factory: :person) + association(:mentions_container, factory: :status_message) + end + + factory(:mention_in_comment, class: Mention) do + association(:person, factory: :person) + association(:mentions_container, factory: :comment) end factory(:conversation) do diff --git a/spec/helper_methods.rb b/spec/helper_methods.rb index ad4371a34..177999a96 100644 --- a/spec/helper_methods.rb +++ b/spec/helper_methods.rb @@ -3,10 +3,12 @@ require Rails.root.join("spec", "support", "inlined_jobs") module HelperMethods def connect_users_with_aspects(u1, u2) - aspect1 = u1.aspects.length == 1 ? u1.aspects.first : u1.aspects.where(:name => "Besties").first - aspect2 = u2.aspects.length == 1 ? u2.aspects.first : u2.aspects.where(:name => "Besties").first + aspect1, aspect2 = [u1, u2].map do |user| + user.aspects.where(name: "Besties").first.presence || user.aspects.first + end connect_users(u1, aspect1, u2, aspect2) end + def connect_users(user1, aspect1, user2, aspect2) user1.contacts.create!(:person => user2.person, :aspects => [aspect1], @@ -42,4 +44,18 @@ module HelperMethods body.close if body.respond_to?(:close) # avoids deadlock after 3 tests ActionDispatch::TestResponse.new(status, headers, body) end + + def text_mentioning(*people) + people.map {|person| + "this is a text mentioning @{#{person.name}; #{person.diaspora_handle}} ... have fun testing!" + }.join(" ") + end + + 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 + signable_fields = attributes.keys - [:author_signature] + + entity_class.new(attributes, [*signable_fields, *additional_xml_elements.keys], additional_xml_elements) + end end diff --git a/spec/helpers/notifications_helper_spec.rb b/spec/helpers/notifications_helper_spec.rb index 7f5f2e601..1aa84d6a9 100644 --- a/spec/helpers/notifications_helper_spec.rb +++ b/spec/helpers/notifications_helper_spec.rb @@ -90,6 +90,37 @@ describe NotificationsHelper, type: :helper do end end end + + let(:status_message) { + FactoryGirl.create(:status_message_in_aspect, author: alice.person, text: text_mentioning(bob)) + } + + describe "when mentioned in status message" do + it "should include correct wording and post link" do + Notifications::MentionedInPost.notify(status_message, [bob.id]) + notification = Notifications::MentionedInPost.last + expect(notification).not_to be_nil + + link = object_link(notification, notification_people_link(notification)) + expect(link).to include("mentioned you in the post") + expect(link).to include(post_path(status_message)) + end + end + + describe "when mentioned in comment" do + it "should include correct wording, post link and comment link" do + comment = FactoryGirl.create(:comment, author: bob.person, text: text_mentioning(alice), post: status_message) + Notifications::MentionedInComment.notify(comment, [alice.id]) + notification = Notifications::MentionedInComment.last + expect(notification).not_to be_nil + + link = object_link(notification, notification_people_link(notification)) + expect(link).to include("mentioned you in a") + expect(link).to include(">comment") + expect(link).to include("href=\"#{post_path(status_message)}\"") + expect(link).to include("#{post_path(status_message)}##{comment.guid}") + end + end end describe '#display_year?' do diff --git a/spec/integration/mentioning_spec.rb b/spec/integration/mentioning_spec.rb index 8b8af8d0b..2e6b4292d 100644 --- a/spec/integration/mentioning_spec.rb +++ b/spec/integration/mentioning_spec.rb @@ -1,10 +1,39 @@ module MentioningSpecHelpers - def default_aspect - @user1.aspects.where(name: "generic").first + def notifications_about_mentioning(user, object) + table = object.class.table_name + + if object.is_a?(StatusMessage) + klass = Notifications::MentionedInPost + elsif object.is_a?(Comment) + klass = Notifications::MentionedInComment + end + + klass + .where(recipient_id: user.id) + .joins("LEFT OUTER JOIN mentions ON notifications.target_id = mentions.id AND "\ + "notifications.target_type = 'Mention'") + .joins("LEFT OUTER JOIN #{table} ON mentions_container_id = #{table}.id AND "\ + "mentions_container_type = '#{object.class.base_class}'").where(table.to_sym => {id: object.id}) end - def text_mentioning(user) - "this is a text mentioning @{#{user.name}; #{user.diaspora_handle}} ... have fun testing!" + def mention_container_path(object) + object.is_a?(Post) ? post_path(object) : post_path(object.parent, anchor: object.guid) + end + + def mentioning_mail_notification(user, object) + ActionMailer::Base.deliveries.select {|delivery| + delivery.to.include?(user.email) && + delivery.subject.include?(I18n.t("notifier.mentioned.subject", name: "")) && + delivery.body.parts[0].body.include?(mention_container_path(object)) + } + end + + def also_commented_mail_notification(user, post) + ActionMailer::Base.deliveries.select {|delivery| + delivery.to.include?(user.email) && + delivery.subject.include?(I18n.t("notifier.also_commented.limited_subject")) && + delivery.body.parts[0].body.include?(post_path(post)) + } end def stream_for(user) @@ -17,77 +46,336 @@ module MentioningSpecHelpers stream.posts end - def users_connected?(user1, user2) - user1.contacts.where(person_id: user2.person).count > 0 + def post_status_message(mentioned_user, aspects=nil) + aspects = user1.aspects.first.id.to_s if aspects.nil? + sign_in user1 + status_msg = nil + inlined_jobs do + post "/status_messages.json", status_message: {text: text_mentioning(mentioned_user)}, aspect_ids: aspects + status_msg = StatusMessage.find(JSON.parse(response.body)["id"]) + end + status_msg + end + + def receive_each(entity, recipients) + inlined_jobs do + recipients.each do |recipient| + DiasporaFederation.callbacks.trigger(:receive_entity, entity, entity.author, recipient.id) + end + end + end + + def find_private_message(guid) + StatusMessage.find_by(guid: guid).tap do |status_msg| + expect(status_msg).not_to be_nil + expect(status_msg.public?).to be false + end + end + + def receive_status_message_via_federation(text, *recipients) + entity = FactoryGirl.build( + :status_message_entity, + author: remote_raphael.diaspora_handle, + text: text, + public: false + ) + + expect { + receive_each(entity, recipients) + }.to change(Post, :count).by(1).and change(ShareVisibility, :count).by(recipients.count) + + find_private_message(entity.guid) + end + + def receive_comment_via_federation(text, parent) + entity = build_relayable_federation_entity( + :comment, + parent_guid: parent.guid, + author: remote_raphael.diaspora_handle, + parent: Diaspora::Federation::Entities.related_entity(parent), + text: text + ) + + receive_each(entity, [parent.author.owner]) + + Comment.find_by(guid: entity.guid) end end - describe "mentioning", type: :request do include MentioningSpecHelpers - before do - @user1 = FactoryGirl.create :user_with_aspect - @user2 = FactoryGirl.create :user - @user3 = FactoryGirl.create :user + RSpec::Matchers.define :be_mentioned_in do |object| + include Rails.application.routes.url_helpers - @user1.share_with(@user2.person, default_aspect) - sign_in @user1 + def user_notified?(user, object) + notifications_about_mentioning(user, object).any? && mentioning_mail_notification(user, object).any? + end + + match do |user| + object.message.markdownified.include?(person_path(id: user.person.guid)) && user_notified?(user, object) + end + + match_when_negated do |user| + !user_notified?(user, object) + end end - # see: https://github.com/diaspora/diaspora/issues/4160 - it "doesn't mention people that aren't in the target aspect" do - expect(users_connected?(@user1, @user3)).to be false + RSpec::Matchers.define :be_in_streams_of do |user| + match do |status_message| + stream_for(user).map(&:id).include?(status_message.id) && + mention_stream_for(user).map(&:id).include?(status_message.id) + end + match_when_negated do |status_message| + !stream_for(user).map(&:id).include?(status_message.id) && + !mention_stream_for(user).map(&:id).include?(status_message.id) + end + end + + let(:user1) { FactoryGirl.create(:user_with_aspect) } + let(:user2) { FactoryGirl.create(:user_with_aspect, friends: [user1, user3]) } + let(:user3) { FactoryGirl.create(:user_with_aspect) } + + # see: https://github.com/diaspora/diaspora/issues/4160 + it "only mentions people that are in the target aspect" do status_msg = nil expect { - post "/status_messages.json", status_message: {text: text_mentioning(@user3)}, aspect_ids: default_aspect.id.to_s - status_msg = StatusMessage.find(JSON.parse(response.body)["id"]) + status_msg = post_status_message(user3) }.to change(Post, :count).by(1).and change(AspectVisibility, :count).by(1) expect(status_msg).not_to be_nil expect(status_msg.public?).to be false - expect(status_msg.text).to include(@user3.name) - expect(status_msg.text).not_to include(@user3.diaspora_handle) - expect(status_msg.text).to include(user_profile_path(username: @user3.username)) + expect(status_msg.text).to include(user3.name) - expect(stream_for(@user3).map(&:id)).not_to include(status_msg.id) - expect(mention_stream_for(@user3).map(&:id)).not_to include(status_msg.id) + expect(user3).not_to be_mentioned_in(status_msg) + expect(status_msg).not_to be_in_streams_of(user3) + end + + context "in private post via federation" do + let(:status_msg) { + receive_status_message_via_federation(text_mentioning(user2, user3), user3) + } + + it "receiver is mentioned in status message" do + expect(user3).to be_mentioned_in(status_msg) + end + + it "receiver can see status message in streams" do + expect(status_msg).to be_in_streams_of(user3) + end + + it "non-receiver is not mentioned in status message" do + expect(user2).not_to be_mentioned_in(status_msg) + end + + it "non-receiver can't see status message in streams" do + expect(status_msg).not_to be_in_streams_of(user2) + end + end + + context "in private post via federation with multiple recipients" do + let(:status_msg) { + receive_status_message_via_federation(text_mentioning(user3, user2), user3, user2) + } + + it "mentions all recipients in the status message" do + [user2, user3].each do |user| + expect(user).to be_mentioned_in(status_msg) + end + end + + it "all recipients can see status message in streams" do + [user2, user3].each do |user| + expect(status_msg).to be_in_streams_of(user) + end + end end it "mentions people in public posts" do - expect(users_connected?(@user1, @user3)).to be false - status_msg = nil expect { - post "/status_messages.json", status_message: {text: text_mentioning(@user3)}, aspect_ids: "public" - status_msg = StatusMessage.find(JSON.parse(response.body)["id"]) + status_msg = post_status_message(user3, "public") }.to change(Post, :count).by(1) expect(status_msg).not_to be_nil expect(status_msg.public?).to be true - expect(status_msg.text).to include(@user3.name) - expect(status_msg.text).to include(@user3.diaspora_handle) + expect(status_msg.text).to include(user3.name) + expect(status_msg.text).to include(user3.diaspora_handle) - expect(stream_for(@user3).map(&:id)).to include(status_msg.id) - expect(mention_stream_for(@user3).map(&:id)).to include(status_msg.id) + expect(user3).to be_mentioned_in(status_msg) + expect(status_msg).to be_in_streams_of(user3) end it "mentions people that are in the target aspect" do - expect(users_connected?(@user1, @user2)).to be true - status_msg = nil expect { - post "/status_messages.json", status_message: {text: text_mentioning(@user2)}, aspect_ids: default_aspect.id.to_s - status_msg = StatusMessage.find(JSON.parse(response.body)["id"]) + status_msg = post_status_message(user2) }.to change(Post, :count).by(1).and change(AspectVisibility, :count).by(1) expect(status_msg).not_to be_nil expect(status_msg.public?).to be false - expect(status_msg.text).to include(@user2.name) - expect(status_msg.text).to include(@user2.diaspora_handle) + expect(status_msg.text).to include(user2.name) + expect(status_msg.text).to include(user2.diaspora_handle) - expect(stream_for(@user2).map(&:id)).to include(status_msg.id) - expect(mention_stream_for(@user2).map(&:id)).to include(status_msg.id) + expect(user2).to be_mentioned_in(status_msg) + expect(status_msg).to be_in_streams_of(user2) + end + + context "in comments" do + let(:author) { FactoryGirl.create(:user_with_aspect) } + + shared_context "commenter is author" do + let(:commenter) { author } + end + + shared_context "commenter is author's friend" do + let(:commenter) { FactoryGirl.create(:user_with_aspect, friends: [author]) } + end + + shared_context "commenter is not author's friend" do + let(:commenter) { FactoryGirl.create(:user) } + end + + shared_context "mentioned user is author" do + let(:mentioned_user) { author } + end + + shared_context "mentioned user is author's friend" do + let(:mentioned_user) { FactoryGirl.create(:user_with_aspect, friends: [author]) } + end + + shared_context "mentioned user is not author's friend" do + let(:mentioned_user) { FactoryGirl.create(:user) } + end + + context "with public post" do + let(:status_msg) { FactoryGirl.create(:status_message, author: author.person, public: true) } + + [ + ["commenter is author's friend", "mentioned user is not author's friend"], + ["commenter is author's friend", "mentioned user is author"], + ["commenter is not author's friend", "mentioned user is author's friend"], + ["commenter is not author's friend", "mentioned user is not author's friend"], + ["commenter is author", "mentioned user is author's friend"], + ["commenter is author", "mentioned user is not author's friend"] + ].each do |commenters_context, mentioned_context| + context "when #{commenters_context} and #{mentioned_context}" do + include_context commenters_context + include_context mentioned_context + + let(:comment) { + inlined_jobs do + commenter.comment!(status_msg, text_mentioning(mentioned_user)) + end + } + + subject { mentioned_user } + it { is_expected.to be_mentioned_in(comment) } + end + end + + context "when comment is received via federation" do + context "when mentioned user is remote" do + it "relays the comment to the mentioned user" do + mentioned_person = FactoryGirl.create(:person) + expect_any_instance_of(Diaspora::Federation::Dispatcher::Public) + .to receive(:deliver_to_remote).with([mentioned_person]) + + receive_comment_via_federation(text_mentioning(mentioned_person), status_msg) + end + end + end + end + + context "with private post" do + [ + ["commenter is author's friend", "mentioned user is author's friend"], + ["commenter is author", "mentioned user is author's friend"], + ["commenter is author's friend", "mentioned user is author"] + ].each do |commenters_context, mentioned_context| + context "when #{commenters_context} and #{mentioned_context}" do + include_context commenters_context + include_context mentioned_context + + let(:parent) { FactoryGirl.create(:status_message_in_aspect, author: author.person) } + let(:comment) { + inlined_jobs do + commenter.comment!(parent, text_mentioning(mentioned_user)) + end + } + + before do + mentioned_user.like!(parent) + end + + subject { mentioned_user } + it { is_expected.to be_mentioned_in(comment) } + end + end + + context "when comment is received via federation" do + let(:parent) { FactoryGirl.create(:status_message_in_aspect, author: user2.person) } + + before do + user3.like!(parent) + user1.like!(parent) + end + + let(:comment_text) { text_mentioning(user2, user3, user1) } + let(:comment) { receive_comment_via_federation(comment_text, parent) } + + it "mentions all the recepients" do + [user1, user2, user3].each do |user| + expect(user).to be_mentioned_in(comment) + end + end + + context "with only post author mentioned" do + let(:post_author) { parent.author.owner } + let(:comment_text) { text_mentioning(post_author) } + + it "makes only one notification for each recipient" do + expect { + comment + }.to change { Notifications::MentionedInComment.for(post_author).count }.by(1) + .and change { Notifications::AlsoCommented.for(user1).count }.by(1) + .and change { Notifications::AlsoCommented.for(user3).count }.by(1) + + expect(mentioning_mail_notification(post_author, comment).count).to eq(1) + + [user1, user3].each do |user| + expect(also_commented_mail_notification(user, parent).count).to eq(1) + end + end + end + end + + context "commenter can't mention a non-participant" do + let(:status_msg) { FactoryGirl.create(:status_message_in_aspect, author: author.person) } + + [ + ["commenter is author's friend", "mentioned user is not author's friend"], + ["commenter is not author's friend", "mentioned user is author's friend"], + ["commenter is not author's friend", "mentioned user is not author's friend"], + ["commenter is author", "mentioned user is author's friend"], + ["commenter is author", "mentioned user is not author's friend"] + ].each do |commenters_context, mentioned_context| + context "when #{commenters_context} and #{mentioned_context}" do + include_context commenters_context + include_context mentioned_context + + let(:comment) { + inlined_jobs do + commenter.comment!(status_msg, text_mentioning(mentioned_user)) + end + } + + subject { mentioned_user } + it { is_expected.not_to be_mentioned_in(comment) } + end + end + end + end end end diff --git a/spec/lib/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb index 7e55bb7b8..c31f6332d 100644 --- a/spec/lib/diaspora/federation/receive_spec.rb +++ b/spec/lib/diaspora/federation/receive_spec.rb @@ -15,17 +15,15 @@ describe Diaspora::Federation::Receive do end describe ".comment" do - let(:comment_data) { - FactoryGirl.attributes_for( - :comment_entity, - author: sender.diaspora_handle, - parent_guid: post.guid, - author_signature: "aa" - ) - } let(:comment_entity) { - DiasporaFederation::Entities::Comment.new( - comment_data, [:author, :guid, :parent_guid, :text, "new_property"], "new_property" => "data" + build_relayable_federation_entity( + :comment, + { + author: sender.diaspora_handle, + parent_guid: post.guid, + author_signature: "aa" + }, + "new_property" => "data" ) } @@ -57,7 +55,7 @@ describe Diaspora::Federation::Receive do expect(comment.signature).not_to be_nil expect(comment.signature.author_signature).to eq("aa") expect(comment.signature.additional_data).to eq("new_property" => "data") - expect(comment.signature.order).to eq(%w(author guid parent_guid text new_property)) + expect(comment.signature.order).to eq(comment_entity.xml_order.map(&:to_s)) end let(:entity) { comment_entity } diff --git a/spec/lib/diaspora/mentionable_spec.rb b/spec/lib/diaspora/mentionable_spec.rb index 365d2dd04..de8e2fcac 100644 --- a/spec/lib/diaspora/mentionable_spec.rb +++ b/spec/lib/diaspora/mentionable_spec.rb @@ -102,7 +102,7 @@ STR end end - describe "#filter_for_aspects" do + describe "#filter_people" do before do @user_a = FactoryGirl.create(:user_with_aspect, username: "user_a") @user_b = FactoryGirl.create(:user, username: "user_b") @@ -122,8 +122,10 @@ STR end it "filters mention, if contact is not in a given aspect" do - aspect_id = @user_a.aspects.where(name: "generic").first.id - txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_c, @user_a, aspect_id) + txt = Diaspora::Mentionable.filter_people( + @test_txt_c, + @user_a.aspects.where(name: "generic").first.contacts.map(&:person_id) + ) expect(txt).to include("user C") expect(txt).to include(local_or_remote_person_path(@user_c.person)) @@ -132,18 +134,13 @@ STR end it "leaves mention, if contact is in a given aspect" do - aspect_id = @user_a.aspects.where(name: "generic").first.id - txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_b, @user_a, aspect_id) + txt = Diaspora::Mentionable.filter_people( + @test_txt_b, + @user_a.aspects.where(name: "generic").first.contacts.map(&:person_id) + ) expect(txt).to include("user B") expect(txt).to include(@mention_b) end - - it "recognizes 'all' as keyword for aspects" do - txt = Diaspora::Mentionable.filter_for_aspects(@test_txt_bc, @user_a, "all") - - expect(txt).to include(@mention_b) - expect(txt).to include(@mention_c) - end end end diff --git a/spec/mailers/notifier_spec.rb b/spec/mailers/notifier_spec.rb index 13a05a27e..d0f545bc8 100644 --- a/spec/mailers/notifier_spec.rb +++ b/spec/mailers/notifier_spec.rb @@ -79,9 +79,9 @@ describe Notifier, type: :mailer do before do @user = alice @post = FactoryGirl.create(:status_message, public: true) - @mention = Mention.create(person: @user.person, post: @post) + @mention = Mention.create(person: @user.person, mentions_container: @post) - @mail = Notifier.mentioned(@user.id, @post.author.id, @mention.id) + @mail = Notifier.send_notification("mentioned", @user.id, @post.author.id, @mention.id) end it "TO: goes to the right person" do @@ -106,13 +106,41 @@ describe Notifier, type: :mailer do end end + describe ".mentioned_in_comment" do + let(:user) { alice } + let(:comment) { FactoryGirl.create(:comment) } + let(:mention) { Mention.create(person: user.person, mentions_container: comment) } + let(:mail) { Notifier.send_notification("mentioned_in_comment", user.id, comment.author.id, mention.id) } + + it "TO: goes to the right person" do + expect(mail.to).to eq([user.email]) + end + + it "SUBJECT: has the name of person mentioning in the subject" do + expect(mail.subject).to include(comment.author.name) + end + + it "has the comment link in the body" do + expect(mail.body.encoded).to include(post_url(comment.parent, anchor: comment.guid)) + end + + it "renders proper wording when limited" do + expect(mail.body.encoded).to include(I18n.translate("notifier.mentioned_in_comment.limited_post")) + end + + it "renders comment text when public" do + comment.parent.update(public: true) + expect(mail.body.encoded).to include(comment.message.plain_text_without_markdown) + end + end + describe ".mentioned limited" do before do @user = alice @post = FactoryGirl.create(:status_message, public: false) - @mention = Mention.create(person: @user.person, post: @post) + @mention = Mention.create(person: @user.person, mentions_container: @post) - @mail = Notifier.mentioned(@user.id, @post.author.id, @mention.id) + @mail = Notifier.send_notification("mentioned", @user.id, @post.author.id, @mention.id) end it "TO: goes to the right person" do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 4fe1d983b..05499dc39 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -7,6 +7,8 @@ describe Comment, type: :model do let(:status_bob) { bob.post(:status_message, text: "hello", to: bob.aspects.first.id) } let(:comment_alice) { alice.comment!(status_bob, "why so formal?") } + it_behaves_like "it is mentions container" + describe "#destroy" do it "should delete a participation" do comment_alice @@ -21,6 +23,55 @@ describe Comment, type: :model do end end + describe "#people_allowed_to_be_mentioned" do + let(:kate) { FactoryGirl.create(:user_with_aspect, friends: [bob]) } + let(:olga) { FactoryGirl.create(:user_with_aspect, friends: [bob]) } + + it "returns the author and people who have commented or liked the private post" do + eve.comment!(status_bob, "comment text") + kate.like!(status_bob) + olga.participate!(status_bob) + status_bob.reload + expect(comment_alice.people_allowed_to_be_mentioned).to match_array([alice, bob, eve, kate].map(&:person_id)) + end + + it "returns :all for public posts" do + # set parent public + status_bob.update(public: true) + expect(comment_alice.people_allowed_to_be_mentioned).to eq(:all) + end + end + + describe "#subscribers" do + let(:status_bob) { FactoryGirl.create(:status_message, public: true, author: bob.person) } + let(:comment_alice) { + FactoryGirl.create( + :comment, + text: text_mentioning(remote_raphael, local_luke), + post: status_bob, + author: alice.person + ) + } + + context "on the parent post pod" do + it "includes mentioned people to subscribers list" do + expect(comment_alice.subscribers).to include(remote_raphael) + end + + it "doesn't include local mentioned people if they aren't participant or contact" do + expect(comment_alice.subscribers).not_to include(local_luke) + end + end + + context "on a non parent post pod" do + let(:status_bob) { FactoryGirl.create(:status_message) } # make the message remote + + it "doesn't include mentioned people to subscribers list" do + expect(comment_alice.subscribers).not_to include(remote_raphael) + end + end + end + describe "User#comment" do it "should be able to comment on one's own status" do bob.comment!(status_bob, "sup dog") diff --git a/spec/models/mention_spec.rb b/spec/models/mention_spec.rb index e0e3a2a46..6fe5ecdb5 100644 --- a/spec/models/mention_spec.rb +++ b/spec/models/mention_spec.rb @@ -6,9 +6,9 @@ describe Mention, type: :model do describe "after destroy" do it "destroys a notification" do sm = alice.post(:status_message, text: "hi", to: alice.aspects.first) - mention = Mention.create!(person: bob.person, post: sm) + mention = Mention.create!(person: bob.person, mentions_container: sm) - Notifications::Mentioned.notify(sm, [bob.id]) + Notifications::MentionedInPost.notify(sm, [bob.id]) expect { mention.destroy diff --git a/spec/models/notifications/mentioned_in_post_spec.rb b/spec/models/notifications/mentioned_in_post_spec.rb new file mode 100644 index 000000000..90c5b007d --- /dev/null +++ b/spec/models/notifications/mentioned_in_post_spec.rb @@ -0,0 +1,72 @@ +describe Notifications::MentionedInPost, type: :model do + let(:sm) { + FactoryGirl.create(:status_message, author: alice.person, text: "hi @{bob; #{bob.diaspora_handle}}", public: true) + } + let(:mentioned_notification) { Notifications::MentionedInPost.new(recipient: bob) } + + describe ".notify" do + it "calls create_notification with mention" do + expect(Notifications::MentionedInPost).to receive(:create_notification).with( + bob, sm.mentions.first, sm.author + ).and_return(mentioned_notification) + + Notifications::MentionedInPost.notify(sm, []) + end + + it "sends an email to the mentioned person" do + allow(Notifications::MentionedInPost).to receive(:create_notification).and_return(mentioned_notification) + expect(bob).to receive(:mail).with(Workers::Mail::Mentioned, bob.id, sm.author.id, sm.mentions.first.id) + + Notifications::MentionedInPost.notify(sm, []) + end + + it "does nothing if the mentioned person is not local" do + sm = FactoryGirl.create( + :status_message, + author: alice.person, + text: "hi @{raphael; #{remote_raphael.diaspora_handle}}", + public: true + ) + expect(Notifications::MentionedInPost).not_to receive(:create_notification) + + Notifications::MentionedInPost.notify(sm, []) + end + + it "does not notify if the author of the post is ignored" do + bob.blocks.create(person: sm.author) + + expect_any_instance_of(Notifications::MentionedInPost).not_to receive(:email_the_user) + + Notifications::MentionedInPost.notify(sm, []) + + expect(Notifications::MentionedInPost.where(target: sm.mentions.first)).not_to exist + end + + context "with private post" do + let(:private_sm) { + FactoryGirl.create( + :status_message, + author: remote_raphael, + text: "hi @{bob; #{bob.diaspora_handle}}", + public: false + ).tap {|private_sm| + private_sm.receive([bob.id, alice.id]) + } + } + + it "calls create_notification if the mentioned person is a recipient of the post" do + expect(Notifications::MentionedInPost).to receive(:create_notification).with( + bob, private_sm.mentions.first, private_sm.author + ).and_return(mentioned_notification) + + Notifications::MentionedInPost.notify(private_sm, [bob.id]) + end + + it "does not call create_notification if the mentioned person is not a recipient of the post" do + expect(Notifications::MentionedInPost).not_to receive(:create_notification) + + Notifications::MentionedInPost.notify(private_sm, [alice.id]) + end + end + end +end diff --git a/spec/models/notifications/mentioned_spec.rb b/spec/models/notifications/mentioned_spec.rb index d9f05f30e..65ab03a49 100644 --- a/spec/models/notifications/mentioned_spec.rb +++ b/spec/models/notifications/mentioned_spec.rb @@ -1,70 +1,53 @@ -describe Notifications::Mentioned, type: :model do - let(:sm) { - FactoryGirl.create(:status_message, author: alice.person, text: "hi @{bob; #{bob.diaspora_handle}}", public: true) - } - let(:mentioned_notification) { Notifications::Mentioned.new(recipient: bob) } +describe Notifications::Mentioned do + class TestNotification < Notification + include Notifications::Mentioned + + def self.filter_mentions(mentions, *) + mentions + end + end describe ".notify" do - it "calls create_notification with mention" do - expect(Notifications::Mentioned).to receive(:create_notification).with( - bob, sm.mentions.first, sm.author - ).and_return(mentioned_notification) + let(:status_message) { + FactoryGirl.create(:status_message, text: text_mentioning(remote_raphael, alice, bob, eve), author: eve.person) + } - Notifications::Mentioned.notify(sm, []) + it "calls filter_mentions on self" do + expect(TestNotification).to receive(:filter_mentions).with( + Mention.where(mentions_container: status_message, person: [alice, bob].map(&:person)), + status_message, + [alice.id, bob.id] + ).and_return([]) + + TestNotification.notify(status_message, [alice.id, bob.id]) end - it "sends an email to the mentioned person" do - allow(Notifications::Mentioned).to receive(:create_notification).and_return(mentioned_notification) - expect(bob).to receive(:mail).with(Workers::Mail::Mentioned, bob.id, sm.author.id, sm.mentions.first.id) - - Notifications::Mentioned.notify(sm, []) - end - - it "does nothing if the mentioned person is not local" do - sm = FactoryGirl.create( - :status_message, - author: alice.person, - text: "hi @{raphael; #{remote_raphael.diaspora_handle}}", - public: true - ) - expect(Notifications::Mentioned).not_to receive(:create_notification) - - Notifications::Mentioned.notify(sm, []) - end - - it "does not notify if the author of the post is ignored" do - bob.blocks.create(person: sm.author) - - expect_any_instance_of(Notifications::Mentioned).not_to receive(:email_the_user) - - Notifications::Mentioned.notify(sm, []) - - expect(Notifications::Mentioned.where(target: sm.mentions.first)).not_to exist - end - - context "with private post" do - let(:private_sm) { - FactoryGirl.create( - :status_message, - author: remote_raphael, - text: "hi @{bob; #{bob.diaspora_handle}}", - public: false + it "creates notification for each mention" do + [alice, bob].each do |recipient| + expect(TestNotification).to receive(:create_notification).with( + recipient, + Mention.where(mentions_container: status_message, person: recipient.person_id).first, + status_message.author ) - } - - it "calls create_notification if the mentioned person is a recipient of the post" do - expect(Notifications::Mentioned).to receive(:create_notification).with( - bob, private_sm.mentions.first, private_sm.author - ).and_return(mentioned_notification) - - Notifications::Mentioned.notify(private_sm, [bob.id]) end - it "does not call create_notification if the mentioned person is not a recipient of the post" do - expect(Notifications::Mentioned).not_to receive(:create_notification) + TestNotification.notify(status_message, nil) + end - Notifications::Mentioned.notify(private_sm, [alice.id]) - end + it "creates email notification for mention" do + status_message = FactoryGirl.create(:status_message, text: text_mentioning(alice), author: eve.person) + expect_any_instance_of(TestNotification).to receive(:email_the_user).with( + Mention.where(mentions_container: status_message, person: alice.person_id).first, + status_message.author + ) + + TestNotification.notify(status_message, nil) + end + + it "doesn't create notification if it was filtered out by filter_mentions" do + expect(TestNotification).to receive(:filter_mentions).and_return([]) + expect(TestNotification).not_to receive(:create_notification) + TestNotification.notify(status_message, nil) end end end diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb index 843305813..804a423b8 100644 --- a/spec/models/status_message_spec.rb +++ b/spec/models/status_message_spec.rb @@ -14,11 +14,12 @@ describe StatusMessage, type: :model do it "returns status messages where the given person is mentioned" do @bob = bob.person @test_string = "@{Daniel; #{@bob.diaspora_handle}} can mention people like Raph" + post1 = FactoryGirl.create(:status_message, text: @test_string, public: true) + post2 = FactoryGirl.create(:status_message, text: @test_string, public: true) FactoryGirl.create(:status_message, text: @test_string) - FactoryGirl.create(:status_message, text: @test_string) - FactoryGirl.create(:status_message) + FactoryGirl.create(:status_message, public: true) - expect(StatusMessage.where_person_is_mentioned(bob).count).to eq(2) + expect(StatusMessage.where_person_is_mentioned(@bob).ids).to match_array([post1.id, post2.id]) end end @@ -81,14 +82,6 @@ describe StatusMessage, type: :model do end end - describe ".after_create" do - it "calls create_mentions" do - status = FactoryGirl.build(:status_message, text: "text @{Test; #{alice.diaspora_handle}}") - expect(status).to receive(:create_mentions).and_call_original - status.save - end - end - context "emptiness" do it "needs either a message or at least one photo" do post = user.build_post(:status_message, text: nil) @@ -131,57 +124,20 @@ describe StatusMessage, type: :model do expect(status_message).not_to be_valid end - describe "mentions" do - let(:people) { [alice, bob, eve].map(&:person) } - let(:test_string) { - "@{Raphael; #{people[0].diaspora_handle}} can mention people like Raphael @{Ilya; #{people[1].diaspora_handle}} - can mention people like Raphaellike Raphael @{Daniel; #{people[2].diaspora_handle}} can mention people like Raph" - } - let(:status_message) { create(:status_message, text: test_string) } + it_behaves_like "it is mentions container" - describe "#create_mentions" do - it "creates a mention for everyone mentioned in the message" do - status_message - expect(Diaspora::Mentionable).to receive(:people_from_string).and_return(people) - status_message.mentions.delete_all - status_message.create_mentions - expect(status_message.mentions(true).map(&:person).to_set).to eq(people.to_set) - end + describe "#people_allowed_to_be_mentioned" do + it "returns only aspects members for private posts" do + sm = FactoryGirl.build(:status_message_in_aspect) + sm.author.owner.share_with(alice.person, sm.author.owner.aspects.first) + sm.author.owner.share_with(eve.person, sm.author.owner.aspects.first) + sm.save! - it "does not barf if it gets called twice" do - status_message.create_mentions - - expect { - status_message.create_mentions - }.to_not raise_error - end + expect(sm.people_allowed_to_be_mentioned).to match_array([alice.person_id, eve.person_id]) end - describe "#mentioned_people" do - it "does not call create_mentions if there are no mentions in the db" do - status_message.mentions.delete_all - expect(status_message).not_to receive(:create_mentions) - status_message.mentioned_people - end - - it "returns the mentioned people" do - expect(status_message.mentioned_people.to_set).to eq(people.to_set) - end - - it "does not call create_mentions if there are mentions in the db" do - expect(status_message).not_to receive(:create_mentions) - status_message.mentioned_people - end - end - - describe "#mentions?" do - it "returns true if the person was mentioned" do - expect(status_message.mentions?(people[0])).to be true - end - - it "returns false if the person was not mentioned" do - expect(status_message.mentions?(FactoryGirl.build(:person))).to be false - end + it "returns :all for public posts" do + expect(FactoryGirl.create(:status_message, public: true).people_allowed_to_be_mentioned).to eq(:all) end end diff --git a/spec/presenters/post_presenter_spec.rb b/spec/presenters/post_presenter_spec.rb index 0c2a5c993..952da0a2c 100644 --- a/spec/presenters/post_presenter_spec.rb +++ b/spec/presenters/post_presenter_spec.rb @@ -126,9 +126,9 @@ describe PostPresenter do it "does not raise if the root of a reshare does not exist anymore" do reshare = FactoryGirl.create(:reshare) - reshare.root = nil + reshare.update(root: nil) - expect(PostPresenter.new(reshare).send(:description)).to eq(nil) + expect(PostPresenter.new(Post.find(reshare.id)).send(:description)).to eq(nil) end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb new file mode 100644 index 000000000..8ef4bec5d --- /dev/null +++ b/spec/services/notification_service_spec.rb @@ -0,0 +1,74 @@ +describe NotificationService do + describe "notification interrelation" do + context "with mention in comment" do + let(:status_message) { + FactoryGirl.create(:status_message, public: true, author: alice.person).tap {|status_message| + eve.comment!(status_message, "whatever") + } + } + + let(:comment) { + FactoryGirl.create( + :comment, + author: bob.person, + text: text_mentioning(alice, eve), + post: status_message + ) + } + + it "sends only mention notification" do + [alice, eve].each do |user| + expect(Workers::Mail::MentionedInComment).to receive(:perform_async).with( + user.id, + bob.person.id, + *comment.mentions.where(person: user.person).ids + ) + end + + expect { + NotificationService.new.notify(comment, []) + }.to change { Notification.where(recipient_id: alice).count }.by(1) + .and change { Notification.where(recipient_id: eve).count }.by(1) + + [alice, eve].each do |user| + expect( + Notifications::MentionedInComment.where(target: comment.mentions, recipient_id: user.id) + ).to exist + + expect( + Notifications::CommentOnPost.where(target: comment.parent, recipient_id: user.id) + ).not_to exist + + expect( + Notifications::AlsoCommented.where(target: comment.parent, recipient_id: user.id) + ).not_to exist + end + end + + context "with \"mentioned in comment\" email turned off" do + before do + alice.user_preferences.create(email_type: "mentioned_in_comment") + eve.user_preferences.create(email_type: "mentioned_in_comment") + end + + it "calls appropriate mail worker instead" do + expect(Workers::Mail::MentionedInComment).not_to receive(:perform_async) + + expect(Workers::Mail::CommentOnPost).to receive(:perform_async).with( + alice.id, + bob.person.id, + *comment.mentions.where(person: alice.person).ids + ) + + expect(Workers::Mail::AlsoCommented).to receive(:perform_async).with( + eve.id, + bob.person.id, + *comment.mentions.where(person: eve.person).ids + ) + + NotificationService.new.notify(comment, []) + end + end + end + end +end diff --git a/spec/services/post_service_spec.rb b/spec/services/post_service_spec.rb index 3d6b96a48..b19a1638d 100644 --- a/spec/services/post_service_spec.rb +++ b/spec/services/post_service_spec.rb @@ -111,6 +111,8 @@ describe PostService do end describe "#mark_user_notifications" do + let(:status_text) { text_mentioning(alice) } + it "marks a corresponding notifications as read" do FactoryGirl.create(:notification, recipient: alice, target: post, unread: true) FactoryGirl.create(:notification, recipient: alice, target: post, unread: true) @@ -121,7 +123,6 @@ describe PostService do end it "marks a corresponding mention notification as read" do - status_text = "this is a text mentioning @{Mention User ; #{alice.diaspora_handle}} ... have fun testing!" mention_post = bob.post(:status_message, text: status_text, public: true) expect { @@ -129,6 +130,16 @@ describe PostService do }.to change(Notification.where(unread: true), :count).by(-1) end + it "marks a corresponding mention in comment notification as read" do + notification = FactoryGirl.create(:notification_mentioned_in_comment) + status_message = notification.target.mentions_container.parent + user = notification.recipient + + expect { + PostService.new(user).mark_user_notifications(status_message.id) + }.to change(Notification.where(unread: true), :count).by(-1) + end + it "does not change the update_at date/time for post notifications" do notification = Timecop.travel(1.minute.ago) do FactoryGirl.create(:notification, recipient: alice, target: post, unread: true) @@ -140,7 +151,6 @@ describe PostService do end it "does not change the update_at date/time for mention notifications" do - status_text = "this is a text mentioning @{Mention User ; #{alice.diaspora_handle}} ... have fun testing!" mention_post = Timecop.travel(1.minute.ago) do bob.post(:status_message, text: status_text, public: true) end diff --git a/spec/services/status_message_creation_service_spec.rb b/spec/services/status_message_creation_service_spec.rb index 80631f266..47838d172 100644 --- a/spec/services/status_message_creation_service_spec.rb +++ b/spec/services/status_message_creation_service_spec.rb @@ -151,44 +151,6 @@ describe StatusMessageCreationService do end end - context "with mentions" do - let(:mentioned_user) { FactoryGirl.create(:user) } - let(:text) { - "Hey @{#{bob.name}; #{bob.diaspora_handle}} and @{#{mentioned_user.name}; #{mentioned_user.diaspora_handle}}!" - } - - it "calls Diaspora::Mentionable#filter_for_aspects for private posts" do - expect(Diaspora::Mentionable).to receive(:filter_for_aspects).with(text, alice, aspect.id.to_s) - .and_call_original - StatusMessageCreationService.new(alice).create(params) - end - - it "keeps mentions for users in one of the aspects" do - status_message = StatusMessageCreationService.new(alice).create(params) - expect(status_message.text).to include bob.name - expect(status_message.text).to include bob.diaspora_handle - end - - it "removes mentions for users in other aspects" do - status_message = StatusMessageCreationService.new(alice).create(params) - expect(status_message.text).to include mentioned_user.name - expect(status_message.text).not_to include mentioned_user.diaspora_handle - end - - it "doesn't call Diaspora::Mentionable#filter_for_aspects for public posts" do - expect(Diaspora::Mentionable).not_to receive(:filter_for_aspects) - StatusMessageCreationService.new(alice).create(params.merge(public: true)) - end - - it "keeps all mentions in public posts" do - status_message = StatusMessageCreationService.new(alice).create(params.merge(public: true)) - expect(status_message.text).to include bob.name - expect(status_message.text).to include bob.diaspora_handle - expect(status_message.text).to include mentioned_user.name - expect(status_message.text).to include mentioned_user.diaspora_handle - end - end - context "dispatch" do it "dispatches the StatusMessage" do expect(alice).to receive(:dispatch_post).with(instance_of(StatusMessage), hash_including(service_types: [])) @@ -201,6 +163,16 @@ describe StatusMessageCreationService do hash_including(service_types: array_including(%w(Services::Facebook Services::Twitter)))) StatusMessageCreationService.new(alice).create(params.merge(services: %w(twitter facebook))) end + + context "with mention" do + let(:text) { text_mentioning(eve) } + + # this is only required until changes from #6818 are deployed on every pod + it "filters out mentions from text attribute" do + status_message = StatusMessageCreationService.new(alice).create(params) + expect(status_message.text).not_to include(eve.diaspora_handle) + end + end end end end diff --git a/spec/shared_behaviors/mentions_container.rb b/spec/shared_behaviors/mentions_container.rb new file mode 100644 index 000000000..87dedfef5 --- /dev/null +++ b/spec/shared_behaviors/mentions_container.rb @@ -0,0 +1,44 @@ +shared_examples_for "it is mentions container" do + let(:people) { [alice, bob, eve].map(&:person) } + let(:test_string) { + "@{Raphael; #{people[0].diaspora_handle}} can mention people like @{Ilya; #{people[1].diaspora_handle}}"\ + "can mention people like @{Daniel; #{people[2].diaspora_handle}}" + } + let(:target) { FactoryGirl.build(described_class.to_s.underscore.to_sym, text: test_string, author: alice.person) } + let(:target_persisted) { + target.save! + target + } + + describe ".after_create" do + it "calls create_mentions" do + expect(target).to receive(:create_mentions).and_call_original + target.save + end + end + + describe "#create_mentions" do + it "creates a mention for everyone mentioned in the message" do + people.each do |person| + expect(target.mentions).to receive(:find_or_create_by).with(person_id: person.id) + end + target.create_mentions + end + + it "does not barf if it gets called twice" do + expect { + target_persisted.create_mentions + }.to_not raise_error + end + end + + describe "#mentioned_people" do + it "returns the mentioned people if non-persisted" do + expect(target.mentioned_people).to match_array(people) + end + + it "returns the mentioned people if persisted" do + expect(target_persisted.mentioned_people).to match_array(people) + end + end +end diff --git a/spec/workers/mail/mentioned_spec.rb b/spec/workers/mail/mentioned_spec.rb index 5d8d76cd7..cc06f5d58 100644 --- a/spec/workers/mail/mentioned_spec.rb +++ b/spec/workers/mail/mentioned_spec.rb @@ -7,11 +7,12 @@ describe Workers::Mail::Mentioned do it "should call .deliver on the notifier object" do user = alice sm = FactoryGirl.build(:status_message) - m = Mention.new(:person => user.person, :post=> sm) + m = Mention.new(person: user.person, mentions_container: sm) mail_double = double() expect(mail_double).to receive(:deliver_now) - expect(Notifier).to receive(:send_notification).with("mentioned", user.id, sm.author.id, m.id).and_return(mail_double) + expect(Notifier).to receive(:send_notification) + .with("mentioned", user.id, sm.author.id, m.id).and_return(mail_double) Workers::Mail::Mentioned.new.perform(user.id, sm.author.id, m.id) end