"Joel Peshkin <bugreport@peshkin.net> has asked you for review on bug #179491 (Searchs of attchaments containing a string do not enforce attchment privacy), attachment #105832 [details] [diff] [review] (patch)." Because of the way reviewers@bugzilla.org works, mail like this comes to everyone on that list. You have to look at the headers to see who the request is actually aimed at. I suggest something like: Joel Peshkin <bugreport@peshkin.net> has asked Gervase Markham <gerv@mozilla.org>, for review... Gerv

Sends requests as Gerv example shows. Additionally shows the requestor (setter) on the fullfilment.

assigning to me.

I'd actually like to see the entire format of this e-mail change. I think it's too compact and unlike what we use for the rest of our bugmail. I'd like to see a nicely formatted page. In IRC, bzbarsky@mit.edu suggested the following format: Requested: review Requested By: foo Bug #: bar Patch Descrption: baz Bug Summary: die die die I'd actually like to see something like this, although I kinda like the two column approach of New: bugmail. Does the template toolkit have something that allows us to approximate formline? Not to confuse the issue on you Jeff, but I think nothing about the review request (except the fact that it's a review request) is imediately obvious due to how compact this message is and adding another bit of information isn't gonna help any.

Ok, this takes the suggestions into consideration. Here is how a new request looks: ------------------------------- Flag Requested: review Flag Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> Requested By: testbugs <testbugs@matrixsi.com> Bug: 37 - test flag requests testbugs <testbugs@matrixsi.com> has requested review from Jeff Hedlund <jeff.hedlund@matrixsi.com> http://xxxxxxxxxxx/show_bug.cgi?id=37 ------------------------------- And here is a fulfillment: ------------------------------- Flag Requested: review Flag Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> Requested By: testbugs <testbugs@matrixsi.com> Bug: 37 - test flag requests testbugs <testbugs@matrixsi.com> has approved the request. http://xxxxxxxxxxxxx/show_bug.cgi?id=37 ------------------------------- If it is a flag for an attachment, the attachment shows below the bug in the same format, eg: Attachment: 1234 - the attachment summary And the link for the attachment is shown instead of a link to the bug. Comments/nits/critiques?

sounds good to me so far; I'd need to try it in action for a bit to really have a good idea of how it's working...

Will that new format always include a link to the bug, even if the flag is for an attachment? If not, please include that :) I like to look at the bug before looking at the patch when someone requests a review from me.

> Will that new format always include a link to the bug, even if the flag is for > an attachment? Yeah, that's a good idea. I'll put the bug at the top of the e-mail like all other bugmail, and then if there is an attachment then I'll put the attachment link at the bottom.

Ok, quick change, here's how it looks on a new request: (similar for fulfillment) ------------------------------------------------------------------------------ http://xxxxx/show_bug.cgi?id=3 Flag Requested: review_test Flag Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> Requested By: testbugs <testbugs@matrixsi.com> Bug: 3 - testing flag requests Attachment: 2 - patch v.1 testbugs <testbugs@matrixsi.com> has requested review_test from Jeff Hedlund <jeff.hedlund@matrixsi.com> http://xxxxx/attachment.cgi?id=2&action=edit ------------------------------------------------------------------------------

Comment on attachment 105967 [details] [diff] [review] Patch v.3 r=joel I'd like to see a 2xr on this one to make sure we have consensus that it is the right thing.

Comment on attachment 105967 [details] [diff] [review] Patch v.3 r=gerv on the layout, and the code seems to match it :-) Let's try this out on people and see what they say; this doesn't preclude tweaking it again later. Gerv

*** Bug 179783 has been marked as a duplicate of this bug. ***

In order to make it more readable, myk has come up with this format: (attachment information is of course not listed on a bug only flag) -------------------------------------------------------------------- review_test [requested/approved/denied/cancelled]: Requested By: testbugs <testbugs@matrixsi.com> Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> Bug: 3 - testing flag requests Attachment: 2 - patch v.1 http://xxxx/show_bug.cgi?id=3 http://xxxx/attachment.cgi?id=2&action=edit

I did some more testing and thinking, and here's what I think works best: -------------------------------------------------------------------- review [requested/approved/denied/cancelled]: Bug 3: testing flag requests http://xxxx/show_bug.cgi?id=3 Attachment 2 [details] [diff]: patch v.1 http://xxxx/attachment.cgi?id=2&action=edit Requested By: testbugs <testbugs@matrixsi.com> Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> -------------------------------------------------------------------- Like the previous version we tell the recipient why they are receiving this email on the first line ("review requested"), but in this version we give them the bug and attachment summaries/links next, since that's more important information than who the players are (i.e. they don't need to know who asked them--but they do need to know the links--to do the review).

This bug was opened because it was not easy to see at a glance whether a review request was for you or not. Myk's new layout, while a great improvement in general, doesn't really improve that problem. Could the first line be: review requested of foo@bar.com: or review denied by baz@quux.com: ... ? Also, the further the link to the bug and the link to the attachment are from one another, the more likely I am to always hit the right one when trying to do the review, using my spatial memory. Gerv

*** Bug 180176 has been marked as a duplicate of this bug. ***

>Could the first line be: > >review requested of foo@bar.com: >or >review denied by baz@quux.com: Sounds good to me. >Also, the further the link to the bug and the link to the attachment are from >one another, the more likely I am to always hit the right one when trying to do >the review, using my spatial memory. The mockup in comment 13 should satisfy this by grouping the links with their description rather than each other.

Ok, I'll get a patch together for the changes. One other suggestion I have that I'd like opinion on: In the subject of the e-mail, it would be nice to have: review [requested/approved/cancelled/denied]: (rest of current subject line) So that it's easier to tell what kind of flag e-mail it is. I'll go ahead and add it to the patch unless someone has a problem/suggestion about it.

Could you also add the additional comments that were made when requesting review to the email? Or does that rather belong to another bug (bug 179577)?

+ Added [request/approved/denied/cancelled] to Subject: line + Made formatting changes (comment 16) + Added comments made on the attachment or bug to the e-mail (if any) + Fixed some minor bugs from previous patches Note: The created- and fulfilled- are nearly identical now. I think I could combine them to one (flag-email.txt.tmpl?), but I know that cvs has ugly file removal stuff. Here's the latest format: ----------------------------------------------------------------------------- Subject: review_test approved: [Bug 3] testing flag requests : [Attachment 2 [details] [diff]] patch v.1 ----------------------------------------------------------------------------- review_test approved by Jeff Hedlund <jeff.hedlund@matrixsi.com>: Bug 3: testing flag requests http://xxxx/show_bug.cgi?id=3 Attachment 2 [details] [diff]: patch v.1 http://xxxx/attachment.cgi?id=2&action=edit Requested By: testbugs <testbugs@matrixsi.com> Assigned To: Jeff Hedlund <jeff.hedlund@matrixsi.com> ------- Additional Comments from Jeff Hedlund <jeff.hedlund@matrixsi.com> Looks good, this fixes the issue. -----------------------------------------------------------------------------

(changing summary to better reflect what this bug has become)

*** Bug 179577 has been marked as a duplicate of this bug. ***

Comment on attachment 106499 [details] [diff] [review] Patch v.5 Almost there, but perhaps back where we started... I'm concerned about the inconsistency in placing either the actor or the requestee in the same place on the opening line of the email depending on what kind of email it is: flag requested of requestee: flag action by actor: i.e.: review requested of Jeff Hedlund <jeff.hedlund@matrixsi.com>: review granted by Jeff Hedlund <jeff.hedlund@matrixsi.com>: The words "of" and "by" are insufficiently different to differentiate the two roles, and I think it's likely for people to get regularly confused, even if only slightly and temporarily, by the need to mentally switch their perception of the name in that position. If we want to include the requestee on the first line, we should at least rearrange the non-requestee emails so the actor comes first: flag requested of requestee: actor has action flag: i.e.: review requested of Jeff Hedlund <jeff.hedlund@matrixsi.com>: Jeff Hedlund <jeff.hedlund@matrixsi.com> has granted review: Either way, note that we have almost completely abandoned the formatted approach advocated by bz and Jake, at which point it makes more sense to go back to something similar to the format originally suggested by Gerv when he filed this bug: actor has asked requestee for flag actor has action requester's request for flag i.e.: Myk Melez <myk@mozilla.org> has asked Jeff Hedlund <jeff.hedlund@matrixsi.com> for review: Jeff Hedlund <jeff.hedlund@matrixsi.com> has granted Myk Melez <myk@mozilla.org>'s request for review: The one enhancement I would add is to highlight the action and flag with asterisks to draw them out more: Myk Melez <myk@mozilla.org> has *asked* Jeff Hedlund <jeff.hedlund@matrixsi.com> for *review*: Jeff Hedlund <jeff.hedlund@matrixsi.com> has *granted* Myk Melez <myk@mozilla.org>'s request for *review*: >+ Added [request/approved/denied/cancelled] to Subject: line Good idea. I have mixed feelings about this, partly because it adds to an already crowded subject line and partly because it defeats mail client pseudo-threading, but let's try it and see how it works. The latter problem at least can be dealt with by proper use of mail headers to explicitly identify threading. >+ Added comments made on the attachment or bug to the e-mail (if any) Excellent! >Note: The created- and fulfilled- are nearly identical now. I think I could >combine them to one (flag-email.txt.tmpl?), but I know that cvs has ugly file >removal stuff. CVS is horrible about directories, but it does ok with files. This sounds like a good idea for code quality reasons, so go for it. (Note: cancelling the review request instead of denying this patch review because these are design issues; technically the code is correct.)

Note: when combining templates, use "email.txt.tmpl" instead of "flag-email.txt.tmpl", since the template is already identified as being flag/request-specific by dint of its location in the request/ subdirectory. Also note bug 180541 and bug 180542.

Also note bug 180444.

simple patch to use new combined request/email.txt.tmpl

+ new combined template + fixes URL wrapping problem + changed accepted to granted format now looks like this: ----------------------------------------------------------------------------- Subject: review [requested/granted/denied/cancelled]: [rest of subject] ----------------------------------------------------------------------------- testbugs <testbugs@matrixsi.com> has *granted* Jeff Hedlund <jeff.hedlund@matrixsi.com>'s request for *review_test*: Bug 3: testing flag requests http://xxxx/show_bug.cgi?id=3 Attachment 2 [details] [diff]: patch v.1 http://xxxx/attachment.cgi?id=2&action=edit ------- Additional Comments from testbugs <testbugs@matrixsi.com> granting this request -----------------------------------------------------------------------------

> + changed accepted to granted Er, changed "approved" to "granted".

Comment on attachment 106764 [details] request/email.txt.tmpl >[% USE format_line = format("%15s: %s\n") %] This is no longer necessary. >[% IF flag.status == '?' %] > [% to_email = flag.requestee.email IF flag.requestee.email_prefs.FlagRequester %] This should be FlagRequestee I think. >[% user.realname %] <[% user.login %]> has *[% statuses.${flag.status} %]* >[% to_identity %] for *[% flag.type.name %]*: Now that I see it in action I realize I was wrong about the asterisks. They highlight the appropriate words at the expense of being distracting. Please forget I suggested them and quietly remove them from the final patch.

Taking another look at the patch, it looks like the attachment summary is not being wrapped: >Attachment [% attidsummary %] That should also be wrapped in case it's long.

Theres a space missing before the requester's name. I get: Bradley Baetz <bbaetz@student.usyd.edu.au> has cancelledMyk Melez <myk@mozilla.org>'s request for review: ....

+ Removal of asterisks + Removal of format_line + Fix email pref for requestee + Wrap Attachment Summary if necessary > Theres a space missing before the requester's name. That is because of the removal of the asterisk without adding a forced whitespace. (that is fixed in this template)

We seem to have gotten far away from comment 3... One of the things I don't like about the current review request message is that it's an english sentence. Picking out individual bits of data and processing them in my little mind is much easier in the formatted data approach. Of course, if everyone else likes the sentence, than I guess I loose :)

I'd actually agree with Jake - the format currently in use on b.m.o. isn't easily mentally parseable. However, at 12.05am, I don't have any better suggestions right now :-) Gerv

Comment on attachment 106790 [details] request/email.txt.tmpl v.2 I agree with Jake and Gerv too, but it's not clear what the solution is. Our attempts to define that formatted look haven't gotten very far. This patch is a significant incremental improvement over the current version and should go in while we work on a whole different way of presenting this information. r=myk a=myk

Checking in Bugzilla/Flag.pm;itten /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v <-- Flag.pm new revision: 1.9; previous revision: 1.8 done Removing template/en/default/request/created-email.txt.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/request/created-email.txt.tmpl,v <-- created-email.txt.tmpl new revision: delete; previous revision: 1.2 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v done Checking in template/en/default/request/email.txt.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v <-- email.txt.tmpl initial revision: 1.1 done Removing template/en/default/request/fulfilled-email.txt.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/request/fulfilled-email.txt.tmpl,v <-- fulfilled-email.txt.tmpl new revision: delete; previous revision: 1.1 done