Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record account suspend/silence time and keep track of domain blocks #10660

Merged

Conversation

Projects
None yet
3 participants
@ThibG
Copy link
Collaborator

commented May 1, 2019

Currently, undoing an instance block will either unblock every blocked person on that instance or none (depending on the moderator's choice), because an instance block translates to an individual block for every user of that instance.

This change records the time an user was blocked, and ties the unblock part to blocks performed exactly at the same time as the instance block.

This is a work in progress because we have to figure out what to do with instances that were blocked before that PR. The code in the PR will not unblock them, which is probably not what we want to do. The proposed code will currently unblock anyone that matches the severity of the domain block and who was individually blocked before the migration was run.

Also need to add tests and make sure the interface is clear.

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch 3 times, most recently from 73ce2d3 to 7b7a442 May 1, 2019

@ThibG ThibG removed the work in progress label May 1, 2019

@ThibG ThibG marked this pull request as ready for review May 1, 2019

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch 2 times, most recently from 2a33831 to 91cf394 May 1, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented May 1, 2019

Updated. The unblock screen won't ask whether you want to unblock all existing accounts of that instance anymore, and will instead give the count of accounts that would actually be unblocked.

The accounts affected are those whose block date matches that of the domain block or are nil. That is, those who were definitely blocked through the domain block as well as those who were blocked before the feature was merged. There's a few things we could do slightly differently in that regard, but not much.

@Gargron
Copy link
Member

left a comment

We could remove the booleans

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented May 1, 2019

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

I think the idea here is good but removing the boolean columns now is going to save us a ton of headaches down the road wrt our data model.

If you really really don't want to remove the booleans, then we should encapsulate any logic that refers to the booleans or the dates into the account model so that we only have to care about the legacy data model in one place.

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch from 91cf394 to 474bbfd May 11, 2019

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch 3 times, most recently from 023e3c0 to 51ea81a May 11, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented May 11, 2019

I have changed the migration script to set the silenced_at and suspended_at fields of existing accounts to the date of the corresponding domain block if there is one, and to the current time otherwise.
I have changed the code to not use the suspended and silenced fields anymore and updated the tests accordingly.
Finally, I have added a post-deployment migration script to remove those columns from the database.

I have not tried the migration yet. I have tried on my dev and “production” instances. It seems to work.

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch from 2ec9f35 to 518b373 May 11, 2019

@nightpool
Copy link
Collaborator

left a comment

First pass. I think we can narrow down the amount of files touched by this change, especially spec files as noted below. We should make sure we're using the suspend! and silence! methods in specs going forward.

Show resolved Hide resolved spec/services/notify_service_spec.rb Outdated
Show resolved Hide resolved spec/services/notify_service_spec.rb Outdated
Show resolved Hide resolved db/post_migrate/20190511152737_remove_suspended_silenced_account_fields.rb
Show resolved Hide resolved lib/cli.rb Outdated
Show resolved Hide resolved spec/controllers/application_controller_spec.rb Outdated

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch from 5d44a17 to e9b944e May 11, 2019

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch 2 times, most recently from db22b4c to 986a4d5 May 11, 2019

@nightpool
Copy link
Collaborator

left a comment

now that we can undo domain blocks precisely, is there any argument for leaving in the ability to undo them and NOT unsilence/unsuspend affected users? it seems like an extra level of complexity that doesn't help anyone

Show resolved Hide resolved app/models/account.rb Outdated
Show resolved Hide resolved app/models/account.rb Outdated
Show resolved Hide resolved app/models/account.rb Outdated
Show resolved Hide resolved app/models/account.rb Outdated
Show resolved Hide resolved app/services/resolve_account_service.rb Outdated

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch 2 times, most recently from 6ed0a26 to 320d417 May 12, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

Removed the retroactive option as suggested, making all unblocks/unsilences retroactive.

This removes the checkbox but not the text:

image

@nightpool
Copy link
Collaborator

left a comment

LGTM % a minor question about the tests

@ThibG ThibG force-pushed the ThibG:fixes/instance-individual-vs-wide-blocks branch from 320d417 to 7d7a58c May 13, 2019

@Gargron Gargron merged commit 14f6ce2 into tootsuite:master May 14, 2019

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details

noellabo added a commit to fedibird/mastodon that referenced this pull request May 19, 2019

Record account suspend/silence time and keep track of domain blocks (t…
…ootsuite#10660)

* Record account suspend/silence time and keep track of domain blocks

* Also unblock users who were suspended/silenced before dates were recorded

* Add tests

* Keep track of suspending date for users suspended through the CLI

* Show accurate number of accounts that would be affected by unsuspending an instance

* Change migration to set silenced_at and suspended_at

* Revert "Also unblock users who were suspended/silenced before dates were recorded"

This reverts commit a015c65.

* Switch from using suspended and silenced to suspended_at and silenced_at

* Add post-deployment migration script to remove `suspended` and `silenced` columns

* Use Account#silence! and Account#suspend! instead of updating the underlying property

* Add silenced_at and suspended_at migration to post-migration

* Change account fabricator to translate suspended and silenced attributes

* Minor fixes

* Make unblocking domains always retroactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.