fix: drop stale pending reviews and scope assignment toggle to super admins (#161)

Co-authored-by: Caleb Bae <baecal000@gmail.com>
(cherry picked from commit 5575463)

Author: devin-ai-integration[bot]

cmd/api/reviews.go

@@ -167,19 +167,31 @@ func (app *application) batchAssignReviews(w http.ResponseWriter, r *http.Reques
 // getNextReview assigns and returns the next application needing review
 //
 //	@Summary		Get next review assignment (Admin)
-//	@Description	Automatically assigns the next submitted application needing review to the current admin and returns it
+//	@Description	Automatically assigns the next submitted application needing review to the current admin and returns it. Super admins who have disabled their review assignment toggle are refused.
 //	@Tags			admin/reviews
 //	@Produce		json
 //	@Success		200	{object}	ReviewResponse
 //	@Failure		401	{object}	object{error=string}
-//	@Failure		403	{object}	object{error=string}
+//	@Failure		403	{object}	object{error=string}	"Review assignment disabled for this super admin"
 //	@Failure		404	{object}	object{error=string}	"No applications need review"
 //	@Failure		500	{object}	object{error=string}
 //	@Security		CookieAuth
 //	@Router			/admin/reviews/next [get]
 func (app *application) getNextReview(w http.ResponseWriter, r *http.Request) {
 	user := getUserFromContext(r.Context())
 
+	if user.Role == store.RoleSuperAdmin {
+		enabled, err := app.store.Settings.GetReviewAssignmentToggle(r.Context(), user.ID)
+		if err != nil {
+			app.internalServerError(w, r, err)
+			return
+		}
+		if !enabled {
+			app.forbiddenResponse(w, r, errors.New("review assignment is disabled for this account"))
+			return
+		}
+	}
+
 	reviewsPerApp, err := app.store.Settings.GetReviewsPerApplication(r.Context())
 	if err != nil {
 		app.internalServerError(w, r, err)

cmd/api/reviews_test.go

@@ -341,6 +341,22 @@ func TestGetNextReview(t *testing.T) {
 		mockReviews.AssertExpectations(t)
 		mockSettings.AssertExpectations(t)
 	})
+
+	t.Run("should return 403 for a super admin with assignment disabled", func(t *testing.T) {
+		superAdmin := newSuperAdminUser()
+
+		mockSettings.On("GetReviewAssignmentToggle", superAdmin.ID).Return(false, nil).Once()
+
+		req, err := http.NewRequest(http.MethodGet, "/", nil)
+		require.NoError(t, err)
+		req = setUserContext(req, superAdmin)
+
+		rr := executeRequest(req, http.HandlerFunc(app.getNextReview))
+		checkResponseCode(t, http.StatusForbidden, rr.Code)
+
+		mockReviews.AssertNotCalled(t, "AssignNextForAdmin", superAdmin.ID, 3)
+		mockSettings.AssertExpectations(t)
+	})
 }
 
 func TestBatchAssignReviews(t *testing.T) {

docs/docs.go

@@ -1101,7 +1101,7 @@ const docTemplate = `{
                         "CookieAuth": []
                     }
                 ],
-                "description": "Automatically assigns the next submitted application needing review to the current admin and returns it",
+                "description": "Automatically assigns the next submitted application needing review to the current admin and returns it. Super admins who have disabled their review assignment toggle are refused.",
                 "produces": [
                     "application/json"
                 ],
@@ -1128,7 +1128,7 @@ const docTemplate = `{
                         }
                     },
                     "403": {
-                        "description": "Forbidden",
+                        "description": "Review assignment disabled for this super admin",
                         "schema": {
                             "type": "object",
                             "properties": {

internal/store/reviews.go

@@ -131,7 +131,8 @@ func (s *ApplicationReviewsStore) GetTravelStatusByReviewID(ctx context.Context,
 }
 
 // GetPendingByAdminID returns all reviews assigned to an admin that haven't been voted on yet,
-// including application details for display
+// including application details for display. Reviews on applications that have
+// already been decided are omitted; the next BatchAssign removes them.
 func (s *ApplicationReviewsStore) GetPendingByAdminID(ctx context.Context, adminID string) ([]ApplicationReviewWithDetails, error) {
 	ctx, cancel := context.WithTimeout(ctx, QueryTimeoutDuration)
 	defer cancel()
@@ -155,7 +156,7 @@ func (s *ApplicationReviewsStore) GetPendingByAdminID(ctx context.Context, admin
 		FROM application_reviews ar
 		JOIN applications a ON ar.application_id = a.id
 		JOIN users u ON a.user_id = u.id
-		WHERE ar.admin_id = $1 AND ar.vote IS NULL
+		WHERE ar.admin_id = $1 AND ar.vote IS NULL AND a.status = 'submitted'
 		ORDER BY ar.assigned_at ASC
 	`
 
@@ -295,8 +296,11 @@ type BatchAssignmentResult struct {
 	ReviewsUnfilled         int `json:"reviews_unfilled"`
 }
 
-// BatchAssign recovers inaccessible pending reviews and fills submitted
+// BatchAssign recovers pending reviews that can no longer be acted on (reviewer
+// disabled or demoted, application already decided) and fills submitted
 // applications' assignment targets with distinct, currently eligible reviewers.
+// The assignment toggle only applies to super admins; entries for users who
+// no longer hold that role are dropped so a demoted user is a regular admin.
 func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp int) (*BatchAssignmentResult, error) {
 	ctx, cancel := context.WithTimeout(ctx, QueryTimeoutDuration*2)
 	defer cancel()
@@ -336,10 +340,14 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp
 	result := &BatchAssignmentResult{ReviewsPerApplication: reviewsPerApp}
 	removed, err := tx.ExecContext(ctx, `
 		DELETE FROM application_reviews ar
-		WHERE ar.vote IS NULL AND (
-			ar.admin_id::text = ANY($1::text[]) OR NOT EXISTS (
+		USING applications a
+		WHERE a.id = ar.application_id AND ar.vote IS NULL AND (
+			a.status <> 'submitted' OR NOT EXISTS (
 				SELECT 1 FROM users u
-				WHERE u.id = ar.admin_id AND u.role IN ('admin', 'super_admin')
+				WHERE u.id = ar.admin_id AND (
+					u.role = 'admin' OR
+					(u.role = 'super_admin' AND NOT (u.id::text = ANY($1::text[])))
+				)
 			)
 		)
 	`, disabledIDs)
@@ -354,7 +362,8 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp
 
 	// Read workloads after cleanup. Creation time and ID provide stable ties.
 	adminRows, err := tx.QueryContext(ctx, `
-		SELECT u.id, u.role, COUNT(ar.id), NOT (u.id::text = ANY($1::text[]))
+		SELECT u.id, u.role, COUNT(ar.id),
+			NOT (u.role = 'super_admin' AND u.id::text = ANY($1::text[]))
 		FROM users u
 		LEFT JOIN application_reviews ar ON ar.admin_id = u.id AND ar.vote IS NULL
 		WHERE u.role IN ('admin', 'super_admin')
@@ -370,15 +379,19 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp
 		Pending int
 	}
 	var admins []reviewer
+	superAdmins := make(map[string]bool)
 	for adminRows.Next() {
 		var admin reviewer
 		var role UserRole
 		var enabled bool
 		if err := adminRows.Scan(&admin.ID, &role, &admin.Pending, &enabled); err != nil {
 			return nil, err
 		}
-		if role == RoleSuperAdmin && !listed[admin.ID] {
-			entries = append(entries, ReviewAssignmentEntry{ID: admin.ID, Enabled: true})
+		if role == RoleSuperAdmin {
+			superAdmins[admin.ID] = true
+			if !listed[admin.ID] {
+				entries = append(entries, ReviewAssignmentEntry{ID: admin.ID, Enabled: true})
+			}
 		}
 		if enabled {
 			admins = append(admins, admin)
@@ -389,7 +402,15 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp
 	}
 	adminRows.Close()
 
-	// Normalize legacy settings and retain the super-admin backfill.
+	// Normalize legacy settings, retain the super-admin backfill, and drop
+	// entries for users who are no longer super admins.
+	current := entries[:0]
+	for _, entry := range entries {
+		if superAdmins[entry.ID] {
+			current = append(current, entry)
+		}
+	}
+	entries = current
 	encoded, err := json.Marshal(entries)
 	if err != nil {
 		return nil, err

internal/store/reviews_assignment_integration_test.go

@@ -292,8 +292,11 @@ func TestIntegrationBatchAssign(t *testing.T) {
 					if _, err := (&UsersStore{db: db}).UpdateRole(ctx, admins[0], RoleHacker); err != nil {
 						t.Fatal(err)
 					}
-				} else if err := (&SettingsStore{db: db}).SetReviewAssignmentToggle(ctx, admins[0], false); err != nil {
-					t.Fatal(err)
+				} else {
+					batchTestExec(t, db, "UPDATE users SET role='super_admin' WHERE id=$1", admins[0])
+					if err := (&SettingsStore{db: db}).SetReviewAssignmentToggle(ctx, admins[0], false); err != nil {
+						t.Fatal(err)
+					}
 				}
 				r, err := s.BatchAssign(ctx, 2)
 				if err != nil {
@@ -352,6 +355,71 @@ func TestIntegrationBatchAssign(t *testing.T) {
 			}
 		}
 	})
+	t.Run("decided_application_pending_reviews_removed", func(t *testing.T) {
+		db, s, admins, apps := batchTestSeed(t, 2, 2)
+		batchTestBatch(t, s, 2)
+		pending, err := s.GetPendingByAdminID(ctx, admins[0])
+		if err != nil || len(pending) != 2 {
+			t.Fatalf("pending=%d err=%v", len(pending), err)
+		}
+		var decidedReview string
+		for _, p := range pending {
+			if p.ApplicationID == apps[0] {
+				decidedReview = p.ID
+			}
+		}
+		if _, err := s.SubmitVote(ctx, decidedReview, admins[0], ReviewVoteAccept, nil, nil); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := (&ApplicationsStore{db: db}).SetStatus(ctx, apps[0], StatusAccepted); err != nil {
+			t.Fatal(err)
+		}
+		// The queue hides the decided application before any batch runs.
+		for _, admin := range admins {
+			pending, err := s.GetPendingByAdminID(ctx, admin)
+			if err != nil || len(pending) != 1 || pending[0].ApplicationID != apps[1] {
+				t.Fatalf("admin queue after decision=%+v err=%v", pending, err)
+			}
+		}
+		r, err := s.BatchAssign(ctx, 2)
+		if err != nil {
+			t.Fatal(err)
+		}
+		want := BatchAssignmentResult{ReviewsRemoved: 1, ReviewsPerApplication: 2}
+		if *r != want {
+			t.Errorf("result=%+v, want %+v", *r, want)
+		}
+		if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE application_id=$1 AND vote IS NOT NULL", apps[0]); n != 1 {
+			t.Errorf("completed reviews on decided application=%d, want 1", n)
+		}
+		if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE application_id=$1 AND vote IS NULL", apps[0]); n != 0 {
+			t.Errorf("pending reviews on decided application=%d, want 0", n)
+		}
+	})
+	t.Run("demoted_disabled_super_admin_becomes_eligible_admin", func(t *testing.T) {
+		db, s, admins, _ := batchTestSeed(t, 2, 2)
+		batchTestExec(t, db, "UPDATE users SET role='super_admin' WHERE id=$1", admins[0])
+		settings := &SettingsStore{db: db}
+		if err := settings.SetReviewAssignmentToggle(ctx, admins[0], false); err != nil {
+			t.Fatal(err)
+		}
+		if n := batchTestBatch(t, s, 2); n != 2 {
+			t.Fatalf("created=%d, want 2 (only the enabled admin)", n)
+		}
+		if _, err := (&UsersStore{db: db}).UpdateRole(ctx, admins[0], RoleAdmin); err != nil {
+			t.Fatal(err)
+		}
+		if n := batchTestBatch(t, s, 2); n != 2 {
+			t.Errorf("created=%d, want 2 for the demoted reviewer", n)
+		}
+		if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE admin_id=$1", admins[0]); n != 2 {
+			t.Errorf("demoted reviewer assignments=%d, want 2", n)
+		}
+		entries, err := settings.GetAllReviewAssignmentToggles(ctx)
+		if err != nil || len(entries) != 0 {
+			t.Errorf("stale toggle entries=%+v err=%v", entries, err)
+		}
+	})
 	t.Run("simultaneous_batches", func(t *testing.T) {
 		for _, absent := range []bool{false, true} {
 			t.Run(fmt.Sprint(absent), func(t *testing.T) {