[TEAMMATES#11585] Remove unnecessary data read (TEAMMATES#11646)

kaixin-hc · Mar 13, 2022 · 0642966 · 0642966
1 parent a3c4ba0
commit 0642966
Show file tree

Hide file tree

Showing 9 changed files with 138 additions and 6 deletions.
diff --git a/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java b/src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java
@@ -140,6 +140,25 @@ private boolean areQuestionNumbersConsistent(List<FeedbackQuestionAttributes> qu
         return true;
     }
 
+    /**
+     * 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;
+    }
+
     /**
      * Gets a {@code List} of all questions for the given session that instructors can view/submit.
      */
@@ -180,6 +199,15 @@ public List<FeedbackQuestionAttributes> getFeedbackQuestionsForInstructors(
         return questions;
     }
 
+    /**
+     * 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);
+    }
+
     /**
      * Gets a {@code List} of all questions for the given session that students can view/submit.
      */

diff --git a/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java b/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java
@@ -208,6 +208,9 @@ public List<FeedbackSessionAttributes> getFeedbackSessionsWhichNeedOpenEmailsToB
      */
     public boolean isCreatorOfSession(String feedbackSessionName, String courseId, String userEmail) {
         FeedbackSessionAttributes fs = getFeedbackSession(feedbackSessionName, courseId);
+        if (fs == null) {
+            return false;
+        }
         return fs.getCreatorEmail().equals(userEmail);
     }
 
@@ -558,11 +561,13 @@ public boolean isFeedbackSessionViewableToUserType(FeedbackSessionAttributes ses
      * Returns true if there are any questions for the specified user type (students/instructors) to answer.
      */
     public boolean isFeedbackSessionForUserTypeToAnswer(FeedbackSessionAttributes session, boolean isInstructor) {
-        List<FeedbackQuestionAttributes> questionsToAnswer = isInstructor
-                ? fqLogic.getFeedbackQuestionsForInstructors(session.getFeedbackSessionName(), session.getCourseId(), null)
-                : fqLogic.getFeedbackQuestionsForStudents(session.getFeedbackSessionName(), session.getCourseId());
+        if (!session.isVisible()) {
+            return false;
+        }
 
-        return session.isVisible() && !questionsToAnswer.isEmpty();
+        return isInstructor
+                ? fqLogic.hasFeedbackQuestionsForInstructors(session.getFeedbackSessionName(), session.getCourseId(), null)
+                : fqLogic.hasFeedbackQuestionsForStudents(session.getFeedbackSessionName(), session.getCourseId());
     }
 
 }
diff --git a/src/main/java/teammates/storage/api/FeedbackResponsesDb.java b/src/main/java/teammates/storage/api/FeedbackResponsesDb.java
@@ -419,6 +419,7 @@ boolean hasExistingEntities(FeedbackResponseAttributes entityToCreate) {
                 .filterKey(Key.create(FeedbackResponse.class,
                         FeedbackResponse.generateId(entityToCreate.getFeedbackQuestionId(),
                                 entityToCreate.getGiver(), entityToCreate.getRecipient())))
+                .keys()
                 .list()
                 .isEmpty();
     }

diff --git a/src/main/java/teammates/storage/api/FeedbackSessionsDb.java b/src/main/java/teammates/storage/api/FeedbackSessionsDb.java
@@ -416,6 +416,7 @@ boolean hasExistingEntities(FeedbackSessionAttributes entityToCreate) {
         return !load()
                 .filterKey(Key.create(FeedbackSession.class,
                         FeedbackSession.generateId(entityToCreate.getFeedbackSessionName(), entityToCreate.getCourseId())))
+                .keys()
                 .list()
                 .isEmpty();
     }

diff --git a/src/main/java/teammates/storage/api/StudentsDb.java b/src/main/java/teammates/storage/api/StudentsDb.java
@@ -368,6 +368,7 @@ boolean hasExistingEntities(StudentAttributes entityToCreate) {
         return !load()
                 .filterKey(Key.create(CourseStudent.class,
                         CourseStudent.generateId(entityToCreate.getEmail(), entityToCreate.getCourse())))
+                .keys()
                 .list()
                 .isEmpty();
     }

diff --git a/src/test/java/teammates/logic/core/FeedbackQuestionsLogicTest.java b/src/test/java/teammates/logic/core/FeedbackQuestionsLogicTest.java
@@ -77,7 +77,9 @@ public void testDeleteFeedbackQuestions_byCourseIdAndSessionName_shouldDeleteQue
 
     @Test
     public void allTests() throws Exception {
+        testHasFeedbackQuestionsForInstructor();
         testGetFeedbackQuestionsForInstructor();
+        testHasFeedbackQuestionsForStudents();
         testGetFeedbackQuestionsForStudents();
         testAddQuestion();
     }
@@ -905,6 +907,20 @@ public void testBuildCompleteGiverRecipientMap_teamQuestion_shouldBuildMapCorrec
         assertTrue(completeGiverRecipientMap.get("Team 1.2").contains("Team 1.1</td></div>'\""));
     }
 
+    private void testHasFeedbackQuestionsForInstructor() {
+        ______TS("Valid session valid instructor should have questions");
+        assertTrue(fqLogic.hasFeedbackQuestionsForInstructors(
+                "First feedback session", "idOfTypicalCourse1", "[email protected]"));
+
+        ______TS("Valid session without questions for instructor should return false");
+        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(
+                "session without instructor questions", "idOfTestingSanitizationCourse", "[email protected]"));
+
+        ______TS("Invalid session should not have questions");
+        assertFalse(fqLogic.hasFeedbackQuestionsForInstructors(
+                "Zeroth feedback session", "idOfTypicalCourse1", "[email protected]"));
+    }
+
     private void testGetFeedbackQuestionsForInstructor() {
         List<FeedbackQuestionAttributes> expectedQuestions;
         List<FeedbackQuestionAttributes> actualQuestions;
@@ -963,6 +979,20 @@ private void testGetFeedbackQuestionsForInstructor() {
         assertEquals(actualQuestions, expectedQuestions);
     }
 
+    private void testHasFeedbackQuestionsForStudents() {
+        ______TS("Valid session should have questions");
+        assertTrue(fqLogic.hasFeedbackQuestionsForStudents(
+                "First feedback session", "idOfTypicalCourse1"));
+
+        ______TS("Valid session without questions for students should return false");
+        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(
+                "session without student questions", "idOfArchivedCourse"));
+
+        ______TS("Invalid session should not have questions");
+        assertFalse(fqLogic.hasFeedbackQuestionsForStudents(
+                "Zeroth feedback session", "idOfTypicalCourse1"));
+    }
+
     private void testGetFeedbackQuestionsForStudents() {
         List<FeedbackQuestionAttributes> expectedQuestions;
         List<FeedbackQuestionAttributes> actualQuestions;

diff --git a/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java b/src/test/java/teammates/logic/core/FeedbackSessionsLogicTest.java
@@ -151,6 +151,8 @@ public void testAll() throws Exception {
 
         testCreateAndDeleteFeedbackSession();
 
+        testIsFeedbackSessionForUserTypeToAnswer();
+
         testUpdateFeedbackSession();
         testPublishUnpublishFeedbackSession();
 
@@ -518,6 +520,28 @@ 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");

diff --git a/src/test/java/teammates/storage/api/FeedbackSessionsDbTest.java b/src/test/java/teammates/storage/api/FeedbackSessionsDbTest.java
@@ -57,8 +57,8 @@ public void testGetAllOngoingSessions_typicalCase_shouldQuerySuccessfullyWithout
         Instant rangeStart = Instant.parse("2000-12-03T10:15:30.00Z");
         Instant rangeEnd = Instant.parse("2050-04-30T21:59:00Z");
         List<FeedbackSessionAttributes> actualAttributesList = fsDb.getAllOngoingSessions(rangeStart, rangeEnd);
-        assertEquals("should not return more than 13 sessions as there are only 13 distinct sessions in the range",
-                13, actualAttributesList.size());
+        assertEquals("should not return more than 14 sessions as there are only 14 distinct sessions in the range",
+                14, actualAttributesList.size());
     }
 
     @Test

diff --git a/src/test/resources/data/typicalDataBundle.json b/src/test/resources/data/typicalDataBundle.json
@@ -903,6 +903,27 @@
       "gracePeriod": 5,
       "sentOpeningSoonEmail": true,
       "sentOpenEmail": true,
+      "sentClosingEmail": true,
+      "sentClosedEmail": true,
+      "sentPublishedEmail": true,
+      "isOpeningEmailEnabled": true,
+      "isClosingEmailEnabled": true,
+      "isPublishedEmailEnabled": true
+    },
+    "archiveCourse.session2": {
+      "feedbackSessionName": "session without instructor questions",
+      "courseId": "idOfArchivedCourse",
+      "creatorEmail": "[email protected]",
+      "instructions": "Please please fill in the following questions.",
+      "createdTime": "2013-01-20T23:00:00Z",
+      "startTime": "2013-02-20T23:00:00Z",
+      "endTime": "2026-04-28T23:00:00Z",
+      "sessionVisibleFromTime": "2013-02-20T23:00:00Z",
+      "resultsVisibleFromTime": "2026-04-29T23:00:00Z",
+      "timeZone": "Africa/Johannesburg",
+      "gracePeriod": 5,
+      "sentOpeningSoonEmail": true,
+      "sentOpenEmail": true,
       "sentClosingEmail": false,
       "sentClosedEmail": false,
       "sentPublishedEmail": false,
@@ -1356,6 +1377,27 @@
         "RECEIVER"
       ]
     },
+    "qn1InSession2InArchivedCourse": {
+      "feedbackSessionName": "session without instructor questions",
+      "courseId": "idOfArchivedCourse",
+      "questionDetails": {
+        "questionType": "TEXT",
+        "questionText": "Give feedback to each other"
+      },
+      "questionNumber": 1,
+      "giverType": "STUDENTS",
+      "recipientType": "STUDENTS",
+      "numberOfEntitiesToGiveFeedbackTo": 4,
+      "showResponsesTo": [
+        "RECEIVER"
+      ],
+      "showGiverNameTo": [
+        "RECEIVER"
+      ],
+      "showRecipientNameTo": [
+        "RECEIVER"
+      ]
+    },
     "qn1InSession1InCourse2": {
       "feedbackSessionName": "Instructor feedback session",
       "courseId": "idOfTypicalCourse2",