Merge pull request #1766 from codidact/0valt/1289/flag-validations

Flagging improvements

Author: ArtOfCode-

app/assets/javascripts/flags.js

@@ -1,5 +1,5 @@
 $(() => {
-  $(document).on('click', '.flag-link', (ev) => {
+  $(document).on('click', '.flag-link', async (ev) => {
     ev.preventDefault();
     const self = $(ev.target);
     const isCommentFlag = self.hasClass('js-comment-flag');
@@ -21,6 +21,10 @@ $(() => {
     }
 
     const postId = self.data('post-id');
+
+    /**
+     * @type {QPixelFlagData}
+     */
     const data = {
       'flag_type': (reason !== -1) ? reason : null,
       'post_id': postId,
@@ -29,47 +33,26 @@ $(() => {
     };
 
     if (requiresDetails && data['reason'].length < 1) {
-      QPixel.createNotification('danger',
-                                'Details are required for this flag type - please enter a message.');
+      QPixel.createNotification('danger', 'Details are required for this flag type - please enter a message.');
       return;
     }
 
-    const responseType = isCommentFlag ? null : activeRadio.data('response-type');
+    const closeFlagModal = () => {
+      self.parents('.js-flag-box').removeClass('is-active');
+      $(`#flag-comment-${postId}`).removeClass('is-active');
+    };
 
-    $.ajax({
-      'type': 'POST',
-      'url': '/flags/new',
-      'data': data,
-      // TODO: review
-      // 'target': self
-    })
-      .done((response) => {
-        if(response.status !== 'success') {
-          QPixel.createNotification('danger', '<strong>Failed:</strong> ' + response.message);
-        }
-        else {
-          const messages = {
-            comment: `<strong>Thanks!</strong> Your flag has been added as a comment for the author to review.`
-          };
-          const defaultMessage = `<strong>Thanks!</strong> We will review your flag.`;
-          QPixel.createNotification('success', messages[responseType] || defaultMessage);
-          $(`#flag-post-${postId}`).val('');
-        }
-        self.parents('.js-flag-box').removeClass('is-active');
-        $(`#flag-comment-${postId}`).removeClass('is-active');
+    try {
+      const response = await QPixel.flag(data);
 
-      })
-      .fail((jqXHR, _textStatus, _errorThrown) => {
-        let message = jqXHR.status;
-        try {
-          message = JSON.parse(jqXHR.responseText)['message'];
-        }
-        finally {
-          QPixel.createNotification('danger', '<strong>Failed:</strong> ' + message);
-        }
-        self.parents('.js-flag-box').removeClass('is-active');
-        $(`#flag-comment-${postId}`).removeClass('is-active');
-      });
+      QPixel.handleJSONResponse(response, (data) => {
+        QPixel.createNotification('success', data.message);
+        $(`#flag-post-${postId}`).val('');
+      }, closeFlagModal);
+    } catch(e) {
+      console.warn(`[flags/new] API error:`, e);
+      QPixel.createNotification('danger', 'Failed to flag.');
+    }
   });
 
   $('.js-start-escalate').on('click', (ev) => {

app/assets/javascripts/qpixel_api.js

@@ -443,13 +443,25 @@ window.QPixel = {
     return content;
   },
 
-  handleJSONResponse: (data, onSuccess) => {
+  handleJSONResponse: (data, onSuccess, onFinally) => {
     if (data.status === 'success') {
-      onSuccess(data)
+      onSuccess(data);
     }
     else {
       QPixel.createNotification('danger', data.message);
     }
+
+    onFinally?.(data);
+  },
+
+  flag: async (flag) => {
+    const resp = await QPixel.fetchJSON(`/flags/new`, { ...flag }, {
+      headers: { 'Accept': 'application/json' }
+    });
+
+    const data = await resp.json();
+
+    return data;
   },
 
   deleteComment: async (id) => {

app/controllers/flags_controller.rb

@@ -14,9 +14,7 @@ def new
     max_flags_per_day = SiteSetting[current_user.privilege?('unrestricted') ? 'RL_Flags' : 'RL_NewUserFlags']
 
     if recent_flags >= max_flags_per_day
-      flag_limit_msg = 'Thank you. Flags from people like you help us keep this site clean. ' \
-                       "However, you have reached your daily flag limit of #{max_flags_per_day} " \
-                       'flags. Please come back tomorrow to continue flagging.'
+      flag_limit_msg = I18n.t('flags.errors.rate_limited', count: max_flags_per_day)
 
       AuditLog.rate_limit_log(event_type: 'flag', related: Post.find(params[:post_id]), user: current_user,
                               comment: "limit: #{max_flags_per_day}\n\ntype:#{type}\ncomment:\n#{params[:reason].to_i}")
@@ -27,16 +25,19 @@ def new
 
     if type&.name == "needs author's attention"
       create_as_feedback_comment Post.find(params[:post_id]), current_user, params[:reason]
-      render json: { status: 'success' }, status: :created
+      render json: { status: 'success', message: I18n.t('flags.success.create_author_attention') },
+             status: :created
       return
     end
 
     @flag = Flag.new(post_flag_type: type, reason: params[:reason], post_id: params[:post_id],
                      post_type: params[:post_type], user: current_user)
     if @flag.save
-      render json: { status: 'success' }, status: :created
+      render json: { status: 'success', message: I18n.t('flags.success.create_generic') },
+             status: :created
     else
-      render json: { status: 'failed', message: 'Flag failed to save.' }, status: :internal_server_error
+      render json: { status: 'failed', message: I18n.t('flags.errors.create_generic') },
+             status: :bad_request
     end
   end
 

app/models/flag.rb

@@ -21,9 +21,18 @@ class Flag < ApplicationRecord
 
   scope :escalated, -> { where(escalated: true) }
 
+  validate :maximum_reason_length
+
   # Checks if the flag is confidential as per its type
   # @return [Boolean] check result
   def confidential?
     post_flag_type&.confidential || false
   end
+
+  def maximum_reason_length
+    max_len = SiteSetting['MaxFlagReasonLength'] || 1000
+    if reason.length > [max_len, 1000].min
+      errors.add(:reason, "can't be more than #{max_len} characters")
+    end
+  end
 end

config/locales/strings/en.flags.yml

@@ -0,0 +1,14 @@
+en:
+  flags:
+    errors:
+      create_generic: >
+        Failed to raise the flag.
+      rate_limited: >
+        Thank you. Flags from people like you help us keep this site clean.
+        However, you have reached your daily flag limit of %{count} flags.
+        Please come back tomorrow to continue flagging.
+    success:
+      create_generic: >
+        <strong>Thanks!</strong> We will review your flag.
+      create_author_attention: >
+        <strong>Thanks!</strong> Your flag was added as a comment for the author to review.

db/seeds/site_settings.yml

@@ -143,6 +143,13 @@
   description: >
     A link to the full legal code of the license specified in ContentLicenseName.
 
+- name: MaxFlagReasonLength
+  value: 1000
+  value_type: integer
+  category: SiteDetails
+  description: >
+    The maximum number of characters a single flag's reason may contain. Default is 1000
+
 - name: MaxTagLength
   value: 35
   value_type: integer

global.d.ts

@@ -129,6 +129,13 @@ type QPixelComment = {
   references_comment_id: string | null
 }
 
+type QPixelFlagData = {
+  flag_type: number | null
+  post_id: string
+  post_type: 'Comment' | 'Post'
+  reason?: string
+}
+
 interface GetThreadContentOptions {
   inline?: boolean,
   showDeleted?: boolean
@@ -315,8 +322,11 @@ interface QPixel {
    * Processes JSON responses from QPixel API
    * @param data 
    * @param onSuccess callback to call for successful requests
+   * @param onFinally callback to call for all requests
    */
-  handleJSONResponse?: <T extends QPixelResponseJSON>(data: T, onSuccess: (data: T) => void) => void
+  handleJSONResponse?: <T extends QPixelResponseJSON>(data: T,
+                                                      onSuccess: (data: T) => void,
+                                                      onFinally?: (data: T) => void) => void
 
   /**
    * Attempts to delete a comment
@@ -337,7 +347,14 @@ interface QPixel {
    * @param id id of the comment thread to lock
    * @returns result of the operation
    */
-  lockThread?: (id: string) => Promise<QPixelResponseJSON>;
+  lockThread?: (id: string) => Promise<QPixelResponseJSON>
+
+  /**
+   * Attempts to raise a flag
+   * @param flag new flag data
+   * @returns result of the operation
+   */
+  flag?: (flag: QPixelFlagData) => Promise<QPixelResponseJSON>
 
   // qpixel_dom
   DOM?: QPixelDOM;

test/controllers/flags_controller_test.rb

@@ -5,22 +5,56 @@ class FlagsControllerTest < ActionController::TestCase
 
   test 'should create new post flag' do
     sign_in users(:standard_user)
-    post :new, params: { reason: 'ABCDEF GHIJKL MNOPQR STUVWX YZ', post_id: posts(:answer_two).id, post_type: 'Post' }
 
-    assert_not_nil assigns(:flag)
-    assert_not_nil assigns(:flag).post
-    assert_equal 'success', JSON.parse(response.body)['status']
+    try_create_flag(posts(:answer_two), reason: 'testing post flags')
+
     assert_response(:created)
+
+    @flag = assigns(:flag)
+    assert_not_nil @flag
+    assert_not_nil @flag.post
+    assert_equal 'success', JSON.parse(response.body)['status']
+    assert_equal I18n.t('flags.success.create_generic'), JSON.parse(response.body)['message']
   end
 
   test 'should create new comment flag' do
     sign_in users(:standard_user)
-    post :new, params: { reason: 'ABCDEF GHIJKL MNOPQR STUVWX YZ', post_id: comments(:one).id, post_type: 'Comment' }
 
-    assert_not_nil assigns(:flag)
-    assert_not_nil assigns(:flag).post
-    assert_equal 'success', JSON.parse(response.body)['status']
+    try_create_flag(comments(:one), reason: 'testing comment flags')
+
     assert_response(:created)
+
+    @flag = assigns(:flag)
+    assert_not_nil @flag
+    assert_not_nil @flag.post
+    assert_equal 'success', JSON.parse(response.body)['status']
+  end
+
+  test 'should fail to create invalid flags' do
+    sign_in users(:standard_user)
+
+    try_create_flag(posts(:question_one), reason: 'a' * 1001)
+
+    assert_response(:bad_request)
+    assert_equal 'failed', JSON.parse(response.body)['status']
+    assert_equal I18n.t('flags.errors.create_generic'), JSON.parse(response.body)['message']
+  end
+
+  test 'should fail to create flags for rate-limited users' do
+    {
+      RL_NewUserFlags: users(:basic_user),
+      RL_Flags: users(:standard_user)
+    }.each_pair do |setting, user|
+      sign_in(user)
+
+      SiteSetting[setting] = 0
+
+      try_create_flag(posts(:question_one), reason: 'testing flag rate limits')
+
+      assert_response(:forbidden)
+      assert_equal 'failed', JSON.parse(response.body)['status']
+      assert_equal I18n.t('flags.errors.rate_limited', count: 0), JSON.parse(response.body)['message']
+    end
   end
 
   test 'should retrieve flag queue' do
@@ -120,6 +154,11 @@ class FlagsControllerTest < ActionController::TestCase
 
   private
 
+  def try_create_flag(target, **opts)
+    post :new, params: { post_id: target.id,
+                         post_type: target.is_a?(Post) ? 'Post' : 'Comment' }.merge(opts)
+  end
+
   def try_resolve_flag(flag, result: nil, message: nil)
     post :resolve, params: { id: flag.id, result: result, message: message }
   end

test/models/flag_test.rb

@@ -14,4 +14,20 @@ class FlagTest < ActiveSupport::TestCase
     assert_equal normal.confidential?, false
     assert_equal secret.confidential?, true
   end
+
+  test 'flags should be correctly validated' do
+    SiteSetting['MaxFlagReasonLength'] = 500
+
+    common_attributes = {
+      post: posts(:question_one),
+      user: users(:standard_user)
+    }
+
+    too_long = Flag.new(reason: 'a' * 1000, **common_attributes)
+    assert_not too_long.valid?
+    assert too_long.errors[:reason].any?
+
+    valid = Flag.new(reason: 'my reasons are my own', **common_attributes)
+    assert valid.valid?
+  end
 end