Merge lp:~benji/launchpad/bug-636193 into lp:launchpad

[Benji York]

[r=lifeless][ui=salgado][bug=636193] add a listing of the current (documented) feature flags and scopes

Bug 636193 is about the need to know what feature flags and scopes are
available, what values are valid for them, and what they do.

To that end, I've added simple registries for feature flags and scopes.
However we still want adding feature flags to be lightweight, therefore
requiring documentation or registration for a feature flag before using
it is not desired.

The same does not go for scopes. They are added only rarely and asking
about a non-existent scope is more likely an error, therefore only known
scopes are allowed and an exception is raised when an unknown scope is
requested.

The pre-implementation discussion took place on bug 636193; the outcome
being comment #12 (https://bugs.launchpad.net/launchpad/+bug/636193/comments/12).

Because this effort was primarily a refactoring, relatively few new
tests were needed. All of the feature flag package's tests can be run
like so:

    bin/test c lp.services.features

The feature flags and scopes documentation can be seen at the
/+feature-info page. A link to that page was added to /+feature-rules
so it will be at hand when editing rules through the web.

The only lint output that persists is this:

    ./lib/lp/services/features/templates/feature-info.pt
        65: mismatched tag

...but that can't be helped. If I change the structure of the template
to appease lint, the page template machinery explodes. There can be
only one.

[Robert Collins (lifeless)]

517 + # If we didn't find at least one matching handler, then the requested
518 + # scope is unknown and we want to alert the caller that they did
519 + # something wrong.
520 + if not found_a_handler:
521 + raise LookupError('Unknown scope: %r. This can result from a '
522 + 'typo or perhaps you need to create a new scope handler.'
523 + % (scope_name,))
524

This is a bug: its an operational requirement that the server not crash if a scope goes away during a deploy. Likewise we need to be able to add a rule with scope that isn't used yet but will become active on the next deploy. We can (and should) warn about unrecognised scopes in the +feature-rules page but thats a different discussion.

review: Needs Fixing

[Benji York (benji)]

I've addressed the issue Robert noted.

[Guilherme Salgado (salgado)]

The new page looks fine to me, and other than the link to the LEP suggested by Robert, I think it'd be great to have some examples of complete rules and maybe even user testing it by showing it to a dev and asking them to write a rule to, say, enable a feature X only for members of team Y.

review: Approve (ui*)

[Robert Collins (lifeless)]

So we discussed some possible improvements on IRC, but the key thing of meeting the constraints about deploys is fixed. Thanks!

review: Approve

[Guilherme Salgado (salgado)]

Benji, I'm still being mentored as a UI reviewer (that's what the * in the review type means), so my mentor (Curtis) should've had a look at this before you landed. Sorry if it wasn't clear. I've requested a review from him.

[Benji York (benji)]

On Thu, Jan 27, 2011 at 6:09 AM, Guilherme Salgado
<email address hidden> wrote:
> Benji, I'm still being mentored as a UI reviewer (that's what the * in the
> review type means), so my mentor (Curtis) should've had a look at this before
> you landed. Sorry if it wasn't clear.  I've requested a review from him

=== modified file 'lib/lp/services/features/browser/configure.zcml'
--- lib/lp/services/features/browser/configure.zcml	2010-11-08 13:45:59 +0000
+++ lib/lp/services/features/browser/configure.zcml	2011-01-26 22:11:32 +0000
@@ -11,9 +11,9 @@
 
     <!-- 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.
+     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.webapp.interfaces.ILaunchpadRoot"
@@ -22,4 +22,16 @@
         permission="launchpad.Edit"
         template="../templates/feature-rules.pt"/>
 
+    <!-- View documentary info about the available feature flags.
+
+     Access is guarded by launchpad.Edit on ILaunchpadRoot, just like
+     +feature-rules.
+    -->
+    <browser:page
+        for="canonical.launchpad.webapp.interfaces.ILaunchpadRoot"
+        class="lp.services.features.browser.info.FeatureInfoView"
+        name="+feature-info"
+        permission="launchpad.Edit"
+        template="../templates/feature-info.pt"/>
+
 </configure>

=== added file 'lib/lp/services/features/browser/info.py'
--- lib/lp/services/features/browser/info.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/browser/info.py	2011-01-26 22:11:32 +0000
@@ -0,0 +1,58 @@
+# Copyright 2011 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__ = [
+    'FeatureInfoView',
+    ]
+
+
+from collections import namedtuple
+
+from canonical.launchpad.webapp.publisher import LaunchpadView
+from lp.services.features.flags import (
+    flag_info,
+    undocumented_flags,
+    )
+from lp.services.features.scopes import (
+    HANDLERS,
+    undocumented_scopes,
+    )
+from lp.services.utils import docstring_dedent
+
+
+# Named tuples to use when passing flag and scope data to the template.
+Flag = namedtuple('Flag', ('name', 'domain', 'description', 'default'))
+Scope = namedtuple('Scope', ('regex', 'description'))
+
+
+class FeatureInfoView(LaunchpadView):
+    """Display feature flag documentation and other info."""
+
+    page_title = label = 'Feature flag info'
+
+    @property
+    def flag_info(self):
+        """A list of flags as named tuples, ready to be rendered."""
+        return map(Flag._make, flag_info)
+
+    @property
+    def undocumented_flags(self):
+        """Flag names referenced during process lifetime but not documented.
+        """
+        return ', '.join(undocumented_flags)
+
+    @property
+    def undocumented_scopes(self):
+        """Scope names referenced during process lifetime but not documented.
+        """
+        return ', '.join(undocumented_scopes)
+
+    @property
+    def scope_info(self):
+        """A list of scopes as named tuples, ready to be rendered."""
+        return [
+            Scope._make((handler.pattern, docstring_dedent(handler.__doc__)))
+            for handler in HANDLERS]

=== added file 'lib/lp/services/features/browser/tests/test_feature_info.py'
--- lib/lp/services/features/browser/tests/test_feature_info.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/browser/tests/test_feature_info.py	2011-01-26 22:11:32 +0000
@@ -0,0 +1,159 @@
+# Copyright 2011 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 Not
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.services.features.flags import (
+    documented_flags,
+    flag_info,
+    NullFeatureController,
+    undocumented_flags,
+    )
+from lp.services.features.scopes import (
+    HANDLERS,
+    undocumented_scopes,
+    )
+from lp.testing import (
+    BrowserTestCase,
+    person_logged_in,
+    TestCase,
+    )
+from lp.testing.matchers import Contains
+
+
+class TestFeatureControlPage(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def getFeatureInfoUrl(self):
+        """Find the URL to the feature info page."""
+        root = getUtility(ILaunchpadRoot)
+        return canonical_url(root, view_name='+feature-info')
+
+    def getUserBrowserAsAdmin(self):
+        """Make a new TestBrowser logged in as an admin user."""
+        url = self.getFeatureInfoUrl()
+        admin_team = getUtility(ILaunchpadCelebrities).admin
+        return self.getUserBrowserAsTeamMember([admin_team])
+
+    def getUserBrowserAsTeamMember(self, teams):
+        """Make a TestBrowser authenticated as a team member."""
+        # XXX Martin Pool 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 test_feature_documentation_displayed(self):
+        """The feature flag documentation is displayed on the page."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureInfoUrl())
+        for record in flag_info:
+            for item in record:
+                self.assertThat(browser.contents, Contains(item))
+
+    def test_scope_documentation_displayed(self):
+        """The scope documentation is displayed on the page."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureInfoUrl())
+        for pattern in [handler.pattern for handler in HANDLERS]:
+            self.assertThat(browser.contents, Contains(pattern))
+
+    def test_undocumented_features_displayed(self):
+        """The undocumented feature flag names are displayed on the page."""
+        browser = self.getUserBrowserAsAdmin()
+        # Stash away any already encountered undocumented flags.
+        saved_undocumented = undocumented_flags.copy()
+        undocumented_flags.clear()
+        undocumented_flags.update(['first', 'second'])
+        browser.open(self.getFeatureInfoUrl())
+        # Put the saved undocumented flags back.
+        undocumented_flags.clear()
+        undocumented_flags.update(saved_undocumented)
+        # Are the (injected) undocumented flags shown in the page?
+        self.assertThat(browser.contents, Contains('first'))
+        self.assertThat(browser.contents, Contains('second'))
+
+    def test_undocumented_scope_displayed(self):
+        """The undocumented scope names are displayed on the page."""
+        browser = self.getUserBrowserAsAdmin()
+        # Stash away any already encountered undocumented scopes.
+        saved_undocumented = undocumented_scopes.copy()
+        undocumented_scopes.clear()
+        undocumented_scopes.update(['first', 'second'])
+        browser.open(self.getFeatureInfoUrl())
+        # Put the saved undocumented scopes back.
+        undocumented_scopes.clear()
+        undocumented_scopes.update(saved_undocumented)
+        # Are the (injected) undocumented scopes shown in the page?
+        self.assertThat(browser.contents, Contains('first'))
+        self.assertThat(browser.contents, Contains('second'))
+
+    def test_feature_info_anonymous_unauthorized(self):
+        """Anonymous users can not view the feature flag info page."""
+        browser = self.getUserBrowser()
+        self.assertRaises(Unauthorized,
+            browser.open,
+            self.getFeatureInfoUrl())
+
+    def test_feature_rules_plebian_unauthorized(self):
+        """Unauthorized logged-in users can't view the info page."""
+        browser = self.getUserBrowserAsTeamMember([])
+        self.assertRaises(Unauthorized,
+            browser.open,
+            self.getFeatureInfoUrl())
+
+
+class TestUndocumentedFeatureFlags(TestCase):
+    """Test the code that records accessing of undocumented feature flags."""
+
+    def setUp(self):
+        super(TestUndocumentedFeatureFlags, self).setUp()
+        # Stash away any already encountered undocumented flags.
+        saved_undocumented = undocumented_flags.copy()
+        saved_documented = documented_flags.copy()
+        undocumented_flags.clear()
+        documented_flags.clear()
+
+        def clean_up_undocumented_flags():
+            # Put the saved undocumented flags back.
+            undocumented_flags.clear()
+            documented_flags.clear()
+            undocumented_flags.update(saved_undocumented)
+            documented_flags.update(saved_documented)
+
+        self.addCleanup(clean_up_undocumented_flags)
+
+    def test_reading_undocumented_feature_flags(self):
+        """Reading undocumented feature flags records them as undocumented."""
+        controller = NullFeatureController()
+        # This test assumes there is no flag named "does-not-exist".
+        assert 'does-not-exist' not in documented_flags
+        controller.getFlag('does-not-exist')
+        self.assertThat(undocumented_flags, Contains('does-not-exist'))
+
+    def test_reading_documented_feature_flags(self):
+        """Reading documented flags does not record them as undocumented."""
+        controller = NullFeatureController()
+        # Make sure there is no flag named "documented-flag-name" before we
+        # start testing.
+        assert 'documented-flag-name' not in documented_flags
+        documented_flags.update(['documented-flag-name'])
+        controller.getFlag('documented-flag-name')
+        self.assertThat(
+            undocumented_flags,
+            Not(Contains('documented-flag-name')))

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2010-10-02 10:51:03 +0000
+++ lib/lp/services/features/flags.py	2011-01-26 22:11:32 +0000
@@ -3,7 +3,9 @@
 
 __all__ = [
     'FeatureController',
+    'flag_info',
     'NullFeatureController',
+    'undocumented_flags',
     ]
 
 
@@ -15,8 +17,39 @@
 
 __metaclass__ = type
 
-
-class Memoize(object):
+# This table of flag name, value domain, and prose documentation is used to
+# generate the web-visible feature flag documentation.
+flag_info = sorted([
+    ('code.recipes_enabled',
+     '[on|off]',
+     'enable recipes',
+     'off'),
+    ('hard_timeout',
+     'float',
+     'sets the hard timeout in seconds',
+     ''),
+    ('malone.advanced-subscriptions.enabled',
+     '[on|off]',
+     'enables advanced subscriptions features',
+     'off'),
+    ('memcache',
+     '[enabled|disabled]',
+     'enables/disables memcache',
+     'enabled'),
+    ('publicrestrictedlibrarian',
+     '[on|off]',
+     'redirect to private URLs instead of proxying',
+     'off'),
+    ])
+
+# The set of all flag names that are documented.
+documented_flags = set(info[0] for info in flag_info)
+# The set of all the flags names that have been used during the process
+# lifetime, but were not documented in flag_info.
+undocumented_flags = set()
+
+
+class Memoize():
 
     def __init__(self, calc):
         self._known = {}
@@ -30,7 +63,7 @@
         return v
 
 
-class ScopeDict(object):
+class ScopeDict():
     """Allow scopes to be looked up by getitem"""
 
     def __init__(self, features):
@@ -40,7 +73,7 @@
         return self.features.isInScope(scope_name)
 
 
-class FeatureController(object):
+class FeatureController():
     """A FeatureController tells application code what features are active.
 
     It does this by meshing together two sources of data:
@@ -94,12 +127,15 @@
 
     def getFlag(self, flag):
         """Get the value of a specific flag.
-        
+
         :param flag: A name to lookup. e.g. 'recipes.enabled'
 
         :return: The value of the flag determined by the highest priority rule
         that matched.
         """
+        # If this is an undocumented flag, record it.
+        if flag not in documented_flags:
+            undocumented_flags.add(flag)
         return self._known_flags.lookup(flag)
 
     def _checkFlag(self, flag):

=== added file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/scopes.py	2011-01-26 22:11:32 +0000
@@ -0,0 +1,177 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Connect Feature flags into webapp requests."""
+
+__all__ = [
+    'HANDLERS',
+    'ScopesFromRequest',
+    'undocumented_scopes',
+    ]
+
+__metaclass__ = type
+
+import re
+
+from zope.component import getUtility
+
+from canonical.launchpad.webapp.interfaces import ILaunchBag
+from lp.services.propertycache import cachedproperty
+import canonical.config
+
+
+undocumented_scopes = set()
+
+
+class BaseScope():
+    """A base class for scope handlers.
+
+    The docstring of subclasses is used on the +feature-info page as
+    documentation, so write them accordingly.
+    """
+
+    # The regex pattern used to decide if a handler can evalute a particular
+    # scope.  Also used on +feature-info.
+    pattern = None
+
+    def __init__(self, request):
+        self.request = request
+
+    @cachedproperty
+    def compiled_pattern(self):
+        """The compiled scope matching regex.  A small optimization."""
+        return re.compile(self.pattern)
+
+    def lookup(self, scope_name):
+        """Returns true if the given scope name is "active"."""
+        raise NotImplementedError('Subclasses of BaseScope must implement '
+            'lookup.')
+
+
+class DefaultScope(BaseScope):
+    """The default scope.  Always active."""
+
+    pattern = r'default$'
+
+    def lookup(self, scope_name):
+        return True
+
+
+class PageScope(BaseScope):
+    """The current page ID.
+
+    Pageid scopes are written as 'pageid:' + the pageid to match.  Pageids
+    are treated as a namespace with : and # delimiters.
+
+    For example, the scope 'pageid:Foo' will be active on pages with pageids:
+        Foo
+        Foo:Bar
+        Foo#quux
+    """
+
+    pattern = r'pageid:'
+
+    def lookup(self, scope_name):
+        """Is the given scope match the current pageid?"""
+        pageid_scope = scope_name[len('pageid:'):]
+        scope_segments = self._pageid_to_namespace(pageid_scope)
+        request_segments = self._request_pageid_namespace
+        # In 2.6, this can be replaced with izip_longest
+        for pos, name in enumerate(scope_segments):
+            if pos == len(request_segments):
+                return False
+            if request_segments[pos] != name:
+                return False
+        return True
+
+    @staticmethod
+    def _pageid_to_namespace(pageid):
+        """Return a list of namespace elements for pageid."""
+        # Normalise delimiters.
+        pageid = pageid.replace('#', ':')
+        # Create a list to walk, empty namespaces are elided.
+        return [name for name in pageid.split(':') if name]
+
+    @cachedproperty
+    def _request_pageid_namespace(self):
+        return tuple(self._pageid_to_namespace(
+            self.request._orig_env.get('launchpad.pageid', '')))
+
+
+class TeamScope(BaseScope):
+    """The current user's team memberships.
+
+    Team ID scopes are written as 'team:' + the team name to match.
+
+    The scope 'team:launchpad-beta-users' will match members of the team
+    'launchpad-beta-users'.
+    """
+
+    pattern = r'team:'
+
+    def lookup(self, scope_name):
+        """Is the given scope a team membership?
+
+        This will do a two queries, so we probably want to keep the number of
+        team based scopes in use to a small number. (Person.inTeam could be
+        fixed to reduce this to one query).
+        """
+        team_name = scope_name[len('team:'):]
+        person = getUtility(ILaunchBag).user
+        if person is None:
+            return False
+        return person.inTeam(team_name)
+
+
+class ServerScope(BaseScope):
+    """Matches the current server.
+
+    For example, the scope server.lpnet is active when is_lpnet is set to True
+    in the Launchpad configuration.
+    """
+
+    pattern = r'server\.'
+
+    def lookup(self, scope_name):
+        """Match the current server as a scope."""
+        server_name = scope_name.split('.', 1)[1]
+        try:
+            return canonical.config.config['launchpad']['is_' + server_name]
+        except KeyError:
+            pass
+        return False
+
+
+# These are the handlers for all of the allowable scopes.  Any new scope will
+# need a scope handler and that scope handler has to be added to this list.
+# See BaseScope for hints as to what a scope handler should look like.
+HANDLERS = [DefaultScope, PageScope, TeamScope, ServerScope]
+
+
+class ScopesFromRequest():
+    """Identify feature scopes based on request state."""
+
+    def __init__(self, request):
+        self.request = request
+        self.handlers = [f(request) for f in HANDLERS]
+
+    def lookup(self, scope_name):
+        """Determine if scope_name applies to this request.
+
+        This method iterates over the configured scope hanlders until it
+        either finds one that claims the requested scope name is a match for
+        the current request or the handlers are exhuasted, in which case the
+        scope name is not a match.
+        """
+        found_a_handler = False
+        for handler in self.handlers:
+            if handler.compiled_pattern.match(scope_name):
+                found_a_handler = True
+                if handler.lookup(scope_name):
+                    return True
+
+        # If we didn't find at least one matching handler, then the requested
+        # scope is unknown and we want to record the scope for the +flag-info page to display.
+        if not found_a_handler:
+            undocumented_scopes.add(scope_name)
+

=== removed file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	2010-07-21 11:05:14 +0000
+++ lib/lp/services/features/scopes.py	1970-01-01 00:00:00 +0000
@@ -1,5 +0,0 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Look up current feature flags.
-"""

=== added file 'lib/lp/services/features/templates/feature-info.pt'
--- lib/lp/services/features/templates/feature-info.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/templates/feature-info.pt	2011-01-26 22:11:32 +0000
@@ -0,0 +1,75 @@
+<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 metal:fill-slot="main">
+
+  <h2>Documented flags</h2>
+  <table class="listing">
+    <thead>
+      <tr>
+        <th>Name</th>
+        <th>Value domain</th>
+        <th>Default value</th>
+        <th>Description</th>
+      </tr>
+    </thead>
+    <tbody>
+      <tr tal:repeat="info view/flag_info">
+        <td tal:content="info/name">flag name here</td>
+        <td tal:content="info/domain">flag domain here</td>
+        <td tal:content="info/default">flag description here</td>
+        <td tal:content="info/description">flag description here</td>
+      </tr>
+    </tbody>
+  </table>
+
+  <p>
+  <h2>Undocumented flags</h2>
+  These flags were referenced during this process' lifetime but are not
+  documented:
+  <strong tal:condition="not:view/undocumented_flags">
+    No undocumented feature flags have been used yet.
+  </strong>
+  <strong tal:content="view/undocumented_flags">list of flags</strong>
+
+  <p>
+  <h2>Scopes</h2>
+
+  The table below describes the currently available scopes.  The first column
+  gives the regular expression the scope matches (for example, the
+  "pageid:foo" scopes match the regex "pageid:") and the second gives a
+  description of when the scope is active.
+
+  <p>
+  <table class="listing">
+    <thead>
+      <tr>
+        <th>Form (a regex)</th>
+        <th>Description</th>
+      </tr>
+    </thead>
+    <tbody>
+      <tr tal:repeat="info view/scope_info">
+        <td tal:content="info/regex">scope regex here</td>
+        <td><pre tal:content="info/description">scope description here</pre></td>
+      </tr>
+    </tbody>
+  </table>
+
+  <p>
+  <h2>Undocumented scopes</h2>
+  These scopes were referenced during this process' lifetime but are not
+  documented:
+  <strong tal:condition="not:view/undocumented_scopes">
+    No undocumented scopes have been used yet.
+  </strong>
+  <strong tal:content="view/undocumented_scopes">list of scopes</strong>
+
+</body>
+</html>

=== modified file 'lib/lp/services/features/templates/feature-rules.pt'
--- lib/lp/services/features/templates/feature-rules.pt	2011-01-05 21:50:00 +0000
+++ lib/lp/services/features/templates/feature-rules.pt	2011-01-26 22:11:32 +0000
@@ -11,12 +11,15 @@
 
 <div metal:fill-slot="main">
   <div metal:use-macro="context/@@launchpad_form/form">
-    <div metal:fill-slot="extra_top"
-        tal:condition="view/diff">
+    <div metal:fill-slot="extra_top">
+      <div tal:condition="view/diff">
         <p>Your changes have been applied (and before and after values of the
           rules logged by the <tal:logger replace="view/logger_name"/> logger):
         </p>
         <tal:diff replace="structure view/diff"/>
+      </div>
+      For more information about the available feature flags and scopes see
+      the <a href="+feature-info">feature flag info</a>.
     </div>
   </div>
 </div>

=== modified file 'lib/lp/services/features/tests/test_webapp.py'
--- lib/lp/services/features/tests/test_webapp.py	2010-12-15 06:06:06 +0000
+++ lib/lp/services/features/tests/test_webapp.py	2011-01-26 22:11:32 +0000
@@ -73,6 +73,12 @@
         # There is no such key in the config, so this returns False.
         self.assertFalse(scopes.lookup('server.pink'))
 
+    def test_unknown_scope(self):
+        # Asking about an unknown scope is not an error.
+        request = LaunchpadTestRequest()
+        scopes = webapp.ScopesFromRequest(request)
+        scopes.lookup('not-a-real-scope')
+
 
 class TestDBScopes(TestCaseWithFactory):
 

=== modified file 'lib/lp/services/features/webapp.py'
--- lib/lp/services/features/webapp.py	2010-12-09 10:18:51 +0000
+++ lib/lp/services/features/webapp.py	2011-01-26 22:11:32 +0000
@@ -7,104 +7,10 @@
 
 __metaclass__ = type
 
-from zope.component import getUtility
-
-import canonical.config
-from canonical.launchpad.webapp.interfaces import ILaunchBag
 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
-
-
-class ScopesFromRequest(object):
-    """Identify feature scopes based on request state."""
-
-    def __init__(self, request):
-        self._request = request
-
-    def lookup(self, scope_name):
-        """Determine if scope_name applies to this request.
-
-        Currently supports the following scopes:
-         - default
-         - server.lpnet etc (thunks through to the config is_lpnet)
-         - pageid:
-           This scope works on a namespace model: for a page
-           with pageid SomeType:+view#subselector
-           The following page ids scopes will match:
-             - pageid:   (but use 'default' as it is simpler)
-             - pageid:SomeType
-             - pageid:SomeType:+view
-             - pageid:SomeType:+view#subselector
-         - team:
-           This scope looks up a team. For instance
-             - team:launchpad-beta-users
-        """
-        if scope_name == 'default':
-            return True
-        if scope_name.startswith('pageid:'):
-            return self._lookup_pageid(scope_name[len('pageid:'):])
-        if scope_name.startswith('team:'):
-            return self._lookup_team(scope_name[len('team:'):])
-        parts = scope_name.split('.')
-        if len(parts) == 2:
-            if parts[0] == 'server':
-                try:
-                    return canonical.config.config['launchpad'][
-                        'is_' + parts[1]]
-                except KeyError:
-                    return False
-
-    def _lookup_pageid(self, pageid_scope):
-        """Lookup a pageid as a scope.
-
-        pageid scopes are written as 'pageid:' + the pageid to match.
-        Page ids are treated as a namespace with : and # delimiters.
-
-        E.g. the scope 'pageid:Foo' will affect pages with pageids:
-        Foo
-        Foo:Bar
-        Foo#quux
-        """
-        scope_segments = self._pageid_to_namespace(pageid_scope)
-        request_segments = self._request_pageid_namespace
-        # In 2.6, this can be replaced with izip_longest
-        for pos, name in enumerate(scope_segments):
-            if pos == len(request_segments):
-                return False
-            if request_segments[pos] != name:
-                return False
-        return True
-
-    def _lookup_team(self, team_name):
-        """Lookup a team membership as a scope.
-
-        This will do a two queries, so we probably want to keep the number of
-        team based scopes in use to a small number. (Person.inTeam could be
-        fixed to reduce this to one query).
-
-        teamid scopes are written as 'team:' + the team name to match.
-
-        E.g. the scope 'team:launchpad-beta-users' will match members of
-        the team 'launchpad-beta-users'.
-        """
-        person = getUtility(ILaunchBag).user
-        if person is None:
-            return False
-        return person.inTeam(team_name)
-
-    def _pageid_to_namespace(self, pageid):
-        """Return a list of namespace elements for pageid."""
-        # Normalise delimiters.
-        pageid = pageid.replace('#', ':')
-        # Create a list to walk, empty namespaces are elided.
-        return [name for name in pageid.split(':') if name]
-
-    @cachedproperty
-    def _request_pageid_namespace(self):
-        return tuple(self._pageid_to_namespace(
-            self._request._orig_env.get('launchpad.pageid', '')))
+from lp.services.features.scopes import ScopesFromRequest
 
 
 def start_request(event):

=== modified file 'lib/lp/services/tests/test_utils.py'
--- lib/lp/services/tests/test_utils.py	2010-07-19 15:58:46 +0000
+++ lib/lp/services/tests/test_utils.py	2011-01-26 22:11:32 +0000
@@ -12,6 +12,7 @@
 from lp.services.utils import (
     CachingIterator,
     decorate_with,
+    docstring_dedent,
     iter_split,
     )
 from lp.testing import TestCase
@@ -140,5 +141,19 @@
         self.assertEqual(arbitrary_value, result)
 
 
+class TestDocstringDedent(TestCase):
+    """Tests for `docstring_dedent`."""
+
+    def test_single_line(self):
+        self.assertEqual(docstring_dedent('docstring'), 'docstring')
+
+    def test_multi_line(self):
+        docstring = """This is a multiline docstring.
+
+        This is the second line.
+        """
+        result = 'This is a multiline docstring.\n\nThis is the second line.'
+        self.assertEqual(docstring_dedent(docstring), result)
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/services/utils.py'
--- lib/lp/services/utils.py	2010-10-26 15:47:24 +0000
+++ lib/lp/services/utils.py	2011-01-26 22:11:32 +0000
@@ -11,12 +11,14 @@
 __all__ = [
     'CachingIterator',
     'decorate_with',
+    'docstring_dedent',
     'iter_split',
     'synchronize',
     'text_delta',
     'value_string',
     ]
 
+from textwrap import dedent
 import itertools
 
 from lazr.enum import BaseItem
@@ -153,3 +155,14 @@
                 return function(*a, **kw)
         return mergeFunctionMetadata(function, decorated)
     return decorator
+
+
+def docstring_dedent(s):
+    """Remove leading indentation from a doc string.
+
+    Since the first line doesn't have indentation, split it off, dedent, and
+    then reassemble.
+    """
+    # Make sure there is at least one newline so the split works.
+    first, rest = (s+'\n').split('\n', 1)
+    return (first + '\n' + dedent(rest)).strip()