Code review comment for lp:~mwhudson/launchpad/bzr-svn-ui

  review approve conditional
  merge approved

Just some minor tweaks, but on the whole, awesome!

Tim

> === 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.

> + 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.

> + 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?

> + >>> 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 :)

>
> 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.

> +
> + >>> 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.

> >>> 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
>