Merge lp:~wallyworld/launchpad/branch-infotype-portlet-1040999 into lp:launchpad

[Ian Booth]

Allow branch's information type to be changed from the privacy portlet; always allow branch to be marked as security related.

== Implementation ==

This branch does 2 things:

1. add an edit link to the branch privacy portlet to allow information type to be edited

This branch does not wire up any javascript (that will be done next branch). So the user clicks on the edit link and is taken to a new edit page where they can edit the information type. ie the same non js infrastructure as used elsewhere for lifecycle status etc. The view used to process the information type change does have the ajax processing in place so the next branch will be only javascript.

2. change the algorithm used by getInformationTypesToShow() to determine the allowed information types

Previously, a branch could not be made private/public security unless there were such bugs linked. Now, a branch can always be marked as security. The hooks for checking linked bugs have been left in place though because even though not used right now, very soon user data branches will only be allowed when there are user data linked bugs.

== Tests ==

Add test to TestBranchEditView to check the XHR updating of information type work correctly.

Update tests in TestBranchEditViewInformationTypes to reflect new getInformationTypesToShow algorithm.

Update TestBranchPrivacyPortlet to check for edit link.

== Lint ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/configure.zcml
  lib/lp/code/browser/tests/test_branch.py
  lib/lp/code/templates/branch-portlet-privacy.pt

[William Grant (wgrant)]

16 + @enabled_with_permission('launchpad.Edit')
17 + def visibility(self):
18 + """Return the 'Set information type' Link."""
19 + text = 'Change information type'
20 + return Link('+edit-information-type', text)

Should this be information_type instead of visibility?

55 InformationType.PUBLICSECURITY,
56 InformationType.PRIVATESECURITY,
57 )

Could you fix these to be in the traditional order (between PUBLIC and USERDATA)?

129 + class="lp.code.browser.branch.BranchEditInformationTypeView"
130 + permission="launchpad.Edit"

This should probably be launchpad.Moderate. Curtis added it last week to let project owners change the information type too.

review: Approve (code)

[Ian Booth (wallyworld)]

Thanks!

> 16 + @enabled_with_permission('launchpad.Edit')
> 17 + def visibility(self):
> 18 + """Return the 'Set information type' Link."""
> 19 + text = 'Change information type'
> 20 + return Link('+edit-information-type', text)
>
> Should this be information_type instead of visibility?
>

I considered it but bugs uses 'visibility' for exactly the same thing and I wanted to be consistent.
How strongly do you feel about changing it?

[William Grant (wgrant)]

On 28/08/12 13:39, Ian Booth wrote:
> Thanks!
>
>> 16 + @enabled_with_permission('launchpad.Edit')
>> 17 + def visibility(self):
>> 18 + """Return the 'Set information type' Link."""
>> 19 + text = 'Change information type'
>> 20 + return Link('+edit-information-type', text)
>>
>> Should this be information_type instead of visibility?
>>
>
> I considered it but bugs uses 'visibility' for exactly the same thing and I wanted to be consistent.
> How strongly do you feel about changing it?

Not strongly at all, but Bugs probably isn't good precedent, as the name
derives from the old hybrid privacy/security portlet.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-08-16 05:55:48 +0000
+++ lib/lp/code/browser/branch.py	2012-08-29 04:50:28 +0000
@@ -308,13 +308,19 @@
         'add_subscriber', 'browse_revisions', 'create_recipe', 'link_bug',
         'link_blueprint', 'register_merge', 'source', 'subscription',
         'edit_status', 'edit_import', 'upgrade_branch', 'view_recipes',
-        'create_queue']
+        'create_queue', 'visibility']
 
     @enabled_with_permission('launchpad.Edit')
     def edit_status(self):
         text = 'Change branch status'
         return Link('+edit-status', text, icon='edit')
 
+    @enabled_with_permission('launchpad.Moderate')
+    def visibility(self):
+        """Return the 'Set information type' Link."""
+        text = 'Change information type'
+        return Link('+edit-information-type', text)
+
     def browse_revisions(self):
         """Return a link to the branch's revisions on codebrowse."""
         text = 'All revisions'
@@ -430,8 +436,8 @@
         return branch.url
 
 
-class BranchView(LaunchpadView, FeedsMixin, BranchMirrorMixin,
-                 InformationTypePortletMixin):
+class BranchView(InformationTypePortletMixin, FeedsMixin, BranchMirrorMixin,
+                 LaunchpadView):
 
     feed_types = (
         BranchFeedLink,
@@ -731,22 +737,21 @@
         # If we're stacked on a private branch, only show that
         # information type.
         if self.context.stacked_on and self.context.stacked_on.private:
-            shown_types = set([self.context.stacked_on.information_type])
+            shown_types = {self.context.stacked_on.information_type}
         else:
             shown_types = (
                 InformationType.PUBLIC,
+                InformationType.PUBLICSECURITY,
+                InformationType.PRIVATESECURITY,
                 InformationType.USERDATA,
                 InformationType.PROPRIETARY,
                 InformationType.EMBARGOED,
                 )
 
-            # We only show Private Security and Public Security
-            # if the branch is linked to a bug with one of those types,
-            # as they're confusing and not generally useful otherwise.
-            # Once Proprietary is fully deployed, it should be added here.
+            # XXX Once Branch Visibility Policies are removed, we only want to
+            # show Private (USERDATA) if the branch is linked to such a bug.
             hidden_types = (
-                InformationType.PUBLICSECURITY,
-                InformationType.PRIVATESECURITY,
+                # InformationType.USERDATA,
                 )
             if set(allowed_types).intersection(hidden_types):
                 params = BugTaskSearchParams(
@@ -800,7 +805,8 @@
         """See `LaunchpadFormView`"""
         return {self.schema: self.context}
 
-    @action('Change Branch', name='change')
+    @action('Change Branch', name='change',
+        failure=LaunchpadFormView.ajax_failure_handler)
     def change_action(self, action, data):
         # If the owner or product has changed, add an explicit notification.
         # We take our own snapshot here to make sure that the snapshot records
@@ -849,9 +855,15 @@
             # was in fact a change.
             self.context.date_last_modified = UTC_NOW
 
+        if self.request.is_ajax:
+            return ''
+
     @property
     def next_url(self):
-        return canonical_url(self.context)
+        """Return the next URL to call when this call completes."""
+        if not self.request.is_ajax:
+            return canonical_url(self.context)
+        return None
 
     cancel_url = next_url
 
@@ -868,6 +880,12 @@
     field_names = ['lifecycle_status']
 
 
+class BranchEditInformationTypeView(BranchEditFormView):
+    """A view for editing the information type only."""
+
+    field_names = ['information_type']
+
+
 class BranchMirrorStatusView(LaunchpadFormView):
     """This view displays the mirror status of a branch.
 
@@ -1114,9 +1132,9 @@
     def validate(self, data):
         # Check that we're not moving a team branch to the +junk
         # pseudo project.
-        owner = data['owner']
         if 'name' in data:
             # Only validate if the name has changed or the owner has changed.
+            owner = data['owner']
             if ((data['name'] != self.context.name) or
                 (owner != self.context.owner)):
                 # We only allow moving within the same branch target for now.

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2012-05-29 06:05:16 +0000
+++ lib/lp/code/browser/configure.zcml	2012-08-29 04:50:28 +0000
@@ -480,6 +480,13 @@
         permission="launchpad.Edit"
         template="../../app/templates/generic-edit.pt"/>
     <browser:page
+        name="+edit-information-type"
+        for="lp.code.interfaces.branch.IBranch"
+        layer="lp.code.publisher.CodeLayer"
+        class="lp.code.browser.branch.BranchEditInformationTypeView"
+        permission="launchpad.Moderate"
+        template="../../app/templates/generic-edit.pt"/>
+    <browser:page
         name="+edit"
         for="lp.code.interfaces.branch.IBranch"
         layer="lp.code.publisher.CodeLayer"

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-08-27 23:58:18 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-08-29 04:50:28 +0000
@@ -71,7 +71,10 @@
     setupBrowser,
     setupBrowserForUser,
     )
-from lp.testing.views import create_initialized_view
+from lp.testing.views import (
+    create_initialized_view,
+    create_view,
+    )
 
 
 class TestBranchMirrorHidden(TestCaseWithFactory):
@@ -357,7 +360,8 @@
     def test_linked_bugs_series_branch_query_scaling(self):
         # As we add linked bugs, the query count for a branch index page stays
         # constant.
-        product = self.factory.makeProduct()
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PUBLIC)
         branch = self.factory.makeProductBranch(product=product)
         browses_under_limit = BrowsesWithQueryLimit(54, branch.owner)
         with person_logged_in(product.owner):
@@ -964,7 +968,7 @@
         admin = admins.teamowner
         browser = self.getUserBrowser(
             canonical_url(branch) + '/+edit', user=admin)
-        browser.getControl("Private").click()
+        browser.getControl("Private", index=1).click()
         browser.getControl("Change Branch").click()
         with person_logged_in(person):
             self.assertEqual(
@@ -986,6 +990,29 @@
         self.assertRaises(
             LookupError, browser.getControl, "Information Type")
 
+    def test_edit_view_ajax_render(self):
+        # An information type change request is processed as expected when an
+        # XHR request is made to the view.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+
+        extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+        request = LaunchpadTestRequest(
+            method='POST', form={
+                'field.actions.change': 'Change Branch',
+                'field.information_type': 'PUBLICSECURITY'},
+            **extra)
+        with person_logged_in(person):
+            view = create_view(
+                branch, name='+edit-information-type',
+                request=request, principal=person)
+            request.traversed_objects = [person, branch.product, branch, view]
+            view.initialize()
+            result = view.render()
+            self.assertEqual('', result)
+            self.assertEqual(
+                branch.information_type, InformationType.PUBLICSECURITY)
+
 
 class TestBranchEditViewInformationTypes(TestCaseWithFactory):
     """Tests for BranchEditView.getInformationTypesToShow."""
@@ -1000,28 +1027,14 @@
             self.assertContentEqual(types, view.getInformationTypesToShow())
 
     def test_public_branch(self):
-        # A normal public branch on a public project can only be public.
-        # We don't show information types like Public Security
-        # unless there's a linked branch of that type, as they're not
-        # useful or unconfusing otherwise.
+        # A normal public branch on a public project can only be a public
+        # information type.
         # The model doesn't enforce this, so it's just a UI thing.
         branch = self.factory.makeBranch(
             information_type=InformationType.PUBLIC)
-        self.assertShownTypes([InformationType.PUBLIC], branch)
-
-    def test_public_branch_with_security_bug(self):
-        # A public branch can be set to Public Security if it has a
-        # linked Public Security bug. The project policy doesn't
-        # allow private branches, so Private Security and Private
-        # are unavailable.
-        branch = self.factory.makeBranch(
-            information_type=InformationType.PUBLIC)
-        bug = self.factory.makeBug(
-            information_type=InformationType.PUBLICSECURITY)
-        with admin_logged_in():
-            branch.linkBug(bug, branch.owner)
         self.assertShownTypes(
-            [InformationType.PUBLIC, InformationType.PUBLICSECURITY],
+            [InformationType.PUBLIC,
+             InformationType.PUBLICSECURITY],
             branch)
 
     def test_branch_with_disallowed_type(self):
@@ -1033,7 +1046,10 @@
         branch = self.factory.makeBranch(
             product=product, information_type=InformationType.PROPRIETARY)
         self.assertShownTypes(
-            [InformationType.PUBLIC, InformationType.PROPRIETARY], branch)
+            [InformationType.PUBLIC,
+             InformationType.PUBLICSECURITY,
+             InformationType.PROPRIETARY],
+            branch)
 
     def test_stacked_on_private(self):
         # A branch stacked on a private branch has its choices limited
@@ -1060,24 +1076,10 @@
             branch.product.setBranchVisibilityTeamPolicy(
                 branch.owner, BranchVisibilityRule.PRIVATE)
         self.assertShownTypes(
-            [InformationType.PUBLIC, InformationType.USERDATA], branch)
-
-    def test_private_branch_with_security_bug(self):
-        # Branches on projects that allow private branches can use the
-        # Private Security information type if they have a security
-        # bug linked.
-        branch = self.factory.makeBranch(
-            information_type=InformationType.PUBLIC)
-        with admin_logged_in():
-            branch.product.setBranchVisibilityTeamPolicy(
-                branch.owner, BranchVisibilityRule.PRIVATE)
-        bug = self.factory.makeBug(
-            information_type=InformationType.PUBLICSECURITY)
-        with admin_logged_in():
-            branch.linkBug(bug, branch.owner)
-        self.assertShownTypes(
-            [InformationType.PUBLIC, InformationType.PUBLICSECURITY,
-             InformationType.PRIVATESECURITY, InformationType.USERDATA],
+            [InformationType.PUBLIC,
+             InformationType.PUBLICSECURITY,
+             InformationType.PRIVATESECURITY,
+             InformationType.USERDATA],
             branch)
 
     def test_branch_for_project_with_embargoed_and_proprietary(self):
@@ -1140,6 +1142,7 @@
             owner=owner, information_type=InformationType.USERDATA)
         with person_logged_in(owner):
             view = create_initialized_view(branch, '+portlet-privacy')
+            edit_url = '/' + branch.unique_name + '/+edit-information-type'
             soup = BeautifulSoup(view.render())
         information_type = soup.find('strong')
         description = soup.find('div', id='information-type-description')
@@ -1147,3 +1150,5 @@
             InformationType.USERDATA.title, information_type.renderContents())
         self.assertTextMatchesExpressionIgnoreWhitespace(
             InformationType.USERDATA.description, description.renderContents())
+        self.assertIsNotNone(
+            soup.find('a', id='privacy-link', attrs={'href': edit_url}))

=== modified file 'lib/lp/code/stories/branches/xx-branch-edit-privacy.txt'
--- lib/lp/code/stories/branches/xx-branch-edit-privacy.txt	2012-07-17 14:10:53 +0000
+++ lib/lp/code/stories/branches/xx-branch-edit-privacy.txt	2012-08-29 04:50:28 +0000
@@ -39,7 +39,7 @@
 
     >>> admin_browser.open('http://code.launchpad.dev/~eric/fooix/trunk')
     >>> admin_browser.getLink('Change branch details').click()
-    >>> admin_browser.getControl('Private').click()
+    >>> admin_browser.getControl('Private', index=1).click()
     >>> admin_browser.getControl('Change Branch').click()
 
 The branch is now private, so it assumes the standard private presentation.
@@ -68,7 +68,7 @@
     >>> browser = setupBrowser(auth='Basic [e-mail]:test')
     >>> browser.open('http://code.launchpad.dev/~name12/firefox/main')
     >>> browser.getLink('Change branch details').click()
-    >>> browser.getControl('Private').selected = True
+    >>> browser.getControl('Private', index=1).selected = True
     >>> browser.getControl('Change Branch').click()
 
 Since the branch is now private, it will have the standard Launchpad
@@ -81,7 +81,7 @@
 reverts to its normal presentation.
 
     >>> browser.getLink('Change branch details').click()
-    >>> browser.getControl('Public').selected = True
+    >>> browser.getControl('Public', index=0).selected = True
     >>> browser.getControl('Change Branch').click()
     >>> print extract_text(find_tag_by_id(browser.contents, 'privacy'))
     This branch contains Public information...
@@ -103,7 +103,7 @@
 Mark the public branch private.
 
     >>> browser.getLink('Change branch details').click()
-    >>> browser.getControl('Private').selected = True
+    >>> browser.getControl('Private', index=1).selected = True
     >>> browser.getControl('Change Branch').click()
 
 The branch is now private, so it assumes the standard private presentation.

=== modified file 'lib/lp/code/templates/branch-portlet-privacy.pt'
--- lib/lp/code/templates/branch-portlet-privacy.pt	2012-06-20 05:25:44 +0000
+++ lib/lp/code/templates/branch-portlet-privacy.pt	2012-08-29 04:50:28 +0000
@@ -6,9 +6,15 @@
   tal:attributes="
     class python: path('context/private') and 'portlet private' or 'portlet public'
   "
+  tal:define="link context/menu:context/visibility"
 >
     <span id="information-type-summary"
       tal:attributes="class view/information_type_css;">This branch
-      contains <strong id="information-type" tal:content="view/information_type"></strong> information</span>
-    <div id="information-type-description" style="padding-top: 5px" tal:content="view/information_type_description"></div>
+      contains
+      <strong id="information-type" tal:content="view/information_type"></strong>
+      information</span>&nbsp;<a class="sprite edit action-icon" id="privacy-link"
+           tal:attributes="href link/path" tal:condition="link/enabled"
+           >Edit</a>
+    <div id="information-type-description" style="padding-top: 5px"
+         tal:content="view/information_type_description"></div>
 </div>