179582 - Format flag email to be more informative

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Gervase Markham [:gerv]]

"Joel Peshkin <[e-mail]> 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 [e-mail] 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 <[e-mail]> has asked Gervase Markham
<[e-mail]>, for review...

Gerv

Comment 1

[Jeff Hedlund]

Attachment: Patch v.1

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

Comment 2

[Jeff Hedlund]

assigning to me.

Comment 3

[Jacob Steenhagen]

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, [e-mail] 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.

Comment 4

[Jeff Hedlund]

Attachment: Patch v.2

Ok, this takes the suggestions into consideration. 

Here is how a new request looks:
-------------------------------
    Flag Requested: review
  Flag Assigned To: Jeff Hedlund <[e-mail]>
      Requested By: testbugs <[e-mail]>
	       Bug: 37 - test flag requests

testbugs <[e-mail]> has requested review from Jeff Hedlund
<[e-mail]>

http://xxxxxxxxxxx/show_bug.cgi?id=37

-------------------------------

And here is a fulfillment:

-------------------------------
    Flag Requested: review
  Flag Assigned To: Jeff Hedlund <[e-mail]>
      Requested By: testbugs <[e-mail]>
	       Bug: 37 - test flag requests

testbugs <[e-mail]> 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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 7

[Jeff Hedlund]

> 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.

Comment 8

[Jeff Hedlund]

Attachment: Patch v.3

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 <[e-mail]>
      Requested By: testbugs <[e-mail]>
	       Bug: 3 - testing flag requests
	Attachment: 2 - patch v.1

testbugs <[e-mail]> has requested review_test from Jeff Hedlund
<[e-mail]>

http://xxxxx/attachment.cgi?id=2&action=edit
------------------------------------------------------------------------------

Comment 9

[Joel Peshkin]

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 10

[Gervase Markham [:gerv]]

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

Comment 11

[Jacob Steenhagen]

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

Comment 12

[Jeff Hedlund]

Attachment: Patch v.4

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 <[e-mail]>
       Assigned To: Jeff Hedlund <[e-mail]>
	       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

Comment 13

[Myk Melez [:myk] [@mykmelez]]

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 <[e-mail]>
 Assigned To: Jeff Hedlund <[e-mail]>
--------------------------------------------------------------------

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

Comment 14

[Gervase Markham [:gerv]]

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 [e-mail]:
or 
review denied by [e-mail]:
...
?

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Myk Melez [:myk] [@mykmelez]]

>Could the first line be:
>
>review requested of [e-mail]:
>or 
>review denied by [e-mail]:

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.

Comment 17

[Jeff Hedlund]

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.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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)?

Comment 19

[Jeff Hedlund]

Attachment: Patch v.5

+ 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 <[e-mail]>:

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 <[e-mail]>
    Assigned To: Jeff Hedlund <[e-mail]>


------- Additional Comments from Jeff Hedlund <[e-mail]>
Looks good, this fixes the issue.

-----------------------------------------------------------------------------

Comment 20

[Jeff Hedlund]

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

Comment 21

[Jeff Hedlund]

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

Comment 22

[Myk Melez [:myk] [@mykmelez]]

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 <[e-mail]>:
review granted by Jeff Hedlund <[e-mail]>:

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 <[e-mail]>:
Jeff Hedlund <[e-mail]> 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 <[e-mail]> has asked Jeff Hedlund <[e-mail]>
for review:
Jeff Hedlund <[e-mail]> has granted Myk Melez
<[e-mail]>'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 <[e-mail]> has *asked* Jeff Hedlund
<[e-mail]> for *review*:
Jeff Hedlund <[e-mail]> has *granted* Myk Melez
<[e-mail]>'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.)

Comment 23

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Also note bug 180444.

Comment 25

[Jeff Hedlund]

Attachment: Patch v.6

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

Comment 26

[Jeff Hedlund]

Attachment: 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 <[e-mail]> has *granted* Jeff Hedlund
<[e-mail]>'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 <[e-mail]>
granting this request
-----------------------------------------------------------------------------

Comment 27

[Jeff Hedlund]

> + changed accepted to granted

Er, changed "approved" to "granted".

Comment 28

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 30

[Bradley Baetz (:bbaetz)]

Theres a space missing before the requester's name. I get:

Bradley Baetz <[e-mail]> has cancelledMyk Melez
<[e-mail]>'s request for review:

....

Comment 31

[Jeff Hedlund]

Attachment: request/email.txt.tmpl v.2

+ 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)

Comment 32

[Jacob Steenhagen]

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 :)

Comment 33

[Gervase Markham [:gerv]]

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 34

[Myk Melez [:myk] [@mykmelez]]

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

Comment 35

[Jeff Hedlund]

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