enhanced subject line formatting options for email

[Alex-Jordan]

This is intended to improve feedback emails where the subject is like:

[WWfeedback] course:courseID user:alex.jordan set: prob: sec: rec:

where the set: prob: sec: rec: is empty. This would happen if the user triggers the email from the Assignments page where there is no setID or problemID. And if there is no section or recitation assigned to the sender.

Maybe there is a better way, but what this does is let the format string be like:

'[WWfeedback] course:%c user:%u%s{ set:}%s%p{ prob:}%p%x{ sec:}%x%r{ rec:}%r'

If something like %s{ set:} is in the format string, then the string " set:" will be in the subject line, but only if the setID is defined. So the above would make the subject line be:

[WWfeedback] course:courseID user:alex.jordan

if there is no section, recitation, set, or problem ID.

I believe the change here is backward compatible, so if anyone uses their own format string, nothing changes for them. As long as they were not doing something odd with braces.

[somiaj]

I personally find %s{ set:}%s a bit hard to parse, specifically when chained with other things. And if you are worried about breaking braces, lots of template languages use {{ to avoid that. What about something like.

[WWfeedback] course:%s user:%u{{ set:%s }}{{ prob:%p }}{{ sec:%x }}{{ rec:%r }}

Then have the parser only include stuff between {{...}} if any variables %? used inside are not empty.

[Alex-Jordan]

Yeah, that would be better. Should it display the content if any variables are nonempty, or if all of them are nonempty? I lean toward all of them must be nonempty to show that content. (It is unlikely anyone would ever use this construct with more than one variable in it, but just so I know what to try and code.)

[somiaj]

I don't have a preference. I just mentioned any because my thought was take anything between {{ .... }}, and do string replacements for the ... and then do empty replacements, and compare the output, if they are different include the text. Though unsure if other logic for testing. All could also work, as you said, currently doesn't seem reasonable to include more than one %? in each {{ ... }} block.

[Alex-Jordan]

OK, now this uses @somiaj's suggestion, except with single braces instead of double brace pairs. It seems unlikely (to me) that anyone has been using braces in their subject line formatting. But if that's a concern, we could use double braces. It would complicate the regex in some way I don't understand how to deal with for now though, if it has to match on groups that are delineated by character pairs instead of single characters.

Also, the sequence of things happening here is clunky and I have little doubt that @somiaj or @drgrice1 could do it more elegantly.

But to summarize, the default formatting string is now

[WWfeedback] course:%c user:%u{ set:%s}{ prob:%p}{ sec:%x}{ rec:%r}

and each of the brace-delineated blocks will only come through in the subject line if all variables inside them are nonempty.

[Alex-Jordan]

Some of the logic here that is meant to test true when something is undefined or empty will also test true if that something is 0. Are we trying to guard against that? Can a courseID, userID, setID, problemID, section, or recitation reasonably be "0"?

[somiaj]

Set ID zero is something that comes up that could be 0. If that is allowed might vary. I wondered if just making everything empty strings when undefined would make this easier. The no need to check if things are defined, just replace with the string that is there. Then only check if something is not the empty string when determining if it should be included. So something like 's' => $set ? $set->set_id : '', or 'p' => $problemID // ''

[somiaj]

Seems odd to me that the fallback here is not the same as the default, I wonder if it is worth changing that too.

[somiaj]

I think with the short size of subject lines it won't matter, but instead of using lots of regex, you could just manually loop through the characters once and replace things as needed. This requires more lines of code and extra logic to deal with various cases. Also you might as well move the function to a helper function in lib/WeBWorK/Utils.pm so you don't have to duplicate code. If you want to start with something, here is something I put together and it passed a few tests. Here zeros, 0, should be correctly included.

sub formatEmailSubject ($subject, $map) {
	my @characters =
		$subject
		? split(//, $subject)
		: split(//, '[WWfeedback] course:%c user:%u{ set:%s}{ prob:%p}{ sec:%x}{ rec:%r}');
	my $n_chars     = scalar(@characters);
	my $n           = 0;
	my $new_subject = '';
	while ($n < $n_chars) {
		if ($characters[$n] eq '%' && $n + 1 < $n_chars && exists $map->{ $characters[ $n + 1 ] }) {
			$new_subject .= $map->{ $characters[ $n + 1 ] } if defined $map->{ $characters[ $n + 1 ] };
			$n += 2;
		} elsif ($characters[$n] eq '{') {
			my $is_closed  = 0;
			my $insert_str = 1;
			my $tmp_str    = '';
			$n += 1;
			while ($n < $n_chars) {
				if ($characters[$n] eq '}') {
					$is_closed = 1;
					$n += 1;
					last;
				} elsif ($characters[$n] eq '%' && $n + 1 < $n_chars && exists $map->{ $characters[ $n + 1 ] }) {
					if (defined $map->{ $characters[ $n + 1 ] } && $map->{ $characters[ $n + 1 ] } ne '') {
						$tmp_str .= $map->{ $characters[ $n + 1 ] };
					} else {
						$insert_str = 0;
					}
					$n += 2;
				} else {
					$tmp_str .= $characters[$n];
					$n += 1;
				}
			}
			if ($is_closed) {
				$new_subject .= $tmp_str if $insert_str;
			} else {
				$new_subject .= "{$tmp_str";
			}
		} else {
			$new_subject .= $characters[$n];
			$n += 1;
		}
	}
	return $new_subject;
}

Then you just need something like

$subject = formatEmailSubject(
	$ce->{mail}{feedbackSubjectFormat},
	{
		c   => $courseID,
		u   => $user    ? $user->user_id       : undef,
		s   => $set     ? $set->set_id         : undef,
		p   => $problem ? $problem->problem_id : undef,
		x   => $user && $user->section    ? $user->section    : undef,
		r   => $user && $user->recitation ? $user->recitation : undef,
		'%' => '%',
	}
);

[drgrice1]

Regardless of if you switch to the mustaches syntax (the {{...}} template format @somiaj is suggesting) or not, I think it would be good to move this processing into a Utils file that can be called from both of the locations where it is needed. It would be okay to put it into lib/WeBWorK/Utils.pm for now.

Although there are several email utility methods building up in lib/WeBWorK/Utils.pm. It wouldn't hurt to start a lib/WeBWorK/Utils/Email.pm file and move those there with this method. In fact, I think the jitar_send_warning_email shouldn't be in lib/WeBWorK/Utils/ProblemProcessing.pm, and would fit better in an Email.pm utility file. This can be done at a later time though.

[Alex-Jordan]

This now moves the duplicate code from the two files that generate emails using the feedbackSubjectFormat string to Utils.pm. As @drgrice1 noted, some follow-up PR could move all the email utils to their own file.

For now I don't see a reason to use mustaches instead of single braces, but someone could make that argument.

I also don't see a reason to use a loop rather than regex. The loop code looks about four times as long, and not necessarily easier to read. Maybe for some, but not for me. What I mentioned before was that I mean I don't doubt even shorter regex could get the job done with less fuss than what I have, which uses regex about 6 times and uses an auxiliary hash to keep track of things.

[somiaj]

My only thought on the looping though the string once code might be more efficient than multiple regex, but for short subject strings it won't matter. I don't have a preference one way or another, just gave an alternative that came to mind.

If you want to support braces, you could add %{ and %} to mean just { and } in the output (similar to %%), though this might be a bit harder to deal with in your code due to things like %%{ set:%s}.

[Alex-Jordan]

I think this is ready to review again.