Cascade mute/block destruction to notes (#193)

Some database systems can make better use of an index if the index
columns are ordered left-to-right in increasing specificity.  I'm not
sure if Postgres is one of those databases, but it also makes sense to
me to read an index as "ordered by type and then ID".

app/controllers/api/v1/accounts_controller.rb

@@ -21,12 +21,12 @@ 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
 
   def mute
-    MuteService.new.call(current_user.account, @account, notifications: params[:notifications])
+    MuteService.new.call(current_user.account, @account, notifications: params[:notifications], note: params[:note])
     render json: @account, serializer: REST::RelationshipSerializer, relationships: relationships
   end
 

app/models/account.rb

@@ -52,6 +52,7 @@ class Account < ApplicationRecord
   include AccountFinderConcern
   include AccountHeader
   include AccountInteractions
+  include Glitch::AccountInteractions
   include Attachmentable
   include Remotable
   include Paginable

app/models/account_domain_block.rb

@@ -19,6 +19,8 @@ class AccountDomainBlock < ApplicationRecord
   after_create  :remove_blocking_cache
   after_destroy :remove_blocking_cache
 
+  has_one :note, class_name: 'Glitch::Note', as: :target
+
   private
 
   def remove_blocking_cache

app/models/block.rb

@@ -21,6 +21,8 @@ class Block < ApplicationRecord
   after_create  :remove_blocking_cache
   after_destroy :remove_blocking_cache
 
+  has_one :note, class_name: 'Glitch::Note', as: :target, dependent: :destroy
+
   private
 
   def remove_blocking_cache

app/models/concerns/account_interactions.rb

@@ -82,19 +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
   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

app/models/concerns/glitch/account_interactions.rb

@@ -0,0 +1,25 @@
+# frozen_string_literals: true
+
+module Glitch::AccountInteractions
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def mute_note_map(target_account_ids, account_id)
+      notes_from_relationship(Mute, target_account_ids, account_id).each_with_object({}) do |mute, mapping|
+        mapping[mute.target_account_id] = mute.note&.text
+      end
+    end
+
+    def block_note_map(target_account_ids, account_id)
+      notes_from_relationship(Block, target_account_ids, account_id).each_with_object({}) do |block, mapping|
+        mapping[block.target_account_id] = block.note&.text
+      end
+    end
+
+    private
+
+    def notes_from_relationship(klass, target_account_ids, account_id)
+      klass.where(target_account_id: target_account_ids, account_id: account_id).includes(:note)
+    end
+  end
+end

app/models/glitch/note.rb

@@ -0,0 +1,19 @@
+# == Schema Information
+#
+# Table name: glitch_notes
+#
+#  id          :integer          not null, primary key
+#  target_id   :integer          not null
+#  target_type :string           not null
+#  note        :text             not null
+#  created_at  :datetime         not null
+#  updated_at  :datetime         not null
+#
+
+class Glitch::Note < ApplicationRecord
+  belongs_to :target, polymorphic: true
+
+  validates :note, presence: true
+
+  alias_attribute :text, :note
+end

app/models/mute.rb

@@ -22,6 +22,8 @@ class Mute < ApplicationRecord
   after_create  :remove_blocking_cache
   after_destroy :remove_blocking_cache
 
+  has_one :note, class_name: 'Glitch::Note', as: :target, dependent: :destroy
+
   private
 
   def remove_blocking_cache

app/presenters/account_relationships_presenter.rb

@@ -2,7 +2,8 @@
 
 class AccountRelationshipsPresenter
   attr_reader :following, :followed_by, :blocking,
-              :muting, :requested, :domain_blocking
+              :muting, :requested, :domain_blocking,
+              :mute_notes, :block_notes
 
   def initialize(account_ids, current_account_id, **options)
     @following       = Account.following_map(account_ids, current_account_id).merge(options[:following_map] || {})
@@ -11,5 +12,7 @@ class AccountRelationshipsPresenter
     @muting          = Account.muting_map(account_ids, current_account_id).merge(options[:muting_map] || {})
     @requested       = Account.requested_map(account_ids, current_account_id).merge(options[:requested_map] || {})
     @domain_blocking = Account.domain_blocking_map(account_ids, current_account_id).merge(options[:domain_blocking_map] || {})
+    @mute_notes      = Account.mute_note_map(account_ids, current_account_id).merge(options[:mute_note_map] || {})
+    @block_notes     = Account.block_note_map(account_ids, current_account_id).merge(options[:block_note_map] || {})
   end
 end

app/serializers/rest/relationship_serializer.rb

@@ -2,7 +2,8 @@
 
 class REST::RelationshipSerializer < ActiveModel::Serializer
   attributes :id, :following, :showing_reblogs, :followed_by, :blocking,
-             :muting, :muting_notifications, :requested, :domain_blocking
+             :muting, :muting_notifications, :requested, :domain_blocking,
+             :mute_notes, :block_notes
 
   def id
     object.id.to_s
@@ -34,6 +35,14 @@ class REST::RelationshipSerializer < ActiveModel::Serializer
     (instance_options[:relationships].muting[object.id] || {})[:notifications] || false
   end
 
+  def mute_notes
+    instance_options[:relationships].mute_notes[object.id] || ''
+  end
+
+  def block_notes
+    instance_options[:relationships].block_notes[object.id] || ''
+  end
+
   def requested
     instance_options[:relationships].requested[object.id] ? true : false
   end

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?

app/services/mute_service.rb

@@ -1,10 +1,13 @@
 # frozen_string_literal: true
 
 class MuteService < BaseService
-  def call(account, target_account, notifications: nil)
+  def call(account, target_account, notifications: nil, note: nil)
     return if account.id == target_account.id
-    mute = account.mute!(target_account, notifications: notifications)
-    BlockWorker.perform_async(account.id, target_account.id)
-    mute
+
+    ActiveRecord::Base.transaction do
+      mute = account.mute!(target_account, notifications: notifications, note: note)
+      BlockWorker.perform_async(account.id, target_account.id)
+      mute
+    end
   end
 end

db/migrate/20180118192721_create_glitch_notes.rb

@@ -0,0 +1,11 @@
+class CreateGlitchNotes < ActiveRecord::Migration[5.1]
+  def change
+    create_table :glitch_notes do |t|
+      t.bigint :target_id, null: false
+      t.string :target_type, null: false
+      t.text :note, null: false
+      t.index [:target_type, :target_id]
+      t.timestamps
+    end
+  end
+end

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: 20180106000232) do
+ActiveRecord::Schema.define(version: 20180118192721) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -182,6 +182,15 @@ ActiveRecord::Schema.define(version: 20180106000232) do
     t.index ["account_id"], name: "index_glitch_keyword_mutes_on_account_id"
   end
 
+  create_table "glitch_notes", force: :cascade do |t|
+    t.bigint "target_id", null: false
+    t.string "target_type", null: false
+    t.text "note", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["target_type", "target_id"], name: "index_glitch_notes_on_target_type_and_target_id"
+  end
+
   create_table "imports", force: :cascade do |t|
     t.integer "type", null: false
     t.boolean "approved", default: false, null: false

spec/fabricators/glitch_note_fabricator.rb

@@ -0,0 +1,2 @@
+Fabricator('Glitch::Note') do
+end

spec/models/block_spec.rb

@@ -44,4 +44,12 @@ RSpec.describe Block, type: :model do
     expect(Rails.cache.exist?("exclude_account_ids_for:#{account.id}")).to eq false
     expect(Rails.cache.exist?("exclude_account_ids_for:#{target_account.id}")).to eq false
   end
+
+  it 'removes associated note after destruction' do
+    block = Fabricate(:block)
+    block.create_note!(note: 'Too many oats')
+    block.destroy!
+
+    expect(Glitch::Note.count).to eq(0)
+  end
 end

spec/models/concerns/account_interactions_spec.rb

@@ -119,9 +119,9 @@ describe AccountInteractions do
       context 'arg :notifications is nil' do
         let(:arg_notifications) { nil }
 
-        it 'creates Mute, and returns nil' do
+        it 'creates and returns a Mute' do
           expect do
-            expect(account.mute!(target_account, notifications: arg_notifications)).to be nil
+            expect(account.mute!(target_account, notifications: arg_notifications)).to be_instance_of(Mute)
           end.to change { account.mute_relationships.count }.by 1
         end
       end
@@ -129,9 +129,9 @@ describe AccountInteractions do
       context 'arg :notifications is false' do
         let(:arg_notifications) { false }
 
-        it 'creates Mute, and returns nil' do
+        it 'creates and returns a Mute' do
           expect do
-            expect(account.mute!(target_account, notifications: arg_notifications)).to be nil
+            expect(account.mute!(target_account, notifications: arg_notifications)).to be_instance_of(Mute)
           end.to change { account.mute_relationships.count }.by 1
         end
       end
@@ -139,9 +139,9 @@ describe AccountInteractions do
       context 'arg :notifications is true' do
         let(:arg_notifications) { true }
 
-        it 'creates Mute, and returns nil' do
+        it 'creates and returns a Mute' do
           expect do
-            expect(account.mute!(target_account, notifications: arg_notifications)).to be nil
+            expect(account.mute!(target_account, notifications: arg_notifications)).to be_instance_of(Mute)
           end.to change { account.mute_relationships.count }.by 1
         end
       end

spec/models/concerns/glitch/account_interactions_spec.rb

@@ -0,0 +1,78 @@
+require 'rails_helper'
+
+describe Glitch::AccountInteractions do
+  let(:account)            { Fabricate(:account, username: 'account') }
+  let(:account_id)         { account.id }
+  let(:account_ids)        { [account_id] }
+  let(:target_account)     { Fabricate(:account, username: 'target') }
+  let(:target_account_id)  { target_account.id }
+  let(:target_account_ids) { [target_account_id] }
+  
+  describe '.mute_note_map' do
+    subject { Account.mute_note_map(target_account_ids, account_id) }
+
+    context 'account with Mute' do
+      before do
+        Fabricate(:mute, target_account: target_account, account: account).tap do |m|
+          m.create_note!(note: note) if note
+        end
+      end
+
+      context 'with a note' do
+        let(:note) { 'Too many jorts' }
+
+        it 'returns { target_account_id => "(a note)" }' do
+          is_expected.to eq(target_account_id => 'Too many jorts')
+        end
+      end
+
+      context 'with no associated note' do
+        let(:note) { nil }
+
+        it 'returns { target_account_id => nil }' do
+          is_expected.to eq(target_account_id => nil)
+        end
+      end
+    end
+
+    context 'account without Mute' do
+      it 'returns {}' do
+        is_expected.to eq({})
+      end
+    end
+  end
+
+  describe '.block_note_map' do
+    subject { Account.block_note_map(target_account_ids, account_id) }
+
+    context 'account with Block' do
+      before do
+        Fabricate(:block, target_account: target_account, account: account).tap do |m|
+          m.create_note!(note: note) if note
+        end
+      end
+
+      context 'with a note' do
+        let(:note) { 'Too many oats' }
+
+        it 'returns { target_account_id => "(a note)" }' do
+          is_expected.to eq(target_account_id => 'Too many oats')
+        end
+      end
+
+      context 'with no associated note' do
+        let(:note) { nil }
+
+        it 'returns { target_account_id => nil }' do
+          is_expected.to eq(target_account_id => nil)
+        end
+      end
+    end
+
+    context 'account without Block' do
+      it 'returns {}' do
+        is_expected.to eq({})
+      end
+    end
+  end
+end

spec/models/glitch/note_spec.rb

@@ -0,0 +1,5 @@
+require 'rails_helper'
+
+RSpec.describe Glitch::Note, type: :model do
+  pending "add some examples to (or delete) #{__FILE__}"
+end

spec/models/mute_spec.rb

@@ -1,5 +1,14 @@
 require 'rails_helper'
 
 RSpec.describe Mute, type: :model do
+  subject { Fabricate(:mute) }
 
+  describe '#destroy' do
+    it 'destroys associated notes' do
+      subject.create_note!(note: 'Too many jorts')
+      subject.destroy!
+
+      expect(Glitch::Note.count).to eq(0)
+    end
+  end
 end

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

spec/services/mute_service_spec.rb

@@ -64,4 +64,18 @@ RSpec.describe MuteService do
       }.from(false)
     end
   end
+
+  context 'with a note string' do
+    it 'adds a note to the mute' do
+      mute = described_class.new.call(account, target_account, note: 'Too many jorts')
+      note = mute.note
+      expect(note.text).to eq('Too many jorts')
+    end
+
+    it 'does not add a note if the note is blank' do
+      mute = described_class.new.call(account, target_account, note: ' ')
+      note = mute.note
+      expect(note).to be_nil
+    end
+  end
 end