launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01258
[Merge] lp:~mbp/launchpad/flags-gui into lp:launchpad/devel
You have been requested to review the proposed merge of lp:~mbp/launchpad/flags-gui into lp:launchpad/devel.
This adds two pages, https://launchpad.dev/+feature-rules andttps://launchpad.dev/+feature-rules-edit/
through which you can control feature flags.
The former is readonly and for ~launchpad; the latter is only for ~admins.
See screenshot <https://bugs.edge.launchpad.net/launchpad-foundations/+bug/616631/+attachment/1625731/+files/20100923-feature-rules.png>
Very simple one-rule-per-line textarea, which is the simplest thing that could possibly work.
This could do with a couple more tests for the ui, and perhaps some testing/handling of syntax errors, but aside from that should be good to go.
--
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-27 08:17:44 +0000
@@ -0,0 +1,45 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Feature control view"""
+
+
+import logging
+
+
+from zope.interface import Interface
+from zope.schema import (
+ Text,
+ )
+
+from canonical.launchpad.webapp import (
+ action,
+ LaunchpadFormView,
+ )
+
+
+class IFeatureControlForm(Interface):
+ """Interface specifically for editing a text form of feature rules"""
+
+ def __init__(self, context):
+ self.context = context
+
+ feature_rules = Text(title=u"Feature rules")
+
+
+class FeatureControlView(LaunchpadFormView):
+
+ schema = IFeatureControlForm
+ page_title = label = 'Feature control'
+
+ @action(u"Change", name="change")
+ def change_action(self, action, data):
+ rules_text = self.request.get('feature_rules')
+ logger = logging.getLogger('lp.services.features')
+ logger.warning("Change feature rules to: %s" % (rules_text,))
+ return self.request.features.rule_source.setAllRulesFromText(
+ rules_text)
+
+ def rules_text(self):
+ """Return all rules in an editable text form"""
+ return self.request.features.rule_source.getAllRulesAsText()
=== 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-27 08:17:44 +0000
@@ -0,0 +1,32 @@
+<!-- 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.
+
+ Readonly access is guarded by launchpad.Edit on ILaunchpadRoot, which
+ limits it to ~admins + ~registry, which are all trusted users. Write access
+ is for admins only.
+ -->
+ <browser:page
+ for="canonical.launchpad.interfaces.ILaunchpadRoot"
+ class="lp.services.features.browser.FeatureControlView"
+ name="+feature-rules"
+ permission="launchpad.Edit"
+ template="../templates/feature-rules.pt"/>
+
+ <browser:page
+ for="canonical.launchpad.interfaces.ILaunchpadRoot"
+ class="lp.services.features.browser.FeatureControlView"
+ name="+feature-rules-edit"
+ permission="launchpad.Admin"
+ template="../templates/feature-rules-edit.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-27 08:17:44 +0000
@@ -0,0 +1,111 @@
+# 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 zope.security.interfaces import Unauthorized
+
+from canonical.launchpad.interfaces import (
+ ILaunchpadCelebrities,
+ ILaunchpadRoot,
+ )
+from canonical.launchpad.webapp import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+ BrowserTestCase,
+ TestCase,
+ TestCaseWithFactory,
+ celebrity_logged_in,
+ login_person,
+ person_logged_in,
+ time_counter,
+ )
+
+from lp.services.features.browser import FeatureControlView
+from lp.services.features.model import addFeatureFlagRules
+
+
+class TestFeatureControlPage(BrowserTestCase):
+
+ layer = DatabaseFunctionalLayer
+
+ def getUserBrowserAsTeamMember(self, url, teams):
+ """Make a TestBrowser authenticated as a team member.
+
+ :param teams: List of teams to add the new user to.
+ """
+ # XXX bug=646563: To make a UserBrowser, you must know the password. This
+ # should be separated out into test infrastructure. -- mbp 20100923
+ user = self.factory.makePerson(password='test')
+ for team in teams:
+ with person_logged_in(team.teamowner):
+ team.addMember(user, reviewer=team.teamowner)
+ return self.getUserBrowser(url, user=user, password='test')
+
+ def getFeatureRulesURL(self):
+ root = getUtility(ILaunchpadRoot)
+ url = canonical_url(root, view_name='+feature-rules')
+ return url
+
+ def getFeaturePageBrowserAsAdmin(self):
+ url = self.getFeatureRulesURL()
+ admin_team = getUtility(ILaunchpadCelebrities).admin
+ return self.getUserBrowserAsTeamMember(url, [admin_team])
+
+ def test_feature_page_default_value(self):
+ """No rules in the sampledata gives no content in the page"""
+ browser = self.getFeaturePageBrowserAsAdmin()
+ 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.getFeaturePageBrowserAsAdmin()
+ 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_rules_anonymous_unauthorized(self):
+ self.assertRaises(Unauthorized,
+ self.getUserBrowser,
+ self.getFeatureRulesURL())
+
+ def test_feature_rules_peon_unauthorized(self):
+ """Logged in, but not a member of any interesting teams."""
+ self.assertRaises(Unauthorized,
+ self.getUserBrowserAsTeamMember,
+ self.getFeatureRulesURL(),
+ [])
+
+ 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-27 08:17:44 +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/flags.py'
--- lib/lp/services/features/flags.py 2010-08-18 08:51:26 +0000
+++ lib/lp/services/features/flags.py 2010-09-27 08:17:44 +0000
@@ -7,17 +7,14 @@
]
+from lp.services.features.rulesource import (
+ StormFeatureRuleSource,
+ )
+
+
__metaclass__ = type
-from storm.locals import Desc
-
-from lp.services.features.model import (
- FeatureFlag,
- getFeatureStore,
- )
-
-
class Memoize(object):
def __init__(self, calc):
@@ -65,11 +62,11 @@
of code that has consistent configuration values. For instance there will
be one per web app request.
- Intended performance: when this object is first constructed, it will read
- the whole feature flag table from the database. It is expected to be
- reasonably small. The scopes may be expensive to compute (eg checking
- team membership) so they are checked at most once when they are first
- needed.
+ Intended performance: when this object is first asked about a flag, it
+ will read the whole feature flag table from the database. It is expected
+ to be reasonably small. The scopes may be expensive to compute (eg
+ checking team membership) so they are checked at most once when
+ they are first needed.
The controller is then supposed to be held in a thread-local and reused
for the duration of the request.
@@ -77,17 +74,22 @@
@see: U{https://dev.launchpad.net/LEP/FeatureFlags}
"""
- def __init__(self, scope_check_callback):
+ def __init__(self, scope_check_callback, rule_source=None):
"""Construct a new view of the features for a set of scopes.
:param scope_check_callback: Given a scope name, says whether
- it's active or not.
+ it's active or not.
+
+ :param rule_source: Instance of StormFeatureRuleSource or similar.
"""
self._known_scopes = Memoize(scope_check_callback)
self._known_flags = Memoize(self._checkFlag)
# rules are read from the database the first time they're needed
self._rules = None
self.scopes = ScopeDict(self)
+ if rule_source is None:
+ rule_source = StormFeatureRuleSource()
+ self.rule_source = rule_source
def getFlag(self, flag):
"""Get the value of a specific flag.
@@ -102,7 +104,7 @@
def _checkFlag(self, flag):
self._needRules()
if flag in self._rules:
- for scope, value in self._rules[flag]:
+ for scope, priority, value in self._rules[flag]:
if self._known_scopes.lookup(scope):
return value
@@ -132,21 +134,9 @@
self._needRules()
return dict((f, self.getFlag(f)) for f in self._rules)
- def _loadRules(self):
- store = getFeatureStore()
- d = {}
- rs = (store
- .find(FeatureFlag)
- .order_by(Desc(FeatureFlag.priority))
- .values(FeatureFlag.flag, FeatureFlag.scope,
- FeatureFlag.value))
- for flag, scope, value in rs:
- d.setdefault(str(flag), []).append((str(scope), value))
- return d
-
def _needRules(self):
if self._rules is None:
- self._rules = self._loadRules()
+ self._rules = self.rule_source.getAllRulesAsDict()
def usedFlags(self):
"""Return dict of flags used in this controller so far."""
=== 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-27 08:17:44 +0000
@@ -2,6 +2,7 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
__all__ = [
+ 'addFeatureFlagRules',
'FeatureFlag',
'getFeatureStore',
]
@@ -10,6 +11,7 @@
from storm.locals import (
DateTime,
+ Desc,
Int,
Storm,
Unicode,
@@ -48,3 +50,17 @@
self.priority = priority
self.flag = flag
self.value = value
+
+
+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 file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/rulesource.py 2010-09-27 08:17:44 +0000
@@ -0,0 +1,90 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Returns rules defining which features are active"""
+
+__all__ = [
+ 'FeatureRuleSource',
+ 'NullFeatureRuleSource',
+ 'StormFeatureRuleSource',
+ ]
+
+__metaclass__ = type
+
+import re
+
+from storm.locals import Desc
+
+from lp.services.features.model import (
+ FeatureFlag,
+ getFeatureStore,
+ )
+
+
+class FeatureRuleSource(object):
+ """Access feature rule sources from the database or elsewhere."""
+
+ def getAllRulesAsDict(self):
+ """Return all rule definitions.
+
+ :returns: dict from flag name to an ordered list of
+ (scope, priority, value)
+ tuples, where the first matching scope should be used.
+ """
+ d = {}
+ for (flag, scope, priority, value) in self.getAllRulesAsTuples():
+ d.setdefault(str(flag), []).append((str(scope), priority, value))
+ return d
+
+ def getAllRulesAsText(self):
+ tr = []
+ for (flag, scope, priority, value) in self.getAllRulesAsTuples():
+ tr.append('\t'.join((flag, scope, str(priority), value)))
+ tr.append('')
+ return '\n'.join(tr)
+
+ def setAllRulesFromText(self, text_form):
+ self.setAllRules(self.parseRules(text_form))
+
+ def parseRules(self, text_form):
+ for line in text_form.splitlines():
+ if line.strip() == '':
+ continue
+ flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
+ yield (flag, scope, int(priority_str), unicode(value))
+
+
+class StormFeatureRuleSource(FeatureRuleSource):
+ """Access feature rules stored in the database via Storm.
+ """
+
+ def getAllRulesAsTuples(self):
+ store = getFeatureStore()
+ rs = (store
+ .find(FeatureFlag)
+ .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
+ for r in rs:
+ yield str(r.flag), str(r.scope), r.priority, r.value
+
+ def setAllRules(self, new_rules):
+ """Replace all existing rules with a new set.
+
+ :param new_rules: List of (name, scope, priority, value) tuples.
+ """
+ # XXX: would be slightly better to only update rules as necessary so we keep
+ # timestamps, and to avoid the direct sql etc -- mbp 20100924
+ store = getFeatureStore()
+ store.execute('delete from featureflag')
+ for (flag, scope, priority, value) in new_rules:
+ store.add(FeatureFlag(
+ scope=unicode(scope),
+ flag=unicode(flag),
+ value=value,
+ priority=priority))
+
+
+class NullFeatureRuleSource(FeatureRuleSource):
+ """For use in testing: everything is turned off"""
+
+ def getAllRulesAsTuples(self):
+ return []
=== added directory 'lib/lp/services/features/templates'
=== added file 'lib/lp/services/features/templates/feature-rules-edit.pt'
--- lib/lp/services/features/templates/feature-rules-edit.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/templates/feature-rules-edit.pt 2010-09-27 08:17:44 +0000
@@ -0,0 +1,29 @@
+<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">
+ <div metal:use-macro="context/@@launchpad_form/form">
+
+ <p><a href="http://dev.launchpad.net/LEP/FeatureFlags";>Feature flag</a>
+ rules currently active on Launchpad:</p>
+
+ <div metal:fill-slot="widgets">
+
+ <textarea style="font-family: monospace;"
+ rows="10" columns="80"
+ tal:content="view/rules_text" name="feature_rules">
+ </textarea>
+
+ </div>
+ </div>
+</div>
+</body>
+</html>
=== added file 'lib/lp/services/features/templates/feature-rules.pt'
--- lib/lp/services/features/templates/feature-rules.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/templates/feature-rules.pt 2010-09-27 08:17:44 +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-27 08:17:44 +0000
@@ -16,6 +16,7 @@
per_thread,
)
from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
from lp.testing import TestCase
@@ -46,16 +47,11 @@
def scope_cb(scope):
call_log.append(scope)
return scope in scopes
- return FeatureController(scope_cb), call_log
+ controller = FeatureController(scope_cb, StormFeatureRuleSource())
+ return controller, 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()
@@ -165,3 +161,50 @@
self.assertEqual(False, f.scopes['alpha_user'])
self.assertEqual(True, f.scopes['beta_user'])
self.assertEqual(['beta_user', 'alpha_user'], call_log)
+
+
+test_rules_list = [
+ (notification_name, 'beta_user', 100, notification_value),
+ ('ui.icing', 'beta_user', 300, u'4.0'),
+ ('ui.icing', 'default', 100, u'3.0'),
+ ]
+
+
+class TestStormFeatureRuleSource(TestCase):
+
+ layer = layers.DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestStormFeatureRuleSource, self).setUp()
+ if os.environ.get("STORM_TRACE", None):
+ from storm.tracer import debug
+ debug(True)
+ self.source = StormFeatureRuleSource()
+ self.source.setAllRules(test_rules_list)
+
+ def test_getAllRulesAsTuples(self):
+ self.assertEquals(list(self.source.getAllRulesAsTuples()),
+ test_rules_list)
+
+ def test_getAllRulesAsText(self):
+ self.assertEquals(self.source.getAllRulesAsText(),
+ """\
+%s\tbeta_user\t100\t%s
+ui.icing\tbeta_user\t300\t4.0
+ui.icing\tdefault\t100\t3.0
+""" % (notification_name, notification_value))
+
+ def test_setAllRulesFromText(self):
+ self.source.setAllRulesFromText("""
+
+flag1 beta_user 200 alpha
+flag1 default 100 gamma with spaces
+flag2 default 0\ton
+""")
+ self.assertEquals(self.source.getAllRulesAsDict(), {
+ 'flag1': [
+ ('beta_user', 200, 'alpha'),
+ ('default', 100, 'gamma with spaces'), ],
+ 'flag2': [
+ ('default', 0, 'on'),]})
+
=== modified file 'lib/lp/services/features/webapp.py'
--- lib/lp/services/features/webapp.py 2010-09-12 03:57:21 +0000
+++ lib/lp/services/features/webapp.py 2010-09-27 08:17:44 +0000
@@ -10,6 +10,7 @@
import canonical.config
from lp.services.features import per_thread
from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
from lp.services.propertycache import cachedproperty
@@ -80,7 +81,8 @@
def start_request(event):
"""Register FeatureController."""
event.request.features = per_thread.features = FeatureController(
- ScopesFromRequest(event.request).lookup)
+ ScopesFromRequest(event.request).lookup,
+ StormFeatureRuleSource())
def end_request(event):
References
