Instructor: view results: fix IndexOutOfBoundsException #8415

src/main/java/teammates/common/datatransfer/questions/FeedbackConstantSumQuestionDetails.java

@@ -715,7 +715,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         List<String> errors = new ArrayList<>();
         if (!distributeToRecipients && numOfConstSumOptions < Const.FeedbackQuestion.CONST_SUM_MIN_NUM_OF_OPTIONS) {
             errors.add(Const.FeedbackQuestion.CONST_SUM_ERROR_NOT_ENOUGH_OPTIONS

src/main/java/teammates/common/datatransfer/questions/FeedbackContributionQuestionDetails.java

@@ -709,7 +709,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         return new ArrayList<>();
     }
 

src/main/java/teammates/common/datatransfer/questions/FeedbackMcqQuestionDetails.java

@@ -455,7 +455,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         List<String> errors = new ArrayList<>();
         if (generateOptionsFor == FeedbackParticipantType.NONE
                 && numOfMcqChoices < Const.FeedbackQuestion.MCQ_MIN_NUM_OF_CHOICES) {

src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java

@@ -30,7 +30,6 @@
 import teammates.ui.template.InstructorFeedbackResultsResponseRow;
 
 public class FeedbackMsqQuestionDetails extends FeedbackQuestionDetails {
-    private int numOfMsqChoices;
     private List<String> msqChoices;
     private boolean otherEnabled;
     private FeedbackParticipantType generateOptionsFor;
@@ -41,7 +40,6 @@ public class FeedbackMsqQuestionDetails extends FeedbackQuestionDetails {
     public FeedbackMsqQuestionDetails() {
         super(FeedbackQuestionType.MSQ);
 
-        this.numOfMsqChoices = 0;
         this.msqChoices = new ArrayList<>();
         this.otherEnabled = false;
         this.generateOptionsFor = FeedbackParticipantType.NONE;
@@ -53,7 +51,6 @@ public FeedbackMsqQuestionDetails() {
     public boolean extractQuestionDetails(
             Map<String, String[]> requestParameters,
             FeedbackQuestionType questionType) {
-        int numOfMsqChoices = 0;
         List<String> msqChoices = new LinkedList<>();
         boolean msqOtherEnabled = false;
 
@@ -98,36 +95,26 @@ public boolean extractQuestionDetails(
                                 requestParameters, Const.ParamsNames.FEEDBACK_QUESTION_MSQCHOICE + "-" + i);
                 if (msqChoice != null && !msqChoice.trim().isEmpty()) {
                     msqChoices.add(msqChoice);
-                    numOfMsqChoices++;
                 }
             }
 
-            setMsqQuestionDetails(numOfMsqChoices, msqChoices, msqOtherEnabled);
+            setMsqQuestionDetails(msqChoices, msqOtherEnabled);
         } else {
-            String courseId = HttpRequestHelper.getValueFromParamMap(requestParameters, Const.ParamsNames.COURSE_ID);
-            setMsqQuestionDetails(FeedbackParticipantType.valueOf(generatedMsqOptions), courseId);
+            setMsqQuestionDetails(FeedbackParticipantType.valueOf(generatedMsqOptions));
         }
         return true;
     }
 
-    private void setMsqQuestionDetails(int numOfMsqChoices, List<String> msqChoices, boolean otherEnabled) {
-        this.numOfMsqChoices = numOfMsqChoices;
+    private void setMsqQuestionDetails(List<String> msqChoices, boolean otherEnabled) {
         this.msqChoices = msqChoices;
         this.otherEnabled = otherEnabled;
         this.generateOptionsFor = FeedbackParticipantType.NONE;
     }
 
-    private void setMsqQuestionDetails(FeedbackParticipantType generateOptionsFor, String courseId) {
+    private void setMsqQuestionDetails(FeedbackParticipantType generateOptionsFor) {
         this.msqChoices = new ArrayList<>();
         this.otherEnabled = false;
         this.generateOptionsFor = generateOptionsFor;
-        if (generateOptionsFor.equals(FeedbackParticipantType.STUDENTS_EXCLUDING_SELF)) {
-            this.numOfMsqChoices = generateNumOfChoicesForStudentsExcludingSelf(courseId);
-        } else if (generateOptionsFor.equals(FeedbackParticipantType.TEAMS_EXCLUDING_SELF)) {
-            this.numOfMsqChoices = generateNumOfChoicesForTeamsExcludingSelf(courseId);
-        } else {
-            this.numOfMsqChoices = generateOptionList(courseId).size();
-        }
         Assumption.assertTrue(
                 "Can only generate students, students (excluding self), teams, teams (excluding self) or instructors",
                 generateOptionsFor == FeedbackParticipantType.STUDENTS
@@ -155,7 +142,7 @@ public boolean getOtherEnabled() {
     public boolean shouldChangesRequireResponseDeletion(FeedbackQuestionDetails newDetails) {
         FeedbackMsqQuestionDetails newMsqDetails = (FeedbackMsqQuestionDetails) newDetails;
 
-        if (this.numOfMsqChoices != newMsqDetails.numOfMsqChoices
+        if (this.msqChoices.size() != newMsqDetails.msqChoices.size()
                 || !this.msqChoices.containsAll(newMsqDetails.msqChoices)
                 || !newMsqDetails.msqChoices.containsAll(this.msqChoices)) {
             return true;
@@ -341,32 +328,33 @@ public String getQuestionWithoutExistingResponseSubmissionFormHtml(
                         isMinSelectableChoicesEnabled ? Integer.toString(minSelectableChoices) : "-1");
     }
 
-    /**
-     * Gets the number of MSQ choices for students excluding self.
-     * @return number of MSQ choices for students generated.
-     */
-    private int generateNumOfChoicesForStudentsExcludingSelf(String courseId) {
-        List<StudentAttributes> studentList = StudentsLogic.inst().getStudentsForCourse(courseId);
+    public int getNumOfChoicesForMsq(String courseId, FeedbackParticipantType generateOptionsFor) {
+        if (generateOptionsFor == FeedbackParticipantType.NONE) {
+            return msqChoices.size();
+        }
 
-        return studentList.size() - 1;
-    }
+        if (generateOptionsFor == FeedbackParticipantType.STUDENTS
+                || generateOptionsFor == FeedbackParticipantType.STUDENTS_EXCLUDING_SELF) {
+            List<StudentAttributes> studentList = StudentsLogic.inst().getStudentsForCourse(courseId);
+            int sizeOfStudentlist = studentList.size();
 
-    /**
-     * Gets the number of MSQ choices for teams excluding self.
-     * @return number of MSQ choices for teams generated.
-     */
-    private int generateNumOfChoicesForTeamsExcludingSelf(String courseId) {
-        List<TeamDetailsBundle> teamList;
+            return generateOptionsFor == FeedbackParticipantType.STUDENTS ? sizeOfStudentlist : sizeOfStudentlist - 1;
+        }
 
-        try {
-            teamList = CoursesLogic.inst().getTeamsForCourse(courseId);
+        if (generateOptionsFor == FeedbackParticipantType.TEAMS
+                || generateOptionsFor == FeedbackParticipantType.TEAMS_EXCLUDING_SELF) {
+            try {
+                List<TeamDetailsBundle> teamList = CoursesLogic.inst().getTeamsForCourse(courseId);
+                int sizeOfTeamlist = teamList.size();
 
-            return teamList.size() - 1;
-        } catch (EntityDoesNotExistException e) {
-            Assumption.fail("Course disappeared");
+                return generateOptionsFor == FeedbackParticipantType.TEAMS ? sizeOfTeamlist : sizeOfTeamlist - 1;
+            } catch (EntityDoesNotExistException e) {
+                Assumption.fail("Course disappeared");
+            }
         }
+        List<InstructorAttributes> instructorList = InstructorsLogic.inst().getInstructorsForCourse(courseId);
 
-        return 0;
+        return instructorList.size();
     }
 
     private List<String> generateOptionList(String courseId) {
@@ -452,7 +440,7 @@ public String getQuestionSpecificEditFormHtml(int questionNumber) {
                 Slots.MSQ_EDIT_FORM_OPTION_FRAGMENTS, optionListHtml.toString(),
                 Slots.QUESTION_NUMBER, Integer.toString(questionNumber),
                 Slots.NUMBER_OF_CHOICE_CREATED, Const.ParamsNames.FEEDBACK_QUESTION_NUMBEROFCHOICECREATED,
-                Slots.MSQ_NUMBER_OF_CHOICES, Integer.toString(numOfMsqChoices),
+                Slots.MSQ_NUMBER_OF_CHOICES, Integer.toString(msqChoices.size()),
                 Slots.CHECKED_OTHER_OPTION_ENABLED, otherEnabled ? "checked" : "",
                 Slots.MSQ_PARAM_OTHER_OPTION, Const.ParamsNames.FEEDBACK_QUESTION_MSQOTHEROPTION,
                 Slots.MSQ_PARAM_OTHER_OPTION_FLAG, Const.ParamsNames.FEEDBACK_QUESTION_MSQOTHEROPTIONFLAG,
@@ -488,7 +476,6 @@ public String getQuestionSpecificEditFormHtml(int questionNumber) {
     @Override
     public String getNewQuestionSpecificEditFormHtml() {
         // Add two empty options by default
-        numOfMsqChoices = 2;
         msqChoices.add("");
         msqChoices.add("");
 
@@ -497,6 +484,9 @@ public String getNewQuestionSpecificEditFormHtml() {
              + "</div>";
     }
 
+    // Confusing Ternary flagged for the `else if` condition below.
+    // Note: Exclusion to this rule will be added in future PMD patch.
+    @SuppressWarnings("PMD.ConfusingTernary")
     @Override
     public String getQuestionAdditionalInfoHtml(int questionNumber, String additionalInfoId) {
         StringBuilder optionListHtml = new StringBuilder(200);
@@ -507,14 +497,12 @@ public String getQuestionAdditionalInfoHtml(int questionNumber, String additiona
                     "<br>The options for this question is automatically generated from the list of all %s in this course.",
                     generateOptionsFor.toString().toLowerCase());
             optionListHtml.append(optionHelpText);
-        }
-
-        if (numOfMsqChoices > 0) {
+        } else if (!msqChoices.isEmpty()) {
             optionListHtml.append("<ul style=\"list-style-type: disc;margin-left: 20px;\" >");
-            for (int i = 0; i < numOfMsqChoices; i++) {
+            for (String msqChoice : msqChoices) {
                 String optionFragment =
                         Templates.populateTemplate(optionFragmentTemplate,
-                                Slots.MSQ_CHOICE_VALUE, SanitizationHelper.sanitizeForHtml(msqChoices.get(i)));
+                                Slots.MSQ_CHOICE_VALUE, SanitizationHelper.sanitizeForHtml(msqChoice));
 
                 optionListHtml.append(optionFragment);
             }
@@ -611,10 +599,10 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         List<String> errors = new ArrayList<>();
         if (generateOptionsFor == FeedbackParticipantType.NONE
-                && numOfMsqChoices < Const.FeedbackQuestion.MSQ_MIN_NUM_OF_CHOICES) {
+                && msqChoices.size() < Const.FeedbackQuestion.MSQ_MIN_NUM_OF_CHOICES) {
             errors.add(Const.FeedbackQuestion.MSQ_ERROR_NOT_ENOUGH_CHOICES
                        + Const.FeedbackQuestion.MSQ_MIN_NUM_OF_CHOICES + ".");
         }
@@ -625,7 +613,8 @@ public List<String> validateQuestionDetails() {
         boolean isMinSelectableChoicesEnabled = minSelectableChoices != Integer.MIN_VALUE;
 
         if (isMaxSelectableChoicesEnabled) {
-            if (numOfMsqChoices < maxSelectableChoices) {
+            int numOfMsqChoicesForGeneratedOptions = getNumOfChoicesForMsq(courseId, generateOptionsFor);
+            if (numOfMsqChoicesForGeneratedOptions < maxSelectableChoices) {
                 errors.add(Const.FeedbackQuestion.MSQ_ERROR_MAX_SELECTABLE_EXCEEDED_TOTAL);
             } else if (maxSelectableChoices < 2) {
                 errors.add(Const.FeedbackQuestion.MSQ_ERROR_MIN_FOR_MAX_SELECTABLE_CHOICES);
@@ -686,10 +675,6 @@ public String validateGiverRecipientVisibility(FeedbackQuestionAttributes feedba
         return "";
     }
 
-    public int getNumOfMsqChoices() {
-        return numOfMsqChoices;
-    }
-
     public List<String> getMsqChoices() {
         return msqChoices;
     }

src/main/java/teammates/common/datatransfer/questions/FeedbackNumericalScaleQuestionDetails.java

@@ -711,7 +711,7 @@ private String getPossibleValuesString() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         List<String> errors = new ArrayList<>();
         if (minScale >= maxScale) {
             errors.add(Const.FeedbackQuestion.NUMSCALE_ERROR_MIN_MAX);

src/main/java/teammates/common/datatransfer/questions/FeedbackQuestionDetails.java

@@ -135,11 +135,11 @@ public boolean isIndividualResponsesShownToStudents() {
 
     /**
      * Validates the question details.
-     *
+     * @param courseId courseId of the question
      * @return A {@code List<String>} of error messages (to show as status message to user) if any, or an
      *         empty list if question details are valid.
      */
-    public abstract List<String> validateQuestionDetails();
+    public abstract List<String> validateQuestionDetails(String courseId);
 
     /**
      * Validates {@code List<FeedbackResponseAttributes>} for the question

src/main/java/teammates/common/datatransfer/questions/FeedbackRankOptionsQuestionDetails.java

@@ -431,7 +431,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         List<String> errors = new ArrayList<>();
         if (options.size() < MIN_NUM_OF_OPTIONS) {
             errors.add(ERROR_NOT_ENOUGH_OPTIONS + MIN_NUM_OF_OPTIONS + ".");

src/main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

@@ -453,7 +453,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         return new ArrayList<>();
     }
 

src/main/java/teammates/common/datatransfer/questions/FeedbackRubricQuestionDetails.java

@@ -981,7 +981,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         // For rubric questions,
         // 1) Description size should be valid
         // 2) At least 2 choices

src/main/java/teammates/common/datatransfer/questions/FeedbackTextQuestionDetails.java

@@ -169,7 +169,7 @@ public String getQuestionTypeChoiceOption() {
     }
 
     @Override
-    public List<String> validateQuestionDetails() {
+    public List<String> validateQuestionDetails(String courseId) {
         return new ArrayList<>();
     }
 

src/main/java/teammates/ui/controller/InstructorFeedbackQuestionAddAction.java

@@ -31,7 +31,7 @@ protected ActionResult execute() {
                                     false, Const.ParamsNames.INSTRUCTOR_PERMISSION_MODIFY_SESSION);
 
         FeedbackQuestionAttributes feedbackQuestion = extractFeedbackQuestionData(instructorDetailForCourse.email);
-        List<String> questionDetailsErrors = feedbackQuestion.getQuestionDetails().validateQuestionDetails();
+        List<String> questionDetailsErrors = feedbackQuestion.getQuestionDetails().validateQuestionDetails(courseId);
 
         List<StatusMessage> questionDetailsErrorsMessages = new ArrayList<>();
 

src/main/java/teammates/ui/controller/InstructorFeedbackQuestionEditAction.java

@@ -85,8 +85,9 @@ private void editQuestion(FeedbackQuestionAttributes updatedQuestion) throws Inv
             isError = true;
         }
 
+        String courseId = getNonNullRequestParamValue(Const.ParamsNames.COURSE_ID);
         FeedbackQuestionDetails updatedQuestionDetails = updatedQuestion.getQuestionDetails();
-        List<String> questionDetailsErrors = updatedQuestionDetails.validateQuestionDetails();
+        List<String> questionDetailsErrors = updatedQuestionDetails.validateQuestionDetails(courseId);
         List<StatusMessage> questionDetailsErrorsMessages = new ArrayList<>();
 
         for (String error : questionDetailsErrors) {

src/main/resources/InstructorSampleData.json

@@ -420,7 +420,7 @@
       "courseId": "demo.course",
       "creatorEmail": "teammates.demo.instructor@demo.course",
       "questionMetaData": {
-        "value": "{\"msqChoices\":[\"C\",\"C++\",\"Java\",\"Python\"],\"questionText\":\"Which programming languages do you know?\",\"questionType\":\"MSQ\",\"numOfMsqChoices\":4,\"otherEnabled\":false}"
+        "value": "{\"msqChoices\":[\"C\",\"C++\",\"Java\",\"Python\"],\"questionText\":\"Which programming languages do you know?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"
       },
       "questionNumber": 6,
       "questionType": "MSQ",

src/test/java/teammates/test/cases/action/InstructorFeedbackQuestionEditActionTest.java

@@ -608,7 +608,8 @@ public void testExecuteAndPostProcessMsq() {
                 Const.ParamsNames.FEEDBACK_QUESTION_TYPE, "MSQ",
                 Const.ParamsNames.FEEDBACK_QUESTION_TEXT, "What do you like best about the class?",
                 Const.ParamsNames.FEEDBACK_QUESTION_DESCRIPTION, "more details",
-                Const.ParamsNames.FEEDBACK_QUESTION_NUMBEROFCHOICECREATED, Integer.toString(msqDetails.getNumOfMsqChoices()),
+                Const.ParamsNames.FEEDBACK_QUESTION_NUMBEROFCHOICECREATED,
+                    Integer.toString(msqDetails.getNumOfChoicesForMsq(fs.getCourseId(), FeedbackParticipantType.NONE)),
                 Const.ParamsNames.FEEDBACK_QUESTION_MSQCHOICE + "-0", msqDetails.getMsqChoices().get(0),
                 Const.ParamsNames.FEEDBACK_QUESTION_MSQCHOICE + "-1", msqDetails.getMsqChoices().get(1),
                 Const.ParamsNames.FEEDBACK_QUESTION_NUMBEROFENTITIESTYPE, "max",

src/test/resources/data/FeedbackSessionQuestionTypeTest.json

@@ -686,7 +686,7 @@
       "courseId": "FSQTT.idOfTypicalCourse1",
       "creatorEmail": "instructor1@course1.tmt",
       "questionMetaData": {
-        "value": "{\"msqChoices\":[\"It's good\",\"It's perfect\"],\"questionText\":\"What do you like best about our product?\",\"questionType\":\"MSQ\",\"numOfMsqChoices\":2,\"otherEnabled\":false}"
+        "value": "{\"msqChoices\":[\"It's good\",\"It's perfect\"],\"questionText\":\"What do you like best about our product?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"
       },
       "questionNumber": 1,
       "questionType": "MSQ",
@@ -708,7 +708,7 @@
       "courseId": "FSQTT.idOfTypicalCourse1",
       "creatorEmail": "instructor1@course1.tmt",
       "questionMetaData": {
-        "value": "{\"msqChoices\":[\"It's good\",\"It's perfect\"],\"numOfMsqChoices\":2,\"questionText\":\"What do you like best about the class' product?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"
+        "value": "{\"msqChoices\":[\"It's good\",\"It's perfect\"],\"questionText\":\"What do you like best about the class' product?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"
       },
       "questionNumber": 2,
       "questionType": "MSQ",
@@ -730,7 +730,7 @@
       "courseId": "FSQTT.idOfTypicalCourse1",
       "creatorEmail": "instructor1@course1.tmt",
       "questionMetaData": {
-        "value": "{\"msqChoices\":[\"Pizza\",\"Pasta\",\"Chicken rice\"],\"questionText\":\"Choose all the food you like\",\"questionType\":\"MSQ\",\"numOfMsqChoices\":3,\"otherEnabled\":true}"
+        "value": "{\"msqChoices\":[\"Pizza\",\"Pasta\",\"Chicken rice\"],\"questionText\":\"Choose all the food you like\",\"questionType\":\"MSQ\",\"otherEnabled\":true}"
       },
       "questionNumber": 3,
       "questionType": "MSQ",

src/test/resources/data/InstructorFeedbackResultsPageUiTest.json

@@ -701,7 +701,7 @@
       "courseId": "CFResultsUiT.CS2104",
       "creatorEmail": "CFResultsUiT.instr@gmail.tmt",
       "questionMetaData": {
-        "value": "{\"msqChoices\":[\"FlexiCommand\",\"PowerSearch\",\"GoodUI\",\"Google Integration\"],\"questionText\":\"What is your extra feature?\",\"questionType\":\"MSQ\",\"numOfMsqChoices\":4,\"otherEnabled\":false}"
+        "value": "{\"msqChoices\":[\"FlexiCommand\",\"PowerSearch\",\"GoodUI\",\"Google Integration\"],\"questionText\":\"What is your extra feature?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"
       },
       "questionNumber": 8,
       "questionType": "MSQ",