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(); + }); +});