Emailfrom #63
Emailfrom #63
Conversation
|
@danmarsden Dan you seem like the most active maintainer of this plugin so I thought I'd flag you here. Let me know if you're interested in making this part of the Reengagement plugin. |
lib.php
Outdated
| $context = context_course::instance($reengagement->course); | ||
| if ($userid == 1) { // Default teacher, get first teacher in course. | ||
| global $DB; | ||
| $params = array('roleid' => 3, 'contextid' => $context->id); |
There was a problem hiding this comment.
hard-coded roleid - this should be a site-level setting for re-engagement that allows the admin to select a role that is used for teaching staff or better still, use a has_capability check instead.
There was a problem hiding this comment.
in fact - it would be better to use the get_course_contacts() style handling here with the $CFG->coursecontact settings instead of hard-coding this to a specific role.
There was a problem hiding this comment.
changed this to get_enrolled_users() with a permissions check for 'mod/reengagement:addinstance'
lib.php
Outdated
| $userid = $reengagement->emailfrom; | ||
| if ($userid > 0) { | ||
| $context = context_course::instance($reengagement->course); | ||
| if ($userid == 1) { // Default teacher, get first teacher in course. |
There was a problem hiding this comment.
better to use something like is siteadmin() style code - userid ===1 might not always be the siteadmin.
There was a problem hiding this comment.
This should be the variable REENGAGEMENT_EMAILFROM_TEACHER, but I take your point that the user with id 1 may not be the siteadmin, so I'm changing the value for default teacher to -1, 0 is still the support user, and then any integer above 0 will be the actual user.
lang/en/reengagement.php
Outdated
| @@ -60,6 +60,10 @@ | |||
| $string['emailcontentmanagerdefaultvalue'] = 'This is a reminder notification from course %courseshortname%, regarding user %userfirstname% %userlastname%.'; | |||
| $string['emaildelay'] = 'Notification delay'; | |||
| $string['emaildelay_help'] = 'When module is set to notify users "after delay", this setting controls how long the delay is.'; | |||
| $string['emailfrom'] = 'Send notification from'; | |||
| $string['emailfrom_help'] = 'The user to send the message from, pending permissions check. Default teacher is the first teacher in the course, will send from support user if no teachers are found.'; | |||
| $string['emailfromsupport'] = 'Support User'; | |||
There was a problem hiding this comment.
Moodle uses sentance case.
Support User -> Support user.
Default Teacher -> Default teacher
db/install.xml
Outdated
| @@ -27,6 +27,8 @@ | |||
| <FIELD NAME="remindercount" TYPE="int" LENGTH="3" NOTNULL="true" DEFAULT="1" SEQUENCE="false" COMMENT="how many times to send the email"/> | |||
| <FIELD NAME="suppresstarget" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="ID of module that, if completed, should prevent an email being sent out."/> | |||
| <FIELD NAME="emaildelay" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="604800" SEQUENCE="false" COMMENT="Time setting related to email"/> | |||
| <FIELD NAME="emailfrom" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="ID of user to use as sender, 0 for support user, 1 for default teacher."/> | |||
There was a problem hiding this comment.
it makes me nervous to see xmldb files changed that don't include a version bump, it makes me wonder if this is hand-rolled or if you have used xmldb editor - can you please make sure you have used xmldb editor to add these fields to the install.xml file?
There was a problem hiding this comment.
Hi Dan,
I used XMLDB editor but my dev site is not connected to the github repo, so I had to copy and paste the updated file from there to here. I don't have a lot of experience with the XMLDB editor so I'm not sure how to do it in a better way, hope that clears things up?
|
looks like this has sat here for quite a while! - sorry for the delay in getting back to this, but I've just posted a few comments in-line - in general this seems like a good idea, just the implementation with hard-coded roleids that would need to be fixed before we could merge this. |
changed value to -1 and used in reengagement_get_emailfrom function
using -1 for default teacher since userid 1 could in theory be a valid teacher
instead of looking for the specific teacher role, look for all users enrolled in the course with the capability to add a reengagement activity
patching several issues
|
@danmarsden it took me a while to circle back to this, but I think I've addressed all of the issues with the original pull, let me know what you think |
Adds an option to specify a user to use as the sender of the notification. This can be any user in the course with the capability to add a Reengagement activity, or it can be set to a "Default Teacher" if a course template is being made for multiple teachers to use. The emailfrom user's name, email, and id can also be used as template variables. If a default teacher can't be found in the course at the time the message is sent, or if a specific user selected has had the capability removed, then messages will still default to sending from the support user.
Also adds an option to send notifications as an instant message instead of an email. Both the emailfrom and the instantmessage option can be used together, or one of the other can be used independently.
This change adds new database fields so it requires a version update.