Merge branch 'develop' into feature-2647/volunteer-tracking-bugfixes

Author: thelenw

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<ReviewAssignment> saveAll(UUID reviewPeriodId, List<ReviewAssignment> reviewAssignments, boolean deleteExisting) {
+    public synchronized List<ReviewAssignment> saveAll(UUID reviewPeriodId, List<ReviewAssignment> reviewAssignments, boolean deleteExisting) {
 
         if(deleteExisting) {
             LOG.warn("Deleting all review assignments for review period {}", reviewPeriodId);

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 {

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)

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

server/src/test/java/com/objectcomputing/checkins/services/reviews/ReviewAssignmentControllerTest.java

@@ -146,6 +146,11 @@ void testGETFindAssignmentsByPeriodIdDefaultAssignments() {
 
         final HttpResponse<Set<ReviewAssignment>> 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());
     }
-}
+}

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

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,38 +218,31 @@ 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,
       reviewPeriodId: 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 = () => {