launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/flags-gui into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #616631 feature flags need administration UI
  https://bugs.launchpad.net/bugs/616631


This adds a crude readonly view of the currently active feature rules under https://launchpad.net/+feature-rules.  All the active rules are simply shown in a big textarea.  See screenshot <https://bugs.edge.launchpad.net/launchpad-foundations/+bug/616631/+attachment/1625731/+files/20100923-feature-rules.png>

I don't know if a textarea is enough, but it's the simplest thing that could possibly work, and perhaps is forgiving of editing from textmode browsers or doing multiple updates simultaneously.

At the moment this page is public; for the small number of rules we currently have that's probably fine.  I'm working on making it restricted to ~admins for write mode, and also ~launchpad for read.  I'm ok to stall landing it until that's done but I'd appreciate reviews now.
-- 
https://code.launchpad.net/~mbp/launchpad/flags-gui/+merge/36415
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/flags-gui into lp:launchpad/devel.

=== added directory 'lib/lp/services/features/browser'
=== added file 'lib/lp/services/features/browser/__init__.py'
--- lib/lp/services/features/browser/__init__.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/browser/__init__.py	2010-09-23 07:01:14 +0000
@@ -0,0 +1,33 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Feature control view"""
+
+
+from zope.interface import Interface
+
+from canonical.launchpad.webapp import (
+    LaunchpadFormView,
+    )
+
+from lp.services.features.model import (
+    getAllRules,
+    )
+
+
+class IFeatureControlForm(Interface):
+
+    def __init__(self, context):
+        self.context = context
+
+
+class FeatureControlView(LaunchpadFormView):
+
+    schema = IFeatureControlForm
+    page_title = label = 'Feature control'
+
+    def rules_text(self):
+        """Return all rules in an editable text form"""
+        return '\n'.join(
+            "\t".join((r.flag, r.scope, str(r.priority), r.value))
+            for r in getAllRules())

=== added file 'lib/lp/services/features/browser/configure.zcml'
--- lib/lp/services/features/browser/configure.zcml	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/browser/configure.zcml	2010-09-23 07:01:14 +0000
@@ -0,0 +1,20 @@
+<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+     GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure
+    xmlns="http://namespaces.zope.org/zope";
+    xmlns:browser="http://namespaces.zope.org/browser";
+    xmlns:i18n="http://namespaces.zope.org/i18n";
+    xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc";
+    i18n_domain="launchpad">
+
+    <!-- view or edit all feature rules -->
+    <browser:page
+        for="canonical.launchpad.interfaces.ILaunchpadRoot"
+        class="lp.services.features.browser.FeatureControlView"
+        name="+feature-rules"
+        permission="zope.Public"
+        template="../templates/features.pt"/>
+
+</configure>

=== added directory 'lib/lp/services/features/browser/tests'
=== added file 'lib/lp/services/features/browser/tests/__init__.py'
=== added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
--- lib/lp/services/features/browser/tests/test_feature_editor.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/browser/tests/test_feature_editor.py	2010-09-23 07:01:14 +0000
@@ -0,0 +1,73 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for feature rule editor"""
+
+__metaclass__ = type
+
+
+from testtools.matchers import (
+    Equals,
+    )
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces import ILaunchpadRoot
+from canonical.launchpad.webapp import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    BrowserTestCase, TestCase, TestCaseWithFactory, login_person,
+    person_logged_in, time_counter)
+
+from lp.services.features.browser import FeatureControlView
+from lp.services.features.flags import FeatureController
+from lp.services.features.model import addFeatureFlagRules
+
+
+class TestFeatureControlPage(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def getFeaturePageBrowser(self):
+        root = getUtility(ILaunchpadRoot)
+        url = canonical_url(root,
+            view_name='+feature-rules')
+        return self.getUserBrowser(url)
+
+    def test_feature_page_default_value(self):
+        """No rules in the sampledata gives no content in the page"""
+        browser = self.getFeaturePageBrowser()
+        textarea = browser.getControl(name="feature_rules")
+        # and by default, since there are no rules in the sample data, it's
+        # empty
+        self.assertThat(textarea.value, Equals(''))
+
+    def test_feature_page_from_database(self):
+        addFeatureFlagRules([
+            ('default', 'ui.icing', u'3.0', 100),
+            ('beta_user', 'ui.icing', u'4.0', 300),
+            ])
+        browser = self.getFeaturePageBrowser()
+        textarea = browser.getControl(name="feature_rules")
+        self.assertThat(textarea.value.replace('\r', ''), Equals("""\
+ui.icing\tbeta_user\t300\t4.0
+ui.icing\tdefault\t100\t3.0\
+"""))
+
+    def test_feature_page_submit_changes(self):
+        # XXX: read/write mode not supported yet
+        return
+        ## new_value = 'beta_user 10 some_key some value with spaces'
+        ## browser = self.getFeaturePageBrowser()
+        ## textarea = browser.getControl(name="feature_rules")
+        ## textarea.value = new_value
+        ## browser.getControl(name="submit").click()
+        ## self.assertThat(textarea.value.replace('\t', ' '),
+        ##     Equals(new_value))
+
+
+    # TODO: test that for unauthorized or anonymous users, the page works
+    # (including showing the active rules) but the textarea is readonly and
+    # there is no submit button.
+    #
+    # TODO: test that you can submit it and it changes the database.

=== modified file 'lib/lp/services/features/configure.zcml'
--- lib/lp/services/features/configure.zcml	2010-07-23 14:28:53 +0000
+++ lib/lp/services/features/configure.zcml	2010-09-23 07:01:14 +0000
@@ -6,6 +6,9 @@
     xmlns="http://namespaces.zope.org/zope";
     xmlns:browser="http://namespaces.zope.org/browser";>
 
+    <include
+        package=".browser"/>
+
     <subscriber
         for="canonical.launchpad.webapp.interfaces.IStartRequestEvent"
         handler="lp.services.features.webapp.start_request"

=== modified file 'lib/lp/services/features/model.py'
--- lib/lp/services/features/model.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/features/model.py	2010-09-23 07:01:14 +0000
@@ -2,7 +2,9 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
+    'addFeatureFlagRules',
     'FeatureFlag',
+    'getAllRules',
     'getFeatureStore',
     ]
 
@@ -10,6 +12,7 @@
 
 from storm.locals import (
     DateTime,
+    Desc,
     Int,
     Storm,
     Unicode,
@@ -48,3 +51,31 @@
         self.priority = priority
         self.flag = flag
         self.value = value
+
+
+def getAllRules():
+    """Return model objects for all rules.
+    
+    They're ordered by: flag name, then priority (descending).  If this list
+    is given to a user, they can then fairly easily scan through to see which
+    rule determines a particular value in given scopes.
+    """
+    store = getFeatureStore()
+    rs = (store
+        .find(FeatureFlag)
+        .order_by([FeatureFlag.flag, Desc(FeatureFlag.priority)]))
+    return list(rs)
+
+
+def addFeatureFlagRules(rule_list):
+    """Add rules in to the database; intended for use in testing.
+
+    :param rule_list: [(scope, flag, value, priority)...]
+    """
+    store = getFeatureStore()
+    for (scope, flag, value, priority) in rule_list:
+        store.add(FeatureFlag(
+            scope=unicode(scope),
+            flag=unicode(flag),
+            value=value,
+            priority=priority))

=== added directory 'lib/lp/services/features/templates'
=== added file 'lib/lp/services/features/templates/features.pt'
--- lib/lp/services/features/templates/features.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/templates/features.pt	2010-09-23 07:01:14 +0000
@@ -0,0 +1,30 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml";
+  xmlns:tal="http://xml.zope.org/namespaces/tal";
+  xmlns:metal="http://xml.zope.org/namespaces/metal";
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad"
+  tal:define="page_title string:features;">
+
+<body>
+
+<div metal:fill-slot="main">
+
+  <p><a href="http://dev.launchpad.net/LEP/FeatureFlags";>Feature flag</a>
+    rules currently active on Launchpad:</p>
+
+  <div metal:use-macro="context/@@launchpad_form/form">
+    <div metal:fill-slot="widgets">
+
+      <textarea style="font-family: monospace;" 
+	rows="10" columns="80"
+	readonly="true"
+	tal:content="view/rules_text" name="feature_rules">
+      </textarea>
+
+    </div>
+  </div>
+</div>
+</body>
+</html>

=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py	2010-09-12 05:19:38 +0000
+++ lib/lp/services/features/tests/test_flags.py	2010-09-23 07:01:14 +0000
@@ -49,13 +49,7 @@
         return FeatureController(scope_cb), call_log
 
     def populateStore(self):
-        store = model.getFeatureStore()
-        for (scope, flag, value, priority) in testdata:
-            store.add(model.FeatureFlag(
-                scope=unicode(scope),
-                flag=unicode(flag),
-                value=value,
-                priority=priority))
+        model.addFeatureFlagRules(testdata)
 
     def test_getFlag(self):
         self.populateStore()