Skip to content
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

change introduction to feedback emails #2649

Merged
merged 5 commits into from
Feb 11, 2025
Merged

change introduction to feedback emails #2649

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
048db57
change introduction to feedback emails
Alex-Jordan Dec 18, 2024
7cf323d
trim and alphabetize URLs in feedback emails
Alex-Jordan Feb 1, 2025
e7f0af0
make it so URLs in feedback emails always show the problem grader
Alex-Jordan Feb 1, 2025
aed3a73
Construct the emailable URL directly and don't use systemLink for that.
drgrice1 Feb 1, 2025
b1f4114
Merge pull request #34 from drgrice1/email-format-suggested-changes
Alex-Jordan Feb 2, 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
5 changes: 3 additions & 2 deletions lib/WeBWorK/ContentGenerator.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,10 +1196,11 @@ sub systemLink ($c, $urlpath, %options) {
my $url = $options{use_abs_url} ? $urlpath->to_abs : $urlpath;

for my $name (keys %params) {
$params{$name} = [ $c->param($name) ] if (!defined $params{$name} && defined $c->param($name));
$params{$name} = [ $c->param($name) ]
if (!defined $params{$name} && defined $c->param($name) && $c->param($name) ne '');
}

return %params ? $url->query(%params) : $url;
return %params ? $url->query([ map { $_, $params{$_} } sort { $a cmp $b } keys %params ]) : $url;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the change on this line. There is no need to sort the parameters. Removing the empty string parameters above is good, but the sorting here isn't needed.

I don't see the need to make the link particularly human readable. Most users don't look at URLs anymore anyway. In fact, perhaps it is time to think about changing to using HTML content for the feedback emails and making the link an actual link so that the URL is not shown, and instead a more friendly link name shown instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as the emails still directly show the URL, I do like having a consistent order. Although I looked again at the doc for query and I think the outer brackets here are unnecessary.

Do you see moving to HTML in email before 2.20?

Copy link
Member

Choose a reason for hiding this comment

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

However, this is used for every single URL generated throughout the webwork2 app numerous times on every request. The inefficiency of sorting here is a definite no.

Copy link
Member

Choose a reason for hiding this comment

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

You are correct that the brackets are not needed.

Copy link
Member

Choose a reason for hiding this comment

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

As to moving to HTML before 2.20, that is possible. It wouldn't take much to add basic html formatting.

}

=item nbsp($string)
Expand Down
12 changes: 4 additions & 8 deletions lib/WeBWorK/ContentGenerator/Feedback.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,11 @@ sub initialize ($c) {

my $systemURL = $c->url_for('root')->to_abs;

my $msg = qq/This message was automatically generated by the WeBWorK
system at $systemURL, in response to a request from $remote_host:$remote_port.

Click this link to see the page from which the user sent feedback:
$emailableURL

/;
my $msg = sprintf("Message from %s (%s) via WeBWorK at\n%s\n\n", $user->full_name, $user->user_id, $systemURL);
$msg .= "To visit the page from which the user sent feedback, go to:\n$emailableURL\n\n";

if ($feedback) {
$msg .= qq/***** The feedback message: *****\n\n\n$feedback\n\n\n/;
$msg .= sprintf("%s (%s) wrote:\n\n\n%s\n\n\n", $user->full_name, $user->user_id, $feedback);
}
if ($problem and $verbosity >= 1) {
$msg .=
Expand All @@ -172,6 +167,7 @@ $emailableURL
if ($user && $verbosity >= 1) {
$msg .= "***** Data about the user: *****\n\n";
$msg .= $c->format_user($user) . "\n";
$msg .= "$remote_host:$remote_port\n";
}

if ($problem && $verbosity >= 1) {
Expand Down
15 changes: 11 additions & 4 deletions lib/WeBWorK/Utils.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,11 +358,18 @@ sub generateURLs ($c, %params) {

if ($userName) {
my $routePath;
my @args;
my $args = {};
if (defined $params{set_id} && $params{set_id} ne '') {
if ($params{problem_id}) {
$routePath = $c->url_for('problem_detail', setID => $params{set_id}, problemID => $params{problem_id});
@args = qw/displayMode showOldAnswers showCorrectAnswers showHints showSolutions/;
$args = {
displayMode => undef,
showOldAnswers => undef,
showCorrectAnswers => undef,
showHints => undef,
showSolutions => undef,
showProblemGrader => 1
};
} else {
$routePath = $c->url_for('problem_list', setID => $params{set_id});
}
Expand All @@ -372,10 +379,10 @@ sub generateURLs ($c, %params) {
$emailableURL = $c->systemLink(
$routePath,
authen => 0,
params => [ 'effectiveUser', @args ],
params => { effectiveUser => undef, %{$args} },
use_abs_url => 1,
);
$returnURL = $c->systemLink($routePath, params => [@args]);
$returnURL = $c->systemLink($routePath);
} else {
$emailableURL = '(not available)';
$returnURL = '';
Expand Down