From 5e6a09498a6620d8885da7a4ab52aff11f54e4db Mon Sep 17 00:00:00 2001 From: David Yip Date: Fri, 26 Jan 2018 04:03:02 -0600 Subject: [PATCH] Add notes to blocks (#193) In this commit, we also push note creation into the AccountInteractions concern, where we can: 1. create and persist model objects inside transactions without having to include job queuing inside the transaction 2. commonize the don't-add-a-blank-note check Finally, this commit resolves a invocation-on-nil in REST::RelationshipSerializer. --- app/controllers/api/v1/accounts_controller.rb | 2 +- app/models/concerns/account_interactions.rb | 41 +++++++++++++------ .../rest/relationship_serializer.rb | 2 +- app/services/block_service.rb | 4 +- app/services/mute_service.rb | 3 +- spec/services/block_service_spec.rb | 17 ++++++++ 6 files changed, 51 insertions(+), 18 deletions(-) diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 1929f4a8be..353f40b9db 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -21,7 +21,7 @@ class Api::V1::AccountsController < Api::BaseController end def block - BlockService.new.call(current_user.account, @account) + BlockService.new.call(current_user.account, @account, note: params[:note]) render json: @account, serializer: REST::RelationshipSerializer, relationships: relationships end diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index 8aee103729..caaa7490c6 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -82,21 +82,38 @@ module AccountInteractions rel end - def block!(other_account) - block_relationships.find_or_create_by!(target_account: other_account) - end - - def mute!(other_account, notifications: nil) - notifications = true if notifications.nil? - mute = mute_relationships.create_with(hide_notifications: notifications).find_or_create_by!(target_account: other_account) - # When toggling a mute between hiding and allowing notifications, the mute will already exist, so the find_or_create_by! call will return the existing Mute without updating the hide_notifications attribute. Therefore, we check that hide_notifications? is what we want and set it if it isn't. - if mute.hide_notifications? != notifications - mute.update!(hide_notifications: notifications) + def block!(other_account, note: nil) + transaction do + block = block_relationships.find_or_create_by!(target_account: other_account) + attach_note(block, note) + block end - - mute end + def mute!(other_account, notifications: nil, note: nil) + notifications = true if notifications.nil? + + transaction do + mute = mute_relationships.create_with(hide_notifications: notifications).find_or_create_by!(target_account: other_account) + attach_note(mute, note) + + # When toggling a mute between hiding and allowing notifications, the mute will already exist, so the find_or_create_by! call will return the existing Mute without updating the hide_notifications attribute. Therefore, we check that hide_notifications? is what we want and set it if it isn't. + if mute.hide_notifications? != notifications + mute.update!(hide_notifications: notifications) + end + + mute + end + end + + def attach_note(object, note) + return if note.blank? + + object.create_note!(note: note) + end + + private :attach_note + def mute_conversation!(conversation) conversation_mutes.find_or_create_by!(conversation: conversation) end diff --git a/app/serializers/rest/relationship_serializer.rb b/app/serializers/rest/relationship_serializer.rb index c805d6db7e..257fac4f78 100644 --- a/app/serializers/rest/relationship_serializer.rb +++ b/app/serializers/rest/relationship_serializer.rb @@ -10,7 +10,7 @@ class REST::RelationshipSerializer < ActiveModel::Serializer end def note - object.respond_to?(:note) ? object.note.text : nil + object.respond_to?(:note) ? object.note&.text : nil end def following diff --git a/app/services/block_service.rb b/app/services/block_service.rb index b39c3eef28..6808f71b3e 100644 --- a/app/services/block_service.rb +++ b/app/services/block_service.rb @@ -3,13 +3,13 @@ class BlockService < BaseService include StreamEntryRenderer - def call(account, target_account) + def call(account, target_account, note: nil) return if account.id == target_account.id UnfollowService.new.call(account, target_account) if account.following?(target_account) UnfollowService.new.call(target_account, account) if target_account.following?(account) - block = account.block!(target_account) + block = account.block!(target_account, note: note) BlockWorker.perform_async(account.id, target_account.id) create_notification(block) unless target_account.local? diff --git a/app/services/mute_service.rb b/app/services/mute_service.rb index fdd2c7a6a0..54c159e587 100644 --- a/app/services/mute_service.rb +++ b/app/services/mute_service.rb @@ -5,8 +5,7 @@ class MuteService < BaseService return if account.id == target_account.id ActiveRecord::Base.transaction do - mute = account.mute!(target_account, notifications: notifications) - mute.create_note!(note: note) if !note.blank? + mute = account.mute!(target_account, notifications: notifications, note: note) BlockWorker.perform_async(account.id, target_account.id) mute end diff --git a/spec/services/block_service_spec.rb b/spec/services/block_service_spec.rb index c69ff78047..05bf074e03 100644 --- a/spec/services/block_service_spec.rb +++ b/spec/services/block_service_spec.rb @@ -15,6 +15,23 @@ RSpec.describe BlockService do it 'creates a blocking relation' do expect(sender.blocking?(bob)).to be true end + + describe 'if a note is present' do + let(:note) { 'Too many oats' } + + it 'adds a note to the block' do + block = subject.call(sender, bob, note: note) + note = block.note + + expect(note.text).to eq('Too many oats') + end + + it 'does not add blank notes' do + block = subject.call(sender, bob, note: ' ') + + expect(block.note).to be nil + end + end end describe 'remote OStatus' do