From da2ef4d676ff71e6ab3edf8d1a7cee8bf6b6d353 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 20 Nov 2016 00:33:02 +0100 Subject: [PATCH] Adding unified streamable notifications --- app/channels/application_cable/channel.rb | 2 +- .../api/v1/notifications_controller.rb | 17 +++++++ app/lib/feed_manager.rb | 6 +-- app/mailers/notification_mailer.rb | 36 ++++++--------- app/models/notification.rb | 44 +++++++++++++++++++ app/services/favourite_service.rb | 2 +- app/services/follow_service.rb | 2 +- app/services/notify_service.rb | 36 +++++++++++++++ app/services/process_feed_service.rb | 8 ++-- app/services/process_interaction_service.rb | 8 ++-- app/services/process_mentions_service.rb | 2 +- app/services/reblog_service.rb | 2 +- app/views/api/v1/notifications/index.rabl | 2 + app/views/api/v1/notifications/show.rabl | 11 +++++ config/routes.rb | 2 + .../20161119211120_create_notifications.rb | 13 ++++++ db/schema.rb | 11 ++++- spec/fabricators/notification_fabricator.rb | 4 ++ spec/mailers/notification_mailer_spec.rb | 12 +++-- spec/models/notification_spec.rb | 29 ++++++++++++ 20 files changed, 205 insertions(+), 44 deletions(-) create mode 100644 app/controllers/api/v1/notifications_controller.rb create mode 100644 app/models/notification.rb create mode 100644 app/services/notify_service.rb create mode 100644 app/views/api/v1/notifications/index.rabl create mode 100644 app/views/api/v1/notifications/show.rabl create mode 100644 db/migrate/20161119211120_create_notifications.rb create mode 100644 spec/fabricators/notification_fabricator.rb create mode 100644 spec/models/notification_spec.rb diff --git a/app/channels/application_cable/channel.rb b/app/channels/application_cable/channel.rb index 69e069212..f9154bd7f 100644 --- a/app/channels/application_cable/channel.rb +++ b/app/channels/application_cable/channel.rb @@ -10,7 +10,7 @@ module ApplicationCable return [nil, message] if message['type'] == 'delete' status = Status.find_by(id: message['id']) - message['message'] = FeedManager.instance.inline_render(current_user.account, status) + message['message'] = FeedManager.instance.inline_render(current_user.account, 'api/v1/statuses/show', status) [status, message] end diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb new file mode 100644 index 000000000..509471f61 --- /dev/null +++ b/app/controllers/api/v1/notifications_controller.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class Api::V1::NotificationsController < ApiController + before_action -> { doorkeeper_authorize! :read } + before_action :require_user! + + respond_to :json + + def index + @notifications = Notification.where(account: current_account).with_includes.paginate_by_max_id(20, params[:max_id], params[:since_id]) + + next_path = api_v1_notifications_url(max_id: @notifications.last.id) if @notifications.size == 20 + prev_path = api_v1_notifications_url(since_id: @notifications.first.id) unless @notifications.empty? + + set_pagination_headers(next_path, prev_path) + end +end diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index b808d7a0f..c8512476d 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -26,7 +26,7 @@ class FeedManager def push(timeline_type, account, status) redis.zadd(key(timeline_type, account.id), status.id, status.reblog? ? status.reblog_of_id : status.id) trim(timeline_type, account.id) - broadcast(account.id, type: 'update', timeline: timeline_type, message: inline_render(account, status)) + broadcast(account.id, type: 'update', timeline: timeline_type, message: inline_render(account, 'api/v1/statuses/show', status)) end def broadcast(timeline_id, options = {}) @@ -39,7 +39,7 @@ class FeedManager redis.zremrangebyscore(key(type, account_id), '-inf', "(#{last.last}") end - def inline_render(target_account, status) + def inline_render(target_account, template, object) rabl_scope = Class.new do include RoutingHelper @@ -56,7 +56,7 @@ class FeedManager end end - Rabl::Renderer.new('api/v1/statuses/show', status, view_path: 'app/views', format: :json, scope: rabl_scope.new(target_account)).render + Rabl::Renderer.new(template, object, view_path: 'app/views', format: :json, scope: rabl_scope.new(target_account)).render end private diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index cf5ad3f92..7b2cac7f3 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -3,46 +3,38 @@ class NotificationMailer < ApplicationMailer helper StreamEntriesHelper - def mention(mentioned_account, status) - @me = mentioned_account - @status = status - - return unless @me.user.settings(:notification_emails).mention + def mention(recipient, notification) + @me = recipient + @status = notification.target_status I18n.with_locale(@me.user.locale || I18n.default_locale) do mail to: @me.user.email, subject: I18n.t('notification_mailer.mention.subject', name: @status.account.acct) end end - def follow(followed_account, follower) - @me = followed_account - @account = follower - - return unless @me.user.settings(:notification_emails).follow + def follow(recipient, notification) + @me = recipient + @account = notification.from_account I18n.with_locale(@me.user.locale || I18n.default_locale) do mail to: @me.user.email, subject: I18n.t('notification_mailer.follow.subject', name: @account.acct) end end - def favourite(target_status, from_account) - @me = target_status.account - @account = from_account - @status = target_status - - return unless @me.user.settings(:notification_emails).favourite + def favourite(recipient, notification) + @me = recipient + @account = notification.from_account + @status = notification.target_status I18n.with_locale(@me.user.locale || I18n.default_locale) do mail to: @me.user.email, subject: I18n.t('notification_mailer.favourite.subject', name: @account.acct) end end - def reblog(target_status, from_account) - @me = target_status.account - @account = from_account - @status = target_status - - return unless @me.user.settings(:notification_emails).reblog + def reblog(recipient, notification) + @me = recipient + @account = notification.from_account + @status = notification.target_status I18n.with_locale(@me.user.locale || I18n.default_locale) do mail to: @me.user.email, subject: I18n.t('notification_mailer.reblog.subject', name: @account.acct) diff --git a/app/models/notification.rb b/app/models/notification.rb new file mode 100644 index 000000000..66aefcb74 --- /dev/null +++ b/app/models/notification.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +class Notification < ApplicationRecord + include Paginable + + belongs_to :account + belongs_to :activity, polymorphic: true + + belongs_to :mention, foreign_type: 'Mention', foreign_key: 'activity_id' + belongs_to :status, foreign_type: 'Status', foreign_key: 'activity_id' + belongs_to :follow, foreign_type: 'Follow', foreign_key: 'activity_id' + belongs_to :favourite, foreign_type: 'Favourite', foreign_key: 'activity_id' + + STATUS_INCLUDES = [:account, :media_attachments, mentions: :account, reblog: [:account, mentions: :account]].freeze + + scope :with_includes, -> { includes(status: STATUS_INCLUDES, mention: [status: STATUS_INCLUDES], favourite: [:account, status: STATUS_INCLUDES], follow: :account) } + + def type + case activity_type + when 'Status' + :reblog + else + activity_type.downcase.to_sym + end + end + + def from_account + case type + when :mention + activity.status.account + when :follow, :favourite, :reblog + activity.account + end + end + + def target_status + case type + when :reblog + activity.reblog + when :favourite, :mention + activity.status + end + end +end diff --git a/app/services/favourite_service.rb b/app/services/favourite_service.rb index 9c6f12478..781b03b40 100644 --- a/app/services/favourite_service.rb +++ b/app/services/favourite_service.rb @@ -10,7 +10,7 @@ class FavouriteService < BaseService HubPingWorker.perform_async(account.id) if status.local? - NotificationMailer.favourite(status, account).deliver_later unless status.account.blocking?(account) + NotifyService.new.call(status.account, favourite) else NotificationWorker.perform_async(favourite.stream_entry.id, status.account_id) end diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 3b97840cb..cdae254bf 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -12,7 +12,7 @@ class FollowService < BaseService follow = source_account.follow!(target_account) if target_account.local? - NotificationMailer.follow(target_account, source_account).deliver_later unless target_account.blocking?(source_account) + NotifyService.new.call(target_account, follow) else subscribe_service.call(target_account) NotificationWorker.perform_async(follow.stream_entry.id, target_account.id) diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb new file mode 100644 index 000000000..a51c5b959 --- /dev/null +++ b/app/services/notify_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +class NotifyService < BaseService + def call(recipient, activity) + @recipient = recipient + @activity = activity + @notification = Notification.new(account: @recipient, activity: @activity) + + return if blocked? + + create_notification + send_email if email_enabled? + end + + private + + def blocked? + blocked = false + blocked ||= @recipient.id == @notification.from_account.id + blocked ||= @recipient.blocking?(@notification.from_account) + blocked + end + + def create_notification + @notification.save! + FeedManager.instance.broadcast(@recipient.id, type: 'notification', message: FeedManager.instance.inline_render(@recipient, 'api/v1/notifications/show', @notification)) + end + + def send_email + NotificationMailer.send(@notification.type, @recipient, @notification).deliver_later + end + + def email_enabled? + @recipient.user.settings(:notification_emails).send(@notification.type) + end +end diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb index 8daea1675..4b2fe3799 100644 --- a/app/services/process_feed_service.rb +++ b/app/services/process_feed_service.rb @@ -150,12 +150,10 @@ class ProcessFeedService < BaseService next if mentioned_account.nil? || processed_account_ids.include?(mentioned_account.id) - if mentioned_account.local? - # Send notifications - NotificationMailer.mention(mentioned_account, parent).deliver_later unless mentioned_account.blocking?(parent.account) - end + mention = mentioned_account.mentions.where(status: parent).first_or_create(status: parent) - mentioned_account.mentions.where(status: parent).first_or_create(status: parent) + # Notify local user + NotifyService.new.call(mentioned_account, mention) if mentioned_account.local? # So we can skip duplicate mentions processed_account_ids << mentioned_account.id diff --git a/app/services/process_interaction_service.rb b/app/services/process_interaction_service.rb index ecd3c2b2c..e7bb3c73b 100644 --- a/app/services/process_interaction_service.rb +++ b/app/services/process_interaction_service.rb @@ -65,8 +65,8 @@ class ProcessInteractionService < BaseService end def follow!(account, target_account) - account.follow!(target_account) - NotificationMailer.follow(target_account, account).deliver_later unless target_account.blocking?(account) + follow = account.follow!(target_account) + NotifyService.new.call(target_account, follow) end def unfollow!(account, target_account) @@ -83,8 +83,8 @@ class ProcessInteractionService < BaseService def favourite!(xml, from_account) current_status = status(xml) - current_status.favourites.where(account: from_account).first_or_create!(account: from_account) - NotificationMailer.favourite(current_status, from_account).deliver_later unless current_status.account.blocking?(from_account) + favourite = current_status.favourites.where(account: from_account).first_or_create!(account: from_account) + NotifyService.new.call(current_status.account, favourite) end def add_post!(body, account) diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index fd5a02ffe..98e48299e 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -29,7 +29,7 @@ class ProcessMentionsService < BaseService mentioned_account = mention.account if mentioned_account.local? - NotificationMailer.mention(mentioned_account, status).deliver_later unless mentioned_account.blocking?(status.account) + NotifyService.new.call(mentioned_account, mention) else NotificationWorker.perform_async(status.stream_entry.id, mentioned_account.id) end diff --git a/app/services/reblog_service.rb b/app/services/reblog_service.rb index 884d911a4..6543d4ae7 100644 --- a/app/services/reblog_service.rb +++ b/app/services/reblog_service.rb @@ -11,7 +11,7 @@ class ReblogService < BaseService HubPingWorker.perform_async(account.id) if reblogged_status.local? - NotificationMailer.reblog(reblogged_status, account).deliver_later unless reblogged_status.account.blocking?(account) + NotifyService.new.call(reblogged_status.account, reblog) else NotificationWorker.perform_async(reblog.stream_entry.id, reblogged_status.account_id) end diff --git a/app/views/api/v1/notifications/index.rabl b/app/views/api/v1/notifications/index.rabl new file mode 100644 index 000000000..6abc3da36 --- /dev/null +++ b/app/views/api/v1/notifications/index.rabl @@ -0,0 +1,2 @@ +collection @notifications +extends 'api/v1/notifications/show' diff --git a/app/views/api/v1/notifications/show.rabl b/app/views/api/v1/notifications/show.rabl new file mode 100644 index 000000000..fe2218ed7 --- /dev/null +++ b/app/views/api/v1/notifications/show.rabl @@ -0,0 +1,11 @@ +object @notification + +attributes :id, :type + +child from_account: :account do + extends 'api/v1/accounts/show' +end + +node(:status, if: lambda { |n| [:favourite, :reblog, :mention].include?(n.type) }) do |n| + partial 'api/v1/statuses/show', object: n.target_status +end diff --git a/config/routes.rb b/config/routes.rb index 176a4ccc8..00185c5e8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -74,6 +74,8 @@ Rails.application.routes.draw do resources :media, only: [:create] resources :apps, only: [:create] + resources :notifications, only: [:index] + resources :accounts, only: [:show] do collection do get :relationships diff --git a/db/migrate/20161119211120_create_notifications.rb b/db/migrate/20161119211120_create_notifications.rb new file mode 100644 index 000000000..dcbe51767 --- /dev/null +++ b/db/migrate/20161119211120_create_notifications.rb @@ -0,0 +1,13 @@ +class CreateNotifications < ActiveRecord::Migration[5.0] + def change + create_table :notifications do |t| + t.integer :account_id + t.integer :activity_id + t.string :activity_type + + t.timestamps + end + + add_index :notifications, :account_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 84f75c278..2bc9e5ee8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161116162355) do +ActiveRecord::Schema.define(version: 20161119211120) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -96,6 +96,15 @@ ActiveRecord::Schema.define(version: 20161116162355) do t.index ["account_id", "status_id"], name: "index_mentions_on_account_id_and_status_id", unique: true, using: :btree end + create_table "notifications", force: :cascade do |t| + t.integer "account_id" + t.integer "activity_id" + t.string "activity_type" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id"], name: "index_notifications_on_account_id", using: :btree + end + create_table "oauth_access_grants", force: :cascade do |t| t.integer "resource_owner_id", null: false t.integer "application_id", null: false diff --git a/spec/fabricators/notification_fabricator.rb b/spec/fabricators/notification_fabricator.rb new file mode 100644 index 000000000..08e984904 --- /dev/null +++ b/spec/fabricators/notification_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:notification) do + activity_id 1 + activity_type "MyString" +end diff --git a/spec/mailers/notification_mailer_spec.rb b/spec/mailers/notification_mailer_spec.rb index d7a956b75..d4baca5aa 100644 --- a/spec/mailers/notification_mailer_spec.rb +++ b/spec/mailers/notification_mailer_spec.rb @@ -7,7 +7,8 @@ RSpec.describe NotificationMailer, type: :mailer do let(:own_status) { Fabricate(:status, account: receiver.account) } describe "mention" do - let(:mail) { NotificationMailer.mention(receiver.account, foreign_status) } + let(:mention) { Mention.create!(account: receiver.account, status: foreign_status) } + let(:mail) { NotificationMailer.mention(receiver.account, Notification.create!(account: receiver.account, activity: mention)) } it "renders the headers" do expect(mail.subject).to eq("You were mentioned by bob") @@ -20,7 +21,8 @@ RSpec.describe NotificationMailer, type: :mailer do end describe "follow" do - let(:mail) { NotificationMailer.follow(receiver.account, sender) } + let(:follow) { sender.follow!(receiver.account) } + let(:mail) { NotificationMailer.follow(receiver.account, Notification.create!(account: receiver.account, activity: follow)) } it "renders the headers" do expect(mail.subject).to eq("bob is now following you") @@ -33,7 +35,8 @@ RSpec.describe NotificationMailer, type: :mailer do end describe "favourite" do - let(:mail) { NotificationMailer.favourite(own_status, sender) } + let(:favourite) { Favourite.create!(account: sender, status: own_status) } + let(:mail) { NotificationMailer.favourite(own_status.account, Notification.create!(account: receiver.account, activity: favourite)) } it "renders the headers" do expect(mail.subject).to eq("bob favourited your status") @@ -46,7 +49,8 @@ RSpec.describe NotificationMailer, type: :mailer do end describe "reblog" do - let(:mail) { NotificationMailer.reblog(own_status, sender) } + let(:reblog) { Status.create!(account: sender, reblog: own_status) } + let(:mail) { NotificationMailer.reblog(own_status.account, Notification.create!(account: receiver.account, activity: reblog)) } it "renders the headers" do expect(mail.subject).to eq("bob reblogged your status") diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb new file mode 100644 index 000000000..97e8095cd --- /dev/null +++ b/spec/models/notification_spec.rb @@ -0,0 +1,29 @@ +require 'rails_helper' + +RSpec.describe Notification, type: :model do + describe '#from_account' do + pending + end + + describe '#type' do + it 'returns :reblog for a Status' do + notification = Notification.new(activity: Status.new) + expect(notification.type).to eq :reblog + end + + it 'returns :mention for a Mention' do + notification = Notification.new(activity: Mention.new) + expect(notification.type).to eq :mention + end + + it 'returns :favourite for a Favourite' do + notification = Notification.new(activity: Favourite.new) + expect(notification.type).to eq :favourite + end + + it 'returns :follow for a Follow' do + notification = Notification.new(activity: Follow.new) + expect(notification.type).to eq :follow + end + end +end