From 4536c6b21e03b1b2fe7c47b888df6142ec7fd338 Mon Sep 17 00:00:00 2001 From: Rameez Parkar Date: Thu, 2 Nov 2023 06:00:19 -0300 Subject: [PATCH 1/2] New Test cases for LocalLoggingService, InstructorSearchDocument, StudentSearchDocument --- build.gradle | 2 + .../external/LocalLoggingServiceTest.java | 68 ++++++++++++++++ .../search/InstructorSearchDocumentTest.java | 78 +++++++++++++++++++ .../search/StudentSearchDocumentTest.java | 74 ++++++++++++++++++ .../BaseTestCaseWithLocalDatabaseAccess.java | 8 +- 5 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 src/test/java/teammates/logic/external/LocalLoggingServiceTest.java create mode 100644 src/test/java/teammates/storage/search/InstructorSearchDocumentTest.java create mode 100644 src/test/java/teammates/storage/search/StudentSearchDocumentTest.java diff --git a/build.gradle b/build.gradle index 1613ad421a6..37d2a586e1e 100644 --- a/build.gradle +++ b/build.gradle @@ -41,6 +41,7 @@ def objectify = "com.googlecode.objectify:objectify:6.0.7" def testng = "org.testng:testng:7.6.1" dependencies { + testImplementation 'org.junit.jupiter:junit-jupiter:5.8.1' staticAnalysis("com.puppycrawl.tools:checkstyle:${checkstyleVersion}") staticAnalysis("net.sourceforge.pmd:pmd-java:${pmdVersion}") staticAnalysis("com.github.spotbugs:spotbugs:${spotbugsVersion}") @@ -79,6 +80,7 @@ dependencies { testImplementation("org.seleniumhq.selenium:selenium-java:4.3.0") testImplementation("com.deque.html.axe-core:selenium:4.6.0") testImplementation(testng) + testImplementation("org.mockito:mockito-core:3.11.2") // For supporting authorization code flow locally testImplementation("com.google.oauth-client:google-oauth-client-jetty:1.34.1") // For using Gmail API diff --git a/src/test/java/teammates/logic/external/LocalLoggingServiceTest.java b/src/test/java/teammates/logic/external/LocalLoggingServiceTest.java new file mode 100644 index 00000000000..06df853d999 --- /dev/null +++ b/src/test/java/teammates/logic/external/LocalLoggingServiceTest.java @@ -0,0 +1,68 @@ +package teammates.logic.external; + +import org.testng.annotations.Test; +import teammates.common.datatransfer.FeedbackSessionLogEntry; +import teammates.common.datatransfer.logs.GeneralLogEntry; +import teammates.common.datatransfer.logs.LogSeverity; +import teammates.common.datatransfer.logs.QueryLogsParams; + +import java.time.Instant; +import java.time.temporal.ChronoField; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +public class LocalLoggingServiceTest { + + private final LogService localLoggingService; + + LocalLoggingServiceTest() { + this.localLoggingService = new LocalLoggingService(); + } + + @Test + public void testQueryLogs() { + QueryLogsParams.Builder builder = QueryLogsParams.builder( + Instant.now().getLong(ChronoField.INSTANT_SECONDS), + Instant.now().plusSeconds(300).getLong(ChronoField.INSTANT_SECONDS) + ); + builder.withOrder("asc"); + builder.withSeverityLevel(LogSeverity.INFO); + builder.withMinSeverity(LogSeverity.INFO); + QueryLogsParams queryLogsParams = builder.build(); + + List result = localLoggingService.queryLogs(queryLogsParams).getLogEntries(); + + // Verify the results as needed + assertEquals(0, result.size()); + } + + @Test + public void testCreateFeedbackSessionLog() { + String courseId = "CS101"; + String email = "student@example.com"; + String fsName = "Feedback Session 1"; + String fslType = "Created"; + + localLoggingService.createFeedbackSessionLog(courseId, email, fsName, fslType); + QueryLogsParams.Builder builder = QueryLogsParams.builder(123456, 456890); + List result = localLoggingService.queryLogs(builder.build()).getLogEntries(); + + // Verify the results as needed + assertEquals(0, result.size()); + } + + @Test + public void testGetFeedbackSessionLogs() { + String courseId = "CS101"; + String email = "student@example.com"; + String fsName = "Feedback Session 1"; + long startTime = 0; + long endTime = System.currentTimeMillis(); + + List feedbackSessionLogs = localLoggingService.getFeedbackSessionLogs(courseId, email, startTime, endTime, fsName); + + // Verify the results as needed + assertEquals(1, feedbackSessionLogs.size()); + } +} diff --git a/src/test/java/teammates/storage/search/InstructorSearchDocumentTest.java b/src/test/java/teammates/storage/search/InstructorSearchDocumentTest.java new file mode 100644 index 00000000000..6502cf0dd2a --- /dev/null +++ b/src/test/java/teammates/storage/search/InstructorSearchDocumentTest.java @@ -0,0 +1,78 @@ +package teammates.storage.search; + +import org.testng.annotations.BeforeTest; +import org.testng.annotations.Test; +import teammates.common.datatransfer.attributes.CourseAttributes; +import teammates.common.datatransfer.attributes.InstructorAttributes; +import teammates.storage.entity.Course; +import teammates.storage.entity.Instructor; + +import java.time.Instant; +import java.util.Map; + +public class InstructorSearchDocumentTest extends BaseSearchTest { + Instructor instructor; + InstructorAttributes instructorAttributes; + Course course; + CourseAttributes courseAttributes; + + @BeforeTest + public void setup() { + // Arrange + instructor = new Instructor( + "jacob.martin@gmail.com", + "CS101", + false, + "Jacob Martin", + "jacob.martin@example.com", + "Teaching", + true, + "Professor Martin", + null + ); + instructorAttributes = InstructorAttributes.valueOf(instructor); + } + + @Test + public void testGetSearchableFields() { + // Arrange + course = new Course( + "CS101", + "Advanced Software Development Concepts", + "UTC", + "Dalhousie University", + Instant.now(), + null + ); + courseAttributes = CourseAttributes.valueOf(course); + + InstructorSearchDocument instructorSearchDocument = new InstructorSearchDocument(instructorAttributes, courseAttributes); + + // Act + Map searchableFields = instructorSearchDocument.getSearchableFields(); + + // Assert + assertEquals("jacob.martin@example.com%CS101", searchableFields.get("id")); + assertEquals("Jacob Martin jacob.martin@example.com CS101 Advanced Software Development Concepts jacob.martin@gmail.com Teaching Professor Martin", searchableFields.get("_text_")); + assertEquals("CS101", searchableFields.get("courseId")); + assertEquals("jacob.martin@example.com", searchableFields.get("email")); + } + + @Test + public void testGetSearchableFieldsWithoutCourse() { + // Arrange + course = null; + courseAttributes = null; + + InstructorSearchDocument instructorSearchDocument = new InstructorSearchDocument(instructorAttributes, courseAttributes); + + // Act + Map searchableFields = instructorSearchDocument.getSearchableFields(); + + // Assert + assertEquals("jacob.martin@example.com%CS101", searchableFields.get("id")); + assertEquals("Jacob Martin jacob.martin@example.com CS101 jacob.martin@gmail.com Teaching Professor Martin", searchableFields.get("_text_")); + assertEquals("CS101", searchableFields.get("courseId")); + assertEquals("jacob.martin@example.com", searchableFields.get("email")); + } +} diff --git a/src/test/java/teammates/storage/search/StudentSearchDocumentTest.java b/src/test/java/teammates/storage/search/StudentSearchDocumentTest.java new file mode 100644 index 00000000000..1d604872bd7 --- /dev/null +++ b/src/test/java/teammates/storage/search/StudentSearchDocumentTest.java @@ -0,0 +1,74 @@ +package teammates.storage.search; + +import org.testng.annotations.BeforeTest; +import org.testng.annotations.Test; +import teammates.common.datatransfer.attributes.CourseAttributes; +import teammates.common.datatransfer.attributes.StudentAttributes; +import teammates.storage.entity.Course; +import teammates.storage.entity.CourseStudent; + +import java.time.Instant; +import java.util.Map; + +public class StudentSearchDocumentTest extends BaseSearchTest { + CourseStudent courseStudent; + StudentAttributes studentAttributes; + Course course; + CourseAttributes courseAttributes; + + @BeforeTest + public void setup() { + courseStudent = new CourseStudent( + "john.doe@example.com", + "John Doe", + "john.doe@gmail.com", + "Introduction to Computer Science", + "CS101", + "Engineering", + "ASDC" + ); + studentAttributes = StudentAttributes.valueOf(courseStudent); + } + + @Test + public void testGetSearchableFields() { + // Arrange + course = new Course( + "CS101", + "Advanced Software Development Concepts", + "UTC", + "Dalhousie University", + Instant.now(), + null + ); + courseAttributes = CourseAttributes.valueOf(course); + + StudentSearchDocument studentSearchDocument = new StudentSearchDocument(studentAttributes, courseAttributes); + + // Act + Map searchableFields = studentSearchDocument.getSearchableFields(); + + // Assert + assertEquals("john.doe@example.com%CS101", searchableFields.get("id")); + assertEquals("John Doe john.doe@example.com CS101 Advanced Software Development Concepts Engineering ASDC", searchableFields.get("_text_")); + assertEquals("CS101", searchableFields.get("courseId")); + assertEquals("john.doe@example.com", searchableFields.get("email")); + } + + @Test + public void testGetSearchableFieldsWithoutCourse() { + // Arrange + CourseAttributes courseAttributes = null; + + StudentSearchDocument studentSearchDocument = new StudentSearchDocument(studentAttributes, courseAttributes); + + // Act + Map searchableFields = studentSearchDocument.getSearchableFields(); + + // Assert + assertEquals("john.doe@example.com%CS101", searchableFields.get("id")); + assertEquals("John Doe john.doe@example.com CS101 Engineering ASDC", searchableFields.get("_text_")); + assertEquals("CS101", searchableFields.get("courseId")); + assertEquals("john.doe@example.com", searchableFields.get("email")); + } +} diff --git a/src/test/java/teammates/test/BaseTestCaseWithLocalDatabaseAccess.java b/src/test/java/teammates/test/BaseTestCaseWithLocalDatabaseAccess.java index 03ba65eefe5..08ed7202ca0 100644 --- a/src/test/java/teammates/test/BaseTestCaseWithLocalDatabaseAccess.java +++ b/src/test/java/teammates/test/BaseTestCaseWithLocalDatabaseAccess.java @@ -86,10 +86,10 @@ public void resetDbLayer() throws Exception { LOCAL_DATASTORE_HELPER.reset(); } - @AfterSuite - public void tearDownLocalDatastoreHelper() throws Exception { - LOCAL_DATASTORE_HELPER.stop(); - } + // @AfterSuite + // public void tearDownLocalDatastoreHelper() throws Exception { + // LOCAL_DATASTORE_HELPER.stop(); + // } @Override protected AccountAttributes getAccount(AccountAttributes account) { From 0b96c4df32831fe3ec2f4f6412461278dcabe744 Mon Sep 17 00:00:00 2001 From: Rameez Parkar Date: Mon, 27 Nov 2023 23:23:31 -0400 Subject: [PATCH 2/2] refactored and eliminated implementation and design smells --- .../teammates/logic/api/EmailGenerator.java | 24 +- .../logic/core/FeedbackQuestionsLogic.java | 583 +++++++++--------- .../logic/core/FeedbackResponsesLogic.java | 34 +- .../logic/core/FeedbackSessionsLogic.java | 31 +- .../logic/core/FeedbackVariables.java | 17 + .../logic/core/GiverTypeHandler.java | 9 + .../teammates/logic/core/GiversLogic.java | 35 ++ .../core/InstructorsGiverTypeHandler.java | 18 + .../logic/core/InstructorsLogic.java | 16 +- .../logic/core/SelfGiverTypeHandler.java | 17 + .../logic/core/StudentsGiverTypeHandler.java | 18 + .../teammates/logic/core/StudentsLogic.java | 11 +- .../logic/core/TeamsGiverTypeHandler.java | 14 + .../logic/api/EmailGeneratorTest.java | 23 + .../logic/core/FeedbackSessionsLogicTest.java | 24 - 15 files changed, 497 insertions(+), 377 deletions(-) create mode 100644 src/main/java/teammates/logic/core/FeedbackVariables.java create mode 100644 src/main/java/teammates/logic/core/GiverTypeHandler.java create mode 100644 src/main/java/teammates/logic/core/GiversLogic.java create mode 100644 src/main/java/teammates/logic/core/InstructorsGiverTypeHandler.java create mode 100644 src/main/java/teammates/logic/core/SelfGiverTypeHandler.java create mode 100644 src/main/java/teammates/logic/core/StudentsGiverTypeHandler.java create mode 100644 src/main/java/teammates/logic/core/TeamsGiverTypeHandler.java diff --git a/src/main/java/teammates/logic/api/EmailGenerator.java b/src/main/java/teammates/logic/api/EmailGenerator.java index 965165f1364..e2c266ea8c9 100644 --- a/src/main/java/teammates/logic/api/EmailGenerator.java +++ b/src/main/java/teammates/logic/api/EmailGenerator.java @@ -25,6 +25,7 @@ import teammates.common.util.Templates.EmailTemplates; import teammates.common.util.TimeHelper; import teammates.logic.core.CoursesLogic; +import teammates.logic.core.FeedbackQuestionsLogic; import teammates.logic.core.FeedbackSessionsLogic; import teammates.logic.core.InstructorsLogic; import teammates.logic.core.StudentsLogic; @@ -61,6 +62,8 @@ public final class EmailGenerator { private final CoursesLogic coursesLogic = CoursesLogic.inst(); private final FeedbackSessionsLogic fsLogic = FeedbackSessionsLogic.inst(); + private final FeedbackQuestionsLogic fqLogic = FeedbackQuestionsLogic.inst(); + private final InstructorsLogic instructorsLogic = InstructorsLogic.inst(); private final StudentsLogic studentsLogic = StudentsLogic.inst(); @@ -82,8 +85,8 @@ public List generateFeedbackSessionOpeningEmails(FeedbackSessionAt private List generateFeedbackSessionOpeningOrClosingEmails( FeedbackSessionAttributes session, EmailType emailType) { CourseAttributes course = coursesLogic.getCourse(session.getCourseId()); - boolean isEmailNeededForStudents = fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false); - boolean isEmailNeededForInstructors = fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true); + boolean isEmailNeededForStudents = isFeedbackSessionForUserTypeToAnswer(session, false); + boolean isEmailNeededForInstructors = isFeedbackSessionForUserTypeToAnswer(session, true); List instructorsToNotify = isEmailNeededForStudents ? instructorsLogic.getCoOwnersForCourse(session.getCourseId()) : new ArrayList<>(); @@ -501,9 +504,9 @@ public List generateFeedbackSessionClosingWithExtensionEmails( deadlineExtensions.stream().filter(x -> x.getIsInstructor()).collect(Collectors.toList()); boolean isEmailNeededForStudents = - !studentDeadlines.isEmpty() && fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false); + !studentDeadlines.isEmpty() && isFeedbackSessionForUserTypeToAnswer(session, false); boolean isEmailNeededForInstructors = - !instructorDeadlines.isEmpty() && fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true); + !instructorDeadlines.isEmpty() && isFeedbackSessionForUserTypeToAnswer(session, true); List students = new ArrayList<>(); if (isEmailNeededForStudents) { @@ -1078,4 +1081,17 @@ private String getAdditionalContactInformationFragment(CourseAttributes course, "${coOwnersEmails}", generateCoOwnersEmailsLine(course.getId()), "${supportEmail}", Config.SUPPORT_EMAIL); } + + /** + * Returns true if there are any questions for the specified user type (students/instructors) to answer. + */ + public boolean isFeedbackSessionForUserTypeToAnswer(FeedbackSessionAttributes session, boolean isInstructor) { + if (!session.isVisible()) { + return false; + } + + return isInstructor + ? fqLogic.hasFeedbackQuestionsForInstructors(session, false) + : fqLogic.hasFeedbackQuestionsForStudents(session); + } } diff --git a/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java b/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java index 59633bbfff3..7eeb1c1728f 100644 --- a/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java +++ b/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java @@ -1,16 +1,5 @@ package teammates.logic.core; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; - -import javax.annotation.Nullable; - import teammates.common.datatransfer.AttributesDeletionQuery; import teammates.common.datatransfer.CourseRoster; import teammates.common.datatransfer.FeedbackParticipantType; @@ -28,6 +17,10 @@ import teammates.common.util.Logger; import teammates.storage.api.FeedbackQuestionsDb; +import javax.annotation.Nullable; +import java.util.*; +import java.util.stream.Collectors; + /** * Handles operations related to feedback questions. * @@ -207,8 +200,8 @@ public List getFeedbackQuestionsForInstructors( List questions = new ArrayList<>(); for (FeedbackQuestionAttributes question : allQuestions) { - if (question.getGiverType() == FeedbackParticipantType.INSTRUCTORS - || question.getGiverType() == FeedbackParticipantType.SELF && isCreator) { + if (isInstructorsType(question) || + isSelfTypeAndCreator(question, isCreator)) { questions.add(question); } } @@ -216,6 +209,15 @@ public List getFeedbackQuestionsForInstructors( return questions; } + private boolean isInstructorsType(FeedbackQuestionAttributes question) { + return question.getGiverType() == FeedbackParticipantType.INSTRUCTORS; + } + + private boolean isSelfTypeAndCreator(FeedbackQuestionAttributes question, boolean isCreator) { + return question.getGiverType() == FeedbackParticipantType.SELF && isCreator; + } + + /** * Checks if there are any questions for the given session that students can view/submit. */ @@ -223,7 +225,7 @@ public boolean hasFeedbackQuestionsForStudents(FeedbackSessionAttributes fsa) { return fqDb.hasFeedbackQuestionsForGiverType( fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.STUDENTS) || fqDb.hasFeedbackQuestionsForGiverType( - fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.TEAMS); + fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.TEAMS); } /** @@ -316,146 +318,146 @@ public Map getRecipientsOfQuestion( FeedbackParticipantType generateOptionsFor = recipientType; switch (recipientType) { - case SELF: - if (question.getGiverType() == FeedbackParticipantType.TEAMS) { - recipients.put(giverTeam, - new FeedbackQuestionRecipient(giverTeam, giverTeam)); - } else { - recipients.put(giverEmail, - new FeedbackQuestionRecipient(USER_NAME_FOR_SELF, giverEmail)); - } - break; - case STUDENTS: - case STUDENTS_EXCLUDING_SELF: - case STUDENTS_IN_SAME_SECTION: - List studentList; - if (courseRoster == null) { - if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { - studentList = studentsLogic.getStudentsForSection(giverSection, question.getCourseId()); + case SELF: + if (question.getGiverType() == FeedbackParticipantType.TEAMS) { + recipients.put(giverTeam, + new FeedbackQuestionRecipient(giverTeam, giverTeam)); } else { - studentList = studentsLogic.getStudentsForCourse(question.getCourseId()); + recipients.put(giverEmail, + new FeedbackQuestionRecipient(USER_NAME_FOR_SELF, giverEmail)); } - } else { - if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { - final String finalGiverSection = giverSection; - studentList = courseRoster.getStudents().stream() - .filter(studentAttributes -> studentAttributes.getSection() - .equals(finalGiverSection)).collect(Collectors.toList()); + break; + case STUDENTS: + case STUDENTS_EXCLUDING_SELF: + case STUDENTS_IN_SAME_SECTION: + List studentList; + if (courseRoster == null) { + if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { + studentList = studentsLogic.getStudentsForSection(giverSection, question.getCourseId()); + } else { + studentList = studentsLogic.getStudentsForCourse(question.getCourseId()); + } } else { - studentList = courseRoster.getStudents(); - } - } - for (StudentAttributes student : studentList) { - if (isInstructorGiver && !instructorGiver.isAllowedForPrivilege( - student.getSection(), question.getFeedbackSessionName(), - Const.InstructorPermissions.CAN_SUBMIT_SESSION_IN_SECTIONS)) { - // instructor can only see students in allowed sections for him/her - continue; + if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { + final String finalGiverSection = giverSection; + studentList = courseRoster.getStudents().stream() + .filter(studentAttributes -> studentAttributes.getSection() + .equals(finalGiverSection)).collect(Collectors.toList()); + } else { + studentList = courseRoster.getStudents(); + } } - // Ensure student does not evaluate him/herself if it's STUDENTS_EXCLUDING_SELF or - // STUDENTS_IN_SAME_SECTION - if (giverEmail.equals(student.getEmail()) && generateOptionsFor != FeedbackParticipantType.STUDENTS) { - continue; + for (StudentAttributes student : studentList) { + if (isInstructorGiver && !instructorGiver.isAllowedForPrivilege( + student.getSection(), question.getFeedbackSessionName(), + Const.InstructorPermissions.CAN_SUBMIT_SESSION_IN_SECTIONS)) { + // instructor can only see students in allowed sections for him/her + continue; + } + // Ensure student does not evaluate him/herself if it's STUDENTS_EXCLUDING_SELF or + // STUDENTS_IN_SAME_SECTION + if (giverEmail.equals(student.getEmail()) && generateOptionsFor != FeedbackParticipantType.STUDENTS) { + continue; + } + recipients.put(student.getEmail(), new FeedbackQuestionRecipient(student.getName(), student.getEmail(), + student.getSection(), student.getTeam())); } - recipients.put(student.getEmail(), new FeedbackQuestionRecipient(student.getName(), student.getEmail(), - student.getSection(), student.getTeam())); - } - break; - case INSTRUCTORS: - List instructorsInCourse; - if (courseRoster == null) { - instructorsInCourse = instructorsLogic.getInstructorsForCourse(question.getCourseId()); - } else { - instructorsInCourse = courseRoster.getInstructors(); - } - for (InstructorAttributes instr : instructorsInCourse) { - // remove hidden instructors for students - if (isStudentGiver && !instr.isDisplayedToStudents()) { - continue; + break; + case INSTRUCTORS: + List instructorsInCourse; + if (courseRoster == null) { + instructorsInCourse = instructorsLogic.getInstructorsForCourse(question.getCourseId()); + } else { + instructorsInCourse = courseRoster.getInstructors(); } - // Ensure instructor does not evaluate himself - if (!giverEmail.equals(instr.getEmail())) { - recipients.put(instr.getEmail(), - new FeedbackQuestionRecipient(instr.getName(), instr.getEmail())); + for (InstructorAttributes instr : instructorsInCourse) { + // remove hidden instructors for students + if (isStudentGiver && !instr.isDisplayedToStudents()) { + continue; + } + // Ensure instructor does not evaluate himself + if (!giverEmail.equals(instr.getEmail())) { + recipients.put(instr.getEmail(), + new FeedbackQuestionRecipient(instr.getName(), instr.getEmail())); + } } - } - break; - case TEAMS: - case TEAMS_EXCLUDING_SELF: - case TEAMS_IN_SAME_SECTION: - Map> teamToTeamMembersTable; - List teamStudents; - if (courseRoster == null) { - if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { - teamStudents = studentsLogic.getStudentsForSection(giverSection, question.getCourseId()); + break; + case TEAMS: + case TEAMS_EXCLUDING_SELF: + case TEAMS_IN_SAME_SECTION: + Map> teamToTeamMembersTable; + List teamStudents; + if (courseRoster == null) { + if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { + teamStudents = studentsLogic.getStudentsForSection(giverSection, question.getCourseId()); + } else { + teamStudents = studentsLogic.getStudentsForCourse(question.getCourseId()); + } + teamToTeamMembersTable = CourseRoster.buildTeamToMembersTable(teamStudents); } else { - teamStudents = studentsLogic.getStudentsForCourse(question.getCourseId()); + if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { + final String finalGiverSection = giverSection; + teamStudents = courseRoster.getStudents().stream() + .filter(student -> student.getSection().equals(finalGiverSection)) + .collect(Collectors.toList()); + teamToTeamMembersTable = CourseRoster.buildTeamToMembersTable(teamStudents); + } else { + teamToTeamMembersTable = courseRoster.getTeamToMembersTable(); + } } - teamToTeamMembersTable = CourseRoster.buildTeamToMembersTable(teamStudents); - } else { - if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { - final String finalGiverSection = giverSection; - teamStudents = courseRoster.getStudents().stream() - .filter(student -> student.getSection().equals(finalGiverSection)) - .collect(Collectors.toList()); - teamToTeamMembersTable = CourseRoster.buildTeamToMembersTable(teamStudents); + for (Map.Entry> team : teamToTeamMembersTable.entrySet()) { + if (isInstructorGiver && !instructorGiver.isAllowedForPrivilege( + team.getValue().iterator().next().getSection(), + question.getFeedbackSessionName(), + Const.InstructorPermissions.CAN_SUBMIT_SESSION_IN_SECTIONS)) { + // instructor can only see teams in allowed sections for him/her + continue; + } + // Ensure student('s team) does not evaluate own team if it's TEAMS_EXCLUDING_SELF or + // TEAMS_IN_SAME_SECTION + if (giverTeam.equals(team.getKey()) && generateOptionsFor != FeedbackParticipantType.TEAMS) { + continue; + } + // recipientEmail doubles as team name in this case. + recipients.put(team.getKey(), new FeedbackQuestionRecipient(team.getKey(), team.getKey())); + } + break; + case OWN_TEAM: + recipients.put(giverTeam, new FeedbackQuestionRecipient(giverTeam, giverTeam)); + break; + case OWN_TEAM_MEMBERS: + List students; + if (courseRoster == null) { + students = studentsLogic.getStudentsForTeam(giverTeam, question.getCourseId()); } else { - teamToTeamMembersTable = courseRoster.getTeamToMembersTable(); + students = courseRoster.getTeamToMembersTable().getOrDefault(giverTeam, Collections.emptyList()); } - } - for (Map.Entry> team : teamToTeamMembersTable.entrySet()) { - if (isInstructorGiver && !instructorGiver.isAllowedForPrivilege( - team.getValue().iterator().next().getSection(), - question.getFeedbackSessionName(), - Const.InstructorPermissions.CAN_SUBMIT_SESSION_IN_SECTIONS)) { - // instructor can only see teams in allowed sections for him/her - continue; + for (StudentAttributes student : students) { + if (!student.getEmail().equals(giverEmail)) { + recipients.put(student.getEmail(), new FeedbackQuestionRecipient(student.getName(), student.getEmail(), + student.getSection(), student.getTeam())); + } } - // Ensure student('s team) does not evaluate own team if it's TEAMS_EXCLUDING_SELF or - // TEAMS_IN_SAME_SECTION - if (giverTeam.equals(team.getKey()) && generateOptionsFor != FeedbackParticipantType.TEAMS) { - continue; + break; + case OWN_TEAM_MEMBERS_INCLUDING_SELF: + List teamMembers; + if (courseRoster == null) { + teamMembers = studentsLogic.getStudentsForTeam(giverTeam, question.getCourseId()); + } else { + teamMembers = courseRoster.getTeamToMembersTable().getOrDefault(giverTeam, Collections.emptyList()); } - // recipientEmail doubles as team name in this case. - recipients.put(team.getKey(), new FeedbackQuestionRecipient(team.getKey(), team.getKey())); - } - break; - case OWN_TEAM: - recipients.put(giverTeam, new FeedbackQuestionRecipient(giverTeam, giverTeam)); - break; - case OWN_TEAM_MEMBERS: - List students; - if (courseRoster == null) { - students = studentsLogic.getStudentsForTeam(giverTeam, question.getCourseId()); - } else { - students = courseRoster.getTeamToMembersTable().getOrDefault(giverTeam, Collections.emptyList()); - } - for (StudentAttributes student : students) { - if (!student.getEmail().equals(giverEmail)) { + for (StudentAttributes student : teamMembers) { + // accepts self feedback too recipients.put(student.getEmail(), new FeedbackQuestionRecipient(student.getName(), student.getEmail(), student.getSection(), student.getTeam())); } - } - break; - case OWN_TEAM_MEMBERS_INCLUDING_SELF: - List teamMembers; - if (courseRoster == null) { - teamMembers = studentsLogic.getStudentsForTeam(giverTeam, question.getCourseId()); - } else { - teamMembers = courseRoster.getTeamToMembersTable().getOrDefault(giverTeam, Collections.emptyList()); - } - for (StudentAttributes student : teamMembers) { - // accepts self feedback too - recipients.put(student.getEmail(), new FeedbackQuestionRecipient(student.getName(), student.getEmail(), - student.getSection(), student.getTeam())); - } - break; - case NONE: - recipients.put(Const.GENERAL_QUESTION, - new FeedbackQuestionRecipient(Const.GENERAL_QUESTION, Const.GENERAL_QUESTION)); - break; - default: - break; + break; + case NONE: + recipients.put(Const.GENERAL_QUESTION, + new FeedbackQuestionRecipient(Const.GENERAL_QUESTION, Const.GENERAL_QUESTION)); + break; + default: + break; } return recipients; } @@ -474,41 +476,41 @@ public Map> buildCompleteGiverRecipientMap( List possibleGivers = getPossibleGivers(relatedQuestion, courseRoster); for (String possibleGiver : possibleGivers) { switch (relatedQuestion.getGiverType()) { - case STUDENTS: - StudentAttributes studentGiver = courseRoster.getStudentForEmail(possibleGiver); - completeGiverRecipientMap - .computeIfAbsent(possibleGiver, key -> new HashSet<>()) - .addAll(getRecipientsOfQuestion( - relatedQuestion, null, studentGiver, courseRoster).keySet()); - break; - case TEAMS: - StudentAttributes oneTeamMember = - courseRoster.getTeamToMembersTable().get(possibleGiver).iterator().next(); - completeGiverRecipientMap - .computeIfAbsent(possibleGiver, key -> new HashSet<>()) - .addAll(getRecipientsOfQuestion( - relatedQuestion, null, oneTeamMember, courseRoster).keySet()); - break; - case INSTRUCTORS: - case SELF: - InstructorAttributes instructorGiver = courseRoster.getInstructorForEmail(possibleGiver); - - // only happens when a session creator quits their course - if (instructorGiver == null) { - instructorGiver = - InstructorAttributes - .builder(relatedQuestion.getCourseId(), possibleGiver) - .build(); - } - - completeGiverRecipientMap - .computeIfAbsent(possibleGiver, key -> new HashSet<>()) - .addAll(getRecipientsOfQuestion( - relatedQuestion, instructorGiver, null, courseRoster).keySet()); - break; - default: - log.severe("Invalid giver type specified"); - break; + case STUDENTS: + StudentAttributes studentGiver = courseRoster.getStudentForEmail(possibleGiver); + completeGiverRecipientMap + .computeIfAbsent(possibleGiver, key -> new HashSet<>()) + .addAll(getRecipientsOfQuestion( + relatedQuestion, null, studentGiver, courseRoster).keySet()); + break; + case TEAMS: + StudentAttributes oneTeamMember = + courseRoster.getTeamToMembersTable().get(possibleGiver).iterator().next(); + completeGiverRecipientMap + .computeIfAbsent(possibleGiver, key -> new HashSet<>()) + .addAll(getRecipientsOfQuestion( + relatedQuestion, null, oneTeamMember, courseRoster).keySet()); + break; + case INSTRUCTORS: + case SELF: + InstructorAttributes instructorGiver = courseRoster.getInstructorForEmail(possibleGiver); + + // only happens when a session creator quits their course + if (instructorGiver == null) { + instructorGiver = + InstructorAttributes + .builder(relatedQuestion.getCourseId(), possibleGiver) + .build(); + } + + completeGiverRecipientMap + .computeIfAbsent(possibleGiver, key -> new HashSet<>()) + .addAll(getRecipientsOfQuestion( + relatedQuestion, instructorGiver, null, courseRoster).keySet()); + break; + default: + log.severe("Invalid giver type specified"); + break; } } @@ -526,32 +528,9 @@ private List getPossibleGivers( FeedbackQuestionAttributes fqa, CourseRoster courseRoster) { FeedbackParticipantType giverType = fqa.getGiverType(); List possibleGivers = new ArrayList<>(); - - switch (giverType) { - case STUDENTS: - possibleGivers = courseRoster.getStudents() - .stream() - .map(StudentAttributes::getEmail) - .collect(Collectors.toList()); - break; - case INSTRUCTORS: - possibleGivers = courseRoster.getInstructors() - .stream() - .map(InstructorAttributes::getEmail) - .collect(Collectors.toList()); - break; - case TEAMS: - possibleGivers = new ArrayList<>(courseRoster.getTeamToMembersTable().keySet()); - break; - case SELF: - FeedbackSessionAttributes feedbackSession = - fsLogic.getFeedbackSession(fqa.getFeedbackSessionName(), fqa.getCourseId()); - possibleGivers = Collections.singletonList(feedbackSession.getCreatorEmail()); - break; - default: - log.severe("Invalid giver type specified"); - break; - } + GiversLogic givers = new GiversLogic(); + + possibleGivers = givers.getPossibleGivers(fqa, courseRoster, fsLogic); return possibleGivers; } @@ -567,109 +546,156 @@ private List getPossibleGivers( * it can be {@code null}. */ public void populateFieldsToGenerateInQuestion(FeedbackQuestionAttributes feedbackQuestionAttributes, - String emailOfEntityDoingQuestion, String teamOfEntityDoingQuestion) { + String emailOfEntityDoingQuestion, String teamOfEntityDoingQuestion) { List optionList; - FeedbackParticipantType generateOptionsFor; if (feedbackQuestionAttributes.getQuestionType() == FeedbackQuestionType.MCQ) { - FeedbackMcqQuestionDetails feedbackMcqQuestionDetails = - (FeedbackMcqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy(); - optionList = feedbackMcqQuestionDetails.getMcqChoices(); - generateOptionsFor = feedbackMcqQuestionDetails.getGenerateOptionsFor(); + optionList = handleMcqQuestion(feedbackQuestionAttributes); + generateOptionsFor = getGenerateOptionsFor(feedbackQuestionAttributes); } else if (feedbackQuestionAttributes.getQuestionType() == FeedbackQuestionType.MSQ) { - FeedbackMsqQuestionDetails feedbackMsqQuestionDetails = - (FeedbackMsqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy(); - optionList = feedbackMsqQuestionDetails.getMsqChoices(); - generateOptionsFor = feedbackMsqQuestionDetails.getGenerateOptionsFor(); + optionList = handleMsqQuestion(feedbackQuestionAttributes); + generateOptionsFor = getGenerateOptionsFor(feedbackQuestionAttributes); } else { // other question types return; } + handleGenerateOptionsFor(generateOptionsFor, feedbackQuestionAttributes, emailOfEntityDoingQuestion, teamOfEntityDoingQuestion, optionList); + } + + private List handleMcqQuestion(FeedbackQuestionAttributes feedbackQuestionAttributes) { + FeedbackMcqQuestionDetails feedbackMcqQuestionDetails = + (FeedbackMcqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy(); + return feedbackMcqQuestionDetails.getMcqChoices(); + } + + private List handleMsqQuestion(FeedbackQuestionAttributes feedbackQuestionAttributes) { + FeedbackMsqQuestionDetails feedbackMsqQuestionDetails = + (FeedbackMsqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy(); + return feedbackMsqQuestionDetails.getMsqChoices(); + } + + private FeedbackParticipantType getGenerateOptionsFor(FeedbackQuestionAttributes feedbackQuestionAttributes) { + return feedbackQuestionAttributes.getQuestionType() == FeedbackQuestionType.MCQ ? + ((FeedbackMcqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy()).getGenerateOptionsFor() : + ((FeedbackMsqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy()).getGenerateOptionsFor(); + } + + private void handleGenerateOptionsFor(FeedbackParticipantType generateOptionsFor, + FeedbackQuestionAttributes feedbackQuestionAttributes, + String emailOfEntityDoingQuestion, String teamOfEntityDoingQuestion, + List optionList) { switch (generateOptionsFor) { - case NONE: - break; - case STUDENTS: - case STUDENTS_IN_SAME_SECTION: - case STUDENTS_EXCLUDING_SELF: - List studentList; - if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { + case NONE: + break; + case STUDENTS: + case STUDENTS_IN_SAME_SECTION: + case STUDENTS_EXCLUDING_SELF: + handleStudentOptions(generateOptionsFor, feedbackQuestionAttributes, emailOfEntityDoingQuestion, optionList); + break; + case TEAMS: + case TEAMS_IN_SAME_SECTION: + case TEAMS_EXCLUDING_SELF: + handleTeamOptions(generateOptionsFor, feedbackQuestionAttributes, emailOfEntityDoingQuestion, teamOfEntityDoingQuestion, optionList); + break; + case OWN_TEAM_MEMBERS_INCLUDING_SELF: + case OWN_TEAM_MEMBERS: + handleOwnTeamMembersOptions(generateOptionsFor, feedbackQuestionAttributes, teamOfEntityDoingQuestion, emailOfEntityDoingQuestion, optionList); + break; + case INSTRUCTORS: + handleInstructorsOptions(feedbackQuestionAttributes, optionList); + break; + default: + assert false : "Trying to generate options for neither students, teams nor instructors"; + break; + } + + updateQuestionDetails(feedbackQuestionAttributes, optionList); + } + + private void handleStudentOptions(FeedbackParticipantType generateOptionsFor, + FeedbackQuestionAttributes feedbackQuestionAttributes, + String emailOfEntityDoingQuestion, List optionList) { + List studentList; + + if (generateOptionsFor == FeedbackParticipantType.STUDENTS_IN_SAME_SECTION) { + String courseId = feedbackQuestionAttributes.getCourseId(); + StudentAttributes studentAttributes = + studentsLogic.getStudentForEmail(courseId, emailOfEntityDoingQuestion); + studentList = studentsLogic.getStudentsForSection(studentAttributes.getSection(), courseId); + } else { + studentList = studentsLogic.getStudentsForCourse(feedbackQuestionAttributes.getCourseId()); + } + + if (generateOptionsFor == FeedbackParticipantType.STUDENTS_EXCLUDING_SELF) { + studentList.removeIf(studentInList -> studentInList.getEmail().equals(emailOfEntityDoingQuestion)); + } + + for (StudentAttributes student : studentList) { + optionList.add(student.getName() + " (" + student.getTeam() + ")"); + } + + optionList.sort(null); + } + + private void handleTeamOptions(FeedbackParticipantType generateOptionsFor, + FeedbackQuestionAttributes feedbackQuestionAttributes, + String emailOfEntityDoingQuestion, String teamOfEntityDoingQuestion, + List optionList) { + try { + List teams; + if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { String courseId = feedbackQuestionAttributes.getCourseId(); StudentAttributes studentAttributes = studentsLogic.getStudentForEmail(courseId, emailOfEntityDoingQuestion); - studentList = studentsLogic.getStudentsForSection(studentAttributes.getSection(), courseId); + teams = coursesLogic.getTeamsForSection(studentAttributes.getSection(), courseId); } else { - studentList = studentsLogic.getStudentsForCourse(feedbackQuestionAttributes.getCourseId()); - } - - if (generateOptionsFor == FeedbackParticipantType.STUDENTS_EXCLUDING_SELF) { - studentList.removeIf(studentInList -> studentInList.getEmail().equals(emailOfEntityDoingQuestion)); + teams = coursesLogic.getTeamsForCourse(feedbackQuestionAttributes.getCourseId()); } - for (StudentAttributes student : studentList) { - optionList.add(student.getName() + " (" + student.getTeam() + ")"); + if (generateOptionsFor == FeedbackParticipantType.TEAMS_EXCLUDING_SELF) { + teams.removeIf(team -> team.equals(teamOfEntityDoingQuestion)); } + optionList.addAll(teams); optionList.sort(null); - break; - case TEAMS: - case TEAMS_IN_SAME_SECTION: - case TEAMS_EXCLUDING_SELF: - try { - List teams; - if (generateOptionsFor == FeedbackParticipantType.TEAMS_IN_SAME_SECTION) { - String courseId = feedbackQuestionAttributes.getCourseId(); - StudentAttributes studentAttributes = - studentsLogic.getStudentForEmail(courseId, emailOfEntityDoingQuestion); - teams = coursesLogic.getTeamsForSection(studentAttributes.getSection(), courseId); - } else { - teams = coursesLogic.getTeamsForCourse(feedbackQuestionAttributes.getCourseId()); - } - - if (generateOptionsFor == FeedbackParticipantType.TEAMS_EXCLUDING_SELF) { - teams.removeIf(team -> team.equals(teamOfEntityDoingQuestion)); - } + } catch (EntityDoesNotExistException e) { + assert false : "Course disappeared"; + } + } - for (String team : teams) { - optionList.add(team); - } + private void handleOwnTeamMembersOptions(FeedbackParticipantType generateOptionsFor, + FeedbackQuestionAttributes feedbackQuestionAttributes, + String teamOfEntityDoingQuestion, String emailOfEntityDoingQuestion, + List optionList) { + if (teamOfEntityDoingQuestion != null) { + String courseId = feedbackQuestionAttributes.getCourseId(); + List teamMembers = studentsLogic.getStudentsForTeam(teamOfEntityDoingQuestion, courseId); - optionList.sort(null); - } catch (EntityDoesNotExistException e) { - assert false : "Course disappeared"; + if (generateOptionsFor == FeedbackParticipantType.OWN_TEAM_MEMBERS) { + teamMembers.removeIf(teamMember -> teamMember.getEmail().equals(emailOfEntityDoingQuestion)); } - break; - case OWN_TEAM_MEMBERS_INCLUDING_SELF: - case OWN_TEAM_MEMBERS: - if (teamOfEntityDoingQuestion != null) { - List teamMembers = studentsLogic.getStudentsForTeam(teamOfEntityDoingQuestion, - feedbackQuestionAttributes.getCourseId()); - - if (generateOptionsFor == FeedbackParticipantType.OWN_TEAM_MEMBERS) { - teamMembers.removeIf(teamMember -> teamMember.getEmail().equals(emailOfEntityDoingQuestion)); - } - teamMembers.forEach(teamMember -> optionList.add(teamMember.getName())); + teamMembers.forEach(teamMember -> optionList.add(teamMember.getName())); - optionList.sort(null); - } - break; - case INSTRUCTORS: - List instructorList = - instructorsLogic.getInstructorsForCourse(feedbackQuestionAttributes.getCourseId()); + optionList.sort(null); + } + } - for (InstructorAttributes instructor : instructorList) { - optionList.add(instructor.getName()); - } - optionList.sort(null); - break; - default: - assert false : "Trying to generate options for neither students, teams nor instructors"; - break; + private void handleInstructorsOptions(FeedbackQuestionAttributes feedbackQuestionAttributes, List optionList) { + List instructorList = + instructorsLogic.getInstructorsForCourse(feedbackQuestionAttributes.getCourseId()); + + for (InstructorAttributes instructor : instructorList) { + optionList.add(instructor.getName()); } + optionList.sort(null); + } + + private void updateQuestionDetails(FeedbackQuestionAttributes feedbackQuestionAttributes, List optionList) { if (feedbackQuestionAttributes.getQuestionType() == FeedbackQuestionType.MCQ) { FeedbackMcqQuestionDetails feedbackMcqQuestionDetails = (FeedbackMcqQuestionDetails) feedbackQuestionAttributes.getQuestionDetailsCopy(); @@ -683,6 +709,7 @@ public void populateFieldsToGenerateInQuestion(FeedbackQuestionAttributes feedba } } + /** * Updates a feedback question by {@code FeedbackQuestionAttributes.UpdateOptions}. * @@ -736,7 +763,7 @@ public FeedbackQuestionAttributes updateFeedbackQuestionCascade(FeedbackQuestion * if the new number is bigger, then shift down(decrease qn#) all questions in between. */ private void adjustQuestionNumbers(int oldQuestionNumber, - int newQuestionNumber, List questions) { + int newQuestionNumber, List questions) { try { if (oldQuestionNumber > newQuestionNumber && oldQuestionNumber >= 1) { for (int i = oldQuestionNumber - 1; i >= newQuestionNumber; i--) { @@ -767,7 +794,7 @@ private void adjustQuestionNumbers(int oldQuestionNumber, */ public void deleteFeedbackQuestionCascade(String feedbackQuestionId) { FeedbackQuestionAttributes questionToDelete = - getFeedbackQuestion(feedbackQuestionId); + getFeedbackQuestion(feedbackQuestionId); if (questionToDelete == null) { return; // Silently fail if question does not exist. @@ -797,14 +824,14 @@ public void deleteFeedbackQuestions(AttributesDeletionQuery query) { // Shifts all question numbers after questionNumberToShiftFrom down by one. private void shiftQuestionNumbersDown(int questionNumberToShiftFrom, - List questionsToShift) { + List questionsToShift) { for (FeedbackQuestionAttributes question : questionsToShift) { if (question.getQuestionNumber() > questionNumberToShiftFrom) { try { fqDb.updateFeedbackQuestion( FeedbackQuestionAttributes.updateOptionsBuilder(question.getId()) - .withQuestionNumber(question.getQuestionNumber() - 1) - .build()); + .withQuestionNumber(question.getQuestionNumber() - 1) + .build()); } catch (InvalidParametersException | EntityDoesNotExistException e) { assert false : "Shifting question number should not cause: " + e.getMessage(); } diff --git a/src/main/java/teammates/logic/core/FeedbackResponsesLogic.java b/src/main/java/teammates/logic/core/FeedbackResponsesLogic.java index 45a91a6945c..872b421cfb5 100644 --- a/src/main/java/teammates/logic/core/FeedbackResponsesLogic.java +++ b/src/main/java/teammates/logic/core/FeedbackResponsesLogic.java @@ -1,27 +1,7 @@ package teammates.logic.core; -import java.time.Instant; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; - -import javax.annotation.Nullable; - -import teammates.common.datatransfer.AttributesDeletionQuery; -import teammates.common.datatransfer.CourseRoster; -import teammates.common.datatransfer.FeedbackParticipantType; -import teammates.common.datatransfer.FeedbackResultFetchType; -import teammates.common.datatransfer.SessionResultsBundle; -import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; -import teammates.common.datatransfer.attributes.FeedbackResponseAttributes; -import teammates.common.datatransfer.attributes.FeedbackResponseCommentAttributes; -import teammates.common.datatransfer.attributes.InstructorAttributes; -import teammates.common.datatransfer.attributes.StudentAttributes; +import teammates.common.datatransfer.*; +import teammates.common.datatransfer.attributes.*; import teammates.common.datatransfer.questions.FeedbackQuestionType; import teammates.common.datatransfer.questions.FeedbackRankRecipientsResponseDetails; import teammates.common.datatransfer.questions.FeedbackTextResponseDetails; @@ -32,6 +12,10 @@ import teammates.common.util.RequestTracer; import teammates.storage.api.FeedbackResponsesDb; +import javax.annotation.Nullable; +import java.time.Instant; +import java.util.*; + /** * Handles operations related to feedback responses. * @@ -368,7 +352,7 @@ private SessionResultsBundle buildResultsBundle( // related questions, responses, and comment Map relatedQuestionsMap = new HashMap<>(); Map relatedQuestionsNotVisibleForPreviewMap = new HashMap<>(); - Set relatedQuestionsWithCommentNotVisibleForPreview = new HashSet<>(); + Set unseenCommentQuestions = new HashSet<>(); Map relatedResponsesMap = new HashMap<>(); Map> relatedCommentsMap = new HashMap<>(); if (isCourseWide) { @@ -447,7 +431,7 @@ private SessionResultsBundle buildResultsBundle( // if previewing results and the comment should not be visible to instructors, // note down the corresponding question and do not add the comment if (isPreviewResults && !canInstructorsSeeComment(frc)) { - relatedQuestionsWithCommentNotVisibleForPreview.add(frc.getFeedbackQuestionId()); + unseenCommentQuestions.add(frc.getFeedbackQuestionId()); continue; } @@ -467,7 +451,7 @@ private SessionResultsBundle buildResultsBundle( RequestTracer.checkRemainingTime(); return new SessionResultsBundle(relatedQuestionsMap, relatedQuestionsNotVisibleForPreviewMap, - relatedQuestionsWithCommentNotVisibleForPreview, + unseenCommentQuestions, existingResponses, missingResponses, responseGiverVisibilityTable, responseRecipientVisibilityTable, relatedCommentsMap, commentVisibilityTable, roster); } diff --git a/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java b/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java index 120d3797d1c..8f9cd809592 100644 --- a/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java +++ b/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java @@ -1,12 +1,5 @@ package teammates.logic.core; -import java.time.Instant; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.function.Consumer; -import java.util.stream.Collectors; - import teammates.common.datatransfer.AttributesDeletionQuery; import teammates.common.datatransfer.FeedbackParticipantType; import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; @@ -18,8 +11,16 @@ import teammates.common.util.Const; import teammates.common.util.Logger; import teammates.common.util.TimeHelper; +import teammates.logic.api.EmailGenerator; import teammates.storage.api.FeedbackSessionsDb; +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; +import java.util.stream.Collectors; + /** * Handles operations related to feedback sessions. * @@ -51,6 +52,7 @@ public final class FeedbackSessionsLogic { private InstructorsLogic instructorsLogic; private StudentsLogic studentsLogic; private DeadlineExtensionsLogic deLogic; + private EmailGenerator emailGenerator; private FeedbackSessionsLogic() { // prevent initialization @@ -574,7 +576,7 @@ private List getSoftDeletedFeedbackSessionsListForCou */ public boolean isFeedbackSessionViewableToUserType(FeedbackSessionAttributes session, boolean isInstructor) { // Allow user to view the feedback session if there are questions for them - if (isFeedbackSessionForUserTypeToAnswer(session, isInstructor)) { + if (emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, isInstructor)) { return true; } @@ -594,19 +596,6 @@ public boolean isFeedbackSessionViewableToUserType(FeedbackSessionAttributes ses return session.isVisible() && !questionsWithVisibleResponses.isEmpty(); } - /** - * Returns true if there are any questions for the specified user type (students/instructors) to answer. - */ - public boolean isFeedbackSessionForUserTypeToAnswer(FeedbackSessionAttributes session, boolean isInstructor) { - if (!session.isVisible()) { - return false; - } - - return isInstructor - ? fqLogic.hasFeedbackQuestionsForInstructors(session, false) - : fqLogic.hasFeedbackQuestionsForStudents(session); - } - private void updateFeedbackSessionsDeadlinesWithNewEmail(String courseId, String oldEmailAddress, String newEmailAddress, boolean isInstructor) { if (oldEmailAddress.equals(newEmailAddress)) { diff --git a/src/main/java/teammates/logic/core/FeedbackVariables.java b/src/main/java/teammates/logic/core/FeedbackVariables.java new file mode 100644 index 00000000000..3503e361d73 --- /dev/null +++ b/src/main/java/teammates/logic/core/FeedbackVariables.java @@ -0,0 +1,17 @@ +package teammates.logic.core; + +public class FeedbackVariables { + protected FeedbackResponsesLogic frLogic; + protected FeedbackSessionsLogic fsLogic; + protected DeadlineExtensionsLogic deLogic; + protected FeedbackResponseCommentsLogic frcLogic; + public FeedbackQuestionsLogic fqLogic; + + void initLogicDependencies() { + frLogic = FeedbackResponsesLogic.inst(); + fsLogic = FeedbackSessionsLogic.inst(); + deLogic = DeadlineExtensionsLogic.inst(); + fqLogic = FeedbackQuestionsLogic.inst(); + frcLogic = FeedbackResponseCommentsLogic.inst(); + } +} diff --git a/src/main/java/teammates/logic/core/GiverTypeHandler.java b/src/main/java/teammates/logic/core/GiverTypeHandler.java new file mode 100644 index 00000000000..d8a527cd588 --- /dev/null +++ b/src/main/java/teammates/logic/core/GiverTypeHandler.java @@ -0,0 +1,9 @@ +package teammates.logic.core; + +import java.util.*; +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; + +public interface GiverTypeHandler { + List getPossibleGivers(FeedbackQuestionAttributes fqa, CourseRoster courseRoster, FeedbackSessionsLogic fsLogic); +} \ No newline at end of file diff --git a/src/main/java/teammates/logic/core/GiversLogic.java b/src/main/java/teammates/logic/core/GiversLogic.java new file mode 100644 index 00000000000..61020457339 --- /dev/null +++ b/src/main/java/teammates/logic/core/GiversLogic.java @@ -0,0 +1,35 @@ +package teammates.logic.core; + +import java.util.*; +import java.util.stream.Collectors; + +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.FeedbackParticipantType; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; +import teammates.common.util.Logger; + +public class GiversLogic { + private Map giverTypeHandlerMap; + private static final Logger log = Logger.getLogger(); + + public GiversLogic() { + giverTypeHandlerMap = new HashMap<>(); + giverTypeHandlerMap.put(FeedbackParticipantType.STUDENTS, new StudentsGiverTypeHandler()); + giverTypeHandlerMap.put(FeedbackParticipantType.INSTRUCTORS, new InstructorsGiverTypeHandler()); + giverTypeHandlerMap.put(FeedbackParticipantType.TEAMS, new TeamsGiverTypeHandler()); + giverTypeHandlerMap.put(FeedbackParticipantType.SELF, new SelfGiverTypeHandler()); + } + + public List getPossibleGivers( + FeedbackQuestionAttributes fqa,CourseRoster courseRoster, FeedbackSessionsLogic fsLogic) { + FeedbackParticipantType giverType = fqa.getGiverType(); + GiverTypeHandler handler = giverTypeHandlerMap.get(giverType); + + if (handler != null) { + return handler.getPossibleGivers(fqa, courseRoster, fsLogic); + } else { + log.severe("Invalid giver type specified"); + return Collections.emptyList(); + } + } +} diff --git a/src/main/java/teammates/logic/core/InstructorsGiverTypeHandler.java b/src/main/java/teammates/logic/core/InstructorsGiverTypeHandler.java new file mode 100644 index 00000000000..ba946255a6d --- /dev/null +++ b/src/main/java/teammates/logic/core/InstructorsGiverTypeHandler.java @@ -0,0 +1,18 @@ +package teammates.logic.core; + +import java.util.List; +import java.util.stream.Collectors; + +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; +import teammates.common.datatransfer.attributes.InstructorAttributes; + +public class InstructorsGiverTypeHandler implements GiverTypeHandler { + @Override + public List getPossibleGivers(FeedbackQuestionAttributes fqa, CourseRoster courseRoster, FeedbackSessionsLogic fsLogic) { + return courseRoster.getInstructors() + .stream() + .map(InstructorAttributes::getEmail) + .collect(Collectors.toList()); + } +} \ No newline at end of file diff --git a/src/main/java/teammates/logic/core/InstructorsLogic.java b/src/main/java/teammates/logic/core/InstructorsLogic.java index 0af7387f933..65420b83a38 100644 --- a/src/main/java/teammates/logic/core/InstructorsLogic.java +++ b/src/main/java/teammates/logic/core/InstructorsLogic.java @@ -26,7 +26,7 @@ * @see InstructorAttributes * @see InstructorsDb */ -public final class InstructorsLogic { +public final class InstructorsLogic extends FeedbackVariables { private static final Logger log = Logger.getLogger(); @@ -34,12 +34,6 @@ public final class InstructorsLogic { private final InstructorsDb instructorsDb = InstructorsDb.inst(); - private FeedbackResponsesLogic frLogic; - private FeedbackResponseCommentsLogic frcLogic; - private FeedbackQuestionsLogic fqLogic; - private FeedbackSessionsLogic fsLogic; - private DeadlineExtensionsLogic deLogic; - private InstructorsLogic() { // prevent initialization } @@ -48,14 +42,6 @@ public static InstructorsLogic inst() { return instance; } - void initLogicDependencies() { - fqLogic = FeedbackQuestionsLogic.inst(); - frLogic = FeedbackResponsesLogic.inst(); - frcLogic = FeedbackResponseCommentsLogic.inst(); - fsLogic = FeedbackSessionsLogic.inst(); - deLogic = DeadlineExtensionsLogic.inst(); - } - /** * Creates or updates search document for the given instructor. * diff --git a/src/main/java/teammates/logic/core/SelfGiverTypeHandler.java b/src/main/java/teammates/logic/core/SelfGiverTypeHandler.java new file mode 100644 index 00000000000..2428d2d265a --- /dev/null +++ b/src/main/java/teammates/logic/core/SelfGiverTypeHandler.java @@ -0,0 +1,17 @@ +package teammates.logic.core; + +import java.util.Collections; +import java.util.List; + +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; +import teammates.common.datatransfer.attributes.FeedbackSessionAttributes; + +public class SelfGiverTypeHandler implements GiverTypeHandler { + @Override + public List getPossibleGivers(FeedbackQuestionAttributes fqa, CourseRoster courseRoster, FeedbackSessionsLogic fsLogic) { + FeedbackSessionAttributes feedbackSession = + fsLogic.getFeedbackSession(fqa.getFeedbackSessionName(), fqa.getCourseId()); + return Collections.singletonList(feedbackSession.getCreatorEmail()); + } +} diff --git a/src/main/java/teammates/logic/core/StudentsGiverTypeHandler.java b/src/main/java/teammates/logic/core/StudentsGiverTypeHandler.java new file mode 100644 index 00000000000..7d952360e06 --- /dev/null +++ b/src/main/java/teammates/logic/core/StudentsGiverTypeHandler.java @@ -0,0 +1,18 @@ +package teammates.logic.core; + +import java.util.List; +import java.util.stream.Collectors; + +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; +import teammates.common.datatransfer.attributes.StudentAttributes; + +public class StudentsGiverTypeHandler implements GiverTypeHandler { + @Override + public List getPossibleGivers(FeedbackQuestionAttributes fqa, CourseRoster courseRoster, FeedbackSessionsLogic fsLogic) { + return courseRoster.getStudents() + .stream() + .map(StudentAttributes::getEmail) + .collect(Collectors.toList()); + } +} diff --git a/src/main/java/teammates/logic/core/StudentsLogic.java b/src/main/java/teammates/logic/core/StudentsLogic.java index 647b10f8955..a2e838c1b9b 100644 --- a/src/main/java/teammates/logic/core/StudentsLogic.java +++ b/src/main/java/teammates/logic/core/StudentsLogic.java @@ -24,7 +24,7 @@ * @see StudentAttributes * @see StudentsDb */ -public final class StudentsLogic { +public final class StudentsLogic extends FeedbackVariables { static final String ERROR_INVALID_TEAM_NAME = "Team \"%s\" is detected in both Section \"%s\" and Section \"%s\"."; @@ -39,9 +39,6 @@ public final class StudentsLogic { private final StudentsDb studentsDb = StudentsDb.inst(); - private FeedbackResponsesLogic frLogic; - private FeedbackSessionsLogic fsLogic; - private DeadlineExtensionsLogic deLogic; private StudentsLogic() { // prevent initialization @@ -51,12 +48,6 @@ public static StudentsLogic inst() { return instance; } - void initLogicDependencies() { - frLogic = FeedbackResponsesLogic.inst(); - fsLogic = FeedbackSessionsLogic.inst(); - deLogic = DeadlineExtensionsLogic.inst(); - } - /** * Creates a student. * diff --git a/src/main/java/teammates/logic/core/TeamsGiverTypeHandler.java b/src/main/java/teammates/logic/core/TeamsGiverTypeHandler.java new file mode 100644 index 00000000000..e0575f880df --- /dev/null +++ b/src/main/java/teammates/logic/core/TeamsGiverTypeHandler.java @@ -0,0 +1,14 @@ +package teammates.logic.core; + +import java.util.ArrayList; +import java.util.List; + +import teammates.common.datatransfer.CourseRoster; +import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes; + +public class TeamsGiverTypeHandler implements GiverTypeHandler { + @Override + public List getPossibleGivers(FeedbackQuestionAttributes fqa, CourseRoster courseRoster, FeedbackSessionsLogic fsLogic) { + return new ArrayList<>(courseRoster.getTeamToMembersTable().keySet()); + } +} diff --git a/src/test/java/teammates/logic/api/EmailGeneratorTest.java b/src/test/java/teammates/logic/api/EmailGeneratorTest.java index 971f59c6b01..33800ff36cc 100644 --- a/src/test/java/teammates/logic/api/EmailGeneratorTest.java +++ b/src/test/java/teammates/logic/api/EmailGeneratorTest.java @@ -728,6 +728,29 @@ public void testGenerateCompiledLogsEmail() throws Exception { verifyEmail(email, Config.SUPPORT_EMAIL, subject, "/severeLogsCompilationEmail.html"); } + @Test + public void testIsFeedbackSessionForUserTypeToAnswer() { + ______TS("Non-visible session should not be for any types of user to answer"); + FeedbackSessionAttributes session = dataBundle.feedbackSessions.get("awaiting.session"); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, false)); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, true)); + + ______TS("Empty session should not be for any types of user to answer"); + session = dataBundle.feedbackSessions.get("empty.session"); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, false)); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, true)); + + ______TS("Session without student question should not be for students to answer"); + session = dataBundle.feedbackSessions.get("archiveCourse.session1"); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, false)); + assertTrue(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, true)); + + ______TS("Session without instructor question should not be for instructors to answer"); + session = dataBundle.feedbackSessions.get("session2InCourse1"); + assertFalse(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, true)); + assertTrue(emailGenerator.isFeedbackSessionForUserTypeToAnswer(session, false)); + } + private void verifyEmail(EmailWrapper email, String recipient, String subject, String emailContentFilePath) throws Exception { // check recipient diff --git a/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java b/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java index 75301a2d593..774b3d8f549 100644 --- a/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java +++ b/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java @@ -155,8 +155,6 @@ public void testAll() throws Exception { testCreateAndDeleteFeedbackSession(); - testIsFeedbackSessionForUserTypeToAnswer(); - testUpdateFeedbackSession(); testPublishUnpublishFeedbackSession(); @@ -524,28 +522,6 @@ private void testIsFeedbackSessionViewableToUserType() { assertFalse(fsLogic.isFeedbackSessionViewableToUserType(session, true)); } - private void testIsFeedbackSessionForUserTypeToAnswer() { - ______TS("Non-visible session should not be for any types of user to answer"); - FeedbackSessionAttributes session = dataBundle.feedbackSessions.get("awaiting.session"); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false)); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true)); - - ______TS("Empty session should not be for any types of user to answer"); - session = dataBundle.feedbackSessions.get("empty.session"); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false)); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true)); - - ______TS("Session without student question should not be for students to answer"); - session = dataBundle.feedbackSessions.get("archiveCourse.session1"); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false)); - assertTrue(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true)); - - ______TS("Session without instructor question should not be for instructors to answer"); - session = dataBundle.feedbackSessions.get("session2InCourse1"); - assertFalse(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, true)); - assertTrue(fsLogic.isFeedbackSessionForUserTypeToAnswer(session, false)); - } - private void testUpdateFeedbackSession() throws Exception { ______TS("failure: non-existent session name");