/mastodon

Account domain blocks #2381

Merged
merged 17 commits into from May 18, 2017

Conversation

Reviewers

@mjankowski mjankowski

@beatrix-bitrot beatrix-bitrot

Assignees
No one assigned
Labels
None yet
Projects
Done in v1.4 roadmap
Milestone
No milestone
4 participants
@Gargron @wxcafe @mjankowski @beatrix-bitrot
@Gargron
Member

Gargron commented Apr 24, 2017 •
edited
Edited 1 time
  • @Gargron Gargron edited May 18, 2017 (most recent)

  • Model
  • Filter from public timeline queries
  • Filter from notifications
  • Filter from home feed
  • Filter from streaming API
  • Filter from "context" (conversation threads)
  • API method to block/unblock domain
  • API method to list blocked domains

Probably better done in another PR:

  • Web UI dropdown option to block domain on profile
  • Web UI list of blocked domains
app/models/account.rb
@@ -107,6 +107,10 @@ def blocking?(other_account)
blocking.include?(other_account)
end
+ def domain_blocking?(other_account)
+ AccountDomainBlock.where(account: self, domain: other_account.domain).exists?

This comment has been minimized.

Show comment
Hide comment
@mjankowski

mjankowski Apr 24, 2017

Collaborator

Account could probably use a has_many for :account_domains_blocks, which this method could then use.
@mjankowski

mjankowski Apr 24, 2017

Collaborator

Account could probably use a has_many for :account_domains_blocks, which this method could then use.
app/models/status.rb
@@ -188,6 +188,8 @@ def permitted_for(target_account, account)
def filter_timeline(query, account)
blocked = Rails.cache.fetch("exclude_account_ids_for:#{account.id}") { Block.where(account: account).pluck(:target_account_id) + Block.where(target_account: account).pluck(:account_id) + Mute.where(account: account).pluck(:target_account_id) }
+ domains = Rails.cache.fetch("exclude_domains_for:#{account_id}") { AccountDomainBlock.where(account: account).pluck(:domain) }

This comment has been minimized.

Show comment
Hide comment
@mjankowski

mjankowski Apr 24, 2017

Collaborator

Instead of a local variable here, how about a (Rails cached) method like Account#blocked_domains? which the next line could call directly
@mjankowski

mjankowski Apr 24, 2017

Collaborator

Instead of a local variable here, how about a (Rails cached) method like Account#blocked_domains? which the next line could call directly
db/schema.rb
@@ -202,6 +210,14 @@
t.datetime "image_updated_at"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
+ t.integer "type", default: 0, null: false

This comment has been minimized.

Show comment
Hide comment
@mjankowski

mjankowski Apr 24, 2017

Collaborator

This schema change is likely from another PR.
@mjankowski

mjankowski Apr 24, 2017

Collaborator

This schema change is likely from another PR.
spec/fabricators/account_domain_block_fabricator.rb
@@ -0,0 +1,4 @@
+Fabricator(:account_domain_block) do
+ account_id 1

This comment has been minimized.

Show comment
Hide comment
@mjankowski

mjankowski Apr 24, 2017

Collaborator

ONE!
@mjankowski

mjankowski Apr 24, 2017

Collaborator

ONE!
@wxcafe

This comment has been minimized.

Show comment
Hide comment
@wxcafe

wxcafe Apr 24, 2017

Contributor

Closing #501 since this is the same feature and that old PR is not up to date anymore (by a long shot)
Contributor

wxcafe commented Apr 24, 2017

Closing #501 since this is the same feature and that old PR is not up to date anymore (by a long shot)

@wxcafe wxcafe referenced this pull request Apr 24, 2017

Closed

Better control over who's following me #2187

@staticsafe staticsafe referenced this pull request Apr 24, 2017

Closed

user blocking of domains #501

@Gargron Gargron added this to In progress in v1.4 roadmap May 11, 2017

Gargron added some commits May 7, 2017

@Gargron
Add <ostatus:conversation /> tag to Atom input/output 
Only uses ref attribute (not href) because href would be
the alternate link that's always included also.

Creates new conversation for every non-reply status. Carries
over conversation for every reply. Keeps remote URIs verbatim,
generates local URIs on the fly like the rest of them.
9f8b335
@Gargron
Conversation muting - prevents notifications that reference a convers… 
…ation

(including replies, favourites, reblogs) from being created. API endpoints
/api/v1/statuses/:id/mute and /api/v1/statuses/:id/unmute

Currently no way to tell when a status/conversation is muted, so the web UI
only has a "disable notifications" button, doesn't work as a toggle
cc5b451
@Gargron
Display "Dismiss notifications" on all statuses in notifications colu… 
…mn, not just own
a6ceb48
@Gargron
Add "muted" as a boolean attribute on statuses JSON 
For now always false on contained reblogs, since it's only relevant for
statuses returned from the notifications endpoint, which are not nested

Remove "Disable notifications" from detailed status view, since it's
only relevant in the notifications column
9c557b6
@Gargron
Up max class length
68ee09a
@Gargron
Remove pending test for conversation mute
Loading status checks…
f3ae643
@Gargron
Add tests, clean up
Loading status checks…
b2ed532
@Gargron
Rename to "mute conversation" and "unmute conversation"
7a4ae3e
@Gargron
Merge branch 'master' into feature-conversations-muting
87ded91
@Gargron
Raise validation error when trying to mute/unmute status without conv… 
…ersation
Loading status checks…
6467156
@Gargron
Adding account domain blocks that filter notifications and public tim… 
…elines
5ab9dca
@Gargron
Add tests for domain blocks in notifications, public timelines 
Filter reblogs of blocked domains from home
Loading status checks…
09881b4
@Gargron
Add API for listing and creating account domain blocks
8f62c3a
@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron May 14, 2017

Member

I can't imagine very well how the API for domain blocks should actually look.

GET /api/v1/domain_blocks - returns list of { id: domainBlockId, domain: blockedDomainString }
POST /api/v1/domain_blocks - creates domain block for "domain" param

How to unblock?! Use a DELETE route using the ID of the domain block? Use domains instead of IDs?
Member

Gargron commented May 14, 2017

I can't imagine very well how the API for domain blocks should actually look.

GET /api/v1/domain_blocks - returns list of { id: domainBlockId, domain: blockedDomainString }
POST /api/v1/domain_blocks - creates domain block for "domain" param

How to unblock?! Use a DELETE route using the ID of the domain block? Use domains instead of IDs?

@Gargron Gargron changed the title from [WIP] Account domain blocks to Account domain blocks May 18, 2017

@beatrix-bitrot

beatrix-bitrot requested changes May 18, 2017
View changes

overall looks good, only very minor stuff

app/models/status.rb
- statuses = Status.where(id: ids).to_a
+ statuses = Status.where(id: ids).includes(:account).to_a
+
+ # FIXME: n+1 bonanza

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

Are you gonna fix the N+1 bonanza?
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

Are you gonna fix the N+1 bonanza?

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron May 18, 2017

Member

No it's been present since day 1, it's a fix for another day with less time pressure
@Gargron

Gargron May 18, 2017

Member

No it's been present since day 1, it's a fix for another day with less time pressure
app/controllers/api/v1/domain_blocks_controller.rb
+
+ respond_to :json
+
+ def show

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

curious why this is show and not index since it's giving a collection rather than an individual object
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

curious why this is show and not index since it's giving a collection rather than an individual object

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron May 18, 2017

Member

Because it's a "resource" route and not "resources" (so that we can send DELETE /api/v1/domain_blocks)
@Gargron

Gargron May 18, 2017

Member

Because it's a "resource" route and not "resources" (so that we can send DELETE /api/v1/domain_blocks)
config/routes.rb
@@ -150,7 +150,8 @@
resources :favourites, only: [:index]
resources :reports, only: [:index, :create]
- resource :instance, only: [:show]
+ resource :instance, only: [:show]
+ resource :domain_blocks, only: [:show, :create, :destroy]

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

again, wondering why show and not index
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

again, wondering why show and not index
spec/models/account_domain_block_spec.rb
+require 'rails_helper'
+
+RSpec.describe AccountDomainBlock, type: :model do
+ pending "add some examples to (or delete) #{__FILE__}"

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

indeed! add some tests or delete it
@beatrix-bitrot

beatrix-bitrot May 18, 2017

Collaborator

indeed! add some tests or delete it
@beatrix-bitrot

beatrix-bitrot approved these changes May 18, 2017
View changes

concerns addressed

@Gargron Gargron merged commit 620d0d8 into master May 18, 2017
1 of 3 checks passed

1 of 3 checks passed

codeclimate 1 new issue (1 fixed)
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Gargron Gargron deleted the feature-account-domain-blocks branch May 18, 2017

@Gargron Gargron referenced this pull request May 19, 2017

Closed

user blocking/whitelisting of entire domains #423

@Gargron Gargron moved this from In progress to Done in v1.4 roadmap May 19, 2017

@unarist unarist referenced this pull request May 26, 2017

Closed

Replies from blocked users to non-blocked users visible in thread view #647

@decors decors referenced this pull request in decors/mastodon.cr May 29, 2017

Merged

Domain blocks #2

gol-cha added a commit to gol-cha/mastodon that referenced this pull request May 29, 2017

@Gargron @gol-cha
Account domain blocks (#2381) 
* Add <ostatus:conversation /> tag to Atom input/output

Only uses ref attribute (not href) because href would be
the alternate link that's always included also.

Creates new conversation for every non-reply status. Carries
over conversation for every reply. Keeps remote URIs verbatim,
generates local URIs on the fly like the rest of them.

* Conversation muting - prevents notifications that reference a conversation
(including replies, favourites, reblogs) from being created. API endpoints
/api/v1/statuses/:id/mute and /api/v1/statuses/:id/unmute

Currently no way to tell when a status/conversation is muted, so the web UI
only has a "disable notifications" button, doesn't work as a toggle

* Display "Dismiss notifications" on all statuses in notifications column, not just own

* Add "muted" as a boolean attribute on statuses JSON

For now always false on contained reblogs, since it's only relevant for
statuses returned from the notifications endpoint, which are not nested

Remove "Disable notifications" from detailed status view, since it's
only relevant in the notifications column

* Up max class length

* Remove pending test for conversation mute

* Add tests, clean up

* Rename to "mute conversation" and "unmute conversation"

* Raise validation error when trying to mute/unmute status without conversation

* Adding account domain blocks that filter notifications and public timelines

* Add tests for domain blocks in notifications, public timelines
Filter reblogs of blocked domains from home

* Add API for listing and creating account domain blocks

* API for creating/deleting domain blocks, tests for Status#ancestors
and Status#descendants, filter domain blocks from them

* Filter domains in streaming API

* Update account_domain_block_spec.rb
b9c1361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment