launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-grantee-widgets into lp:launchpad.

Commit message:
Add widgets to display or select a Git access grantee.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1517559 in Launchpad itself: "git fine-grained permissions"
  https://bugs.launchpad.net/launchpad/+bug/1517559

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-grantee-widgets/+merge/357600

This isn't hugely elegant, but it gets the job done.  You can see it in context (with an as-yet-unpushed UI branch) here:

  https://people.canonical.com/~cjwatson/tmp/lp-git-ref-permissions.png
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-grantee-widgets into lp:launchpad.

=== added file 'lib/lp/code/browser/widgets/gitgrantee.py'
--- lib/lp/code/browser/widgets/gitgrantee.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/widgets/gitgrantee.py	2018-10-21 23:56:34 +0000
@@ -0,0 +1,224 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'GitGranteeDisplayWidget',
+    'GitGranteeField',
+    'GitGranteeWidget',
+    ]
+
+from lazr.enum import DBItem
+from z3c.ptcompat import ViewPageTemplateFile
+from zope.formlib.interfaces import (
+    ConversionError,
+    IDisplayWidget,
+    IInputWidget,
+    InputErrors,
+    MissingInputError,
+    WidgetInputError,
+    )
+from zope.formlib.utility import setUpWidget
+from zope.formlib.widget import (
+    BrowserWidget,
+    CustomWidgetFactory,
+    DisplayWidget,
+    InputWidget,
+    renderElement,
+    )
+from zope.interface import implementer
+from zope.schema import (
+    Choice,
+    Field,
+    )
+from zope.schema.interfaces import IField
+from zope.schema.vocabulary import getVocabularyRegistry
+from zope.security.proxy import isinstance as zope_isinstance
+
+from lp.app.errors import UnexpectedFormData
+from lp.app.validators import LaunchpadValidationError
+from lp.app.widgets.popup import PersonPickerWidget
+from lp.code.enums import GitGranteeType
+from lp.code.interfaces.gitref import IGitRef
+from lp.registry.interfaces.person import IPerson
+from lp.services.webapp.interfaces import (
+    IAlwaysSubmittedWidget,
+    IMultiLineWidgetLayout,
+    )
+
+
+class IGitGranteeField(IField):
+    """An interface for a Git access grantee field."""
+
+
+@implementer(IGitGranteeField)
+class GitGranteeField(Field):
+    """A field that holds a Git access grantee."""
+
+    def constraint(self, value):
+        """See `IField`."""
+        if zope_isinstance(value, DBItem) and value.enum == GitGranteeType:
+            return value != GitGranteeType.PERSON
+        else:
+            return value in getVocabularyRegistry().get(
+                None, "ValidPersonOrTeam")
+
+
+@implementer(IMultiLineWidgetLayout)
+class GitGranteeWidgetBase(BrowserWidget):
+
+    template = ViewPageTemplateFile("templates/gitgrantee.pt")
+    default_option = "person"
+    _widgets_set_up = False
+
+    def setUpSubWidgets(self):
+        if self._widgets_set_up:
+            return
+        fields = [
+            Choice(
+                __name__="person", title=u"Person",
+                required=False, vocabulary="ValidPersonOrTeam"),
+            ]
+        if not self._read_only:
+            self.person_widget = CustomWidgetFactory(
+                PersonPickerWidget,
+                # XXX cjwatson 2018-10-18: This is a little unfortunate, but
+                # otherwise there's no spacing at all between the
+                # (deliberately unlabelled) radio button and the text box.
+                style="margin-left: 4px;")
+        for field in fields:
+            setUpWidget(
+                self, field.__name__, field, self._sub_widget_interface,
+                prefix=self.name)
+        self._widgets_set_up = True
+
+    def setUpOptions(self):
+        """Set up options to be rendered."""
+        self.options = {}
+        for option in ("repository_owner", "person"):
+            attributes = {
+                "type": "radio", "name": self.name, "value": option,
+                "id": "%s.option.%s" % (self.name, option),
+                # XXX cjwatson 2018-10-18: Ugly, but it's worse without
+                # this, especially in a permissions table where this widget
+                # is normally used.
+                "style": "margin-left: 0;",
+                }
+            if self.request.form_ng.getOne(
+                    self.name, self.default_option) == option:
+                attributes["checked"] = "checked"
+            if self._read_only:
+                attributes["disabled"] = "disabled"
+            self.options[option] = renderElement("input", **attributes)
+
+    @property
+    def show_options(self):
+        return {
+            option: not self._read_only or self.default_option == option
+            for option in ("repository_owner", "person")}
+
+    def setRenderedValue(self, value):
+        """See `IWidget`."""
+        self.setUpSubWidgets()
+        if value == GitGranteeType.REPOSITORY_OWNER:
+            self.default_option = "repository_owner"
+            return
+        elif value is None or IPerson.providedBy(value):
+            self.default_option = "person"
+            self.person_widget.setRenderedValue(value)
+            return
+        else:
+            raise AssertionError("Not a valid value: %r" % value)
+
+    def __call__(self):
+        """See `zope.formlib.interfaces.IBrowserWidget`."""
+        self.setUpSubWidgets()
+        self.setUpOptions()
+        return self.template()
+
+
+@implementer(IDisplayWidget)
+class GitGranteeDisplayWidget(GitGranteeWidgetBase, DisplayWidget):
+    """Widget for displaying a Git access grantee."""
+
+    _sub_widget_interface = IDisplayWidget
+    _read_only = True
+
+
+@implementer(IAlwaysSubmittedWidget, IInputWidget)
+class GitGranteeWidget(GitGranteeWidgetBase, InputWidget):
+    """Widget for selecting a Git access grantee."""
+
+    _sub_widget_interface = IInputWidget
+    _read_only = False
+    _widgets_set_up = False
+
+    @property
+    def show_options(self):
+        show_options = super(GitGranteeWidget, self).show_options
+        # Hide options that indicate unique grantee_types (e.g.
+        # repository_owner) if they already exist in the context.
+        # XXX untested
+        if IGitRef.providedBy(self.context.context):
+            repository = self.context.context.repository
+            ref_pattern = self.context.context.path
+        else:
+            repository = None
+            ref_pattern = None
+        if (show_options["repository_owner"] and
+            repository is not None and ref_pattern is not None and
+            not repository.findRuleGrantsForRepositoryOwner(
+                ref_pattern=ref_pattern).is_empty()):
+            show_options["repository_owner"] = False
+        return show_options
+
+    def hasInput(self):
+        self.setUpSubWidgets()
+        form_value = self.request.form_ng.getOne(self.name)
+        if form_value is None:
+            return False
+        return form_value != "person" or self.person_widget.hasInput()
+
+    def hasValidInput(self):
+        """See `zope.formlib.interfaces.IInputWidget`."""
+        try:
+            self.getInputValue()
+            return True
+        except (InputErrors, UnexpectedFormData):
+            return False
+
+    def getInputValue(self):
+        """See `zope.formlib.interfaces.IInputWidget`."""
+        self.setUpSubWidgets()
+        form_value = self.request.form_ng.getOne(self.name)
+        if form_value == "repository_owner":
+            return GitGranteeType.REPOSITORY_OWNER
+        elif form_value == "person":
+            try:
+                return self.person_widget.getInputValue()
+            except MissingInputError:
+                raise WidgetInputError(
+                    self.name, self.label,
+                    LaunchpadValidationError(
+                        "Please enter a person or team name"))
+            except ConversionError:
+                entered_name = self.request.form_ng.getOne(
+                    "%s.person" % self.name)
+                raise WidgetInputError(
+                    self.name, self.label,
+                    LaunchpadValidationError(
+                        "There is no person or team named '%s' registered in "
+                        "Launchpad" % entered_name))
+        else:
+            raise UnexpectedFormData("No valid option was selected.")
+
+    def error(self):
+        """See `zope.formlib.interfaces.IBrowserWidget`."""
+        try:
+            if self.hasInput():
+                self.getInputValue()
+        except InputErrors as error:
+            self._error = error
+        return super(GitGranteeWidget, self).error()

=== added file 'lib/lp/code/browser/widgets/templates/gitgrantee.pt'
--- lib/lp/code/browser/widgets/templates/gitgrantee.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/widgets/templates/gitgrantee.pt	2018-10-21 23:56:34 +0000
@@ -0,0 +1,27 @@
+<table>
+  <tr tal:condition="view/show_options/repository_owner">
+    <td colspan="2">
+      <label>
+        <input
+            type="radio" value="repository_owner"
+            tal:condition="not: context/readonly"
+            tal:replace="structure view/options/repository_owner" />
+        Repository owner
+      </label>
+    </td>
+  </tr>
+
+  <tr tal:condition="view/show_options/person">
+    <td>
+      <label>
+        <input
+            type="radio" value="person"
+            tal:condition="not: context/readonly"
+            tal:replace="structure view/options/person" />
+      </label>
+    </td>
+    <td>
+      <tal:person replace="structure view/person_widget" />
+    </td>
+  </tr>
+</table>

=== added file 'lib/lp/code/browser/widgets/tests/test_gitgrantee.py'
--- lib/lp/code/browser/widgets/tests/test_gitgrantee.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/widgets/tests/test_gitgrantee.py	2018-10-21 23:56:34 +0000
@@ -0,0 +1,271 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+import re
+
+from zope.formlib.interfaces import (
+    IBrowserWidget,
+    IDisplayWidget,
+    IInputWidget,
+    WidgetInputError,
+    )
+
+from lp.app.validators import LaunchpadValidationError
+from lp.code.browser.widgets.gitgrantee import (
+    GitGranteeDisplayWidget,
+    GitGranteeField,
+    GitGranteeWidget,
+    )
+from lp.code.enums import GitGranteeType
+from lp.registry.vocabularies import ValidPersonOrTeamVocabulary
+from lp.services.beautifulsoup import BeautifulSoup
+from lp.services.webapp.escaping import html_escape
+from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.testing import (
+    TestCaseWithFactory,
+    verifyObject,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestGitGranteeWidgetBase:
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitGranteeWidgetBase, self).setUp()
+        field = GitGranteeField(__name__="grantee")
+        request = LaunchpadTestRequest()
+        self.widget = self.widget_factory(field, request)
+
+    def test_implements(self):
+        self.assertTrue(verifyObject(IBrowserWidget, self.widget))
+        self.assertTrue(
+            verifyObject(self.expected_widget_interface, self.widget))
+
+    def test_template(self):
+        # The render template is setup.
+        self.assertTrue(
+            self.widget.template.filename.endswith("gitgrantee.pt"),
+            "Template was not set up.")
+
+    def test_default_option(self):
+        # The person field is the default option.
+        self.assertEqual("person", self.widget.default_option)
+
+    def test_setUpSubWidgets_first_call(self):
+        # The subwidget is set up and a flag is set.
+        self.widget.setUpSubWidgets()
+        self.assertTrue(self.widget._widgets_set_up)
+        self.assertIsInstance(
+            self.widget.person_widget.context.vocabulary,
+            ValidPersonOrTeamVocabulary)
+
+    def test_setUpSubWidgets_second_call(self):
+        # The setUpSubWidgets method exits early if a flag is set to
+        # indicate that the subwidget was set up.
+        self.widget._widgets_set_up = True
+        self.widget.setUpSubWidgets()
+        self.assertIsNone(getattr(self.widget, "person_widget", None))
+
+    def test_setUpOptions_default_person_checked(self):
+        # The radio button options are composed of the setup widgets with
+        # the person widget set as the default.
+        self.widget.setUpSubWidgets()
+        self.widget.setUpOptions()
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" ' +
+            self.expected_disabled_attr +
+            'id="field.grantee.option.repository_owner" name="field.grantee" '
+            'type="radio" value="repository_owner" />',
+            self.widget.options["repository_owner"])
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" ' +
+            'checked="checked" ' + self.expected_disabled_attr +
+            'id="field.grantee.option.person" name="field.grantee" '
+            'type="radio" value="person" />',
+            self.widget.options["person"])
+
+    def test_setUpOptions_repository_owner_checked(self):
+        # The repository owner radio button is selected when the form is
+        # submitted when the grantee field's value is 'repository_owner'.
+        form = {"field.grantee": "repository_owner"}
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.widget.setUpSubWidgets()
+        self.widget.setUpOptions()
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" '
+            'checked="checked" ' + self.expected_disabled_attr +
+            'id="field.grantee.option.repository_owner" name="field.grantee" '
+            'type="radio" value="repository_owner" />',
+            self.widget.options["repository_owner"])
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" ' +
+            self.expected_disabled_attr +
+            'id="field.grantee.option.person" name="field.grantee" '
+            'type="radio" value="person" />',
+            self.widget.options["person"])
+
+    def test_setUpOptions_person_checked(self):
+        # The person radio button is selected when the form is submitted
+        # when the grantee field's value is 'person'.
+        form = {"field.grantee": "person"}
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.widget.setUpSubWidgets()
+        self.widget.setUpOptions()
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" ' +
+            self.expected_disabled_attr +
+            'id="field.grantee.option.repository_owner" name="field.grantee" '
+            'type="radio" value="repository_owner" />',
+            self.widget.options["repository_owner"])
+        self.assertEqual(
+            '<input class="radioType" style="margin-left: 0;" ' +
+            'checked="checked" ' + self.expected_disabled_attr +
+            'id="field.grantee.option.person" name="field.grantee" '
+            'type="radio" value="person" />',
+            self.widget.options["person"])
+
+    def test_setRenderedValue_repository_owner(self):
+        # Passing GitGranteeType.REPOSITORY_OWNER will set the widget's
+        # render state to "repository_owner".
+        self.widget.setUpSubWidgets()
+        self.widget.setRenderedValue(GitGranteeType.REPOSITORY_OWNER)
+        self.assertEqual("repository_owner", self.widget.default_option)
+
+    def test_setRenderedValue_person(self):
+        # Passing a person will set the widget's render state to "person".
+        self.widget.setUpSubWidgets()
+        person = self.factory.makePerson()
+        self.widget.setRenderedValue(person)
+        self.assertEqual("person", self.widget.default_option)
+        self.assertEqual(person, self.widget.person_widget._getCurrentValue())
+
+    def test_call(self):
+        # The __call__ method sets up the widgets and the options.
+        markup = self.widget()
+        self.assertIsNotNone(self.widget.person_widget)
+        self.assertIn("repository_owner", self.widget.options)
+        self.assertIn("person", self.widget.options)
+        soup = BeautifulSoup(markup)
+        fields = soup.findAll(["input", "select"], {"id": re.compile(".*")})
+        ids = [field["id"] for field in fields]
+        self.assertContentEqual(self.expected_ids, ids)
+
+
+class TestGitGranteeDisplayWidget(
+        TestGitGranteeWidgetBase, TestCaseWithFactory):
+    """Test the GitGranteeDisplayWidget class."""
+
+    widget_factory = GitGranteeDisplayWidget
+    expected_widget_interface = IDisplayWidget
+    expected_disabled_attr = 'disabled="disabled" '
+    expected_ids = ["field.grantee.option.person"]
+
+
+class TestGitGranteeWidget(TestGitGranteeWidgetBase, TestCaseWithFactory):
+    """Test the GitGranteeWidget class."""
+
+    widget_factory = GitGranteeWidget
+    expected_widget_interface = IInputWidget
+    expected_disabled_attr = ""
+    expected_ids = [
+        "field.grantee.option.repository_owner",
+        "field.grantee.option.person",
+        "field.grantee.person",
+        ]
+
+    def setUp(self):
+        super(TestGitGranteeWidget, self).setUp()
+        self.person = self.factory.makePerson()
+
+    @property
+    def form(self):
+        return {
+            "field.grantee": "person",
+            "field.grantee.person": self.person.name,
+            }
+
+    def test_hasInput_not_in_form(self):
+        # hasInput is false when the widget's name is not in the form data.
+        self.widget.request = LaunchpadTestRequest(form={})
+        self.assertEqual("field.grantee", self.widget.name)
+        self.assertFalse(self.widget.hasInput())
+
+    def test_hasInput_no_person(self):
+        # hasInput is false when the person radio button is selected and the
+        # person widget's name is not in the form data.
+        self.widget.request = LaunchpadTestRequest(
+            form={"field.grantee": "person"})
+        self.assertEqual("field.grantee", self.widget.name)
+        self.assertFalse(self.widget.hasInput())
+
+    def test_hasInput_repository_owner(self):
+        # hasInput is true when the repository owner radio button is selected.
+        self.widget.request = LaunchpadTestRequest(
+            form={"field.grantee": "repository_owner"})
+        self.assertEqual("field.grantee", self.widget.name)
+        self.assertTrue(self.widget.hasInput())
+
+    def test_hasInput_person(self):
+        # hasInput is true when the person radio button is selected and the
+        # person widget's name is in the form data.
+        self.widget.request = LaunchpadTestRequest(form=self.form)
+        self.assertEqual("field.grantee", self.widget.name)
+        self.assertTrue(self.widget.hasInput())
+
+    def test_hasValidInput_true(self):
+        # The field input is valid when all submitted parts are valid.
+        self.widget.request = LaunchpadTestRequest(form=self.form)
+        self.assertTrue(self.widget.hasValidInput())
+
+    def test_hasValidInput_false(self):
+        # The field input is invalid if any of the submitted parts are invalid.
+        form = self.form
+        form["field.grantee.person"] = "non-existent"
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertFalse(self.widget.hasValidInput())
+
+    def test_getInputValue_repository_owner(self):
+        # The field value is GitGranteeType.REPOSITORY_OWNER when the
+        # repository owner radio button is selected.
+        form = self.form
+        form["field.grantee"] = "repository_owner"
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertEqual(
+            GitGranteeType.REPOSITORY_OWNER, self.widget.getInputValue())
+
+    def test_getInputValue_person(self):
+        # The field value is the person when the person radio button is
+        # selected and the person sub field is valid.
+        form = self.form
+        form["field.grantee"] = "person"
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertEqual(self.person, self.widget.getInputValue())
+
+    def test_getInputValue_person_missing(self):
+        # An error is raised when the person field is missing.
+        form = self.form
+        form["field.grantee"] = "person"
+        del form["field.grantee.person"]
+        self.widget.request = LaunchpadTestRequest(form=form)
+        message = "Please enter a person or team name"
+        e = self.assertRaises(WidgetInputError, self.widget.getInputValue)
+        self.assertEqual(LaunchpadValidationError(message), e.errors)
+
+    def test_getInputValue_person_invalid(self):
+        # An error is raised when the person is not valid.
+        form = self.form
+        form["field.grantee"] = "person"
+        form["field.grantee.person"] = "non-existent"
+        self.widget.request = LaunchpadTestRequest(form=form)
+        message = (
+            "There is no person or team named 'non-existent' registered in "
+            "Launchpad")
+        e = self.assertRaises(WidgetInputError, self.widget.getInputValue)
+        self.assertEqual(LaunchpadValidationError(message), e.errors)
+        self.assertEqual(html_escape(message), self.widget.error())