Merge lp:~nhandler/launchpad/bugfix296469.2 into lp:launchpad

[Nathan Handler]

The description for the review_type field on Merge Proposals used to be the same on all pages with this field. Now, each page that has a review_type field overrides the description, allowing Launchpad to display descriptions that are more appropriate for the page that they are being displayed on.

[Nathan Handler (nhandler)]

Summary

Bug #296469 describes how the propose branch for merging page (+register-merge) has a description for the 'Review type' field that says "Lowercase keywords describing the type of review you're performing." You are requesting that someone else review your branch, you are not doing the actual reviewing. As a result, the description should be updated to make this more clear.

Proposed fix

The proposed fix is to adjust the call to copy_field to override the review_type description on all of the browser interfaces that reference review_type.

Pre-implementation notes

I have discussed this patch with Michael Hudson (mwhudson). He said that we should also update the description in the interface to describe what the field means in the object.

Implementation details

lib/lp/code/browser/branch.py:
  * Adjust call to copy_field to override review_type description
lib/lp/code/browser/codereviewcomment.py:
  * Adjust call to copy_field to override review_type description
lib/lp/code/interfaces/codereviewvote.py:
  * Modify description to be generic and applicable to all pages that reference review_type

Tests

$ ./bin/test -vvct stories/codereview
Running tests at level 1
Total: 0 tests, 0 failures, 0 errors in 0.000 seconds.
nhandler@nathan-laptop:~/launchpad/lp-branches/devel$ ./bin/test -vvct stories/branches
Running tests at level 1
Running canonical.testing.layers.PageTestLayer tests:
  Set up canonical.testing.layers.BaseLayer in 0.002 seconds.
  Set up canonical.testing.layers.DatabaseLayer in 0.814 seconds.
  Set up canonical.testing.layers.LibrarianLayer in 6.426 seconds.
  Set up canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Set up canonical.testing.layers.FunctionalLayer in 5.173 seconds.
  Set up canonical.testing.layers.GoogleServiceLayer in 1.413 seconds.
  Set up canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Set up canonical.testing.layers.PageTestLayer in 0.000 seconds.
  Running:
 lib/lp/code/tests/../stories/branches/package-branch-merge-proposals.txt
 lib/lp/code/tests/../stories/branches/package-branch-merges-with-product-branches.txt
 lib/lp/code/tests/../stories/branches/revision-details.txt
 lib/lp/code/tests/../stories/branches/xx-bazaar-home.txt
 lib/lp/code/tests/../stories/branches/xx-branch-deletion.txt
 lib/lp/code/tests/../stories/branches/xx-branch-edit-privacy.txt
 lib/lp/code/tests/../stories/branches/xx-branch-edit.txt
 lib/lp/code/tests/../stories/branches/xx-branch-index.txt
 lib/lp/code/tests/../stories/branches/xx-branch-listings-merge-proposal-badge.txt
 lib/lp/code/tests/../stories/branches/xx-branch-listings.txt
 lib/lp/code/tests/../stories/branches/xx-branch-merge-proposals.txt
 lib/lp/code/tests/../stories/branches/xx-branch-mirror-failures.txt
 lib/lp/code/tests/../stories/branches/xx-branch-reference.txt
 lib/lp/code/tests/../stories/branches/xx-branch-tag-cloud.txt
 lib/lp/code/tests/../stories/branches/xx-branch-url-validation.txt
 lib/lp/code/tests/../stories/branches/xx-branch-visibility-policy.txt
 lib/lp/code/tests/../stories/branches/xx-branchmergeproposal-listings.txt
 lib/lp/code/tests/../stories/branches/xx-bug-branch-links.txt
 lib/lp/code/tests/....

[Michael Hudson-Doyle (mwhudson)]

This is very nearly there. Two things:

1) Grepping about shows that the description appears in two more places:

 * The request a review page ($mp/+request-review).
 * The review page ($mp/+review) -- it's confusing that there's this and the +comment page.

Can you edit IReviewRequest and IAddVote in lp.code.browser.branchmergeproposal as appropriate?

2) I think "Lowercase keywords describing the type of review." would be a better description for the actual interface class itself.

If you can make these changes, I'll land it.

review: Needs Fixing

[Michael Hudson-Doyle (mwhudson)]

Looks great, thanks.

review: Approve

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2009-10-06 16:24:20 +0000
+++ lib/lp/code/browser/branch.py	2009-10-09 04:19:12 +0000
@@ -1133,7 +1133,10 @@
     reviewer = copy_field(
         ICodeReviewVoteReference['reviewer'], required=False)
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'])
+    review_type = copy_field(
+        ICodeReviewVoteReference['review_type'],
+        description=u'Lowercase keywords describing the type of review you '
+                     'would like to be performed.')
 
 
 class RegisterBranchMergeProposalView(LaunchpadFormView):

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2009-09-19 05:01:40 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2009-10-09 04:19:12 +0000
@@ -628,7 +628,10 @@
 
     reviewer = copy_field(ICodeReviewVoteReference['reviewer'])
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'])
+    review_type = copy_field(
+        ICodeReviewVoteReference['review_type'], 
+        description=u'Lowercase keywords describing the type of review you '
+                     'would like to be performed.')
 
 
 class BranchMergeProposalRequestReviewView(LaunchpadEditFormView):
@@ -1125,7 +1128,10 @@
 
     vote = copy_field(ICodeReviewComment['vote'], required=True)
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'])
+    review_type = copy_field(
+        ICodeReviewVoteReference['review_type'], 
+        description=u'Lowercase keywords describing the type of review you '
+                     'are performing.')
 
     comment = Text(title=_('Comment'), required=False)
 

=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2009-09-10 20:49:25 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2009-10-09 04:19:12 +0000
@@ -192,7 +192,10 @@
 
     vote = copy_field(ICodeReviewComment['vote'], required=False)
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'])
+    review_type = copy_field(
+        ICodeReviewVoteReference['review_type'],
+        description=u'Lowercase keywords describing the type of review you '
+                     'are performing.')
 
     comment = Text(title=_('Comment'), required=False)
 

=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
--- lib/lp/code/interfaces/codereviewvote.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/interfaces/codereviewvote.py	2009-10-09 04:19:12 +0000
@@ -58,8 +58,7 @@
         TextLine(
             title=_('Review type'), required=False,
             description=_(
-                "Lowercase keywords describing the type of review you're "
-                "performing.")))
+                "Lowercase keywords describing the type of review.")))
 
     comment = exported(
         Reference(