Instructor: view results: fix IndexOutOfBoundsException #8415

[Haozhe321]

Fixes #8415

Outline of Solution

• Changed the offending lines. Now the msqChoices list will be used to determine the logic for the loop so there will not be a discrepancy between the number of choices and the choices themselves.
• Modified tests to replicate the error before the fix, which passed after the fix.

@craaaa

[whipermr5]

Can we completely remove numOfMsqChoices?

[whipermr5]

for (String msqChoice : msqChoices.size() {

[whipermr5]

If we're not removing numOfMsqChoices, then this needs to be fixed:

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

Lines 120 to 128 in cd9a536

	private void setMsqQuestionDetails(FeedbackParticipantType generateOptionsFor, String courseId) {
	this.msqChoices = new ArrayList<>();
	this.otherEnabled = false;
	this.generateOptionsFor = generateOptionsFor;
	if (generateOptionsFor.equals(FeedbackParticipantType.STUDENTS_EXCLUDING_SELF)) {
	this.numOfMsqChoices = generateNumOfChoicesForStudentsExcludingSelf(courseId);
	} else {
	this.numOfMsqChoices = generateOptionList(courseId).size();
	}

Here, numOfMsqChoices is set to its generated, non-zero value solely for the purpose of min/max options validation in validateQuestionDetails (I think - source: 48f9b24). However, the non-zero value should not be persisted to the datastore, as this makes it inconsistent with msqChoices, which is an empty list when persisted to the datastore. Note: both msqChoices and numOfMsqChoices are stored as a JSON string in the questionText field of the FeedbackQuestion entity (the field is named questionMetaData in FeedbackQuestionDetails).

If we are removing numOfMsqChoices, data migration might be needed.

[whipermr5]

Actually I feel that for clarity, this whole block should be an else-if of the if statement just above. It should only run for non-generated options.

[Haozhe321]

2 possible ways to go from here:

1. Remove numOfMsqChoices. This should be fine for future additions as we can rely on msqChoices.size(). The only exception is the method validateQuestionDetails() that you mentioned, since now we have no ways to check for the violation condition below for msq with generated options:

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

Lines 598 to 599 in cd9a536

	if (numOfMsqChoices < maxSelectableChoices) {
	errors.add(Const.FeedbackQuestion.MSQ_ERROR_MAX_SELECTABLE_EXCEEDED_TOTAL);

For data migration, I'm not exactly sure of the consequences of that, or how much work it could take. @whipermr5 do you have any recommendations on this?

2. Do not remove numOfMsqChoices. In this case we have to make sure that numOfMsqChoices and msqChoices are consistent in the database. This isn't that big a problem before Instructor: edit session: multiple choice questions: support generating options for all other students #8412 #8445 and Instructor: edit session: multiple choice questions: support generating options for all other teams #8661 #8689 since we can just store studentlist/teamList/instructorList as the choices; however, after these 2 commits, students will have different choices for their answers so there is no easy way to decide what should be stored in mcqChoices.
   What we can do in this case is to store the entire studentList/teamList/instructorList in mcqChoices, and set numOfMsqChoices to the size of the list. This should be fine since we use generateOptionsFor to differentiate between a normal msq and a msq with generated options. So for methods like validateQuestionDetails(), we can check for generateOptionsFor first and -1 for questions with generateOptionsFor when checking for maxSelectableChoices violation. For getQuestionAdditionalInfoHtml(), we can also check generateOptionsFor to determine what goes inside the additional info box.

I think numOfMsqChoices can remain without causing more problems.

[Haozhe321]

Also, just curious if there is any tangible negative impact of the inconsistency in the database? Because if we manage it well, it seems fine (or maybe not haha but I'm interested to know the reason).

[Haozhe321]

I tried the approach of storing the entire studentList/teamList/instructorList in mcqChoices, and the result is not ideal. Here's an example:

I created a msq with generated options and saved the question. But as you can see from the ss below, the choices will be populated with the entire class list, and that's not what we want. I guess this is why the mcqChoices list was set to empty and this answer Bao's question.

Since this way doesn't seem to work, and we need a field to store the number of msqChoices for validateQuestionDetails(), we can create a new field to store the number of msq choices solely for generated options(numOfMsqChoices will be set to 0 for generated options to be consistent with the empty mcqChoices list). @whipermr5 what do you think?

[whipermr5]

I think we can keep numOfMsqChoices for that purpose, but make it a transient field so it doesn't get stored in JSON.

[Haozhe321]

@whipermr5 But if we make numOfMsqChoices a transient field, doesn't that mean we can't use it for its purpose since every time a question is deserialized from the database, numOfMsqChoices is set to 0?

[whipermr5]

@Haozhe321 I believe validation is only supposed to be done before a question is persisted to the datastore? Anyway auto-generated options are never supposed to be persisted; they should be generated when accessed as the options can change (e.g. if the student list changes).

[Haozhe321]

Validation is also done when editing a question i.e. changing the maximum/minimum number of choices, which requires retrieving information of the question from the datastore.

Anyway auto-generated options are never supposed to be persisted; they should be generated when accessed as the options can change (e.g. if the student list changes).

I see, that makes sense. Then another way to validate is to use generateNumOfChoicesForStudentsExcludingSelf() when validating, instead of using msqChoices. I guess we don't have to use numOfMsqChoices then. I'll make some changes(i.e. setting numOfMsqChoices to transient) and see how that works.

[crphang]

I think this issue is described in #8646, where we cant have meaningful backend validation for questions (for responses too).

[Haozhe321]

@whipermr5 here are the changes made:

1. made numOfMsqChoices a transient field
2. changed the parameter for validateQuestionDetails() so that it can check the courseId to generate the size of the generated options.
3. replaced instances of numOfMsqChoices with msqChoices.size()
4. Removed methods generateNumOfChoicesForStudentsExcludingSelf() and generateNumOfChoicesForTeamsExcludingSelf(), replacing them with numOfChoicesForMsq() to generated number of choices for every type of option.

So now the field numOfMsqChoices is not used anywhere. That's the first error for PMD. I'm not too sure if removing it is a good idea because questions before this patch would have that field. But as of now, this field is not needed for future questions.

The 2nd error that PMD flagged out is "Confusing Ternary", which should be skipped when confusing else if clauses according to here and here.

[whipermr5]

Lint errors

[Haozhe321]

@whipermr5 I'm aware of the lint errors. Need your opinions on them:

1. Should I remove numOfMsqChoices completely? this is flagged out in PMD as "Avoid unused private fields".
2. Should I create a new issue to add in the exclusion rule for Confusing Tenary when dealing with else if?

Other than the lint errors, the CI tests have passed on my local machine.

[whipermr5]

@Haozhe321 Oops didn't read your previous comment carefully 😅 You can always add suppression annotations for now.

[wkurniawan07]

@Haozhe321 @whipermr5 I'm currently looking at re-organizing the PMD rules. Consider the request for adding ignoreElseIf property in ConfusingTernary heard.

[Haozhe321]

@whipermr5 fixed the style issues.

[whipermr5]

Suppress the specific rule and add a comment? E.g.

teammates/src/main/java/teammates/storage/entity/FeedbackSession.java

Lines 28 to 32 in e32eeff

	// PMD.UnusedPrivateField and SingularField are suppressed
	// as feedbackSessionId is persisted to the database
	@SuppressWarnings({"PMD.UnusedPrivateField", "PMD.SingularField"})
	@Id
	private transient String feedbackSessionId;

[Haozhe321]

done!

[Haozhe321]

@whipermr5 changes made!

[whipermr5]

Do it for the whole method - there should be no else ifs.

[Haozhe321]

I think I misunderstood what you meant. Do you mean the following?

    public int getNumOfChoicesForMsq(String courseId, FeedbackParticipantType generateOptionsFor) {
        if (generateOptionsFor == FeedbackParticipantType.NONE) {
            return msqChoices.size();
        }

        if (generateOptionsFor == FeedbackParticipantType.STUDENTS
                || generateOptionsFor == FeedbackParticipantType.STUDENTS_EXCLUDING_SELF) {
            List<StudentAttributes> studentList = StudentsLogic.inst().getStudentsForCourse(courseId);
            int sizeOfStudentlist = studentList.size();

            return generateOptionsFor == FeedbackParticipantType.STUDENTS ? sizeOfStudentlist : sizeOfStudentlist - 1;
        }

        if (generateOptionsFor == FeedbackParticipantType.TEAMS
                || generateOptionsFor == FeedbackParticipantType.TEAMS_EXCLUDING_SELF) {
            try {
                List<TeamDetailsBundle> teamList = CoursesLogic.inst().getTeamsForCourse(courseId);
                int sizeOfTeamlist = teamList.size();

                return generateOptionsFor == FeedbackParticipantType.TEAMS ? sizeOfTeamlist : sizeOfTeamlist - 1;
            } catch (EntityDoesNotExistException e) {
                Assumption.fail("Course disappeared");
            }
        }
        List<InstructorAttributes> instructorList = InstructorsLogic.inst().getInstructorsForCourse(courseId);

        return instructorList.size();
    }

[whipermr5]

Yup!

[Haozhe321]

I've push up the changes. What's the difference between this version and the previous version?

[whipermr5]

👍 It just looks slightly neater I think

[whipermr5]

LGTM 👍