Feature 2237 clean up FindByFields

server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemController.java

@@ -4,10 +4,8 @@
 import com.objectcomputing.checkins.services.permissions.Permission;
 import com.objectcomputing.checkins.services.permissions.RequiredPermission;
 import io.micronaut.core.annotation.Nullable;
-import io.micronaut.http.HttpRequest;
 import io.micronaut.http.HttpResponse;
 import io.micronaut.http.HttpStatus;
-import io.micronaut.http.MediaType;
 import io.micronaut.http.annotation.*;
 import io.micronaut.http.uri.UriBuilder;
 import io.micronaut.scheduling.TaskExecutors;
@@ -16,7 +14,6 @@
 import io.micronaut.security.rules.SecurityRule;
 import io.swagger.v3.oas.annotations.tags.Tag;
 import jakarta.validation.Valid;
-import reactor.core.publisher.Mono;
 
 import java.net.URI;
 import java.util.List;
@@ -70,7 +67,8 @@ HttpResponse<?> updateAgendaItem(@Body @Valid AgendaItem agendaItem) {
     }
 
     /**
-     * Find agenda items that match all filled in parameters, return all results when given no params
+     * Find agenda items that match all filled in parameters, or return all results when provided no parameters, and
+     * the user has permission to view all checkin items
      *
      * @param checkinid   {@link UUID} of checkin
      * @param createdbyid {@link UUID} of member	

server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemServicesImpl.java

@@ -1,6 +1,7 @@
 package com.objectcomputing.checkins.services.agenda_item;
 
 import com.objectcomputing.checkins.exceptions.BadArgException;
+import com.objectcomputing.checkins.exceptions.PermissionException;
 import com.objectcomputing.checkins.services.checkins.CheckIn;
 import com.objectcomputing.checkins.services.checkins.CheckInRepository;
 import com.objectcomputing.checkins.services.checkins.CheckInServices;
@@ -135,23 +136,13 @@ public AgendaItem update(AgendaItem agendaItem) {
     }
 
     @Override
-    public Set<AgendaItem> findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) {
-
-
+    public Set<AgendaItem> findByFields(@Nullable UUID checkinId, @Nullable UUID createdById) {
         MemberProfile currentUser = currentUserServices.getCurrentUser();
-        final UUID currentUserId = currentUser.getId();
-        boolean canViewAllCheckins = checkInServices.canViewAllCheckins(currentUserId);
-
-        if (checkinid != null) {
-            validate(!checkInServices.accessGranted(checkinid, currentUserId), "User is unauthorized to do this operation");
-        } else if (createbyid != null) {
-            MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow();
-            validate(!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins, "User is unauthorized to do this operation");
-        } else {
-            validate(!canViewAllCheckins, "User is unauthorized to do this operation");
+        if(!checkInServices.doesUserHaveViewAccess(currentUser.getId(), checkinId, createdById)){
+            throw new PermissionException("User is unauthorized to do this operation");
         }
-        LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinid, createbyid);
-        return agendaItemRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid));
+        LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinId, createdById);
+        return agendaItemRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createdById));
     }
 
     @Override

server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteController.java

@@ -5,7 +5,6 @@
 import io.micronaut.core.annotation.Nullable;
 import io.micronaut.http.HttpRequest;
 import io.micronaut.http.HttpResponse;
-import io.micronaut.http.MediaType;
 import io.micronaut.http.annotation.*;
 import io.micronaut.scheduling.TaskExecutors;
 import io.micronaut.scheduling.annotation.ExecuteOn;
@@ -65,16 +64,16 @@ public HttpResponse<CheckinNote> updateCheckinNote(@Body @Valid CheckinNote chec
     }
 
     /**
-     * Get notes by checkind or createbyid
+     * Get notes by checkind or createbyid. If no parameters are provided, and user has permission to view all checkins,
+     * then return all checkin notes.
      *
      * @param checkinid
      * @param createdbyid
      * @return
      */
     @Get("/{?checkinid,createdbyid}")
     @RequiredPermission(Permission.CAN_VIEW_CHECKINS)
-    public Set<CheckinNote> findCheckinNote(@Nullable UUID checkinid,
-                                            @Nullable UUID createdbyid) {
+    public Set<CheckinNote> findCheckinNote(@Nullable UUID checkinid, @Nullable UUID createdbyid) {
         return checkinNoteServices.findByFields(checkinid, createdbyid);
     }
 

server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteServicesImpl.java

@@ -6,7 +6,6 @@
 import com.objectcomputing.checkins.services.checkins.CheckIn;
 import com.objectcomputing.checkins.services.checkins.CheckInRepository;
 import com.objectcomputing.checkins.services.checkins.CheckInServices;
-import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
 import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository;
 import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices;
 import io.micronaut.core.annotation.Nullable;
@@ -124,20 +123,13 @@ public CheckinNote update(@NotNull CheckinNote checkinNote) {
     }
 
     @Override
-    public Set<CheckinNote> findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) {
+    public Set<CheckinNote> findByFields(@Nullable UUID checkinId, @Nullable UUID createById) {
         final UUID currentUserId = currentUserServices.getCurrentUser().getId();
-        boolean canViewAllCheckins = checkinServices.canViewAllCheckins(currentUserId);
-
-        if (checkinid != null) {
-            validate(!checkinServices.accessGranted(checkinid, currentUserId), "User is unauthorized to do this operation");
-        } else if (createbyid != null) {
-            MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow();
-            validate(!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins, "User is unauthorized to do this operation");
-        } else {
-            validate(!canViewAllCheckins, "User is unauthorized to do this operation");
+        if(!checkinServices.doesUserHaveViewAccess(currentUserId, checkinId, createById)){
+            throw new PermissionException("User is unauthorized to do this operation");
         }
-        LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinid, createbyid);
-        return checkinNoteRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid));
+        LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinId, createById);
+        return checkinNoteRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createById));
     }
 
     private void validate(boolean isError, String message, Object... args) {

server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServices.java

@@ -1,10 +1,10 @@
 package com.objectcomputing.checkins.services.checkins;
 
+import com.objectcomputing.checkins.services.permissions.Permission;
+
 import java.util.Set;
 import java.util.UUID;
 
-import com.objectcomputing.checkins.services.permissions.Permission;
-
 
 public interface CheckInServices {
 
@@ -20,6 +20,8 @@ public interface CheckInServices {
 
     Boolean accessGranted(UUID checkinId, UUID memberId);
 
+    Boolean doesUserHaveViewAccess(UUID currentUserId, UUID checkinId, UUID createdById);
+
     Boolean canViewAllCheckins(UUID memberId);
 
     Boolean canUpdateAllCheckins(UUID memberId);

server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServicesImpl.java

@@ -7,7 +7,6 @@
 import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices;
 import com.objectcomputing.checkins.services.permissions.Permission;
 import com.objectcomputing.checkins.services.role.RoleServices;
-import com.objectcomputing.checkins.services.role.RoleType;
 import com.objectcomputing.checkins.services.role.role_permissions.RolePermissionServices;
 import com.objectcomputing.checkins.util.Util;
 import jakarta.inject.Singleton;
@@ -56,21 +55,10 @@ public Boolean hasPermission(@NotNull UUID memberId, @NotNull Permission permiss
     public Boolean accessGranted(@NotNull UUID checkinId, @NotNull UUID memberId) {
         memberRepo.findById(memberId).orElseThrow(() -> new NotFoundException(String.format("Member %s not found", memberId)));
 
-        boolean canViewAllCheckins = canViewAllCheckins(memberId);
-
-        // TODO: the check for Admin as a role should be removed when permissions are fully implemented
-        boolean isAdmin = false;
-        if (roleServices.findByRole(RoleType.ADMIN.name()).isPresent()){
-            isAdmin = roleServices.findUserRoles(memberId)
-                .contains(roleServices.findByRole(RoleType.ADMIN.name()).get());
-            LOG.debug("Member is Admin: {}", isAdmin);
-        }
+        if(!canViewAllCheckins(memberId)) {
+            CheckIn checkinRecord = checkinRepo.findById(checkinId).orElseThrow(() ->
+                    new NotFoundException(String.format("Checkin not found by Id: %s.", checkinId)));
 
-        Boolean grantAccess = false;
-        if(canViewAllCheckins || isAdmin){
-            grantAccess = true;
-        } else {
-            CheckIn checkinRecord = checkinRepo.findById(checkinId).orElseThrow(() -> new NotFoundException(String.format("Checkin %s not found", checkinId)));
             MemberProfile teamMemberOnCheckin = memberRepo.findById(checkinRecord.getTeamMemberId()).orElseThrow(() ->
                 new NotFoundException(String.format("Team member not found %s not found", checkinRecord.getTeamMemberId())));
             UUID currentPdlId = teamMemberOnCheckin.getPdlId();
@@ -80,13 +68,22 @@ public Boolean accessGranted(@NotNull UUID checkinId, @NotNull UUID memberId) {
             LOG.debug("PDL on Checkin: {}", checkinRecord.getPdlId());
             LOG.debug("Current PDL: {}", currentPdlId);
 
-            if (memberId.equals(checkinRecord.getTeamMemberId())
+            return memberId.equals(checkinRecord.getTeamMemberId())
                     || memberId.equals(checkinRecord.getPdlId())
-                    || memberId.equals(currentPdlId)) {
-                grantAccess = true;
-            }
+                    || memberId.equals(currentPdlId);
+        }
+        return true;
+    }
+
+    @Override
+    public Boolean doesUserHaveViewAccess(UUID currentUserId, UUID checkinId, UUID createdById) {
+        if (canViewAllCheckins(currentUserId)) {
+            return true;
+        } else if (checkinId != null) {
+            return accessGranted(checkinId, currentUserId);
+        } else {
+            return createdById != null && createdById.equals(currentUserId);
         }
-        return grantAccess;
     }
 
     @Override

server/src/main/java/com/objectcomputing/checkins/services/private_notes/PrivateNoteServicesImpl.java

@@ -6,7 +6,6 @@
 import com.objectcomputing.checkins.services.checkins.CheckIn;
 import com.objectcomputing.checkins.services.checkins.CheckInRepository;
 import com.objectcomputing.checkins.services.checkins.CheckInServices;
-import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
 import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository;
 import com.objectcomputing.checkins.services.memberprofile.MemberProfileServices;
 import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices;
@@ -133,22 +132,12 @@ public PrivateNote update(@NotNull PrivateNote privateNote) {
     }
 
     @Override
-    public Set<PrivateNote> findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) {
+    public Set<PrivateNote> findByFields(@Nullable UUID checkinId, @Nullable UUID createById) {
         final UUID currentUserId = currentUserServices.getCurrentUser().getId();
-        boolean canViewAllCheckins = checkinServices.canViewAllCheckins(currentUserId);
-
-        if (checkinid != null) {
-            if (!checkinServices.accessGranted(checkinid, currentUserId))
-                throw new PermissionException("User is unauthorized to do this operation");
-        } else if (createbyid != null) {
-            MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow();
-            if (!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins)
-                throw new PermissionException("User is unauthorized to do this operation");
-        } else if (!canViewAllCheckins) {
+        if(!checkinServices.doesUserHaveViewAccess(currentUserId, checkinId, createById)){
             throw new PermissionException("User is unauthorized to do this operation");
         }
-
-        return privateNoteRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid));
+        return privateNoteRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createById));
     }
 
     private void validate(boolean isError, String message, Object... args) {

server/src/main/java/com/objectcomputing/checkins/services/validate/crud/ActionItemCRUDValidator.java

@@ -1,5 +1,6 @@
 package com.objectcomputing.checkins.services.validate.crud;
 
+import com.objectcomputing.checkins.exceptions.PermissionException;
 import com.objectcomputing.checkins.services.action_item.ActionItem;
 import com.objectcomputing.checkins.services.action_item.ActionItemRepository;
 import com.objectcomputing.checkins.services.checkins.CheckIn;
@@ -154,20 +155,10 @@ public void validatePermissionsUpdate(@Valid @NotNull ActionItem actionItem) {
     }
 
     @Override
-    public void validatePermissionsFindByFields(UUID checkinid, UUID createdbyid) {
+    public void validatePermissionsFindByFields(UUID checkinId, UUID createdById) {
         final UUID currentUserId = currentUserServices.getCurrentUser().getId();
-        boolean canViewAllCheckins = checkInServices.canViewAllCheckins(currentUserId);
-
-        if (checkinid != null) { // find by checkinId validation
-            boolean allowedToView = checkInServices.accessGranted(checkinid, currentUserId);
-            permissionsValidation.validatePermissions(!allowedToView, "User is unauthorized to do this operation");
-        } else if (createdbyid != null) { // find by createById validation
-            final UUID memberRecordId = memberServices.getById(createdbyid).getId();
-            boolean currentUserIsCheckinSubject = currentUserId.equals(memberRecordId);
-            permissionsValidation.validatePermissions(!currentUserIsCheckinSubject && !canViewAllCheckins,
-                "User is unauthorized to do this operation");
-        } else { // find all validation
-            permissionsValidation.validatePermissions(!canViewAllCheckins, "User is unauthorized to do this operation");
+        if(!checkInServices.doesUserHaveViewAccess(currentUserId, checkinId, createdById)){
+            throw new PermissionException("User is unauthorized to do this operation");
         }
     }
 

server/src/test/java/com/objectcomputing/checkins/services/action_item/ActionItemControllerTest.java

@@ -20,19 +20,11 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.UUID;
+import java.util.*;
 import java.util.stream.Collectors;
 
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.ADMIN_ROLE;
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.MEMBER_ROLE;
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.PDL_ROLE;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static com.objectcomputing.checkins.services.role.RoleType.Constants.*;
+import static org.junit.jupiter.api.Assertions.*;
 
 class ActionItemControllerTest extends TestContainersSuite implements MemberProfileFixture, RoleFixture, CheckInFixture, ActionItemFixture {
 
@@ -174,7 +166,7 @@ void testCreateAnActionItemForExistingCheckInId() {
         String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText();
 
         assertEquals(request.getPath(), href);
-        assertEquals(String.format("Checkin %s not found", actionItemCreateDTO.getCheckinid()), error);
+        assertEquals(String.format("Checkin not found by Id: %s.", actionItemCreateDTO.getCheckinid()), error);
         assertEquals(HttpStatus.NOT_FOUND, responseException.getStatus());
 
     }
@@ -730,7 +722,7 @@ void testUpdateNonExistingActionItemForCheckInId() {
         String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText();
 
         assertEquals(request.getPath(), href);
-        assertEquals(String.format("Checkin %s not found", actionItem.getCheckinid()), error);
+        assertEquals(String.format("Checkin not found by Id: %s.", actionItem.getCheckinid()), error);
 
     }
 

server/src/test/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemControllerTest.java

@@ -19,19 +19,11 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.UUID;
+import java.util.*;
 import java.util.stream.Collectors;
 
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.ADMIN_ROLE;
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.MEMBER_ROLE;
-import static com.objectcomputing.checkins.services.role.RoleType.Constants.PDL_ROLE;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static com.objectcomputing.checkins.services.role.RoleType.Constants.*;
+import static org.junit.jupiter.api.Assertions.*;
 
 class AgendaItemControllerTest extends TestContainersSuite implements MemberProfileFixture, CheckInFixture, AgendaItemFixture, RoleFixture {
 
@@ -346,7 +338,7 @@ void testFindAllAgendaItemByNonAdmin() {
         String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText();
 
         assertEquals(request.getPath(), href);
-        assertEquals("User is unauthorized to do this operation", error);
+        assertTrue(error.contains("User is unauthorized to do this operation"));
 
     }
 

server/src/test/java/com/objectcomputing/checkins/services/checkins/CheckInControllerTest.java

@@ -14,20 +14,16 @@
 import io.micronaut.http.client.HttpClient;
 import io.micronaut.http.client.annotation.Client;
 import io.micronaut.http.client.exceptions.HttpClientResponseException;
-
+import jakarta.inject.Inject;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-import jakarta.inject.Inject;
 import java.time.LocalDateTime;
 import java.util.*;
 import java.util.stream.Collectors;
 
 import static com.objectcomputing.checkins.services.role.RoleType.Constants.*;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.*;
 
 class CheckInControllerTest extends TestContainersSuite implements MemberProfileFixture, CheckInFixture, RoleFixture {
 
@@ -375,7 +371,7 @@ void testGetReadCheckInDoesNotExist() {
         String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText();
 
         assertEquals(request.getPath(), href);
-        assertEquals(String.format("Checkin %s not found", randomCheckinID), error);
+        assertEquals(String.format("Checkin not found by Id: %s.", randomCheckinID), error);
     }
 
     @Test