New methods to access and update skip_content_moderation value #23432

Erin007 · 2018-06-29T19:29:00Z

To make the buttons in #23406 function so that project validators can enable or disable automated content moderation for a given project, this PR introduces methods to access and update the value of skip_content_moderation. Also included are tests for the new methods.

islemaster · 2018-06-29T20:28:29Z

shared/middleware/helpers/storage_apps.rb

@@ -14,7 +14,7 @@ def initialize(storage_id)
    @table = PEGASUS_DB[:storage_apps]
  end

-  def create(value, ip:, type: nil, published_at: nil, remix_parent_id: nil)
+  def create(value, ip:, type: nil, published_at: nil, remix_parent_id: nil, skip_content_moderation: false)


Do we ever need to create a storage_apps row with skip_content_moderation: true? If so, let's add tests for this case. If not, maybe we don't want it to be a parameter here.

islemaster · 2018-06-29T20:30:17Z

shared/middleware/helpers/storage_apps.rb

@@ -138,6 +139,15 @@ def get_abuse(channel_id)
    row[:abuse_score]
  end

+  def get_skip_content_moderation_value(channel_id)


In #23406 you settled on disable-auto-moderation as the consistent language for this feature. Our server code should use the same language, unless there's a particular reason it's different.

Idiomatic ruby nit: Name methods that return a boolean value as a query def auto_moderation_disabled?(channel_id)

islemaster · 2018-06-29T20:41:44Z

shared/middleware/helpers/storage_apps.rb

+    raise NotFound, "channel `#{channel_id}` not found" unless row
+
+    update_content_moderation = @table.where(id: id).exclude(state: 'deleted').update({skip_content_moderation: should_skip})
+    raise NotFound, "channel `#{channel_id}` not found" if update_content_moderation == should_skip


I don't think this works the way you describe it (there's no test for it) and I'm not sure we want to do it anyway.

Sequel::Dataset#update returns the number of rows updated, not the new value; therefore update_content_moderation == should_skip is probably never true becase it's comparing an integer to a boolean.

It's generally a good idea to make server APIs idempotent (I enjoyed this article on the subject). In this case, that means we want this method to do nothing and report success if the setting is already the desired value, rather than throw an error. This way, if a client requests this operation but the server's response gets lost on the way back, the client can safely re-send the request and not worry about causing an error.

islemaster · 2018-06-29T20:43:49Z

shared/middleware/helpers/storage_apps.rb

+  # @raise [NotFound] if the channel does not exist or already has the desired
+  # value for skip_content_moderation
+  #
+  def change_content_moderation(channel_id, should_skip)


Naming nit: set instead of change. Since this is a boolean value it might be a non-issue, but with objects it matters. "Change" implies the subject may inherit some of its past state, while "set" suggests the value after the operation will be only the new information you passed it. It's like the difference between PUT and PATCH in HTTP.

islemaster · 2018-06-29T20:46:56Z

shared/middleware/helpers/storage_apps.rb

+    row = @table.where(id: id).exclude(state: 'deleted').first
+    raise NotFound, "channel `#{channel_id}` not found" unless row
+
+    update_content_moderation = @table.where(id: id).exclude(state: 'deleted').update({skip_content_moderation: should_skip})


Because update is safe if the project is not present and it returns the number of rows modified, you can implement this API with one query instead of two.

_owner, id = storage_decrypt_channel_id(channel_id) rows_changed = @table. where(id: id). exclude(state: 'deleted'). update({skip_content_moderation: should_skip}) raise NotFound, "channel `#{channel_id}` not found" unless rows_changed > 0

davidsbailey · 2018-07-11T19:44:42Z

@Erin007 please tag me when you are ready for another look

…ests

…ue on creation

…dator permissions

…or? to access request so current_user is known

Erin007 · 2018-07-12T23:17:29Z

The buttons work! Thank you for your help @davidsbailey and @islemaster. This PR is ready for review.

Erin007 · 2018-07-12T23:18:28Z

shared/test/test_channels.rb

@@ -303,6 +303,29 @@ def test_abuse

    # Ideally we would also test that deleting abuse works when we're an admin
    # but don't currently have a way to simulate admin from tests
+    # TODO (Erin B) confirm if this is accurate and update tests if needed.
+    # You might not need to be an admin, you might need project validator
+    # permissions.


I'm going to tackle this in a follow up PR.

islemaster

I'd like to see showProjectAdmin.js updated to respect the async nature of the API call. The rest are optional cleanup suggestions.

islemaster · 2018-07-12T23:35:44Z

apps/src/code-studio/showProjectAdmin.js

@@ -83,14 +83,16 @@ export default () => {
  }

  $('#disable-auto-moderation').click(function () {
-    alert("You clicked the disable automated image moderation button. It doesn't do anything yet because our moderation filter is turned off, but it will be functional soon. Thanks for your patience with this work-in-progress button!");
+    dashboard.project.disableAutoContentModeration();
+    alert("Our moderation filter is currently turned off for all projects, but your choice will be saved and applied when we re-enable the moderation service.");
    $('#disable-auto-moderation').hide();
    $('#moderation-explanation').hide();
    $('#enable-auto-moderation').show();


It would be nice if we didn't switch the displayed buttons until/unless the request to enable/disable moderation succeeds. Also a great opportunity to use async/await!

// in project.js // Make these methods return a promise that resolves when the request succeeds, or rejects if it fails. disableAutoContentModeration() { return new Promise((resolve, reject) => { channels.update(`${this.getCurrentId()}/disable-content-moderation`, (err) => { err ? reject(err) : resolve() }); }); }, // in showProjectAdmin.js // Make the click handler async, and await disableAutoContentModeration before changing the buttons. $('#disable-auto-moderation').click(async function() { await dashboard.project.disableAutoContentModeration(); alert("Our moderation filter..."); $('#disable-auto-moderation').hide(); $('#moderation-explanation').hide(); $('#enable-auto-moderation').show(); });

islemaster · 2018-07-12T23:37:08Z

shared/middleware/channels_api.rb

+      bad_request
+    end
+    {skip_content_moderation: value}.to_json
+  end


These two blocks are identical except for the route name and the boolean value being set. Let's extract a helper method!

islemaster · 2018-07-12T23:37:37Z

shared/middleware/channels_api.rb

@@ -283,4 +317,8 @@ class ChannelsApi < Sinatra::Base
  post %r{/v3/channels/([^/]+)/abuse/delete$} do |_id|
    call(env.merge('REQUEST_METHOD' => 'DELETE', 'PATH_INFO' => File.dirname(request.path_info)))
  end
+
+  def project_validator?


Possibly worth a comment explaining this pass-through method is here so we can stub it in tests.

islemaster · 2018-07-12T23:38:43Z

shared/middleware/helpers/auth_helpers.rb

@@ -69,15 +69,15 @@ def has_permission?(permission)
 end

 # @param [Integer] section_id
-# @returns [Boolean] true iff the current user is the owner of the given section.
+# @returns [Boolean] true if the current user is the owner of the given section.
 #          Note: NOT always true for admins.


😆 This isn't a typo - it's shorthand for "if and only if".

But maybe you're right to correct that, given the note on the next line!

ah, I had no idea. Thanks!

islemaster · 2018-07-12T23:42:43Z

shared/test/test_channels.rb

+
+    post "/v3/channels/#{channel_id}/enable-content-moderation"
+    assert last_response.ok?
+    assert_equal false, JSON.parse(last_response.body)['skip_content_moderation']


Paranoid testing suggestion - double each of these tests, so you call disable-content-moderation twice in a row and enable-content-moderation twice in a row, verifying that each route is a no-op when the project is already in the desired state, and not just toggling the value.

Erin007 · 2018-07-17T17:23:04Z

I updated the test coverage and implemented async await, anything else pressing before merging @islemaster?

islemaster

Looks great!

Erin007 requested review from davidsbailey and islemaster June 29, 2018 19:29

Erin007 added the star-labs label Jun 29, 2018

islemaster reviewed Jun 29, 2018

View reviewed changes

Erin007 added 9 commits July 12, 2018 10:38

new methods to access and update skip_content_moderation, including t…

d63d96f

…ests

don't use skip_content_moderation parameter since it will never be tr…

ca7f793

…ue on creation

use disable/enable to be more consistent

9b6be7a

update language in tests to match

3f5d8e9

change -> set

da51ec9

consolidate queries for set_content_moderation

32da72e

enable/disable content mod from channels_api route

0ffa94c

JS functions for enable/disable content mod

8a60bba

test channels_api for enable/disable content mod, except project vali…

e8d3fcf

…dator permissions

Erin007 force-pushed the functional-content-mod-buttons branch from ab55b1c to a36f65f Compare July 12, 2018 17:39

Erin007 added 3 commits July 12, 2018 16:14

channels_api test now stub project_validator? check

6075cde

use an instance method rather than a class method for project_validat…

d436b55

…or? to access request so current_user is known

finish JS functions

35bb84d

Erin007 force-pushed the functional-content-mod-buttons branch from 7281052 to 35bb84d Compare July 12, 2018 23:15

Erin007 commented Jul 12, 2018

View reviewed changes

islemaster reviewed Jul 12, 2018

View reviewed changes

Erin007 added 2 commits July 16, 2018 15:48

use async await and add in comment about pass-through method

3ed74e7

make tests slightly more robust

0d284eb

islemaster approved these changes Jul 17, 2018

View reviewed changes

Erin007 merged commit 953b56f into staging Jul 17, 2018

Erin007 deleted the functional-content-mod-buttons branch July 17, 2018 17:37

Erin007 mentioned this pull request Jul 23, 2018

Projects: Test deleting abuse with the project_validator? helper #23874

Merged

Erin007 mentioned this pull request Sep 18, 2018

Image Moderation: functional disable automated moderation buttons #24869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New methods to access and update skip_content_moderation value #23432

New methods to access and update skip_content_moderation value #23432

Erin007 commented Jun 29, 2018

islemaster Jun 29, 2018

islemaster Jun 29, 2018

islemaster Jun 29, 2018

islemaster Jun 29, 2018

islemaster Jun 29, 2018

islemaster Jun 29, 2018

davidsbailey commented Jul 11, 2018

Erin007 commented Jul 12, 2018

Erin007 Jul 12, 2018

islemaster left a comment

islemaster Jul 12, 2018

islemaster Jul 12, 2018

islemaster Jul 12, 2018

islemaster Jul 12, 2018

islemaster Jul 12, 2018

Erin007 Jul 16, 2018

islemaster Jul 12, 2018

Erin007 Jul 17, 2018

Erin007 commented Jul 17, 2018

islemaster left a comment

New methods to access and update skip_content_moderation value #23432

New methods to access and update skip_content_moderation value #23432

Conversation

Erin007 commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Jul 11, 2018

Erin007 commented Jul 12, 2018

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Erin007 commented Jul 17, 2018

islemaster left a comment

Choose a reason for hiding this comment