change introduction to feedback emails

[Alex-Jordan]

This changes some things about the opening lines to a feedback email.

This may be controversial. I'm happy to make changes. This started out simply as removing the double space that was there, but then I kept going. I think it's nicer this way, but not opposed to further changes or retractions.

[Alex-Jordan]

Here is what an email looks like with this:

This message was generated by WeBWorK at
https://webwork-hosting.runestone.academy/webwork2
in response to a request from Alex Jordan (alex.jordan) at
162.158.63.39:20448

To visit the page from which the user sent feedback, go to:
https://webwork-hosting.runestone.academy/webwork2/Active-Calculus-Demo/1_1-How_do_we_measure_velocity_Exercises/1?effectiveUser=alex.jordan&showSolutions=1&showCorrectAnswers=1&displayMode=MathJax&showHints=1&showOldAnswers=1

Alex Jordan (alex.jordan) wrote:


I need help with this exercise!

Maybe the ***** The feedback message: ***** part should be retained. I don't know.

What is supposed to be the point of the IP address? When I initiated this message, I was at home using PCC's VPN, logged in to a course on the Runestone WW server. I looked up this IP address and it's somewhere in New Jersey. I turned off the VPN and then I get the same IP address, so it's not some mysterious thing about PCC's VPN. Also, it is not the IP address for the Runestone WW server.

[somiaj]

I would be okay with simplifying the whole first paragraph. If someone prefers to include the users ip, maybe an option to not include it would be nice. I was thinking along the lines of of Message from <user> using WeBWorK at <course url>.

[drgrice1]

This looks fine to me.

[drgrice1]

I like the addition of the user name and login id to the email message. I would be fine with removing the remote host and port from the email. I doubt that is actually useful to anyone in reality. Remote host ip addresses rarely are anymore.

[drgrice1]

Another idea would be to move the host ip and port down with the data about the user at the end of the email.

[somiaj]

Here are some more ideas, modify what you think is best, just thinking of other things that could be updated.

If wanting to keep the ip address, maybe add that to the $mail{feedbackVerbosity}='debug' blocks, but omit it in other cases.

Also since you are editing this and the user's id and full name is now included by default, you might want to update the output for both standard and debug output. The format_user data also gives the user id, full name, and email (since it is the from: field), so consider if duplicated output is useful (or maybe only include the user id or full name in your changes as the user data will include the full set of info). You may also want to update the other headers like $msg .= "***** Data about the user: *****\n\n"; to match the changes you have already made as well.

[Alex-Jordan]

I'm making a few changes to this now as suggested here.

One more change I would like to make is have it so that when the email comes from a problem, the URL would include showProblemGrader=1. But I can't work out how to do that. Simply adding showProblemGrader to line 355 of lib/WeBWorK/Utils.pm is not enough.

[Alex-Jordan]

Now an email looks like:

Message from Alex Jordan (alex.jordan) via WeBWorK at
https://webwork-hosting.runestone.academy/webwork2

To visit the page from which the user sent feedback, go to:
https://webwork-hosting.runestone.academy/webwork2/Active-Calculus-Demo/1_1-How_do_we_measure_velocity_Exercises/1?effectiveUser=alex.jordan&showSolutions=1&showCorrectAnswers=1&displayMode=MathJax&showHints=1&showOldAnswers=1

Alex Jordan (alex.jordan) wrote:


I need help with this exercise!


***** Other stuff, if verbosity is high *****

[drgrice1]

I just noticed something wrong with the feedback page. It is not caused by this pull request, and is also in develop. The "Cancel E-mail" link shown before email is sent, and the "Return to your work" link shown after sending an email also get the parameters from the generateURLs method. That means that the parameters showCorrectAnswers, showHints, showSolutions, etc., are all added to those links. Those shouldn't be there. I noticed this because I tested sending an email as my instructor user, and when returning to the page the correct answers were all shown. Those links really shouldn't come from that method at all.

Sorry, this is a sidetrack from what this pull request is about, but I wanted it noted.

[drgrice1]

One more change I would like to make is have it so that when the email comes from a problem, the URL would include showProblemGrader=1. But I can't work out how to do that. Simply adding showProblemGrader to line 355 of lib/WeBWorK/Utils.pm is not enough.

I think that you are misinterpreting what it means to pass the params option to systemLink as an array reference as is done in generateURLs. It doesn't mean add those parameters to the link. It means add the values of those parameters from the current request to the link if they are set. So the current things in the @args array are added to the link if those parameters were set for the user's request that is sending the email.

Note that many of the parameters are passed from the problem to the email form when the feedbackMacro method is called in lib/WeBWorK/ContentGenerator/Problem.pm and the feedback_macro_email.html.ep template is rendered. But that is still only in the case that those parameters were set for the user on the problem page to begin with.

What you could do is change what is passed to the systemLink call to a hash reference instead of an array reference. Anything in the hash that has a value of undef will work in the same way as for the array approach. However, if you add showProblemGrader => 1 instead of showProblemGrader => undef to the hash, then it will say to use the value you give rather than use the value from the current request parameters.

If you do that (and even if you don't) the params argument should not be passed to the systemLink call used for the $returnURL. That is the reason for the issue that noted in my other comment. So line 368 in the generateURLs method of Utils.pm should be changed from $returnURL = $c->systemLink($routePath, params => [@args]); to $returnURL = $c->systemLink($routePath);.

[drgrice1]

By the way, you should note that there is actually never any reason that any of those parameters should be in the $returnURL since that is only ever used for internal links back to the page the user came from before clicking the "Email Instructor" link.

[Alex-Jordan]

OK, I followed your suggestion and changed args to a hash reference so I could slip in the showProblemGrader. We should think about if this is desirable enough to just make it so, or if it should be configurable. I like the immediate access to it, especially because I know many faculty are unaware the problem grader even exists. And this will bring it to their attention. I don't love that it takes up lots of real estate before you see the problem though.

Also I made some changes so that the query string part of the URL is shorter and pieces are in consistent order. For example what used to be:

https://webwork.pcc.edu/webwork2/course/set/probNum?showOldAnswers=1&showCorrectAnswers=&showHints=&showProblemGrader=1&showSolutions=&effectiveUser=test.student&displayMode=MathJax

is now

https://webwork.pcc.edu/webwork2/course/set/probNum?displayMode=MathJax&effectiveUser=test.student&showOldAnswers=1&showProblemGrader=1

Pointless bits like showHints= are gone. And the query keys are now in alphabetical order.

[drgrice1]

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.

[Alex-Jordan]

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?

[drgrice1]

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.

[drgrice1]

You are correct that the brackets are not needed.

[drgrice1]

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

[drgrice1]

@Alex-Jordan: I put in a pull request to your branch for this pull request with a suggested change. It changes this so that the $emailableURL constructed in the generateURLs method of Utils.pm does not use systemLink at all. Instead it just constructs the URL on its own (basically doing the same thing that systemLink does). It is actually quite simple in this case. This way the parameters can be sorted for this, but not in the systemLink method that is a general utility method and needs to remain efficient.

[Alex-Jordan]

OK, thanks for that. I was thinking of making a sortedParams option for systemLink but maybe this is better.

I'm a bit slow to act on things last week and this week. I fractured a rib in a sledding incident testing out a run ahead of my kids, and between the pain, discomfort, and opioids, I'm not even sure what day it is. So it may be a while before I get to merging that. Or maybe I will be awake again at midnight with nothing to do except patrol WW/PG PRs :)