Launchpad itself

Merge lp:~mbp/launchpad/flags-gui into lp:launchpad

flags-gui
Merge into devel

Proposed by Martin Pool on 2010-09-23

Status:

Merged

Approved by:

Jeroen T. Vermeulen on 2010-09-29

Approved revision:

no longer in the source branch.

Merged at revision:

11657

Proposed branch:

lp:~mbp/launchpad/flags-gui

Merge into:

lp:launchpad

Diff against target:

693 lines (+476/-42)

11 files modified

lib/lp/services/features/__init__.py (+11/-1)
lib/lp/services/features/browser/configure.zcml (+25/-0)
lib/lp/services/features/browser/edit.py (+84/-0)
lib/lp/services/features/browser/tests/test_feature_editor.py (+138/-0)
lib/lp/services/features/configure.zcml (+3/-0)
lib/lp/services/features/flags.py (+19/-29)
lib/lp/services/features/model.py (+1/-0)
lib/lp/services/features/rulesource.py (+116/-0)
lib/lp/services/features/templates/feature-rules.pt (+18/-0)
lib/lp/services/features/tests/test_flags.py (+58/-11)
lib/lp/services/features/webapp.py (+3/-1)

To merge this branch:

bzr merge lp:~mbp/launchpad/flags-gui

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)	code	2010-09-23	Approve on 2010-09-29
Launchpad code reviewers		2010-09-28	Pending
Review via email: mp+36415@code.launchpad.net

Commit message

Feature flags UI.

Description of the change

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.

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-09-27:

Download full text (18.8 KiB)

Hi Martin,

The UI isn't very nice, but since this is meant for internal use only I can see the value in getting something simple working quickly. However I don't think this branch is quite ready yet.

  1 === added directory 'lib/lp/services/features/browser'
  2 === added file 'lib/lp/services/features/browser/__init__.py'
  3 --- lib/lp/services/features/browser/__init__.py 1970-01-01 00:00:00 +0000
  4 +++ lib/lp/services/features/browser/__init__.py 2010-09-27 08:17:44 +0000
  5 @@ -0,0 +1,45 @@
  6 +# Copyright 2010 Canonical Ltd. This software is licensed under the
  7 +# GNU Affero General Public License version 3 (see the file LICENSE).

I don't much care for putting code in __init__.py. I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

8 +
9 +"""Feature control view"""
10 +

Where's the __all__?

11 +
12 +import logging
13 +
14 +
15 +from zope.interface import Interface
16 +from zope.schema import (
17 + Text,
18 + )
19 +
20 +from canonical.launchpad.webapp import (
21 + action,
22 + LaunchpadFormView,
23 + )
24 +
25 +
26 +class IFeatureControlForm(Interface):
27 + """Interface specifically for editing a text form of feature rules"""
28 +
29 + def __init__(self, context):
30 + self.context = context
31 +
32 + feature_rules = Text(title=u"Feature rules")
33 +
34 +
35 +class FeatureControlView(LaunchpadFormView):

Needs a docstring.

91 === added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
92 --- lib/lp/services/features/browser/tests/test_feature_editor.py 1970-01-01 00:00:00 +0000
93 +++ lib/lp/services/features/browser/tests/test_feature_editor.py 2010-09-27 08:17:44 +0000

130 +class TestFeatureControlPage(BrowserTestCase):
131 +
132 + layer = DatabaseFunctionalLayer
133 +
134 + def getUserBrowserAsTeamMember(self, url, teams):
135 + """Make a TestBrowser authenticated as a team member.
136 +
137 + :param teams: List of teams to add the new user to.
138 + """

This method dithers between being a test setup helper and a test verification helper. Things end up muddled: when you expect getUserBrowserAsTeamMember to raise an authorization error, you're making an assertion about the "open the page" part but the code makes it look as if you're talking about the "add user to teams" part instead.

139 + # XXX bug=646563: To make a UserBrowser, you must know the password. This
140 + # should be separated out into test infrastructure. -- mbp 20100923

The XXX format as I recall it goes:

# XXX MartinPool 2010-09-23 bug=646563: To make a UserBrowser, you
# etc.

141 + user = self.factory.makePerson(password='test')
142 + for team in teams:
143 + with person_logged_in(team.teamowner):
144 + team.addMember(user, reviewer=team.teamowner)
145 + return self.getUserBrowser(url, user=user, password='test')

I would suggest not opening "url" here, so that getUserBrowserAsTeamMember can serve as an unmistakable setup helper. That also leaves you free to assume an admin identity just once (or remove...

Hi Martin,

The UI isn't very nice, but since this is meant for internal use only I can see the value in getting something simple working quickly.  However I don't think this branch is quite ready yet.

1	=== added directory 'lib/lp/services/features/browser'
  2	=== added file 'lib/lp/services/features/browser/__init__.py'
  3	--- lib/lp/services/features/browser/__init__.py	1970-01-01 00:00:00 +0000
  4	+++ lib/lp/services/features/browser/__init__.py	2010-09-27 08:17:44 +0000
  5	@@ -0,0 +1,45 @@
  6	+# Copyright 2010 Canonical Ltd.  This software is licensed under the
  7	+# GNU Affero General Public License version 3 (see the file LICENSE).

I don't much care for putting code in __init__.py.  I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

8	+
  9	+"""Feature control view"""
 10	+

Where's the __all__?

11	+
 12	+import logging 
 13	+
 14	+
 15	+from zope.interface import Interface
 16	+from zope.schema import (
 17	+    Text,
 18	+    )
 19	+
 20	+from canonical.launchpad.webapp import (
 21	+    action,
 22	+    LaunchpadFormView,
 23	+    )
 24	+
 25	+
 26	+class IFeatureControlForm(Interface):
 27	+    """Interface specifically for editing a text form of feature rules"""
 28	+
 29	+    def __init__(self, context):
 30	+        self.context = context
 31	+
 32	+    feature_rules = Text(title=u"Feature rules")
 33	+
 34	+
 35	+class FeatureControlView(LaunchpadFormView):

Needs a docstring.

91	=== added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
 92	--- lib/lp/services/features/browser/tests/test_feature_editor.py	1970-01-01  00:00:00 +0000
 93	+++ lib/lp/services/features/browser/tests/test_feature_editor.py	2010-09-27 08:17:44 +0000

130	+class TestFeatureControlPage(BrowserTestCase):
131	+
132	+    layer = DatabaseFunctionalLayer
133	+
134	+    def getUserBrowserAsTeamMember(self, url, teams):
135	+        """Make a TestBrowser authenticated as a team member.
136	+        
137	+        :param teams: List of teams to add the new user to.
138	+        """

This method dithers between being a test setup helper and a test verification helper.  Things end up muddled: when you expect getUserBrowserAsTeamMember to raise an authorization error, you're making an assertion about the "open the page" part but the code makes it look as if you're talking about the "add user to teams" part instead.

139	+        # XXX bug=646563: To make a UserBrowser, you must know the password.  This
140	+        # should be separated out into test infrastructure.  -- mbp 20100923

The XXX format as I recall it goes:

# XXX MartinPool 2010-09-23 bug=646563: To make a UserBrowser, you
    # etc.

141	+        user = self.factory.makePerson(password='test')
142	+        for team in teams:
143	+            with person_logged_in(team.teamowner):
144	+                team.addMember(user, reviewer=team.teamowner)
145	+        return self.getUserBrowser(url, user=user, password='test')

I would suggest not opening "url" here, so that getUserBrowserAsTeamMember can serve as an unmistakable setup helper.  That also leaves you free to assume an admin identity just once (or removeSecurityProxy) in to add the user to all teams.  It's only confusing for a setup method to engage with the security model in too much detail; leave that stuff for the actual actions that you're testing.

147	+    def getFeatureRulesURL(self):

Needs a docstring.

148	+        root = getUtility(ILaunchpadRoot)
149	+        url = canonical_url(root, view_name='+feature-rules')
150	+        return url

Why the extra line?

152	+    def getFeaturePageBrowserAsAdmin(self):
153	+        url = self.getFeatureRulesURL()
154	+        admin_team = getUtility(ILaunchpadCelebrities).admin
155	+        return self.getUserBrowserAsTeamMember(url, [admin_team])

It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."  Have you considered creating one?

157	+    def test_feature_page_default_value(self):
158	+        """No rules in the sampledata gives no content in the page"""
159	+        browser = self.getFeaturePageBrowserAsAdmin()
160	+        textarea = browser.getControl(name="feature_rules")
161	+        # and by default, since there are no rules in the sample data, it's
162	+        # empty
163	+        self.assertThat(textarea.value, Equals(''))

docstring and comment lack punctuation and comment lacks capitalization

165	+    def test_feature_page_from_database(self):
166	+        addFeatureFlagRules([
167	+            ('default', 'ui.icing', u'3.0', 100),
168	+            ('beta_user', 'ui.icing', u'4.0', 300),
169	+            ])
170	+        browser = self.getFeaturePageBrowserAsAdmin()
171	+        textarea = browser.getControl(name="feature_rules")
172	+        self.assertThat(textarea.value.replace('\r', ''), Equals("""\
173	+ui.icing\tbeta_user\t300\t4.0
174	+ui.icing\tdefault\t100\t3.0
175	+"""))

Try BrowserTestCase.assertTextMatchesExpressionIgnoreWhitespace instead.  It's not great, so feel free to improve it and help everyone.  (Also, if you find yourself dedenting manually like this in the future, consider using textwrap.dedent).

177	+    def test_feature_rules_anonymous_unauthorized(self):
178	+        self.assertRaises(Unauthorized,
179	+            self.getUserBrowser,
180	+            self.getFeatureRulesURL())

This will be tons clearer once "set up a browser for testing" is separate from "try opening the given page with this browser."

182	+    def test_feature_rules_peon_unauthorized(self):
183	+        """Logged in, but not a member of any interesting teams."""
184	+        self.assertRaises(Unauthorized,
185	+            self.getUserBrowserAsTeamMember,
186	+            self.getFeatureRulesURL(),
187	+            [])

"Peon," eh?  An inspired naming choice.  I like it.

189	+    def test_feature_page_submit_changes(self):
190	+        # XXX: read/write mode not supported yet
191	+        return
192	+        ## new_value = 'beta_user 10 some_key some value with spaces'
193	+        ## browser = self.getFeaturePageBrowser()
194	+        ## textarea = browser.getControl(name="feature_rules")
195	+        ## textarea.value = new_value
196	+        ## browser.getControl(name="submit").click()
197	+        ## self.assertThat(textarea.value.replace('\t', ' '),
198	+        ##     Equals(new_value))

A forgotten corner of the branch!  If you do activate the commented-out code, please use one of the accepted line-breaking styles for the assertion.  If not, remove this test altogether and file a separate bug.

201	+    # TODO: test that for unauthorized or anonymous users, the page works
202	+    # (including showing the active rules) but the textarea is readonly and
203	+    # there is no submit button.

It may help prevent accidents if you have a display page for all, plus an edit link for admins.

205	+    # TODO: test that you can submit it and it changes the database.

That'd be nice to test, yes.  (Though not vital I guess, given how much this relies on standard functionality).

322	=== modified file 'lib/lp/services/features/model.py'
323	--- lib/lp/services/features/model.py	2010-08-20 20:31:18 +0000
324	+++ lib/lp/services/features/model.py	2010-09-27 08:17:44 +0000

347	+def addFeatureFlagRules(rule_list):

A standalone function should follow the lower_case_with_underscores naming convention.

348	+    """Add rules in to the database; intended for use in testing.

Then shouldn't it live in lp.testing?  Call it makeFeatureFlagRule and it'd fit perfectly in the LaunchpadObjectFactory.

350	+    :param rule_list: [(scope, flag, value, priority)...]

How do these 4 values relate to the ones in the screenshot?  They seem to be in a different order.

360	=== added file 'lib/lp/services/features/rulesource.py'
361	--- lib/lp/services/features/rulesource.py	1970-01-01 00:00:00 +0000
362	+++ lib/lp/services/features/rulesource.py	2010-09-27 08:17:44 +0000
363	@@ -0,0 +1,90 @@
364	+# Copyright 2010 Canonical Ltd.  This software is licensed under the
365	+# GNU Affero General Public License version 3 (see the file LICENSE).
366	+
367	+"""Returns rules defining which features are active"""

This looks like a function docstring.  Could you make it applicable to the module as a whole?

I'm not sure about the design of this module.  Separating out the storage layer is obviously very useful, and it looks as if you could do with a setAllRules in NullFeatureRuleSource as well.  But the testing code and production-usable code live very close together here.  Wouldn't it be more appropriate (and consistent with our existing codebase) to have a functional FeatureRuleSource for production, plus a FakeRuleSource in lp.testing that overrides the two storage methods?

387	+class FeatureRuleSource(object):
388	+    """Access feature rule sources from the database or elsewhere."""
389	+
390	+    def getAllRulesAsDict(self):
391	+        """Return all rule definitions.
392	+
393	+        :returns: dict from flag name to an ordered list of 
394	+            (scope, priority, value) 
395	+            tuples, where the first matching scope should be used.

I don't understand what "where the first matching scope should be used" means in this context.

396	+        """
397	+        d = {}
398	+        for (flag, scope, priority, value) in self.getAllRulesAsTuples():

This leaves unsaid where getAllRulesAsTuples should come from.  I'd introduce it as a mandatory overridable through a nonfunctional "raise NotImplemented" implementation.

402	+    def getAllRulesAsText(self):

Needs a docstring.

403	+        tr = []
404	+        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
405	+            tr.append('\t'.join((flag, scope, str(priority), value)))

Would this be more naturally be expressed as a list comprehension?

406	+        tr.append('')
407	+        return '\n'.join(tr)
408	+
409	+    def setAllRulesFromText(self, text_form):
410	+        self.setAllRules(self.parseRules(text_form))

Needs a docstring.  And again please introduce setAllRules as an overridable.  That will also produce clearer errors if a test tries to set rules on a NullFeatureRuleSource.

412	+    def parseRules(self, text_form):

Needs a docstring.

413	+        for line in text_form.splitlines():
414	+            if line.strip() == '':
415	+                continue
416	+            flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
417	+            yield (flag, scope, int(priority_str), unicode(value))

This seems to be the only place where the human input gets validated before it gets flushed to the database (probably sometime after you leave setAllRulesFromText and the caller has moved on to do something else).

A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case.  What if they accidentally enter one field too many, for instance?  What of duplicate lines?  What of near-duplicate lines with different values?

Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!

420	+class StormFeatureRuleSource(FeatureRuleSource):
421	+    """Access feature rules stored in the database via Storm.
422	+    """
423	+
424	+    def getAllRulesAsTuples(self):

Needs a docstring.  If you introduce the method in FeatureRuleSource, it can just read """See `FeatureRuleSource`."""

425	+        store = getFeatureStore()
426	+        rs = (store
427	+                .find(FeatureFlag)
428	+                .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
429	+        for r in rs:
430	+            yield str(r.flag), str(r.scope), r.priority, r.value
431	+
432	+    def setAllRules(self, new_rules):
433	+        """Replace all existing rules with a new set.
434	+
435	+        :param new_rules: List of (name, scope, priority, value) tuples.
436	+        """
437	+        # XXX: would be slightly better to only update rules as necessary so we keep
438	+        # timestamps, and to avoid the direct sql etc -- mbp 20100924

XXX format.

439	+        store = getFeatureStore()
440	+        store.execute('delete from featureflag')

We usually upper-case SQL statements.  It's a bit noisy but also keeps them nicely recognizable.

449	+class NullFeatureRuleSource(FeatureRuleSource):
450	+    """For use in testing: everything is turned off"""
451	+
452	+    def getAllRulesAsTuples(self):
453	+        return []

It may make more sense to move this into lp.testing.

456	=== added file 'lib/lp/services/features/templates/feature-rules-edit.pt'
457	--- lib/lp/services/features/templates/feature-rules-edit.pt	1970-01-01 00:00:00 +0000
458	+++ lib/lp/services/features/templates/feature-rules-edit.pt	2010-09-27 08:17:44 +0000
459	@@ -0,0 +1,29 @@
460	+<html
461	+  xmlns="http://www.w3.org/1999/xhtml"
462	+  xmlns:tal="http://xml.zope.org/namespaces/tal"
463	+  xmlns:metal="http://xml.zope.org/namespaces/metal"
464	+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
465	+  metal:use-macro="view/macro:page/main_only"
466	+  i18n:domain="launchpad"
467	+  tal:define="page_title string:features;">
468	+
469	+<body>
470	+
471	+<div metal:fill-slot="main">
472	+  <div metal:use-macro="context/@@launchpad_form/form">
473	+
474	+    <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
475	+      rules currently active on Launchpad:</p>
476	+
477	+    <div metal:fill-slot="widgets">
478	+
479	+	<textarea style="font-family: monospace;"

Is the style attribute the right way to do this?  You could also try to find a single, more succinct CSS class name for this existing rule:

pre.changelog, table.diff,
.bug-comment, .bug-activity, .codereviewcomment {
    font-size: 93%;
    font-family: 'UbuntuBeta Mono', 'Ubuntu Mono', monospace;
    }

(See lib/canonical/launchpad/icing/style-3-0.css.in).

490	=== added file 'lib/lp/services/features/templates/feature-rules.pt'
491	--- lib/lp/services/features/templates/feature-rules.pt	1970-01-01 00:00:00 +0000
492	+++ lib/lp/services/features/templates/feature-rules.pt	2010-09-27 08:17:44 +0000

It may be more consistent to rename this template feature-rules-index.pt.

493	@@ -0,0 +1,30 @@
494	+<html
495	+  xmlns="http://www.w3.org/1999/xhtml"
496	+  xmlns:tal="http://xml.zope.org/namespaces/tal"
497	+  xmlns:metal="http://xml.zope.org/namespaces/metal"
498	+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
499	+  metal:use-macro="view/macro:page/main_only"
500	+  i18n:domain="launchpad"
501	+  tal:define="page_title string:features;">
502	+
503	+<body>
504	+
505	+<div metal:fill-slot="main">
506	+
507	+  <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
508	+    rules currently active on Launchpad:</p>
509	+
510	+  <div metal:use-macro="context/@@launchpad_form/form">
511	+    <div metal:fill-slot="widgets">
512	+
513	+      <textarea style="font-family: monospace;" 
514	+	rows="10" columns="80"
515	+	readonly="true"
516	+	tal:content="view/rules_text" name="feature_rules">
517	+      </textarea>

If you've got a separate template for this anyway, I think it'd be much appreciated if you could render this as a table.  It would help expose some basic mistakes, and besides, the listing sorely needs column headers!

525	=== modified file 'lib/lp/services/features/tests/test_flags.py'
526	--- lib/lp/services/features/tests/test_flags.py	2010-09-12 05:19:38 +0000
527	+++ lib/lp/services/features/tests/test_flags.py	2010-09-27 08:17:44 +0000

562	+test_rules_list = [
563	+    (notification_name, 'beta_user', 100, notification_value),
564	+    ('ui.icing', 'beta_user', 300, u'4.0'),
565	+    ('ui.icing', 'default', 100, u'3.0'),
566	+    ]
567	+
568	+
569	+class TestStormFeatureRuleSource(TestCase):
570	+
571	+    layer = layers.DatabaseFunctionalLayer
572	+
573	+    def setUp(self):
574	+        super(TestStormFeatureRuleSource, self).setUp()
575	+        if os.environ.get("STORM_TRACE", None):
576	+            from storm.tracer import debug
577	+            debug(True)

What is this for?  Can the import be moved to a more conventional place at the top of the file?

578	+        self.source = StormFeatureRuleSource()
579	+        self.source.setAllRules(test_rules_list)

Calling setAllRules here saves a line in each of the tests, but obscures the relationship between input and output.

Testing a multiplicity of data in one go is usually a bad idea: it makes for brittle tests that are also hard to fix once they break.  It also discourages testing of error conditions, corner-case inputs (empty file!) and input validation, which is currently seriously underexistent.  Better to do most of your unit-testing with single lines, and only use multiple lines when specifically testing for interactions between lines.

581	+    def test_getAllRulesAsTuples(self):
582	+        self.assertEquals(list(self.source.getAllRulesAsTuples()),
583	+            test_rules_list)

What exactly are you testing here?  The basic function of getAllRulesAsTuples for non-empty files, or that it preserves the ordering of this particular input?  In the former case, use assertContentEqual to eliminate ordering sensitivity and test separately for ordering.  In the latter case, at least show that feeding in the same ruleset in a different order will produce the same output.

Since you're line-breaking in the middle of an arguments list, could you also break the line after its opening parenthesis?

For assertEquals, we pass the expected value as the first argument and the actual value as the second.

585	+    def test_getAllRulesAsText(self):
586	+        self.assertEquals(self.source.getAllRulesAsText(),
587	+            """\
588	+%s\tbeta_user\t100\t%s
589	+ui.icing\tbeta_user\t300\t4.0
590	+ui.icing\tdefault\t100\t3.0
591	+""" % (notification_name, notification_value))

This is hard to read.  It would help if you were to construct the expected output in a separate variable.  Also, the expected output should come before the actual output in the arguments list and the first argument shouldn't be on the same line as the method name if later arguments to the same call aren't on the same line.  And again, consider using either the BrowserTestCase helper for whitespace-insensitive comparison or textwrap.dedent (and perhaps the "strip" methods) to fix the irregular indentation.

593	+    def test_setAllRulesFromText(self):
594	+        self.source.setAllRulesFromText("""
595	+
596	+flag1   beta_user   200   alpha
597	+flag1   default     100   gamma with spaces
598	+flag2   default     0\ton
599	+""")

Here I'd definitely recommend dedent and friends.

600	+        self.assertEquals(self.source.getAllRulesAsDict(), {

The expected value should come first.

601	+            'flag1': [
602	+                ('beta_user', 200, 'alpha'),
603	+                ('default', 100, 'gamma with spaces'), ],
604	+            'flag2': [
605	+                ('default', 0, 'on'),]})

Neither the dict nor its embedded lists follow our syntax conventions.  There should be a newline before the closing brace or bracket.  This is easier to get right if you store the dict in a variable so that you can pass it by name.

Jeroen

review: Needs Fixing (code)

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-28:

Download full text (23.3 KiB)

Hi, thanks for putting so much work into the review. Everything I
haven't commented on I agree with.

> The UI isn't very nice, but since this is meant for internal use only I can see the value in getting something simple working quickly.

Right, getting something better than raw sql is useful, and perhaps
some experience with it will show what people most want to have
changed. If you have ideas about what an ideal UI would be like I'd
be happy to hear them, at least for a later branch.

On 27 September 2010 22:17, Jeroen T. Vermeulen <email address hidden> wrote:
> Review: Needs Fixing code
> Hi Martin,
>
However I don't think this branch is quite ready yet.
>
>
> 1 === added directory 'lib/lp/services/features/browser'
> 2 === added file 'lib/lp/services/features/browser/__init__.py'
> 3 --- lib/lp/services/features/browser/__init__.py 1970-01-01 00:00:00 +0000
> 4 +++ lib/lp/services/features/browser/__init__.py 2010-09-27 08:17:44 +0000
> 5 @@ -0,0 +1,45 @@
> 6 +# Copyright 2010 Canonical Ltd. This software is licensed under the
> 7 +# GNU Affero General Public License version 3 (see the file LICENSE).
>
> I don't much care for putting code in __init__.py. I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

OK. Can I make it just browser.py, or should I have a submodule with
a named file within it?

>
>
> 8 +
> 9 +"""Feature control view"""
> 10 +
>
> Where's the __all__?
>
>
> 11 +
> 12 +import logging
> 13 +
> 14 +
> 15 +from zope.interface import Interface
> 16 +from zope.schema import (
> 17 + Text,
> 18 + )
> 19 +
> 20 +from canonical.launchpad.webapp import (
> 21 + action,
> 22 + LaunchpadFormView,
> 23 + )
> 24 +
> 25 +
> 26 +class IFeatureControlForm(Interface):
> 27 + """Interface specifically for editing a text form of feature rules"""
> 28 +
> 29 + def __init__(self, context):
> 30 + self.context = context
> 31 +
> 32 + feature_rules = Text(title=u"Feature rules")
> 33 +
> 34 +
> 35 +class FeatureControlView(LaunchpadFormView):
>
> Needs a docstring.
>
>
> 91 === added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
> 92 --- lib/lp/services/features/browser/tests/test_feature_editor.py 1970-01-01 00:00:00 +0000
> 93 +++ lib/lp/services/features/browser/tests/test_feature_editor.py 2010-09-27 08:17:44 +0000
>
> 130 +class TestFeatureControlPage(BrowserTestCase):
> 131 +
> 132 + layer = DatabaseFunctionalLayer
> 133 +
> 134 + def getUserBrowserAsTeamMember(self, url, teams):
> 135 + """Make a TestBrowser authenticated as a team member.
> 136 +
> 137 + :param teams: List of teams to add the new user to.
> 138 + """
>
> This method dithers between being a test setup helper and a test verification helper. Things end up muddled: when you expect getUserBrowserAsTeamMember to raise an authorization error, you're making an assertion about the "open the page"...

Hi, thanks for putting so much work into the review.  Everything I
haven't commented on I agree with.

> The UI isn't very nice, but since this is meant for internal use only I can see the value in getting something simple working quickly.

Right, getting something better than raw sql is useful, and perhaps
some experience with it will show what people most want to have
changed.  If you have ideas about what an ideal UI would be like I'd
be happy to hear them, at least for a later branch.

On 27 September 2010 22:17, Jeroen T. Vermeulen <jtv@canonical.com> wrote:
> Review: Needs Fixing code
> Hi Martin,
>
  However I don't think this branch is quite ready yet.
>
>
>  1     === added directory 'lib/lp/services/features/browser'
>  2     === added file 'lib/lp/services/features/browser/__init__.py'
>  3     --- lib/lp/services/features/browser/__init__.py        1970-01-01 00:00:00 +0000
>  4     +++ lib/lp/services/features/browser/__init__.py        2010-09-27 08:17:44 +0000
>  5     @@ -0,0 +1,45 @@
>  6     +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>  7     +# GNU Affero General Public License version 3 (see the file LICENSE).
>
> I don't much care for putting code in __init__.py.  I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

OK.  Can I make it just browser.py, or should I have a submodule with
a named file within it?

>
>
>  8     +
>  9     +"""Feature control view"""
>  10     +
>
> Where's the __all__?
>
>
>  11     +
>  12     +import logging
>  13     +
>  14     +
>  15     +from zope.interface import Interface
>  16     +from zope.schema import (
>  17     +    Text,
>  18     +    )
>  19     +
>  20     +from canonical.launchpad.webapp import (
>  21     +    action,
>  22     +    LaunchpadFormView,
>  23     +    )
>  24     +
>  25     +
>  26     +class IFeatureControlForm(Interface):
>  27     +    """Interface specifically for editing a text form of feature rules"""
>  28     +
>  29     +    def __init__(self, context):
>  30     +        self.context = context
>  31     +
>  32     +    feature_rules = Text(title=u"Feature rules")
>  33     +
>  34     +
>  35     +class FeatureControlView(LaunchpadFormView):
>
> Needs a docstring.
>
>
>  91     === added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
>  92     --- lib/lp/services/features/browser/tests/test_feature_editor.py       1970-01-01  00:00:00 +0000
>  93     +++ lib/lp/services/features/browser/tests/test_feature_editor.py       2010-09-27 08:17:44 +0000
>
> 130     +class TestFeatureControlPage(BrowserTestCase):
> 131     +
> 132     +    layer = DatabaseFunctionalLayer
> 133     +
> 134     +    def getUserBrowserAsTeamMember(self, url, teams):
> 135     +        """Make a TestBrowser authenticated as a team member.
> 136     +
> 137     +        :param teams: List of teams to add the new user to.
> 138     +        """
>
> This method dithers between being a test setup helper and a test verification helper.  Things end up muddled: when you expect getUserBrowserAsTeamMember to raise an authorization error, you're making an assertion about the "open the page" part but the code makes it look as if you're talking about the "add user to teams" part instead.

Hm, so perhaps (if it's possible) it would be clearer to write something like:

browser = getUserBrowserAsTeamMember(teams)
  self.assertRaises(Unauthorized, browser.open(url))

>
>
> 139     +        # XXX bug=646563: To make a UserBrowser, you must know the password.  This
> 140     +        # should be separated out into test infrastructure.  -- mbp 20100923
>
> The XXX format as I recall it goes:
>
>    # XXX MartinPool 2010-09-23 bug=646563: To make a UserBrowser, you
>    # etc.

Thanks.

>
>
> 141     +        user = self.factory.makePerson(password='test')
> 142     +        for team in teams:
> 143     +            with person_logged_in(team.teamowner):
> 144     +                team.addMember(user, reviewer=team.teamowner)
> 145     +        return self.getUserBrowser(url, user=user, password='test')
>
> I would suggest not opening "url" here, so that getUserBrowserAsTeamMember can serve as an unmistakable setup helper.  That also leaves you free to assume an admin identity just once (or removeSecurityProxy) in to add the user to all teams.  It's only confusing for a setup method to engage with the security model in too much detail; leave that stuff for the actual actions that you're testing.

>
>
> 147     +    def getFeatureRulesURL(self):
>
> Needs a docstring.
>
>
> 148     +        root = getUtility(ILaunchpadRoot)
> 149     +        url = canonical_url(root, view_name='+feature-rules')
> 150     +        return url
>
> Why the extra line?
>
>
> 152     +    def getFeaturePageBrowserAsAdmin(self):
> 153     +        url = self.getFeatureRulesURL()
> 154     +        admin_team = getUtility(ILaunchpadCelebrities).admin
> 155     +        return self.getUserBrowserAsTeamMember(url, [admin_team])
>
> It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."  Have you considered creating one?

Right, that's bug 646563, but this branch is getting big enough already.

To me it seems overspecific to say "admin browser" in particular.

It seems strange this doesn't already exist, but I can't find it if it does.

>
>
> 157     +    def test_feature_page_default_value(self):
> 158     +        """No rules in the sampledata gives no content in the page"""
> 159     +        browser = self.getFeaturePageBrowserAsAdmin()
> 160     +        textarea = browser.getControl(name="feature_rules")
> 161     +        # and by default, since there are no rules in the sample data, it's
> 162     +        # empty
> 163     +        self.assertThat(textarea.value, Equals(''))
>
> docstring and comment lack punctuation and comment lacks capitalization
>
>
> 165     +    def test_feature_page_from_database(self):
> 166     +        addFeatureFlagRules([
> 167     +            ('default', 'ui.icing', u'3.0', 100),
> 168     +            ('beta_user', 'ui.icing', u'4.0', 300),
> 169     +            ])
> 170     +        browser = self.getFeaturePageBrowserAsAdmin()
> 171     +        textarea = browser.getControl(name="feature_rules")
> 172     +        self.assertThat(textarea.value.replace('\r', ''), Equals("""\
> 173     +ui.icing\tbeta_user\t300\t4.0
> 174     +ui.icing\tdefault\t100\t3.0
> 175     +"""))
>
> Try BrowserTestCase.assertTextMatchesExpressionIgnoreWhitespace instead.  It's not great, so feel free to improve it and help everyone.  (Also, if you find yourself dedenting manually like this in the future, consider using textwrap.dedent).

Are we actually better off ignoring whitespace?  Perhaps so.  If I was
going to change it, I'd change it into a testtools Matcher.

>
>
> 177     +    def test_feature_rules_anonymous_unauthorized(self):
> 178     +        self.assertRaises(Unauthorized,
> 179     +            self.getUserBrowser,
> 180     +            self.getFeatureRulesURL())
>
> This will be tons clearer once "set up a browser for testing" is separate from "try opening the given page with this browser."

Right.

>
>
> 182     +    def test_feature_rules_peon_unauthorized(self):
> 183     +        """Logged in, but not a member of any interesting teams."""
> 184     +        self.assertRaises(Unauthorized,
> 185     +            self.getUserBrowserAsTeamMember,
> 186     +            self.getFeatureRulesURL(),
> 187     +            [])
>
> "Peon," eh?  An inspired naming choice.  I like it.

:) Possible 'plebian' is more accurate.

>
>
> 189     +    def test_feature_page_submit_changes(self):
> 190     +        # XXX: read/write mode not supported yet
> 191     +        return
> 192     +        ## new_value = 'beta_user 10 some_key some value with spaces'
> 193     +        ## browser = self.getFeaturePageBrowser()
> 194     +        ## textarea = browser.getControl(name="feature_rules")
> 195     +        ## textarea.value = new_value
> 196     +        ## browser.getControl(name="submit").click()
> 197     +        ## self.assertThat(textarea.value.replace('\t', ' '),
> 198     +        ##     Equals(new_value))
>
> A forgotten corner of the branch!

Not forgotten, I just wanted to get some review in parallel with
finishing it off.

> If you do activate the commented-out code, please use one of the accepted line-breaking styles for the assertion.  If not, remove this test altogether and file a separate bug.

iow with the first line break immediately after the opening parenthesis.

>
>
> 201     +    # TODO: test that for unauthorized or anonymous users, the page works
> 202     +    # (including showing the active rules) but the textarea is readonly and
> 203     +    # there is no submit button.
>
> It may help prevent accidents if you have a display page for all, plus an edit link for admins.

Mm, I actually ended up deciding to take that course, but the comment
is out of date with it.

Also I need an actual link from the view page to the edit page.

>
>
> 205     +    # TODO: test that you can submit it and it changes the database.
>
> That'd be nice to test, yes.  (Though not vital I guess, given how much this relies on standard functionality).
>
>
> 322     === modified file 'lib/lp/services/features/model.py'
> 323     --- lib/lp/services/features/model.py   2010-08-20 20:31:18 +0000
> 324     +++ lib/lp/services/features/model.py   2010-09-27 08:17:44 +0000
>
> 347     +def addFeatureFlagRules(rule_list):
>
> A standalone function should follow the lower_case_with_underscores naming convention.

stand_alone_function vs objectMethod? omg.

ok.

>
>
> 348     +    """Add rules in to the database; intended for use in testing.
>
> Then shouldn't it live in lp.testing?  Call it makeFeatureFlagRule and it'd fit perfectly in the LaunchpadObjectFactory.

Actually I think for most testing, we want to just change the
featurecontroller, not actually change the database.  And perhaps it
should be a test fixture not on that monolith class?
>
>
> 350     +    :param rule_list: [(scope, flag, value, priority)...]
>
> How do these 4 values relate to the ones in the screenshot?  They seem to be in a different order.

I think the screenshot is out of date.  I want to consistently use that order.

>
>
> 360     === added file 'lib/lp/services/features/rulesource.py'
> 361     --- lib/lp/services/features/rulesource.py      1970-01-01 00:00:00 +0000
> 362     +++ lib/lp/services/features/rulesource.py      2010-09-27 08:17:44 +0000
> 363     @@ -0,0 +1,90 @@
> 364     +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> 365     +# GNU Affero General Public License version 3 (see the file LICENSE).
> 366     +
> 367     +"""Returns rules defining which features are active"""
>
> This looks like a function docstring.  Could you make it applicable to the module as a whole?

Do you mean, rephrase it so it talks about what kind of thing the
module contains, rather than what the class does.

>
> I'm not sure about the design of this module.  Separating out the storage layer is obviously very useful, and it looks as if you could do with a setAllRules in NullFeatureRuleSource as well.  But the testing code and production-usable code live very close together here.  Wouldn't it be more appropriate (and consistent with our existing codebase) to have a functional FeatureRuleSource for production, plus a FakeRuleSource in lp.testing that overrides the two storage methods?

If the consistent pattern is that implementations only for testing use
go into lp.testing then I'll move it there.

>
>
> 387     +class FeatureRuleSource(object):
> 388     +    """Access feature rule sources from the database or elsewhere."""
> 389     +
> 390     +    def getAllRulesAsDict(self):
> 391     +        """Return all rule definitions.
> 392     +
> 393     +        :returns: dict from flag name to an ordered list of
> 394     +            (scope, priority, value)
> 395     +            tuples, where the first matching scope should be used.
>
> I don't understand what "where the first matching scope should be used" means in this context.

I'll clarify that it means: take the first tuple from the returned
list that names an active scope, and use that value.

>
>
> 396     +        """
> 397     +        d = {}
> 398     +        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
>
> This leaves unsaid where getAllRulesAsTuples should come from.  I'd introduce it as a mandatory overridable through a nonfunctional "raise NotImplemented" implementation.
>
>
> 402     +    def getAllRulesAsText(self):
>
> Needs a docstring.
>
>
> 403     +        tr = []
> 404     +        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
> 405     +            tr.append('\t'.join((flag, scope, str(priority), value)))
>
> Would this be more naturally be expressed as a list comprehension?
>
>
> 406     +        tr.append('')
> 407     +        return '\n'.join(tr)
> 408     +
> 409     +    def setAllRulesFromText(self, text_form):
> 410     +        self.setAllRules(self.parseRules(text_form))
>
> Needs a docstring.  And again please introduce setAllRules as an overridable.  That will also produce clearer errors if a test tries to set rules on a NullFeatureRuleSource.
>
>
> 412     +    def parseRules(self, text_form):
>
> Needs a docstring.
>
>
> 413     +        for line in text_form.splitlines():
> 414     +            if line.strip() == '':
> 415     +                continue
> 416     +            flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
> 417     +            yield (flag, scope, int(priority_str), unicode(value))
>
> This seems to be the only place where the human input gets validated before it gets flushed to the database (probably sometime after you leave setAllRulesFromText and the caller has moved on to do something else).
>
> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case.  What if they accidentally enter one field too many, for instance?  What of duplicate lines?  What of near-duplicate lines with different values?
>
> Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!
>
>
> 420     +class StormFeatureRuleSource(FeatureRuleSource):
> 421     +    """Access feature rules stored in the database via Storm.
> 422     +    """
> 423     +
> 424     +    def getAllRulesAsTuples(self):
>
> Needs a docstring.  If you introduce the method in FeatureRuleSource, it can just read """See `FeatureRuleSource`."""
>
>
> 425     +        store = getFeatureStore()
> 426     +        rs = (store
> 427     +                .find(FeatureFlag)
> 428     +                .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
> 429     +        for r in rs:
> 430     +            yield str(r.flag), str(r.scope), r.priority, r.value
> 431     +
> 432     +    def setAllRules(self, new_rules):
> 433     +        """Replace all existing rules with a new set.
> 434     +
> 435     +        :param new_rules: List of (name, scope, priority, value) tuples.
> 436     +        """
> 437     +        # XXX: would be slightly better to only update rules as necessary so we keep
> 438     +        # timestamps, and to avoid the direct sql etc -- mbp 20100924
>
> XXX format.
>
>
> 439     +        store = getFeatureStore()
> 440     +        store.execute('delete from featureflag')
>
> We usually upper-case SQL statements.  It's a bit noisy but also keeps them nicely recognizable.
>
>
> 449     +class NullFeatureRuleSource(FeatureRuleSource):
> 450     +    """For use in testing: everything is turned off"""
> 451     +
> 452     +    def getAllRulesAsTuples(self):
> 453     +        return []
>
> It may make more sense to move this into lp.testing.
>
>
> 456     === added file 'lib/lp/services/features/templates/feature-rules-edit.pt'
> 457     --- lib/lp/services/features/templates/feature-rules-edit.pt    1970-01-01 00:00:00 +0000
> 458     +++ lib/lp/services/features/templates/feature-rules-edit.pt    2010-09-27 08:17:44 +0000
> 459     @@ -0,0 +1,29 @@
> 460     +<html
> 461     +  xmlns="http://www.w3.org/1999/xhtml"
> 462     +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> 463     +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> 464     +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> 465     +  metal:use-macro="view/macro:page/main_only"
> 466     +  i18n:domain="launchpad"
> 467     +  tal:define="page_title string:features;">
> 468     +
> 469     +<body>
> 470     +
> 471     +<div metal:fill-slot="main">
> 472     +  <div metal:use-macro="context/@@launchpad_form/form">
> 473     +
> 474     +    <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
> 475     +      rules currently active on Launchpad:</p>
> 476     +
> 477     +    <div metal:fill-slot="widgets">
> 478     +
> 479     +       <textarea style="font-family: monospace;"
>
> Is the style attribute the right way to do this?  You could also try to find a single, more succinct CSS class name for this existing rule:
>
> pre.changelog, table.diff,
> .bug-comment, .bug-activity, .codereviewcomment {
>    font-size: 93%;
>    font-family: 'UbuntuBeta Mono', 'Ubuntu Mono', monospace;
>    }
>
> (See lib/canonical/launchpad/icing/style-3-0.css.in).

You're probably right that it should use a class not an inline style.

(I think tweaking the size for what's essentially body text is a bit
strange, since the user has presumably already set the size to
something they like and Launchpad has an annoying habit of using
inconsistent sizes, but that's a separate issue.  Avoiding inline css
can only help.)

>
>
> 490     === added file 'lib/lp/services/features/templates/feature-rules.pt'
> 491     --- lib/lp/services/features/templates/feature-rules.pt 1970-01-01 00:00:00 +0000
> 492     +++ lib/lp/services/features/templates/feature-rules.pt 2010-09-27 08:17:44 +0000
>
> It may be more consistent to rename this template feature-rules-index.pt.
>
>
> 493     @@ -0,0 +1,30 @@
> 494     +<html
> 495     +  xmlns="http://www.w3.org/1999/xhtml"
> 496     +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> 497     +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> 498     +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> 499     +  metal:use-macro="view/macro:page/main_only"
> 500     +  i18n:domain="launchpad"
> 501     +  tal:define="page_title string:features;">
> 502     +
> 503     +<body>
> 504     +
> 505     +<div metal:fill-slot="main">
> 506     +
> 507     +  <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
> 508     +    rules currently active on Launchpad:</p>
> 509     +
> 510     +  <div metal:use-macro="context/@@launchpad_form/form">
> 511     +    <div metal:fill-slot="widgets">
> 512     +
> 513     +      <textarea style="font-family: monospace;"
> 514     +       rows="10" columns="80"
> 515     +       readonly="true"
> 516     +       tal:content="view/rules_text" name="feature_rules">
> 517     +      </textarea>
>
> If you've got a separate template for this anyway, I think it'd be much appreciated if you could render this as a table.  It would help expose some basic mistakes, and besides, the listing sorely needs column headers!

>
>
> 525     === modified file 'lib/lp/services/features/tests/test_flags.py'
> 526     --- lib/lp/services/features/tests/test_flags.py        2010-09-12 05:19:38 +0000
> 527     +++ lib/lp/services/features/tests/test_flags.py        2010-09-27 08:17:44 +0000
>
> 562     +test_rules_list = [
> 563     +    (notification_name, 'beta_user', 100, notification_value),
> 564     +    ('ui.icing', 'beta_user', 300, u'4.0'),
> 565     +    ('ui.icing', 'default', 100, u'3.0'),
> 566     +    ]
> 567     +
> 568     +
> 569     +class TestStormFeatureRuleSource(TestCase):
> 570     +
> 571     +    layer = layers.DatabaseFunctionalLayer
> 572     +
> 573     +    def setUp(self):
> 574     +        super(TestStormFeatureRuleSource, self).setUp()
> 575     +        if os.environ.get("STORM_TRACE", None):
> 576     +            from storm.tracer import debug
> 577     +            debug(True)
>
> What is this for?  Can the import be moved to a more conventional place at the top of the file?
>
>
> 578     +        self.source = StormFeatureRuleSource()
> 579     +        self.source.setAllRules(test_rules_list)
>
> Calling setAllRules here saves a line in each of the tests, but obscures the relationship between input and output.
>
> Testing a multiplicity of data in one go is usually a bad idea: it makes for brittle tests that are also hard to fix once they break.  It also discourages testing of error conditions, corner-case inputs (empty file!) and input validation, which is currently seriously underexistent.  Better to do most of your unit-testing with single lines, and only use multiple lines when specifically testing for interactions between lines.
>
>
> 581     +    def test_getAllRulesAsTuples(self):
> 582     +        self.assertEquals(list(self.source.getAllRulesAsTuples()),
> 583     +            test_rules_list)
>
> What exactly are you testing here?  The basic function of getAllRulesAsTuples for non-empty files, or that it preserves the ordering of this particular input?  In the former case, use assertContentEqual to eliminate ordering sensitivity and test separately for ordering.  In the latter case, at least show that feeding in the same ruleset in a different order will produce the same output.

The ordering does matter, and I can add a test that we get it right.

>
> Since you're line-breaking in the middle of an arguments list, could you also break the line after its opening parenthesis?
>
> For assertEquals, we pass the expected value as the first argument and the actual value as the second.

That's a good reason for me to use assertThat so I can't get it wrong.

>
>
> 585     +    def test_getAllRulesAsText(self):
> 586     +        self.assertEquals(self.source.getAllRulesAsText(),
> 587     +            """\
> 588     +%s\tbeta_user\t100\t%s
> 589     +ui.icing\tbeta_user\t300\t4.0
> 590     +ui.icing\tdefault\t100\t3.0
> 591     +""" % (notification_name, notification_value))
>
> This is hard to read.  It would help if you were to construct the expected output in a separate variable.  Also, the expected output should come before the actual output in the arguments list and the first argument shouldn't be on the same line as the method name if later arguments to the same call aren't on the same line.  And again, consider using either the BrowserTestCase helper for whitespace-insensitive comparison or textwrap.dedent (and perhaps the "strip" methods) to fix the irregular indentation.
>
>
> 593     +    def test_setAllRulesFromText(self):
> 594     +        self.source.setAllRulesFromText("""
> 595     +
> 596     +flag1   beta_user   200   alpha
> 597     +flag1   default     100   gamma with spaces
> 598     +flag2   default     0\ton
> 599     +""")
>
> Here I'd definitely recommend dedent and friends.
>
>
> 600     +        self.assertEquals(self.source.getAllRulesAsDict(), {
>
> The expected value should come first.
>
>
> 601     +            'flag1': [
> 602     +                ('beta_user', 200, 'alpha'),
> 603     +                ('default', 100, 'gamma with spaces'), ],
> 604     +            'flag2': [
> 605     +                ('default', 0, 'on'),]})
>
> Neither the dict nor its embedded lists follow our syntax conventions.  There should be a newline before the closing brace or bracket.  This is easier to get right if you store the dict in a variable so that you can pass it by name.

OK, thanks.

-- 
Martin

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-28:

Oh, I'll just mention that one reason i used a textarea even for the readonly view is so that the rules can easily be copied/pasted, mailed, and then re-pasted back in to the writable view. So I will stick with that for now, but add some text explaining how the fields should be interpreted.

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-28:

> I don't much care for putting code in __init__.py. I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

I realize you said this an a preference not a rule, but I got the idea this was common because there are dozens of existing modules with nontrivial inits. Perhaps that's no longer prefered. If so, it should go into https://dev.launchpad.net/PythonStyleGuide.

>> 147 + def getFeatureRulesURL(self):
> Needs a docstring.

This is the kind of thing where I question "everything should have a docstring." It's in test code; it's only used in this class; the name is pretty selfexplanatory and it's only two lines of code.

> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case. What if they accidentally enter one field too many, for instance? What of duplicate lines? What of near-duplicate lines with different values? Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!

If we look at the various species of pilot error:

0- syntactically invalid, for instance having not enough fields or conflicting priorities: you'll get an oops; it would be much nicer to give you a clean message explaining where you went wrong. however the change won't be committed to the database and it has no lasting impact.

1 - syntactically valid, but not what you meant to say: will be committed to the database and may cause arbitrarily weird things to happen to users.

To me 1 is more important to guard against for obvious reasons.

To reduce the chance of that happening there are various things we can do:
* show a diff of the changes before they're applied
* have a smarter editor that knows what options are available and what values they can take
* allow reverting changes
* store and show a history of changes

I think fixing 1 will largely obsolete 0 because we'll go away from just having a plaintext editor; so I'm not so interested in fixing it by itself. (This is, I admit, also an argument from impatience.)

I think the risk is tolerable because the people with access to this already can and do run arbitrary shell and sql commands, and the damage they can do here is strictly less than is possible with them. I would like to get this just rolling too, and give them something better than raw sql to use.

> I don't much care for putting code in __init__.py.  I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

I realize you said this an a preference not a rule, but I got the idea this was common because there are dozens of existing modules with nontrivial inits.  Perhaps that's no longer prefered.  If so, it should go into https://dev.launchpad.net/PythonStyleGuide.

>> 147     +    def getFeatureRulesURL(self):
> Needs a docstring.

This is the kind of thing where I question "everything should have a docstring."  It's in test code; it's only used in this class; the name is pretty selfexplanatory and it's only two lines of code.

> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case.  What if they accidentally enter one field too many, for instance?  What of duplicate lines?  What of near-duplicate lines with different values? Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!

If we look at the various species of pilot error:

0- syntactically invalid, for instance having not enough fields or conflicting priorities: you'll get an oops; it would be much nicer to give you a clean message explaining where you went wrong.  however the change won't be committed to the database and it has no lasting impact.

1 - syntactically valid, but not what you meant to say: will be committed to the database and may cause arbitrarily weird things to happen to users.

To me 1 is more important to guard against for obvious reasons.

To reduce the chance of that happening there are various things we can do:
 * show a diff of the changes before they're applied
 * have a smarter editor that knows what options are available and what values they can take
 * allow reverting changes
 * store and show a history of changes

I think fixing 1 will largely obsolete 0 because we'll go away from just having a plaintext editor; so I'm not so interested in fixing it by itself.  (This is, I admit, also an argument from impatience.)

I think the risk is tolerable because the people with access to this already can and do run arbitrary shell and sql commands, and the damage they can do here is strictly less than is possible with them.  I would like to get this just rolling too, and give them something better than raw sql to use.

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-28:

The current changes address, I think, everything substantial here. The ui is not ideal, but I think is better than raw sql, and I'm impatient to get something like that up.

I may see about changing it to have just one url, that appears in either readonly or read-write mode as appropriate.

Revision history for this message

Robert Collins (lifeless) wrote on 2010-09-28:

On Tue, Sep 28, 2010 at 10:23 PM, Martin Pool <email address hidden> wrote:
>> I don't much care for putting code in __init__.py. I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.
>
> I realize you said this an a preference not a rule, but I got the idea this was common because there are dozens of existing modules with nontrivial inits. Perhaps that's no longer prefered. If so, it should go into https://dev.launchpad.net/PythonStyleGuide.

Putting stuff in __init__ is fine. If its not clear, or the file is
'large', split it out - but thats a normal rule anyhow.

>>> 147 + def getFeatureRulesURL(self):
>> Needs a docstring.
>
> This is the kind of thing where I question "everything should have a docstring." It's in test code; it's only used in this class; the name is pretty selfexplanatory and it's only two lines of code.

Agreed; I certainly woudn't put a docstring unless its nonobvious what it is.

> I think the risk is tolerable because the people with access to this already can and do run arbitrary shell and sql commands, and the damage they can do here is strictly less than is possible with them. I would like to get this just rolling too, and give them something better than raw sql to use.

Also by definition these config values will (at worst) enable a
feature to users that didn't expect it (or conversely disable things
we didn't want disabled [because they needed an explicit-on to work].

Not a huge issue. I like the idea of a diff showing 'this is what
changed', if you care to do it - but I wouldn't block on having it.

-Rob

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-29:

I'm going to do one more change here, which is to consider unifying
the view and edit pages, which will also fix an issue of a useless
"submit" button appearing on the view page, and perhaps will remove
some unnecessary repetition. Then I think this is ready to land; more
can be done both in UI and other aspects of flags but this is an
incremental improvement.

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-09-29:

Download full text (6.9 KiB)

Hi Martin,

Following up in two parts. This is part one. I, too, will skip the parts I agree with and which seem to require no further action.

> > 1 === added directory 'lib/lp/services/features/browser'
> > 2 === added file 'lib/lp/services/features/browser/__init__.py'
> > 3 --- lib/lp/services/features/browser/__init__.py 1970-01-01
> 00:00:00 +0000
> > 4 +++ lib/lp/services/features/browser/__init__.py 2010-09-27
> 08:17:44 +0000
> > 5 @@ -0,0 +1,45 @@
> > 6 +# Copyright 2010 Canonical Ltd. This software is licensed under the
> > 7 +# GNU Affero General Public License version 3 (see the file
> LICENSE).
> >
> > I don't much care for putting code in __init__.py. I can see that it keeps
> the imports shorter, but it's unusual enough that it may be easy to miss.
>
> OK. Can I make it just browser.py, or should I have a submodule with
> a named file within it?

If a nontrivial __init__.py is already a normal thing to write, then there's no reason to worry about it here after all. So it's fine to leave it as it is.

> > 91 === added file
> 'lib/lp/services/features/browser/tests/test_feature_editor.py'
> > 92 --- lib/lp/services/features/browser/tests/test_feature_editor.py
> 1970-01-01 00:00:00 +0000
> > 93 +++ lib/lp/services/features/browser/tests/test_feature_editor.py
> 2010-09-27 08:17:44 +0000
> >
> > 130 +class TestFeatureControlPage(BrowserTestCase):
> > 131 +
> > 132 + layer = DatabaseFunctionalLayer
> > 133 +
> > 134 + def getUserBrowserAsTeamMember(self, url, teams):
> > 135 + """Make a TestBrowser authenticated as a team member.
> > 136 +
> > 137 + :param teams: List of teams to add the new user to.
> > 138 + """
> >
> > This method dithers between being a test setup helper and a test
> verification helper. Things end up muddled: when you expect
> getUserBrowserAsTeamMember to raise an authorization error, you're making an
> assertion about the "open the page" part but the code makes it look as if
> you're talking about the "add user to teams" part instead.
>
> Hm, so perhaps (if it's possible) it would be clearer to write something like:
>
> browser = getUserBrowserAsTeamMember(teams)
> self.assertRaises(Unauthorized, browser.open(url))

Yes, exactly. And AFAICS that should work.

> > 152 + def getFeaturePageBrowserAsAdmin(self):
> > 153 + url = self.getFeatureRulesURL()
> > 154 + admin_team = getUtility(ILaunchpadCelebrities).admin
> > 155 + return self.getUserBrowserAsTeamMember(url, [admin_team])
> >
> > It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."
> Have you considered creating one?
>
> Right, that's bug 646563, but this branch is getting big enough already.

Very well.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

James Troup

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Martin Pool

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

 === modified file 'lib/lp/services/features/__init__.py'
 --- lib/lp/services/features/__init__.py	2010-09-21 13:16:25 +0000
 +++ lib/lp/services/features/__init__.py	2010-09-29 08:38:48 +0000
@@ -137,6 +137,7 @@
  __all__ = [
++    'get_relevant_feature_controller',
      'getFeatureFlag',
      'per_thread',
+     ]
@@ -150,11 +151,20 @@
  """
++def get_relevant_feature_controller():
++    """Get a FeatureController for this thread."""
++
++    # The noncommittal name "relevant" is because this function may change to
++    # look things up from the current request or some other mechanism in
++    # future.
++    return getattr(per_thread, 'features', None)
++
++
  def getFeatureFlag(flag):
      """Get the value of a flag for this thread's scopes."""
      # Workaround for bug 631884 - features have two homes, threads and
      # requests.
--    features = getattr(per_thread, 'features', None)
++    features = get_relevant_feature_controller()
      if features is None:
          return None
      return features.getFlag(flag)
 === added directory 'lib/lp/services/features/browser'
 === added file 'lib/lp/services/features/browser/__init__.py'
 === 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-29 08:38:48 +0000
@@ -0,0 +1,25 @@
++<!-- 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.edit.FeatureControlView"
++        name="+feature-rules"
++        permission="launchpad.Edit"
++        template="../templates/feature-rules.pt"/>
++
++</configure>
 === added file 'lib/lp/services/features/browser/edit.py'
 --- lib/lp/services/features/browser/edit.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/services/features/browser/edit.py	2010-09-29 08:38:48 +0000
@@ -0,0 +1,84 @@
++# Copyright 2010 Canonical Ltd.  This software is licensed under the
++# GNU Affero General Public License version 3 (see the file LICENSE).
++
++"""View and edit feature rules."""
++
++__metaclass__ = type
++__all__ = [
++    'FeatureControlView',
++    'IFeatureControlForm',
++    ]
++
++
++import logging
++
++
++from zope.interface import Interface
++from zope.schema import (
++    Text,
++    )
++
++from canonical.launchpad.webapp import (
++    action,
++    enabled_with_permission,
++    LaunchpadFormView,
++    )
++from canonical.launchpad.webapp.authorization import (
++    check_permission,
++    )
++
++
++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",
++        description=(
++            u"Rules to control feature flags on Launchpad.  "
++            u"On each line: (flag, scope, priority, value), "
++            u"whitespace-separated.  Numerically higher "
++            u"priorities match first."
++            ),
++        required=False,
++        )
++
++
++class FeatureControlView(LaunchpadFormView):
++    """Text view of feature rules.
++
++    Presents a text area, either read-only or read-write, showing currently
++    active rules.
++    """
++
++    schema = IFeatureControlForm
++    page_title = label = 'Feature control'
++    field_names = ['feature_rules']
++
++    @action(u"Change", name="change")
++    def change_action(self, action, data):
++        if not check_permission('launchpad.Admin', self.context):
++            raise Unauthorized()
++        rules_text = data.get('feature_rules') or ''
++        logger = logging.getLogger('lp.services.features')
++        logger.warning("Change feature rules to: %s" % (rules_text,))
++        self.request.features.rule_source.setAllRulesFromText(
++            rules_text)
++
++    @property
++    def initial_values(self):
++        return dict(
++            feature_rules=self.request.features.rule_source.getAllRulesAsText(),
++            )
++
++    def validate(self, data):
++        # Try parsing the rules so we give a clean error: at the moment the
++        # message is not great, but it's better than an oops.
++        try:
++            # Unfortunately if the field is '', zope leaves it out of data.
++            self.request.features.rule_source.parseRules(
++                data.get('feature_rules') or '')
++        except (IndexError, TypeError, ValueError), e:
++            self.setFieldError('feature_rules', 'Invalid rule syntax: %s' % e)
 === 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-29 08:38:48 +0000
@@ -0,0 +1,138 @@
++# 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 import (
++    get_relevant_feature_controller,
++    )
++from lp.services.features.browser.edit import (
++    FeatureControlView,
++    )
++from lp.services.features.rulesource import (
++    StormFeatureRuleSource,
++    )
++
++
++class TestFeatureControlPage(BrowserTestCase):
++
++    layer = DatabaseFunctionalLayer
++
++    def getUserBrowserAsTeamMember(self, teams):
++        """Make a TestBrowser authenticated as a team member.
++
++        :param teams: List of teams to add the new user to.
++        """
++        # XXX MartinPool 2010-09-23 bug=646563: To make a UserBrowser, you
++        # must know the password; we can't get the password for an existing
++        # user so we have to make a new one.
++        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=None, user=user, password='test')
++
++    def getUserBrowserAsAdmin(self):
++        """Make a new TestBrowser logged in as an admin user."""
++        url = self.getFeatureRulesViewURL()
++        admin_team = getUtility(ILaunchpadCelebrities).admin
++        return self.getUserBrowserAsTeamMember([admin_team])
++
++    def getFeatureRulesViewURL(self):
++        root = getUtility(ILaunchpadRoot)
++        return canonical_url(root, view_name='+feature-rules')
++
++    def getFeatureRulesEditURL(self):
++        root = getUtility(ILaunchpadRoot)
++        return canonical_url(root, view_name='+feature-rules')
++
++    def test_feature_page_default_value(self):
++        """No rules in the sampledata gives no content in the page"""
++        browser = self.getUserBrowserAsAdmin()
++        browser.open(self.getFeatureRulesViewURL())
++        textarea = browser.getControl(name="field.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):
++        StormFeatureRuleSource().setAllRules([
++            ('ui.icing', 'default',   100, u'3.0'),
++            ('ui.icing', 'beta_user', 300, u'4.0'),
++            ])
++        browser = self.getUserBrowserAsAdmin()
++        browser.open(self.getFeatureRulesViewURL())
++        textarea = browser.getControl(name="field.feature_rules")
++        self.assertThat(
++            textarea.value.replace('\r', ''),
++            Equals(
++                "ui.icing\tbeta_user\t300\t4.0\n"
++                "ui.icing\tdefault\t100\t3.0\n"))
++
++    def test_feature_rules_anonymous_unauthorized(self):
++        browser = self.getUserBrowser()
++        self.assertRaises(Unauthorized,
++            browser.open,
++            self.getFeatureRulesViewURL())
++
++    def test_feature_rules_plebian_unauthorized(self):
++        """Logged in, but not a member of any interesting teams."""
++        browser = self.getUserBrowserAsTeamMember([])
++        self.assertRaises(Unauthorized,
++            browser.open,
++            self.getFeatureRulesViewURL())
++
++    def test_feature_page_submit_changes(self):
++        """Submitted changes show up in the db."""
++        browser = self.getUserBrowserAsAdmin()
++        browser.open(self.getFeatureRulesEditURL())
++        new_value = 'beta_user some_key 10 some value with spaces'
++        textarea = browser.getControl(name="field.feature_rules")
++        textarea.value = new_value
++        browser.getControl(name="field.actions.change").click()
++        self.assertThat(
++            list(StormFeatureRuleSource().getAllRulesAsTuples()),
++            Equals([
++                ('beta_user', 'some_key', 10, 'some value with spaces'),
++                ]))
++
++    def test_feature_page_submit_change_to_empty(self):
++        """Correctly handle submitting an empty value."""
++        # Zope has the quirk of conflating empty with absent; make sure we
++        # handle it properly.
++        browser = self.getUserBrowserAsAdmin()
++        browser.open(self.getFeatureRulesEditURL())
++        new_value = ''
++        textarea = browser.getControl(name="field.feature_rules")
++        textarea.value = new_value
++        browser.getControl(name="field.actions.change").click()
++        self.assertThat(
++            list(StormFeatureRuleSource().getAllRulesAsTuples()),
++            Equals([
++                ]))
 === 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-29 08:38:48 +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-29 08:38:48 +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-29 08:38:48 +0000
@@ -10,6 +10,7 @@
  from storm.locals import (
      DateTime,
++    Desc,
      Int,
      Storm,
      Unicode,
 === 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-29 08:38:48 +0000
@@ -0,0 +1,116 @@
++# 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 a list of
++            (scope, priority, value)
++            in descending order by priority.
++        """
++        d = {}
++        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
++            d.setdefault(str(flag), []).append((str(scope), priority, value))
++        return d
++
++    def getAllRulesAsTuples(self):
++        """Generate list of (flag, scope, priority, value)"""
++        raise NotImplementedError()
++
++    def getAllRulesAsText(self):
++        """Return a text for of the rules.
++
++        This has one line per rule, with tab-separate
++        (flag, scope, prioirity, value), as used in the flag editor web
++        interface.
++        """
++        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):
++        """Update all rules from text input.
++
++        The input is similar in form to that generated by getAllRulesAsText:
++        one line per rule, with whitespace-separated (flag, scope,
++        priority, value).  Whitespace is allowed in the flag value.
++
++        """
++        self.setAllRules(self.parseRules(text_form))
++
++    def parseRules(self, text_form):
++        """Return a list of tuples for the parsed form of the text input.
++
++        For each non-blank line gives back a tuple of (flag, scope, priority, value).
++
++        Returns a list rather than a generator so that you see any syntax
++        errors immediately.
++        """
++        r = []
++        for line in text_form.splitlines():
++            if line.strip() == '':
++                continue
++            flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
++            r.append((flag, scope, int(priority_str), unicode(value)))
++        return r
++
++
++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.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-29 08:38:48 +0000
@@ -0,0 +1,18 @@
++<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">
++  </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-29 08:38:48 +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
@@ -24,9 +25,9 @@
  testdata = [
--    ('beta_user', notification_name, notification_value, 100),
--    ('default', 'ui.icing', u'3.0', 100),
--    ('beta_user', 'ui.icing', u'4.0', 300),
++    (notification_name, 'beta_user', 100, notification_value),
++    ('ui.icing', 'default', 100, u'3.0'),
++    ('ui.icing', 'beta_user', 300, u'4.0'),
+     ]
@@ -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))
++        StormFeatureRuleSource().setAllRules(testdata)
      def test_getFlag(self):
          self.populateStore()
@@ -165,3 +161,54 @@
          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 test_getAllRulesAsTuples(self):
++        source = StormFeatureRuleSource()
++        source.setAllRules(test_rules_list)
++        self.assertEquals(
++            test_rules_list,
++            list(source.getAllRulesAsTuples()))
++
++    def test_getAllRulesAsText(self):
++        source = StormFeatureRuleSource()
++        source.setAllRules(test_rules_list)
++        self.assertEquals(
++            """\
++%s\tbeta_user\t100\t%s
++ui.icing\tbeta_user\t300\t4.0
++ui.icing\tdefault\t100\t3.0
++""" % (notification_name, notification_value),
++            source.getAllRulesAsText())
++
++    def test_setAllRulesFromText(self):
++        # We will overwrite existing data.
++        source = StormFeatureRuleSource()
++        source.setAllRules(test_rules_list)
++        source.setAllRulesFromText("""
++
++flag1   beta_user   200   alpha
++flag1   default     100   gamma with spaces
++flag2   default     0\ton
++""")
++        self.assertEquals({
++            'flag1': [
++                ('beta_user', 200, 'alpha'),
++                ('default', 100, 'gamma with spaces'),
++                ],
++            'flag2': [
++                ('default', 0, 'on'),
++                ],
++            },
++            source.getAllRulesAsDict())
 === 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-29 08:38:48 +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):