Skip to content

enhanced subject line formatting options for email #2866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Alex-Jordan wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

enhanced subject line formatting options for email #2866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
655833f
enhanced subject line formatting options for email
Alex-Jordan Dec 22, 2025
008dbb3
consolidate code
Alex-Jordan Dec 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conf/defaults.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ $mail{feedbackRecipients} = [
# %% = literal percent sign
#

$mail{feedbackSubjectFormat} = "[WWfeedback] course:%c user:%u set:%s prob:%p sec:%x rec:%r";
$mail{feedbackSubjectFormat} = '[WWfeedback] course:%c user:%u{ set:%s}{ prob:%p}{ sec:%x}{ rec:%r}';

# feedbackVerbosity:
# 0: send only the feedback comment and context link
Expand Down
12 changes: 7 additions & 5 deletions lib/WeBWorK/ConfigValues.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,13 +829,15 @@ sub getConfigValues ($ce) {
x('E-Mail'),
{
var => 'mail{feedbackSubjectFormat}',
doc => x('Format for the subject line in feedback emails'),
doc => x('Format for the subject of feedback emails'),
doc2 => x(
'When students click the <em>Email Instructor</em> button to send feedback, WeBWorK fills in the '
. 'subject line. Here you can set the subject line. In it, you can have various bits of '
. 'information filled in with the following escape sequences.<p><ul><li>%c = course ID</li>'
'<p>When students click the <em>Email Instructor</em> button to send feedback, WeBWorK fills in '
. 'the subject line. Here you can set the subject line. In it, you can have various bits of '
. 'information filled in with the following escape sequences.</p><ul><li>%c = course ID</li>'
. '<li>%u = user ID</li><li>%s = set ID</li><li>%p = problem ID</li><li>%x = section</li>'
. '<li>%r = recitation</li><li>%% = literal percent sign</li></ul>'
. '<li>%r = recitation</li><li>%% = literal percent sign</li></ul><p>If content is between '
. "a brace pair, like '{ rec:%r}', then it will only be included in the subject line if all "
. 'substitutions within the double brace pair are defined and nonempty.'
),
width => 45,
type => 'text'
Expand Down
21 changes: 9 additions & 12 deletions lib/WeBWorK/ContentGenerator/Feedback.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use Email::Stuffer;
use Try::Tiny;

use WeBWorK::Upload;
use WeBWorK::Utils qw(createEmailSenderTransportSMTP fetchEmailRecipients);
use WeBWorK::Utils qw(createEmailSenderTransportSMTP fetchEmailRecipients formatEmailSubject);

# request paramaters used
#
Expand Down Expand Up @@ -108,18 +108,15 @@ sub initialize ($c) {
}
}

my %subject_map = (
'c' => $courseID,
'u' => $user ? $user->user_id : undef,
's' => $set ? $set->set_id : undef,
'p' => $problem ? $problem->problem_id : undef,
'x' => $user ? $user->section : undef,
'r' => $user ? $user->recitation : undef,
'%' => '%',
my $subject = formatEmailSubject(
$ce->{mail}{feedbackSubjectFormat},
$courseID,
$user ? $user->user_id : '',
$set ? $set->set_id : '',
$problem ? $problem->problem_id : '',
$user ? $user->section : '',
$user ? $user->recitation : ''
);
my $chars = join('', keys %subject_map);
my $subject = $ce->{mail}{feedbackSubjectFormat} || 'WeBWorK question from %c: %u set %s/prob %p';
$subject =~ s/%([$chars])/defined $subject_map{$1} ? $subject_map{$1} : ''/eg;

my %data = (
user => $user,
Expand Down
28 changes: 28 additions & 0 deletions lib/WeBWorK/Utils.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ our @EXPORT_OK = qw(
processEmailMessage
createEmailSenderTransportSMTP
generateURLs
formatEmailSubject
getAssetURL
x
);
Expand Down Expand Up @@ -382,6 +383,33 @@ sub generateURLs ($c, %params) {
}
}

sub formatEmailSubject ($formatString, $courseID, $userID, $setID, $problemID, $section, $recitation) {
my %subject_map = (
c => $courseID,
u => $userID,
s => $setID,
p => $problemID,
x => $section,
r => $recitation,
'%' => '%',
);
my $chars = join('', keys %subject_map);
my $subject = $formatString;
# extract the brace pairs
my @braces = $formatString =~ /(\{(?:[^{}]*|(?0))*\})/xg;
Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the recursive sub-pattern in this is right. I think it should be removed, and this should be

Suggested change
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).

Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@drgrice1 drgrice1 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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('|', keys %braces);
$regex = qr/$regex/;
$subject =~ s/($regex)/$braces{$1}/g;
}
$subject =~ s/%([$chars])/$subject_map{$1} ne '' ? $subject_map{$1} : ''/eg;
return $subject;
}

my $staticWWAssets;
my $staticPGAssets;
my $thirdPartyWWDependencies;
Expand Down
19 changes: 6 additions & 13 deletions lib/WeBWorK/Utils/ProblemProcessing.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use Try::Tiny;
use Mojo::JSON qw(encode_json decode_json);

use WeBWorK::Debug;
use WeBWorK::Utils qw(encodeAnswers createEmailSenderTransportSMTP);
use WeBWorK::Utils qw(encodeAnswers createEmailSenderTransportSMTP formatEmailSubject);
use WeBWorK::Utils::DateTime qw(before after);
use WeBWorK::Utils::JITAR qw(jitar_id_to_seq jitar_problem_adjusted_status);
use WeBWorK::Utils::Logs qw(writeLog writeCourseLog);
Expand Down Expand Up @@ -383,19 +383,12 @@ sub jitar_send_warning_email ($c, $userProblem) {

$problemID = join('.', jitar_id_to_seq($problemID));

my %subject_map = (
'c' => $courseID,
'u' => $userID,
's' => $setID,
'p' => $problemID,
'x' => $user->section,
'r' => $user->recitation,
'%' => '%',
my $subject = formatEmailSubject(
$ce->{mail}{feedbackSubjectFormat},
$courseID, $userID, $setID, $problemID,
$user ? $user->section : '',
$user ? $user->recitation : ''
);
my $chars = join('', keys %subject_map);
my $subject = $ce->{mail}{feedbackSubjectFormat}
|| 'WeBWorK question from %c: %u set %s/prob %p'; # default if not entered
$subject =~ s/%([$chars])/defined $subject_map{$1} ? $subject_map{$1} : ""/eg;

my $full_name = $user->full_name;
my $email_address = $user->email_address;
Expand Down