Tim Penhey wrote:
> Review: Approve conditional
>   review approve conditional
>   merge approved
> 
> Just some minor tweaks, but on the whole, awesome!

Thanks.

>> === modified file 'lib/lp/code/browser/branch.py'
>> --- lib/lp/code/browser/branch.py	2009-12-07 02:11:31 +0000
>> +++ lib/lp/code/browser/branch.py	2009-12-08 03:10:29 +0000
>> @@ -79,7 +79,8 @@
>>  from lp.code.browser.branchmergeproposal import (
>>      latest_proposals_for_each_branch)
>>  from lp.code.enums import (
>> -    BranchLifecycleStatus, BranchType, UICreatableBranchType)
>> +    BranchLifecycleStatus, BranchType, RevisionControlSystems,
>> +    UICreatableBranchType)
>>  from lp.code.errors import InvalidBranchMergeProposal
>>  from lp.code.interfaces.branch import (
>>      BranchCreationForbidden, BranchExists, IBranch,
>> @@ -501,6 +502,14 @@
>>          return list(self.context.code_import.results[:10])
>>  
>>      @property
>> +    def is_svn_import(self):
>> +        """True if an imported branch is a SVN import."""
>> +        # You should only be calling this if it's an SVN code import
> 
> Well, we should only be calling this if it is an import of any kind.

Uh.  Right.  Fixed.

>> +        assert self.context.code_import
>> +        return self.context.code_import.rcs_type in \
>> +               (RevisionControlSystems.SVN, RevisionControlSystems.BZR_SVN)
>> +
>> +    @property
>>      def svn_url_is_web(self):
>>          """True if an imported branch's SVN URL is HTTP or HTTPS."""
>>          # You should only be calling this if it's an SVN code import
>> === added file 'lib/lp/code/browser/tests/test_codeimport.py'
>> --- lib/lp/code/browser/tests/test_codeimport.py	1970-01-01 00:00:00 +0000
>> +++ lib/lp/code/browser/tests/test_codeimport.py	2009-12-08 03:10:29 +0000
>> @@ -0,0 +1,58 @@
>> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
>> +# GNU Affero General Public License version 3 (see the file LICENSE).
>> +
>> +"""Tests for the code import browser code."""
>> +
>> +__metaclass__ = type
>> +
>> +import re
>> +import unittest
>> +
>> +from canonical.launchpad.testing.pages import extract_text, find_tag_by_id
>> +from canonical.launchpad.webapp import canonical_url
>> +from canonical.testing.layers import DatabaseFunctionalLayer
>> +
>> +from lp.code.enums import RevisionControlSystems
>> +from lp.testing import TestCaseWithFactory
>> +
>> +
>> +class TestImportDetails(TestCaseWithFactory):
>> +
>> +    layer = DatabaseFunctionalLayer
>> +
>> +    def assertSvnDetailsDisplayed(self, svn_details, rcs_type, url):
>> +        """Assert the `svn_details` tag described a Subversion import."""
>> +        self.assertEquals(rcs_type.title, svn_details.span['title'])
>> +        text = re.sub('\s+', ' ', extract_text(svn_details))
>> +        self.assertTrue(
>> +            text.startswith(
>> +                'This branch is an import of the Subversion branch'))
>> +        self.assertEquals(url, svn_details.a['href'])
> 
> I'm not sure I'd add this last assert here as we only show the anchor if
> the subversion url is http.  But the rest looks good.

You're right, the tests are more focused without that assert.

>> +    def test_bzr_svn_import(self):
>> +        # The branch page for a bzr-svn-imported branch contains a summary 
> of
>> +        # the import details.
>> +        bzr_svn_import = self.factory.makeCodeImport(
>> +            rcs_type=RevisionControlSystems.BZR_SVN)
>> +        browser = self.getUserBrowser(canonical_url(bzr_svn_import.branch))
>> +        svn_details = find_tag_by_id(browser.contents, 'svn-import-details')
>> +        self.assertSvnDetailsDisplayed(
>> +            svn_details, RevisionControlSystems.BZR_SVN,
>> +            bzr_svn_import.svn_branch_url)
>> +
>> +    def test_cscvs_svn_import(self):
>> +        # The branch page for a cscvs-imported svn branch contains a 
> summary
>> +        # of the import details.
>> +        bzr_svn_import = self.factory.makeCodeImport(
>> +            rcs_type=RevisionControlSystems.SVN)
>> +        browser = self.getUserBrowser(canonical_url(bzr_svn_import.branch))
>> +        svn_details = find_tag_by_id(browser.contents, 'svn-import-details')
>> +        self.assertSvnDetailsDisplayed(
>> +            svn_details, RevisionControlSystems.SVN,
>> +            bzr_svn_import.svn_branch_url)
>> +
>> +
>> +
>> +def test_suite():
>> +    return unittest.TestLoader().loadTestsFromName(__name__)
>> +
>>
>> === modified file 'lib/lp/code/stories/codeimport/xx-admin-codeimport.txt'
>> --- lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	2009-11-13 
> 10:43:32 +0000
>> +++ lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	2009-12-08 
> 03:10:29 +0000
>> @@ -1,4 +1,5 @@
>> -= Administrating code imports =
>> +Administrating code imports
>> +===========================
>>  
>>  The code import details are displayed on the main branch page for
>>  imported branches.  If the logged in user is an import operator
>> @@ -6,6 +7,7 @@
>>  to edit the details.
>>  
>>      >>> from canonical.launchpad.ftests import ANONYMOUS, login, logout
> 
> Can you please change these to come from lp.testing?

Sure.

>> +    >>> from lp.code.enums import RevisionControlSystems
>>      >>> login('<email address hidden>')
>>  
>>      >>> svn_import = factory.makeCodeImport(
>> @@ -87,7 +97,8 @@
>>      >>> print_form_fields(import_browser)
>>      field.git_repo_url: git://git.example.org/fooix
>>  
>> -=== Editing the import location ===
>> +Editing the import location
>> ++++++++++++++++++++++++++++
> 
> Hmm.. I didn't know about this third level ReST heading.
> I'm assuming it is right :)

Well, ReST doesn't really care.  I think it's the common convention though.

>>  
>>  The +edit-import page allows the import operator to edit the location
>>  an import is from.
>> @@ -102,6 +113,17 @@
>>      ...     print extract_text(message)
>>      The code import has been updated.
>>  
>> +bzr-svn imports,
>> +
> 
> One less line here.

Fixed.

>> +
>> +    >>> import_browser.open(bzr_svn_import_location + '/+edit-import')
>> +    >>> import_browser.getControl('Branch URL').value = \
>> +    ...     'svn://svn-new.example.com/bzr-svn/trunk'
>> +    >>> import_browser.getControl('Update').click()
>> +    >>> for message in get_feedback_messages(import_browser.contents):
>> +    ...     print extract_text(message)
>> +    The code import has been updated.
>> +
>>  CVS imports,
>>  
>>      >>> import_browser.open(cvs_import_location + '/+edit-import')
>> === modified file 'lib/lp/code/stories/codeimport/xx-codeimport-list.txt'
>> --- lib/lp/code/stories/codeimport/xx-codeimport-list.txt	2009-06-15 
> 21:05:06 +0000
>> +++ lib/lp/code/stories/codeimport/xx-codeimport-list.txt	2009-12-08 
> 03:10:29 +0000
>> @@ -9,6 +9,7 @@
>>      >>> from lp.codehosting.codeimport.tests.test_workermonitor import (
>>      ...     nuke_codeimport_sample_data)
>>      >>> from canonical.database.sqlbase import flush_database_updates
>> +    >>> from lp.code.enum import RevisionControlSystems
>>      >>> from lp.code.model.tests.test_codeimport import (
>>      ...     make_active_import)
>>      >>> from lp.code.interfaces.codeimport import ICodeImportSet
>> @@ -21,23 +22,28 @@
>>      ...     factory, product_name="myproject", branch_name="trunk",
>>      ...     svn_branch_url="http://example.com/svn/myproject/trunk",
>>      ...     last_update=datetime.datetime(2007, 1, 1, tzinfo=pytz.UTC))
>> +    >>> active_bzr_svn_import = make_active_import(
>> +    ...     factory, product_name="ourproject", branch_name="trunk",
>> +    ...     svn_branch_url="http://example.com/bzr-svn/myproject/trunk",
>> +    ...     rcs_type=RevisionControlSystems.BZR_SVN,
>> +    ...     last_update=datetime.datetime(2007, 1, 2, tzinfo=pytz.UTC))
>>      >>> active_cvs_import = make_active_import(
>>      ...     factory, product_name="hisproj", branch_name="main",
>>      ...     cvs_root=":pserver:<email address hidden>:/cvs", 
> cvs_module="hisproj",
>> -    ...     last_update=datetime.datetime(2007, 1, 2, tzinfo=pytz.UTC))
>> +    ...     last_update=datetime.datetime(2007, 1, 3, tzinfo=pytz.UTC))
>>      >>> active_git_import = make_active_import(
>>      ...     factory, product_name="herproj", branch_name="master",
>>      ...     git_repo_url="git://git.example.org/herproj",
>> -    ...     last_update=datetime.datetime(2007, 1, 3, tzinfo=pytz.UTC))
>> +    ...     last_update=datetime.datetime(2007, 1, 4, tzinfo=pytz.UTC))
>>      >>> flush_database_updates()
> 
> Can you try this test without this function?  I don't believe it is
> needed any more.

You are right -- and I also found that I clearly hadn't run the test
before submitting it for review!

>>      >>> len(list(code_import_set.getActiveImports()))
>> -    3
>> +    4
>>      >>> logout()
>>  
>>  The page is linked to from the "$N imported branches" text.
>>  
>>      >>> browser.open('http://code.launchpad.dev')
>> -    >>> browser.getLink('3 imported branches').click()
>> +    >>> browser.getLink('4 imported branches').click()
>>      >>> print browser.title
>>      Available code imports
>>  
> 

Changes made, ec2 now chewing on the branch.

Cheers,
mwh