Hi Brad,

Thanks for making all the changes. This branch is definitely a lot harder than
I thought.

Since this branch is not linked anywhere yet, I would be ok with you deferring
items 2 and 3 below to a followup branch so you can land this before it gets
any bigger. I also have some comments inline, but they shouldn't be too hard
to fix in this branch.

merge-conditional

1. jslint: Lint found in '/home/egrubbs/canonical/lp-branches/review/lib/canonical/launchpad/javascript/code/tests/test_productseries_setbranch.js':
    Line 65 character 15: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
                for (var sub in subscribers) {

    The easiest way to avoid this potential Javascript problem is:
                    Y.each(subscribers, function(sub) {

2. If the Branch name already exists, it appears as if the form does nothing.

3. If there is an existing imported SVN branch on a given URL, the form will
give you the correct error message, but if there is an existing imported BZR
branch on a given URL, it will give you this exception.

Traceback (most recent call last):
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/zope.publisher-3.10.0-py2.5.egg/zope/publisher/publish.py", line 134, in publish
    result = publication.callObject(request, obj)
  File "/home/egrubbs/canonical/lp-branches/review/lib/canonical/launchpad/webapp/publication.py", line 422, in callObject
    return mapply(ob, request.getPositionalArguments(), request)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/zope.publisher-3.10.0-py2.5.egg/zope/publisher/publish.py", line 109, in mapply
    return debug_call(obj, args)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/zope.publisher-3.10.0-py2.5.egg/zope/publisher/publish.py", line 115, in debug_call
    return obj(*args)
  File "/home/egrubbs/canonical/lp-branches/review/lib/canonical/launchpad/webapp/publisher.py", line 274, in __call__
    self.initialize()
  File "/home/egrubbs/canonical/lp-branches/review/lib/canonical/launchpad/webapp/launchpadform.py", line 111, in initialize
    self.form_result = action.success(data)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/zope.formlib-3.6.0-py2.5.egg/zope/formlib/form.py", line 606, in success
    return self.success_handler(self.form, self, data)
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/registry/browser/productseries.py", line 993, in update_action
    data['repo_url'])
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/registry/browser/productseries.py", line 1040, in _createBzrBranch
    url=repo_url)
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/code/model/branchnamespace.py", line 103, in createBranch
    implicit_subscription = self.getPrivacySubscriber()
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/code/model/branchnamespace.py", line 343, in getPrivacySubscriber
    rule = self.product.getBranchVisibilityRuleForTeam(self.owner)
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/code/model/branchvisibilitypolicy.py", line 108, in getBranchVisibilityRuleForTeam
    item = self._selectOneBranchVisibilityTeamPolicy(team)
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/code/model/branchvisibilitypolicy.py", line 90, in _selectOneBranchVisibilityTeamPolicy
    if self.isUsingInheritedBranchVisibilityPolicy():
  File "/home/egrubbs/canonical/lp-branches/review/lib/lp/code/model/branchvisibilitypolicy.py", line 145, in isUsingInheritedBranchVisibilityPolicy
    return self._policy_items.count() == 0
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/sqlobject.py", line 559, in count
    result_set = self._without_prejoins()._result_set
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/sqlobject.py", line 505, in _result_set
    self._finished_result_set = self._finish_result_set()
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/sqlobject.py", line 489, in _finish_result_set
    result = self._prepare_result_set()
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/sqlobject.py", line 483, in _prepare_result_set
    return store.find(find_spec, *args)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/store.py", line 206, in find
    self.flush()
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/store.py", line 486, in flush
    self._flush_one(obj_info)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/store.py", line 523, in _flush_one
    result = self._connection.execute(expr)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/databases/postgres.py", line 243, in execute
    result = Connection.execute(self, Returning(statement), params)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/database.py", line 217, in execute
    raw_cursor = self.raw_execute(statement, params)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/databases/postgres.py", line 259, in raw_execute
    return Connection.raw_execute(self, statement, params)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/database.py", line 299, in raw_execute
    self._check_disconnect(raw_cursor.execute, *args)
  File "/home/egrubbs/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-i686.egg/storm/database.py", line 342, in _check_disconnect
    return function(*args, **kwargs)
IntegrityError: duplicate key value violates unique constraint "branch_url_unique"



>=== modified file 'lib/lp/registry/browser/productseries.py'
>--- lib/lp/registry/browser/productseries.py	2010-03-25 22:35:41 +0000
>+++ lib/lp/registry/browser/productseries.py	2010-04-02 19:35:10 +0000
>@@ -757,7 +753,7 @@
>         'branch_type': LINK_LP_BZR,
>         }
> 
>-    def setUpWidgets(self):
>+    def xsetUpWidgets(self):



I assume you want to really delete this method since you are no longer
using it.



>         super(ProductSeriesSetBranchView, self).setUpWidgets()
> 
>         # Extract the radio buttons from the rcs_type widget, so we can
>@@ -785,6 +781,52 @@
>         self.branch_type_import = str(import_button)
>         self.branch_type_emptymarker = str(emptymarker)
> 
>+    def setUpWidgets(self):
>+        super(ProductSeriesSetBranchView, self).setUpWidgets()
>+
>+        def render(widget, term, current_value, label=None):
>+            if term.value == current_value:
>+                render = widget.renderSelectedItem
>+            else:
>+                render = widget.renderItem
>+            if label is None:
>+                label = term.title
>+            try:
>+                value = term.name


I didn't realize that suggesting this type of solution would be so complicated
due to subtle differences between DBItem and SimpleTerm. I had to prove to
myself whether this could be simplified. Here is a diff that passes your tests
and uses getTerm to convert strings to SimpleTerm objects and DBItem to
lazr.enum._enum.TokenizedItem objects, which have the same attributes.

{{{
=== modified file 'lib/lp/registry/browser/productseries.py'
--- a/lib/lp/registry/browser/productseries.py	2010-04-03 17:21:56 +0000
+++ b/lib/lp/registry/browser/productseries.py	2010-04-03 17:33:12 +0000
@@ -784,17 +784,15 @@
     def setUpWidgets(self):
         super(ProductSeriesSetBranchView, self).setUpWidgets()
 
-        def render(widget, term, current_value, label=None):
+        def render(widget, term_value, current_value, label=None):
+            term = widget.vocabulary.getTerm(term_value)
             if term.value == current_value:
                 render = widget.renderSelectedItem
             else:
                 render = widget.renderItem
             if label is None:
                 label = term.title
-            try:
-                value = term.name
-            except AttributeError:
-                value = term.value
+            value = term.token
             return render(index=term.value,
                           text=label,
                           value=value,
@@ -803,11 +801,7 @@
 
         widget = self.widgets['rcs_type']
         vocab = widget.vocabulary
-        form_value = widget._getFormValue()
-        try:
-            current_value = form_value.value
-        except AttributeError:
-            current_value = vocab.BZR.value
+        current_value = widget._getFormValue()
         self.rcs_type_cvs = render(widget, vocab.CVS, current_value, 'CVS')
         self.rcs_type_svn = render(widget, vocab.BZR_SVN, current_value,
                                    'SVN')
@@ -823,7 +817,7 @@
         (self.branch_type_link,
          self.branch_type_create,
          self.branch_type_import) = [
-            render(widget, vocab.by_value[value], current_value)
+            render(widget, value, current_value)
             for value in (LINK_LP_BZR, CREATE_NEW, IMPORT_EXTERNAL)]
}}}



>+            except AttributeError:
>+                value = term.value
>+            return render(index=term.value,
>+                          text=label,
>+                          value=value,
>+                          name=widget.name,
>+                          cssClass='')
>+
>+        widget = self.widgets['rcs_type']
>+        vocab = widget.vocabulary
>+        form_value = widget._getFormValue()
>+        try:
>+            current_value = form_value.value
>+        except AttributeError:
>+            current_value = vocab.BZR.value
>+        self.rcs_type_cvs = render(widget, vocab.CVS, current_value, 'CVS')
>+        self.rcs_type_svn = render(widget, vocab.BZR_SVN, current_value,
>+                                   'SVN')
>+        self.rcs_type_git = render(widget, vocab.GIT, current_value)
>+        self.rcs_type_hg = render(widget, vocab.HG, current_value)
>+        self.rcs_type_bzr = render(widget, vocab.BZR, current_value)
>+        self.rcs_type_emptymarker = widget._emptyMarker()
>+
>+        widget = self.widgets['branch_type']
>+        current_value = widget._getFormValue()
>+        vocab = widget.vocabulary
>+
>+        (self.branch_type_link,
>+         self.branch_type_create,
>+         self.branch_type_import) = [
>+            render(widget, vocab.by_value[value], current_value)
>+            for value in (LINK_LP_BZR, CREATE_NEW, IMPORT_EXTERNAL)]
>+
>+
>     def _validateLinkLpBzr(self, data):
>         """Validate data for link-lp-bzr case."""
>         if 'branch_location' not in data:
>=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
>--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2010-03-25 22:35:41 +0000
>+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2010-04-02 19:35:10 +0000
>@@ -190,11 +193,11 @@
>     ...     print error
>     ('repo_url'...'The URI scheme "svn" is not allowed.  Only
>     URIs with the following schemes may be used: git, http, https'))
>-    You must set the external repository URL.
> 
> But Git branches may use git.
> 
>     >>> series = factory.makeProductSeries(name="chevette", product=product)
>+    >>> chevette = series



Variable unused. Lint is falling down on the job.



>     >>> transaction.commit()
>     >>> form = {
>     ...     'field.branch_type': 'import-external',
>@@ -311,3 +316,24 @@
>     Code import created and branch linked to the series.
>     >>> print series.branch.name
>     corvair-branch
>+
>+Attempting to import a location that has already been imported results
>+in an error.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'GIT',
>+    ...     'field.branch_name': 'chevette-branch-dup',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'git://github.com/chevette',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)


Line too long.


>+    >>> for error in view.errors:
>+    ...     print error
>+    <BLANKLINE>
>+    This foreign branch URL is already specified for
>+    the imported branch <a href="http://code.launchpad.dev/~.../chevy/chevette-branch">~.../chevy/chevette-branch</a>.
>+
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>
>=== added file 'lib/lp/registry/stories/productseries/xx-productseries-set-branch.txt'
>--- lib/lp/registry/stories/productseries/xx-productseries-set-branch.txt	1970-01-01 00:00:00 +0000
>+++ lib/lp/registry/stories/productseries/xx-productseries-set-branch.txt	2010-04-02 19:35:10 +0000
>@@ -0,0 +1,147 @@
>+Setting the branch for a product series
>+=======================================
>+
>+A product series should have a branch set for it.  The branch can be
>+hosted on Launchpad or somewhere else.  Foreign branches can be in
>+Bazaar, Git, Mercurial, Subversion, or CVS.  Though internally
>+Launchpad treats those scenarios differently we provide a single page
>+to the user to set up the branch.
>+
>+At present, the unified page for setting up the branch is not linked
>+from anywhere, so it must be navigated to directly.
>+
>+    >>> browser = setupBrowser(auth="Basic <email address hidden>:test")
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+
>+The default choice for the type of branch to set is one that
>+already exists on Launchpad.
>+
>+    >>> print_radio_button_field(browser.contents, 'branch_type')
>+    (*) Link to a Bazaar branch already on Launchpad
>+    ( ) Create a new, empty branch in Launchpad and link to this series
>+    ( ) Import a branch hosted somewhere else
>+
>+
>+Linking to an existing branch
>+-----------------------------
>+
>+A user can choose to link to an existing branch on Launchpad.
>+
>+    >>> login('<email address hidden>')
>+    >>> from zope.component import getUtility
>+    >>> from lp.registry.interfaces.product import IProductSet
>+    >>> productset = getUtility(IProductSet)
>+    >>> firefox = productset.getByName('firefox')
>+    >>> branch = factory.makeBranch(name="firefox-hosted-branch", product=firefox)
>+    >>> branch_name = branch.unique_name
>+    >>> logout()
>+
>+    >>> browser.getControl(name='field.branch_location').value = branch_name
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    Series code location updated.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+
>+
>+Creating a new branch
>+---------------------
>+
>+A brand new, empty branch on Launchpad can be created and set as the
>+series branch.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Create a new, empty branch in Launchpad and link to this series').click()


Since getControl() does a substring match, you could shorten this line
to something that would still be unique.



>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    There is 1 error.
>+    Required input is missing.
>+
>+However in order to create the branch the name and owner must be
>+specified.  The owner is a pre-populated dropdown list so the default
>+can be used.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Create a new, empty branch in Launchpad and link to this series').click()


Same here.


>+    >>> browser.getControl(name='field.branch_name').value = 'new-firefox-branch'
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    New branch created and linked to the series.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+
>+
>+Linking to an external branch
>+-----------------------------
>+
>+An external branch can be linked.  The branch can be a Bazaar branch
>+or be a Git, Mercurial, Subversion, or CVS branch.
>+
>+Each of these types must provide the URL of the external repository,
>+the branch name to use in Launchpad, and the branch owner.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Import a branch hosted somewhere else').click()
>+    >>> browser.getControl('Branch name').value = 'bzr-firefox-branch'
>+    >>> browser.getControl('Bazaar', index=0).click()
>+    >>> browser.getControl('Branch URL').value = 'https://bzr.example.com/branch'
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    Series code location updated.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+
>+The process is the same for a Git external branch, though the novel
>+"git://" scheme can also be used.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Import a branch hosted somewhere else').click()
>+    >>> browser.getControl('Branch name').value = 'git-firefox-branch'
>+    >>> browser.getControl('Git').click()
>+    >>> browser.getControl('Branch URL').value = 'git://git.example.com/branch'
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    Code import created and branch linked to the series.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+
>+Likewise Subversion can use the "svn://" scheme.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Import a branch hosted somewhere else').click()
>+    >>> browser.getControl('Branch name').value = 'svn-firefox-branch'
>+    >>> browser.getControl('SVN').click()
>+    >>> browser.getControl('Branch URL').value = 'svn://svn.example.com/branch'
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    Code import created and branch linked to the series.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+
>+The branch owner can be the logged in user or one of her teams.
>+
>+    >>> browser.open('http://launchpad.dev/firefox/trunk/+setbranch')
>+    >>> browser.getControl('Import a branch hosted somewhere else').click()
>+    >>> browser.getControl('Branch name').value = 'hg-firefox-branch'
>+    >>> browser.getControl('Mercurial').click()
>+    >>> browser.getControl('Branch URL').value = 'http://hg.example.com/branch'
>+    >>> browser.getControl('Branch owner').value = ['hwdb-team']
>+    >>> browser.getControl('Update').click()
>+    >>> for message in get_feedback_messages(browser.contents):
>+    ...     print extract_text(message)
>+    Code import created and branch linked to the series.
>+    >>> print browser.url
>+    http://launchpad.dev/firefox/trunk
>+    >>> login('<email address hidden>')
>+    >>> firefox_trunk = firefox.getSeries('trunk')
>+    >>> print firefox_trunk.branch.unique_name
>+    ~hwdb-team/firefox/hg-firefox-branch
>+    >>> print firefox_trunk.branch.owner.name
>+    hwdb-team
>+    >>> logout()
>