Send Email Notification for Achievements #2214

drdrew42 · 2023-09-18T20:55:06Z

New Achievement Feature

This PR enables email notifications on a per-achievement basis.

Each achievement can be paired with an email template that will be filled out and sent to the student when they earn the achievement.

The email template itself is a hybrid of Mojo::Template and our own email template system (as implemented on the Email page). A default template (default.html.ep) must be present in the [TMPL]/achievements/ folder. This file is provided in courses.dist/achievements/

I have left the template as an HTML template, in case there is desire to support MIME::HTML email messages.

To enable an email notification:

the site.conf (or course.conf, at least) must have nonempty $mail{smtpSender} or $mail{set_return_path}
the default.html.ep must be copied from courses.dist/achievements/ into the [TMPL]/achievements folder (I'm hoping that Improvements to the course directory update method. #2173 will help ensure this happens)
achievements must be enabled in course config
import and assign default_achievements.axp (or whatever else you might have)
head to the Achievement List page and click "Enable Email Notification" (this should default to default.html.ep)
make modifications to the template or "use existing template" default.html.ep

Once enabled, the email notification can be disabled by clicking into "Edit Email Template" and selecting the "Disable Notifications" tab.

Caveat

This PR affects the webwork2-job-queue service, and as such you must restart both webwork2 and webwork2-job-queue when testing.

update copypasta fix template -> .html.ep fix url_for achievement notification editor

pass vars to template template variable updates sender fix linting

lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm

drgrice1

There is quite a bit of clean up needed. Most of this is the result of copying the AchievementEditor.pm file and its related files which also need a lot of clean up. Since these are new files, it would be good to add them already cleaned up.

lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm

lib/WeBWorK/ContentGenerator/Instructor/AchievementEditor.pm

lib/WeBWorK/ContentGenerator/Instructor/AchievementNotificationEditor.pm

templates/ContentGenerator/Instructor/AchievementList/default_table.html.ep

templates/ContentGenerator/Instructor/AchievementNotificationEditor/save_as_form.html.ep

lib/WeBWorK/ContentGenerator/Instructor/AchievementNotificationEditor.pm

drdrew42 · 2023-10-06T20:31:30Z

Okay, I think that all of the feedback has been resolved. In multiple places:

extra dereferencing arrows are removed
stash values are not duplicated on the controller
ui messages are passed through maketext

Also, cleanup of conditionals (e.g. not checking conditions that already returned early); removal of unused variables and modules; and related template cleanup.

drgrice1

There are a few more things I observed in the code. I also got to testing functionality, and I am seeing some warnings.

First, I am getting Use of uninitialized value $val in pattern match (m//) at /usr/share/perl5/Email/MIME/Encode.pm line 36. warnings anytime an email is sent by the send_achievement_email task. I am not sure what is causing that exactly. This does not happen with the send_instructor_email task.

courses.dist/modelCourse/templates/achievements/default.html.ep

lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm

lib/WeBWorK/ContentGenerator/Instructor/AchievementEditor.pm

lib/WeBWorK/AchievementEvaluator.pm

templates/ContentGenerator/Instructor/AchievementNotificationEditor/existing_form.html.ep

drgrice1

The achievement notification editor page needs a help file. It should be templates/HelpFiles/InstructorAchievementNotificationEditor.html.ep. One thing that is not immediately clear is how you actually enable email notifications, and what the "Use Existing Template" form is for. I see that the "Use Existing Template" form activates sending emails for the achievement, but a help file to explain that would help. Also, maybe it would be better if that were a select that shows the available templates to choose from.

templates/ContentGenerator/Instructor/AchievementList/default_table.html.ep

templates/ContentGenerator/Instructor/AchievementNotificationEditor.html.ep

templates/ContentGenerator/Instructor/AchievementList/default_table.html.ep

lib/WeBWorK/ContentGenerator/Instructor/AchievementNotificationEditor.pm

lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm

drgrice1 · 2023-11-04T10:57:11Z

lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm

+		$mail_data->{from} =
+			$ce->{mail}{smtpSender} || $ce->{mail}{set_return_path};


I don't think that either of $mail{smtpSender} or $mail{set_return_path} are really the right things to use for the from field of this email. In fact the $mail{smtpSender} is specifically documented in site.conf as NOT being the from address for emails and is typically the email address of a system administrator. The $mail{set_return_path} is supposed to only be a return path which is typically a noreply address, and some email servers might reject sending of emails with such an address. The instructor of the course should really be the sender. The problem is in finding that as there may be multiple instructors and such. The instructor email page deals with this by allowing the user to choose the sender. That is not exactly an option in this case.

Would it make sense to have a configuration variable for the from address on these messages?

I think that is needed. This could use a course setting for this.

lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm

drgrice1

This is looking pretty good now. One minor issue needs to be fixed.

lib/WeBWorK/AchievementEvaluator.pm

Co-authored-by: Glenn Rice <[email protected]>

somiaj · 2023-11-30T05:24:03Z

Overall seems like a nice feature. Some things I noticed playing around with it.

First, the Achievement Notification editor still needs a help page (previously mentioned by @drgrice1).

Do you think instead of the link just saying, "Edit Email Template", it could state which template is being used and edited, since you can use the same template for multiple achievements? Maybe "Edit Email Template default" (the name of the template minus the .html.ep suffix). This way it is clear from the editor which template is being used.

Also when selecting a template in the "Use Existing Templates" form, do you think using a drop down menu that lists all .html.ep files in the templates/achievements directory would be nice (so a user doesn't have to know them by name)?

The Save As form doesn't select an option by default, and one needs to be selected to work (though I guess this is the same behavior as the Evaluator Editor, so if you decide to provide a default, that should be adjusted as well). I was just comparing this to the problem editor which selects an option by default under Save As.

The "Save As" form selecting "use in new achievement" both seems weird and doesn't work. If you enter in an achievement name that exist you get an error, "Achievement ID exists! No new achievement created. File not saved.", but if you enter in a new name you get the error "Don't recognize saveMode: |use_in_new|. Unknown error.", so this doesn't appear to do much. I don't think creating a new achievement here is the correct thing to do (since the best you can do is attach a email template to it). Maybe instead this should be add to existing achievement, and a drop down menu with all the current achievements you can add the email template to.

When using the achievement notification editor, a sub menu item for the achievement appears, but isn't fully clear what it is for, maybe call it "name evaluator" to make it clearer that links is for the evaluator editor. Would adding one for the email template editor be helpful too (just a thought, this will probably be too clunky)?

Also, for those who don't know mojolicious the suffix html.ep might be a bit odd for them to understand. Would it be helpful to hide this from the user and append it as needed? I guess they can see this in the file manager though, so this might just add useful info for some.

drgrice1

Unfortunately, I must revoke my approval of this pull request. I started looking into things a little bit deeper after @somiaj's review. I realized I didn't test some things very well when I say his comments. Those things are not to bad. I did quite a bit of work to fix those, and was going to put in a pull request to this branch to fix those things and some other issues I encountered along the way. Then I found another problem, that can not be easily resolved. That is the usage of Mojo::Template to render templates that are created by instructors. That is a major security vulnerability, and unfortunately can't be allowed. Any perl code can be executed in these templates. Modules can be used, any files that the server has permissions to modify or delete can be deleted, etc. If the templates could be compiled in the safe zone, then perhaps this could be fixed, but I haven't found a way to do that yet. It most likely can not be securely done. So another approach for email templates is going to need to be found.

drgrice1 · 2023-12-01T23:33:14Z

I went ahead and put the work that I had done to address @somiaj's comments and some other things that I saw into a pull request to this branch. I will see what alternatives I can devise to using the Mojo::Template module.

somiaj · 2023-12-02T01:21:49Z

Emails don't need to include any html, you could use a template method similar to what is being done on the email page.

drgrice1 · 2023-12-02T01:28:01Z

Yes, that is obvious. But that is a far more limited method.

drgrice1 · 2023-12-02T04:07:59Z

@drdrew42: The pull request now has been switched to using the basic interpolation method of WeBWorK::Utils::processEmailMessage again, and then interpolates a few other variables specific to achievements. It no longer uses Mojolicious templates ... sigh. There may be other variables that you might want interpolated in as well though. If you merge that pull request, then this can pull request can proceed again. I am still probably going to see if I can find a way to get Mojolicious templates to work in the safe zone, but it doesn't look likely.

drgrice1 · 2023-12-02T21:00:40Z

@drdrew42: I got Mojolicious template compilation to work in the safe zone! I removed interpolation approach that I had added to the pull request to this branch, and switched it to using templates again with this approach. So if you merge that pull request into this one, then this is good to go again.

…t-notifications

@somiaj

The `$mail{achievementEmailFrom}` setting was not working because in the task it tried to use a non-existent `$mail{achievementEmailSender}` variable. Make it so that achievement notifications are only sent if `$mail{achievementEmailFrom}` is set, and do NOT fall back to using `$mail{smtpSender}` or `$$mail{set_return_path}`. Those are not valid for this in any way. Make the default template only remark on the number of points remaining until the next level-up if `$nextLevelPoints` is set. It is likely that a non-level achievement will occur before the first level achievement (for example if both of the one_click and level_one achievements are enabled, and the student gets the first question correct in one attempt) in which case the next level points has not been set yet. If that happens then the email says there are a negative number of points remaining until the next level-up. Add a help file for the achievement notification editor page. On the achievement list page, make the "Notifications" column say "Edit Email Template $templateName" if an email notification template is enabled for the achievement as requested by @somiaj. I would be happy with reverting this though as it can make this column rather wide. Remove all of the radio options in the "Save As" form, and just assume it is being saved for the current achievement. I don't really see the need for any of those options. Add the `[ACHEVDIR]` prefix to the "Save As" form, and add `dir => 'ltr'` to file names since those don't work right in right to left languages otherwise. In the site navigation when editing an achievement evaluator, an achievement notification, or the achievement users, show sub links for all of those editors. Change the `text_field` in the `existing_form.html.ep` to a `select_field` that shows all of the existing template files. Don't use the WeBWorK::Utils::processEmailMessage method. I see no reason for that. There is not actually any merge data, so in reality this is only used to get the user's first name and potentially other information from the user record. That can be passed directly to the template and used there, so that is done instead. The default template is modified accordingly. Lots of clean up of the AchievementNotificationEditor.pm file and the AchivievementNotification.pm file. Instead of storing achievement email notification templates in the course templates/achievements directory directly, store them in templates/achievements/notifications subdirectory. This directory is added to the list of directories in the `updateCourseDirectories` method of `WeBWorK::Utils::CourseIntegrityCheck` that can be copied from the modelCourse. This has a couple of advantages. First, most courses will not have this directory so if the modelCourse is updated, then this directory will be copied and get the default template when the course is upgraded from the admin course. Second, it separates the achievement evaluator files from the achievement notification templates.

…tment. To do so the WeBWorK::SafeTemplate module must be used. This module derives from the Mojo::Template module and overrides the _trap method. The Mojo::Template _wrap method adds "use Mojo::Base -strict" and "no warnings 'ambiguous'" to the code generated in this method. When that is called later it causes an error in the safe compartment because "require" is trapped. So instead the WeBWorK::SafeTemplate override method imports strict, warnings, and utf8 which is equivalent to calling "use Mojo::Base -strict". Calling "no warnings 'ambiguous'" prevents warnings from a lack of parentheses on a scalar call in the generated code. So if this package is used, those warnings need to be prevented in another way. That is done when the module is used in Mojolicious::WeBWorK::Tasks::AchievementNotification using a $SIG{__WARN__} handler that doesn't do anything if the ambiguous warning is encountered.

…-fixes Fixes and improvements for achievement notifications.

drgrice1

Looks good now!

drdrew42 added 4 commits September 10, 2023 21:42

initial draft

a9ff08f

fixing silly mistakes

9c29d9c

update copypasta fix template -> .html.ep fix url_for achievement notification editor

fix templates & perltidy

ea05e95

pass vars to template template variable updates sender fix linting

update default email template

6974fa3

pstaabp reviewed Sep 19, 2023

View reviewed changes

lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm Outdated Show resolved Hide resolved

drgrice1 reviewed Sep 29, 2023

View reviewed changes

cleanup based on pr feedback

170eac5

drdrew42 force-pushed the feature/achievement-notifications branch from bb29a44 to 170eac5 Compare October 6, 2023 20:20

drgrice1 reviewed Oct 12, 2023

View reviewed changes

drdrew42 force-pushed the feature/achievement-notifications branch from 4c84e4f to 1c8b7e8 Compare October 30, 2023 20:36

PR feedback

52140e3

drdrew42 force-pushed the feature/achievement-notifications branch from 1c8b7e8 to 52140e3 Compare October 30, 2023 20:39

Merge branch 'develop' into feature/achievement-notifications

6499e40

drgrice1 reviewed Nov 4, 2023

View reviewed changes

add config setting for email notifications and cleanup

e721fde

drdrew42 force-pushed the feature/achievement-notifications branch from 1ac54bf to e721fde Compare November 20, 2023 22:02

drgrice1 approved these changes Nov 21, 2023

View reviewed changes

lib/WeBWorK/AchievementEvaluator.pm Outdated Show resolved Hide resolved

Update lib/WeBWorK/AchievementEvaluator.pm

381bf3f

Co-authored-by: Glenn Rice <[email protected]>

drgrice1 requested changes Dec 1, 2023

View reviewed changes

drdrew42 and others added 4 commits December 4, 2023 21:01

Merge remote-tracking branch 'origin/develop' into feature/achievemen…

332cba7

…t-notifications

Merge pull request #6 from drgrice1/feature/achievement-notifications…

1a42cfb

…-fixes Fixes and improvements for achievement notifications.

pluralization

5ba4b9a

drgrice1 approved these changes Dec 4, 2023

View reviewed changes

pstaabp merged commit 8ef0c73 into openwebwork:develop Dec 4, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send Email Notification for Achievements #2214

Send Email Notification for Achievements #2214

drdrew42 commented Sep 18, 2023 •

edited

Loading

drgrice1 left a comment

drdrew42 commented Oct 6, 2023

drgrice1 left a comment

drgrice1 left a comment

drgrice1 Nov 4, 2023

dlglin Nov 6, 2023

drgrice1 Nov 6, 2023

drgrice1 left a comment

somiaj commented Nov 30, 2023

drgrice1 left a comment

drgrice1 commented Dec 1, 2023

somiaj commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 left a comment

		$mail_data->{from} =
		$ce->{mail}{smtpSender} \|\| $ce->{mail}{set_return_path};

Send Email Notification for Achievements #2214

Send Email Notification for Achievements #2214

Conversation

drdrew42 commented Sep 18, 2023 • edited Loading

New Achievement Feature

Caveat

drgrice1 left a comment

Choose a reason for hiding this comment

drdrew42 commented Oct 6, 2023

drgrice1 left a comment

Choose a reason for hiding this comment

drgrice1 left a comment

Choose a reason for hiding this comment

drgrice1 Nov 4, 2023

Choose a reason for hiding this comment

dlglin Nov 6, 2023

Choose a reason for hiding this comment

drgrice1 Nov 6, 2023

Choose a reason for hiding this comment

drgrice1 left a comment

Choose a reason for hiding this comment

somiaj commented Nov 30, 2023

drgrice1 left a comment

Choose a reason for hiding this comment

drgrice1 commented Dec 1, 2023

somiaj commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 commented Dec 2, 2023

drgrice1 left a comment

Choose a reason for hiding this comment

drdrew42 commented Sep 18, 2023 •

edited

Loading