feat(comments): keep Save & Review Next inside review queue (ENGAGE-235)

met-api/src/met_api/models/comment.py

@@ -7,7 +7,7 @@
 from datetime import datetime
 from operator import or_
 
-from sqlalchemy import and_, asc, desc
+from sqlalchemy import and_, asc, case, desc
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.sql.expression import true
 from sqlalchemy.sql.schema import ForeignKey
@@ -154,19 +154,30 @@ def get_by_survey_id_paginated(
         if advanced_search_filters:
             query = cls._filter_by_advanced_filters(query, advanced_search_filters)
 
+        # Status ids do not sort into review priority on their own, so map them onto one.
+        _status_priority = case(
+            {
+                CommentStatus.Pending.value: 1,
+                CommentStatus.Needs_further_review.value: 2,
+                CommentStatus.Approved.value: 3,
+                CommentStatus.Rejected.value: 4,
+            },
+            value=Submission.comment_status_id,
+        )
         _sort_columns = {
             'id': Submission.id,
             'submission.id': Submission.id,
-            'submission.comment_status_id': Submission.comment_status_id,
-            'comment_status_id': Submission.comment_status_id,
+            'submission.comment_status_id': _status_priority,
+            'comment_status_id': _status_priority,
             'created_date': Submission.created_date,
             'reviewed_by': Submission.reviewed_by,
             'review_date': Submission.review_date,
         }
         col = _sort_columns.get(pagination_options.sort_key, Submission.id)
         sort = asc(col) if pagination_options.sort_order == 'asc' else desc(col)
 
-        query = query.order_by(sort)
+        # Secondary sort keeps rows with equal sort values stable across pages.
+        query = query.order_by(sort, Submission.id.asc())
 
         no_pagination_options = not pagination_options.page or not pagination_options.size
         if no_pagination_options:

met-api/tests/unit/api/test_submission.py

@@ -21,6 +21,7 @@
 
 import pytest
 
+from met_api.constants.comment_status import Status as CommentStatus
 from met_api.constants.membership_type import MembershipType
 from met_api.utils.enums import ContentType
 from tests.utilities.factory_scenarios import TestJwtClaims, TestSubmissionInfo
@@ -156,6 +157,37 @@ def test_get_comment_filtering(client, jwt, session):  # pylint:disable=unused-a
         'items')) == 2, 'Team Member with team membership can see unapproved and unapproved comments'
 
 
+def test_get_submission_page_sorted_by_review_priority(client, jwt, session):  # pylint:disable=unused-argument
+    """Assert that sorting by status puts Pending first and Needs further review second."""
+    claims = TestJwtClaims.staff_admin_role
+
+    participant = factory_participant_model()
+    survey, eng = factory_survey_and_eng_model()
+    # Created out of priority order so the assertion cannot pass on insertion order alone.
+    for status in (CommentStatus.Approved, CommentStatus.Rejected,
+                   CommentStatus.Needs_further_review, CommentStatus.Pending):
+        submission = factory_submission_model(
+            survey.id, eng.id, participant.id,
+            {**TestSubmissionInfo.submission1, 'comment_status_id': status.value})
+        factory_comment_model(survey.id, submission.id)
+
+    headers = factory_auth_header(jwt=jwt, claims=claims)
+    rv = client.get(
+        f'/api/submissions/survey/{survey.id}',
+        headers=headers,
+        content_type=ContentType.JSON.value,
+        query_string={'sort_key': 'submission.comment_status_id', 'sort_order': 'asc'}
+    )
+
+    assert rv.status_code == 200
+    assert [item.get('comment_status_id') for item in rv.json.get('items', [])] == [
+        CommentStatus.Pending.value,
+        CommentStatus.Needs_further_review.value,
+        CommentStatus.Approved.value,
+        CommentStatus.Rejected.value,
+    ]
+
+
 def test_invalid_submission(client, jwt, session):  # pylint:disable=unused-argument
     """Assert that an engagement can be POSTed."""
     claims = TestJwtClaims.public_user_role

met-web/src/components/admin/comments/admin/CommentReview/index.tsx

@@ -16,7 +16,8 @@ import {
     Box,
     Button,
 } from '@mui/material';
-import { getSubmission, getSubmissionPage, getSubmissionVersions, reviewComments } from 'services/submissionService';
+import { getSubmission, getSubmissionVersions, reviewComments } from 'services/submissionService';
+import { resolveNextSubmissionId } from './nextInQueue';
 import { useAppDispatch, useAppTranslation } from 'hooks';
 import { useParams, useNavigate } from 'react-router-dom';
 import { openNotification } from 'services/notificationService/notificationSlice';
@@ -76,7 +77,7 @@ const CommentReview = () => {
     const [survey, setSurvey] = useState<Survey>(createDefaultSurvey());
     const [isEditingThreatContact, setIsEditingThreatContact] = useState(false);
     const [threatContact, setThreatContact] = useState<ThreatContact | null>(null);
-    const [nextPendingId, setNextPendingId] = useState<number | null>(null);
+    const [nextSubmissionId, setNextSubmissionId] = useState<number | null>(null);
     const [versions, setVersions] = useState<SubmissionVersion[]>([]);
     const [selectedVersion, setSelectedVersion] = useState<SubmissionVersion | null>(null);
     const [versionDrawerOpen, setVersionDrawerOpen] = useState(false);
@@ -105,25 +106,6 @@ const CommentReview = () => {
         );
     };
 
-    const fetchNextPending = async (currentId: number) => {
-        try {
-            const result = await getSubmissionPage({
-                survey_id: Number(surveyId),
-                queryParams: {
-                    page: 1,
-                    size: 10,
-                    status: CommentStatus.Pending,
-                    sort_key: 'submission.id',
-                    sort_order: 'asc',
-                },
-            });
-            const next = result.items.find((s) => Number(s.id) !== currentId);
-            setNextPendingId(next?.id ?? null);
-        } catch (err) {
-            setNextPendingId(null);
-        }
-    };
-
     const fetchSubmission = async () => {
         try {
             if (isNaN(Number(submissionId))) {
@@ -133,6 +115,13 @@ const CommentReview = () => {
             const fetchedSurvey = await getSurvey(Number(surveyId));
             setSubmission(fetchedSubmission);
             setSurvey(fetchedSurvey);
+            setNextSubmissionId(
+                await resolveNextSubmissionId(
+                    Number(surveyId),
+                    Number(submissionId),
+                    fetchedSubmission.comment_status_id,
+                ),
+            );
             setHasOtherReason(!!fetchedSubmission.rejected_reason_other);
             setOtherReason(fetchedSubmission.rejected_reason_other ?? '');
             setHasPersonalInfo(fetchedSubmission.has_personal_info ?? false);
@@ -166,9 +155,8 @@ const CommentReview = () => {
     };
 
     useEffect(() => {
-        const currentId = Number(submissionId);
         setIsLoading(true);
-        setNextPendingId(null);
+        setNextSubmissionId(null);
         setReview(CommentStatus.Approved);
         setHasOtherReason(false);
         setOtherReason('');
@@ -181,7 +169,6 @@ const CommentReview = () => {
         setSelectedVersion(null);
         setVersionDrawerOpen(false);
         fetchSubmission();
-        fetchNextPending(currentId);
     }, [submissionId]);
 
     useEffect(() => {
@@ -272,7 +259,7 @@ const CommentReview = () => {
             });
             setIsSaving(false);
             dispatch(openNotification({ severity: 'success', text: 'Comments successfully reviewed.' }));
-            navigate(`/surveys/${surveyId}/submissions/${nextPendingId}/review`);
+            navigate(`/surveys/${surveyId}/submissions/${nextSubmissionId}/review`);
         } catch (error) {
             dispatch(openNotification({ severity: 'error', text: 'Error occurred while sending comments review.' }));
             setIsSaving(false);
@@ -900,7 +887,7 @@ const CommentReview = () => {
                         </When>
                         <Grid item xs={12}>
                             <Stack direction="row" spacing={2}>
-                                {nextPendingId !== null ? (
+                                {nextSubmissionId !== null ? (
                                     <>
                                         <PrimaryButton loading={isSaving} onClick={handleSaveAndNext}>
                                             {'Save & Review Next'}

met-web/src/components/admin/comments/admin/CommentReview/nextInQueue.ts

@@ -0,0 +1,59 @@
+import { getSubmissionPage } from 'services/submissionService';
+import { CommentStatus } from 'constants/commentStatus';
+
+// A queue is the set of submissions a reviewer works through without leaving the review page.
+// Needs Further Review is ordered by review_date so a comment that is re-marked as
+// Needs Further Review lands at the back of its own queue instead of being served again.
+const PENDING_QUEUE = { status: CommentStatus.Pending, sort_key: 'submission.id' } as const;
+const NEEDS_FURTHER_REVIEW_QUEUE = {
+    status: CommentStatus.NeedsFurtherReview,
+    sort_key: 'review_date',
+} as const;
+
+const firstInQueue = async (
+    surveyId: number,
+    currentId: number,
+    queue: typeof PENDING_QUEUE | typeof NEEDS_FURTHER_REVIEW_QUEUE,
+): Promise<number | null> => {
+    const result = await getSubmissionPage({
+        survey_id: surveyId,
+        // Two rows is enough: at most one of them can be the submission being reviewed.
+        queryParams: { page: 1, size: 2, status: queue.status, sort_key: queue.sort_key, sort_order: 'asc' },
+    });
+    const next = result.items.find((submission) => Number(submission.id) !== currentId);
+    return next?.id ?? null;
+};
+
+/**
+ * Resolve the submission the 'Save & Review Next' button should navigate to.
+ *
+ * The reviewer stays in the queue they entered, and falls through to the other queue once
+ * theirs is empty. Returns null when neither queue has anything left, which hides the button.
+ */
+export const resolveNextSubmissionId = async (
+    surveyId: number,
+    currentId: number,
+    openedStatus: number,
+): Promise<number | null> => {
+    let queues: (typeof PENDING_QUEUE | typeof NEEDS_FURTHER_REVIEW_QUEUE)[];
+    if (openedStatus === CommentStatus.NeedsFurtherReview) {
+        queues = [NEEDS_FURTHER_REVIEW_QUEUE, PENDING_QUEUE];
+    } else if (openedStatus === CommentStatus.Pending) {
+        queues = [PENDING_QUEUE, NEEDS_FURTHER_REVIEW_QUEUE];
+    } else {
+        // Already approved or rejected - the comment is not part of a review queue.
+        return null;
+    }
+
+    try {
+        for (const queue of queues) {
+            const next = await firstInQueue(surveyId, currentId, queue);
+            if (next !== null) {
+                return next;
+            }
+        }
+        return null;
+    } catch (err) {
+        return null;
+    }
+};

met-web/tests/unit/components/comments/nextInQueue.test.ts

@@ -0,0 +1,83 @@
+import '@testing-library/jest-dom';
+import { resolveNextSubmissionId } from 'components/admin/comments/admin/CommentReview/nextInQueue';
+import * as submissionService from 'services/submissionService';
+import { CommentStatus } from 'constants/commentStatus';
+
+const mockGetSubmissionPage = jest.spyOn(submissionService, 'getSubmissionPage');
+
+const page = (...ids: number[]) => ({ items: ids.map((id) => ({ id })), total: ids.length } as never);
+
+const CURRENT_ID = 5;
+const SURVEY_ID = 1;
+
+beforeEach(() => {
+    jest.clearAllMocks();
+});
+
+describe('resolveNextSubmissionId', () => {
+    test('a Needs Further Review comment stays in the Needs Further Review queue', async () => {
+        mockGetSubmissionPage.mockResolvedValueOnce(page(CURRENT_ID, 9));
+
+        const next = await resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.NeedsFurtherReview);
+
+        expect(next).toBe(9);
+        expect(mockGetSubmissionPage).toHaveBeenCalledTimes(1);
+        expect(mockGetSubmissionPage.mock.calls[0][0].queryParams).toMatchObject({
+            status: CommentStatus.NeedsFurtherReview,
+            sort_key: 'review_date',
+            sort_order: 'asc',
+        });
+    });
+
+    test('falls through to Pending once the Needs Further Review queue is exhausted', async () => {
+        mockGetSubmissionPage.mockResolvedValueOnce(page(CURRENT_ID)).mockResolvedValueOnce(page(12));
+
+        const next = await resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.NeedsFurtherReview);
+
+        expect(next).toBe(12);
+        expect(mockGetSubmissionPage.mock.calls[1][0].queryParams).toMatchObject({
+            status: CommentStatus.Pending,
+            sort_key: 'submission.id',
+        });
+    });
+
+    test('a Pending comment stays in the Pending queue', async () => {
+        mockGetSubmissionPage.mockResolvedValueOnce(page(CURRENT_ID, 7));
+
+        const next = await resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Pending);
+
+        expect(next).toBe(7);
+        expect(mockGetSubmissionPage.mock.calls[0][0].queryParams).toMatchObject({
+            status: CommentStatus.Pending,
+        });
+    });
+
+    test('falls through to Needs Further Review once the Pending queue is exhausted', async () => {
+        mockGetSubmissionPage.mockResolvedValueOnce(page()).mockResolvedValueOnce(page(3));
+
+        const next = await resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Pending);
+
+        expect(next).toBe(3);
+        expect(mockGetSubmissionPage.mock.calls[1][0].queryParams).toMatchObject({
+            status: CommentStatus.NeedsFurtherReview,
+        });
+    });
+
+    test('returns null when both queues are empty', async () => {
+        mockGetSubmissionPage.mockResolvedValue(page());
+
+        await expect(resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Pending)).resolves.toBeNull();
+    });
+
+    test('returns null without calling the API for an already reviewed comment', async () => {
+        await expect(resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Approved)).resolves.toBeNull();
+        await expect(resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Rejected)).resolves.toBeNull();
+        expect(mockGetSubmissionPage).not.toHaveBeenCalled();
+    });
+
+    test('returns null when the lookup fails', async () => {
+        mockGetSubmissionPage.mockRejectedValueOnce('boom');
+
+        await expect(resolveNextSubmissionId(SURVEY_ID, CURRENT_ID, CommentStatus.Pending)).resolves.toBeNull();
+    });
+});