Optimize the process of following someone (#9220)

* Eliminate extra accounts select query from FollowService

* Optimistically update follow state in web UI and hide loading bar

Fix #6205

* Asynchronize NotifyService in FollowService

And fix failing test

* Skip Webfinger resolve routine when called from FollowService if possible

If an account is ActivityPub, then webfinger re-resolving is not necessary
when called from FollowService. Improve options of ResolveAccountService
This commit is contained in:
Eugen Rochko 2018-11-08 21:05:42 +01:00 committed by GitHub
parent 9cfd610484
commit 6d59dfa15d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 67 additions and 26 deletions

View File

@ -17,7 +17,7 @@ class Api::V1::AccountsController < Api::BaseController
end end
def follow def follow
FollowService.new.call(current_user.account, @account.acct, reblogs: truthy_param?(:reblogs)) FollowService.new.call(current_user.account, @account, reblogs: truthy_param?(:reblogs))
options = @account.locked? ? {} : { following_map: { @account.id => { reblogs: truthy_param?(:reblogs) } }, requested_map: { @account.id => false } } options = @account.locked? ? {} : { following_map: { @account.id => { reblogs: truthy_param?(:reblogs) } }, requested_map: { @account.id => false } }

View File

@ -145,12 +145,14 @@ export function fetchAccountFail(id, error) {
export function followAccount(id, reblogs = true) { export function followAccount(id, reblogs = true) {
return (dispatch, getState) => { return (dispatch, getState) => {
const alreadyFollowing = getState().getIn(['relationships', id, 'following']); const alreadyFollowing = getState().getIn(['relationships', id, 'following']);
dispatch(followAccountRequest(id)); const locked = getState().getIn(['accounts', id, 'locked'], false);
dispatch(followAccountRequest(id, locked));
api(getState).post(`/api/v1/accounts/${id}/follow`, { reblogs }).then(response => { api(getState).post(`/api/v1/accounts/${id}/follow`, { reblogs }).then(response => {
dispatch(followAccountSuccess(response.data, alreadyFollowing)); dispatch(followAccountSuccess(response.data, alreadyFollowing));
}).catch(error => { }).catch(error => {
dispatch(followAccountFail(error)); dispatch(followAccountFail(error, locked));
}); });
}; };
}; };
@ -167,10 +169,12 @@ export function unfollowAccount(id) {
}; };
}; };
export function followAccountRequest(id) { export function followAccountRequest(id, locked) {
return { return {
type: ACCOUNT_FOLLOW_REQUEST, type: ACCOUNT_FOLLOW_REQUEST,
id, id,
locked,
skipLoading: true,
}; };
}; };
@ -179,13 +183,16 @@ export function followAccountSuccess(relationship, alreadyFollowing) {
type: ACCOUNT_FOLLOW_SUCCESS, type: ACCOUNT_FOLLOW_SUCCESS,
relationship, relationship,
alreadyFollowing, alreadyFollowing,
skipLoading: true,
}; };
}; };
export function followAccountFail(error) { export function followAccountFail(error, locked) {
return { return {
type: ACCOUNT_FOLLOW_FAIL, type: ACCOUNT_FOLLOW_FAIL,
error, error,
locked,
skipLoading: true,
}; };
}; };
@ -193,6 +200,7 @@ export function unfollowAccountRequest(id) {
return { return {
type: ACCOUNT_UNFOLLOW_REQUEST, type: ACCOUNT_UNFOLLOW_REQUEST,
id, id,
skipLoading: true,
}; };
}; };
@ -201,6 +209,7 @@ export function unfollowAccountSuccess(relationship, statuses) {
type: ACCOUNT_UNFOLLOW_SUCCESS, type: ACCOUNT_UNFOLLOW_SUCCESS,
relationship, relationship,
statuses, statuses,
skipLoading: true,
}; };
}; };
@ -208,6 +217,7 @@ export function unfollowAccountFail(error) {
return { return {
type: ACCOUNT_UNFOLLOW_FAIL, type: ACCOUNT_UNFOLLOW_FAIL,
error, error,
skipLoading: true,
}; };
}; };

View File

@ -1,6 +1,10 @@
import { import {
ACCOUNT_FOLLOW_SUCCESS, ACCOUNT_FOLLOW_SUCCESS,
ACCOUNT_FOLLOW_REQUEST,
ACCOUNT_FOLLOW_FAIL,
ACCOUNT_UNFOLLOW_SUCCESS, ACCOUNT_UNFOLLOW_SUCCESS,
ACCOUNT_UNFOLLOW_REQUEST,
ACCOUNT_UNFOLLOW_FAIL,
ACCOUNT_BLOCK_SUCCESS, ACCOUNT_BLOCK_SUCCESS,
ACCOUNT_UNBLOCK_SUCCESS, ACCOUNT_UNBLOCK_SUCCESS,
ACCOUNT_MUTE_SUCCESS, ACCOUNT_MUTE_SUCCESS,
@ -37,6 +41,14 @@ const initialState = ImmutableMap();
export default function relationships(state = initialState, action) { export default function relationships(state = initialState, action) {
switch(action.type) { switch(action.type) {
case ACCOUNT_FOLLOW_REQUEST:
return state.setIn([action.id, action.locked ? 'requested' : 'following'], true);
case ACCOUNT_FOLLOW_FAIL:
return state.setIn([action.id, action.locked ? 'requested' : 'following'], false);
case ACCOUNT_UNFOLLOW_REQUEST:
return state.setIn([action.id, 'following'], false);
case ACCOUNT_UNFOLLOW_FAIL:
return state.setIn([action.id, 'following'], true);
case ACCOUNT_FOLLOW_SUCCESS: case ACCOUNT_FOLLOW_SUCCESS:
case ACCOUNT_UNFOLLOW_SUCCESS: case ACCOUNT_UNFOLLOW_SUCCESS:
case ACCOUNT_BLOCK_SUCCESS: case ACCOUNT_BLOCK_SUCCESS:

View File

@ -18,6 +18,6 @@ module AuthorExtractor
acct = "#{username}@#{domain}" acct = "#{username}@#{domain}"
end end
ResolveAccountService.new.call(acct, update_profile) ResolveAccountService.new.call(acct, update_profile: update_profile)
end end
end end

View File

@ -7,9 +7,9 @@ class FollowService < BaseService
# @param [Account] source_account From which to follow # @param [Account] source_account From which to follow
# @param [String, Account] uri User URI to follow in the form of username@domain (or account record) # @param [String, Account] uri User URI to follow in the form of username@domain (or account record)
# @param [true, false, nil] reblogs Whether or not to show reblogs, defaults to true # @param [true, false, nil] reblogs Whether or not to show reblogs, defaults to true
def call(source_account, uri, reblogs: nil) def call(source_account, target_account, reblogs: nil)
reblogs = true if reblogs.nil? reblogs = true if reblogs.nil?
target_account = uri.is_a?(Account) ? uri : ResolveAccountService.new.call(uri) target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true)
raise ActiveRecord::RecordNotFound if target_account.nil? || target_account.id == source_account.id || target_account.suspended? raise ActiveRecord::RecordNotFound if target_account.nil? || target_account.id == source_account.id || target_account.suspended?
raise Mastodon::NotPermittedError if target_account.blocking?(source_account) || source_account.blocking?(target_account) raise Mastodon::NotPermittedError if target_account.blocking?(source_account) || source_account.blocking?(target_account)
@ -42,7 +42,7 @@ class FollowService < BaseService
follow_request = FollowRequest.create!(account: source_account, target_account: target_account, show_reblogs: reblogs) follow_request = FollowRequest.create!(account: source_account, target_account: target_account, show_reblogs: reblogs)
if target_account.local? if target_account.local?
NotifyService.new.call(target_account, follow_request) LocalNotificationWorker.perform_async(target_account.id, follow_request.id, follow_request.class.name)
elsif target_account.ostatus? elsif target_account.ostatus?
NotificationWorker.perform_async(build_follow_request_xml(follow_request), source_account.id, target_account.id) NotificationWorker.perform_async(build_follow_request_xml(follow_request), source_account.id, target_account.id)
AfterRemoteFollowRequestWorker.perform_async(follow_request.id) AfterRemoteFollowRequestWorker.perform_async(follow_request.id)
@ -57,7 +57,7 @@ class FollowService < BaseService
follow = source_account.follow!(target_account, reblogs: reblogs) follow = source_account.follow!(target_account, reblogs: reblogs)
if target_account.local? if target_account.local?
NotifyService.new.call(target_account, follow) LocalNotificationWorker.perform_async(target_account.id, follow.id, follow.class.name)
else else
Pubsubhubbub::SubscribeWorker.perform_async(target_account.id) unless target_account.subscribed? Pubsubhubbub::SubscribeWorker.perform_async(target_account.id) unless target_account.subscribed?
NotificationWorker.perform_async(build_follow_xml(follow), source_account.id, target_account.id) NotificationWorker.perform_async(build_follow_xml(follow), source_account.id, target_account.id)

View File

@ -47,7 +47,7 @@ class ProcessMentionsService < BaseService
mentioned_account = mention.account mentioned_account = mention.account
if mentioned_account.local? if mentioned_account.local?
LocalNotificationWorker.perform_async(mention.id) LocalNotificationWorker.perform_async(mentioned_account.id, mention.id, mention.class.name)
elsif mentioned_account.ostatus? && !@status.stream_entry.hidden? elsif mentioned_account.ostatus? && !@status.stream_entry.hidden?
NotificationWorker.perform_async(ostatus_xml, @status.account_id, mentioned_account.id) NotificationWorker.perform_async(ostatus_xml, @status.account_id, mentioned_account.id)
elsif mentioned_account.activitypub? elsif mentioned_account.activitypub?

View File

@ -9,17 +9,27 @@ class ResolveAccountService < BaseService
# Find or create a local account for a remote user. # Find or create a local account for a remote user.
# When creating, look up the user's webfinger and fetch all # When creating, look up the user's webfinger and fetch all
# important information from their feed # important information from their feed
# @param [String] uri User URI in the form of username@domain # @param [String, Account] uri User URI in the form of username@domain
# @param [Hash] options
# @return [Account] # @return [Account]
def call(uri, update_profile = true, redirected = nil) def call(uri, options = {})
@username, @domain = uri.split('@') @options = options
@update_profile = update_profile
return Account.find_local(@username) if TagManager.instance.local_domain?(@domain) if uri.is_a?(Account)
@account = uri
@username = @account.username
@domain = @account.domain
@account = Account.find_remote(@username, @domain) return @account if @account.local? || !webfinger_update_due?
else
@username, @domain = uri.split('@')
return @account unless webfinger_update_due? return Account.find_local(@username) if TagManager.instance.local_domain?(@domain)
@account = Account.find_remote(@username, @domain)
return @account unless webfinger_update_due?
end
Rails.logger.debug "Looking up webfinger for #{uri}" Rails.logger.debug "Looking up webfinger for #{uri}"
@ -30,8 +40,8 @@ class ResolveAccountService < BaseService
if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
@username = confirmed_username @username = confirmed_username
@domain = confirmed_domain @domain = confirmed_domain
elsif redirected.nil? elsif options[:redirected].nil?
return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) return call("#{confirmed_username}@#{confirmed_domain}", options.merge(redirected: true))
else else
Rails.logger.debug 'Requested and returned acct URIs do not match' Rails.logger.debug 'Requested and returned acct URIs do not match'
return return
@ -76,7 +86,7 @@ class ResolveAccountService < BaseService
end end
def webfinger_update_due? def webfinger_update_due?
@account.nil? || @account.possibly_stale? @account.nil? || ((!@options[:skip_webfinger] || @account.ostatus?) && @account.possibly_stale?)
end end
def activitypub_ready? def activitypub_ready?
@ -93,7 +103,7 @@ class ResolveAccountService < BaseService
end end
def update_profile? def update_profile?
@update_profile @options[:update_profile]
end end
def handle_activitypub def handle_activitypub

View File

@ -3,9 +3,16 @@
class LocalNotificationWorker class LocalNotificationWorker
include Sidekiq::Worker include Sidekiq::Worker
def perform(mention_id) def perform(receiver_account_id, activity_id = nil, activity_class_name = nil)
mention = Mention.find(mention_id) if activity_id.nil? && activity_class_name.nil?
NotifyService.new.call(mention.account, mention) activity = Mention.find(receiver_account_id)
receiver = activity.account
else
receiver = Account.find(receiver_account_id)
activity = activity_class_name.constantize.find(activity_id)
end
NotifyService.new.call(receiver, activity)
rescue ActiveRecord::RecordNotFound rescue ActiveRecord::RecordNotFound
true true
end end

View File

@ -99,10 +99,12 @@ describe AuthorizeInteractionsController do
allow(ResolveAccountService).to receive(:new).and_return(service) allow(ResolveAccountService).to receive(:new).and_return(service)
allow(service).to receive(:call).with('user@hostname').and_return(target_account) allow(service).to receive(:call).with('user@hostname').and_return(target_account)
allow(service).to receive(:call).with(target_account, skip_webfinger: true).and_return(target_account)
post :create, params: { acct: 'acct:user@hostname' } post :create, params: { acct: 'acct:user@hostname' }
expect(service).to have_received(:call).with('user@hostname') expect(service).to have_received(:call).with(target_account, skip_webfinger: true)
expect(account.following?(target_account)).to be true expect(account.following?(target_account)).to be true
expect(response).to render_template(:success) expect(response).to render_template(:success)
end end