Updates based on code review feedback

- The name of a variable in the Term model test suite has been updated
  to hopefully be clearer about its use.

- The confirmation workflow is now restrictd to already-categorized
  terms for non-admin users. Admins will still have an ability to load
  all terms for this workflow, but as noted this is not the smoothest
  workflow yet.

Side note: the restriction being enacted here is on the ability to load
the _index_ in a specific state, but the actual confirmation form is not
affected. A non-admin user who understands the URL structure will be
able to guess the right URL for a given term (or pick one randomly), and
submit a confirmation for a term that has not been categorized.

I believe this is an acceptable risk, as the impact of such a user is
that we'd have some extra confirmation records in the system - which are
useful only for adjusting future operations, so I struggle to think of
a downside to having some extras.

Author: matt-bernhardt

app/controllers/term_controller.rb

@@ -8,6 +8,7 @@ class TermController < ApplicationController
   # confirmed.
   def unconfirmed
     terms = if params[:show] == 'all'
+              authorize! :confirm_uncategorized, Term
               Term.user_unconfirmed
             else
               Term.categorized.user_unconfirmed

app/models/ability.rb

@@ -40,6 +40,7 @@ def initialize(user)
     # Rules for admins
     return unless user.admin?
 
+    can :confirm_uncategorized, Term
     can :manage, :all
   end
 end

app/views/term/unconfirmed.html.erb

@@ -3,10 +3,12 @@
 <p>The terms listed here have not yet been reviewed by a person and placed into a category. Clicking on any term will
 take you to the form for submitting this information.</p>
 
+<% if can? :confirm_uncategorized, Term %>
 <p class="wrap-filters">View:
   <a class="btn button-small button-secondary" href="<%= terms_unconfirmed_path %>">Categorized terms</a>
   <a class="btn button-small button-secondary" href="<%= terms_unconfirmed_path(show: 'all') %>">All terms</a>
 </p>
+<% end %>
 
 <p><%== pagy_info(@pagy) %></p>
 

test/controllers/term_controller_test.rb

@@ -26,10 +26,41 @@ class TermControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
-  test 'confirmation index can show two different sets of terms' do
+  test 'basic users see only one confirmation option' do
     sign_in users(:basic)
     get terms_unconfirmed_path
 
+    assert_select 'p.wrap-filters', text: "View:\n  Categorized terms\n  All terms", count: 0
+  end
+
+  test 'admin users see two confirmation options' do
+    sign_in users(:admin)
+    get terms_unconfirmed_path
+
+    assert_select 'p.wrap-filters', text: "View:\n  Categorized terms\n  All terms", count: 1
+  end
+
+  test 'basic users cannot access the confirmation option for uncategorized terms' do
+    sign_in users(:basic)
+    get terms_unconfirmed_path(show: 'all')
+
+    assert_redirected_to '/'
+    follow_redirect!
+
+    assert_select 'div.alert', text: 'Not authorized.', count: 1
+  end
+
+  test 'admin users can access the confirmation option for uncategorized terms' do
+    sign_in users(:admin)
+    get terms_unconfirmed_path(show: 'all')
+
+    assert_response :success
+  end
+
+  test 'confirmation index can show two different sets of terms for admin users' do
+    sign_in users(:admin)
+    get terms_unconfirmed_path
+
     # default_pagy will be something like "Displaying 10 items"
     default_pagy = response.parsed_body.xpath('//main//span').first.text
     default_pagy_count = default_pagy.split.second.to_i

test/models/term_test.rb

@@ -306,11 +306,10 @@ class TermTest < ActiveSupport::TestCase
   test 'categorized scope accounts for terms with multiple categorizations' do
     categorized_count = Term.categorized.count
     t = terms('doi')
-
-    term_category_count = t.categorizations.count
+    orig_categorization_count = t.categorizations.count
 
     # term has been categorized already
-    assert_operator 1, :<=, term_category_count
+    assert_operator 1, :<=, orig_categorization_count
 
     new_record = {
       term: t,
@@ -321,7 +320,7 @@ class TermTest < ActiveSupport::TestCase
     Categorization.create!(new_record)
 
     # The term has gained a category, but the categorized scope has not changed size.
-    assert_operator term_category_count, :<, t.categorizations.count
+    assert_operator orig_categorization_count, :<, t.categorizations.count
     assert_equal categorized_count, Term.categorized.count
   end