launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05855
[Merge] lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #892211 in Launchpad itself: "reset to default in bug listing visibility widget doesn't work"
https://bugs.launchpad.net/launchpad/+bug/892211
For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-reset-to-default/+merge/84833
= Summary =
Fix bug #892211: reset to default in bug listing visibility widget doesn't work
== Proposed fix ==
Stop updating from cookies, since those are processed server-side.
Fix cookie removal.
== Pre-implementation notes ==
Discussed with Deryck
== Implementation details ==
The field_visibility values are calculated on the server side with due consideration to the cookie value, so there's no need read the cookie on the client side. On load, it will be in sync with the field_visibility values, and afterward, we explicitly synchronize it with the current settings. So it is reasonable to remove this functionality.
This solves bug #892211, but there was an additional problem that the cookies were not being deleted by "Reset to default". The test for this was faulty, because it assumed that Y.Cookie.get would return "" for a deleted cookie, when in fact, it returns null. The Chrom(e|ium) workaround of Y.Cookie._setDoc({cookie: ""}) breaks Y.Cookie.remove, so we now use it only for Chromium-base browsers, and we disable buglisting_display_utils_tests
== Tests ==
bin/test -t test_buglisting_utils.html
== Demo and Q/A ==
Go to a bug listing.
Click the gear icon.
Add "Bug age" to the list of fields.
Click the gear icon.
Choose "reset to default".
The default format should be displayed.
Hit reload.
The default format should still be displayed.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/tests/test_buglisting_utils.js
lib/lp/bugs/javascript/buglisting_utils.js
--
https://code.launchpad.net/~abentley/launchpad/fix-reset-to-default/+merge/84833
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js 2011-12-05 06:27:11 +0000
+++ lib/lp/bugs/javascript/buglisting_utils.js 2011-12-07 20:11:26 +0000
@@ -250,31 +250,6 @@
},
/**
- * Update field_visibility based on fields stored
- * in cookies. This is used as a light-weight
- * page to page persistence mechanism.
- *
- * @method updateFromCookie
- */
- updateFromCookie: function() {
- var cookie_name = this.get('cookie_name');
- var cookie_fields = Y.Cookie.getSubs(cookie_name);
- if (Y.Lang.isValue(cookie_fields)) {
- // We get true/false back as strings from Y.Cookie,
- // so we have to convert them to booleans.
- Y.each(cookie_fields, function(val, key, obj) {
- if (val === 'true') {
- val = true;
- } else {
- val = false;
- }
- obj[key] = val;
- });
- this.updateFieldVisibilty(cookie_fields);
- }
- },
-
- /**
* Set the given value for the buglisting config cookie.
* If config is not specified, the cookie will be cleared.
*
@@ -287,7 +262,7 @@
path: '/',
expires: new Date('January 19, 2038')});
} else {
- Y.Cookie.remove(cookie_name);
+ Y.Cookie.remove(cookie_name, {path: '/'});
}
},
@@ -298,7 +273,6 @@
* @method _extraRenderUI
*/
_extraRenderUI: function() {
- this.updateFromCookie();
var form_content = this.buildFormContent();
var on_submit_callback = Y.bind(this.handleOverlaySubmit, this);
util_overlay = new Y.lazr.FormOverlay({
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-11-28 21:28:44 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-12-07 20:11:26 +0000
@@ -10,10 +10,19 @@
var ArrayAssert = Y.ArrayAssert;
var ObjectAssert = Y.ObjectAssert;
-suite.add(new Y.Test.Case({
+
+var running_in_chrome = function(){
+ return (/Chrome/).test(navigator.userAgent);
+};
+
+
+var buglisting_display_utils_tests = new Y.Test.Case({
name: 'buglisting_display_utils_tests',
+ _should: {ignore: []},
+
+
setUp: function() {
// Default values for model config.
this.defaults = {
@@ -48,7 +57,9 @@
}
};
// _setDoc is required for tests using cookies to pass.
- Y.Cookie._setDoc({cookie: ""});
+ if (running_in_chrome()){
+ Y.Cookie._setDoc({cookie: ""});
+ }
// Simulate LP.cache.field_visibility which will be
// present in the actual page.
window.LP = {
@@ -69,8 +80,10 @@
this.list_util.destroy();
}
// Cleanup cookies.
- Y.Cookie.remove(this.cookie_name);
- Y.Cookie._setDoc(Y.config.doc);
+ Y.Cookie.remove(this.cookie_name, {path: "/"});
+ if (running_in_chrome()){
+ Y.Cookie._setDoc(Y.config.doc);
+ }
},
/**
@@ -122,32 +135,6 @@
this.defaults.field_visibility_defaults);
},
- test_cookie_updates_field_visibility_config: function() {
- // If the $USER-buglist-fields cookie is present,
- // the widget will update field_visibility to these values.
- var expected_config = {
- show_title: true,
- show_id: true,
- show_importance: true,
- show_status: true,
- show_bug_heat: false,
- show_bugtarget: false,
- show_age: true,
- show_last_updated: true,
- show_assignee: false,
- show_reporter: false,
- show_milestone_name: false,
- show_tags: false
- };
- Y.Cookie.setSubs(this.cookie_name, expected_config);
- this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil(
- this.defaults);
- this.list_util.render();
- var model = this.list_util.get('model');
- var actual_config = model.get_field_visibility();
- ObjectAssert.areEqual(expected_config, actual_config);
- },
-
test_field_visibility_form_reference: function() {
// The form created from field_visibility defaults is referenced
// via BugListingConfigUtil.get('form')
@@ -344,6 +331,7 @@
field_visibility: field_visibility,
field_visibility_defaults: this.defaults.field_visibility_defaults
});
+ this.list_util.setCookie(field_visibility);
this.list_util.render();
// Poke at the page to reset the form.
var config = Y.one('.config');
@@ -426,6 +414,9 @@
test_form_reset_removes_cookie: function() {
// Clicking "reset to defaults" on the overlay will
// remove any cookie added.
+
+ Y.Cookie.remove(this.cookie_name, {path: "/"});
+ Assert.isNull(Y.Cookie.get(this.cookie_name));
this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil(
this.defaults);
this.list_util.render();
@@ -438,12 +429,23 @@
update.simulate('click');
// Now reset from the form.
config.simulate('click');
+ Assert.isNotNull(Y.Cookie.get(this.cookie_name));
Y.one('.reset-buglisting').simulate('click');
- var cookie = Y.Cookie.get(this.cookie_name);
- Assert.areSame('', cookie);
+ Assert.isNull(Y.Cookie.get(this.cookie_name));
}
-}));
+});
+
+
+/**
+ * The Chrome workaround breaks Y.Cookie.remove behaviour
+ */
+if (running_in_chrome()){
+ var ignore = buglisting_display_utils_tests._should.ignore;
+ ignore.test_form_reset_removes_cookie = true;
+}
+
+suite.add(buglisting_display_utils_tests);
buglisting_utils.suite = suite;
