Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#9604] Add feature to regenerate student's session links #9610

Merged
merged 48 commits into from May 27, 2020

Conversation

amrut-prabhu
Copy link
Contributor

@amrut-prabhu amrut-prabhu commented Mar 25, 2019

Fixes #9604

Outline of Solution

20190413_05h03_40

Email is sent out to the student. Only links of sessions that have sent an opening email or published email are included in this email.

2019-04-13_07h22_37

Accessing old submission link:

2019-03-31_14h56_45

Accessing old results link:
2019-03-31_14h56_29


Email content:

Course join:

For Unregistered student, course join section is included
For Registered student, there is no course join content

Sessions:

The updated link for a session is included if isSentOpenEmail or isSentPublishedEmail is true (not isEmail*Enabled).

If no sessions match this criteria, the no session links found message is included instead.

@amrut-prabhu amrut-prabhu added the s.Ongoing The PR is being worked on by the author(s) label Mar 25, 2019
@damithc
Copy link
Contributor

damithc commented Mar 26, 2019

Usually, all links in a course (for a specific student) use the same key. When one link is compromised, all links are compromised. Are you generating links for all links in the course?

@amrut-prabhu
Copy link
Contributor Author

amrut-prabhu commented Mar 26, 2019

Usually, all links in a course (for a specific student) use the same key. When one link is compromised, all links are compromised. Are you generating links for all links in the course?

Yep, the Regenerate button appears for each course and will regenerate all the links for that course

@damithc Is it Usually the case, or all the time? If it's the former, can you expand on the non typical case?

@amrut-prabhu amrut-prabhu marked this pull request as ready for review March 31, 2019 07:40
@amrut-prabhu
Copy link
Contributor Author

@wenmogu Can you do an initial review of this before I proceed to add tests?

Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just glance the PR. I think it would be better to have one guy from API team to take a look. @ChooJeremy can you take a look at this?

* The API output format for the regenerate student's course links request.
*/
public class RegenerateStudentCourseLinksData extends ApiOutput {
private final String message;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need the message (which is like a status message)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the message that is shown in the snackbar (like in MessageOutput).

Copy link
Contributor

@ChooJeremy ChooJeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. I have tested your branch with feedback sessions of varying statues (not started and not visible, visible but not started, started (with and without account), ended) and have found no information leakage with the emails being sent.
Some notes:
This doesn't work if the other student creates a google ID before the reset, right? When I did that the email sent basically contains no information at all.
If the user is unregistered, the email sent should contain the feedback session links, right? Not just the course. My test on an open feedback session without signing up for a google ID sent an email like this:

expand to see email

INFO: TEAMMATESEMAILLOG|||1@1.com|||TEAMMATES: Updated links for course [someName][Course ID: someID]|||

Hello 1,

TEAMMATES has updated your links for the course [Course name: someName] [Course ID: someID]. Given below are the new links for you course registration and feedback sessions.

The course someName is using the TEAMMATES System to collect feedback.

To "join" the course, please go to this Web address: http://localhost:4200/web/join?key=815DE366BFC31E71408DD79220A29935925E15D39517E64971C1E6A97BAEF90D&studentemail=1%401.com&courseid=someID&entitytype=student

  • If prompted to log in, use your Google account to log in. If you do not have a Google account, please create one from the Google Accounts page.
  • The above link is unique to you. Please do not share it with your classmates.

Note that If you wish to access TEAMMATES without using your Google account, you do not need to �join� the course as instructed above. You will still be able to submit/view feedback by following the instructions sent to you by TEAMMATES at the appropriate times. However, we recommend joining the course using your Google account, because it gives you more convenient access to all your feedback stored in TEAMMATES.

For any non-technical queries (e.g. assigned to the wrong team, misspelled name, request for deadline extension, etc.), you can contact your instructor(s) at: sample (test@example.com).

Below are the new links to the feedback sessions of the course.

  • Requests for deadline extensions, problems regarding team/student data (e.g. wrong team, misspelled name), and other non-technical queries about the feedback session:
    Contact the instructors of the course: sample (test@example.com)
  • Technical help regarding TEAMMATES (e.g. submission link is not working): Email TEAMMATES support team at app_admin@gmail.com.

Regards,
TEAMMATES Team.

I am unsure about the changes in terms of design - seems a bit hacky in terms of piggybacking off currently existing structures & methods, but as I'm not too familiar with that part of the code bases I'll leave that to the others.

I'll wait for your tests for the new endpoint.

@amrut-prabhu amrut-prabhu force-pushed the 9604-regenerate-link branch 2 times, most recently from 7f69ec5 to 55d5409 Compare April 12, 2019 23:18
@amrut-prabhu amrut-prabhu added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Apr 13, 2019
@amrut-prabhu
Copy link
Contributor Author

Ready for review. Please read the PR description for some details about the expected behaviour.

@amrut-prabhu amrut-prabhu force-pushed the 9604-regenerate-link branch 2 times, most recently from 1dece33 to 33b76ab Compare April 13, 2019 15:48
@amrut-prabhu
Copy link
Contributor Author

@wenmogu Requesting your review :)

src/main/java/teammates/common/util/Const.java Outdated Show resolved Hide resolved
src/main/java/teammates/common/util/Const.java Outdated Show resolved Hide resolved
src/main/java/teammates/logic/api/Logic.java Outdated Show resolved Hide resolved
src/main/java/teammates/common/util/Templates.java Outdated Show resolved Hide resolved
src/main/java/teammates/logic/core/StudentsLogic.java Outdated Show resolved Hide resolved
@amrut-prabhu amrut-prabhu force-pushed the 9604-regenerate-link branch 3 times, most recently from c9afd2e to a0385e5 Compare April 16, 2019 17:42
@amrut-prabhu
Copy link
Contributor Author

@xpdavid Requested changes have been made

Copy link
Contributor

@ChooJeremy ChooJeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I have re-checked the previous test cases, this time injecting in

logic.updateFeedbackSession(
        FeedbackSessionAttributes
                .updateOptionsBuilder(session.getFeedbackSessionName(), session.getCourseId())
                .withSentOpenEmail(true)
                .build());

to force the database to update the sent email attributes so that the correct URLs are properly sent. I can confirm that for both cases mentioned above, emails are properly sent now. This doesn't fix the issue for if the other user has already signed up with an account, but in that case the Admin can always reset the Google ID.

Small nits:

Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@amrut-prabhu
Copy link
Contributor Author

@xpdavid Added to StudentProfileTest and made other requested changes. Can you take a look?

Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in passing.

amrut-prabhu and others added 6 commits May 19, 2020 23:31
)

Co-authored-by: Ahmed Bahajjaj <Ahmed_Bahajjaj@u.nus.edu>
…0074)

* added html tags for rank questions

* added help data

* updated default help data

* fixed identation and missing semicolon

Co-authored-by: Ahmed Bahajjaj <Ahmed_Bahajjaj@u.nus.edu>
@madanalogy madanalogy added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels May 20, 2020
@madanalogy madanalogy requested review from wkurniawan07 and removed request for wkurniawan07 May 20, 2020 01:14
@damithc
Copy link
Contributor

damithc commented May 20, 2020

thanks for resuming this PR @amrut-prabhu Let's try to get it merged as it would be a pity to let your good work go to a waste.

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amrut-prabhu thanks for not giving up on this PR! I have some questions.

src/main/java/teammates/logic/core/StudentsLogic.java Outdated Show resolved Hide resolved
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than conflict resolution and the minor suggested change, all are good. Thanks for seeing through this PR to completion! 💯

You can merge the PR yourself after all checks pass; I trust you still remember how to?

src/main/java/teammates/storage/api/StudentsDb.java Outdated Show resolved Hide resolved
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels May 27, 2020
@amrut-prabhu
Copy link
Contributor Author

Great!

Btw I made a minor change- Moved the http request from CourseService to StudentService (since it contains other such methods for a student in a course).

I'll merge after checks pass!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add feature to regenerate student's session links
9 participants