launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05574
[Merge] lp:~abentley/launchpad/view-flags into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/view-flags into lp:launchpad with lp:~abentley/launchpad/next-prev as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #891714 in Launchpad itself: "Feature flags are not accessible in JavaScript."
https://bugs.launchpad.net/launchpad/+bug/891714
For more details, see:
https://code.launchpad.net/~abentley/launchpad/view-flags/+merge/82570
= 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
--
https://code.launchpad.net/~abentley/launchpad/view-flags/+merge/82570
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/view-flags into lp:launchpad.
=== 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-17 17:21:30 +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-17 17:21:30 +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-17 17:21:30 +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-17 17:21:30 +0000
@@ -18,11 +18,22 @@
* 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 = [];
+ Y.each(LP.cache.related_features, function(info){
+ if (!info.is_beta){
+ return;
+ }
+ 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('')));
+ });
+ 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 +48,9 @@
'<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>'));
- }
+ Y.each(notifications, function(notification){
+ beta_notification_node.appendChild(notification);
+ });
close_box.on('click', function(e) {
e.halt();
var fade_out = new Y.Anim({
=== 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-17 17:21:30 +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 = {
+ related_features: {
+ 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-17 17:17:38 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-11-17 17:21:30 +0000
@@ -2453,7 +2453,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 = (
Follow ups
