[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-read-only-views into lp:launchpad

[noreply]

The proposal to merge lp:~cjwatson/launchpad/codeimport-git-read-only-views 
into lp:launchpad has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-read-only-views/+merge/308374
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-read-only-views into lp:launchpad

[William Grant]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/browser/codeimport.py'
> --- lib/lp/code/browser/codeimport.py 2016-10-06 15:59:34 +
> +++ lib/lp/code/browser/codeimport.py 2016-10-13 12:55:20 +
> @@ -543,6 +549,10 @@
>  else:
>  raise AssertionError('Unknown rcs_type for code import.')
>  
> +if (self.code_import.target_rcs_type ==
> +TargetRevisionControlSystems.GIT):

I find the contrast between the == BZR above and the == GIT here disturbing. 
Perhaps != BZR?

> +self.form_fields = self.form_fields.omit('whiteboard')
> +
>  def _showButtonForStatus(self, status):
>  """If the status is different, and the user is super, show button."""
>  return self._super_user and self.code_import.review_status != status
> 
> === added file 'lib/lp/code/templates/gitrepository-import-details.pt'
> --- lib/lp/code/templates/gitrepository-import-details.pt 1970-01-01 
> 00:00:00 +
> +++ lib/lp/code/templates/gitrepository-import-details.pt 2016-10-13 
> 12:55:20 +
> @@ -0,0 +1,95 @@
> + +  xmlns:tal="http://xml.zope.org/namespaces/tal;
> +  xmlns:metal="http://xml.zope.org/namespaces/metal;
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n;
> +  tal:define="context_menu view/context/menu:context">
> +
> +   +  condition="context/repository_type/enumvalue:IMPORTED">
> +
> +   +  define="code_import repository/code_import">
> +
> +Import Status:
> +   +tal:content="code_import/review_status/title"/>
> +
> +
> +
> +  
> +This repository is an import of the Git repository at
> +
> +   + tal:content="code_import/url" />.
> +
> +
> +  .
> +
> +  
> +
> +
> + + condition="job">
> +  
> +  
> +An import is currently running on
> +,
> +and was started
> +
> +  2 hours ago
> +.
> +
> +  The last few lines of the job's output were:
> +  
> +
> +  
> +
> +  
> +  
> +The next import is scheduled to run
> +
> +  as soon as possible +condition="job/requesting_user">
> +(requested by
> + +   replace="structure job/requesting_user/fmt:link">
> +  Some user.
> +).
> +
> +
> +  
> +in 2 hours
> +  .
> +
> +  
> +  
> +
> +
> +
> +  
> +The import has been suspended because it failed
> + content="modules/lp.services.config/config/codeimport/consecutive_failure_limit"/>
> +or more times in succession.
> +  
> +
> +
> +
> +  
> +Last successful import was
> + replace="code_import/date_last_successful/fmt:displaydate">
> +  2 hours ago
> +.
> +  
> +
> +
> + tal:condition="view/latest_code_import_results">
> +  
> +
> +  
> +
> +  
> +
> +  
> +
> +

Is this non-trivially different from the branch equivalent? It's a reasonably 
complex bit of UI that would be nice to not duplicate.



-- 
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-read-only-views/+merge/308374
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-read-only-views into lp:launchpad

[Colin Watson]

Colin Watson has proposed merging 
lp:~cjwatson/launchpad/codeimport-git-read-only-views into lp:launchpad with 
lp:~cjwatson/launchpad/git-code-import-security-deletion as a prerequisite.

Commit message:
Add read-only views for Git-targeted code imports.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1469459 in Launchpad itself: "import external code into a LP git repo 
(natively)"
  https://bugs.launchpad.net/launchpad/+bug/1469459

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-read-only-views/+merge/308374

We don't have much in the way of tests for this yet, but that will be easier to 
bring up once we have creation webservice/view handling in place.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/codeimport-git-read-only-views into lp:launchpad.
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2016-05-07 00:40:18 +
+++ lib/lp/code/browser/branch.py	2016-10-13 12:55:20 +
@@ -89,12 +89,12 @@
 latest_proposals_for_each_branch,
 )
 from lp.code.browser.branchref import BranchRef
+from lp.code.browser.codeimport import CodeImportTargetMixin
 from lp.code.browser.decorations import DecoratedBranch
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
 from lp.code.browser.widgets.branchtarget import BranchTargetWidget
 from lp.code.enums import (
 BranchType,
-CodeImportResultStatus,
 CodeImportReviewStatus,
 )
 from lp.code.errors import (
@@ -403,7 +403,7 @@
 
 
 class BranchView(InformationTypePortletMixin, FeedsMixin, BranchMirrorMixin,
- LaunchpadView, HasSnapsViewMixin):
+ LaunchpadView, HasSnapsViewMixin, CodeImportTargetMixin):
 
 feed_types = (
 BranchFeedLink,
@@ -596,31 +596,6 @@
 return collection.getExtendedRevisionDetails(
 self.user, self.context.latest_revisions)
 
-@cachedproperty
-def latest_code_import_results(self):
-"""Return the last 10 CodeImportResults."""
-return list(self.context.code_import.results[:10])
-
-def iconForCodeImportResultStatus(self, status):
-"""The icon to represent the `CodeImportResultStatus` `status`."""
-if status == CodeImportResultStatus.SUCCESS_PARTIAL:
-return "/@@/yes-gray"
-elif status in CodeImportResultStatus.successes:
-return "/@@/yes"
-else:
-return "/@@/no"
-
-@property
-def url_is_web(self):
-"""True if an imported branch's URL is HTTP or HTTPS."""
-# You should only be calling this if it's an SVN, BZR or GIT code
-# import
-assert self.context.code_import
-url = self.context.code_import.url
-assert url
-# https starts with http too!
-return url.startswith("http")
-
 @property
 def show_merge_links(self):
 """Return whether or not merge proposal links should be shown.
@@ -799,7 +774,7 @@
 target is not None and target != self.context.target):
 try:
 self.context.setTarget(self.user, project=target)
-except BranchTargetError, e:
+except BranchTargetError as e:
 self.setFieldError('target', e.message)
 return
 

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2016-10-06 15:59:34 +
+++ lib/lp/code/browser/codeimport.py	2016-10-13 12:55:20 +
@@ -12,6 +12,7 @@
 'CodeImportSetBreadcrumb',
 'CodeImportSetNavigation',
 'CodeImportSetView',
+'CodeImportTargetMixin',
 'validate_import_url',
 ]
 
@@ -49,6 +50,7 @@
 from lp.code.enums import (
 BranchSubscriptionDiffSize,
 BranchSubscriptionNotificationLevel,
+CodeImportResultStatus,
 CodeImportReviewStatus,
 CodeReviewNotificationLevel,
 NON_CVS_RCS_TYPES,
@@ -191,8 +193,8 @@
 self.addError(structured("""
 Those CVS details are already specified for
 the imported branch %s.""",
-canonical_url(code_import.branch),
-code_import.branch.unique_name))
+canonical_url(code_import.target),
+code_import.target.unique_name))
 
 def _validateURL(self, url, rcs_type, existing_import=None,
  field_name='url'):
@@ -495,12 +497,12 @@
 class CodeImportEditView(CodeImportBaseView):
 """View for editing code imports.
 
-This view is registered against the branch, but mostly edits the code
-import for that branch -- the exception being that it also allows the
-editing of the branch whiteboard.  If the branch has no associated code
-import, then the result is a 404.  If the branch does have a code import,
-then the adapters property allows the form internals