launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05521
[Merge] lp:~deryck/launchpad/field-visibility-dry into lp:launchpad
Deryck Hodge has proposed merging lp:~deryck/launchpad/field-visibility-dry into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~deryck/launchpad/field-visibility-dry/+merge/82316
This branch fixes BugListingConfigUtil to get its default values from the json cache rather than hard-coded in js. This is mostly to prevent violating DRY, but there's also some minor cleanup here. I've also pushed the defaults in the cache separately from field_visibility since the widget always needs a reference to the defaults, and I've added a test for that. I've also ensured the widget updates field_visibility in the cache, since other code may look to the cache to get these values.
--
https://code.launchpad.net/~deryck/launchpad/field-visibility-dry/+merge/82316
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/field-visibility-dry into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-11-14 17:57:46 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-11-15 21:07:29 +0000
@@ -2266,6 +2266,7 @@
'show_tags': False,
'show_title': True,
}
+ self.field_visibility_defaults = self.field_visibility
TableBatchNavigator.__init__(
self, tasks, request, columns_to_show=columns_to_show, size=size)
@@ -2591,6 +2592,8 @@
cache.objects['mustache_model'] = batch_navigator.model
cache.objects['field_visibility'] = (
batch_navigator.field_visibility)
+ cache.objects['field_visibility_defaults'] = (
+ batch_navigator.field_visibility_defaults)
def _getBatchInfo(batch):
if batch is None:
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-11-14 15:59:34 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-11-15 21:07:29 +0000
@@ -1783,6 +1783,15 @@
field_visibility = cache.objects['field_visibility']
self.assertTrue(field_visibility['show_title'])
+ def test_cache_field_visibility_defaults(self):
+ """Cache contains sane-looking field_visibility_defaults values."""
+ task = self.factory.makeBugTask()
+ with self.dynamic_listings():
+ view = self.makeView(task, memo=1, forwards=False, size=1)
+ cache = IJSONRequestCache(view.request)
+ field_visibility_defaults = cache.objects['field_visibility_defaults']
+ self.assertTrue(field_visibility_defaults['show_title'])
+
def getBugtaskBrowser(self, title=None, no_login=False):
"""Return a browser for a new bugtask."""
bugtask = self.factory.makeBugTask()
=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js 2011-11-11 21:10:55 +0000
+++ lib/lp/bugs/javascript/buglisting_utils.js 2011-11-15 21:07:29 +0000
@@ -36,24 +36,6 @@
BugListingConfigUtil.NAME = 'buglisting-config-util';
/**
- * Object to reference defaults for field_visibility.
- */
- BugListingConfigUtil.field_visibility_defaults = {
- show_title: true,
- show_id: true,
- show_importance: true,
- show_status: true,
- show_bug_heat: true,
- show_bugtarget: true,
- show_age: false,
- show_last_updated: false,
- show_assignee: false,
- show_reporter: false,
- show_milestone_name: false,
- show_tags: false
- };
-
- /**
* Object to reference display names for field_visibility
* form inputs.
*/
@@ -83,15 +65,43 @@
*/
field_visibility: {
valueFn: function() {
- return this.constructor.field_visibility_defaults;
+ return this.get('field_visibility_defaults');
},
setter: function(value) {
- var defaults = this.constructor.field_visibility_defaults;
+ var defaults = this.get('field_visibility_defaults');
return Y.merge(defaults, value);
}
},
/**
+ * Defaults from field_visibility which are taken from LP.cache.
+ *
+ * This utility will error if LP.cache doesn't exist or doesn't
+ * have field_visibility defined.
+ *
+ * @attribute field_visibility_defaults
+ * @type Object
+ * @default null
+ * @readOnly
+ */
+ field_visibility_defaults: {
+ valueFn: function() {
+ if (
+ Y.Lang.isValue(window.LP) &&
+ Y.Lang.isValue(LP.cache.field_visibility)) {
+ return LP.cache.field_visibility_defaults;
+ } else {
+ var msg = [
+ 'LP.cache.field_visibility must be defined ',
+ 'when using BugListingConfigUtil.'
+ ].join('');
+ Y.error(msg);
+ }
+ },
+ readOnly: true
+ },
+
+ /**
* A reference to the form overlay used in the overlay.
*
* @attribute form
@@ -171,12 +181,8 @@
link.addClass('reset-buglisting');
link.setContent('Reset to default');
link.on('click', function(e) {
- var defaults = this.constructor.field_visibility_defaults;
- this.set(FIELD_VISIBILITY, defaults);
- var form = this.get(FORM);
- form.hide();
- this.destructor();
- this._extraRenderUI();
+ var defaults = this.get('field_visibility_defaults');
+ this.updateFieldVisibilty(defaults, true);
}, this);
return link;
},
@@ -210,10 +216,29 @@
this.after('field_visibilityChange', function() {
var event_name = this.constructor.NAME + ':fields-changed';
Y.fire(event_name);
+ // Update the cache if it exists.
+ if (Y.Lang.isValue(LP.cache.field_visibility)) {
+ LP.cache.field_visibility = this.get(FIELD_VISIBILITY);
+ }
});
},
/**
+ * Helper method for updating field_visibility.
+ *
+ * @method updateFieldVisibilty
+ */
+ updateFieldVisibilty: function(fields, destroy_form) {
+ this.set(FIELD_VISIBILITY, fields);
+ this.get(FORM).hide();
+ // Destroy the form and rebuild it.
+ if (destroy_form === true) {
+ this.get(FORM).hide();
+ this._extraRenderUI();
+ }
+ },
+
+ /**
* Process the data from the form overlay submit.
*
* data is an object whose members are the checked
@@ -224,7 +249,7 @@
* @method handleOverlaySubmit
*/
handleOverlaySubmit: function(data) {
- var fields = Y.clone(this.constructor.field_visibility_defaults);
+ var fields = this.get('field_visibility_defaults');
var member;
for (member in fields) {
if (fields.hasOwnProperty(member)) {
@@ -239,8 +264,7 @@
}
}
}
- this.set(FIELD_VISIBILITY, fields);
- this.get(FORM).hide();
+ this.updateFieldVisibilty(fields);
},
/**
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-11-11 22:19:55 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-11-15 21:07:29 +0000
@@ -14,6 +14,54 @@
name: 'buglisting_display_utils_tests',
+ _should: {
+ error: {
+
+ test_widget_requires_cache: [
+ 'LP.cache.field_visibility must be defined ',
+ 'when using BugListingConfigUtil.'
+ ].join('')
+ }
+ },
+
+ setUp: function() {
+ // Simulate LP.cache.field_visibility which will be
+ // present in the actual page.
+ window.LP = {
+ cache: {
+ field_visibility: {
+ show_title: true,
+ show_id: false,
+ show_importance: false,
+ show_status: true,
+ show_bug_heat: true,
+ show_bugtarget: true,
+ show_age: false,
+ show_last_updated: false,
+ show_assignee: false,
+ show_reporter: false,
+ show_milestone_name: false,
+ show_tags: false
+ },
+
+ field_visibility_defaults: {
+ show_title: true,
+ show_id: true,
+ show_importance: true,
+ show_status: true,
+ show_bug_heat: true,
+ show_bugtarget: true,
+ show_age: false,
+ show_last_updated: false,
+ show_assignee: false,
+ show_reporter: false,
+ show_milestone_name: false,
+ show_tags: false
+ }
+ }
+ };
+ },
+
tearDown: function() {
if (Y.Lang.isValue(this.list_util)) {
this.list_util.destroy();
@@ -48,14 +96,25 @@
},
test_default_field_visibility_config: function() {
- // The default field_visibility should exist in a new widget.
- var expected_config = {
+ // The default field_visibility must be supplied by LP.cache.
+ this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+ var defaults_from_cache = LP.cache.field_visibility_defaults;
+ var field_visibility_defaults = this.list_util.get(
+ 'field_visibility_defaults');
+ ObjectAssert.areEqual(defaults_from_cache, field_visibility_defaults);
+ },
+
+ test_field_visibility_updates_cache: function() {
+ // Updating field_visbility also updates LP's json cache.
+ this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+ this.list_util.render();
+ var fields = {
show_title: true,
- show_id: true,
- show_importance: true,
- show_status: true,
- show_bug_heat: true,
- show_bugtarget: true,
+ show_id: false,
+ show_importance: false,
+ show_status: false,
+ show_bug_heat: false,
+ show_bugtarget: false,
show_age: false,
show_last_updated: false,
show_assignee: false,
@@ -63,9 +122,33 @@
show_milestone_name: false,
show_tags: false
};
- this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
- ObjectAssert.areEqual(
- expected_config, this.list_util.get('field_visibility'));
+ this.list_util.set('field_visibility', fields);
+ ObjectAssert.areEqual(
+ LP.cache.field_visibility,
+ this.list_util.get('field_visibility'));
+ },
+
+ test_widget_requires_cache: function() {
+ // BugListingConfigUtil requires our JSON cache for
+ // field_visibility and will error on render if not found.
+ delete window.LP;
+ this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+ this.list_util.render();
+ },
+
+ test_field_visibility_defaults_readonly: function() {
+ // field_visibility_defaults is a readOnly attribute,
+ // so field_visibility_defaults will always return the same
+ // as LP.cache.field_visibility_defaults.
+ this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+ var attempted_defaults = {
+ foo: 'bar',
+ baz: 'bop'
+ };
+ this.list_util.set('field_visibility_defaults', attempted_defaults);
+ ObjectAssert.areEqual(
+ this.list_util.get('field_visibility_defaults'),
+ LP.cache.field_visibility_defaults);
},
test_supplied_field_visibility_config: function() {
@@ -288,6 +371,10 @@
config.simulate('click');
Y.one('.reset-buglisting').simulate('click');
Assert.isTrue(event_fired);
+ // Also assert that the cache has been updated.
+ ObjectAssert.areEqual(
+ LP.cache.field_visibility,
+ LP.cache.field_visibility_defaults);
},
test_fields_visibility_form_reset_hides_overlay: function() {
Follow ups
