My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 157: Please show package & file names in code review emails, and fix links
  Back to list
Status:  Released
Owner:  code-rev...@gtempaccount.com
Closed:  Oct 2012


Sign in to add a comment
 
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
Reported by Andrew D. Stadler (Google) <stadler@android.com> on Wed Apr 29 09:06:47 PDT 2009
Source: JIRA GERRIT-157

Some general feedback on code review emails, and suggestions to improve
them.   Typical email:

---------- Forwarded message ----------
From: Bjorn Bringert <bringert@android.com>
Date: Wed, Apr 29, 2009 at 7:28 AM
Subject: [android-search] [donut] Change 725: (platform/frameworks/base) Add
'exported' attribute to searchable meta-data.
To: Android Search ML <android-search@google.com>
Cc: Bjorn Bringert <bringert@android.com>

Bjorn Bringert has requested that you review a change:

725 - Add 'exported' attribute to searchable meta-data.

--
To respond visit https://android-git.corp.google.com/g/725
To unsubscribe, visit https://android-git.corp.google.com/g/settings


My requests.

1.  Please list the files, not just the package.  Knowing which files are
affected is a big piece (for me) in deciding how to approach a code review,
how to prioritize it, etc.

2.  The primary "action" of this email is to click the link.  But the link is
buried down at the bottom, and even worse, it's right next to another link
with a drastically different outcome.  Purely from a UX perspective, this
slows you down because you have to consciously move slowly to pick one or the
other action.  I would request that the two links in the email be moved away
from each other, so they can be more easily targeted by a user who is moving
quickly.  I think Mondrian does this well, so following their lead, how about
calling out the primary action link and doing it right up there at the top:

I'd like you to do a code review. Please point your web browser to:
        https://android-git.corp.google.com/g/725

Administrative tasks, like the unsubscribe link, should be all the way at the
bottom.

3.  The snippet for this code review (in a reasonably-sized Gmail window)
looks like this:
   [android-search] [donut] Change 725: (platform/frameworks/base) Add
'exported' at

I know that [android-search] was added by a mailing list, so this is worse
than normal, but my main point is that (in my opinion) the change description
is more important than the package, and I would prefer to see the package
removed so we can see more of the change description.

Thanks,
--Andy
Sep 24, 2009
#1 code-rev...@gtempaccount.com
Comment by Andrew D. Stadler (Google) <stadler@android.com> on Wed Apr 29 10:54:13 PDT 2009

4.  I just noticed that at the very end of a review cycle, you get an email
announcing the submit, and it includes the original change description.  I
think that this change description should have been included in the original
email requesting the review.
Sep 24, 2009
#2 code-rev...@gtempaccount.com
Comment by Andrew D. Stadler (Google) <stadler@android.com> on Mon May 04 15:15:51 PDT 2009

5.  When multiple reviewers are requested, please indicate this in the email
notification(s).  Multiple recipients on the notification email would be
great, extra text in the email messages would be OK.

Knowing who else is reviewing a change is important context.
Sep 24, 2009
#3 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Wed May 20 19:23:28 PDT 2009
Sep 24, 2009
#4 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Thu May 21 11:39:11 PDT 2009

Fixed by https://review.source.android.com/10033
Sep 24, 2009
#5 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Thu May 21 11:39:11 PDT 2009

Fixed in version 2.0.13.
Status: Fixed
Sep 25, 2009
#6 code-rev...@gtempaccount.com
(No comment was entered for this change.)
Labels: FixedIn-2.0.13
Oct 25, 2012
#7 sop@google.com
(No comment was entered for this change.)
Status: Released
Sign in to add a comment

Powered by Google Project Hosting