[#11466] Add remind sub-menu to UI

[NicolasCwy]

Fixes #11466

Outline of Solution

1. Add dropdown to Remind button

• "remind all non-submitters" - opens a modal and auto selects both students and instructors that have not submitted feedback session
• "select non-submitters to remind" - opens a modal with no selection made

2. Fixed bug where "select non-submitters to remind" would open the model with the "Select all instructors not yet submitted" option checked even if no instructor was checked (all instructors submitted form)
   

• Amended logic in send-reminders-to-respondents-modal.component.ts isAllYetToSubmitStudentsSelected and isAllYetToSubmitInstructorsSelected which is used to determine if "Select all instructors not yet submitted" and "Select all students not yet submitted" boxes are checked

3. Fixed E2E tests

• InstructorFeedbackSessionsPageE2ETest and InstructorHomePageE2ETest tests both reminder options. Due to the constraint of how many students/ instructors can use TestProperties.TEST_EMAIL, only one non-submitter student is verified to have received the reminder email.

[nusoss-bot]

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

[daongochieu2810]

@NicolasCwy Seems like CI is failing, let us know once you fix it

[NicolasCwy]

Still working on this, taking abit of a break! Will convert it into a PR when I'm ready for review

[nusoss-bot]

Guys, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢
Hope someone can get it to move forward again soon...

[samuelfangjw]

Hey! Took a quick look at your PR while you are fixing the e2e tests :)

[NicolasCwy]

Hi @daongochieu2810 I have finished the PR, would I need to create test for the bug I fixed (Number 2)? Thinking of adding tests to send-reminders-to-respondents-modal.component.spec.ts to test the isAllYetToSubmitStudentsSelected and isAllYetToSubmitInstructorsSelected

[samuelfangjw]

Implementation looks good to me! Just a small code quality related comment below.

[NicolasCwy]

@samuelfangjw I've tried to create an E2E test to check if all non-submitters have been notified when the reminder button has been clicked, but I'm not sure if there's something wrong with the BaseE2ETestCase.verifyEmailSent() as I am able to call it more times than there are emails sent out, and it would not fail although the implementation seems to search unread messages and mark the message with the subject as read.

[samuelfangjw]

@samuelfangjw I've tried to create an E2E test to check if all non-submitters have been notified when the reminder button has been clicked, but I'm not sure if there's something wrong with the BaseE2ETestCase.verifyEmailSent() as I am able to call it more times than there are emails sent out, and it would not fail although the implementation seems to search unread messages and mark the message with the subject as read.

Looking at the method, I believe it skips the check on the dev server.

[samuelfangjw]

I think it would be more appropriate to place this in the prepareTestData method, since this seems like preparing test data.

[NicolasCwy]

I thought so too, but saw that InstructorFeedbackReportPageE2ETest.java, was doing it in the classSetup(), which I thought was to keep the prepareTestData method lean, should I still move it?

[samuelfangjw]

Yes! For InstructorFeedbackReportPageE2ETest.java, in the classSetup() method, they are fetching data from the database but not modifying anything. In the prepareTestData method, they are modifying the testData that will be persisted into the database. In fact, there is a similar line of code in prepareTestData() that is almost identical to the changes you made.

studentToEmail = testData.students.get("Emily");
studentToEmail.setEmail(TestProperties.TEST_EMAIL);
removeAndRestoreDataBundle(testData);

[NicolasCwy]

Yea I saw this, I thought because the studentToEmail and the instructor are entities used in every test as compared to my test data which was used only in one test, I would have to differentiate them by placing them in a different method. Will make the changes!

[samuelfangjw]

@samuelfangjw The new E2E tests I've added does not check if the instructor (who is a non-submitter) would receive the email, only students are checked. This is because the instructor email is used to create sessions and I didn't know if there would be any problems changing the email after the creation of the session.

protected void prepareTestData() {
        testData = loadDataBundle("/InstructorFeedbackSessionsPageE2ETest.json");
        studentToEmail = testData.students.get("charlie.tmms@IFSess.CS1101");
        studentToEmail.setEmail(TestProperties.TEST_EMAIL);
        removeAndRestoreDataBundle(testData);
        ...
    }

I don't believe there will be any issue! Looking at the existing implementation of prepareTestData, probably can add another instructorToEmail if you wish to test it :)

[NicolasCwy]

@samuelfangjw The new E2E tests I've added does not check if the instructor (who is a non-submitter) would receive the email, only students are checked. This is because the instructor email is used to create sessions and I didn't know if there would be any problems changing the email after the creation of the session.

protected void prepareTestData() {
        testData = loadDataBundle("/InstructorFeedbackSessionsPageE2ETest.json");
        studentToEmail = testData.students.get("charlie.tmms@IFSess.CS1101");
        studentToEmail.setEmail(TestProperties.TEST_EMAIL);
        removeAndRestoreDataBundle(testData);
        ...
    }

I don't believe there will be any issue! Looking at the existing implementation of prepareTestData, probably can add another instructorToEmail if you wish to test it :)

I would be able to add another instructor to test that instructors are emailed. I think I'm concerned if omitting the instructor who is creating the session is ok?

[samuelfangjw]

@samuelfangjw The new E2E tests I've added does not check if the instructor (who is a non-submitter) would receive the email, only students are checked. This is because the instructor email is used to create sessions and I didn't know if there would be any problems changing the email after the creation of the session.

protected void prepareTestData() {
        testData = loadDataBundle("/InstructorFeedbackSessionsPageE2ETest.json");
        studentToEmail = testData.students.get("charlie.tmms@IFSess.CS1101");
        studentToEmail.setEmail(TestProperties.TEST_EMAIL);
        removeAndRestoreDataBundle(testData);
        ...
    }

I don't believe there will be any issue! Looking at the existing implementation of prepareTestData, probably can add another instructorToEmail if you wish to test it :)

I would be able to add another instructor to test that instructors are emailed. I think I'm concerned if omitting the instructor who is creating the session is ok?

I don't think it matters, will probably work. I believe instructor is associated to course by courseId and not email

[nusoss-bot]

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

[NicolasCwy]

@samuelfangjw As spoken since we are unable to set multiple entities to use TestProperties.TEST_EMAIL, because removeAndRestoreDataBundle persists entities and we are not able to persist entities. I would only be testing that remind all and remind selected non-submitters reminds one of the non-submitters

[samuelfangjw]

Looks good to me :)