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

Add Admin-level API for actioning Statuses #41

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Conversation

aaga
Copy link

@aaga aaga commented Sep 14, 2023

Creates three new Admin API endpoints for taking actions on Statuses

  • /api/v1/admin/statuses/:id (DELETE and GET)
  • /api/v1/admin/statuses/:id/unsensitive (POST)
  • /api/v1/admin/statuses/:id/action (POST)

(see design doc below for usage)


One-pager problem statement: https://docs.google.com/document/d/1Vyy6FY7hA8rXyXRmC7D0XxLt71jVHHLQK4UpGLKSosg/edit#heading=h.acfm3nlbz5rz

Tickets:
https://mozilla-hub.atlassian.net/browse/SOCIALPLAT-528
https://mozilla-hub.atlassian.net/browse/SOCIALPLAT-529

Design Doc:
https://docs.google.com/document/d/11p55NqeffaonPrH87cYxpN5iv6ubUK-6C1hzRRVRRXU/edit

Note that I changed the design doc slightly from what was previously approved (see 1, 2, and 3)

This needs to be merged and tested on staging, at which point we can change the mastodon-cinder bridge to use the new endpoints.

@aaga aaga requested review from ml8 and toufali September 14, 2023 19:51
@@ -31,7 +31,7 @@ def process_action!
case type
when 'delete'
handle_delete!
when 'mark_as_sensitive'
when 'mark_as_sensitive', 'sensitive'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing non-API controller uses mark_as_sensitive as the action "type". I wanted the new API to be consistent with the nomenclature of the equivalent Accounts API (which just uses sensitive), so I decided to add another "type" that maps to the same action.

@@ -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?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted the definition of status.reported? to be true even if the Report against the Status has already been resolved.

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 reported? to persist as true even after the report is resolved.

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.

# frozen_string_literal: true

class Api::V1::Admin::StatusActionsController < Api::BaseController
# modeled on api/v1/admin/account_actions_controller.rb
Copy link
Author

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.

@ml8 ml8 requested a review from wtfluckey September 14, 2023 23:22
@ml8
Copy link

ml8 commented Sep 14, 2023

This LGTM, but I would like @wtfluckey to take a look if possible!

authorize [:admin, @status], :update?
representative_account = Account.representative
ApplicationRecord.transaction do
UpdateStatusService.new.call(@status, representative_account.id, sensitive: false) if @status.with_media?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about including if @status.with_media?. I see that's how they are doing it in the approve_appeal_service but I don't really get why. Doesn't this mean that UpdateStatusService won't get called if the status doesn't have media?

Copy link
Author

@aaga aaga Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a bit weird but it does mean that UpdateStatusService won't get called if the status doesn't have media. I left it like this to match the equivalent action – mark_as_sensitive – in status_batch_action.rb, which similarly doesn't call UpdateStatusService with sensitive: true unless there is media.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, that sounds excellent!

Copy link

@wtfluckey wtfluckey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a thorough look at this and I left a couple clarifying questions, but overall this makes a ton of sense to me and matches what was described in the design documents.

@wtfluckey
Copy link

My face when reviewing ruby code...it never gets old to me how delightful it is.

5ecd91ee021183cb68799e51d465924e

@aaga aaga merged commit b796d6b into main Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants