Merge lp:~abentley/launchpad/view-flags into lp:launchpad

[Aaron Bentley]

Provide access to feature flags in JavaScript.

= Summary =
Make feature flags available to JavaScript

== Proposed fix ==
Tweak the existing beta_features mechanism so that it reports about "related_features", rather than beta features. The beta status is reflected by a boolean, and the current value is reported.

== Pre-implementation notes ==
Discussed with abel

== Implementation details ==
Changed the JSON output to a dict so that it is more legible, and easier to add value and beta to. Consolidated all functionality into a single for-loop to simplify the code and avoid re-retrieving data.

Tweaked beta banner generation so that the banner is shown if any notifications were generated, rather than if there are any entries (because now there can be non-beta entries.)

== Tests ==
bin/test -t test_publisher -t test_view_name

== Demo and Q/A ==
Ensure dynamic bug listings are enabled for you. Go to a +bugs page. You should see the beta banner. View source. You should see that the value of 'bugs.dynamic_bug_listings.enabled' is 'on'.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/beta-notification.js
  lib/lp/app/javascript/tests/test_beta_notification.js
  lib/canonical/launchpad/webapp/publisher.py
  lib/lp/bugs/javascript/buglisting.js
  lib/canonical/launchpad/webapp/tests/test_publisher.py
  lib/canonical/launchpad/webapp/tests/test_view_model.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/javascript/tests/test_buglisting.js

[Gavin Panella (allenap)]

[1]

+ var chunks = ['<span class="beta-feature"> ', info.title];
+ if (info.url.length > 0) {
+ chunks.push(' (<a href="', info.url, '" class="info-link">',
+ 'read more</a>)');
+ }
+ chunks.push('</span>');
+ notifications.push(Y.Node.create(chunks.join('')));

This is XSS territory. This must be done something like:

  var node = Y.Node.create(
      '<span class="beta-feature"><a class="info-link"></a></span>');
  node.set("text", info.title);
  node.one("a").set("href", info.url);

Obviously that's not the whole story, but string concatenation is
going to get you in a lot of trouble.

... Review in progress ...

[Gavin Panella (allenap)]

[2]

+ LP.cache.related_features = {
+ related_features: {
+ is_beta: false,
+ title: 'Non-beta feature',
+ url: 'http://example.org'
+ }};

Having a feature called related_features is a bit confusing.

Nice change, r=me.

review: Approve

[Gavin Panella (allenap)]

Actually, on second thoughts, Needs Fixing for [1].

review: Needs Fixing

[Gavin Panella (allenap)]

+ var notifications = Mustache.to_html([

Hehe, you rule :)

Also, I later realised that you had just moved the code, not created
it, so sorry if I was telling you things you already know.

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2011-11-08 20:32:38 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-11-18 18:04:13 +0000
@@ -80,7 +80,6 @@
 from lp.app.errors import NotFoundError
 from lp.services.encoding import is_ascii_only
 from lp.services.features import (
-    currentScope,
     defaultFlagValue,
     getFeatureFlag,
     )
@@ -278,13 +277,8 @@
         # 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)
+        related_features = cache.setdefault('related_features', {})
+        related_features.update(self.related_feature_info)
 
     def initialize(self):
         """Override this in subclasses.
@@ -390,29 +384,35 @@
         # 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 = ()
+    # Names of feature flags which affect a view.
+    related_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.
+    def related_feature_info(self):
+        """Related feature flags that are active for this context and scope.
+
+        This property describes all features marked as related_features in the
+        view.  is_beta means that the value is not the default value.
+
+        Return a dict of flags keyed by flag_name, with title and url as given
+        by the flag's description.  Value is the value in the current scope,
+        and is_beta is true if this 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
-            ]
+        beta_info = {}
+        for (flag_name, value_domain, documentation, default_behavior, title,
+             url) in flag_info:
+            if flag_name not in self.related_features:
+                continue
+            value = getFeatureFlag(flag_name)
+            beta_info[flag_name] = {
+                'is_beta': (defaultFlagValue(flag_name) != value),
+                'title': title,
+                'url': url,
+                'value': value,
+            }
         return beta_info
 
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publisher.py'
--- lib/canonical/launchpad/webapp/tests/test_publisher.py	2011-11-08 20:32:38 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publisher.py	2011-11-18 18:04:13 +0000
@@ -49,7 +49,7 @@
 
     def test_getCacheJSON_non_resource_context(self):
         view = LaunchpadView(object(), LaunchpadTestRequest())
-        self.assertEqual('{"beta_features": []}', view.getCacheJSON())
+        self.assertEqual('{"related_features": {}}', view.getCacheJSON())
 
     @staticmethod
     def getCanada():
@@ -77,7 +77,8 @@
         IJSONRequestCache(request).objects['my_bool'] = True
         with person_logged_in(self.factory.makePerson()):
             self.assertEqual(
-                '{"beta_features": [], "my_bool": true}', view.getCacheJSON())
+                '{"related_features": {}, "my_bool": true}',
+                view.getCacheJSON())
 
     def test_getCacheJSON_resource_object(self):
         request = LaunchpadTestRequest()
@@ -112,25 +113,30 @@
         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.
+    def test_related_feature_info__default(self):
+        # By default, LaunchpadView.related_feature_info is empty.
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        self.assertEqual(0, len(view.related_feature_info))
+
+    def test_related_feature_info__with_related_feature_nothing_enabled(self):
+        # If a view has a non-empty sequence of related feature flags but if
+        # no matching feature rules are defined, is_beta is False.
+        request = LaunchpadTestRequest()
+        view = LaunchpadView(object(), request)
+        view.related_features = ['test_feature']
+        self.assertEqual({
+            'test_feature': {
+                'is_beta': False,
+                'title': 'title',
+                'url': 'http://wiki.lp.dev/LEP/sample',
+                'value': None,
+            }
+        }, view.related_feature_info)
+
+    def test_related_feature_info__default_scope_only(self):
+        # If a view has a non-empty sequence of related feature flags but if
+        # only a default scope is defined, it is not considered beta.
         self.useFixture(FeatureFixture(
             {},
             (
@@ -143,13 +149,18 @@
                 )))
         request = LaunchpadTestRequest()
         view = LaunchpadView(object(), request)
-        view.beta_features = ['test_feature']
-        self.assertEqual([], view.active_beta_features)
+        view.related_features = ['test_feature']
+        self.assertEqual({'test_feature': {
+            'is_beta': False,
+            'title': 'title',
+            'url': 'http://wiki.lp.dev/LEP/sample',
+            'value': 'on',
+        }}, view.related_feature_info)
 
-    def test_active_beta_features__enabled_feature(self):
-        # If a view has a non-empty sequence of beta feature flags and if
+    def test_active_related_features__enabled_feature(self):
+        # If a view has a non-empty sequence of related feature flags and if
         # only a non-default scope is defined and active, the property
-        # active_beta_features contains this feature flag.
+        # active_related_features contains this feature flag.
         self.useFixture(FeatureFixture(
             {},
             (
@@ -162,11 +173,15 @@
                 )))
         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)
+        view.related_features = ['test_feature']
+        self.assertEqual({
+            'test_feature': {
+                'is_beta': True,
+                'title': 'title',
+                'url': 'http://wiki.lp.dev/LEP/sample',
+                'value': 'on'}
+            },
+            view.related_feature_info)
 
     def makeFeatureFlagDictionaries(self, default_value, scope_value):
         # Return two dictionaries describing a feature for each test feature.
@@ -185,42 +200,49 @@
             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):
+    def test_related_features__enabled_feature_with_default(self):
         # If a view
-        #   * has a non-empty sequence of beta feature flags,
+        #   * has a non-empty sequence of related 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.
+        # then the property related_feature_info 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(
+        view.related_features = ['test_feature']
+        self.assertEqual({
+            'test_feature': {
+                'is_beta': True,
+                'title': 'title',
+                'url': 'http://wiki.lp.dev/LEP/sample',
+                'value': 'on',
+            }},
+            view.related_feature_info)
+
+    def test_related_feature_info__enabled_feature_with_default_same_value(
         self):
         # If a view
-        #   * has a non-empty sequence of beta feature flags,
+        #   * has a non-empty sequence of related 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.
+        # then is_beta is false.
         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)
+        view.related_features = ['test_feature']
+        self.assertEqual({'test_feature': {
+            'is_beta': False,
+            'title': 'title',
+            'url': 'http://wiki.lp.dev/LEP/sample',
+            'value': 'on',
+        }}, view.related_feature_info)
 
-    def test_json_cache_has_beta_features(self):
-        # The property beta_features is copied into the JSON cache.
+    def test_json_cache_has_related_features(self):
+        # The property related_features is copied into the JSON cache.
         class TestView(LaunchpadView):
-            beta_features = ['test_feature']
+            related_features = ['test_feature']
 
         self.useFixture(FeatureFixture(
             {}, self.makeFeatureFlagDictionaries(u'', u'on')))
@@ -228,20 +250,23 @@
         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"]]}',
+                '{"related_features": {"test_feature": {'
+                '"url": "http://wiki.lp.dev/LEP/sample", '
+                '"is_beta": true, '
+                '"value": "on", '
+                '"title": "title"'
+                '}}}',
                 view.getCacheJSON())
 
-    def test_json_cache_collects_beta_features_from_all_views(self):
+    def test_json_cache_collects_related_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
+        # for example, from macros. Related features from these sub-views
         # are included in the JSON cache.
         class TestView(LaunchpadView):
-            beta_features = ['test_feature']
+            related_features = ['test_feature']
 
         class TestView2(LaunchpadView):
-            beta_features = ['test_feature_2']
+            related_features = ['test_feature_2']
 
         self.useFixture(FeatureFixture(
             {}, self.makeFeatureFlagDictionaries(u'', u'on')))
@@ -250,12 +275,11 @@
         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"]]}',
+                '{"related_features": '
+                '{"test_feature_2": {"url": "http://wiki.lp.dev/LEP/sample2",'
+                ' "is_beta": true, "value": "on", "title": "title"}, '
+                '"test_feature": {"url": "http://wiki.lp.dev/LEP/sample", '
+                '"is_beta": true, "value": "on", "title": "title"}}}',
                 view.getCacheJSON())
 
     def test_view_creation_with_fake_or_none_request(self):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_view_model.py'
--- lib/canonical/launchpad/webapp/tests/test_view_model.py	2011-11-04 11:10:23 +0000
+++ lib/canonical/launchpad/webapp/tests/test_view_model.py	2011-11-18 18:04:13 +0000
@@ -123,7 +123,7 @@
         self.configZCML()
         browser = self.getUserBrowser(self.url)
         cache = loads(browser.contents)
-        self.assertEqual(['beta_features', 'context'], cache.keys())
+        self.assertEqual(['related_features', 'context'], cache.keys())
 
     def test_JsonModel_custom_cache(self):
         # Adding an item to the cache in the initialize method results in it
@@ -141,7 +141,7 @@
         browser = self.getUserBrowser(self.url)
         cache = loads(browser.contents)
         self.assertThat(
-            cache, KeysEqual('beta_features', 'context', 'target_info'))
+            cache, KeysEqual('related_features', 'context', 'target_info'))
 
     def test_JsonModel_custom_cache_wrong_method(self):
         # Adding an item to the cache in some other method is not recognized,
@@ -166,4 +166,4 @@
         browser = self.getUserBrowser(self.url)
         cache = loads(browser.contents)
         self.assertThat(
-            cache, KeysEqual('beta_features', 'context', 'target_info'))
+            cache, KeysEqual('related_features', 'context', 'target_info'))

=== modified file 'lib/lp/app/javascript/beta-notification.js'
--- lib/lp/app/javascript/beta-notification.js	2011-11-10 15:16:02 +0000
+++ lib/lp/app/javascript/beta-notification.js	2011-11-18 18:04:13 +0000
@@ -18,11 +18,18 @@
  * This should be called after the page has loaded e.g. on 'domready'.
  */
 function display_beta_notification() {
-    if (LP.cache.beta_features.length === 0) {
+    var notifications = Mustache.to_html([
+        '{{#features}}{{#is_beta}}',
+        '<span class="beta-feature"> {{title}}',
+        '{{#url}}',
+        ' (<a href="{{url}}" class="info-link">read more</a>)',
+        '{{/url}}',
+        '</span>',
+        '{{/is_beta}}{{/features}}'].join(''),
+        {features: Y.Object.values(LP.cache.related_features)});
+    if (notifications.length === 0) {
         return;
     }
-
-    var beta_features = LP.cache.beta_features;
     var body = Y.one('body');
     body.addClass('global-notification-visible');
     var main = Y.one('#maincontent');
@@ -37,19 +44,7 @@
         '<span class="notification-close sprite" /></a>');
     beta_notification_node.appendChild(close_box);
     beta_notification_node.append('Some parts of this page are in beta: ');
-    var index;
-    for (index = 0; index < beta_features.length; index++) {
-        var feature_name = beta_features[index][4];
-        var info_link = beta_features[index][5];
-        if (info_link.length > 0) {
-            info_link =
-                ' (<a href="' + info_link + '" class="info-link">' +
-                'read more</a>)';
-        }
-        beta_notification_node.appendChild(Y.Node.create(
-            '<span class="beta-feature"> ' + feature_name + info_link +
-            '</span>'));
-    }
+    beta_notification_node.append(notifications);
     close_box.on('click', function(e) {
         e.halt();
         var fade_out = new Y.Anim({

=== modified file 'lib/lp/app/javascript/tests/test_beta_notification.html'
--- lib/lp/app/javascript/tests/test_beta_notification.html	2011-11-10 10:32:25 +0000
+++ lib/lp/app/javascript/tests/test_beta_notification.html	2011-11-18 18:04:13 +0000
@@ -9,6 +9,8 @@
   <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
   <script type="text/javascript"
           src="../../../app/javascript/testing/testrunner.js"></script>
+  <script type="text/javascript"
+          src="../../../contrib/javascript/mustache.js"></script>
 
     <!-- The module under test. -->
     <script type="text/javascript" src="../beta-notification.js"></script>

=== modified file 'lib/lp/app/javascript/tests/test_beta_notification.js'
--- lib/lp/app/javascript/tests/test_beta_notification.js	2011-11-10 10:32:25 +0000
+++ lib/lp/app/javascript/tests/test_beta_notification.js	2011-11-18 18:04:13 +0000
@@ -40,8 +40,12 @@
         },
 
         test_beta_banner_one_beta_feature: function() {
-            LP.cache.beta_features = [
-                ['', '', '', '', 'A beta feature', 'http://lp.dev/LEP/one']];
+            LP.cache.related_features = {
+                '': {
+                    is_beta: true,
+                    title: 'A beta feature',
+                    url: 'http://lp.dev/LEP/one'
+            }};
             Y.lp.app.beta_features.display_beta_notification();
 
             var body = Y.one('body');
@@ -63,9 +67,17 @@
         },
 
         test_beta_banner_two_beta_features: function() {
-            LP.cache.beta_features = [
-                ['', '', '', '', 'Beta feature 1', 'http://lp.dev/LEP/one'],
-                ['', '', '', '', 'Beta feature 2', '']];
+            LP.cache.related_features = {
+                '1': {
+                    is_beta: true,
+                    title: 'Beta feature 1',
+                    url: 'http://lp.dev/LEP/one'
+                },
+                '2': {
+                    is_beta: true,
+                    title: 'Beta feature 2',
+                    url: ''
+                }};
             Y.lp.app.beta_features.display_beta_notification();
 
             var body = Y.one('body');
@@ -81,7 +93,7 @@
             info_link = feature_info.get('children').item(0);
             Y.Assert.areEqual('http://lp.dev/LEP/one', info_link.get('href'));
 
-            // If an entry in LP.cache.beta_features does not provide a
+            // If an entry in LP.cache.related_features does not provide a
             // "read more" link, the corrsponding node is not added.
             feature_info = sub_nodes.filter('.beta-feature').item(1);
             Y.Assert.areEqual(
@@ -90,7 +102,12 @@
         },
 
         test_beta_banner_no_beta_features_defined: function() {
-            LP.cache.beta_features = [];
+            LP.cache.related_features = {
+                foo_feature: {
+                    is_beta: false,
+                    title: 'Non-beta feature',
+                    url: 'http://example.org'
+                }};
             Y.lp.app.beta_features.display_beta_notification();
 
             var body = Y.one('body');
@@ -100,8 +117,12 @@
         },
 
         test_hide_beta_banner: function() {
-            LP.cache.beta_features = [
-                ['', '', '', '', 'A beta feature', 'http://lp.dev/LEP/one']];
+            LP.cache.related_features = {
+                '': {
+                    is_beta: true,
+                    title: 'A beta feature',
+                    url: 'http://lp.dev/LEP/one'
+            }};
             Y.lp.app.beta_features.display_beta_notification();
             var body = Y.one('body');
             var banner = Y.one('.beta-banner');

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-18 04:43:38 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-18 18:04:13 +0000
@@ -2455,7 +2455,7 @@
 
     implements(IBugTaskSearchListingMenu)
 
-    beta_features = ['bugs.dynamic_bug_listings.enabled']
+    related_features = ['bugs.dynamic_bug_listings.enabled']
 
     # Only include <link> tags for bug feeds when using this view.
     feed_types = (