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/../stories/branches/xx-claiming-team-code-reviews.txt
 lib/lp/code/tests/../stories/branches/xx-code-review-comments.txt
 lib/lp/code/tests/../stories/branches/xx-creating-branches.txt
 lib/lp/code/tests/../stories/branches/xx-distribution-branches.txt
 lib/lp/code/tests/../stories/branches/xx-distroseries-branches.txt
 lib/lp/code/tests/../stories/branches/xx-junk-branches.txt
 lib/lp/code/tests/../stories/branches/xx-person-branches.txt
 lib/lp/code/tests/../stories/branches/xx-person-portlet-teambranches.txt
 lib/lp/code/tests/../stories/branches/xx-personproduct-branch-listings.txt
 lib/lp/code/tests/../stories/branches/xx-private-branch-listings.txt
 lib/lp/code/tests/../stories/branches/xx-product-branches.txt
 lib/lp/code/tests/../stories/branches/xx-product-overview.txt
 lib/lp/code/tests/../stories/branches/xx-project-branches.txt
 lib/lp/code/tests/../stories/branches/xx-register-a-branch.txt
 lib/lp/code/tests/../stories/branches/xx-source-package-branches-empty.txt
 lib/lp/code/tests/../stories/branches/xx-source-package-branches-listing.txt
 lib/lp/code/tests/../stories/branches/xx-subscribing-branches.txt
 lib/lp/code/tests/../stories/branches/xx-upload-directions.txt
  Ran 391 tests with 0 failures and 0 errors in 2 minutes 59.598 seconds.
Tearing down left over layers:
  Tear down canonical.testing.layers.PageTestLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LibrarianLayer in 0.258 seconds.
  Tear down canonical.testing.layers.GoogleServiceLayer in 0.086 seconds.
  Tear down canonical.testing.layers.FunctionalLayer ... not supported
  Tear down canonical.testing.layers.DatabaseLayer in 0.206 seconds.
  Tear down canonical.testing.layers.BaseLayer in 0.000 seconds.


Demo and Q/A

  * Log on as Foo Bar (<email address hidden>:test)

  * Visit https://code.launchpad.dev/~name12/firefox/main/+register-merge

  * Observe the modified description for the 'Review type' field.

  * Propose the branch for merging into lp://dev/~mark/firefox/release-0.9

  * Visit https://code.launchpad.dev/~name12/firefox/main/+merge/1/+comment

  * Observe that a different description is displayed for the 'Review type' field.


lint

$ make lint
utilities/shhh.py PYTHONPATH= python2.4 bootstrap.py\
                --ez_setup-source=ez_setup.py \
		--download-base=download-cache/dist --eggs=eggs
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/codereviewcomment.py
  lib/lp/code/interfaces/codereviewvote.py


== Pylint notices ==

lib/lp/code/browser/branch.py
    44: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    45: [F0401] Unable to import 'lazr.enum' (No module named enum)
    46: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    47: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    54: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)

lib/lp/code/browser/codereviewcomment.py
    21: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    22: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)

lib/lp/code/interfaces/codereviewvote.py
    20: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    21: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)


diff

{{{
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2009-10-09 02:01:02 +0000
+++ lib/lp/code/browser/branch.py	2009-10-09 02:36:43 +0000
@@ -1132,7 +1132,10 @@
     reviewer = copy_field(
         ICodeReviewVoteReference['reviewer'], required=False)
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'],description=u'Lowercase keywords describing the type of review you would like to be performed.')
+    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/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2009-10-09 02:01:02 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2009-10-09 02:42:18 +0000
@@ -192,7 +192,10 @@
 
     vote = copy_field(ICodeReviewComment['vote'], required=False)
 
-    review_type = copy_field(ICodeReviewVoteReference['review_type'],description=u'Lowercase keywords describing the type of review you are performing.')
+    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-10-09 02:01:02 +0000
+++ lib/lp/code/interfaces/codereviewvote.py	2009-10-09 02:32:22 +0000
@@ -59,7 +59,7 @@
             title=_('Review type'), required=False,
             description=_(
                 "Lowercase keywords describing the type of review you are "
-                "performing or would like to be performed.")))
+                 "performing or would like to be performed.")))
 
     comment = exported(
         Reference(

}}}