From c57b5ab6a7fe133332c58302710b8abaaf69d295 Mon Sep 17 00:00:00 2001 From: Priyanka Mutha Date: Wed, 7 Oct 2026 13:24:29 -0700 Subject: [PATCH 1/4] Prevent duplicate PR entries and retain added PRs --- .../PRGradingScreen.duplicates.test.jsx | 125 ++++++++++++++++++ .../PRGradingScreen/PRGradingScreen.jsx | 113 +++++++++++++--- .../PRGradingScreen.module.css | 8 +- .../PRGradingScreenContainer.test.jsx | 49 +++++++ .../PRGradingScreen/PRGradingTest.jsx | 13 +- src/components/PRGradingScreen/index.jsx | 55 +++++++- .../PRGradingScreen/prGradingPersistence.js | 52 ++++++++ .../prGradingPersistence.test.js | 75 +++++++++++ 8 files changed, 466 insertions(+), 24 deletions(-) create mode 100644 src/components/PRGradingScreen/PRGradingScreen.duplicates.test.jsx create mode 100644 src/components/PRGradingScreen/PRGradingScreenContainer.test.jsx create mode 100644 src/components/PRGradingScreen/prGradingPersistence.js create mode 100644 src/components/PRGradingScreen/prGradingPersistence.test.js diff --git a/src/components/PRGradingScreen/PRGradingScreen.duplicates.test.jsx b/src/components/PRGradingScreen/PRGradingScreen.duplicates.test.jsx new file mode 100644 index 00000000000..4a7f7bd8db6 --- /dev/null +++ b/src/components/PRGradingScreen/PRGradingScreen.duplicates.test.jsx @@ -0,0 +1,125 @@ +import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import axios from 'axios'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import PRGradingScreen from './PRGradingScreen'; + +vi.mock('axios'); +vi.mock('react-redux', () => ({ + useSelector: selector => selector({ theme: { darkMode: false } }), +})); + +const teamData = { + teamName: 'Test Team', + dateRange: { start: '5/18/2025', end: '5/25/2025' }, +}; +const reviewers = [ + { + id: '1', + reviewer: 'Alice', + prsNeeded: 3, + gradedPrs: [{ id: 'pr1', prNumbers: '1234 + 5678', grade: 'Okay' }], + }, + { id: '2', reviewer: 'Bob', prsNeeded: 3, gradedPrs: [] }, +]; + +const addPR = async (row, numbers) => { + const addNewButton = within(row).getByRole('button', { name: '+ Add new' }); + await waitFor(() => expect(addNewButton).toBeEnabled()); + fireEvent.click(addNewButton); + fireEvent.change(within(row).getByPlaceholderText('1070 or 1070 + 1256'), { + target: { value: numbers }, + }); + fireEvent.click(within(row).getByRole('button', { name: 'Add' })); + await waitFor(() => + expect(within(row).queryByRole('button', { name: 'Adding...' })).not.toBeInTheDocument(), + ); +}; + +describe('Weekly PR grading duplicate entries', () => { + beforeEach(() => { + vi.clearAllMocks(); + axios.get.mockResolvedValue({ data: [] }); + axios.post.mockResolvedValue({ data: { status: 'ok' } }); + }); + + it('allows a pair containing two unused PR numbers and counts it as one review', async () => { + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + + await addPR(aliceRow, '9981 + 9982'); + + expect(within(aliceRow).getByText('9981 + 9982')).toBeInTheDocument(); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(2); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('rejects a PR already present inside a pair for the same reviewer', async () => { + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + + await addPR(aliceRow, '5678 + 9999'); + + expect(screen.getByRole('alert')).toHaveTextContent('already been added'); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(1); + expect(within(aliceRow).queryByText('5678 + 9999')).not.toBeInTheDocument(); + }); + + it('rejects a repeated number within a new pair', async () => { + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + + await addPR(aliceRow, '4321 + 4321'); + + expect(screen.getByRole('alert')).toHaveTextContent('already been added'); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(1); + }); + + it('rejects a repeated addition but allows another reviewer to use that PR number', async () => { + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + const bobRow = screen.getByRole('row', { name: /Bob/ }); + + await addPR(aliceRow, '4321'); + await addPR(aliceRow, '4321'); + + expect(screen.getByRole('alert')).toHaveTextContent('already been added'); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(2); + + await addPR(bobRow, '4321'); + expect(within(bobRow).getByRole('spinbutton')).toHaveValue(1); + }); + + it('loads a saved PR again after the screen is remounted', async () => { + axios.get + .mockResolvedValueOnce({ data: [] }) + .mockResolvedValueOnce({ data: [] }) + .mockResolvedValueOnce({ + data: [ + { + reviewer: 'Alice', + gradedPrs: [{ _id: 'saved1', prNumbers: '9981', grade: 'Okay' }], + }, + ], + }); + const { unmount } = render(); + await addPR(screen.getByRole('row', { name: /Alice/ }), '9981'); + unmount(); + + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + await waitFor(() => expect(within(aliceRow).getByText('9981')).toBeInTheDocument()); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(2); + }); + + it('keeps the input open when saving fails instead of showing an unsaved PR', async () => { + axios.post.mockRejectedValueOnce(new Error('Network error')); + render(); + const aliceRow = screen.getByRole('row', { name: /Alice/ }); + + await addPR(aliceRow, '9981'); + + expect(within(aliceRow).getByRole('alert')).toHaveTextContent('Could not save'); + expect(within(aliceRow).getByRole('spinbutton')).toHaveValue(1); + expect(within(aliceRow).queryByText('9981')).not.toBeInTheDocument(); + }); +}); diff --git a/src/components/PRGradingScreen/PRGradingScreen.jsx b/src/components/PRGradingScreen/PRGradingScreen.jsx index 4c96cf1d2f4..379f3529ce4 100644 --- a/src/components/PRGradingScreen/PRGradingScreen.jsx +++ b/src/components/PRGradingScreen/PRGradingScreen.jsx @@ -1,10 +1,22 @@ import PropTypes from 'prop-types'; -import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import { Button, Card, Col, Container, Row } from 'react-bootstrap'; import { useSelector } from 'react-redux'; import { v4 as uuidv4 } from 'uuid'; +import { loadSavedGradings, mergeSavedGradings, saveAddedPR } from './prGradingPersistence'; import styles from './PRGradingScreen.module.css'; +const hasDuplicatePR = (gradedPrs, value) => { + const enteredNumbers = value.split('+').map(number => number.trim()); + const existingNumbers = gradedPrs.flatMap(pr => + pr.prNumbers.split('+').map(number => number.trim()), + ); + return ( + new Set(enteredNumbers).size !== enteredNumbers.length || + enteredNumbers.some(number => existingNumbers.includes(number)) + ); +}; + const PRGradingScreen = ({ teamData, reviewers }) => { const darkMode = useSelector(state => state.theme.darkMode); @@ -14,6 +26,31 @@ const PRGradingScreen = ({ teamData, reviewers }) => { const [inputError, setInputError] = useState(''); const [showGradingModal, setShowGradingModal] = useState(null); const [isFinalized, setIsFinalized] = useState(false); + const [isLoadingSaved, setIsLoadingSaved] = useState(true); + const [isSavingPR, setIsSavingPR] = useState(false); + const [loadError, setLoadError] = useState(''); + + useEffect(() => { + if (!teamData || !reviewers) return undefined; + let active = true; + setIsLoadingSaved(true); + loadSavedGradings(teamData) + .then(saved => { + if (active) { + setReviewerData(mergeSavedGradings(reviewers, saved)); + setLoadError(''); + } + }) + .catch(() => { + if (active) setLoadError('Could not load saved PRs. Try again before adding a PR.'); + }) + .finally(() => { + if (active) setIsLoadingSaved(false); + }); + return () => { + active = false; + }; + }, [teamData, reviewers]); // Search state const [searchTerm, setSearchTerm] = useState(''); @@ -62,24 +99,51 @@ const PRGradingScreen = ({ teamData, reviewers }) => { setInputError(''); }; - const handleInputSubmit = reviewerId => { - if (isFinalized) return; + const handleInputSubmit = async reviewerId => { + if (isFinalized || isLoadingSaved || isSavingPR) return; const validation = validatePRNumber(inputValue); if (!validation.isValid) { setInputError(validation.error); return; } - const newPREntry = { id: uuidv4(), prNumbers: inputValue.trim(), grade: 'Okay' }; - setReviewerData(prev => - prev.map(r => - r.id === reviewerId - ? { ...r, gradedPrs: [...r.gradedPrs, newPREntry], prsReviewed: r.gradedPrs.length + 1 } - : r, - ), - ); - setActiveInput(null); - setInputValue(''); - setInputError(''); + const reviewer = reviewerData.find(item => item.id === reviewerId); + if (hasDuplicatePR(reviewer.gradedPrs, inputValue)) { + setInputError('This PR number has already been added for this reviewer.'); + return; + } + setIsSavingPR(true); + try { + const saved = await loadSavedGradings(teamData); + const currentReviewers = mergeSavedGradings(reviewerData, saved); + const currentReviewer = currentReviewers.find(item => item.id === reviewerId); + if (hasDuplicatePR(currentReviewer.gradedPrs, inputValue)) { + setReviewerData(currentReviewers); + setInputError('This PR number has already been added for this reviewer.'); + return; + } + const prNumbers = inputValue.trim(); + await saveAddedPR(teamData, currentReviewer, prNumbers); + const newPREntry = { id: uuidv4(), prNumbers, grade: 'Okay' }; + setReviewerData( + currentReviewers.map(item => + item.id === reviewerId + ? { + ...item, + gradedPrs: [...item.gradedPrs, newPREntry], + prsReviewed: item.gradedPrs.length + 1, + } + : item, + ), + ); + setActiveInput(null); + setInputValue(''); + setInputError(''); + setLoadError(''); + } catch { + setInputError('Could not save this PR. Please try again.'); + } finally { + setIsSavingPR(false); + } }; const handleCancel = () => { @@ -134,13 +198,15 @@ const PRGradingScreen = ({ teamData, reviewers }) => { + + {loadError &&

{loadError}

} @@ -240,6 +306,7 @@ const PRGradingScreen = ({ teamData, reviewers }) => { size="sm" className={styles['pr-grading-screen-add-btn']} onClick={() => handleAddNewClick(reviewer.id)} + disabled={isLoadingSaved} > + Add new @@ -250,16 +317,28 @@ const PRGradingScreen = ({ teamData, reviewers }) => { setInputValue(e.target.value)} + onChange={e => { + setInputValue(e.target.value); + setInputError(''); + }} className={styles['pr-grading-screen-pr-number-input']} placeholder="1070 or 1070 + 1256" /> + {inputError && ( +

+ {inputError} +

+ )} @@ -389,7 +417,7 @@ const PRGradingScreen = ({ teamData, reviewers }) => { handleGradeChange(showGradingModal, pr.id, 'Exceptional') @@ -399,7 +427,7 @@ const PRGradingScreen = ({ teamData, reviewers }) => { handleGradeChange(showGradingModal, pr.id, 'Okay')} /> @@ -407,7 +435,7 @@ const PRGradingScreen = ({ teamData, reviewers }) => { handleGradeChange(showGradingModal, pr.id, 'Unsatisfactory') @@ -417,10 +445,12 @@ const PRGradingScreen = ({ teamData, reviewers }) => { - handleGradeChange(showGradingModal, pr.id, 'Cannot find image') + handleGradeChange(showGradingModal, pr.id, 'No Correct Image') } /> @@ -429,8 +459,18 @@ const PRGradingScreen = ({ teamData, reviewers }) => { + {gradeError && ( +

+ {gradeError} +

+ )} +
-
diff --git a/src/components/PRGradingScreen/prGradingPersistence.js b/src/components/PRGradingScreen/prGradingPersistence.js index e0ea546762c..9058c00b974 100644 --- a/src/components/PRGradingScreen/prGradingPersistence.js +++ b/src/components/PRGradingScreen/prGradingPersistence.js @@ -6,21 +6,22 @@ const getPRNumbers = value => value.split('+').map(number => number.trim()); const getGradingKey = teamData => { const [month, day, year] = teamData.dateRange.start.split('/').map(Number); const date = `${year}-${String(month).padStart(2, '0')}-${String(day).padStart(2, '0')}`; - return { teamCode: `pr-grading-screen:${teamData.teamName}`, date }; + return { teamKey: `pr-grading-screen:${teamData.teamName}`, date }; }; export const loadSavedGradings = async teamData => { - const { teamCode, date } = getGradingKey(teamData); + const { teamKey, date } = getGradingKey(teamData); const response = await axios.get(ENDPOINTS.WEEKLY_GRADING, { - params: { team: teamCode, date }, + params: { team: teamKey, date, weekStart: date }, }); return response.data; }; export const saveAddedPR = (teamData, reviewer, prNumbers) => { - const { teamCode, date } = getGradingKey(teamData); + const { teamKey, date } = getGradingKey(teamData); return axios.post(ENDPOINTS.WEEKLY_GRADING_SAVE, { - teamCode, + teamCode: teamKey, + teamName: teamKey, date, gradings: [ { @@ -33,12 +34,33 @@ export const saveAddedPR = (teamData, reviewer, prNumbers) => { }); }; +export const saveGradeChange = (teamData, reviewer, prNumbers, grade) => { + const { teamKey, date } = getGradingKey(teamData); + return axios.post(ENDPOINTS.WEEKLY_GRADING_SAVE, { + teamCode: teamKey, + teamName: teamKey, + date, + gradings: [ + { + reviewer: reviewer.reviewer, + prsNeeded: reviewer.prsNeeded, + prsReviewed: reviewer.gradedPrs.length, + gradedPrs: [{ prNumbers, grade }], + }, + ], + }); +}; + export const mergeSavedGradings = (reviewers, savedGradings) => { const savedByReviewer = new Map(savedGradings.map(grading => [grading.reviewer, grading])); return reviewers.map(reviewer => { + const savedPrs = savedByReviewer.get(reviewer.reviewer)?.gradedPrs || []; + const savedGrades = new Map( + savedPrs.map(pr => [getPRNumbers(pr.prNumbers).join('+'), pr.grade]), + ); const seen = new Set(reviewer.gradedPrs.flatMap(pr => getPRNumbers(pr.prNumbers))); - const additions = (savedByReviewer.get(reviewer.reviewer)?.gradedPrs || []) + const additions = savedPrs .filter(pr => { const numbers = getPRNumbers(pr.prNumbers); if (numbers.some(number => seen.has(number))) return false; @@ -46,7 +68,13 @@ export const mergeSavedGradings = (reviewers, savedGradings) => { return true; }) .map(pr => ({ ...pr, id: String(pr._id || pr.id || pr.prNumbers) })); - const gradedPrs = [...reviewer.gradedPrs, ...additions]; + const gradedPrs = [ + ...reviewer.gradedPrs.map(pr => ({ + ...pr, + grade: savedGrades.get(getPRNumbers(pr.prNumbers).join('+')) || pr.grade, + })), + ...additions, + ]; return { ...reviewer, gradedPrs, prsReviewed: gradedPrs.length }; }); }; diff --git a/src/components/PRGradingScreen/prGradingPersistence.test.js b/src/components/PRGradingScreen/prGradingPersistence.test.js index ded3c2dd2f2..73820bfab7d 100644 --- a/src/components/PRGradingScreen/prGradingPersistence.test.js +++ b/src/components/PRGradingScreen/prGradingPersistence.test.js @@ -1,7 +1,12 @@ import axios from 'axios'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { ENDPOINTS } from '../../utils/URL'; -import { loadSavedGradings, mergeSavedGradings, saveAddedPR } from './prGradingPersistence'; +import { + loadSavedGradings, + mergeSavedGradings, + saveAddedPR, + saveGradeChange, +} from './prGradingPersistence'; vi.mock('axios'); @@ -18,7 +23,11 @@ describe('PR grading persistence', () => { await expect(loadSavedGradings(teamData)).resolves.toHaveLength(1); expect(axios.get).toHaveBeenCalledWith(ENDPOINTS.WEEKLY_GRADING, { - params: { team: 'pr-grading-screen:91NePRT', date: '2025-05-18' }, + params: { + team: 'pr-grading-screen:91NePRT', + date: '2025-05-18', + weekStart: '2025-05-18', + }, }); }); @@ -33,6 +42,7 @@ describe('PR grading persistence', () => { expect(axios.post).toHaveBeenCalledWith(ENDPOINTS.WEEKLY_GRADING_SAVE, { teamCode: 'pr-grading-screen:91NePRT', + teamName: 'pr-grading-screen:91NePRT', date: '2025-05-18', gradings: [ { @@ -45,6 +55,30 @@ describe('PR grading persistence', () => { }); }); + it('saves only the changed grade without increasing the reviewed count', async () => { + const reviewer = { + reviewer: 'Alice', + prsNeeded: 3, + gradedPrs: [{ prNumbers: '1234', grade: 'Okay' }], + }; + + await saveGradeChange(teamData, reviewer, '1234', 'Exceptional'); + + expect(axios.post).toHaveBeenCalledWith(ENDPOINTS.WEEKLY_GRADING_SAVE, { + teamCode: 'pr-grading-screen:91NePRT', + teamName: 'pr-grading-screen:91NePRT', + date: '2025-05-18', + gradings: [ + { + reviewer: 'Alice', + prsNeeded: 3, + prsReviewed: 1, + gradedPrs: [{ prNumbers: '1234', grade: 'Exceptional' }], + }, + ], + }); + }); + it('merges saved PRs with built-in entries without duplicating them', () => { const reviewers = [ { @@ -72,4 +106,25 @@ describe('PR grading persistence', () => { ], }); }); + + it('uses the saved grade for a built-in PR after reloading', () => { + const reviewers = [ + { + id: '1', + reviewer: 'Alice', + gradedPrs: [{ id: 'pr1', prNumbers: '1234 + 5678', grade: 'Okay' }], + }, + ]; + const saved = [ + { + reviewer: 'Alice', + gradedPrs: [{ prNumbers: '1234+5678', grade: 'Exceptional' }], + }, + ]; + + expect(mergeSavedGradings(reviewers, saved)[0]).toMatchObject({ + prsReviewed: 1, + gradedPrs: [{ id: 'pr1', prNumbers: '1234 + 5678', grade: 'Exceptional' }], + }); + }); }); From ff5bea8e4b9b72f343f3d116188ee0b88d3ee058 Mon Sep 17 00:00:00 2001 From: Priyanka Mutha Date: Sun, 25 Oct 2026 11:09:04 -0700 Subject: [PATCH 3/4] Use Set for duplicate PR number lookup --- src/components/PRGradingScreen/PRGradingScreen.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/PRGradingScreen/PRGradingScreen.jsx b/src/components/PRGradingScreen/PRGradingScreen.jsx index b6452ba6b0f..489a6adf9b3 100644 --- a/src/components/PRGradingScreen/PRGradingScreen.jsx +++ b/src/components/PRGradingScreen/PRGradingScreen.jsx @@ -13,12 +13,12 @@ import { usePRPromotion } from './usePRPromotion'; const hasDuplicatePR = (gradedPrs, value) => { const enteredNumbers = value.split('+').map(number => number.trim()); - const existingNumbers = gradedPrs.flatMap(pr => - pr.prNumbers.split('+').map(number => number.trim()), + const existingNumbers = new Set( + gradedPrs.flatMap(pr => pr.prNumbers.split('+').map(number => number.trim())), ); return ( new Set(enteredNumbers).size !== enteredNumbers.length || - enteredNumbers.some(number => existingNumbers.includes(number)) + enteredNumbers.some(number => existingNumbers.has(number)) ); }; From f164f2d181fee70e35e0089f0925b65f002efb3e Mon Sep 17 00:00:00 2001 From: Priyanka Mutha Date: Sun, 25 Oct 2026 11:25:03 -0700 Subject: [PATCH 4/4] Fix ActivityList sort test for past mock events --- .../CommunityPortal/Activities/__tests__/ActivityList.test.jsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/CommunityPortal/Activities/__tests__/ActivityList.test.jsx b/src/components/CommunityPortal/Activities/__tests__/ActivityList.test.jsx index d5751c59432..f0b1639a039 100644 --- a/src/components/CommunityPortal/Activities/__tests__/ActivityList.test.jsx +++ b/src/components/CommunityPortal/Activities/__tests__/ActivityList.test.jsx @@ -57,6 +57,7 @@ describe('ActivityList', () => { test('sorts activities by date (latest to earliest)', () => { renderActivityList(); + fireEvent.click(screen.getByLabelText(/Show Past Events:/i)); fireEvent.change(screen.getByLabelText(/Sort By:/i), { target: { value: 'latest' },