TEAMMATES
Instructor copy session: support edit-before-save
Current: When an instructor copies a session, start/end/publish dates are copied as is. The user has to change those dates via a subsequent edit operation.
The problem: For the brief period, the session is saved with possibly wrong dates. On a rare case, the hourly email generation scripts can run during that period, generating automated emails incorrectly. Happened once recently, sending 'session published' emails for a session incorrectly.
Suggested: When the user clicks the copy button, open a modal containing the full details of the session (i.e., same as the form used when editing an existing session) and allow the user to edit any field, before saving the session. This way, the session will not exist with incorrect dates, even for a brief period.
Complication: How to deal with this case when copying to multiple courses?
The problem: For the brief period, the session is saved with possibly wrong dates. On a rare case, the hourly email generation scripts can run during that period, generating automated emails incorrectly. Happened once recently, sending 'session published' emails for a session incorrectly.
This happened again today. We might have to do a patch first before implementing a proper solution. In this case, I think the 'published emails sent' flag should be copied as-is, to prevent the 'results published' email being sent again. When the instructor changes the publishing date, the flag can be reset.
@damithc One proposal for this is to:
- Set
submissionStartTimestampandsubmissionStartTimestampto be yesterday's date (or a date in the past): - Set
isClosingEmailEnabledandisPublishedEmailEnabledto be false
https://github.com/TEAMMATES/teammates/blob/3801819753e9c69b7ed3e3c0282cab73a9a4c517/src/web/app/pages-instructor/instructor-session-base-page.component.ts#L52-L67
- Print a message to user in all our copy modals (such as the below) with a message like "Dates have not been sent and emails are disabled by default. Please edit session after copying to update those)"
Do you think that works?
In this case, I think the 'published emails sent' flag should be copied as-is, to prevent the 'results published' email being sent again. When the instructor changes the publishing date, the flag can be reset.
Isn't this simpler? If the email was already sent out in the original session, the flag must be set accordingly in the original to prevent resending the email again. We just need to replicate the same flag in the copy.
Isn't this simpler? If the email was already sent out in the original session, the flag must be set accordingly in the original to prevent resending the email again. We just need to replicate the same flag in the copy.
The issue with this is that we do not expose this flag in the API. This flag is only updated by the email action workers. We have to change the CreateFeedbackSessionAction to support this. Plus, we also have to change this flag somehow when the published date is changed (which probably involves an API change too). I don't think this is a simple change unless I am missing something.
The above proposal is much easier - just change existing function (the one I pasted) and update UI.
The above proposal is much easier - just change existing function (the one I pasted) and update UI.
Well, I hesitate to change fields arbitrarily. What if the user wanted to keep them as they are? It reduces the value of copying if the user has to go and change things back to original values again. Feels like shifting our work to the user. In that case, perhaps we can go for the proper solution, which is to show the full 'session edit' form in a follow up modal and let the user change anything she wishes before saving the session.
Yes true, it is definitely abit of a hack to change fields arbitrarily.
For the proper solution, we may have to change the feature a little. Right now, we allow for one session to be copied to multiple courses. Do we want to make it one-to-one instead? You have to copy one at a time but you will get a form modal with the details.
For the proper solution, we may have to change the feature a little. Right now, we allow for one session to be copied to multiple courses. Do we want to make it one-to-one instead? You have to copy one at a time but you will get a form modal with the details.
Perhaps whatever the changes user specifies in the second modal can be copied to all target courses? We don't allow specifying different values for different copies.