enhanced subject line formatting options for email #2866
Conversation
|
I personally find Then have the parser only include stuff between |
|
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.) |
|
I don't have a preference. I just mentioned any because my thought was take anything between |
Alex-Jordan
force-pushed
the
email-subject-format
branch
from
114dd2c to
5327dbf
Compare
|
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 and each of the brace-delineated blocks will only come through in the subject line if all variables inside them are nonempty. |
|
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"? |
|
Set ID zero is something that comes up that could be |
| '%' => '%', | ||
| ); | ||
| my $chars = join('', keys %subject_map); | ||
| my $subject = $ce->{mail}{feedbackSubjectFormat} || 'WeBWorK question from %c: %u set %s/prob %p'; |
There was a problem hiding this comment.
Seems odd to me that the fallback here is not the same as the default, I wonder if it is worth changing that too.
|
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 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,
'%' => '%',
}
); |
|
Regardless of if you switch to the mustaches syntax (the Although there are several email utility methods building up in |
Alex-Jordan
force-pushed
the
email-subject-format
branch
from
5327dbf to
b108d27
Compare
|
This now moves the duplicate code from the two files that generate emails using the 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. |
|
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 |
Alex-Jordan
force-pushed
the
email-subject-format
branch
from
b108d27 to
655833f
Compare
|
I think this is ready to review again. |
Co-authored-by: Jaimos Skriletz <jaimosskriletz@gmail.com>
| my $chars = join('', keys %subject_map); | ||
| my $subject = $formatString; | ||
| # extract the brace pairs | ||
| my @braces = $formatString =~ /(\{(?:[^{}]*|(?0))*\})/xg; |
There was a problem hiding this comment.
I don't think that the recursive sub-pattern in this is right. I think it should be removed, and this should be
| my @braces = $formatString =~ /(\{(?:[^{}]*|(?0))*\})/xg; | |
| my @braces = $formatString =~ /(\{(?:[^{}]*)*\})/xg; |
The problem with the recursive sub-pattern is that it matches balanced braces which means it will include matched braces inside a matched brace pair. At first you might think that is correct, but it really isn't. For example, consider the $mail{feedbackSubectFormat} that contains [WWfeedback] course:%c user:%u{{ set:%s}}{ prob:%p}{ sec:%x}{ rec:%r}. With that subject format the recursive sub-pattern makes @braces contain {{ set:%s}}, {prob:%p}, {sec:%x}, and {$rec:%r}. Then on line 406 $regex is {{ set:%s}}|{ rec:%r}|{ prob:%p}|{ sec:%x} and that causes the warning Unescaped left brace in regex is passed through in regex.
If you remove the recursive sub-pattern, then your current value for $mail{feedbackSubjectFormat} still works, and so does the modified value I gave in the example above.
In any case, there should never be any warnings regardless of what the $mail{feedbackSubjectFormat} is set to since it is string that can be set by a user (the instructor of the course in this case).
There was a problem hiding this comment.
Another option if you really think the recursive sub-pattern is necessary is to change line 404 to my $regex = join('|', map { $_ =~ s/([{}])/\\$1/gr } keys %braces);. I.e., escape the braces for the regex.
There was a problem hiding this comment.
Hmm. I am seeing other warnings in certain cases as well. If a brace pair contains some other character that needs to be escaped in a regular expression, then there are still warnings. For example, if the $mail{feedbackSubjectFormat} contains something like { [set:$s}. That is not very well formed, but could happen, and should not result in warnings.
There was a problem hiding this comment.
So here is the best code that I have found so far. It keeps the sub-pattern recursion and I haven't been able to get warnings from anything so far.
my @braces = $formatString =~ /(\{(?:[^{}]*|(?0))*\})/xg;
if (@braces) {
# for each brace pair, do substitutions, but leave %c etc when variable is empty
my %braces = map { $_ => $_ =~ s/%([$chars])/$subject_map{$1} ne '' ? $subject_map{$1} : "%$1"/egr } @braces;
# If there is an instance of %c, etc, nullify the whole thing. Remove outer braces.
%braces = map { $_ => $braces{$_} =~ /%[$chars]/ ? '' : substr($braces{$_}, 1, -1) } keys %braces;
my $regex = join('|', map { $_ =~ s/([{}])/\\$1/gr } keys %braces);
eval {
$regex = qr/$regex/;
$subject =~ s/($regex)/$braces{$1}/g;
};
}
$subject =~ s/%([$chars])/$subject_map{$1} ne '' ? $subject_map{$1} : ''/eg;The eval prevents the warnings. Although, it also prevents the brace substitutions from happening if there is something that causes issues with creating the regular expression.
There was a problem hiding this comment.
Note I did see a reason to keep the recursive sub-pattern. If the subject template contains {{ rec:%r}} and the recitation is not set, then that entire thing is removed from the end result with the recursive sub-pattern. However, the outer braces is kept without the recursive sub-pattern.
There was a problem hiding this comment.
You could add brackets to the escaping substitution if you want to (so my $regex = join('|', map { $_ =~ s/([{}[\]])/\\$1/gr } keys %braces);) if you want to make bad brackets work. However, the eval is still needed, because there are lots of other cases where something in the user defined $mai{feedbackSubjectFormat} will cause issues.
There was a problem hiding this comment.
Ah-hah. Even better. Use
my @braces = $formatString =~ /(\{(?:[^{}]*|(?0))*\})/xg;
if (@braces) {
# for each brace pair, do substitutions, but leave %c etc when variable is empty
my %braces = map { $_ => $_ =~ s/%([$chars])/$subject_map{$1} ne '' ? $subject_map{$1} : "%$1"/egr } @braces;
# If there is an instance of %c, etc, nullify the whole thing. Remove outer braces.
%braces = map { $_ => $braces{$_} =~ /%[$chars]/ ? '' : substr($braces{$_}, 1, -1) } keys %braces;
my $regex = join('|', map {"\Q$_\E"} keys %braces);
$regex = qr/$regex/;
$subject =~ s/($regex)/$braces{$1}/g;
}I remembered the \Q...\E construct. That says to disable pattern meta chars for anything inside.
3 participants
This is intended to improve feedback emails where the subject is like:
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:
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: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.