← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~adeuring/launchpad/banner-for-beta-features into lp:launchpad

 

Abel Deuring has proposed merging lp:~adeuring/launchpad/banner-for-beta-features into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/banner-for-beta-features/+merge/81165

his branch is a step to show a banner about features in beta status
on Launchpad pages (similar to the "privacy banner" for private bugs).
The banner will display a text like "BETA -- some parts of this page
are in beta: Improved bug comments (link:read more). New bug columns
(link:read more)."

Like the privacy banner, the beta banner will be created via Javascript,
meaning that we need information about active beta features somewhere
in the pages. The obvious place to put the data is the JSON cache.

What we need to know:

1. if a feature flag affects a given view
2. if a feature flag is a beta feature (in contrast to the flag
   hard_timeout, for example, which allos us increase the timeout
   for "slow pages".)
3. if the feature is enabled, but not enabled by default, i.e.,
   if it is visible for beta testers only.

There a lots of ways to determine if a feature is in beta status, for
example, we could check if a feature's scope is a team/person but not
a page ID; we could explicitly mark teams as "beta tester teams" etc etc.

I discussed different approaches with abentley, and it seems that
the following is reasonably precise without requiring for example
a DB patch, like Team.is_beta_tester_team:

- Add a property LaunchpadView.beta_features, an empty list.
  beta_features should be redefined in view classes which have features
  in beta status.
- When a page is rendered, determine if the value of the feature flag
  is different from the default value. If this is the case, display
  a message for this  flag in the banner.

LaunchpadView.__init__() now adds any active beta features to the JSON
cache. Note that for most if not all Launchpad HTML pages, more than one
view is created when the page is rendered, for example, when a TAL macro
is inserted into a template. Hence LPView.__init__() collects data about
beta features from different views in the JSON cache.

The sample text for the "beta banner" above needs a kind of title for
a faeture flag and a URL for the link "read more". We already have the
list lp.services.features.flags.flag_info, but it's current data is
not very useful for the banner page: The description (texts like "Hide
the link to the Canonical Careers site") would look a bit odd, so I added
two elements to each tuple, for the title and the URL. Most values
are empty strings; only the feature bugs.dynamic_bug_listings.enabled
has for now a "real" title and URL.

As written above, LaunchpadView.active_beta_features determines if
a beta feature is active in the given context. This is done via
two new functions currentScope(flag) and defaultFlagValue(). (I used
camelCase names, because other functions already defined in
lp.services.features.flags use the same pattern.)

tests:

./bin/test features -vv
./bin/test canonical -vvt test_publisher

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/banner-for-beta-features/+merge/81165
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/banner-for-beta-features into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2011-09-19 06:57:55 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-11-03 15:41:30 +0000
@@ -79,6 +79,11 @@
 from canonical.launchpad.webapp.vhosts import allvhosts
 from lp.app.errors import NotFoundError
 from lp.services.encoding import is_ascii_only
+from lp.services.features import (
+    currentScope,
+    defaultFlagValue,
+    getFeatureFlag,
+    )
 
 # Monkeypatch NotFound to always avoid generating OOPS
 # from NotFound in web service calls.
@@ -257,6 +262,21 @@
         self.request = request
         self._error_message = None
         self._info_message = None
+        # We don't have an adapter FakeRequest -> IJSONRequestCache
+        # and some tests create views without providing any request
+        # object at all.
+        if request is not None and not isinstance(request, FakeRequest):
+            cache = IJSONRequestCache(self.request).objects
+            # Several view objects may be created for one page request:
+            # One view for the main context and template, and other views
+            # for macros included in the main template.
+            if 'beta_features' not in cache:
+                cache['beta_features'] = self.active_beta_features
+            else:
+                beta_info = cache['beta_features']
+                for feature in self.active_beta_features:
+                    if feature not in beta_info:
+                        beta_info.append(feature)
 
     def initialize(self):
         """Override this in subclasses.
@@ -363,6 +383,31 @@
         # Those that can be must override this method.
         raise NotFound(self, name, request=request)
 
+    # Flags for new features in beta status which affect a view.
+    beta_features = ()
+
+    @property
+    def active_beta_features(self):
+        """Beta feature flags that are active for this context and scope.
+
+        This property consists of all feature flags from beta_features
+        whose current value is not the default value.
+        """
+        # Avoid circular imports.
+        from lp.services.features.flags import flag_info
+
+        def flag_in_beta_status(flag):
+            return (
+                currentScope(flag) not in ('default', None) and
+                defaultFlagValue(flag) != getFeatureFlag(flag))
+
+        active_beta_flags = set(
+            flag for flag in self.beta_features if flag_in_beta_status(flag))
+        beta_info = [
+            info for info in flag_info if info[0] in active_beta_flags
+            ]
+        return beta_info
+
 
 class LaunchpadXMLRPCView(UserAttributeCache):
     """Base class for writing XMLRPC view code."""

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publisher.py'
--- lib/canonical/launchpad/webapp/tests/test_publisher.py	2011-07-18 19:27:35 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publisher.py	2011-11-03 15:41:30 +0000
@@ -12,8 +12,13 @@
 from zope.component import getUtility
 
 from canonical.testing.layers import DatabaseFunctionalLayer
-from canonical.launchpad.webapp.publisher import LaunchpadView
+from canonical.launchpad.webapp.publisher import (
+    FakeRequest,
+    LaunchpadView,
+    )
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from lp.services.features.flags import flag_info
+from lp.services.features.testing import FeatureFixture
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.testing import (
     logout,
@@ -28,6 +33,20 @@
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestLaunchpadView, self).setUp()
+        flag_info.append(
+            ('test_feature', 'boolean', 'documentation', 'default_value_1',
+             'title', 'http://wiki.lp.dev/LEP/sample'))
+        flag_info.append(
+            ('test_feature_2', 'boolean', 'documentation', 'default_value_2',
+             'title', 'http://wiki.lp.dev/LEP/sample2'))
+
+    def tearDown(self):
+        flag_info.pop()
+        flag_info.pop()
+        super(TestLaunchpadView, self).tearDown()
+
     def test_getCacheJSON_non_resource_context(self):
         view = LaunchpadView(object(), LaunchpadTestRequest())
         self.assertEqual('{}', view.getCacheJSON())
@@ -57,7 +76,8 @@
         view = LaunchpadView(object(), request)
         IJSONRequestCache(request).objects['my_bool'] = True
         with person_logged_in(self.factory.makePerson()):
-            self.assertEqual('{"my_bool": true}', view.getCacheJSON())
+            self.assertEqual(
+                '{"beta_features": [], "my_bool": true}', view.getCacheJSON())
 
     def test_getCacheJSON_resource_object(self):
         request = LaunchpadTestRequest()
@@ -92,6 +112,158 @@
         self.assertIs(None, view.user)
         self.assertNotIn('user@domain', view.getCacheJSON())
 
+    def test_active_beta_features__default(self):
+        # By default, LaunchpadView.active_beta_features is empty.
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        self.assertEqual(0, len(view.active_beta_features))
+
+    def test_active_beta_features__with_beta_feature_nothing_enabled(self):
+        # If a view has a non-empty sequence of beta feature flags but if
+        # no matching feature rules are defined, its property
+        # active_beta_features is empty.
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.beta_features = ['test_feature']
+        self.assertEqual(0, len(view.active_beta_features))
+
+    def test_active_beta_features__default_scope_only(self):
+        # If a view has a non-empty sequence of beta feature flags but if
+        # only a default scope is defined, its property
+        # active_beta_features is empty.
+        self.useFixture(FeatureFixture(
+            {},
+            (
+                {
+                    u'flag': u'test_feature',
+                    u'scope': u'default',
+                    u'priority': 0,
+                    u'value': u'on',
+                    },
+                )))
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.beta_features = ['test_feature']
+        self.assertEqual([], view.active_beta_features)
+
+    def test_active_beta_features__enabled_feature(self):
+        # If a view has a non-empty sequence of beta feature flags and if
+        # only a non-default scope is defined and active, the property
+        # active_beta_features contains this feature flag.
+        self.useFixture(FeatureFixture(
+            {},
+            (
+                {
+                    u'flag': u'test_feature',
+                    u'scope': u'pageid:foo',
+                    u'priority': 0,
+                    u'value': u'on',
+                    },
+                )))
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.beta_features = ['test_feature']
+        self.assertEqual(
+            [('test_feature', 'boolean', 'documentation', 'default_value_1',
+              'title', 'http://wiki.lp.dev/LEP/sample')],
+            view.active_beta_features)
+
+    def makeFeatureFlagDictionaries(self, default_value, scope_value):
+        # Return two dictionaries describing a feature for each test feature.
+        # One dictionary specifies the default value, the other specifies
+        # a more restricted scope.
+        def makeFeatureDict(flag, value, scope, priority):
+            return {
+                u'flag': flag,
+                u'scope': scope,
+                u'priority': priority,
+                u'value': value,
+                }
+        return (
+            makeFeatureDict('test_feature', default_value, u'default', 0),
+            makeFeatureDict('test_feature', scope_value, u'pageid:foo', 10),
+            makeFeatureDict('test_feature_2', default_value, u'default', 0),
+            makeFeatureDict('test_feature_2', scope_value, u'pageid:bar', 10))
+
+    def test_active_beta_features__enabled_feature_with_default(self):
+        # If a view
+        #   * has a non-empty sequence of beta feature flags,
+        #   * the default scope and a non-default scope are defined
+        #     but have different values,
+        # then the property active_beta_features contains this feature flag.
+        self.useFixture(FeatureFixture(
+            {}, self.makeFeatureFlagDictionaries(u'', u'on')))
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.beta_features = ['test_feature']
+        self.assertEqual(
+
+            [('test_feature', 'boolean', 'documentation', 'default_value_1',
+              'title', 'http://wiki.lp.dev/LEP/sample')],
+            view.active_beta_features)
+
+    def test_active_beta_features__enabled_feature_with_default_same_value(
+        self):
+        # If a view
+        #   * has a non-empty sequence of beta feature flags,
+        #   * the default scope and a non-default scope are defined
+        #     and have the same values,
+        # then the property active_beta_features does not contain this
+        # feature flag.
+        self.useFixture(FeatureFixture(
+            {}, self.makeFeatureFlagDictionaries(u'on', u'on')))
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.beta_features = ['test_feature']
+        self.assertEqual([], view.active_beta_features)
+
+    def test_json_cache_has_beta_features(self):
+        # The property beta_features is copied into the JSON cache.
+        class TestView(LaunchpadView):
+            beta_features = ['test_feature']
+
+        self.useFixture(FeatureFixture(
+            {}, self.makeFeatureFlagDictionaries(u'', u'on')))
+        request = LaunchpadTestRequest()
+        view = TestView(object(), request)
+        with person_logged_in(self.factory.makePerson()):
+            self.assertEqual(
+                '{"beta_features": [["test_feature", "boolean", '
+                '"documentation", "default_value_1", "title", '
+                '"http://wiki.lp.dev/LEP/sample"]]}',
+                view.getCacheJSON())
+
+    def test_json_cache_collects_beta_features_from_all_views(self):
+        # A typical page includes data from more than one view,
+        # for example, from macros. Beta features from these sub-views
+        # are included in the JSON cache.
+        class TestView(LaunchpadView):
+            beta_features = ['test_feature']
+
+        class TestView2(LaunchpadView):
+            beta_features = ['test_feature_2']
+
+        self.useFixture(FeatureFixture(
+            {}, self.makeFeatureFlagDictionaries(u'', u'on')))
+        request = LaunchpadTestRequest()
+        view = TestView(object(), request)
+        TestView2(object(), request)
+        with person_logged_in(self.factory.makePerson()):
+            self.assertEqual(
+                '{"beta_features": [["test_feature", "boolean", '
+                '"documentation", "default_value_1", "title", '
+                '"http://wiki.lp.dev/LEP/sample";], '
+                '["test_feature_2", "boolean", "documentation", '
+                '"default_value_2", "title", '
+                '"http://wiki.lp.dev/LEP/sample2"]]}',
+                view.getCacheJSON())
+
+    def test_view_creation_with_fake_or_none_request(self):
+        # LaunchpadView.__init__() does not crash with a FakeRequest.
+        LaunchpadView(object(), FakeRequest())
+        # Or when no request at all is passed.
+        LaunchpadView(object(), None)
+
 
 def test_suite():
     suite = TestSuite()

=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py	2011-09-21 21:12:02 +0000
+++ lib/lp/services/features/__init__.py	2011-11-03 15:41:30 +0000
@@ -183,6 +183,8 @@
 
 
 __all__ = [
+    'currentScope',
+    'defaultFlagValue',
     'get_relevant_feature_controller',
     'getFeatureFlag',
     'install_feature_controller',
@@ -222,6 +224,23 @@
     return features.getFlag(flag)
 
 
+def currentScope(flag):
+    """Get the current scope of the flag for this thread's scopes."""
+    # Workaround for bug 631884 - features have two homes, threads and
+    # requests.
+    features = get_relevant_feature_controller()
+    if features is None:
+        return None
+    return features.currentScope(flag)
+
+
+def defaultFlagValue(flag):
+    features = get_relevant_feature_controller()
+    if features is None:
+        return None
+    return features.defaultFlagValue(flag)
+
+
 def make_script_feature_controller(script_name):
     """Create a `FeatureController` for the named script.
 

=== modified file 'lib/lp/services/features/browser/info.py'
--- lib/lp/services/features/browser/info.py	2011-02-15 05:31:03 +0000
+++ lib/lp/services/features/browser/info.py	2011-11-03 15:41:30 +0000
@@ -25,7 +25,8 @@
 
 
 # Named tuples to use when passing flag and scope data to the template.
-Flag = namedtuple('Flag', ('name', 'domain', 'description', 'default'))
+Flag = namedtuple(
+    'Flag', ('name', 'domain', 'description', 'default', 'title', 'link'))
 ValueDomain = namedtuple('ValueDomain', ('name', 'description'))
 Scope = namedtuple('Scope', ('regex', 'description'))
 

=== modified file 'lib/lp/services/features/browser/tests/test_feature_info.py'
--- lib/lp/services/features/browser/tests/test_feature_info.py	2011-08-02 23:42:08 +0000
+++ lib/lp/services/features/browser/tests/test_feature_info.py	2011-11-03 15:41:30 +0000
@@ -44,7 +44,6 @@
 
     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])
 
@@ -64,7 +63,7 @@
         browser = self.getUserBrowserAsAdmin()
         browser.open(self.getFeatureInfoUrl())
         for record in flag_info:
-            for item in record:
+            for item in record[:4]:
                 self.assertThat(browser.contents, Contains(item))
 
     def test_value_domain_documentation_displayed(self):

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-11-01 20:06:33 +0000
+++ lib/lp/services/features/flags.py	2011-11-03 15:41:30 +0000
@@ -33,7 +33,8 @@
 # Data for generating web-visible feature flag documentation.
 #
 # Entries for each flag are:
-# flag name, value domain, prose documentation, default behaviour.
+# flag name, value domain, prose documentation, default behaviour, title,
+# URL to a page with more information about the feature.
 #
 # Value domain as in value_domain_info above.
 #
@@ -43,128 +44,186 @@
     ('baselayout.careers_link.disabled',
      'boolean',
      'Hide the link to the Canonical Careers site.',
+     '',
+     '',
      ''),
     ('bugs.bugtracker_components.enabled',
      'boolean',
      ('Enables the display of bugtracker components.'),
+     '',
+     '',
      ''),
     ('bugs.dynamic_bug_listings.enabled',
      'boolean',
      ('Enables the dynamic configuration of bug listings.'),
-     ''),
+     '',
+     'Dynamic bug listings',
+     'https://dev.launchpad.net/LEP/CustomBugListings'),
     ('code.ajax_revision_diffs.enabled',
      'boolean',
      ("Offer expandable inline diffs for branch revisions."),
+     '',
+     '',
      ''),
     ('code.branchmergequeue',
      'boolean',
      'Enables merge queue pages and lists them on branch pages.',
+     '',
+     '',
      ''),
     ('code.incremental_diffs.enabled',
      'boolean',
      'Shows incremental diffs on merge proposals.',
+     '',
+     '',
      ''),
     ('code.simplified_branches_menu.enabled',
      'boolean',
      ('Display a simplified version of the branch menu (omit the counts).'),
+     '',
+     '',
      ''),
     ('hard_timeout',
      'float',
      'Sets the hard request timeout in milliseconds.',
+     '',
+     '',
      ''),
     ('mail.dkim_authentication.disabled',
      'boolean',
      'Disable DKIM authentication checks on incoming mail.',
+     '',
+     '',
      ''),
     ('malone.disable_targetnamesearch',
      'boolean',
      'If true, disables consultation of target names during bug text search.',
+     '',
+     '',
      ''),
     ('memcache',
      'boolean',
      'Enables use of memcached where it is supported.',
-     'enabled'),
+     'enabled',
+     '',
+     ''),
     ('profiling.enabled',
      'boolean',
      'Overrides config.profiling.profiling_allowed to permit profiling.',
+     '',
+     '',
      ''),
     ('soyuz.derived_series.max_synchronous_syncs',
      'int',
      "How many package syncs may be done directly in a web request.",
-     '100'),
+     '100',
+     '',
+     ''),
     ('soyuz.derived_series_ui.enabled',
      'boolean',
      'Enables derivative distributions pages.',
+     '',
+     '',
      ''),
     ('soyuz.derived_series_sync.enabled',
      'boolean',
      'Enables syncing of packages on derivative distributions pages.',
+     '',
+     '',
      ''),
     ('soyuz.derived_series_upgrade.enabled',
      'boolean',
      'Enables mass-upgrade of packages on derivative distributions pages.',
+     '',
+     '',
      ''),
     ('soyuz.derived_series_jobs.enabled',
      'boolean',
      "Compute package differences for derived distributions.",
+     '',
+     '',
      ''),
     ('translations.sharing_information.enabled',
      'boolean',
      'Enables display of sharing information on translation pages.',
+     '',
+     '',
      ''),
     ('visible_render_time',
      'boolean',
      'Shows the server-side page render time in the login widget.',
+     '',
+     '',
      ''),
     ('disclosure.dsp_picker.enabled',
      'boolean',
      'Enables the use of the new DistributionSourcePackage vocabulary for '
      'the source and binary package name pickers.',
+     '',
+     '',
      ''),
     ('disclosure.private_bug_visibility_rules.enabled',
      'boolean',
      ('Enables the application of additional privacy filter terms in bug '
       'queries to allow defined project roles to see private bugs.'),
+     '',
+     '',
      ''),
     ('disclosure.enhanced_private_bug_subscriptions.enabled',
      'boolean',
      ('Enables the auto subscribing and unsubscribing of users as a bug '
       'transitions between public, private and security related states.'),
+     '',
+     '',
      ''),
     ('disclosure.delete_bugtask.enabled',
      'boolean',
      'Enables bugtasks to be deleted by authorised users.',
+     '',
+     '',
      ''),
     ('disclosure.allow_multipillar_private_bugs.enabled',
      'boolean',
      'Allows private bugs to have more than one bug task.',
+     '',
+     '',
      ''),
     ('bugs.autoconfirm.enabled_distribution_names',
      'space delimited',
      ('Enables auto-confirming bugtasks for distributions (and their '
       'series and packages).  Use the default domain.  Specify a single '
       'asterisk ("*") to enable for all distributions.'),
-     'None are enabled'),
+     'None are enabled',
+     '',
+     ''),
     ('bugs.autoconfirm.enabled_product_names',
      'space delimited',
      ('Enables auto-confirming bugtasks for products (and their '
       'series).  Use the default domain.  Specify a single '
       'asterisk ("*") to enable for all products.'),
-     'None are enabled'),
+     'None are enabled',
+     '',
+     ''),
     ('longpoll.merge_proposals.enabled',
      'boolean',
      ('Enables the longpoll mechanism for merge proposals so that diffs, '
       'for example, are updated in-page when they are ready.'),
+     '',
+     '',
      ''),
     ('ajax.batch_navigator.enabled',
      'boolean',
      ('If true, batch navigators which have been wired to do so use ajax '
      'calls to load the next batch of data.'),
+     '',
+     '',
      ''),
     ('disclosure.log_private_team_leaks.enabled',
      'boolean',
      ('Enables soft OOPSes for code that is mixing visibility rules, such '
       'as disclosing private teams, so the data can be analyzed.'),
+     '',
+     '',
      ''),
     ])
 
@@ -250,6 +309,7 @@
         if rule_source is None:
             rule_source = StormFeatureRuleSource()
         self.rule_source = rule_source
+        self._current_scopes = Memoize(self._findCurrentScope)
 
     def getFlag(self, flag):
         """Get the value of a specific flag.
@@ -265,11 +325,26 @@
         return self._known_flags.lookup(flag)
 
     def _checkFlag(self, flag):
+        return self._currentValueAndScope(flag)[0]
+
+    def _currentValueAndScope(self, flag):
         self._needRules()
         if flag in self._rules:
             for scope, priority, value in self._rules[flag]:
                 if self._known_scopes.lookup(scope):
-                    return value
+                    return (value, scope)
+        return (None, None)
+
+    def currentScope(self, flag):
+        """The name of the scope of the matching rule with the highest
+        priority.
+        """
+        return self._current_scopes.lookup(flag)
+
+    def _findCurrentScope(self, flag):
+        """Lookup method for self._current_scopes. See also `currentScope()`.
+        """
+        return self._currentValueAndScope(flag)[1]
 
     def isInScope(self, scope):
         return self._known_scopes.lookup(scope)
@@ -309,6 +384,15 @@
         """Return {scope: active} for scopes that have been used so far."""
         return dict(self._known_scopes._known)
 
+    def defaultFlagValue(self, flag):
+        """Return the flag's value in the default scope."""
+        self._needRules()
+        if flag in self._rules:
+            for scope, priority, value in self._rules[flag]:
+                if scope == 'default':
+                    return value
+        return None
+
 
 class NullFeatureController(FeatureController):
     """For use in testing: everything is turned off"""

=== modified file 'lib/lp/services/features/testing.py'
--- lib/lp/services/features/testing.py	2011-03-29 00:11:57 +0000
+++ lib/lp/services/features/testing.py	2011-11-03 15:41:30 +0000
@@ -4,7 +4,7 @@
 """Helpers for writing tests that use feature flags."""
 
 __metaclass__ = type
-__all__ = ['active_features']
+__all__ = ['FeatureFixture']
 
 
 from fixtures import Fixture
@@ -37,13 +37,14 @@
     The values are restored when the block exits.
     """
 
-    def __init__(self, features_dict):
+    def __init__(self, features_dict, full_feature_rules=None):
         """Constructor.
 
         :param features_dict: A dictionary-like object with keys and values
             that are flag names and those flags' settings.
         """
         self.desired_features = features_dict
+        self.full_feature_rules = full_feature_rules
 
     def setUp(self):
         """Set the feature flags that this fixture is responsible for."""
@@ -77,4 +78,9 @@
             for flag_name, value in self.desired_features.iteritems()
                 if value is not None]
 
+        if self.full_feature_rules is not None:
+            new_rules.extend(
+                Rule(**rule_spec)
+                for rule_spec in self.full_feature_rules)
+
         return new_rules

=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py	2011-08-02 23:42:08 +0000
+++ lib/lp/services/features/tests/test_flags.py	2011-11-03 15:41:30 +0000
@@ -72,6 +72,40 @@
         self.assertEqual(dict(beta_user=False, default=True),
             control.usedScopes())
 
+    def test_currentScope(self):
+        # currentScope() returns the scope of the matching rule with
+        # the highest priority rule
+        self.populateStore()
+        # If only one scope matches, its name is returned.
+        control, call_log = self.makeControllerInScopes(['default'])
+        self.assertEqual('default', control.currentScope('ui.icing'))
+        # If two scopes match, the one with the higer priority is returned.
+        control, call_log = self.makeControllerInScopes(
+            ['default', 'beta_user'])
+        self.assertEqual('beta_user', control.currentScope('ui.icing'))
+
+    def test_currentScope__undefined_feature(self):
+        # currentScope() returns None for a non-existent flaeture flag.
+        self.populateStore()
+        control, call_log = self.makeControllerInScopes(['default'])
+        self.assertIs(None, control.currentScope('undefined_feature'))
+
+    def test_defaultFlagValue(self):
+        # defaultFlagValue() returns the default value of a flag even if
+        # another scopewith a higher priority matches.
+        self.populateStore()
+        control, call_log = self.makeControllerInScopes(
+            ['default', 'beta_user'])
+        self.assertEqual('3.0', control.defaultFlagValue('ui.icing'))
+
+    def test_defaultFlagValue__undefined_feature(self):
+        # defaultFlagValue() returns None if no default scope is defined
+        # for a feature.
+        self.populateStore()
+        control, call_log = self.makeControllerInScopes(
+            ['default', 'beta_user'])
+        self.assertIs(None, control.defaultFlagValue('undefined_feature'))
+
     def test_getAllFlags(self):
         # can fetch all the active flags, and it gives back only the
         # highest-priority settings.  this may be expensive and shouldn't


Follow ups