From b5ec50ecb62d568b28ce0b87d4c42a68d4dd63b5 Mon Sep 17 00:00:00 2001 From: Corey Koelewyn Date: Mon, 27 Jul 2026 14:12:39 -0700 Subject: [PATCH] feat(comments): keep Save & Review Next inside review queue (ENGAGE-235) Save & Review Next always jumped to the next Pending comment, so staff reviewing Needs Further Review had to Save & Close and re-apply the status filter to reach each subsequent one. The button now resolves the next comment from the queue the reviewer entered, falling through to the other queue once theirs is empty and disappearing when both are. Needs Further Review is ordered by review_date, so a comment re-marked as Needs Further Review goes to the back of its own queue. The button is hidden for already approved or rejected comments. Comment listing status sort now maps status ids onto a review priority (Pending, Needs Further Review, Approved, Rejected) rather than relying on their numeric order, and takes a secondary sort on submission id so rows with equal status keep a stable order across pages. Ref ENGAGE-235 --- met-api/src/met_api/models/comment.py | 19 ++++- met-api/tests/unit/api/test_submission.py | 32 +++++++ .../comments/admin/CommentReview/index.tsx | 39 +++------ .../admin/CommentReview/nextInQueue.ts | 59 +++++++++++++ .../components/comments/nextInQueue.test.ts | 83 +++++++++++++++++++ 5 files changed, 202 insertions(+), 30 deletions(-) create mode 100644 met-web/src/components/admin/comments/admin/CommentReview/nextInQueue.ts create mode 100644 met-web/tests/unit/components/comments/nextInQueue.test.ts diff --git a/met-api/src/met_api/models/comment.py b/met-api/src/met_api/models/comment.py index c17a9edbf..b64df8c64 100644 --- a/met-api/src/met_api/models/comment.py +++ b/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,11 +154,21 @@ 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, @@ -166,7 +176,8 @@ def get_by_survey_id_paginated( 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: diff --git a/met-api/tests/unit/api/test_submission.py b/met-api/tests/unit/api/test_submission.py index e73d3adfb..06ac392f3 100644 --- a/met-api/tests/unit/api/test_submission.py +++ b/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 diff --git a/met-web/src/components/admin/comments/admin/CommentReview/index.tsx b/met-web/src/components/admin/comments/admin/CommentReview/index.tsx index 8ff9f91c6..9dff67ada 100644 --- a/met-web/src/components/admin/comments/admin/CommentReview/index.tsx +++ b/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(createDefaultSurvey()); const [isEditingThreatContact, setIsEditingThreatContact] = useState(false); const [threatContact, setThreatContact] = useState(null); - const [nextPendingId, setNextPendingId] = useState(null); + const [nextSubmissionId, setNextSubmissionId] = useState(null); const [versions, setVersions] = useState([]); const [selectedVersion, setSelectedVersion] = useState(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 = () => { - {nextPendingId !== null ? ( + {nextSubmissionId !== null ? ( <> {'Save & Review Next'} diff --git a/met-web/src/components/admin/comments/admin/CommentReview/nextInQueue.ts b/met-web/src/components/admin/comments/admin/CommentReview/nextInQueue.ts new file mode 100644 index 000000000..484bbc125 --- /dev/null +++ b/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 => { + 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 => { + 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; + } +}; diff --git a/met-web/tests/unit/components/comments/nextInQueue.test.ts b/met-web/tests/unit/components/comments/nextInQueue.test.ts new file mode 100644 index 000000000..264ab0263 --- /dev/null +++ b/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(); + }); +});