Merge pull request #1846 from codidact/0valt/flag-sort

 Sorting options for flags

Author: Oaphi

app/assets/stylesheets/flags.scss

@@ -0,0 +1,15 @@
+.flags-list-header {
+  display: flex;
+  justify-content: space-between;
+}
+
+.header {
+  .header--menu {
+    .header--item {
+      .header--alert {
+        min-width: 2em;
+        padding: 0 0.5em;
+      }
+    }
+  }
+}

app/controllers/flags_controller.rb

@@ -4,6 +4,7 @@ class FlagsController < ApplicationController
   before_action :verify_moderator, only: [:queue, :handled, :escalate]
   before_action :verify_admin, only: [:escalated_queue]
   before_action :flag_verify, only: [:resolve]
+  before_action :set_sorting, only: [:escalated_queue, :queue, :handled]
 
   def new
     type = if params[:flag_type].present?
@@ -51,12 +52,26 @@ def history
     @statuses = @flags.group(:status).count(:status)
   end
 
+  def escalated_queue
+    @flags = Flag.unhandled
+                 .includes(:post, :user)
+                 .where(escalated: true)
+                 .order(@sort_type => @sort_order)
+                 .paginate(page: params[:page], per_page: 20)
+    render :queue
+  end
+
   def queue
-    @flags = Flag.unhandled.includes(:post, :user).paginate(page: params[:page], per_page: 20)
+    @flags = Flag.unhandled
+                 .includes(:post, :user)
+                 .order(@sort_type => @sort_order)
+                 .paginate(page: params[:page], per_page: 20)
   end
 
   def handled
-    @flags = Flag.handled.includes(:post, :user, :handled_by).newest_first
+    @flags = Flag.handled
+                 .includes(:post, :user, :handled_by)
+                 .order(@sort_type => @sort_order)
                  .paginate(page: params[:page], per_page: 50)
   end
 
@@ -78,11 +93,6 @@ def escalate
     render json: { status: 'success' }
   end
 
-  def escalated_queue
-    @flags = Flag.unhandled.includes(:post, :user).where(escalated: true).paginate(page: params[:page], per_page: 20)
-    render :queue
-  end
-
   private
 
   def flag_verify
@@ -110,4 +120,20 @@ def create_as_feedback_comment(post, user, content)
     end
     comment
   end
+
+  def set_sorting
+    sort_orders = { asc: :asc, desc: :desc }
+    sort_types = { age: :created_at,
+                   escalated: :escalated_at,
+                   handled: :handled_at }
+
+    @default_sort_type, @default_sort_order = case params[:action]
+                                              when 'escalated_queue' then [:escalated, :desc]
+                                              when 'handled' then [:handled, :desc]
+                                              else [:age, :asc]
+                                              end
+
+    @sort_order = sort_orders[params[:order]&.to_sym] || sort_orders[@default_sort_order]
+    @sort_type = sort_types[params[:sort]&.to_sym] || sort_types[@default_sort_type]
+  end
 end

app/views/flags/_flag.html.erb

@@ -1,11 +1,11 @@
 <%#
-    Single flag handling view.
+   "Single flag handling view.
 
-    Params:
-     * flag : the flag to display
-     * controls : boolean, whether or not to display handling controls - true by default
-     * escalation : boolean, if true will display escalation comment - false by default
-%>
+    Variables:
+      flag : the flag to display
+      controls : boolean, whether or not to display handling controls - true by default
+      escalation : boolean, if true will display escalation comment - false by default
+"%>
 
 <% controls ||= defined?(controls) && !controls.nil? ? controls : true %>
 <% escalation ||= defined?(escalation) && !escalation.nil? ? escalation : false %>
@@ -36,7 +36,7 @@
         <%= user_link flag.escalated_by, { host: flag.community.host } %> at <%= flag.escalated_at.iso8601 %>
       </p>
       <p>
-	      <strong>Attention</strong>: The reply to the flag will be shown to the flagger, not the mod escalating this
+        <strong>Attention</strong>: The reply to the flag will be shown to the flagger, not the mod escalating this
         flag. Do not share sensitive, mod-only information.
       </p>
     </div>

app/views/flags/handled.html.erb

@@ -3,12 +3,18 @@
 <h1>Handled Flags</h1>
 <p>The following list contains flags that have already been handled. You can't change the outcome of these any more.</p>
 
-<div class="button-list is-gutterless">
-  <%= link_to 'Active', flag_queue_path, class: 'button is-muted is-outlined' %>
-  <%= link_to 'Handled', handled_flags_path, class: 'button is-muted is-outlined is-active' %>
-  <% if admin? %>
-    <%= link_to 'Escalated', escalated_flags_path, class: 'button is-muted is-outlined' %>
-  <% end %>
+<div class="flags-list-header">
+  <div class="button-list is-gutterless">
+    <%= link_to 'Active', flag_queue_path, class: 'button is-muted is-outlined' %>
+    <%= link_to 'Handled', handled_flags_path, class: 'button is-muted is-outlined is-active' %>
+    <% if admin? %>
+      <%= link_to 'Escalated', escalated_flags_path, class: 'button is-muted is-outlined' %>
+    <% end %>
+  </div>
+
+  <%= render 'shared/sorting', default_order: @default_sort_order,
+                               default_type: @default_sort_type,
+                               types: ['age', { type: 'handled', title: 'time handled' }] %>
 </div>
 
 <% @flags.each do |flag| %>

app/views/flags/queue.html.erb

@@ -8,14 +8,24 @@
   moderator attention; use that to help you determine what needs to be done.</p>
 <p>You can mark a flag helpful even if you take no action. If a post was edited after the flag was raised, for example, the problem might already be fixed.</p>
 
-<div class="button-list is-gutterless">
-  <%= link_to 'Active', flag_queue_path,
-              class: "button is-muted is-outlined #{current_page?(flag_queue_path) ? 'is-active' : ''}" %>
-  <%= link_to 'Handled', handled_flags_path, class: 'button is-muted is-outlined' %>
-  <% if admin? %>
-    <%= link_to 'Escalated', escalated_flags_path,
-                class: "button is-muted is-outlined #{current_page?(escalated_flags_path) ? 'is-active' : ''}" %>
-  <% end %>
+<div class="flags-list-header">
+  <div class="button-list is-gutterless">
+    <%= link_to 'Active', flag_queue_path,
+              class: "button is-muted is-outlined #{'is-active' if current_page?(flag_queue_path)}" %>
+    <%= link_to 'Handled', handled_flags_path, class: 'button is-muted is-outlined' %>
+    <% if admin? %>
+      <%= link_to 'Escalated', escalated_flags_path,
+                class: "button is-muted is-outlined #{'is-active' if current_page?(escalated_flags_path)}" %>
+    <% end %>
+  </div>
+
+  <% sorting_types = current_page?(escalated_flags_path) ?
+                      ['age', { type: 'escalated', title: 'time escalated' }] :
+                      %w[age] %>
+
+  <%= render 'shared/sorting', default_order: @default_sort_order,
+                               default_type: @default_sort_type,
+                               types: sorting_types %>
 </div>
 
 <% @flags.each do |flag| %>

app/views/shared/_sorting.html.erb

@@ -2,7 +2,7 @@
    "Renders a sorting widget
 
     Variables:
-      types : Array<String> of available sorting types
+      types         : list of available sorting types
     ? default_order : Defualt sorting order, if any
     ? default_type  : Default sorting type, if any
 "%>
@@ -16,13 +16,15 @@
 
 <div class="button-list is-gutterless">
 
-  <% types.each do |type| %>
+  <% types.each do |option| %>
+    <% type = option.is_a?(String) ? option : option[:type] %>
+    <% title = "Sort by #{option.is_a?(String) ? option.humanize.downcase : option[:title]}" %>
     <% is_active = params[:sort] == type || (params[:sort].nil? && default_type.to_s == type) %>
     <%= link_to request.params.merge(sort: type, order: is_active ? reverse_order : nil),
                 class: "button is-muted is-outlined #{'is-active' if is_active}",
                 role: 'button',
-                title: "Sort by #{type.humanize.downcase}",
-                aria: { label: "Sort by #{type.humanize.downcase}" } do %>
+                title: title,
+                aria: { label: title } do %>
       <% if is_active %>
         <i class="has-margin-right-1 fa fa-caret-<%= order == :asc ? 'up' : 'down' %>"></i> <%= type.humanize %>
       <% else %>

test/controllers/flags_controller_test.rb

@@ -57,11 +57,40 @@ class FlagsControllerTest < ActionController::TestCase
     end
   end
 
-  test 'should retrieve flag queue' do
+  test 'flag queues should require authentication' do
+    [:escalated_queue, :handled, :queue].each do |action|
+      get action
+      assert_redirected_to_sign_in
+    end
+  end
+
+  test 'unprivileged users should not be able to access flag queues' do
+    sign_in users(:standard_user)
+
+    [:escalated_queue, :handled, :queue].each do |action|
+      get action
+      assert_response(:not_found)
+    end
+  end
+
+  test 'moderators should be able to access flag queues' do
     sign_in users(:moderator)
-    get :queue
-    assert_not_nil assigns(:flags)
+
+    [:handled, :queue].each do |action|
+      get action
+      assert_response(:success)
+      assert_not_nil assigns(:flags)
+      # TODO: add assertions for correct flag types
+    end
+  end
+
+  test 'admins should be able to access escalated flag queues' do
+    sign_in users(:admin)
+
+    get :escalated_queue
+
     assert_response(:success)
+    assert_not_nil assigns(:flags)
   end
 
   test 'should add status to flag' do
@@ -82,18 +111,6 @@ class FlagsControllerTest < ActionController::TestCase
     assert_response(:found)
   end
 
-  test 'should require authentication to get queue' do
-    sign_out :user
-    get :queue
-    assert_response(:found)
-  end
-
-  test 'should require moderator status to get queue' do
-    sign_in users(:standard_user)
-    get :queue
-    assert_response(:not_found)
-  end
-
   test 'should require authentication to resolve flag' do
     sign_out :user
     try_resolve_flag(flags(:one))
@@ -118,24 +135,6 @@ class FlagsControllerTest < ActionController::TestCase
     assert_response(:not_found)
   end
 
-  test 'should get handled flags list' do
-    sign_in users(:moderator)
-    get :handled
-    assert_response(:success)
-    assert_not_nil assigns(:flags)
-  end
-
-  test 'should require authentication to get handled flags list' do
-    get :handled
-    assert_response(:found)
-  end
-
-  test 'should require moderator status to get handled flags list' do
-    sign_in users(:standard_user)
-    get :handled
-    assert_response(:not_found)
-  end
-
   test 'non-moderator users should only see their flag history' do
     mod_user = users(:moderator)
     std_user = users(:standard_user)