Merge branch '557-course-filter-approved-requests-broken' into 'development'

Resolve "Course filter approved requests broken"

Closes #557

See merge request !628

CHANGELOG.md

@@ -50,6 +50,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Prevent error on returning to catalog or enrol page after enrolling in a course edition [@cchen9](https://gitlab.ewi.tudelft.nl/cchen9)
 - Form submission buttons on requests are disabled after they are pressed, preventing 2 requests to be sent. [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
 - Feedback is now deleted when the associated request is deleted [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
+- Assistants can no longer see requests belonging to other editions in a shared session [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
 
 ## [1.2.0] - [2020-06-06]
 ### Added

build.gradle.kts

@@ -9,7 +9,7 @@ version = "2.0.0"
 val javaVersion = JavaVersion.VERSION_17
 
 val libradorVersion = "1.0.3-SNAPSHOT6"
-val labradoorVersion = "1.3.1-SNAPSHOT"
+val labradoorVersion = "1.3.5"
 val queryDslVersion = "4.4.0"
 
 // A definition of all dependencies and repositories where to find them that need to

src/main/java/nl/tudelft/queue/controller/RequestController.java

@@ -46,6 +46,7 @@ import nl.tudelft.queue.repository.LabRequestRepository;
 import nl.tudelft.queue.service.*;
 
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.data.domain.PageImpl;
 import org.springframework.data.domain.Pageable;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.web.PageableDefault;
@@ -144,10 +145,16 @@ public class RequestController {
 				.map(qs -> (Lab) qs)
 				.collect(Collectors.toList());
 
+		List<LabRequest> filteredRequests = rs
+				.filterRequestsSharedEditionCheck(lrr.findAllByFilter(labs, filter, pageable).getContent(),
+						assistant);
+
+		var requestsViews = rts.convertRequestsToView(
+				new PageImpl<>(filteredRequests, pageable, filteredRequests.size()), filteredRequests.size());
+
 		model.addAttribute("page", "requests");
 		model.addAttribute("filter", filter);
-		model.addAttribute("requests", rts.convertRequestsToView(
-				lrr.findAllByFilter(labs, filter, pageable), lrr.countByFilter(labs, filter)));
+		model.addAttribute("requests", requestsViews);
 		model.addAttribute("requestCounts", rts.labRequestCounts(
 				labs, assistant, filter));
 

src/main/java/nl/tudelft/queue/repository/LabRequestRepository.java

@@ -35,10 +35,7 @@ import nl.tudelft.queue.model.enums.RequestStatus;
 import nl.tudelft.queue.model.labs.AbstractSlottedLab;
 import nl.tudelft.queue.model.labs.Lab;
 
-import org.springframework.data.domain.Page;
-import org.springframework.data.domain.PageRequest;
-import org.springframework.data.domain.Pageable;
-import org.springframework.data.domain.Sort;
+import org.springframework.data.domain.*;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.querydsl.QuerydslPredicateExecutor;
 import org.springframework.lang.NonNull;

src/main/java/nl/tudelft/queue/service/PermissionService.java

@@ -330,7 +330,7 @@ public class PermissionService {
 	 * @return            Whether the currently authenticated person has a staff role in any of the given
 	 *                    course editions.
 	 */
-	private boolean hasStaffRole(List<Long> editionIds) {
+	public boolean hasStaffRole(List<Long> editionIds) {
 		return withAnyRole(editionIds, (person, role) -> STAFF_ROLES.contains(role));
 	}
 

src/main/java/nl/tudelft/queue/service/RequestService.java

@@ -24,6 +24,7 @@ import java.util.stream.Collectors;
 
 import javax.transaction.Transactional;
 
+import nl.tudelft.labracore.api.AssignmentControllerApi;
 import nl.tudelft.labracore.api.ModuleControllerApi;
 import nl.tudelft.labracore.api.QuestionControllerApi;
 import nl.tudelft.labracore.api.StudentGroupControllerApi;
@@ -80,6 +81,12 @@ public class RequestService {
 	@Autowired
 	private QuestionControllerApi qApi;
 
+	@Autowired
+	private AssignmentControllerApi asApi;
+
+	@Autowired
+	private RoleDTOService roleDTOService;
+
 	@Autowired
 	private PersonCacheManager pCache;
 
@@ -99,6 +106,9 @@ public class RequestService {
 	@Lazy
 	private LabService lService;
 
+	@Autowired
+	private PermissionService permissionService;
+
 	private static final ReentrantLock lock = new ReentrantLock();
 
 	/**
@@ -505,6 +515,34 @@ public class RequestService {
 		});
 	}
 
+	/**
+	 * This is meant to act as a filter for a list of lab requests that originates from a shared session. This
+	 * is so that assistants from other editions don't see requests from other editions they are not an
+	 * assistant of.
+	 *
+	 * @param  requests A list of requests to filter
+	 * @param  person   The assistant that will see the lab requests
+	 * @return          A new list of lab requests that the assistant will see only if they are an assistant
+	 *                  of the edition the request belongs.
+	 */
+	public List<LabRequest> filterRequestsSharedEditionCheck(List<LabRequest> requests, Person person) {
+		// Check to make sure requests are not empty, otherwise API call fails.
+		if (requests.isEmpty()) {
+			return requests;
+		}
+
+		var assignmentIds = requests.stream().map(LabRequest::getAssignment).distinct().toList();
+
+		var allowedAssignments = Objects
+				.requireNonNull(asApi.getAssignmentsWithModules(assignmentIds).collectList().block()).stream()
+				.filter(assignment -> permissionService
+						.hasStaffRole(List.of(assignment.getModule().getEdition().getId())))
+				.map(AssignmentModuleDetailsDTO::getId)
+				.collect(Collectors.toSet());
+
+		return requests.stream().filter(r -> allowedAssignments.contains(r.getAssignment())).toList();
+	}
+
 	/**
 	 * Creates an individual student group for one student without a current group so that they may place a
 	 * request with that student group.

src/test/java/nl/tudelft/queue/service/RequestServiceTest.java

@@ -29,10 +29,14 @@ import java.util.stream.Collectors;
 
 import javax.transaction.Transactional;
 
+import nl.tudelft.labracore.api.AssignmentControllerApi;
+import nl.tudelft.labracore.api.RoleControllerApi;
 import nl.tudelft.labracore.api.StudentGroupControllerApi;
 import nl.tudelft.labracore.api.dto.*;
+import nl.tudelft.labracore.lib.security.user.Person;
 import nl.tudelft.queue.cache.SessionCacheManager;
 import nl.tudelft.queue.dto.create.requests.SelectionRequestCreateDTO;
+import nl.tudelft.queue.model.LabRequest;
 import nl.tudelft.queue.model.QSelectionRequest;
 import nl.tudelft.queue.model.SelectionRequest;
 import nl.tudelft.queue.model.embeddables.CapacitySessionConfig;
@@ -40,6 +44,7 @@ import nl.tudelft.queue.model.embeddables.LabRequestConstraints;
 import nl.tudelft.queue.model.enums.RequestStatus;
 import nl.tudelft.queue.model.enums.SelectionProcedure;
 import nl.tudelft.queue.model.labs.CapacitySession;
+import nl.tudelft.queue.model.labs.RegularLab;
 import nl.tudelft.queue.repository.CapacitySessionRepository;
 import nl.tudelft.queue.repository.SelectionRequestRepository;
 
@@ -47,11 +52,14 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.SpyBean;
 import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.test.context.support.WithUserDetails;
 import org.springframework.test.annotation.DirtiesContext;
 
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
+import test.TestDatabaseLoader;
 import test.labracore.*;
 import test.test.TestQueueApplication;
 
@@ -89,9 +97,17 @@ public class RequestServiceTest {
 
 	private CapacitySession session;
 
+	private RegularLab oopNowRegularLab1;
+
+	private RoleDetailsDTO[] oopNowTAs;
+
+	private LabRequest[] rlOopNowSharedLabRequests;
+
 	CapacitySessionConfig.CapacitySessionConfigBuilder sessionConfigBuilderSelectNow;
 	CapacitySession.CapacitySessionBuilder<?, ? extends CapacitySession.CapacitySessionBuilder<?, ?>> sessionBuilder;
 
+	@Autowired
+	private TestDatabaseLoader db;
 	@Autowired
 	private CapacitySessionRepository csr;
 
@@ -116,12 +132,24 @@ public class RequestServiceTest {
 	@Autowired
 	private PersonApiMocker pApiMocker;
 
+	@Autowired
+	private RoleApiMocker rlApiMocker;
+
 	@Autowired
 	private ModuleApiMocker mApiMocker;
 
+	@Autowired
+	private AssignmentApiMocker asApiMocker;
+
+	@SpyBean
+	private AssignmentControllerApi asApi;
+
 	@Autowired
 	private StudentGroupControllerApi sgApi;
 
+	@Autowired
+	private RoleControllerApi rlApi;
+
 	@BeforeEach
 	void setUp() {
 		sessionBuilder = CapacitySession.builder()
@@ -134,6 +162,12 @@ public class RequestServiceTest {
 				.enrolmentClosesAt(LocalDateTime.now().minusDays(1))
 				.selectionAt(LocalDateTime.now().minusMinutes(3));
 
+		oopNowRegularLab1 = db.getOopNowRegularLab1();
+
+		oopNowTAs = db.getOopNowTAs();
+
+		rlOopNowSharedLabRequests = db.getRlOopNowSharedLabRequests();
+
 		sApiMocker.save(lcSessionNow);
 		sApiMocker.save(lcSessionOld1);
 		sApiMocker.save(lcSessionOld2);
@@ -143,8 +177,10 @@ public class RequestServiceTest {
 		sApiMocker.mock();
 		sgApiMocker.mock();
 		rApiMocker.mock();
+		rlApiMocker.mock();
 		pApiMocker.mock();
 		mApiMocker.mock();
+		asApiMocker.mock();
 	}
 
 	private void mockStudentGroup(Long moduleId, List<Long> requesterIds, Long studentGroupId) {
@@ -304,4 +340,22 @@ public class RequestServiceTest {
 		verify(sgApi).getStudentGroupsById(List.of(6L));
 		verify(sgApi).addGroup(any());
 	}
+
+	@Test
+	void sharedEditionFilterDoesNotCallApiWhenEmptyRequests() {
+		assertThat(rs.filterRequestsSharedEditionCheck(new ArrayList<>(), new Person())).isEmpty();
+
+		verify(asApi, never()).getAssignmentsWithModules(any());
+
+	}
+
+	@Test
+	@WithUserDetails("student200")
+	void oopTaGetsOopRequestsOnlyInSharedSession() {
+		Person p1 = new Person();
+		p1.setId(oopNowTAs[0].getPerson().getId());
+		assertThat(rs.filterRequestsSharedEditionCheck(Arrays.stream(rlOopNowSharedLabRequests).toList(), p1))
+				.containsExactly(rlOopNowSharedLabRequests);
+	}
+
 }

src/test/java/test/TestDatabaseLoader.java

@@ -267,6 +267,8 @@ public class TestDatabaseLoader {
 	private RegularLab oopNowRegularLab2;
 	private LabRequest[] oopNowRegularLab2Requests;
 
+	private LabRequest[] rlOopNowSharedLabRequests;
+
 	private RegularLab oopNowRegularLab3;
 	private RegularLab oopNowRegularLab4;
 
@@ -753,6 +755,8 @@ public class TestDatabaseLoader {
 		oopNowRegularLab1Requests = new LabRequest[50];
 		oopNowRegularLab2Requests = new LabRequest[50];
 
+		rlOopNowSharedLabRequests = new LabRequest[50];
+
 		oopPastLectureRequests = new SelectionRequest[50];
 
 		for (int i = 50; i < 100; i++) {
@@ -763,6 +767,9 @@ public class TestDatabaseLoader {
 					RequestType.SUBMISSION,
 					null);
 
+			var reqSharedLab1 = createLabRequest(rlOopNowSharedLab, students[i],
+					oopNowAssignments[i % oopNowAssignments.length], RequestType.SUBMISSION, null);
+
 			var reqPastLecture = createSelectionRequest(oopPastLecture, students[i]);
 			createSelectionRequest(oopLectureRandom, students[i]);
 			createSelectionRequest(oopLectureWeightLR, students[i]);
@@ -788,6 +795,7 @@ public class TestDatabaseLoader {
 			oopNowRegularLab1Requests[i - 50] = reqRegLab1;
 			oopNowRegularLab2Requests[i - 50] = reqRegLab2;
 			oopPastLectureRequests[i - 50] = reqPastLecture;
+			rlOopNowSharedLabRequests[i - 50] = reqSharedLab1;
 		}
 	}
 
@@ -1314,8 +1322,9 @@ public class TestDatabaseLoader {
 		var nAssignments = 3;
 		var assignments = new AssignmentDetailsDTO[nAssignments];
 		for (int i = 0; i < nAssignments; i++) {
+			long assignmentId = module.getId() * 1000L + i;
 			assignments[i] = aApiMocker.save(new AssignmentDetailsDTO()
-					.id(module.getId() * 1000L + i)
+					.id(assignmentId)
 					.name("Assignment " + i)
 					.sessions(new ArrayList<>())
 					.description(new Description().text("Nothing special, just an assignment"))
@@ -1327,6 +1336,8 @@ public class TestDatabaseLoader {
 					.submissions(new ArrayList<>())
 					.submissionLimit(null));
 			module.addAssignmentsItem(toView(assignments[i], AssignmentSummaryDTO.class));
+
+			aApiMocker.initializeAssignmentWithModule(assignmentId, module);
 		}
 		return assignments;
 	}

src/test/java/test/labracore/AssignmentApiMocker.java

@@ -20,28 +20,41 @@ package test.labracore;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 
 import lombok.AllArgsConstructor;
 import nl.tudelft.labracore.api.AssignmentControllerApi;
-import nl.tudelft.labracore.api.dto.AssignmentDetailsDTO;
-import nl.tudelft.labracore.api.dto.AssignmentSummaryDTO;
+import nl.tudelft.labracore.api.dto.*;
 
 import org.modelmapper.ModelMapper;
 
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 
+import com.google.common.base.Predicates;
+import com.google.common.collect.Maps;
+
 @AllArgsConstructor
 public class AssignmentApiMocker extends LabracoreApiMocker<AssignmentDetailsDTO, Long> {
 	private final AssignmentControllerApi aApi;
 
+	private final Map<Long, AssignmentModuleDetailsDTO> assignmentsWithModules = new HashMap<>();
+
 	@Override
 	public void mock() {
 		when(aApi.getAllAssignmentsById(any())).thenAnswer(getAllByIds);
 
 		when(aApi.getAssignmentById(any())).thenReturn(Mono.empty());
 
+		when(aApi.getAssignmentsWithModules(any())).thenAnswer(invocation -> {
+			List<Long> ids = invocation.getArgument(0);
+			return Flux.fromIterable(Maps.filterKeys(assignmentsWithModules, Predicates.in(ids)).values());
+		});
+
 		when(aApi.getAllAssignments()).thenAnswer(invocation -> {
 			var mapper = new ModelMapper();
 			var summaries = data.values().stream()
@@ -52,8 +65,27 @@ public class AssignmentApiMocker extends LabracoreApiMocker<AssignmentDetailsDTO
 		});
 	}
 
+	@Override
+	public AssignmentDetailsDTO save(AssignmentDetailsDTO dto) {
+		super.save(dto);
+
+		var assignmentWithModule = mapper.map(dto, AssignmentModuleDetailsDTO.class);
+
+		assignmentsWithModules.put(Objects.requireNonNull(dto.getId()), assignmentWithModule);
+
+		return dto;
+	}
+
 	@Override
 	public Long getId(AssignmentDetailsDTO dto) {
 		return dto.getId();
 	}
+
+	public void initializeAssignmentWithModule(long assignmentId, ModuleDetailsDTO module) {
+		var moduleSummary = Objects.requireNonNull(assignmentsWithModules.get(assignmentId));
+
+		var mappedModule = mapper.map(module, ModuleLayer1DTO.class);
+
+		moduleSummary.setModule(mappedModule);
+	}
 }