-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Admin-level API for actioning Statuses #41
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# frozen_string_literal: true | ||
|
||
class Api::V1::Admin::StatusActionsController < Api::BaseController | ||
# modeled on api/v1/admin/account_actions_controller.rb | ||
|
||
include Authorization | ||
|
||
# only support a subset of StatusBatchAction types | ||
ALLOWED_TYPES = %w( | ||
delete | ||
sensitive | ||
).freeze | ||
|
||
before_action -> { authorize_if_got_token! :'admin:write', :'admin:write:statuses' } | ||
before_action :set_status | ||
|
||
after_action :verify_authorized | ||
|
||
def create | ||
authorize [:admin, @status], :update? | ||
raise ActiveRecord::RecordInvalid unless valid_type? | ||
|
||
status_batch_action = Admin::StatusBatchAction.new(resource_params) | ||
status_batch_action.status_ids = [@status.id] | ||
status_batch_action.current_account = current_account | ||
status_batch_action.save! | ||
|
||
render_empty | ||
end | ||
|
||
private | ||
|
||
def valid_type? | ||
params[:type] && ALLOWED_TYPES.include?(params[:type]) | ||
end | ||
|
||
def set_status | ||
@status = Status.find(params[:status_id]) | ||
end | ||
|
||
def resource_params | ||
params.permit( | ||
:type, | ||
:report_id, | ||
:text, | ||
:send_email_notification | ||
) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# frozen_string_literal: true | ||
|
||
class Api::V1::Admin::StatusesController < Api::BaseController | ||
# modeled on api/v1/admin/accounts_controller.rb | ||
|
||
include Authorization | ||
include AccountableConcern | ||
|
||
before_action -> { authorize_if_got_token! :'admin:read', :'admin:read:statuses' }, only: [:show] | ||
before_action -> { authorize_if_got_token! :'admin:write', :'admin:write:statuses' }, except: [:show] | ||
before_action :set_status | ||
|
||
after_action :verify_authorized | ||
|
||
def show | ||
authorize [:admin, @status], :show? | ||
render json: @status, serializer: REST::StatusSerializer | ||
end | ||
|
||
def destroy | ||
# modeled on handle_delete from status_batch_action.rb | ||
authorize [:admin, @status], :destroy? | ||
ApplicationRecord.transaction do | ||
@status.discard_with_reblogs | ||
log_action :destroy, @status | ||
Tombstone.find_or_create_by(uri: @status.uri, account: @status.account, by_moderator: true) | ||
end | ||
json = render_to_body json: @status, serializer: REST::StatusSerializer, source_requested: true | ||
|
||
RemovalWorker.perform_async(@status.id, { 'preserve' => @status.account.local?, 'immediate' => !@status.account.local? }) | ||
|
||
render json: json | ||
end | ||
|
||
def unsensitive | ||
# modeled on undo_mark_statuses_as_sensitive from approve_appeal_service.rb | ||
authorize [:admin, @status], :update? | ||
representative_account = Account.representative | ||
ApplicationRecord.transaction do | ||
UpdateStatusService.new.call(@status, representative_account.id, sensitive: false) if @status.with_media? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about including There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's a bit weird but it does mean that Since this method is intended to undo moderation decisions, I figured it made sense to mirror the behavior. Zooming out, I guess the intention is that moderators should only be able to mark visual media as sensitive since it can be jarring/triggering/affective in a user's feed. I'll ask the T&S team if they want to change that policy to allow marking text-only posts as sensitive. Then, we can change both the action and undo action together. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perfect, that sounds excellent! |
||
log_action :unsensitive, @status | ||
end | ||
render json: @status, serializer: REST::StatusSerializer | ||
end | ||
|
||
private | ||
|
||
def set_status | ||
@status = Status.find(params[:id]) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ def process_action! | |
case type | ||
when 'delete' | ||
handle_delete! | ||
when 'mark_as_sensitive' | ||
when 'mark_as_sensitive', 'sensitive' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing non-API controller uses |
||
handle_mark_as_sensitive! | ||
when 'report' | ||
handle_report! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,7 +273,7 @@ def non_sensitive_with_media? | |
end | ||
|
||
def reported? | ||
@reported ||= Report.where(target_account: account).unresolved.where('? = ANY(status_ids)', id).exists? | ||
@reported ||= Report.where(target_account: account).where('? = ANY(status_ids)', id).exists? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted the definition of The Admin Status Policy lets the Admin API "show (GET)" Statuses that would otherwise not be visible to them, if the status is reported. Since we auto-resolve reports in the bridge, we need I don't think this changes the intention too much, so I think upstream will be ok with this too. If not, it's easy to remove for the PR we do there. |
||
end | ||
|
||
def emojis | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
|
||
RSpec.describe Api::V1::Admin::StatusesController do | ||
render_views | ||
|
||
let(:role) { UserRole.find_by(name: 'Moderator') } | ||
let(:user) { Fabricate(:user, role: role) } | ||
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } | ||
|
||
before do | ||
allow(controller).to receive(:doorkeeper_token) { token } | ||
end | ||
|
||
describe 'GET #show' do | ||
let(:scopes) { 'admin:read:statuses' } | ||
let(:status) { Fabricate(:status) } | ||
|
||
before do | ||
get :show, params: { id: status.id } | ||
end | ||
|
||
it_behaves_like 'forbidden for wrong scope', 'read:statuses' # non-admin scope | ||
it_behaves_like 'forbidden for wrong role', '' | ||
|
||
it 'returns http success' do | ||
expect(response).to have_http_status(200) | ||
end | ||
end | ||
|
||
describe 'DELETE #destroy' do | ||
let(:scopes) { 'admin:write:statuses' } | ||
let(:status) { Fabricate(:status) } | ||
|
||
before do | ||
post :destroy, params: { id: status.id } | ||
end | ||
|
||
it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses' | ||
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope | ||
it_behaves_like 'forbidden for wrong role', '' | ||
|
||
it 'returns http success' do | ||
expect(response).to have_http_status(200) | ||
end | ||
|
||
it 'removes the status' do | ||
expect(Status.find_by(id: status.id)).to be_nil | ||
end | ||
end | ||
|
||
describe 'POST #unsensitive' do | ||
let(:scopes) { 'admin:write:statuses' } | ||
let(:media) { Fabricate(:media_attachment) } | ||
let(:status) { Fabricate(:status, media_attachments: [media], sensitive: true) } | ||
|
||
before do | ||
post :unsensitive, params: { id: status.id } | ||
end | ||
|
||
it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses' | ||
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope | ||
it_behaves_like 'forbidden for wrong role', '' | ||
|
||
it 'returns http success' do | ||
expect(response).to have_http_status(200) | ||
end | ||
|
||
it 'unmarks status as sensitive' do | ||
expect(status.reload.sensitive?).to be false | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
|
||
RSpec.describe 'Status actions' do | ||
let(:role) { UserRole.find_by(name: 'Moderator') } | ||
let(:user) { Fabricate(:user, role: role) } | ||
let(:scopes) { 'admin:write admin:write:statuses' } | ||
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } | ||
let(:headers) { { 'Authorization' => "Bearer #{token.token}" } } | ||
let(:mailer) { instance_double(ActionMailer::MessageDelivery, deliver_later!: nil) } | ||
|
||
before do | ||
allow(UserMailer).to receive(:warning).with(status.account.user, anything).and_return(mailer) | ||
end | ||
|
||
shared_examples 'a successful notification delivery' do | ||
it 'notifies the user about the action taken' do | ||
subject | ||
|
||
expect(UserMailer).to have_received(:warning).with(status.account.user, anything).once | ||
expect(mailer).to have_received(:deliver_later!).once | ||
end | ||
end | ||
|
||
shared_examples 'a successful logged action' do |action_type, target_type| | ||
it 'logs action' do | ||
subject | ||
|
||
log_item = Admin::ActionLog.where(action: action_type).last | ||
|
||
expect(log_item).to be_present | ||
expect(log_item.account_id).to eq(user.account_id) | ||
expect(log_item.target_id).to eq(target_type == :status ? status.id : report.id) | ||
end | ||
end | ||
|
||
describe 'POST /api/v1/admin/statuses/:id/action' do | ||
subject do | ||
post "/api/v1/admin/statuses/#{status.id}/action", headers: headers, params: params | ||
end | ||
|
||
let(:account) { Fabricate(:account, domain: nil) } # local account for email notification | ||
let(:media) { Fabricate(:media_attachment) } | ||
let(:status) { Fabricate(:status, media_attachments: [media], account: account) } | ||
|
||
context 'with type of delete' do | ||
let(:params) { { type: 'delete' } } | ||
|
||
it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses' | ||
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope | ||
it_behaves_like 'forbidden for wrong role', '' | ||
it_behaves_like 'a successful logged action', :destroy, :status | ||
|
||
it 'returns http success' do | ||
subject | ||
|
||
expect(response).to have_http_status(200) | ||
end | ||
|
||
it 'deletes the status' do | ||
expect { subject }.to change { Status.find_by(id: status.id) }.from(status).to(nil) | ||
end | ||
end | ||
|
||
context 'with type of sensitive' do | ||
let(:params) { { type: 'sensitive' } } | ||
|
||
it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses' | ||
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope | ||
it_behaves_like 'forbidden for wrong role', '' | ||
it_behaves_like 'a successful logged action', :update, :status | ||
|
||
it 'returns http success' do | ||
subject | ||
|
||
expect(response).to have_http_status(200) | ||
end | ||
|
||
it 'marks the status as sensitive' do | ||
expect { subject }.to change { status.reload.sensitive? }.from(false).to(true) | ||
end | ||
end | ||
|
||
context 'with no type' do | ||
let(:params) { {} } | ||
|
||
it 'returns http unprocessable entity' do | ||
subject | ||
|
||
expect(response).to have_http_status(422) | ||
end | ||
end | ||
|
||
context 'with invalid type' do | ||
let(:params) { { type: 'invalid' } } | ||
|
||
it 'returns http unprocessable entity' do | ||
subject | ||
|
||
expect(response).to have_http_status(422) | ||
end | ||
end | ||
|
||
context 'with notification delivery' do | ||
let(:params) { { type: 'delete', send_email_notification: true } } | ||
|
||
it_behaves_like 'a successful notification delivery' | ||
end | ||
|
||
context 'with report' do | ||
let(:report) { Fabricate(:report) } | ||
let(:params) { { type: 'delete', report_id: report.id } } | ||
|
||
it_behaves_like 'a successful logged action', :resolve, :report | ||
|
||
it 'resolves report' do | ||
expect { subject }.to change { report.reload.unresolved? }.from(true).to(false) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left 3 of these
modeled on
comments in so that it's clear where some of the new code is descended/remixed from. Happy to remove them before merging.