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.
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' code/browser/ branch. py 2009-12-07 02:11:31 +0000 code/browser/ branch. py 2009-12-08 03:10:29 +0000 browser. branchmergeprop osal import ( proposals_ for_each_ branch) Status, BranchType, UICreatableBran chType) Status, BranchType, RevisionControl Systems, chType) rgeProposal interfaces. branch import ( orbidden, BranchExists, IBranch, context. code_import. results[ :10]) import( self):
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -79,7 +79,8 @@
>> from lp.code.
>> latest_
>> from lp.code.enums import (
>> - BranchLifecycle
>> + BranchLifecycle
>> + UICreatableBran
>> from lp.code.errors import InvalidBranchMe
>> from lp.code.
>> BranchCreationF
>> @@ -501,6 +502,14 @@
>> return list(self.
>>
>> @property
>> + def is_svn_
>> + """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 code_import. rcs_type in \ lSystems. SVN, RevisionControl Systems. BZR_SVN) is_web( self): code/browser/ tests/test_ codeimport. py' code/browser/ tests/test_ codeimport. py 1970-01-01 00:00:00 +0000 code/browser/ tests/test_ codeimport. py 2009-12-08 03:10:29 +0000 launchpad. testing. pages import extract_text, find_tag_by_id launchpad. webapp import canonical_url testing. layers import DatabaseFunctio nalLayer Systems ls(TestCaseWith Factory) : nalLayer sDisplayed( self, svn_details, rcs_type, url): ls(rcs_ type.title, svn_details. span['title' ]) text(svn_ details) ) ls(url, svn_details. a['href' ])
>> + return self.context.
>> + (RevisionContro
>> +
>> + @property
>> def svn_url_
>> """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/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -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.
>> +from canonical.
>> +from canonical.
>> +
>> +from lp.code.enums import RevisionControl
>> +from lp.testing import TestCaseWithFactory
>> +
>> +
>> +class TestImportDetai
>> +
>> + layer = DatabaseFunctio
>> +
>> + def assertSvnDetail
>> + """Assert the `svn_details` tag described a Subversion import."""
>> + self.assertEqua
>> + text = re.sub('\s+', ' ', extract_
>> + self.assertTrue(
>> + text.startswith(
>> + 'This branch is an import of the Subversion branch'))
>> + self.assertEqua
>
> 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): makeCodeImport( RevisionControl Systems. BZR_SVN) wser(canonical_ url(bzr_ svn_import. branch) ) by_id(browser. contents, 'svn-import- details' ) etailsDisplayed ( Systems. BZR_SVN, import. svn_branch_ url) svn_import( self): makeCodeImport( RevisionControl Systems. SVN) wser(canonical_ url(bzr_ svn_import. branch) ) by_id(browser. contents, 'svn-import- details' ) etailsDisplayed ( Systems. SVN, import. svn_branch_ url) TestLoader( ).loadTestsFrom Name(__ name__) code/stories/ codeimport/ xx-admin- codeimport. txt' code/stories/ codeimport/ xx-admin- codeimport. txt 2009-11-13 code/stories/ codeimport/ xx-admin- codeimport. txt 2009-12-08 ======= ======= ======= launchpad. ftests import ANONYMOUS, login, logout
>> + # The branch page for a bzr-svn-imported branch contains a summary
> of
>> + # the import details.
>> + bzr_svn_import = self.factory.
>> + rcs_type=
>> + browser = self.getUserBro
>> + svn_details = find_tag_
>> + self.assertSvnD
>> + svn_details, RevisionControl
>> + bzr_svn_
>> +
>> + def test_cscvs_
>> + # The branch page for a cscvs-imported svn branch contains a
> summary
>> + # of the import details.
>> + bzr_svn_import = self.factory.
>> + rcs_type=
>> + browser = self.getUserBro
>> + svn_details = find_tag_
>> + self.assertSvnD
>> + svn_details, RevisionControl
>> + bzr_svn_
>> +
>> +
>> +
>> +def test_suite():
>> + return unittest.
>> +
>>
>> === modified file 'lib/lp/
>> --- lib/lp/
> 10:43:32 +0000
>> +++ lib/lp/
> 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.
>
> Can you please change these to come from lp.testing?
Sure.
>> + >>> from lp.code.enums import RevisionControl Systems makeCodeImport( fields( import_ browser) example. org/fooix +++++++ +++++++ +++++++
>> >>> login('<email address hidden>')
>>
>> >>> svn_import = factory.
>> @@ -87,7 +97,8 @@
>> >>> print_form_
>> field.git_repo_url: git://git.
>>
>> -=== 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.
>> text(message)
>> The +edit-import page allows the import operator to edit the location
>> an import is from.
>> @@ -102,6 +113,17 @@
>> ... print extract_
>> The code import has been updated.
>>
>> +bzr-svn imports,
>> +
>
> One less line here.
Fixed.
>> + browser. open(bzr_ svn_import_ location + '/+edit-import') browser. getControl( 'Branch URL').value = \ svn-new. example. com/bzr- svn/trunk' browser. getControl( 'Update' ).click( ) messages( import_ browser. contents) : text(message) browser. open(cvs_ import_ location + '/+edit-import') code/stories/ codeimport/ xx-codeimport- list.txt' code/stories/ codeimport/ xx-codeimport- list.txt 2009-06-15 code/stories/ codeimport/ xx-codeimport- list.txt 2009-12-08 codeimport. tests.test_ workermonitor import ( _sample_ data) database. sqlbase import flush_database_ updates Systems model.tests. test_codeimport import ( interfaces. codeimport import ICodeImportSet name="myproject ", branch_ name="trunk" , example. com/svn/ myproject/ trunk", datetime. datetime( 2007, 1, 1, tzinfo=pytz.UTC)) bzr_svn_ import = make_active_import( name="ourprojec t", branch_ name="trunk" , example. com/bzr- svn/myproject/ trunk", RevisionControl Systems. BZR_SVN, datetime. datetime( 2007, 1, 2, tzinfo=pytz.UTC)) name="hisproj" , branch_name="main", ":pserver: <email address hidden>:/cvs", "hisproj" , datetime. datetime( 2007, 1, 2, tzinfo=pytz.UTC)) datetime. datetime( 2007, 1, 3, tzinfo=pytz.UTC)) name="herproj" , branch_ name="master" , url="git: //git.example. org/herproj" , datetime. datetime( 2007, 1, 3, tzinfo=pytz.UTC)) datetime. datetime( 2007, 1, 4, tzinfo=pytz.UTC)) updates( )
>> + >>> import_
>> + >>> import_
>> + ... 'svn://
>> + >>> import_
>> + >>> for message in get_feedback_
>> + ... print extract_
>> + The code import has been updated.
>> +
>> CVS imports,
>>
>> >>> import_
>> === modified file 'lib/lp/
>> --- lib/lp/
> 21:05:06 +0000
>> +++ lib/lp/
> 03:10:29 +0000
>> @@ -9,6 +9,7 @@
>> >>> from lp.codehosting.
>> ... nuke_codeimport
>> >>> from canonical.
>> + >>> from lp.code.enum import RevisionControl
>> >>> from lp.code.
>> ... make_active_import)
>> >>> from lp.code.
>> @@ -21,23 +22,28 @@
>> ... factory, product_
>> ... svn_branch_url="http://
>> ... last_update=
>> + >>> active_
>> + ... factory, product_
>> + ... svn_branch_url="http://
>> + ... rcs_type=
>> + ... last_update=
>> >>> active_cvs_import = make_active_import(
>> ... factory, product_
>> ... cvs_root=
> cvs_module=
>> - ... last_update=
>> + ... last_update=
>> >>> active_git_import = make_active_import(
>> ... factory, product_
>> ... git_repo_
>> - ... last_update=
>> + ... last_update=
>> >>> flush_database_
>
> 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.getActiveIm ports() )) code.launchpad. dev')
>> - 3
>> + 4
>> >>> logout()
>>
>> The page is linked to from the "$N imported branches" text.
>>
>> >>> browser.open('http://
>> - >>> 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