update: reduce more redundant data read from checking presence of ins…

…tructor questions
TEAMMATES · Mar 26, 2022 · 189d194 · 189d194
1 parent 35a490a
commit 189d194
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 44 deletions.
diff --git a/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java b/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java
@@ -144,33 +144,14 @@ private boolean areQuestionNumbersConsistent(List<FeedbackQuestionAttributes> qu
      * Checks if there are any questions for the given session that instructors can view/submit.
      */
     public boolean hasFeedbackQuestionsForInstructors(
-            String feedbackSessionName, String courseId, String userEmail) {
-        boolean hasQuestions = fqDb.hasFeedbackQuestionsForGiverType(
-                feedbackSessionName, courseId, FeedbackParticipantType.INSTRUCTORS);
-        if (hasQuestions) {
-            return true;
-        }
-
-        if (userEmail != null && fsLogic.isCreatorOfSession(feedbackSessionName, courseId, userEmail)) {
-            hasQuestions = fqDb.hasFeedbackQuestionsForGiverType(
-                    feedbackSessionName, courseId, FeedbackParticipantType.SELF);
-        }
-
-        return hasQuestions;
-    }
-
-    /**
-     * Checks if there are any questions for the given session that instructors can view/submit.
-     */
-    public boolean hasFeedbackQuestionsForInstructors(
-            FeedbackSessionAttributes fsa, String userEmail) {
+            FeedbackSessionAttributes fsa, Boolean isCreator) {
         boolean hasQuestions = fqDb.hasFeedbackQuestionsForGiverType(
                 fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.INSTRUCTORS);
         if (hasQuestions) {
             return true;
         }
 
-        if (userEmail != null && fsa.isCreator(userEmail)) {
+        if (isCreator) {
             hasQuestions = fqDb.hasFeedbackQuestionsForGiverType(
                     fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.SELF);
         }
@@ -221,10 +202,11 @@ public List<FeedbackQuestionAttributes> getFeedbackQuestionsForInstructors(
     /**
      * Checks if there are any questions for the given session that students can view/submit.
      */
-    public boolean hasFeedbackQuestionsForStudents(
-            String feedbackSessionName, String courseId) {
-        return fqDb.hasFeedbackQuestionsForGiverType(feedbackSessionName, courseId, FeedbackParticipantType.STUDENTS)
-                || fqDb.hasFeedbackQuestionsForGiverType(feedbackSessionName, courseId, FeedbackParticipantType.TEAMS);
+    public boolean hasFeedbackQuestionsForStudents(FeedbackSessionAttributes fsa) {
+        return fqDb.hasFeedbackQuestionsForGiverType(
+                fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.STUDENTS)
+                || fqDb.hasFeedbackQuestionsForGiverType(
+                        fsa.getFeedbackSessionName(), fsa.getCourseId(), FeedbackParticipantType.TEAMS);
     }
 
     /**

diff --git a/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java b/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java
@@ -3,6 +3,7 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import teammates.common.datatransfer.AttributesDeletionQuery;
@@ -495,14 +496,25 @@ public int getExpectedTotalSubmission(FeedbackSessionAttributes fsa) {
         List<String> instructorEmails = instructorsLogic.getInstructorEmailsForCourse(fsa.getCourseId());
 
         int expectedTotal = 0;
-        if (fqLogic.hasFeedbackQuestionsForStudents(fsa.getFeedbackSessionName(), fsa.getCourseId())) {
+        if (fqLogic.hasFeedbackQuestionsForStudents(fsa)) {
             expectedTotal += numOfStudents;
         }
 
-        for (String instructorEmail : instructorEmails) {
-            if (fqLogic.hasFeedbackQuestionsForInstructors(fsa, instructorEmail)) {
-                expectedTotal += 1;
-            }
+        // Check a creator and a non-creator once each to prevent unnecessary data read.
+        List<String> creatorEmails = instructorEmails.stream()
+                .filter(fsa::isCreator)
+                .collect(Collectors.toList());
+        if (!creatorEmails.isEmpty()
+                && fqLogic.hasFeedbackQuestionsForInstructors(fsa, true)) {
+            expectedTotal += creatorEmails.size();
+        }
+
+        List<String> nonCreatorEmails = instructorEmails.stream()
+                .filter(email -> !fsa.isCreator(email))
+                .collect(Collectors.toList());
+        if (!nonCreatorEmails.isEmpty()
+                && fqLogic.hasFeedbackQuestionsForInstructors(fsa, false)) {
+            expectedTotal += nonCreatorEmails.size();
         }
 
         return expectedTotal;
@@ -559,8 +571,8 @@ public boolean isFeedbackSessionForUserTypeToAnswer(FeedbackSessionAttributes se
         }
 
         return isInstructor
-                ? fqLogic.hasFeedbackQuestionsForInstructors(session.getFeedbackSessionName(), session.getCourseId(), null)
-                : fqLogic.hasFeedbackQuestionsForStudents(session.getFeedbackSessionName(), session.getCourseId());
+                ? fqLogic.hasFeedbackQuestionsForInstructors(session, false)
+                : fqLogic.hasFeedbackQuestionsForStudents(session);
     }
 
 }
diff --git a/src/test/java/teammates/logic/core/FeedbackQuestionsLogicTest.java b/src/test/java/teammates/logic/core/FeedbackQuestionsLogicTest.java
@@ -907,18 +907,19 @@ public void testBuildCompleteGiverRecipientMap_teamQuestion_shouldBuildMapCorrec
         assertTrue(completeGiverRecipientMap.get("Team 1.2").contains("Team 1.1</td></div>'\""));
     }
 
+    @Test
     private void testHasFeedbackQuestionsForInstructor() {
         ______TS("Valid session valid instructor should have questions");
-        assertTrue(fqLogic.hasFeedbackQuestionsForInstructors(
-                "First feedback session", "idOfTypicalCourse1", "instructor1@course1.tmt"));
+        FeedbackSessionAttributes fsa = fsLogic.getFeedbackSession("First feedback session", "idOfTypicalCourse1");
+        assertTrue(fqLogic.hasFeedbackQuestionsForInstructors(fsa, false));
 
         ______TS("Valid session without questions for instructor should return false");
-        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(
-                "session without instructor questions", "idOfTestingSanitizationCourse", "instructor1@course1.tmt"));
+        fsa = fsLogic.getFeedbackSession("session without instructor questions", "idOfArchivedCourse");
+        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(fsa, false));
 
         ______TS("Invalid session should not have questions");
-        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(
-                "Zeroth feedback session", "idOfTypicalCourse1", "nonexistent@course1.tmt"));
+        fsa.setFeedbackSessionName("non-existent session");
+        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(fsa, false));
     }
 
     private void testGetFeedbackQuestionsForInstructor() {
@@ -981,16 +982,16 @@ private void testGetFeedbackQuestionsForInstructor() {
 
     private void testHasFeedbackQuestionsForStudents() {
         ______TS("Valid session should have questions");
-        assertTrue(fqLogic.hasFeedbackQuestionsForStudents(
-                "First feedback session", "idOfTypicalCourse1"));
+        FeedbackSessionAttributes fsa = fsLogic.getFeedbackSession("First feedback session", "idOfTypicalCourse1");
+        assertTrue(fqLogic.hasFeedbackQuestionsForStudents(fsa));
 
         ______TS("Valid session without questions for students should return false");
-        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(
-                "session without student questions", "idOfArchivedCourse"));
+        fsa = fsLogic.getFeedbackSession("session without student questions", "idOfArchivedCourse");
+        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(fsa));
 
         ______TS("Invalid session should not have questions");
-        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(
-                "Zeroth feedback session", "idOfTypicalCourse1"));
+        fsa = fsLogic.getFeedbackSession("Zeroth feedback session", "idOfTypicalCourse1");
+        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(fsa));
     }
 
     private void testGetFeedbackQuestionsForStudents() {