Skip to content
Snippets Groups Projects
Commit 79a6960c authored by Ruben Backx's avatar Ruben Backx :coffee:
Browse files

Merge branch '660-feedback-page-should-use-all-feedbacks-for-summary' into 'development' 

Resolve "Feedback page should use all feedbacks for summary"

See merge request !728
parents 7cd39805 8dc5c1c5
Branches
Tags
2 merge requests!728Resolve "Feedback page should use all feedbacks for summary",!724Deploy
Show whitespace changes
Inline Side-by-side
Showing
with 364 additions and 140 deletions
CHANGELOG.md
+ 11
− 0
View file @ 79a6960c
...@@ -9,6 +9,17 @@ All notable changes to this project will be documented in this file. ...@@ -9,6 +9,17 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
## [Unreleased]
### Added
### Changed
- Written feedback sorting now follows a reverse chronological order. [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
### Fixed
- Teachers can now view their own feedback again. [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
- The feedback graph for assistants now incorporates feedback from other courses, with written feedback limited to visibility for course managers. [@hpage](https://gitlab.ewi.tudelft.nl/hpage)
## [2.2.2] ## [2.2.2]
### Added ### Added
......
src/main/java/nl/tudelft/queue/PageUtil.java
+ 27
− 5
View file @ 79a6960c
...@@ -17,6 +17,8 @@ ...@@ -17,6 +17,8 @@
*/ */
package nl.tudelft.queue; package nl.tudelft.queue;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List; import java.util.List;
import org.springframework.data.domain.Page; import org.springframework.data.domain.Page;
...@@ -27,15 +29,23 @@ import org.springframework.data.domain.Pageable; ...@@ -27,15 +29,23 @@ import org.springframework.data.domain.Pageable;
* A utility class used for turning lists into pages. * A utility class used for turning lists into pages.
*/ */
public class PageUtil { public class PageUtil {
/** /**
* Turns the given list of values into a page of values which represent a sublist of the original. * Turns the given list of values into a page of values which represent a sublist of the original.
* *
* @param pageable The pageable describing what to show on the page. * @param pageable The pageable describing what to show on the page.
* @param values The total list of values of which a page needs to be shown. * @param values The total list of values of which a page needs to be shown.
* @param comparator The comparator used to sort the values
* @param <T> The type of the values in the page. * @param <T> The type of the values in the page.
* @return The Page cut out from the original list of values. * @return The Page cut out from the original list of values.
*/ */
public static <T> Page<T> toPage(Pageable pageable, List<T> values) { public static <T> Page<T> toPage(Pageable pageable, List<T> values, Comparator<T> comparator) {
if (values == null || values.isEmpty())
return new PageImpl<>(new ArrayList<>(), pageable, 0);
values = new ArrayList<>(values);
if (comparator != null) {
values.sort(comparator);
}
int start = (int) pageable.getOffset(); int start = (int) pageable.getOffset();
int end = Math.min(start + pageable.getPageSize(), values.size()); int end = Math.min(start + pageable.getPageSize(), values.size());
...@@ -45,4 +55,16 @@ public class PageUtil { ...@@ -45,4 +55,16 @@ public class PageUtil {
return new PageImpl<>(values.subList(start, end), pageable, values.size()); return new PageImpl<>(values.subList(start, end), pageable, values.size());
} }
/**
* Turns the given list of values into a page of values which represent a sublist of the original.
*
* @param pageable The pageable describing what to show on the page.
* @param values The total list of values of which a page needs to be shown.
* @param <T> The type of the values in the page.
* @return The Page cut out from the original list of values.
*/
public static <T> Page<T> toPage(Pageable pageable, List<T> values) {
return toPage(pageable, values, null);
}
} }
src/main/java/nl/tudelft/queue/controller/EditionController.java
+ 3
− 2
View file @ 79a6960c
...@@ -176,7 +176,7 @@ public class EditionController { ...@@ -176,7 +176,7 @@ public class EditionController {
if (person.getDefaultRole() != DefaultRole.ADMIN) { if (person.getDefaultRole() != DefaultRole.ADMIN) {
editions = editions.stream().filter(e -> !e.getHidden()).toList(); editions = editions.stream().filter(e -> !e.getHidden()).toList();
} }
var page = PageUtil.toPage(pageable, editions); var page = PageUtil.toPage(pageable, editions, Comparator.comparing(QueueEditionDetailsDTO::getId));
model.addAttribute("editions", page); model.addAttribute("editions", page);
model.addAttribute("programs", cCache.getAll() model.addAttribute("programs", cCache.getAll()
...@@ -304,7 +304,8 @@ public class EditionController { ...@@ -304,7 +304,8 @@ public class EditionController {
mCache.getAndIgnoreMissing(edition.getModules().stream().map(ModuleSummaryDTO::getId)) mCache.getAndIgnoreMissing(edition.getModules().stream().map(ModuleSummaryDTO::getId))
.stream() .stream()
.flatMap(m -> m.getAssignments().stream()).toList()); .flatMap(m -> m.getAssignments().stream()).toList());
model.addAttribute("students", toPage(pageable, students)); model.addAttribute("students",
toPage(pageable, students, Comparator.comparing(PersonSummaryDTO::getDisplayName)));
return "edition/view/participants"; return "edition/view/participants";
} }
......
src/main/java/nl/tudelft/queue/controller/HomeController.java
+ 12
− 7
View file @ 79a6960c
...@@ -50,7 +50,6 @@ import nl.tudelft.queue.service.LabService; ...@@ -50,7 +50,6 @@ import nl.tudelft.queue.service.LabService;
import nl.tudelft.queue.service.PermissionService; import nl.tudelft.queue.service.PermissionService;
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Pageable;
import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.security.access.prepost.PreAuthorize;
...@@ -305,6 +304,7 @@ public class HomeController { ...@@ -305,6 +304,7 @@ public class HomeController {
*/ */
private void fillInFeedbackModel(Long assistantId, Person person, Model model, Pageable pageable, private void fillInFeedbackModel(Long assistantId, Person person, Model model, Pageable pageable,
Boolean restrictToCourseManager) { Boolean restrictToCourseManager) {
var assistant = pCache.getRequired(assistantId); var assistant = pCache.getRequired(assistantId);
if (!Objects.equals(person.getId(), assistantId) if (!Objects.equals(person.getId(), assistantId)
...@@ -314,16 +314,21 @@ public class HomeController { ...@@ -314,16 +314,21 @@ public class HomeController {
"Teachers are not permitted to view the feedback of other teachers."); "Teachers are not permitted to view the feedback of other teachers.");
} }
Page<Feedback> feedback = assistantId.equals(person.getId()) List<Feedback> feedback = assistantId.equals(person.getId())
? fr.findByAssistantAnonymised(assistantId, pageable) ? fr.findByAssistantAnonymised(assistantId)
: restrictToCourseManager : restrictToCourseManager
? fs.filterFeedbackForManagerCourses(fr.findByAssistant(assistantId), pageable) ? fs.filterFeedbackForManagerCourses(fr.findByAssistant(assistantId))
: PageUtil.toPage(pageable, fr.findByAssistant(assistantId)); : fr.findByAssistant(assistantId);
model.addAttribute("assistant", assistant); model.addAttribute("assistant", assistant);
model.addAttribute("feedback", model.addAttribute("feedback",
feedback.map(fb -> View.convert(fb, FeedbackViewDTO.class))); PageUtil.toPage(pageable, feedback,
model.addAttribute("stars", fs.countRatings(assistantId)); Comparator.comparing(Feedback::getLastUpdatedAt).reversed())
.map(fb -> View.convert(fb, FeedbackViewDTO.class)));
model.addAttribute("stars",
assistantId.equals(person.getId())
? fs.countRatings(fr.findByAssistantAnonymised(assistantId))
: fs.countRatings(fr.findAllByAssistant(assistantId)));
} }
/** /**
......
src/main/java/nl/tudelft/queue/repository/FeedbackRepository.java
+ 4
− 19
View file @ 79a6960c
...@@ -26,9 +26,6 @@ import java.util.stream.StreamSupport; ...@@ -26,9 +26,6 @@ import java.util.stream.StreamSupport;
import nl.tudelft.queue.model.Feedback; import nl.tudelft.queue.model.Feedback;
import nl.tudelft.queue.model.QFeedback; import nl.tudelft.queue.model.QFeedback;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageImpl;
import org.springframework.data.domain.Pageable;
import org.springframework.data.querydsl.QuerydslPredicateExecutor; import org.springframework.data.querydsl.QuerydslPredicateExecutor;
import org.springframework.data.repository.CrudRepository; import org.springframework.data.repository.CrudRepository;
import org.springframework.lang.NonNull; import org.springframework.lang.NonNull;
...@@ -61,14 +58,6 @@ public interface FeedbackRepository ...@@ -61,14 +58,6 @@ public interface FeedbackRepository
return findAll(qf.id.assistantId.eq(assistantId)); return findAll(qf.id.assistantId.eq(assistantId));
} }
/**
* @param assistantId The id of the assistant to lookup feedback for.
* @return All feedback objects submitted for the assistant with the given id.
*/
default List<Feedback> findAllWithRatingByAssistant(Long assistantId) {
return findAll(qf.id.assistantId.eq(assistantId).and(qf.rating.isNotNull()));
}
/** /**
* @param assistantId The assistant to find Feedback for. * @param assistantId The assistant to find Feedback for.
* @return The page of feedback for the given Assistant. * @return The page of feedback for the given Assistant.
...@@ -81,20 +70,16 @@ public interface FeedbackRepository ...@@ -81,20 +70,16 @@ public interface FeedbackRepository
/** /**
* Gets the feedback for an assistant, but leaves out too recent feedback. * Gets the feedback for an assistant, but leaves out too recent feedback.
* *
* @param assistantId The assistant to find Feedback for. * @param assistantId The assistant to find the feedback for.
* @param pageable The pageable object to page feedback with. * @return The list of feedbacks that match the aforementioned criteria.
* @return The page of feedback for the given Assistant.
*/ */
default Page<Feedback> findByAssistantAnonymised(Long assistantId, Pageable pageable) { default List<Feedback> findByAssistantAnonymised(Long assistantId) {
var feedback = StreamSupport.stream(findAll(qf.id.assistantId.eq(assistantId) return StreamSupport.stream(findAll(qf.id.assistantId.eq(assistantId)
.and(qf.createdAt.before(LocalDateTime.now().minusWeeks(2))), qf.createdAt.desc()) .and(qf.createdAt.before(LocalDateTime.now().minusWeeks(2))), qf.createdAt.desc())
.spliterator(), .spliterator(),
false) false)
.skip(5) .skip(5)
.filter(f -> f.getFeedback() != null && !f.getFeedback().isEmpty()) .filter(f -> f.getFeedback() != null && !f.getFeedback().isEmpty())
.collect(Collectors.toList()); .collect(Collectors.toList());
var page = feedback.subList(pageable.getPageNumber() * pageable.getPageSize(),
Math.min(feedback.size(), (pageable.getPageNumber() + 1) * pageable.getPageSize()));
return new PageImpl<>(page, pageable, feedback.size());
} }
} }
src/main/java/nl/tudelft/queue/service/FeedbackService.java
+ 9
− 14
View file @ 79a6960c
...@@ -29,7 +29,6 @@ import javax.validation.ValidationException; ...@@ -29,7 +29,6 @@ import javax.validation.ValidationException;
import nl.tudelft.labracore.api.dto.PersonSummaryDTO; import nl.tudelft.labracore.api.dto.PersonSummaryDTO;
import nl.tudelft.labracore.api.dto.SessionDetailsDTO; import nl.tudelft.labracore.api.dto.SessionDetailsDTO;
import nl.tudelft.queue.PageUtil;
import nl.tudelft.queue.cache.PersonCacheManager; import nl.tudelft.queue.cache.PersonCacheManager;
import nl.tudelft.queue.cache.SessionCacheManager; import nl.tudelft.queue.cache.SessionCacheManager;
import nl.tudelft.queue.dto.patch.FeedbackPatchDTO; import nl.tudelft.queue.dto.patch.FeedbackPatchDTO;
...@@ -40,8 +39,6 @@ import nl.tudelft.queue.repository.LabRequestRepository; ...@@ -40,8 +39,6 @@ import nl.tudelft.queue.repository.LabRequestRepository;
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Lazy; import org.springframework.context.annotation.Lazy;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
@Service @Service
...@@ -81,16 +78,16 @@ public class FeedbackService { ...@@ -81,16 +78,16 @@ public class FeedbackService {
} }
/** /**
* Counts the occurrences of each of the five star ratings and returns the counts of ratings for the * Counts the occurences of each of the five-star ratings and returns the counts of ratings for the given
* assistant with the given id. * lists of feedbacks.
* *
* @param assistantId The id of the assistant for which to count ratings. * @param feedbacks The list of feedbacks to consider.
* @return The counts of feedbacks with each of the five ratings. * @return The number of ratings (pertaining to a scale from 1 to 5) in buckets.
*/ */
public List<Integer> countRatings(Long assistantId) { public List<Integer> countRatings(List<Feedback> feedbacks) {
List<Integer> counts = new ArrayList<>(List.of(0, 0, 0, 0, 0)); List<Integer> counts = new ArrayList<>(List.of(0, 0, 0, 0, 0));
for (var feedback : fr.findAllWithRatingByAssistant(assistantId)) { for (Feedback feedback : feedbacks.stream().filter(fb -> fb.getRating() != null).toList()) {
counts.set(feedback.getRating() - 1, counts.get(feedback.getRating() - 1) + 1); counts.set(feedback.getRating() - 1, counts.get(feedback.getRating() - 1) + 1);
} }
...@@ -100,7 +97,7 @@ public class FeedbackService { ...@@ -100,7 +97,7 @@ public class FeedbackService {
/** /**
* Updates feedback by the given request id and assistant id, but only if a feedback is already in the * Updates feedback by the given request id and assistant id, but only if a feedback is already in the
* database by these ids. If none is found, a new feedback object is saved to the database and the given * database by these ids. If none is found, a new feedback object is saved to the database and the given
* rating and feedback string are filled in in it. * rating and feedback string are filled in it.
* *
* @param request The request the feedback is on. * @param request The request the feedback is on.
* @param assistantId The id of the assistant the feedback is on. * @param assistantId The id of the assistant the feedback is on.
...@@ -132,10 +129,9 @@ public class FeedbackService { ...@@ -132,10 +129,9 @@ public class FeedbackService {
* managed. * managed.
* *
* @param feedback The list of feedback to be filtered. * @param feedback The list of feedback to be filtered.
* @param pageable The pageable used for paging.
* @return A Page of feedback that the manager can see. * @return A Page of feedback that the manager can see.
*/ */
public Page<Feedback> filterFeedbackForManagerCourses(List<Feedback> feedback, Pageable pageable) { public List<Feedback> filterFeedbackForManagerCourses(List<Feedback> feedback) {
List<Long> sessionIds = feedback.stream() List<Long> sessionIds = feedback.stream()
.map(fb -> fb.getRequest().getSession().getSession()) .map(fb -> fb.getRequest().getSession().getSession())
.distinct() .distinct()
...@@ -152,7 +148,6 @@ public class FeedbackService { ...@@ -152,7 +148,6 @@ public class FeedbackService {
.filter(fb -> canManage.getOrDefault(fb.getRequest().getSession().getSession(), false)) .filter(fb -> canManage.getOrDefault(fb.getRequest().getSession().getSession(), false))
.toList(); .toList();
return PageUtil.toPage(pageable, filteredFeedback); return filteredFeedback;
} }
} }
src/main/java/nl/tudelft/queue/service/RequestTableService.java
+ 1
− 0
View file @ 79a6960c
...@@ -276,6 +276,7 @@ public class RequestTableService { ...@@ -276,6 +276,7 @@ public class RequestTableService {
.filter(r -> r.getEventInfo().getStatus() != RequestStatus.FORWARDED)) .filter(r -> r.getEventInfo().getStatus() != RequestStatus.FORWARDED))
.toList(); .toList();
} }
return PageUtil.toPage(pageable, convertRequestsToView(sortedRequestList)); return PageUtil.toPage(pageable, convertRequestsToView(sortedRequestList));
} }
......
src/main/resources/templates/home/feedback.html
+ 103
− 84
View file @ 79a6960c
...@@ -23,6 +23,7 @@ ...@@ -23,6 +23,7 @@
<!--@thymesVar id="assistant" type="nl.tudelft.labracore.api.dto.PersonDetailsDTO"--> <!--@thymesVar id="assistant" type="nl.tudelft.labracore.api.dto.PersonDetailsDTO"-->
<!--@thymesVar id="feedback" type="org.springframework.data.domain.Page<nl.tudelft.queue.dto.view.FeedbackViewDTO>"--> <!--@thymesVar id="feedback" type="org.springframework.data.domain.Page<nl.tudelft.queue.dto.view.FeedbackViewDTO>"-->
<!--@thymesVar id="stars" type="java.util.List<java.lang.Integer>"-->
<head> <head>
<title th:text="|Feedback for ${assistant.displayName}|"></title> <title th:text="|Feedback for ${assistant.displayName}|"></title>
...@@ -34,45 +35,22 @@ ...@@ -34,45 +35,22 @@
<main layout:fragment="content"> <main layout:fragment="content">
<h1 class="font-800 mb-5" th:text="|Feedback for ${assistant.displayName}|"></h1> <h1 class="font-800 mb-5" th:text="|Feedback for ${assistant.displayName}|"></h1>
<div th:if="${feedback.isEmpty()}"> <div class="flex vertical space-between">
<h4 th:if="${#authenticatedP.id == assistant.id}">You have no feedback yet.</h4> <section id="feedback-stat-chart-section">
<h4 th:if="${#authenticatedP.id != assistant.id}">There is no feedback for this assistant yet.</h4> <div class="surface p-0">
</div> <div class="surface__header flex align-center">
<h2 class="font-500">Rating breakdown</h2>
<div class="grid col-2 | md:col-1" th:unless="${feedback.isEmpty()}"> <div class="tooltip">
<div class="flex vertical"> <button class="tooltip__control fa-question"></button>
<div> <p role="tooltip" style="width: 30rem; white-space: initial">
<h2 class="font-500 mb-3">Feedback comments</h2> Ratings range from 1 (needs improvement) to 5 (excellent) for request handling performance.
<ul class="surface divided list pil-5 pbl-3" role="list">
<li class="flex vertical gap-3 pbl-3" th:each="fb : ${feedback}" th:if="${fb.feedback != null && !fb.feedback.isEmpty()}">
<p th:text="${fb.feedback}"></p>
<p th:if="${fb.rating != null}" class="flex gap-1" style="color: goldenrod">
<span class="fa-solid fa-star" th:each="_ : ${#numbers.sequence(1, fb.rating, 1)}"></span>
<span class="fa-regular fa-star" th:each="_ : ${#numbers.sequence(1, 5 - fb.rating, 1)}"></span>
</p>
<p class="font-200" th:if="${@permissionService.canViewDeanonimizedFeedback()}">
<span>Feedback</span>
<span th:if="${fb.rating != null}" th:text="|of ${fb.rating + '/5'} |"></span>
<span th:text="|by ${fb.groupName} on Request|"></span>
<a class="link" th:text="${'#' + fb.request.id}" th:href="@{/request/{id}(id = ${fb.request.id})}"></a>
<br />
<span
th:text="|Last updated on ${#temporals.format(fb.lastUpdatedAt, 'dd-MM-yyyy')}, created ${#temporals.format(fb.createdAt, 'dd-MM-yyyy')}|"></span>
</p> </p>
</li>
</ul>
</div> </div>
<th:block th:replace="pagination :: pagination (page=${feedback}, size=3)"></th:block>
</div> </div>
<div class="surface__content">
<div>
<h2 class="font-500 mb-3">Overview (5 is most positive)</h2>
<canvas id="feedback-histogram"></canvas> <canvas id="feedback-histogram"></canvas>
</div>
<script th:inline="javascript" type="text/javascript"> <script th:inline="javascript" type="text/javascript">
//<![CDATA[ //<![CDATA[
const stars = /*[[${stars}]]*/ [0, 0, 0, 0, 0]; const stars = /*[[${stars}]]*/ [0, 0, 0, 0, 0];
...@@ -111,16 +89,57 @@ ...@@ -111,16 +89,57 @@
y: { y: {
ticks: { ticks: {
beginAtZero: true, beginAtZero: true,
precision: 0 precision: 0,
callback: value => {if (value % 1 === 0) return value;} },
} },
}
}, },
}, },
}); });
//]]> //]]>
</script> </script>
</div> </div>
</section>
<section id="commentary-feedback-section">
<div class="flex vertical">
<div class="surface p-0">
<h2 class="surface__header font-500">Written feedback</h2>
<div class="surface__content">
<div class="banner" data-type="info" role="banner" th:if="${feedback.isEmpty()}">
<span class="banner__icon fa-solid fa-info-circle"></span>
<p th:if="${#authenticatedP.id == assistant.id}">You have no written feedback.</p>
<p th:if="${#authenticatedP.id != assistant.id}">This person has no written feedback.</p>
</div>
<ul class="surface divided list pil-5 pbl-3" role="list" th:unless="${feedback.isEmpty()}">
<li
class="flex vertical gap-3 pbl-3"
th:each="fb : ${feedback}"
th:if="${fb.feedback != null && !fb.feedback.isEmpty()}">
<p th:text="${fb.feedback}"></p>
<p th:if="${fb.rating != null}" class="flex gap-1" style="color: goldenrod">
<span class="fa-solid fa-star" th:each="_ : ${#numbers.sequence(1, fb.rating, 1)}"></span>
<span class="fa-regular fa-star" th:each="_ : ${#numbers.sequence(1, 5 - fb.rating, 1)}"></span>
</p>
<p class="font-200" th:if="${@permissionService.canViewDeanonimizedFeedback()}">
<span>Feedback</span>
<span th:if="${fb.rating != null}" th:text="|of ${fb.rating + '/5'} |"></span>
<span th:text="|by ${fb.groupName} on Request|"></span>
<a class="link" th:text="${'#' + fb.request.id}" th:href="@{/request/{id}(id = ${fb.request.id})}"></a>
<br />
<span
th:text="|Last updated on ${#temporals.format(fb.lastUpdatedAt, 'dd-MM-yyyy')}, created ${#temporals.format(fb.createdAt, 'dd-MM-yyyy')}|"></span>
</p>
</li>
</ul>
</div>
</div>
<th:block th:replace="pagination :: pagination (page=${feedback}, size=3)"></th:block>
</div>
</section>
</div> </div>
</main> </main>
</body> </body>
......
src/test/java/nl/tudelft/queue/PageUtilTest.java 0 → 100644
+ 114
− 0
View file @ 79a6960c
/*
* Queue - A Queueing system that can be used to handle labs in higher education
* Copyright (C) 2016-2021 Delft University of Technology
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
package nl.tudelft.queue;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageImpl;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
class PageUtilTest {
String s1;
String s2;
String s3;
String s4;
String s5;
String s6;
Comparator<String> stringComparator;
@BeforeEach
void setup() {
s1 = "1111";
s2 = "2222";
s3 = "3333";
s4 = "4444";
s5 = "5555";
s6 = "6666";
stringComparator = String::compareToIgnoreCase;
}
@Test
void testNullAndEmpty() {
Pageable pageable = PageRequest.of(0, 1);
Page<String> expectedResult = new PageImpl<>(List.of(), pageable, 0);
assertThat(PageUtil.toPage(pageable, null, stringComparator)).isEqualTo(expectedResult);
assertThat(PageUtil.toPage(pageable, Collections.emptyList(), stringComparator))
.isEqualTo(expectedResult);
}
@Test
void immutableListStillReturnsCorrectPageSorted() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
Pageable pageable = PageRequest.of(0, 3);
assertThat(PageUtil.toPage(pageable, values, stringComparator).getContent()).containsExactly(s1, s2,
s3);
}
@Test
void outOfBoundsJustReturnsAnEmptyPage() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
var sut = PageUtil.toPage(PageRequest.of(3, 2), values, stringComparator);
assertThat(sut.getContent()).isEmpty();
assertThat(sut.getTotalPages()).isEqualTo(3);
assertThat(sut.getTotalElements()).isEqualTo(6L);
}
@Test
void pageSizeBiggerReturnsTheWholeContent() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
var sut = PageUtil.toPage(PageRequest.of(0, 100), values, stringComparator.reversed());
assertThat(sut.getContent()).containsExactlyInAnyOrderElementsOf(values);
assertThat(sut.getContent()).isSortedAccordingTo(stringComparator.reversed());
}
@Test
void edgeCasePage() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
var sut = PageUtil.toPage(PageRequest.of(5, 1), values, stringComparator);
assertThat(sut.getNumberOfElements()).isEqualTo(1);
assertThat(sut.getContent().get(0)).isEqualTo(s6);
}
@Test
void nullComparatorDoesNotSortTheListWhenConvertingToAPage() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
var sut = PageUtil.toPage(PageRequest.of(0, 99), values);
assertThat(sut.getContent()).isEqualTo(values);
}
@Test
void nullComparatorTakesTheCorrectValuesIfPageSizeIsSmallerThanResultSet() {
List<String> values = List.of(s2, s3, s5, s4, s6, s1);
var sut = PageUtil.toPage(PageRequest.of(1, 2), values);
assertThat(sut.getContent()).isEqualTo(List.of(s5, s4));
}
}
src/test/java/nl/tudelft/queue/controller/HomeControllerTest.java
+ 62
− 4
View file @ 79a6960c
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
*/ */
package nl.tudelft.queue.controller; package nl.tudelft.queue.controller;
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.Matchers.*; import static org.hamcrest.Matchers.*;
import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyLong;
...@@ -36,6 +37,8 @@ import nl.tudelft.labracore.api.PersonControllerApi; ...@@ -36,6 +37,8 @@ import nl.tudelft.labracore.api.PersonControllerApi;
import nl.tudelft.labracore.api.dto.*; import nl.tudelft.labracore.api.dto.*;
import nl.tudelft.labracore.api.dto.EditionSummaryDTO; import nl.tudelft.labracore.api.dto.EditionSummaryDTO;
import nl.tudelft.labracore.api.dto.PersonSummaryDTO; import nl.tudelft.labracore.api.dto.PersonSummaryDTO;
import nl.tudelft.queue.model.Feedback;
import nl.tudelft.queue.model.LabRequest;
import nl.tudelft.queue.repository.FeedbackRepository; import nl.tudelft.queue.repository.FeedbackRepository;
import nl.tudelft.queue.service.PermissionService; import nl.tudelft.queue.service.PermissionService;
...@@ -50,7 +53,6 @@ import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMock ...@@ -50,7 +53,6 @@ import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMock
import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.SpyBean; import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.data.domain.Page; import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageRequest;
import org.springframework.security.test.context.support.WithUserDetails; import org.springframework.security.test.context.support.WithUserDetails;
import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MockMvc;
...@@ -88,7 +90,13 @@ class HomeControllerTest { ...@@ -88,7 +90,13 @@ class HomeControllerTest {
private PersonSummaryDTO teacher2; private PersonSummaryDTO teacher2;
private final ModelMapper mapper = new ModelMapper(); private LabRequest[] oopNowRegLab1Requests;
Feedback f1;
Feedback f2;
Feedback f3;
@BeforeEach @BeforeEach
void setUp() { void setUp() {
...@@ -103,8 +111,32 @@ class HomeControllerTest { ...@@ -103,8 +111,32 @@ class HomeControllerTest {
teacher2 = db.getTeachers()[2]; teacher2 = db.getTeachers()[2];
oopNowRegLab1Requests = db.getOopNowRegularLab1Requests();
// fake feedback
f1 = Feedback.builder()
.id(new Feedback.Id(oopNowRegLab1Requests[0].getId(), student100.getId()))
.request(oopNowRegLab1Requests[0])
.rating(5)
.feedback("GREAT TA")
.build();
f2 = Feedback.builder()
.id(new Feedback.Id(oopNowRegLab1Requests[1].getId(), student100.getId()))
.request(oopNowRegLab1Requests[1])
.rating(1)
.build();
f3 = Feedback.builder()
.id(new Feedback.Id(oopNowRegLab1Requests[2].getId(), student100.getId()))
.request(oopNowRegLab1Requests[2])
.feedback("Mediocre")
.build();
// This mock has been untrustworthy in the past, so we should reset it every test. // This mock has been untrustworthy in the past, so we should reset it every test.
reset(feedbackRepository); reset(feedbackRepository);
feedbackRepository.saveAll(List.of(f1, f2, f3));
} }
@Test @Test
...@@ -187,7 +219,7 @@ class HomeControllerTest { ...@@ -187,7 +219,7 @@ class HomeControllerTest {
.andExpect(model().attributeExists("assistant", "feedback")) .andExpect(model().attributeExists("assistant", "feedback"))
.andExpect(view().name("home/feedback")); .andExpect(view().name("home/feedback"));
verify(feedbackRepository).findByAssistantAnonymised(student100.getId(), PageRequest.of(0, 1)); verify(feedbackRepository, times(2)).findByAssistantAnonymised(student100.getId());
verify(permissionService, atLeastOnce()).canViewOwnFeedback(); verify(permissionService, atLeastOnce()).canViewOwnFeedback();
} }
...@@ -214,6 +246,32 @@ class HomeControllerTest { ...@@ -214,6 +246,32 @@ class HomeControllerTest {
verify(permissionService).canViewFeedback(student100.getId()); verify(permissionService).canViewFeedback(student100.getId());
} }
@Test
@WithUserDetails("teacher0")
void teacherCanSeeAssociatedWrittenFeedbackInManagerview() throws Exception {
var queryResult = mvc.perform(get("/feedback/{id}/manager?page=0&size=100", student100.getId()))
.andExpect(status().isOk())
.andExpect(model().attribute("stars", List.of(1, 0, 0, 0, 1)))
.andReturn();
assertThat(queryResult.getResponse().getContentAsString()).contains(f1.getFeedback(),
f3.getFeedback());
}
@Test
@WithUserDetails("teacher1")
void teachersCanSeeStarsAssociatedWithOtherCoursesButNotWrittenFeedback() throws Exception {
var queryResult = mvc.perform(get("/feedback/{id}/manager?page=0&size=100", student100.getId()))
.andExpect(status().isOk())
.andExpect(model().attribute("stars", List.of(1, 0, 0, 0, 1)))
.andReturn();
assertThat(queryResult.getResponse().getContentAsString()).doesNotContain(f1.getFeedback(),
f3.getFeedback());
}
@Test @Test
@WithUserDetails("teacher1") @WithUserDetails("teacher1")
void managerViewWorks() throws Exception { void managerViewWorks() throws Exception {
...@@ -282,7 +340,7 @@ class HomeControllerTest { ...@@ -282,7 +340,7 @@ class HomeControllerTest {
.andExpect(model().attributeExists("finishedRoles")) .andExpect(model().attributeExists("finishedRoles"))
.andExpect(model().attribute("finishedRoles", .andExpect(model().attribute("finishedRoles",
List.of(roles[0], roles[1], roles[2], roles[3]).stream() List.of(roles[0], roles[1], roles[2], roles[3]).stream()
.map(r -> mapper.map(r, RoleEditionDetailsDTO.class)) .map(r -> new ModelMapper().map(r, RoleEditionDetailsDTO.class))
.collect(Collectors.toList()))) .collect(Collectors.toList())))
.andExpect(view().name("home/dashboard")); .andExpect(view().name("home/dashboard"));
} }
......
src/test/java/nl/tudelft/queue/service/FeedbackServiceTest.java
+ 18
− 5
View file @ 79a6960c
...@@ -192,18 +192,31 @@ public class FeedbackServiceTest { ...@@ -192,18 +192,31 @@ public class FeedbackServiceTest {
@Test @Test
void countRatingsWorks() { void countRatingsWorks() {
assertThat(fs.countRatings(people[0].getId())) var p0FeedBacks = fr.findAllByAssistant(people[0].getId());
var p1FeedBacks = fr.findAllByAssistant(people[1].getId());
var p2FeedBacks = fr.findAllByAssistant(people[2].getId());
var p3FeedBacks = fr.findAllByAssistant(people[3].getId());
var p4FeedBacks = fr.findAllByAssistant(people[4].getId());
assertThat(fs.countRatings(p0FeedBacks))
.isEqualTo(List.of(0, 0, 0, 2, 0)); .isEqualTo(List.of(0, 0, 0, 2, 0));
assertThat(fs.countRatings(people[1].getId())) assertThat(fs.countRatings(p1FeedBacks))
.isEqualTo(List.of(0, 1, 0, 0, 0)); .isEqualTo(List.of(0, 1, 0, 0, 0));
assertThat(fs.countRatings(people[2].getId())) assertThat(fs.countRatings(p2FeedBacks))
.isEqualTo(List.of(0, 0, 1, 0, 0)); .isEqualTo(List.of(0, 0, 1, 0, 0));
assertThat(fs.countRatings(people[3].getId())) assertThat(fs.countRatings(p3FeedBacks))
.isEqualTo(List.of(0, 0, 0, 0, 0)); .isEqualTo(List.of(0, 0, 0, 0, 0));
assertThat(fs.countRatings(people[4].getId())) assertThat(fs.countRatings(p4FeedBacks))
.isEqualTo(List.of(0, 0, 0, 0, 0)); .isEqualTo(List.of(0, 0, 0, 0, 0));
} }
@Test
void countRatingsDoesNotConsiderRatingsFeedbacksWithNoRating() {
Feedback f1 = Feedback.builder().feedback("Good TA").build();
Feedback f2 = Feedback.builder().feedback("Bad TA").rating(1).build();
assertThat(fs.countRatings(List.of(f1, f2))).isEqualTo(List.of(1, 0, 0, 0, 0));
}
@Test @Test
void updateFeedbackCreatesNewFeedbackWhenNeeded() { void updateFeedbackCreatesNewFeedbackWhenNeeded() {
assertThat(fr.findAllByAssistant(people[4].getId())).isEmpty(); assertThat(fr.findAllByAssistant(people[4].getId())).isEmpty();
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please to comment