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

[Martin Pool]

Feature flags UI.

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.

[Jeroen T. Vermeulen (jtv)]

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...

review: Needs Fixing (code)

[Martin Pool (mbp)]

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"...

[Martin Pool (mbp)]

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.

[Martin Pool (mbp)]

> 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.

[Martin Pool (mbp)]

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.

[Robert Collins (lifeless)]

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

[Martin Pool (mbp)]

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.

[Jeroen T. Vermeulen (jtv)]

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.

> > 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     +        te...

[Jeroen T. Vermeulen (jtv)]

This is part two of my follow-up. I'm close to an Approved vote, but a quick glance at the current diff still shows the irregularly indented test strings. Have you been fixing the formatting problems I pointed out?

> >> 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.

It would have been, apart from the uncertainty over the read/edit page dichotomy. Your updated test has methods for both, making this a lot clearer to me. With that I no longer care about the docstring.

> > 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.

Still, a separate input validation pass will more or less automatically lead to better error detection and reporting. On IRC you mentioned that you can do that in a validate method, which is the right way to do it. Adding the explanatory text will also help make some mistakes more obvious.

Jeroen

[Martin Pool (mbp)]

> > 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.

I already moved it, and perhaps it's better that way as it leaves room for growth.

> > 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.

Yep, and that is better.

> I think ignoring whitespace would be a fine thing to do, assuming that a
> separate test verifies that the whitespace conforms to expectations.
>
> A Matcher would be very nice here.

Done, and reformatted to not be specially indented.

>

> > 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?
>
> I could imagine a need for both "create a rule in the database" and "create a
> rule in whatever rules source I'm currently using." Which you want this
> function for is up to you, and I defer to your judgment.

There's some other existing code that's directly poking things in to the db. I'd like to reconsider the way it sets things up, but separately from this.

[Martin Pool (mbp)]

> This is part two of my follow-up. I'm close to an Approved vote, but a quick
> glance at the current diff still shows the irregularly indented test strings.
> Have you been fixing the formatting problems I pointed out?

I have, and now that one is done too.

> Still, a separate input validation pass will more or less automatically lead
> to better error detection and reporting. On IRC you mentioned that you can do
> that in a validate method, which is the right way to do it. Adding the
> explanatory text will also help make some mistakes more obvious.

I did that.

[Jeroen T. Vermeulen (jtv)]

Much better, thanks. Still a bit of un-indented string in there, but believe it or not I'm not here to stop you from getting your branch in. :)

I'll proceed to land it for you, as requested.

review: Approve (code)

=== 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):