Format flag email to be more informative

RESOLVED FIXED in Bugzilla 2.18

Status

()

defect
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: gerv, Assigned: jeff.hedlund)

Tracking

2.17
Bugzilla 2.18
Dependency tree / graph

Details

Attachments

(2 attachments, 6 obsolete attachments)

"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
Assignee

Comment 1

17 years ago
Posted patch Patch v.1 (obsolete) — Splinter Review
Sends requests as Gerv example shows.  Additionally shows the requestor
(setter) on the fullfilment.
Assignee

Comment 2

17 years ago
assigning to me.
Assignee: myk → jeff.hedlund
Assignee

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 3

17 years ago
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.
Assignee

Comment 4

17 years ago
Posted patch Patch v.2 (obsolete) — Splinter Review
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?
Attachment #105926 - Attachment is obsolete: true
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.
Assignee

Comment 7

17 years ago
> 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.
Assignee

Comment 8

17 years ago
Posted patch Patch v.3 (obsolete) — Splinter Review
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
------------------------------------------------------------------------------
Attachment #105933 - Attachment is obsolete: true

Comment 9

17 years ago
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.
Attachment #105967 - Flags: review+
Assignee

Updated

17 years ago
Attachment #105967 - Flags: review?(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
Attachment #105967 - Flags: review?(gerv)

Comment 11

17 years ago
*** Bug 179783 has been marked as a duplicate of this bug. ***
Assignee

Comment 12

17 years ago
Posted patch Patch v.4 (obsolete) — Splinter Review
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
Attachment #105967 - Attachment is obsolete: true
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.
Assignee

Comment 17

17 years ago
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)?
Assignee

Comment 19

17 years ago
Posted patch Patch v.5 (obsolete) — Splinter Review
+ 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.

-----------------------------------------------------------------------------
Attachment #106060 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #106499 - Flags: review?(myk)
Assignee

Comment 20

17 years ago
(changing summary to better reflect what this bug has become)
Summary: It's not immediately obvious who review mails are for → Format flag email to be more informative
Assignee

Comment 21

17 years ago
*** 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.)
Attachment #106499 - Flags: review?(myk)
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.
Assignee

Comment 25

17 years ago
Posted patch Patch v.6Splinter Review
simple patch to use new combined request/email.txt.tmpl
Attachment #106499 - Attachment is obsolete: true
Assignee

Comment 26

17 years ago
Posted file request/email.txt.tmpl (obsolete) —
+ 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
-----------------------------------------------------------------------------
Assignee

Updated

17 years ago
Blocks: 180542
Assignee

Updated

17 years ago
Blocks: 180541
Assignee

Comment 27

17 years ago
> + changed accepted to granted

Er, changed "approved" to "granted".
Assignee

Updated

17 years ago
Attachment #106764 - Flags: review?(myk)
Attachment #106763 - Flags: review+
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.
Attachment #106764 - Flags: review?(myk) → review-

Updated

17 years ago
Flags: approval+

Updated

17 years ago
Flags: approval+
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:

....
Assignee

Comment 31

17 years ago
+ 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)
Attachment #106764 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #106790 - Flags: review?(myk)

Comment 32

17 years ago
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
Attachment #106790 - Flags: review?(myk) → review+
Assignee

Comment 35

17 years ago
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 2.18
QA Contact: matty_is_a_geek → default-qa

Updated

5 years ago
You need to log in before you can comment on or make changes to this bug.