From cc1ec5f6498af672bbde7c118a2e819d30e304b1 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Fri, 18 Oct 2024 08:40:10 -0500 Subject: [PATCH 1/5] Ensure that we do not have duplicate review assignments. --- .../services/reviews/ReviewAssignmentServicesImpl.java | 6 +++++- .../common/V115__review_assignments_modify_primary_key.sql | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 server/src/main/resources/db/common/V115__review_assignments_modify_primary_key.sql diff --git a/server/src/main/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentServicesImpl.java index 08ae74353c..3fae5ff53c 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentServicesImpl.java @@ -43,8 +43,12 @@ public ReviewAssignment save(ReviewAssignment reviewAssignment) { return newAssignment; } + // Now that uniqueness constraints have been placed on the + // reviewee-reviewer-reviewPeriod, this method needs to be synchronized + // to avoid multiple calls from the client-side overlapping and attempting + // to create the same review assignments multiple times. @Override - public List saveAll(UUID reviewPeriodId, List reviewAssignments, boolean deleteExisting) { + public synchronized List saveAll(UUID reviewPeriodId, List reviewAssignments, boolean deleteExisting) { if(deleteExisting) { LOG.warn("Deleting all review assignments for review period {}", reviewPeriodId); diff --git a/server/src/main/resources/db/common/V115__review_assignments_modify_primary_key.sql b/server/src/main/resources/db/common/V115__review_assignments_modify_primary_key.sql new file mode 100644 index 0000000000..9e4df8632a --- /dev/null +++ b/server/src/main/resources/db/common/V115__review_assignments_modify_primary_key.sql @@ -0,0 +1,7 @@ +DELETE FROM review_assignments o USING review_assignments n +WHERE o.id < n.id AND (o.reviewee_id = n.reviewee_id AND + o.reviewer_id = n.reviewer_id AND + o.review_period_id = n.review_period_id); + +ALTER TABLE review_assignments +ADD CONSTRAINT unique_assignment UNIQUE (reviewee_id, reviewer_id, review_period_id) From d24493dca2590784add6c9dea3013a73d7038757 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Fri, 18 Oct 2024 08:41:22 -0500 Subject: [PATCH 2/5] Fixed pre-baked review period dates so that they are displayed correctly. --- server/src/main/resources/db/dev/R__Load_testing_data.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/resources/db/dev/R__Load_testing_data.sql b/server/src/main/resources/db/dev/R__Load_testing_data.sql index 1814e284e4..2aa8a6afc3 100644 --- a/server/src/main/resources/db/dev/R__Load_testing_data.sql +++ b/server/src/main/resources/db/dev/R__Load_testing_data.sql @@ -1291,12 +1291,12 @@ VALUES INSERT INTO review_periods (id, name, review_status, review_template_id, self_review_template_id, launch_date, self_review_close_date, close_date, period_start_date, period_end_date) VALUES - ('12345678-e29c-4cf4-9ea4-6baa09405c57', 'Review Period 1', 'PLANNING', 'd1e94b60-47c4-4945-87d1-4dc88f088e57', '926a37a4-4ded-4633-8900-715b0383aecc', '2024-09-01', '2024-09-02', '2024-09-03', '2024-01-01', '2024-12-31'); + ('12345678-e29c-4cf4-9ea4-6baa09405c57', 'Review Period 1', 'PLANNING', 'd1e94b60-47c4-4945-87d1-4dc88f088e57', '926a37a4-4ded-4633-8900-715b0383aecc', '2024-09-01 06:00:00', '2024-09-02 06:00:00', '2024-09-03 06:00:00', '2024-01-01 06:00:00', '2024-08-31 06:00:00'); INSERT INTO review_periods (id, name, review_status, review_template_id, self_review_template_id, launch_date, self_review_close_date, close_date, period_start_date, period_end_date) VALUES - ('12345678-e29c-4cf4-9ea4-6baa09405c58', 'Review Period 2', 'PLANNING', 'd1e94b60-47c4-4945-87d1-4dc88f088e57', '926a37a4-4ded-4633-8900-715b0383aecc', '2024-09-10', '2024-09-11', '2024-09-12', '2024-01-01', '2024-12-31'); + ('12345678-e29c-4cf4-9ea4-6baa09405c58', 'Review Period 2', 'PLANNING', 'd1e94b60-47c4-4945-87d1-4dc88f088e57', '926a37a4-4ded-4633-8900-715b0383aecc', '2024-09-10 06:00:00', '2024-09-11 06:00:00', '2024-09-12 06:00:00', '2024-01-01 06:00:00', '2024-08-31 06:00:00'); -- CAE - Self-Review feedback request, Creator: Big Boss INSERT INTO feedback_requests From ee5fa80c98f5dd15d0efd41c37823279635b1697 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Fri, 18 Oct 2024 08:43:02 -0500 Subject: [PATCH 3/5] We no longer have to fix-up the assignments after setting the team mebers being reviewed. They are handled correctly on the server side and we can just use them as we have received them. --- web-ui/src/components/reviews/TeamReviews.jsx | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/web-ui/src/components/reviews/TeamReviews.jsx b/web-ui/src/components/reviews/TeamReviews.jsx index 78c072138b..990daf08b8 100644 --- a/web-ui/src/components/reviews/TeamReviews.jsx +++ b/web-ui/src/components/reviews/TeamReviews.jsx @@ -192,7 +192,6 @@ const TeamReviews = ({ onBack, periodId }) => { }; const loadAssignments = async () => { - const myId = currentUser?.id; const res = await resolve({ method: 'GET', url: `${reviewAssignmentsUrl}/period/${periodId}`, @@ -235,6 +234,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const updateTeamMembers = async teamMembers => { + // First, create a set of team members, each with a default reviewer. const data = teamMembers.map(tm => ({ revieweeId: tm.id, reviewerId: tm.supervisorid, @@ -242,7 +242,8 @@ const TeamReviews = ({ onBack, periodId }) => { approved: false })); - const res = await resolve({ + // Set those on the server as the review assignments. + let res = await resolve({ method: 'POST', url: reviewAssignmentsUrl + '/' + periodId, data, @@ -254,14 +255,22 @@ const TeamReviews = ({ onBack, periodId }) => { }); if (res.error) return; - setTeamMembers(teamMembers); - addAssignmentForMemberWithNone(teamMembers); + // Get the list of review assignments from the server to ensure that we are + // reflecting what was actually created. + res = await resolve({ + method: 'GET', + url: `${reviewAssignmentsUrl}/period/${periodId}`, + headers: { + 'X-CSRF-Header': csrf, + Accept: 'application/json', + 'Content-Type': 'application/json;charset=UTF-8' + } + }); + const assignments = res.error ? [] : res.payload.data; - // Now that teamMembers has been updated, we need to make sure that the - // assignments reflects the set of team members. - const ids = teamMembers.map(m => m.id); - const newAssignments = assignments.filter(a => a.revieweeId && ids.includes(a.revieweeId)); - setAssignments(newAssignments); + // Update our reactive assignment and member lists. + setAssignments(assignments); + setTeamMembers(teamMembers); }; const addAssignmentForMemberWithNone = async (members) => { @@ -942,7 +951,7 @@ const TeamReviews = ({ onBack, periodId }) => { const approveReviewAssignment = async (assignment, approved) => { const res = await resolve({ method: assignment.id === null ? 'POST' : 'PUT', - url: '/services/review-assignments', + url: reviewAssignmentsUrl, headers: { Accept: 'application/json', 'Content-Type': 'application/json;charset=UTF-8', From d290a9ecb425c4005f9fa291ad123aea6c22593b Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Fri, 18 Oct 2024 10:04:48 -0500 Subject: [PATCH 4/5] Factored review assignment API calls into a separate file. --- web-ui/src/api/reviewassignments.js | 44 ++++++++++ web-ui/src/components/reviews/TeamReviews.jsx | 82 +++---------------- 2 files changed, 57 insertions(+), 69 deletions(-) create mode 100644 web-ui/src/api/reviewassignments.js diff --git a/web-ui/src/api/reviewassignments.js b/web-ui/src/api/reviewassignments.js new file mode 100644 index 0000000000..9c9593cb27 --- /dev/null +++ b/web-ui/src/api/reviewassignments.js @@ -0,0 +1,44 @@ +import { resolve } from './api.js'; + +const reviewAssignmentsUrl = '/services/review-assignments'; + +export const getReviewAssignments = async (id, cookie) => { + return resolve({ + url: `${reviewAssignmentsUrl}/period/${id}`, + headers: { 'X-CSRF-Header': cookie, Accept: 'application/json' } + }); +}; + +export const createReviewAssignments = async (id, assignments, cookie) => { + return resolve({ + method: 'POST', + url: `${reviewAssignmentsUrl}/${id}`, + data: assignments, + headers: { + 'X-CSRF-Header': cookie, + Accept: 'application/json', + 'Content-Type': 'application/json;charset=UTF-8' + } + }); +}; + +export const updateReviewAssignment = async (assignment, cookie) => { + return resolve({ + method: assignment.id === null ? 'POST' : 'PUT', + url: reviewAssignmentsUrl, + data: assignment, + headers: { + 'X-CSRF-Header': cookie, + Accept: 'application/json', + 'Content-Type': 'application/json;charset=UTF-8' + } + }); +}; + +export const removeReviewAssignment = async (id, cookie) => { + return resolve({ + method: 'DELETE', + url: `${reviewAssignmentsUrl}/${id}`, + headers: { 'X-CSRF-Header': cookie } + }); +}; diff --git a/web-ui/src/components/reviews/TeamReviews.jsx b/web-ui/src/components/reviews/TeamReviews.jsx index 990daf08b8..93f5761436 100644 --- a/web-ui/src/components/reviews/TeamReviews.jsx +++ b/web-ui/src/components/reviews/TeamReviews.jsx @@ -34,7 +34,12 @@ import { import { styled } from '@mui/material/styles'; import ConfirmationDialog from '../dialogs/ConfirmationDialog'; -import { resolve } from '../../api/api.js'; +import { + getReviewAssignments, + createReviewAssignments, + updateReviewAssignment, + removeReviewAssignment, +} from '../../api/reviewassignments.js'; import { findReviewRequestsByPeriod, findSelfReviewRequestsByPeriodAndTeamMembers @@ -158,8 +163,6 @@ const TeamReviews = ({ onBack, periodId }) => { const isAdmin = selectIsAdmin(state); const period = selectReviewPeriod(state, periodId); - const reviewAssignmentsUrl = '/services/review-assignments'; - useEffect(() => { loadAssignments(); }, [currentMembers]); @@ -192,15 +195,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const loadAssignments = async () => { - const res = await resolve({ - method: 'GET', - url: `${reviewAssignmentsUrl}/period/${periodId}`, - headers: { - 'X-CSRF-Header': csrf, - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8' - } - }); + const res = await getReviewAssignments(periodId, csrf); if (res.error) return; const assignments = res.payload.data; @@ -243,29 +238,12 @@ const TeamReviews = ({ onBack, periodId }) => { })); // Set those on the server as the review assignments. - let res = await resolve({ - method: 'POST', - url: reviewAssignmentsUrl + '/' + periodId, - data, - headers: { - 'X-CSRF-Header': csrf, - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8' - } - }); + let res = await createReviewAssignments(periodId, data, csrf); if (res.error) return; // Get the list of review assignments from the server to ensure that we are // reflecting what was actually created. - res = await resolve({ - method: 'GET', - url: `${reviewAssignmentsUrl}/period/${periodId}`, - headers: { - 'X-CSRF-Header': csrf, - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8' - } - }); + res = await getReviewAssignments(periodId, csrf); const assignments = res.error ? [] : res.payload.data; // Update our reactive assignment and member lists. @@ -614,11 +592,7 @@ const TeamReviews = ({ onBack, periodId }) => { const { id, revieweeId, reviewerId } = assignment; if (id) { - const res = await resolve({ - method: 'DELETE', - url: `${reviewAssignmentsUrl}/${id}`, - headers: { 'X-CSRF-Header': csrf } - }); + const res = await removeReviewAssignment(id, csrf); if (res.error) { console.error('Error deleting assignment:', res.error); @@ -650,19 +624,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const updateReviewPeriodStatus = async reviewStatus => { - const res = await resolve({ - method: 'PUT', - url: '/services/review-periods', - headers: { - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8', - 'X-CSRF-Header': csrf - }, - data: { - ...period, - reviewStatus - } - }); + const res = await updateReviewPeriod({ ...period, reviewStatus }, csrf); if (res.error) return; onBack(); @@ -730,16 +692,7 @@ const TeamReviews = ({ onBack, periodId }) => { } } - const res = await resolve({ - method: 'POST', - url: `${reviewAssignmentsUrl}/${periodId}`, - data: newAssignments, - headers: { - 'X-CSRF-Header': csrf, - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8' - } - }); + const res = await createReviewAssignments(periodId, newAssignments, csrf); if (res.error) return; newAssignments = sortMembers(res.payload.data); @@ -949,16 +902,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const approveReviewAssignment = async (assignment, approved) => { - const res = await resolve({ - method: assignment.id === null ? 'POST' : 'PUT', - url: reviewAssignmentsUrl, - headers: { - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8', - 'X-CSRF-Header': csrf - }, - data: { ...assignment, approved } - }); + await updateReviewAssignment({ ...assignment, approved }, csrf); }; const visibleTeamMembers = () => { From cbe1f5780af9be8b53d2c3993c41edb3af48875a Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Fri, 18 Oct 2024 10:25:18 -0500 Subject: [PATCH 5/5] Removed the workaround for dealing with default review assignments overwriting chosen review assignments. --- web-ui/src/components/reviews/TeamReviews.jsx | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/web-ui/src/components/reviews/TeamReviews.jsx b/web-ui/src/components/reviews/TeamReviews.jsx index 93f5761436..47ed014ed6 100644 --- a/web-ui/src/components/reviews/TeamReviews.jsx +++ b/web-ui/src/components/reviews/TeamReviews.jsx @@ -204,12 +204,6 @@ const TeamReviews = ({ onBack, periodId }) => { }; const loadTeamMembers = () => { - // If we already have a list of team members, we should not overwrite the - // list with the original list of team members. - if (teamMembers.length > 0) { - return; - } - let source; if (!approvalMode || (isAdmin && showAll)) { source = currentMembers; @@ -224,7 +218,7 @@ const TeamReviews = ({ onBack, periodId }) => { // Always filter the members down to existing selected assignments. // We do not want to add members that were not already selected. const memberIds = assignments.map(a => a.revieweeId); - let members = source.filter(m => memberIds.includes(m.id)); + const members = source.filter(m => memberIds.includes(m.id)); setTeamMembers(members); };