From 49445150202f0bdaae942b9ae1ba44802a1c22e9 Mon Sep 17 00:00:00 2001
From: aschmitz <aschmitz@lardbucket.org>
Date: Thu, 9 Nov 2017 08:41:10 -0600
Subject: "Show reblogs" per-follower UI/database changes

TODO:

* Tests (particularly for FollowRequests).
* Anything to respect the setting when putting reblogs in timelines.
---
 app/models/concerns/account_interactions.rb | 23 +++++++++++++++++++----
 app/models/follow.rb                        |  1 +
 app/models/follow_request.rb                |  3 ++-
 3 files changed, 22 insertions(+), 5 deletions(-)

(limited to 'app/models')

diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb
index 0afdebf89..088fef4da 100644
--- a/app/models/concerns/account_interactions.rb
+++ b/app/models/concerns/account_interactions.rb
@@ -5,7 +5,11 @@ module AccountInteractions
 
   class_methods do
     def following_map(target_account_ids, account_id)
-      follow_mapping(Follow.where(target_account_id: target_account_ids, account_id: account_id), :target_account_id)
+      Follow.where(target_account_id: target_account_ids, account_id: account_id).each_with_object({}) do |follow, mapping|
+        mapping[follow.target_account_id] = {
+          reblogs: follow.show_reblogs?
+        }
+      end
     end
 
     def followed_by_map(target_account_ids, account_id)
@@ -25,7 +29,11 @@ module AccountInteractions
     end
 
     def requested_map(target_account_ids, account_id)
-      follow_mapping(FollowRequest.where(target_account_id: target_account_ids, account_id: account_id), :target_account_id)
+      FollowRequest.where(target_account_id: target_account_ids, account_id: account_id).each_with_object({}) do |follow_request, mapping|
+        mapping[follow_request.target_account_id] = {
+          reblogs: follow_request.show_reblogs?
+        }
+      end
     end
 
     def domain_blocking_map(target_account_ids, account_id)
@@ -66,8 +74,15 @@ module AccountInteractions
     has_many :domain_blocks, class_name: 'AccountDomainBlock', dependent: :destroy
   end
 
-  def follow!(other_account)
-    active_relationships.find_or_create_by!(target_account: other_account)
+  def follow!(other_account, reblogs: nil)
+    reblogs = true if reblogs.nil?
+    rel = active_relationships.create_with(show_reblogs: reblogs).find_or_create_by!(target_account: other_account)
+    if rel.show_reblogs != reblogs
+      rel.show_reblogs = reblogs
+      rel.save!
+    end
+    
+    rel
   end
 
   def block!(other_account)
diff --git a/app/models/follow.rb b/app/models/follow.rb
index 667720a88..a8ddcb7f0 100644
--- a/app/models/follow.rb
+++ b/app/models/follow.rb
@@ -8,6 +8,7 @@
 #  account_id        :integer          not null
 #  id                :integer          not null, primary key
 #  target_account_id :integer          not null
+#  show_reblogs      :boolean          default(TRUE), not null
 #
 
 class Follow < ApplicationRecord
diff --git a/app/models/follow_request.rb b/app/models/follow_request.rb
index 60036d903..0608ffabc 100644
--- a/app/models/follow_request.rb
+++ b/app/models/follow_request.rb
@@ -8,6 +8,7 @@
 #  account_id        :integer          not null
 #  id                :integer          not null, primary key
 #  target_account_id :integer          not null
+#  show_reblogs      :boolean          default(TRUE), not null
 #
 
 class FollowRequest < ApplicationRecord
@@ -21,7 +22,7 @@ class FollowRequest < ApplicationRecord
   validates :account_id, uniqueness: { scope: :target_account_id }
 
   def authorize!
-    account.follow!(target_account)
+    account.follow!(target_account, reblogs: reblogs)
     MergeWorker.perform_async(target_account.id, account.id)
 
     destroy!
-- 
cgit 


From b95c48748cd4e7a1181cdf3f17e23d6e526a9d95 Mon Sep 17 00:00:00 2001
From: aschmitz <aschmitz@lardbucket.org>
Date: Fri, 10 Nov 2017 20:11:10 -0600
Subject: Per-user reblog hiding implementation/fixes/tests

Note that this will only hide/show *future* reblogs by a user, and does
nothing to remove/add reblogs that are already in the timeline. I don't
think that's a particularly confusing behavior, and it's a lot easier
to implement (similar to mutes, I believe).
---
 app/models/concerns/account_interactions.rb        |  6 ++-
 app/models/follow_request.rb                       |  2 +-
 app/services/follow_service.rb                     |  2 +-
 app/services/notify_service.rb                     |  2 +-
 .../20171028221157_add_reblogs_to_follows.rb       | 21 ++++++++
 .../v1/accounts/relationships_controller_spec.rb   |  4 +-
 .../controllers/api/v1/accounts_controller_spec.rb |  8 +--
 spec/models/concerns/account_interactions_spec.rb  | 37 ++++++++++++++
 spec/models/follow_request_spec.rb                 | 24 ++++++++-
 spec/services/follow_service_spec.rb               | 58 ++++++++++++++++++++--
 spec/services/notify_service_spec.rb               | 20 ++++++++
 11 files changed, 170 insertions(+), 14 deletions(-)
 create mode 100644 db/migrate/20171028221157_add_reblogs_to_follows.rb

(limited to 'app/models')

diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb
index 088fef4da..60fd6ded5 100644
--- a/app/models/concerns/account_interactions.rb
+++ b/app/models/concerns/account_interactions.rb
@@ -81,7 +81,7 @@ module AccountInteractions
       rel.show_reblogs = reblogs
       rel.save!
     end
-    
+
     rel
   end
 
@@ -156,6 +156,10 @@ module AccountInteractions
     mute_relationships.where(target_account: other_account, hide_notifications: true).exists?
   end
 
+  def muting_reblogs?(other_account)
+    active_relationships.where(target_account: other_account, show_reblogs: false).exists?
+  end
+
   def requested?(other_account)
     follow_requests.where(target_account: other_account).exists?
   end
diff --git a/app/models/follow_request.rb b/app/models/follow_request.rb
index 0608ffabc..1a1c52382 100644
--- a/app/models/follow_request.rb
+++ b/app/models/follow_request.rb
@@ -22,7 +22,7 @@ class FollowRequest < ApplicationRecord
   validates :account_id, uniqueness: { scope: :target_account_id }
 
   def authorize!
-    account.follow!(target_account, reblogs: reblogs)
+    account.follow!(target_account, reblogs: show_reblogs)
     MergeWorker.perform_async(target_account.id, account.id)
 
     destroy!
diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb
index 70572110d..6db591999 100644
--- a/app/services/follow_service.rb
+++ b/app/services/follow_service.rb
@@ -39,7 +39,7 @@ class FollowService < BaseService
   private
 
   def request_follow(source_account, target_account, reblogs: true)
-    follow_request = FollowRequest.create!(account: source_account, target_account: target_account, reblogs: reblogs)
+    follow_request = FollowRequest.create!(account: source_account, target_account: target_account, show_reblogs: reblogs)
 
     if target_account.local?
       NotifyService.new.call(target_account, follow_request)
diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb
index fb09df983..3fa3f152c 100644
--- a/app/services/notify_service.rb
+++ b/app/services/notify_service.rb
@@ -29,7 +29,7 @@ class NotifyService < BaseService
   end
 
   def blocked_reblog?
-    false
+    @recipient.muting_reblogs?(@notification.from_account)
   end
 
   def blocked_follow_request?
diff --git a/db/migrate/20171028221157_add_reblogs_to_follows.rb b/db/migrate/20171028221157_add_reblogs_to_follows.rb
new file mode 100644
index 000000000..eb4640a20
--- /dev/null
+++ b/db/migrate/20171028221157_add_reblogs_to_follows.rb
@@ -0,0 +1,21 @@
+require Rails.root.join('lib', 'mastodon', 'migration_helpers')
+
+class AddReblogsToFollows < ActiveRecord::Migration[5.1]
+  include Mastodon::MigrationHelpers
+
+  safety_assured do
+    disable_ddl_transaction!
+  end
+
+  def up
+    safety_assured do
+      add_column_with_default :follows, :show_reblogs, :boolean, default: true, allow_null: false
+      add_column_with_default :follow_requests, :show_reblogs, :boolean, default: true, allow_null: false
+    end
+  end
+  
+  def down
+    remove_column :follows, :show_reblogs
+    remove_column :follow_requests, :show_reblogs
+  end
+end
diff --git a/spec/controllers/api/v1/accounts/relationships_controller_spec.rb b/spec/controllers/api/v1/accounts/relationships_controller_spec.rb
index 431fc2194..f25b86ac1 100644
--- a/spec/controllers/api/v1/accounts/relationships_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts/relationships_controller_spec.rb
@@ -32,7 +32,7 @@ describe Api::V1::Accounts::RelationshipsController do
         json = body_as_json
 
         expect(json).to be_a Enumerable
-        expect(json.first[:following]).to be true
+        expect(json.first[:following]).to be_truthy
         expect(json.first[:followed_by]).to be false
       end
     end
@@ -51,7 +51,7 @@ describe Api::V1::Accounts::RelationshipsController do
 
         expect(json).to be_a Enumerable
         expect(json.first[:id]).to eq simon.id.to_s
-        expect(json.first[:following]).to be true
+        expect(json.first[:following]).to be_truthy
         expect(json.first[:followed_by]).to be false
         expect(json.first[:muting]).to be false
         expect(json.first[:requested]).to be false
diff --git a/spec/controllers/api/v1/accounts_controller_spec.rb b/spec/controllers/api/v1/accounts_controller_spec.rb
index 053c53e5a..f3b879421 100644
--- a/spec/controllers/api/v1/accounts_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts_controller_spec.rb
@@ -31,10 +31,10 @@ RSpec.describe Api::V1::AccountsController, type: :controller do
         expect(response).to have_http_status(:success)
       end
 
-      it 'returns JSON with following=true and requested=false' do
+      it 'returns JSON with following=truthy and requested=false' do
         json = body_as_json
 
-        expect(json[:following]).to be true
+        expect(json[:following]).to be_truthy
         expect(json[:requested]).to be false
       end
 
@@ -50,11 +50,11 @@ RSpec.describe Api::V1::AccountsController, type: :controller do
         expect(response).to have_http_status(:success)
       end
 
-      it 'returns JSON with following=false and requested=true' do
+      it 'returns JSON with following=false and requested=truthy' do
         json = body_as_json
 
         expect(json[:following]).to be false
-        expect(json[:requested]).to be true
+        expect(json[:requested]).to be_truthy
       end
 
       it 'creates a follow request relation between user and target user' do
diff --git a/spec/models/concerns/account_interactions_spec.rb b/spec/models/concerns/account_interactions_spec.rb
index ef957fc1d..f47d9d057 100644
--- a/spec/models/concerns/account_interactions_spec.rb
+++ b/spec/models/concerns/account_interactions_spec.rb
@@ -37,4 +37,41 @@ describe AccountInteractions do
       end
     end
   end
+
+  describe 'ignoring reblogs from an account' do
+    before do
+      @me = Fabricate(:account, username: 'Me')
+      @you = Fabricate(:account, username: 'You')
+    end
+
+    context 'with the reblogs option unspecified' do
+      before do
+        @me.follow!(@you)
+      end
+
+      it 'defaults to showing reblogs' do
+        expect(@me.muting_reblogs?(@you)).to be(false)
+      end
+    end
+
+    context 'with the reblogs option set to false' do
+      before do
+        @me.follow!(@you, reblogs: false)
+      end
+
+      it 'does mute reblogs' do
+        expect(@me.muting_reblogs?(@you)).to be(true)
+      end
+    end
+
+    context 'with the reblogs option set to true' do
+      before do
+        @me.follow!(@you, reblogs: true)
+      end
+
+      it 'does not mute reblogs' do
+        expect(@me.muting_reblogs?(@you)).to be(false)
+      end
+    end
+  end
 end
diff --git a/spec/models/follow_request_spec.rb b/spec/models/follow_request_spec.rb
index cc6f8ee62..62bd724d7 100644
--- a/spec/models/follow_request_spec.rb
+++ b/spec/models/follow_request_spec.rb
@@ -1,7 +1,29 @@
 require 'rails_helper'
 
 RSpec.describe FollowRequest, type: :model do
-  describe '#authorize!'
+  describe '#authorize!' do
+    it 'generates a Follow' do
+      follow_request = Fabricate.create(:follow_request)
+      follow_request.authorize!
+      target = follow_request.target_account
+      expect(follow_request.account.following?(target)).to be true
+    end
+
+    it 'correctly passes show_reblogs when true' do
+      follow_request = Fabricate.create(:follow_request, show_reblogs: true)
+      follow_request.authorize!
+      target = follow_request.target_account
+      expect(follow_request.account.muting_reblogs?(target)).to be false
+    end
+
+    it 'correctly passes show_reblogs when false' do
+      follow_request = Fabricate.create(:follow_request, show_reblogs: false)
+      follow_request.authorize!
+      target = follow_request.target_account
+      expect(follow_request.account.muting_reblogs?(target)).to be true
+    end
+  end
+
   describe '#reject!'
 
   describe 'validations' do
diff --git a/spec/services/follow_service_spec.rb b/spec/services/follow_service_spec.rb
index ceb39e5e6..e59a2f1a6 100644
--- a/spec/services/follow_service_spec.rb
+++ b/spec/services/follow_service_spec.rb
@@ -13,8 +13,20 @@ RSpec.describe FollowService do
         subject.call(sender, bob.acct)
       end
 
-      it 'creates a follow request' do
-        expect(FollowRequest.find_by(account: sender, target_account: bob)).to_not be_nil
+      it 'creates a follow request with reblogs' do
+        expect(FollowRequest.find_by(account: sender, target_account: bob, show_reblogs: true)).to_not be_nil
+      end
+    end
+
+    describe 'locked account, no reblogs' do
+      let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, locked: true, username: 'bob')).account }
+
+      before do
+        subject.call(sender, bob.acct, reblogs: false)
+      end
+
+      it 'creates a follow request without reblogs' do
+        expect(FollowRequest.find_by(account: sender, target_account: bob, show_reblogs: false)).to_not be_nil
       end
     end
 
@@ -25,8 +37,22 @@ RSpec.describe FollowService do
         subject.call(sender, bob.acct)
       end
 
-      it 'creates a following relation' do
+      it 'creates a following relation with reblogs' do
+        expect(sender.following?(bob)).to be true
+        expect(sender.muting_reblogs?(bob)).to be false
+      end
+    end
+
+    describe 'unlocked account, no reblogs' do
+      let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account }
+
+      before do
+        subject.call(sender, bob.acct, reblogs: false)
+      end
+
+      it 'creates a following relation without reblogs' do
         expect(sender.following?(bob)).to be true
+        expect(sender.muting_reblogs?(bob)).to be true
       end
     end
 
@@ -42,6 +68,32 @@ RSpec.describe FollowService do
         expect(sender.following?(bob)).to be true
       end
     end
+
+    describe 'already followed account, turning reblogs off' do
+      let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account }
+
+      before do
+        sender.follow!(bob, reblogs: true)
+        subject.call(sender, bob.acct, reblogs: false)
+      end
+
+      it 'disables reblogs' do
+        expect(sender.muting_reblogs?(bob)).to be true
+      end
+    end
+
+    describe 'already followed account, turning reblogs on' do
+      let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account }
+
+      before do
+        sender.follow!(bob, reblogs: false)
+        subject.call(sender, bob.acct, reblogs: true)
+      end
+
+      it 'disables reblogs' do
+        expect(sender.muting_reblogs?(bob)).to be false
+      end
+    end
   end
 
   context 'remote OStatus account' do
diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb
index 7088ae9d1..250a880a2 100644
--- a/spec/services/notify_service_spec.rb
+++ b/spec/services/notify_service_spec.rb
@@ -48,6 +48,26 @@ RSpec.describe NotifyService do
     is_expected.to_not change(Notification, :count)
   end
 
+  describe 'reblogs' do
+    let(:status)   { Fabricate(:status, account: Fabricate(:account)) }
+    let(:activity) { Fabricate(:status, account: sender, reblog: status) }
+
+    it 'shows reblogs by default' do
+      recipient.follow!(sender)
+      is_expected.to change(Notification, :count)
+    end
+
+    it 'shows reblogs when explicitly enabled' do
+      recipient.follow!(sender, reblogs: true)
+      is_expected.to change(Notification, :count)
+    end
+
+    it 'hides reblogs when disabled' do
+      recipient.follow!(sender, reblogs: false)
+      is_expected.to_not change(Notification, :count)
+    end
+  end
+
   context do
     let(:asshole)  { Fabricate(:account, username: 'asshole') }
     let(:reply_to) { Fabricate(:status, account: asshole) }
-- 
cgit 


From 5128c4261e8c067321e70ffda560f14036f56bb0 Mon Sep 17 00:00:00 2001
From: aschmitz <aschmitz@lardbucket.org>
Date: Sat, 11 Nov 2017 14:37:23 -0600
Subject: Updates per code review

Thanks, @valerauko!
---
 app/controllers/api/v1/accounts_controller.rb | 2 +-
 app/models/concerns/account_interactions.rb   | 5 +----
 app/services/follow_service.rb                | 6 ++----
 3 files changed, 4 insertions(+), 9 deletions(-)

(limited to 'app/models')

diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb
index afdbf6e2d..85eb2d60e 100644
--- a/app/controllers/api/v1/accounts_controller.rb
+++ b/app/controllers/api/v1/accounts_controller.rb
@@ -17,7 +17,7 @@ class Api::V1::AccountsController < Api::BaseController
     
     FollowService.new.call(current_user.account, @account.acct, reblogs_arg)
 
-    options = @account.locked? ? {} : { following_map: reblogs_arg, requested_map: { @account.id => false } }
+    options = @account.locked? ? {} : { following_map: { @account.id => reblogs_arg }, requested_map: { @account.id => false } }
 
     render json: @account, serializer: REST::RelationshipSerializer, relationships: relationships(options)
   end
diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb
index 60fd6ded5..a68f7c3d8 100644
--- a/app/models/concerns/account_interactions.rb
+++ b/app/models/concerns/account_interactions.rb
@@ -77,10 +77,7 @@ module AccountInteractions
   def follow!(other_account, reblogs: nil)
     reblogs = true if reblogs.nil?
     rel = active_relationships.create_with(show_reblogs: reblogs).find_or_create_by!(target_account: other_account)
-    if rel.show_reblogs != reblogs
-      rel.show_reblogs = reblogs
-      rel.save!
-    end
+    rel.update!(show_reblogs: reblogs)
 
     rel
   end
diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb
index 6db591999..20579ca63 100644
--- a/app/services/follow_service.rb
+++ b/app/services/follow_service.rb
@@ -6,6 +6,7 @@ class FollowService < BaseService
   # Follow a remote user, notify remote user about the 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 [true, false, nil] reblogs Whether or not to show reblogs, defaults to true
   def call(source_account, uri, reblogs: nil)
     reblogs = true if reblogs.nil?
     target_account = uri.is_a?(Account) ? uri : ResolveRemoteAccountService.new.call(uri)
@@ -22,10 +23,7 @@ class FollowService < BaseService
       # This isn't managed by a method in AccountInteractions, so we modify it
       # ourselves if necessary.
       req = follow_requests.find_by(target_account: other_account)
-      if req.show_reblogs != reblogs
-        req.show_reblogs = reblogs
-        req.save!
-      end
+      req.update!(show_reblogs: reblogs)
       return
     end
 
-- 
cgit