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.

[drgrice1]

This should be

Suggested change

	$set ? $set->set_id : '',
	$problem ? $problem->problem_id : '',
	$setID, $problemID,

The variables $set and $problem are not defined here.

[drgrice1]

I am seeing warnings from this line if the $mail{feedbackSubjectFormat} string is used with this code. These warnings will occur for any string that doesn't have any of these brace pairs in it. The issue is that if a string doesn't have any of the brace pairs, then the %braces hash has no key/value pairs, and so the $regex will be the regex that matches the empty string string. Since that matches the empty string between every successive pair of characters in the $formatString and $braces{''} is undefined, there are warnings. One fix is to change this line to

Suggested change

	my $subject = $formatString =~ s/($regex)/$braces{$1}/gr;
	my $subject = $formatString =~ s!($regex)!$braces{$1} // ''!egr;

However, that is not the most efficient, because that regular expression will replace the empty string between every successive pair of characters with the empty string. It would probably be better to restructure the code a bit to skip this regular expression substitution if the %braces hash is empty.

[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}.

[somiaj]

Any reason you can't just leave the outer braces out from the start to avoid having to removing them later? Removing the braces from the grouping seems to work in my testing.

	my @braces = $formatString =~ /\{((?:[^{}]*|(?0))*)\}/xg;

[Alex-Jordan]

Suppose the string were something dumb like %x taco{%x taco} and there is no value for %x. If I immediately drop the braces, the array has %x taco. Later we will end up substituting away both instances of %x taco. Unless of course we look for braces as well at the place where that substitution happens. So it seemed cleaner to me to not have to remember to bring back the braces later.

[somiaj]

I see, I missed that you were using the braces to know what to replace in the original string.

Though I still think you can remove this line, and include this above. We know that we just need to remove the first and last character, and only in the case that we actually include the content. So I think line 402 could be (haven't tested).

%braces = map { $_ => $braces{$_} =~ /%[$chars]/ ? '' : substr($braces{$_}, 1, -1) } keys %braces;

[somiaj]

Also looking at this, I know it is inconsistent in the codebase, but I think using sentences (capitalization and periods) on the comments is easier to read.

[somiaj]

Shouldn't this be more declarative now?

sub formatEmailSubject ($formatString, $courseID, $userID, $setID, $problemID, $section, $recitation) {

[drgrice1]

Yes. Use signatures.

[Alex-Jordan]

I think this is ready to review again.