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 08ae74353..3fae5ff53 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/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java b/server/src/main/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java index 70e00fc0e..8a5d5786d 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java @@ -11,7 +11,7 @@ import java.util.List; import java.util.UUID; -@Secured(SecurityRule.IS_ANONYMOUS) +@Secured(SecurityRule.IS_AUTHENTICATED) @ExecuteOn(TaskExecutors.BLOCKING) @Controller("/services/roles/members") public class MemberRoleController { 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 000000000..9e4df8632 --- /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) 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 1814e284e..2aa8a6afc 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 diff --git a/server/src/test/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentControllerTest.java index 209531709..02860db4b 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentControllerTest.java @@ -146,6 +146,11 @@ void testGETFindAssignmentsByPeriodIdDefaultAssignments() { final HttpResponse> response = client.toBlocking().exchange(request, Argument.setOf(ReviewAssignment.class)); + // A review period only has default review assignments added to it when + // the review period is created through the controller. And, they are + // no longer added to the review period when retrieving the review + // assignments for a specific review period. Therefore, this review + // period should have zero review assignments associated with it. assertNotNull(response.body()); assertEquals(0, Objects.requireNonNull(response.body()).size()); assertEquals(HttpStatus.OK, response.getStatus()); @@ -284,4 +289,4 @@ void deleteReviewAssignmentWithoutPermissions() { assertNotNull(responseException.getResponse()); assertEquals(HttpStatus.FORBIDDEN, responseException.getStatus()); } -} \ No newline at end of file +} diff --git a/web-ui/src/api/reviewassignments.js b/web-ui/src/api/reviewassignments.js new file mode 100644 index 000000000..9c9593cb2 --- /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 78c072138..47ed014ed 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,16 +195,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const loadAssignments = async () => { - const myId = currentUser?.id; - 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; @@ -210,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; @@ -230,11 +218,12 @@ 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); }; 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,26 +231,18 @@ const TeamReviews = ({ onBack, periodId }) => { approved: false })); - const res = await resolve({ - method: 'POST', - url: reviewAssignmentsUrl + '/' + periodId, - data, - headers: { - 'X-CSRF-Header': csrf, - Accept: 'application/json', - 'Content-Type': 'application/json;charset=UTF-8' - } - }); + // Set those on the server as the review assignments. + let res = await createReviewAssignments(periodId, data, csrf); 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 getReviewAssignments(periodId, csrf); + 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) => { @@ -605,11 +586,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); @@ -641,19 +618,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(); @@ -721,16 +686,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); @@ -940,16 +896,7 @@ const TeamReviews = ({ onBack, periodId }) => { }; const approveReviewAssignment = async (assignment, approved) => { - const res = await resolve({ - method: assignment.id === null ? 'POST' : 'PUT', - url: '/services/review-assignments', - 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 = () => {