launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:change-permissions-builder-reset into launchpad:master.

Commit message:
Change permissions required to reset builders

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/393716

This branch changes permissions required to reset builders to:

     ~admins and ~launchpad-buildd-admins can edit the entire set of attributes on IBuilderView
     ~registry team members (registry experts) can edit only 3 attributes on builder: builderok, manual and failnotes

These will be exposed in browser via the "Change details" link on the Edit Builder view.

The branch also introduces unit test for the builder view (they're currently doctests).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:change-permissions-builder-reset into launchpad:master.

diff --git a/lib/lp/buildmaster/browser/builder.py b/lib/lp/buildmaster/browser/builder.py
index 1c67f11..3db020e 100644
--- a/lib/lp/buildmaster/browser/builder.py
+++ b/lib/lp/buildmaster/browser/builder.py
@@ -23,6 +23,7 @@ from datetime import (
 from itertools import groupby
 import operator
 
+from lazr.restful.interface import copy_field, use_template
 from lazr.restful.utils import smartquote
 import pytz
 import six
@@ -30,6 +31,7 @@ from zope.component import getUtility
 from zope.event import notify
 from zope.formlib.widget import CustomWidgetFactory
 from zope.formlib.widgets import TextWidget
+from zope.interface import Interface
 from zope.lifecycleevent import ObjectCreatedEvent
 
 from lp import _
@@ -43,8 +45,8 @@ from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.app.widgets.owner import HiddenUserWidget
 from lp.buildmaster.interfaces.builder import (
     IBuilder,
-    IBuilderSet,
-    )
+    IBuilderSet, IBuilderView, IBuilderEdit, IBuilderModerateAttributes,
+)
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildSource,
     )
@@ -412,16 +414,36 @@ class BuilderSetAddView(LaunchpadFormView):
         return canonical_url(self.context)
 
 
-class BuilderEditView(LaunchpadEditFormView):
+class BuilderEditViewAdmins(LaunchpadEditFormView):
     """View class for changing builder details."""
 
-    schema = IBuilder
+    field_names = None
+
+    @cachedproperty
+    def schema(self):
+        class BuilderEditSchema(Interface):
+            """Defines the fields for the edit form.
+
+            This is necessary to make various fields editable that are not
+            normally editable through the interface.
+            """
+
+            name = copy_field(IBuilder["name"], readonly=False)
+            title = copy_field(IBuilder["title"], readonly=False)
+            processors = copy_field(IBuilder["processors"], readonly=False)
+            url = copy_field(IBuilder["url"], readonly=False)
+            owner = copy_field(IBuilder["owner"], readonly=False)
+            virtualized = copy_field(IBuilder["virtualized"], readonly=False)
+            vm_host = copy_field(IBuilder["vm_host"], readonly=False)
+            vm_reset_protocol = copy_field(IBuilder["vm_reset_protocol"], readonly=False)
+            active = copy_field(IBuilder["active"], readonly=False)
+
+            use_template(IBuilder, include=["builderok"])
+            use_template(IBuilder, include=["manual"])
+            use_template(IBuilder, include=["failnotes"])
+
+        return BuilderEditSchema
 
-    field_names = [
-        'name', 'title', 'processors', 'url', 'manual', 'owner',
-        'virtualized', 'builderok', 'failnotes', 'vm_host',
-        'vm_reset_protocol', 'active',
-        ]
     custom_widget_processors = LabeledMultiCheckBoxWidget
 
     @action(_('Change'), name='update')
diff --git a/lib/lp/buildmaster/browser/configure.zcml b/lib/lp/buildmaster/browser/configure.zcml
index c1a40b4..18a8899 100644
--- a/lib/lp/buildmaster/browser/configure.zcml
+++ b/lib/lp/buildmaster/browser/configure.zcml
@@ -90,9 +90,9 @@
     </browser:pages>
     <browser:page
         name="+edit"
-        for="lp.buildmaster.interfaces.builder.IBuilder"
-        class="lp.buildmaster.browser.builder.BuilderEditView"
-        permission="launchpad.Edit"
+        for="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"
+        class="lp.buildmaster.browser.builder.BuilderEditViewAdmins"
+        permission="launchpad.Moderate"
         template="../templates/builder-edit.pt"/>
     <browser:menus
         classes="BuilderSetOverviewMenu"
diff --git a/lib/lp/buildmaster/browser/tests/test_builder_views.py b/lib/lp/buildmaster/browser/tests/test_builder_views.py
index 24a735b..6cacbee 100644
--- a/lib/lp/buildmaster/browser/tests/test_builder_views.py
+++ b/lib/lp/buildmaster/browser/tests/test_builder_views.py
@@ -17,6 +17,7 @@ from lp.buildmaster.enums import (
     BuildFarmJobType,
     BuildStatus,
     )
+from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildfarmjob import (
     IBuildFarmJobSource,
     InconsistentBuildFarmJobError,
@@ -29,8 +30,8 @@ from lp.testing import (
     celebrity_logged_in,
     record_two_runs,
     StormStatementRecorder,
-    TestCaseWithFactory,
-    )
+    TestCaseWithFactory, BrowserTestCase, admin_logged_in, login_person,
+)
 from lp.testing.layers import LaunchpadFunctionalLayer
 from lp.testing.matchers import HasQueryCount
 from lp.testing.sampledata import ADMIN_EMAIL
@@ -159,6 +160,74 @@ class BuildCreationMixin(object):
         return build
 
 
+class TestBuilderView(BrowserTestCase):
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestBuilderView, self).setUp()
+        self.builder = getUtility(IBuilderSet)['bob']
+
+    def test_builder_change_details_buildd_admin(self):
+        person = self.factory.makePerson()
+        badmins = getUtility(IPersonSet).getByName('launchpad-buildd-admins')
+        with admin_logged_in():
+            badmins.addMember(person, badmins)
+
+        login_person(person)
+
+        browser = self.getViewBrowser(self.builder, user=person)
+        browser.getLink("Change details").click()
+        self.assertEqual(browser.getControl(name="field.name").value, 'bob')
+        self.assertEqual(browser.getControl(
+            name="field.title").value, 'Bob The Builder')
+        self.assertEqual(browser.getControl(
+            name="field.processors").value, ['386'])
+        self.assertEqual(browser.getControl(
+            name="field.owner").value, 'launchpad-buildd-admins')
+        self.assertEqual(browser.getControl('Manual Mode').selected, False)
+        self.assertEqual(browser.getControl(name="field.vm_host").value, '')
+        self.assertEqual(browser.getControl(
+            name="field.builderok").value, True)
+        self.assertEqual(browser.getControl(name="field.failnotes").value, '')
+        self.assertEqual(browser.getControl(name="field.active").value, True)
+
+        self.builder.builderok = True
+
+    def test_builder_change_details_registry_expert(self):
+        person = self.factory.makePerson()
+        reg_expert = getUtility(IPersonSet).getByName('registry')
+        with admin_logged_in():
+            reg_expert.addMember(person, reg_expert)
+        login_person(person)
+
+        # browser = self.getViewBrowser(self.builder, user=person)
+        # browser.getLink("Change details").click()
+        # self.assertEqual(browser.getControl(name="field.name").value, 'bob')
+        #
+        # title = browser.getControl(
+        #     name="field.title").value, 'Bob The Builder'
+        # title[0] = 'Changed title'
+        # title[1] = u'Changed title'
+        # browser.getLink("Change").click()
+        # self.assertEqual(browser.title, 'Changed title')
+        self.builder.builderok = True
+        # self.builder.title = 'Changed title'
+
+        #
+        # self.assertEqual(browser.getControl(
+        #     name="field.title").value, 'Bob The Builder')
+        # self.assertEqual(browser.getControl(
+        #     name="field.processors").value, ['386'])
+        # self.assertEqual(browser.getControl(
+        #     name="field.owner").value, 'launchpad-buildd-admins')
+        # self.assertEqual(browser.getControl('Manual Mode').selected, False)
+        # self.assertEqual(browser.getControl(name="field.vm_host").value, '')
+        # self.assertEqual(browser.getControl(
+        #     name="field.builderok").value, True)
+        # self.assertEqual(browser.getControl(name="field.failnotes").value, '')
+        # self.assertEqual(browser.getControl(name="field.active").value, True)
+
+
 class TestBuilderHistoryView(TestCaseWithFactory, BuildCreationMixin):
 
     layer = LaunchpadFunctionalLayer
diff --git a/lib/lp/buildmaster/configure.zcml b/lib/lp/buildmaster/configure.zcml
index 3d34cb1..35ccf9d 100644
--- a/lib/lp/buildmaster/configure.zcml
+++ b/lib/lp/buildmaster/configure.zcml
@@ -32,14 +32,17 @@
     </securedutility>
 
     <!-- Builder -->
-    <class
-        class="lp.buildmaster.model.builder.Builder">
-        <allow
-            interface="lp.buildmaster.interfaces.builder.IBuilderView"/>
+    <class class="lp.buildmaster.model.builder.Builder">
+        <require
+            permission="zope.Public"
+            interface="lp.app.interfaces.launchpad.IPrivacy
+                       lp.buildmaster.interfaces.builder.IBuilderView" />
+        <require
+            permission="launchpad.Moderate"
+            interface="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"/>
         <require
             permission="launchpad.Edit"
-            interface="lp.buildmaster.interfaces.builder.IBuilderEdit"
-            set_schema="lp.buildmaster.interfaces.builder.IBuilderView"/>
+            interface="lp.buildmaster.interfaces.builder.IBuilderEdit"/>
     </class>
 
 
diff --git a/lib/lp/buildmaster/interfaces/builder.py b/lib/lp/buildmaster/interfaces/builder.py
index 6be226f..8540e03 100644
--- a/lib/lp/buildmaster/interfaces/builder.py
+++ b/lib/lp/buildmaster/interfaces/builder.py
@@ -98,6 +98,21 @@ class BuildSlaveFailure(BuildDaemonError):
     """The build slave has suffered an error and cannot be used."""
 
 
+class IBuilderModerateAttributes(Interface):
+    manual = exported(Bool(
+        title=_('Manual Mode'), required=False, default=False,
+        description=_('The auto-build system does not dispatch '
+                      'jobs automatically for slaves in manual mode.')))
+
+    builderok = exported(Bool(
+        title=_('Builder State OK'), required=True, default=True,
+        description=_('Whether or not the builder is ok')))
+
+    failnotes = exported(Text(
+        title=_('Failure Notes'), required=False,
+        description=_('The reason for a builder not being ok')))
+
+
 class IBuilderView(IHasBuildRecords, IHasOwner):
 
     id = Attribute("Builder identifier")
@@ -146,19 +161,6 @@ class IBuilderView(IHasBuildRecords, IHasOwner):
         description=_('Whether or not the builder is a virtual Xen '
                       'instance.')))
 
-    manual = exported(Bool(
-        title=_('Manual Mode'), required=False, default=False,
-        description=_('The auto-build system does not dispatch '
-                      'jobs automatically for slaves in manual mode.')))
-
-    builderok = exported(Bool(
-        title=_('Builder State OK'), required=True, default=True,
-        description=_('Whether or not the builder is ok')))
-
-    failnotes = exported(Text(
-        title=_('Failure Notes'), required=False,
-        description=_('The reason for a builder not being ok')))
-
     vm_host = exported(TextLine(
         title=_('VM host'), required=False,
         description=_('The machine hostname hosting the virtual '
@@ -220,7 +222,7 @@ class IBuilderEdit(Interface):
 
 
 @exported_as_webservice_entry()
-class IBuilder(IBuilderEdit, IBuilderView):
+class IBuilder(IBuilderEdit, IBuilderView, IBuilderModerateAttributes):
     """Build-slave information and state.
 
     Builder instance represents a single builder slave machine within the
diff --git a/lib/lp/security.py b/lib/lp/security.py
index 243c1a8..af7ecbc 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -61,8 +61,9 @@ from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.bugs.model.bugtasksearch import get_bug_privacy_filter
 from lp.buildmaster.interfaces.builder import (
     IBuilder,
+    IBuilderModerateAttributes,
     IBuilderSet,
-    )
+)
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.code.interfaces.branch import (
@@ -1970,10 +1971,24 @@ class AdminBuilder(AdminByBuilddAdmin):
     usedfor = IBuilder
 
 
-class EditBuilder(AdminByBuilddAdmin):
+class EditBuilder(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IBuilder
 
+    def checkAuthenticated(self, user):
+        return (user.in_admin
+                or user.in_buildd_admin)
+
+
+class ModerateEditBuilder(EditBuilder):
+    permission = 'launchpad.Moderate'
+    usedfor = IBuilderModerateAttributes
+
+    def checkAuthenticated(self, user):
+        return (user.in_admin
+                or user.in_registry_experts
+                or user.in_buildd_admin)
+
 
 class AdminBuildRecord(AdminByBuilddAdmin):
     usedfor = IBuildFarmJob