Launchpad itself

lib/lp/app/templates/base-layout-macros.pt (+5/-2)
lib/lp/code/browser/bazaar.py (+3/-1)
lib/lp/code/browser/branch.py (+3/-2)
lib/lp/code/browser/configure.zcml (+0/-7)
lib/lp/code/interfaces/codeimport.py (+0/-10)
lib/lp/code/javascript/tests/test_productseries_setbranch.js (+3/-3)
lib/lp/code/model/codeimport.py (+1/-49)
lib/lp/code/model/tests/test_codeimport.py (+0/-194)
lib/lp/code/stories/branches/xx-bazaar-home.txt (+1/-1)
lib/lp/code/stories/branches/xx-branchmergeproposals.txt (+4/-1)
lib/lp/code/stories/branches/xx-propose-for-merging.txt (+2/-0)
lib/lp/code/stories/codeimport/xx-codeimport-list.txt (+0/-72)
lib/lp/code/stories/codeimport/xx-codeimport-view.txt (+3/-3)
lib/lp/code/templates/bazaar-index.pt (+1/-1)
lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt (+4/-0)
lib/lp/registry/browser/configure.zcml (+7/-0)
lib/lp/registry/browser/productseries.py (+380/-24)
lib/lp/registry/browser/tests/productseries-setbranch-view.txt (+339/-0)
lib/lp/registry/stories/productseries/xx-productseries-set-branch.txt (+147/-0)
lib/lp/registry/templates/productseries-codesummary.pt (+3/-3)
lib/lp/registry/templates/productseries-linkbranch.pt (+38/-2)
lib/lp/registry/templates/productseries-setbranch.pt (+129/-0)
lib/lp/testing/factory.py (+4/-4)

To merge this branch:

bzr merge lp://staging/~bac/launchpad/bug-524302

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	code, ui	2010-03-25	Approve on 2010-04-06
Edwin Grubbs (community)	code ui*		Approve on 2010-04-03
Review via email: mp+22180@code.staging.launchpad.net

Commit message

Create a productseries/+setbranch page for setting/creating/importing a branch for the productseries.

Description of the change

Add productseries/+setbranch view to consolidate many other views dealing with creating/mirroring/importing branches. This view allows a user to do one of those things (though the terminology is hidden) and links the branch to the product series.

The view is not currently navigable from anywhere. Eventually it will replace +linkbranch.

A new view test has been created:

bin/test -vvt productseries-setbranch-views.txt

It may be preferred to roll that test into the productseries-views.txt test but it was expeditious to create it stand alone.

There are some known issues listed in the BRANCH.TODO file.

To demo, create a new project and go to https://launchpad.dev/<newproject>/trunk/+setbranch

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2010-03-30:

Download full text (31.1 KiB)

Hi Brad,

This interface is a nice improvement. I've only set the series for a
branch once before, and I totally did it wrong because I was on the wrong
form.

Since I don't know if you are planning a followup branch for your
BRANCH.TODO items, I'm marking this:
needs-fixing

It seems odd that productseries-setbranch.pt is in lp.registry but
productseries-setbranch.js is in lp.code. There are some more
comments below.

-Edwin

>=== modified file 'BRANCH.TODO'
>--- BRANCH.TODO 2010-03-19 07:13:15 +0000
>+++ BRANCH.TODO 2010-03-26 15:28:25 +0000
>@@ -2,3 +2,10 @@
> # landing. There is a test to ensure it is empty in trunk. If there is
> # stuff still here when you are ready to land, the items should probably
> # be converted to bugs so they can be scheduled.
>+
>+TODO:
>+
>+* validation errors give misleading messages
>+* uncaught constraint error on duplicate of code import URL
>+* code.lp.dev/proj/series displays the overview page but it should
>+ direct away from the code vhost
>=== modified file 'lib/lp/registry/browser/productseries.py'
>--- lib/lp/registry/browser/productseries.py 2010-03-23 00:39:45 +0000
>+++ lib/lp/registry/browser/productseries.py 2010-03-26 15:28:25 +0000
>@@ -644,7 +658,340 @@
> self.next_url = canonical_url(product)
>
>
>-class ProductSeriesLinkBranchView(LaunchpadEditFormView):
>+LINK_LP_BZR = 'link-lp-bzr'
>+CREATE_NEW = 'create-new'
>+IMPORT_EXTERNAL = 'import-external'
>+
>+
>+def _getBranchTypeVocabulary():
>+ items = (
>+ (LINK_LP_BZR,
>+ _("Link to a Bazaar branch already on Launchpad")),
>+ (CREATE_NEW,
>+ _("Create a new, empty branch in Launchpad and "
>+ "link to this series")),
>+ (IMPORT_EXTERNAL,
>+ _("Import a branch hosted somewhere else")),
>+ )
>+ terms = [
>+ SimpleTerm(name, name, label) for name, label in items]
>+ return SimpleVocabulary(terms)

Why is this a function instead of a constant? If you are
trying to avoid extra variables defined in the module, you could
just do:
  BRANCH_TYPE_VOCABULARY = SimpleVocabulary((
      SimpleTerm(LINK_LP_BZR, LINK_LP_BZR, 'foo'),
      ...

Hi Brad,

This interface is a nice improvement. I've only set the series for a
branch once before, and I totally did it wrong because I was on the wrong
form.

Since I don't know if you are planning a followup branch for your
BRANCH.TODO items, I'm marking this:
    needs-fixing

It seems odd that productseries-setbranch.pt is in lp.registry but
productseries-setbranch.js is in lp.code. There are some more
comments below.

-Edwin

>=== modified file 'BRANCH.TODO'
>--- BRANCH.TODO	2010-03-19 07:13:15 +0000
>+++ BRANCH.TODO	2010-03-26 15:28:25 +0000
>@@ -2,3 +2,10 @@
> # landing. There is a test to ensure it is empty in trunk. If there is
> # stuff still here when you are ready to land, the items should probably
> # be converted to bugs so they can be scheduled.
>+
>+TODO:
>+
>+* validation errors give misleading messages
>+* uncaught constraint error on duplicate of code import URL
>+* code.lp.dev/proj/series displays the overview page but it should
>+    direct away from the code vhost
>=== modified file 'lib/lp/registry/browser/productseries.py'
>--- lib/lp/registry/browser/productseries.py	2010-03-23 00:39:45 +0000
>+++ lib/lp/registry/browser/productseries.py	2010-03-26 15:28:25 +0000
>@@ -644,7 +658,340 @@
>         self.next_url = canonical_url(product)
> 
> 
>-class ProductSeriesLinkBranchView(LaunchpadEditFormView):
>+LINK_LP_BZR = 'link-lp-bzr'
>+CREATE_NEW = 'create-new'
>+IMPORT_EXTERNAL = 'import-external'
>+
>+
>+def _getBranchTypeVocabulary():
>+    items = (
>+        (LINK_LP_BZR,
>+         _("Link to a Bazaar branch already on Launchpad")),
>+        (CREATE_NEW,
>+         _("Create a new, empty branch in Launchpad and "
>+           "link to this series")),
>+        (IMPORT_EXTERNAL,
>+         _("Import a branch hosted somewhere else")),
>+        )
>+    terms = [
>+        SimpleTerm(name, name, label) for name, label in items]
>+    return SimpleVocabulary(terms)

>+class RevisionControlSystemsExtended(RevisionControlSystems):
>+    """External RCS plus Bazaar."""
>+    BZR = DBItem(99, """
>+        Bazaar
>+
>+        External Bazaar branch.
>+        """)
>+
>+
>+class SetBranchForm(Interface):
>+    """The fields presented on the form for setting a branch."""
>+
>+    use_template(
>+        ICodeImport,
>+        ['cvs_module'])
>+
>+    rcs_type = Choice(title=_("Type of RCS"),
>+        required=False, vocabulary=RevisionControlSystemsExtended,
>+        description=_(
>+            "The version control system to import from. "))
>+
>+    repo_url = URIField(
>+        title=_("Branch URL"), required=True,
>+        description=_("The URL of the branch."),
>+        allowed_schemes=["http", "https"],
>+        allow_userinfo=False,
>+        allow_port=True,
>+        allow_query=False,
>+        allow_fragment=False,
>+        trailing_slash=False)
>+
>+    branch_location = copy_field(
>+        IProductSeries['branch'],
>+        __name__='branch_location',
>+        title=_('Branch'),
>+        description=_(
>+            "The Bazaar branch for this series in Launchpad, "
>+            "if one exists."),
>+        )
>+
>+    branch_type = Choice(
>+        title=_('Import type'),
>+        vocabulary=_getBranchTypeVocabulary(),
>+        description=_("The type of import"),
>+        required=True)
>+
>+    branch_name = copy_field(
>+        IBranch['name'],
>+        __name__='branch_name',
>+        title=_('Branch name'),
>+        description=_(''),
>+        required=True,
>+        )
>+
>+    branch_owner = copy_field(
>+        IBranch['owner'],
>+        __name__='branch_owner',
>+        title=_('Branch owner'),
>+        description=_(''),
>+        required=True,
>+        )
>+
>+
>+class ProductSeriesSetBranchView(LaunchpadFormView, ProductSeriesView,
>+                                 BranchNameValidationMixin):
>+    """The view to set a branch for the ProductSeries."""
>+
>+    schema = SetBranchForm
>+    # Set for_input to True to ensure fields marked read-only will be editable
>+    # upon creation.
>+    for_input = True
>+
>+    custom_widget('rcs_type', LaunchpadRadioWidget)
>+    custom_widget('branch_type', LaunchpadRadioWidget)
>+    initial_values = {
>+        'rcs_type': RevisionControlSystemsExtended.BZR,
>+        'branch_type': LINK_LP_BZR,
>+        }
>+
>+    def setUpWidgets(self):
>+        super(ProductSeriesSetBranchView, self).setUpWidgets()
>+
>+        # Extract the radio buttons from the rcs_type widget, so we can
>+        # display them separately in the form.
>+        soup = BeautifulSoup(self.widgets['rcs_type']())
>+        fields = soup.findAll('input')
>+        [cvs_button, svn_button, git_button, hg_button,
>+         bzr_button, empty_marker] = [
>+            field for field in fields
>+            if field.get('value') in ['CVS', 'BZR_SVN', 'GIT', 'HG',
>+                                      'BZR', '1']]
>+        # The following attributes are used only in the page template.
>+        self.rcs_type_cvs = str(cvs_button)
>+        self.rcs_type_svn = str(svn_button)
>+        self.rcs_type_git = str(git_button)
>+        self.rcs_type_hg = str(hg_button)
>+        self.rcs_type_bzr = str(bzr_button)
>+        self.rcs_type_emptymarker = str(empty_marker)

It seems like this could be simplified like this:
        def render(term, current_value, count):
            if term.value == current_value:
                render = self.renderSelectedItem
            else:
                render = self.renderItem
            rendered_item = render(count,
                self.textForValue(term)
                term.token,
                self.name,
                cssClass)

current_value = self.widgets['rcs_type']._getFormValue()
        vocab = self.widgets['rcs_type'].vocabulary
        self.rcs_type_cvs = render(vocab.CVS, current_value, 0)
        self.rcs_type_svn = render(vocab.BZR_SVN, current_value, 1)
        ...
        self.rcs_type_emptymarker = self.widgets['rcs_type']._emptyMarker()

or with all the rendering in the template as:
    <input name="field.rcs_type" type="radio" class="radioType"
        tal:attributes="value view/rcs_type_vocabulary/CVS/token">

>
>+
>+        soup = BeautifulSoup(self.widgets['branch_type']())
>+        fields = soup.findAll('input')
>+        (link_button, create_button, import_button, emptymarker) = fields
>+        self.branch_type_link = str(link_button)
>+        self.branch_type_create = str(create_button)
>+        self.branch_type_import = str(import_button)
>+        self.branch_type_emptymarker = str(emptymarker)

Same here.

>+
>+    def _validateLinkLpBzr(self, data):
>+        """Validate data for link-lp-bzr case."""
>+        if 'branch_location' not in data:
>+            self.setFieldError(
>+                'branch_location',
>+                'The branch location must be set.')
>+
>+    def _validateCreateNew(self, data):
>+        """Validate data for create new case."""
>+        self._validateBranch(data)
>+
>+    def validateImportExternal(self, data):
>+        """Validate data for import external case."""
>+        rcs_type = data.get('rcs_type')
>+        repo_url = data.get('repo_url')
>+
>+        if repo_url is None:
>+            self.setFieldError('repo_url',
>+                               'You must set the external repository URL.')

This overwrites the error message from the URIField, so you won't see:
    The URI scheme "svn" is not allowed. Only URIs with the following
    schemes may be used: git, http, https
I'm not sure why your view tests aren't discovering that.

>+
>+        # RCS type is mandatory.
>+        # This condition should never happen since an initial value is set.
>+        if rcs_type is None:
>+            # The error shows but does not identify the widget.
>+            self.setFieldError(
>+                'rcs_type',
>+                'You must specify the type of RCS for the remote host.')
>+        elif rcs_type == RevisionControlSystemsExtended.CVS:
>+            if 'cvs_module' not in data:
>+                self.setFieldError(
>+                    'cvs_module',
>+                    'The CVS module must be set.')
>+        self._validateBranch(data)
>+
>+    def _validateBranch(self, data):
>+        """Validate that branch name and owner are set."""
>+        if 'branch_name' not in data:
>+            self.setFieldError(
>+                'branch_name',
>+                'The branch name must be set.')
>+        if 'branch_owner' not in data:
>+            self.setFieldError(
>+                'branch_owner',
>+                'The branch owner must be set.')
>+
>+    def _setRequired(self, names, value):
>+        """Mark the widget field as optional."""
>+        for name in names:
>+            widget = self.widgets[name]
>+            # The 'required' property on the widget context is set to False.
>+            # The widget also has a 'required' property but it isn't used
>+            # during validation.
>+            widget.context.required = value
>+
>+    def _validSchemes(self, rcs_type):
>+        """Return the valid schemes for the repository URL."""
>+        schemes = set(['http', 'https'])
>+        # Extend the allowed schemes for the repository URL based on
>+        # rcs_type.
>+        extra_schemes = {
>+            RevisionControlSystemsExtended.BZR_SVN:['svn'],
>+            RevisionControlSystemsExtended.GIT:['git'],
>+            }
>+        schemes.update(extra_schemes.get(rcs_type, []))
>+        return schemes
>+
>+    def validate_widgets(self, data, names=None):
>+        """See `LaunchpadFormView`."""
>+        names = ['branch_type', 'rcs_type']
>+        super(ProductSeriesSetBranchView, self).validate_widgets(data, names)
>+        branch_type = data.get('branch_type')
>+        if branch_type == LINK_LP_BZR:
>+            # Mark other widgets as non-required.
>+            self._setRequired(['rcs_type', 'repo_url', 'cvs_module',
>+                               'branch_name', 'branch_owner'], False)
>+        elif branch_type == CREATE_NEW:
>+            self._setRequired(['branch_location', 'rcs_type', 'cvs_module'],
>+                              False)
>+        elif branch_type == IMPORT_EXTERNAL:
>+            rcs_type = data.get('rcs_type')
>+
>+            # Set the valid schemes based on rcs_type.
>+            self.widgets['repo_url'].field.allowed_schemes = (
>+                self._validSchemes(rcs_type))
>+            # The branch location is not required for validation.
>+            self._setRequired(['branch_location'], False)
>+            # The cvs_module is required if it is a CVS import.
>+            if rcs_type == RevisionControlSystemsExtended.CVS:
>+                self._setRequired(['cvs_module'], True)
>+        else:
>+            raise AssertionError("Unknown branch type %s" % branch_type)
>+        # Perform full validation now.
>+        super(ProductSeriesSetBranchView, self).validate_widgets(data)
>+
>+    def validate(self, data):
>+        """See `LaunchpadFormView`."""
>+        branch_type = data['branch_type']
>+        if branch_type == IMPORT_EXTERNAL:
>+            self.validateImportExternal(data)
>+        elif branch_type == LINK_LP_BZR:
>+            self._validateLinkLpBzr(data)
>+        elif branch_type == CREATE_NEW:
>+            self._validateCreateNew(data)
>+        else:
>+            raise AssertionError("Unknown branch type %s" % branch_type)
>+
>+    @property
>+    def target(self):
>+        """The branch target for the context."""
>+        return IBranchTarget(self.context)
>+
>+    @action(_('Update'), name='update')
>+    def update_action(self, action, data):
>+        self.next_url = canonical_url(self.context)
>+        branch_type = data.get('branch_type')
>+        if branch_type == LINK_LP_BZR:
>+            branch_location = data.get('branch_location')
>+            if branch_location != self.context.branch:
>+                self.context.branch = branch_location
>+                # Request an initial upload of translation files.
>+                getUtility(IRosettaUploadJobSource).create(
>+                    self.context.branch, NULL_REVISION)
>+            else:
>+                self.context.branch = branch_location
>+            self.request.response.addInfoNotification(
>+                'Series code location updated.')
>+        else:
>+            branch_name = data.get('branch_name')
>+            branch_owner = data.get('branch_owner')
>+
>+            # Create a new branch.
>+            if branch_type == CREATE_NEW:
>+                branch = self._createBzrBranch(
>+                    BranchType.HOSTED, branch_name, branch_owner)
>+                if branch is not None:
>+                    self.context.branch = branch
>+                    self.request.response.addInfoNotification(
>+                        'New branch created and linked to the series.')
>+
>+            # Import or mirror an external branch.
>+            elif branch_type == IMPORT_EXTERNAL:
>+                # Either create an externally hosted bzr branch
>+                # (a.k.a. 'mirrored') or create a new code import.
>+                rcs_type = data.get('rcs_type')
>+                if rcs_type == RevisionControlSystemsExtended.BZR:
>+                    branch = self._createBzrBranch(
>+                        BranchType.MIRRORED, branch_name, branch_owner,
>+                        data['repo_url'])
>+                    if branch is not None:
>+                        self.context.branch = branch
>+                        self.request.response.addInfoNotification(
>+                            'Mirrored branch created and linked to '
>+                            'the series.')
>+                else:
>+                    # We need to create an import request.
>+                    if rcs_type == RevisionControlSystemsExtended.CVS:
>+                        cvs_root = data.get('repo_url')
>+                        cvs_module = data.get('cvs_module')
>+                        url = None
>+                    else:
>+                        cvs_root = None
>+                        cvs_module = None
>+                        url = data.get('repo_url')
>+                    code_import = getUtility(ICodeImportSet).new(
>+                        registrant=self.user,
>+                        target=self.target,
>+                        branch_name=branch_name,
>+                        rcs_type=RevisionControlSystems.items[rcs_type.name],
>+                        url=url,
>+                        cvs_root=cvs_root,
>+                        cvs_module=cvs_module)
>+                    self.context.branch = code_import.branch
>+                    self.request.response.addInfoNotification(
>+                        'Code import created and branch linked to the series.')
>+            else:
>+                raise UnexpectedFormData(branch_type)
>+
>+    def _createBzrBranch(self, branch_type, branch_name,
>+                         branch_owner, repo_url=None):
>+        """Create a new Bazaar branch.  It may be hosted or mirrored.
>+
>+        Return the branch on success or None.
>+        """
>+        branch = None
>+        try:
>+            namespace = self.target.getNamespace(branch_owner)
>+            branch = namespace.createBranch(branch_type=branch_type,
>+                                            name=branch_name,
>+                                            registrant=self.user,
>+                                            url=repo_url)
>+            if branch_type == BranchType.MIRRORED:
>+                branch.requestMirror()
>+        except BranchCreationForbidden:
>+            self.addError(
>+                "You are not allowed to create branches in %s." %
>+                self.context.displayname)
>+        except BranchExists, e:
>+            self._setBranchExists(e.existing_branch, 'branch_name')
>+        return branch
>+
>+    @property
>+    def cancel_url(self):
>+        """See `LaunchpadFormView`."""
>+        return canonical_url(self.context)
>+
>+
>+class ProductSeriesLinkBranchView(LaunchpadEditFormView, ProductSeriesView):
>     """View to set the bazaar branch for a product series."""
> 
>     schema = IProductSeries
>
>=== added file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
>--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	1970-01-01 00:00:00 +0000
>+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2010-03-26 15:28:25 +0000
>@@ -0,0 +1,313 @@
>+    >>> from canonical.launchpad.testing.pages import find_tag_by_id
>+

This looks weird at the very top of the file. Can you move that
somewhere below the first heading?

>+Set branch
>+----------
>+
>+The productseries +setbranch view allows the user to set a branch for
>+this series.  The branch can one that already exists in Launchpad, or

Missing verb: "can one that"

>+a new branch in Launchpad can be defined, or it can be a repository
>+that exists externally in a variety of version control systems.
>+
>+    >>> product = factory.makeProduct(name="chevy")
>+    >>> series = factory.makeProductSeries(name="impala", product=product)
>+    >>> transaction.commit()
>+    >>> login_person(product.owner)
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner)
>+    >>> print find_tag_by_id(view.render(), 'maincontent')
>+    <div...
>+    Link to a Bazaar branch already in Launchpad...
>+    Create a new, empty branch in Launchpad and link to this series...
>+    Import a branch hosted somewhere else...
>+    ...Branch name:...
>+    ...Branch owner:...
>+
>+
>+Linking to an existing branch
>+-----------------------------
>+
>+If linking to an existing branch is selected then the branch location must be provided.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'link-lp-bzr',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for error in view.errors:
>+    ...     print error
>+    The branch location must be set.
>+
>+Setting the branch location to an invalid branch results in another
>+validation error.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'link-lp-bzr',
>+    ...     'field.branch_location': 'foo',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for error in view.errors:
>+    ...     print error
>+    ('Invalid value', InvalidValue("token 'foo' not found in vocabulary"))
>+    The branch location must be set.
>+
>+Providing a valid branch results in a successful linking.
>+
>+    >>> series.branch is None
>+    True
>+    >>> branch = factory.makeBranch(name='impala-branch', owner=product.owner, product=product)

Wrap line.

>+    >>> form = {
>+    ...     'field.branch_type': 'link-lp-bzr',
>+    ...     'field.branch_location': branch.unique_name,
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Series code location updated.
>+
>+    >>> print series.branch.name
>+    impala-branch
>+
>+
>+Creating a new branch
>+---------------------
>+
>+When creating a new branch the branch name and owner must be specified.
>+
>+    >>> series = factory.makeProductSeries(name="camaro", product=product)
>+    >>> transaction.commit()
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'create-new',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>+    The branch name must be set.
>+    The branch owner must be set.
>+
>+    >>> from lp.registry.interfaces.person import IPersonSet
>+    >>> mark = getUtility(IPersonSet).getByEmail('mark@example.com')
>+    >>> form = {
>+    ...     'field.branch_type': 'create-new',
>+    ...     'field.branch_name': 'camaro-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    New branch created and linked to the series.
>+    >>> print series.branch.name
>+    camaro-branch
>+
>+
>+Import a branch hosted elsewhere
>+--------------------------------
>+
>+Importing an externally hosted branch can either be a mirror, if a
>+Bazaar branch, or an import, if a git, hg, cvs, or svn branch.
>+
>+Lots of data are required to create an import.
>+
>+    >>> series = factory.makeProductSeries(name="blazer", product=product)
>+    >>> transaction.commit()
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)

Wrap line.

>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>+    You must set the external repository URL.
>+    You must specify the type of RCS for the remote host.
>+    The branch name must be set.
>+    The branch owner must be set.
>+
>+For Bazaar branches the scheme may only be http or https.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'BZR',
>+    ...     'field.branch_name': 'blazer-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'bzr://bzr.com/foo',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>+    ('repo_url'...The URI scheme "bzr" is not allowed.  Only URIs with the following schemes may be

Wrap line.

>+    used: http, https'))
>+    You must set the external repository URL.
>+
>+A correct URL is accepted.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'BZR',
>+    ...     'field.branch_name': 'blazer-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'http://bzr.com/foo',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Mirrored branch created and linked to the series.
>+    >>> print series.branch.name
>+    blazer-branch
>+
>+Git branches cannnot use svn.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'GIT',
>+    ...     'field.branch_name': 'chevette-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'svn://svn.com/chevette',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     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)
>+    >>> transaction.commit()
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'GIT',
>+    ...     'field.branch_name': 'chevette-branch',
>+    ...     '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)
>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Code import created and branch linked to the series.
>+    >>> print series.branch.name
>+    chevette-branch
>+
>+But Subversion branches cannnot use git.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'BZR_SVN',
>+    ...     'field.branch_name': 'suburban-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'git://github.com/suburban',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>+    ('repo_url'...'The URI scheme "git" is not allowed.  Only
>+    URIs with the following schemes may be used: http, https, svn'))
>+    You must set the external repository URL.
>+
>+But Subversion branches may use svn as the scheme.
>+
>+    >>> series = factory.makeProductSeries(name="suburban", product=product)
>+    >>> transaction.commit()
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'BZR_SVN',
>+    ...     'field.branch_name': 'suburban-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'svn://svn.com/suburban',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Code import created and branch linked to the series.
>+    >>> print series.branch.name
>+    suburban-branch
>+
>+Mercurial branches must use http or https as the scheme.
>+
>+    >>> series = factory.makeProductSeries(name="malibu", product=product)
>+    >>> transaction.commit()
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'HG',
>+    ...     'field.branch_name': 'malibu-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'https://mercurial.com/branch',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Code import created and branch linked to the series.
>+    >>> print series.branch.name
>+    malibu-branch
>+
>+CVS branches must use http or https as the scheme and must have the
>+CVS module field specified.
>+
>+    >>> series = factory.makeProductSeries(name="corvair", product=product)
>+    >>> transaction.commit()
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'CVS',
>+    ...     'field.branch_name': 'corvair-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'https://cvs.com/branch',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>+    The CVS module must be set.
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'import-external',
>+    ...     'field.rcs_type': 'CVS',
>+    ...     'field.branch_name': 'corvair-branch',
>+    ...     'field.branch_owner': product.owner.name,
>+    ...     'field.repo_url': 'https://cvs.com/branch',
>+    ...     'field.cvs_module': 'root',
>+    ...     'field.actions.update': 'Update',
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for error in view.errors:
>+    ...     print error
>+    >>> for notification in view.request.response.notifications:
>+    ...     print notification.message
>+    Code import created and branch linked to the series.
>+    >>> print series.branch.name
>+    corvair-branch
>
>=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
>--- lib/lp/registry/browser/tests/productseries-views.txt	2010-03-10 22:14:10 +0000
>+++ lib/lp/registry/browser/tests/productseries-views.txt	2010-03-26 15:28:25 +0000
>@@ -463,3 +463,34 @@
>     You cannot delete a series that is linked to packages in distributions.
>     You can remove the links from the <a ...>project packaging</a> page.
>     This series cannot be deleted because it has translations.
>+
>+
>+Set branch
>+----------
>+
>+The productseries +setbranch view allows the user to set a branch for
>+this series.  The branch can one that already exists in Launchpad, or

Missing verb. Actually, do you need an extra test in this file, if
you have a bunch of tests in productseries-setbranch-view.txt.

>+a new branch in Launchpad can be defined, or it can be a repository
>+that exists externally in a variety of version control systems.
>+
>+    >>> product = factory.makeProduct(name="roomba")
>+    >>> series = factory.makeProductSeries(name="rumba", product=product)
>+    >>> login_person(product.owner)
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner)
>+    >>> print find_tag_by_id(view.render(), 'maincontent')
>+    <div...
>+    Link to a Bazaar branch already in Launchpad...
>+    Create a new, empty branch in Launchpad and link to this series...
>+    Import a branch hosted somewhere else...
>+    ...Branch name:...
>+    ...Branch owner:...
>+
>+    >>> form = {
>+    ...     'field.branch_type': 'link-lp-bzr'
>+    ...     'field.action.update' 'Update'
>+    ...     }
>+    >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
>+    >>> for notification in view.request.notifications:
>+    ...     print notification.message
>+    >>> for error in view.errors:
>+    ...     print error
>

review: Needs Fixing

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-02:

Thanks for the excellent review Edwin. I've incorporated all of your suggestions. Having the view create the rendered items was a little more difficult due to the two different types of vocabularies.

I also took care of the items in my BRANCH.TODO list including not masking widget validation errors (which you also noted) and catching an error condition when an import URL has been requested before to avoid a db IntegrityError.

Finally I added a story test to show the high-level working of the new page.

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-02:

Incremental diff at http://pastebin.ubuntu.com/408292/

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2010-04-03:

Download full text (19.5 KiB)

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.

Hi Brad,

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

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.

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 test@canonical.com: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('test@canonical.com')
>+    >>> 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('test@canonical.com')
>+    >>> 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()
>

review: Approve (code ui*)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-05:

Edwin,

Thanks for the review and the ideas about cleaning up the render() method.

I am going to defer the other two items for another, quick follow-on branch.

The incremental is at:
http://pastebin.ubuntu.com/409614/

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-05:

Curtis a screenshot is available at http://people.canonical.com/~bac/setbranch.png

Revision history for this message

Curtis Hovey (sinzui) wrote on 2010-04-05:

Hi Brad.

I think Branch name and owner are confusing. I think they subordinate to importing or creating a new branch but they appear to be enabled for setting the series to an existing branch. I know I cannot change the owner or name for the first option. I expect these two options to be, and appear to be, disbaled when I choose Link to a bazaar branch in Launchpad.

I do not see any test to verify the script is loaded on the page. Either we need to show that the
script is in the page or use windmill to verify it executed.

review: Needs Information (code, ui)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-06:

As we discussed, the branch name and owner are conditionally disabled correctly, though the screen shot does not show it well.

The extra windmill test will be added to the follow up branch.

Revision history for this message

Curtis Hovey (sinzui) on 2010-04-06:

review: Approve (code, ui)