Don't let requirements checkers claim the review (#413)

Author: aaravjhamb

app/controllers/admin/reviews_controller.rb

@@ -86,7 +86,8 @@ def leaderboard
   def show
     authorize @project, :review_screen?
 
-    @session = ensure_session(@project) if @project.pending?
+    can_review = policy(@project).review?
+    @session = ensure_session(@project) if @project.pending? && can_review
     concurrent = concurrent_active_reviewers(@project)
     claim = claim_state(@project)
     next_pending_id = next_in_queue(@project)
@@ -107,9 +108,9 @@ def show
         slack_id: current_user.slack_id
       },
       can: {
-        review: policy(@project).review?,
+        review: can_review,
         requirements_check: policy(@project).requirements_check?,
-        claim: claim[:locked_by].nil? || current_user.superadmin?
+        claim: !can_review || claim[:locked_by].nil? || current_user.superadmin?
       },
       claim: claim,
       session_stats: current_user.superadmin? ? session_stats(@project) : nil,
@@ -217,7 +218,7 @@ def claim_state(project)
           avatar: holder.reviewer.avatar,
           since: holder.started_at.iso8601
         },
-        can_take_over: current_user.superadmin?
+        can_take_over: current_user.superadmin? && policy(project).review?
       }
     end
   end

app/javascript/pages/Admin/Reviews/Show.tsx

@@ -685,7 +685,7 @@ export default function AdminReviewsShow({
           unflagging={unflagging}
         />
       )}
-      {claim.locked_by ? (
+      {claim.locked_by && !requirementsOnly ? (
         <ClaimBanner claim={claim} onTakeOver={takeOver} takingOver={takingOver} />
       ) : (
         <ConcurrentReviewersBanner reviewers={concurrent_reviewers} />

test/controllers/admin/reviews_requirements_claim_test.rb

@@ -0,0 +1,76 @@
+require "test_helper"
+
+class Admin::ReviewsRequirementsClaimTest < ActionDispatch::IntegrationTest
+  def make_user(attrs = {})
+    token = SecureRandom.hex(6)
+    User.create!({
+      avatar: "avatar",
+      display_name: "User #{token}",
+      email: "#{token}@example.com",
+      timezone: "UTC",
+      slack_id: "S#{token}",
+      hca_id: "H#{token}",
+      roles: [ "user" ]
+    }.merge(attrs))
+  end
+
+  def sign_in_as(user)
+    original = User.method(:exchange_hca_token)
+    User.define_singleton_method(:exchange_hca_token) { |*_| user }
+    get hca_callback_path, params: { code: "x" }
+  ensure
+    User.define_singleton_method(:exchange_hca_token, original)
+  end
+
+  setup do
+    @checker = make_user(roles: %w[user reviewer], permissions: %w[review_requirements], birthday: Date.new(2008, 1, 1))
+    @project = Project.create!(
+      user: make_user,
+      name: "Needs A Requirements Check",
+      tier: "tier_2",
+      status: :pending,
+      submitted_at: 1.hour.ago
+    )
+  end
+
+  test "requirements checker opening a review does not claim it" do
+    sign_in_as(@checker)
+
+    assert_no_difference "ReviewSession.count" do
+      get admin_review_path(@project)
+    end
+    assert_response :success
+  end
+
+  test "a tier reviewer opening a review still claims it" do
+    reviewer = make_user(roles: %w[user reviewer], permissions: User::ROLE_DEFAULT_PERMISSIONS["reviewer"], birthday: Date.new(2008, 1, 1))
+    sign_in_as(reviewer)
+
+    assert_difference "ReviewSession.count", 1 do
+      get admin_review_path(@project)
+    end
+    assert_response :success
+  end
+
+  test "requirements checker can still act while a reviewer holds the claim" do
+    reviewer = make_user(roles: %w[user reviewer], permissions: User::ROLE_DEFAULT_PERMISSIONS["reviewer"], birthday: Date.new(2008, 1, 1))
+    ReviewSession.create!(project: @project, reviewer: reviewer, started_at: Time.current, last_heartbeat_at: Time.current)
+
+    sign_in_as(@checker)
+    get admin_review_path(@project)
+
+    assert_equal true, inertia.props[:can][:claim]
+    assert_equal false, inertia.props[:claim][:can_take_over]
+  end
+
+  test "requirements checker cannot take over a claim" do
+    reviewer = make_user(roles: %w[user reviewer], permissions: User::ROLE_DEFAULT_PERMISSIONS["reviewer"], birthday: Date.new(2008, 1, 1))
+    ReviewSession.create!(project: @project, reviewer: reviewer, started_at: Time.current, last_heartbeat_at: Time.current)
+
+    sign_in_as(@checker)
+    post admin_claim_review_path(@project)
+
+    assert_redirected_to root_path
+    assert ReviewSession.active.for_project(@project).exists?(reviewer_id: reviewer.id)
+  end
+end