launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04401
[Merge] lp:~abentley/launchpad/translations-sharing into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/translations-sharing into lp:launchpad with lp:~abentley/launchpad/reload-cache as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~abentley/launchpad/translations-sharing/+merge/69531
= Summary =
Reduce roundtrips on +sharing-details page
== Proposed fix ==
Use the new lp.client.load_model functionality to retrieve an updated copy of
the IJSONRequestCache.
== Pre-implementation notes ==
None
== Implementation details ==
This is based on ~abentley/launchpad/reload-cache, which implemented the
load_model functionality.
Most of the updates that happen on +sharing-details require updating more than
one object. In this branch, those methods are changed to use load_model,
via IOHandler.refresh_config.
TranslationSharingController.update_from_model is extracted to update
everything from a new copy of the model. This makes many existing functions
redundant, and so they are deleted.
IOHandler.refresh_config automates the whole process, including the green flash
on success. This is now tested, using IORecorder. The simplification means
that chain_config is no longer needed for most operations; get_config can be
used instead.
== Tests ==
bin/test -t test_sourcepackage_sharing_details
== Demo and Q/A ==
Go to the +sharing-details for a sourcepackage. All operations should succeed,
and do so quickly.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/tests/test_lp_client.js
lib/lp/soyuz/browser/distroseriessourcepackagerelease.py
lib/lp/registry/browser/sourcepackage.py
lib/lp/registry/browser/peoplemerge.py
lib/canonical/launchpad/webapp/publisher.py
lib/canonical/launchpad/webapp/templates/launchpad-model.pt
lib/lp/code/browser/bazaar.py
lib/lp/registry/browser/codeofconduct.py
lib/lp/soyuz/browser/sourcepackage.py
lib/lp/registry/browser/distributionsourcepackage.py
lib/canonical/launchpad/webapp/tests/test_publisher.py
lib/lp/app/javascript/client.js
lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js
lib/lp/app/javascript/tests/test_lp_client.html
lib/lp/soyuz/browser/distroseriesbinarypackage.py
lib/lp/app/templates/base-layout-macros.pt
lib/lp/testing/__init__.py
lib/lp/bugs/browser/configure.zcml
lib/canonical/launchpad/webapp/tests/test_view_model.py
lib/lp/bugs/browser/cve.py
lib/lp/soyuz/browser/distroarchseriesbinarypackagerelease.py
lib/lp/soyuz/browser/distroarchseriesbinarypackage.py
lib/lp/translations/browser/tests/test_sharing_details.py
lib/canonical/launchpad/webapp/configure.zcml
lib/lp/translations/browser/translationgroup.py
lib/lp/app/webservice/tests/test_marshallers.py
lib/lp/blueprints/browser/configure.zcml
lib/lp/app/javascript/testing/iorecorder.js
lib/lp/translations/javascript/sourcepackage_sharing_details.js
lib/lp/registry/browser/teammembership.py
lib/lp/app/webservice/marshallers.py
lib/lp/translations/browser/translations.py
lib/canonical/launchpad/webapp/namespace.py
lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.html
lib/lp/translations/browser/sourcepackage.py
lib/lp/registry/browser/tests/test_subscription_links.py
./lib/lp/translations/browser/tests/test_sharing_details.py
218: E251 no spaces around keyword / parameter equals
226: E251 no spaces around keyword / parameter equals
254: E251 no spaces around keyword / parameter equals
265: E251 no spaces around keyword / parameter equals
277: E251 no spaces around keyword / parameter equals
311: E251 no spaces around keyword / parameter equals
--
https://code.launchpad.net/~abentley/launchpad/translations-sharing/+merge/69531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/translations-sharing into lp:launchpad.
=== modified file 'lib/lp/translations/javascript/sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/sourcepackage_sharing_details.js 2011-07-27 19:02:50 +0000
+++ lib/lp/translations/javascript/sourcepackage_sharing_details.js 2011-07-27 19:02:51 +0000
@@ -219,6 +219,8 @@
function IOHandler(controller, check, error_handler) {
that = this;
+ this.check = check;
+ this.controller = controller;
if (!Y.Lang.isValue(error_handler)){
this.error_handler = new Y.lp.client.ErrorHandler();
}
@@ -233,6 +235,23 @@
};
}
+IOHandler.prototype.show_success = function(){
+ this.check.set('pending', false);
+ this.controller.update();
+ this.controller.flash_check_green(this.check);
+};
+
+IOHandler.prototype.refresh_from_model = function(model){
+ this.controller.update_from_model(model);
+ this.show_success();
+};
+
+IOHandler.prototype.refresh_config = function(){
+ return this.chain_config(
+ Y.bind("load_model", this.controller),
+ Y.bind('refresh_from_model', this));
+};
+
/**
* Return an LP client config using error_handler.
@@ -273,6 +292,9 @@
};
+namespace.IOHandler = IOHandler;
+
+
/**
* This class is the controller for updating the TranslationSharingConfig.
* It handles updating the HTML and the DB model.
@@ -296,17 +318,24 @@
* @param branch_summary {Object} An object containing api_url, css,
* description, value, title
*/
- configure: function(config, branch_picker_config, unlink_overlay,
+ configure: function(model, branch_picker_config, unlink_overlay,
import_overlay, usage_overlay) {
this.set('branch_picker_config', branch_picker_config);
- this.set('source_package', config.context);
this.set('unlink_overlay', unlink_overlay);
this.set('import_overlay', import_overlay);
this.set('usage_overlay', usage_overlay);
- this.replace_productseries(config.productseries);
- this.replace_product(config.product);
- this.set_branch(config.upstream_branch);
- this.set_permissions(config);
+ this.update_from_model(model);
+ },
+ update_from_model: function(model) {
+ this.set('source_package', model.context);
+ this.replace_productseries(model.productseries);
+ this.replace_product(model.product);
+ this.set_branch(model.upstream_branch);
+ this.set_permissions(model);
+ },
+ load_model: function(config){
+ var source_package = this.get('source_package');
+ Y.lp.client.load_model(source_package, '+sharing-details', config);
},
set_permissions: function(permissions){
var usage = this.get('tsconfig').get('translations_usage');
@@ -399,37 +428,10 @@
var source_package = that.get('source_package');
config.parameters = {
productseries: productseries_summary.api_uri};
- source_package.named_post(
- 'setPackagingReturnSharingDetailPermissions', config);
- }
- function get_productseries(config, permissions) {
- that.set_permissions(permissions);
- lp_client.get(productseries_summary.api_uri, config);
- }
- function cache_productseries(config, productseries) {
- that.replace_productseries(productseries);
- var branch_link = productseries.get('branch_link');
- if (branch_link === null){
- config.on.success(null);
- }
- else {
- lp_client.get(branch_link, config);
- }
- }
- function cache_branch(config, branch) {
- that.set_branch(branch);
- project_link = that.get('productseries').get('project_link');
- lp_client.get(project_link, config);
- }
- function set_usage(product) {
- that.replace_product(product);
- productseries_check.set('pending', false);
- that.update();
- that.flash_check_green(productseries_check);
+ source_package.named_post('setPackaging', config);
}
var io_handler = new IOHandler(this, productseries_check);
- save_productseries(io_handler.chain_config(
- get_productseries, cache_productseries, cache_branch, set_usage));
+ save_productseries(io_handler.refresh_config());
},
remove_productseries: function(productseries_summary) {
var that = this;
@@ -444,12 +446,10 @@
that.replace_productseries(null);
that.replace_product(null);
that.set_branch(null);
- productseries_check.set('pending', false);
- that.update();
- that.flash_check_green(productseries_check);
+ io_handler.show_success();
}
var io_handler = new IOHandler(this, productseries_check);
- delete_packaging(io_handler.chain_config(set_checks));
+ delete_packaging(io_handler.get_config(set_checks));
},
select_branch: function(branch_summary) {
var that = this;
@@ -457,35 +457,14 @@
var branch_check = that.get('tsconfig').get('branch');
var productseries = that.get('productseries');
- /* Here begin a series of methods which each represent a step in
- * setting the branch. They each take a config to use in an lp_client
- * call, except the last one. This allows them to be chained
- * together.
- *
- * They take full advantage of their access to variables in the
- * closure, such as "that" and "branch_summary".
- */
function save_branch(config) {
branch_check.set('pending', true);
that.update_check(branch_check);
productseries.set('branch_link', branch_summary.api_uri);
productseries.lp_save(config);
}
- function get_branch(config){
- lp_client.get(branch_summary.api_uri, config);
- }
- function set_link(config, branch){
- that.set_branch(branch);
- lp_client.get(productseries.get('self_link'), config);
- }
- function finish(new_productseries){
- that.replace_productseries(new_productseries);
- branch_check.set('pending', false);
- that.update();
- that.flash_check_green(branch_check);
- }
var io_handler = new IOHandler(this, branch_check);
- save_branch(io_handler.chain_config(get_branch, set_link, finish));
+ save_branch(io_handler.refresh_config());
},
/**
* Update the display of all checklist items.
@@ -510,6 +489,9 @@
visible_check_selector: function(check) {
return this.check_selector(check, check.get('complete'));
},
+ spinner_selector: function(check) {
+ return this.visible_check_selector(check) + '-spinner';
+ },
picker_selector: function(check, complete) {
return this.check_selector(check, complete) + '-picker a';
},
@@ -541,8 +523,7 @@
if (incomplete_picker !== null) {
incomplete_picker.toggleClass('unseen', hide_picker);
}
- var selector = this.visible_check_selector(check) + '-spinner';
- var spinner = Y.one(selector);
+ var spinner = Y.one(this.spinner_selector(check));
if (Y.Lang.isValue(spinner)){
spinner.toggleClass('unseen', !check.get('pending'));
}
@@ -611,9 +592,7 @@
handler = new IOHandler(sharing_controller, autoimport_check);
function update_controller() {
sharing_controller.set_autoimport_mode(mode);
- autoimport_check.set('pending', false);
- sharing_controller.update();
- sharing_controller.flash_check_green(autoimport_check);
+ handler.show_success();
}
autoimport_check.set('pending', true);
sharing_controller.update_check(autoimport_check);
@@ -624,7 +603,7 @@
* the value stored in productseries.
*/
product_series.removeAttr('http_etag');
- product_series.lp_save(handler.chain_config(update_controller));
+ product_series.lp_save(handler.get_config(update_controller));
});
var autoimport = sharing_controller.get('tsconfig').get('autoimport');
sharing_controller.set_check_picker(autoimport, import_overlay);
@@ -632,33 +611,11 @@
var usage_overlay = create_form_overlay(
'<h2>Configure translations<h2>', function(form_data) {
var product = sharing_controller.get('product');
- function reload_entry(config, entry) {
- lp_client.get(entry.get('self_link'), config);
- }
- function get_productseries(config) {
- var productseries = sharing_controller.get('productseries');
- reload_entry(config, productseries);
- }
- function replace_productseries(config, new_productseries) {
- sharing_controller.replace_productseries(new_productseries);
- config.on.success();
- }
- function get_product(config) {
- reload_entry(config, product);
- }
- function replace_product(new_product) {
- sharing_controller.replace_product(new_product);
- usage.set('pending', false);
- sharing_controller.update();
- sharing_controller.flash_check_green(usage);
- }
var io_handler = new IOHandler(
sharing_controller, usage, new Y.lp.client.FormErrorHandler());
usage.set('pending', true);
sharing_controller.update_check(usage);
- var config = io_handler.chain_config(
- get_productseries, replace_productseries, get_product,
- replace_product);
+ var config = io_handler.refresh_config();
submit_form(
config, form_data, product, '+configure-translations', 'change');
});
@@ -669,4 +626,5 @@
sharing_controller.update();
};
}, "0.1", {"requires": [
- 'lp', 'lp.app.errors', 'lp.app.picker', 'oop', 'lp.client']});
+ 'lp', 'lp.app.errors', 'lp.app.picker', 'oop', 'lp.client',
+ 'lazr.anim']});
=== modified file 'lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.html'
--- lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.html 2011-07-08 06:06:15 +0000
+++ lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.html 2011-07-27 19:02:51 +0000
@@ -12,6 +12,7 @@
<script type="text/javascript" src="../../../app/javascript/client.js"></script>
<script type="text/javascript" src="../../../app/javascript/lp.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/anim/anim.js"></script>
<!-- The module under test -->
<script type="text/javascript" src="../sourcepackage_sharing_details.js"></script>
=== modified file 'lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js 2011-07-27 19:02:50 +0000
+++ lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js 2011-07-27 19:02:51 +0000
@@ -7,6 +7,7 @@
var suite = new Y.Test.Suite("sourcepackage_sharing_details Tests");
var namespace = Y.lp.translations.sourcepackage_sharing_details;
+ var IOHandler = namespace.IOHandler;
var TranslationSharingConfig = namespace.TranslationSharingConfig;
var TranslationSharingController = namespace.TranslationSharingController;
var CheckItem = (
@@ -101,12 +102,12 @@
},
test_configure_empty: function() {
var ctrl = new TranslationSharingController();
- var cache = {
+ var model = {
productseries: null,
product: null,
upstream_branch: null
};
- ctrl.configure(cache, {});
+ ctrl.configure(model, {});
},
test_configure: function() {
var cache = {
@@ -133,31 +134,90 @@
};
var ctrl = new TranslationSharingController();
var lp_client = new Y.lp.client.Launchpad();
- cache = lp_client.wrap_resource(null, cache);
- var import_overlay = {
- loadFormContentAndRender: function() {},
- render: function() {},
- get: function(ignore) {
- return Y.Node.create(
- '<p><a href="http://fake";>fake</a></p>');
- }
- };
- unlink_overlay = import_overlay;
- usage_overlay = import_overlay;
- ctrl.configure(
- cache, {}, unlink_overlay, import_overlay, usage_overlay);
- var tsconfig = ctrl.get('tsconfig');
- Y.Assert.areEqual(
- tsconfig.get('product_series').get('text'), 'title1');
- Y.Assert.areEqual(
- tsconfig.get('product_series').get('url'), 'http://web1');
- Y.Assert.areEqual(
- tsconfig.get('branch').get('text'), 'lp:title2');
- Y.Assert.isTrue(tsconfig.get('branch').get('user_authorized'));
- Y.Assert.isTrue(tsconfig.get('autoimport').get('complete'));
- Y.Assert.isTrue(
- tsconfig.get('translations_usage').get('complete'));
- Y.Assert.areSame(ctrl.get('source_package'), cache.context);
+ var model = lp_client.wrap_resource(null, cache);
+ var import_overlay = {
+ loadFormContentAndRender: function() {},
+ render: function() {},
+ get: function(ignore) {
+ return Y.Node.create(
+ '<p><a href="http://fake";>fake</a></p>');
+ }
+ };
+ unlink_overlay = import_overlay;
+ usage_overlay = import_overlay;
+ ctrl.configure(
+ model, {}, unlink_overlay, import_overlay, usage_overlay);
+ var tsconfig = ctrl.get('tsconfig');
+ Y.Assert.areEqual(
+ tsconfig.get('product_series').get('text'), 'title1');
+ Y.Assert.areEqual(
+ tsconfig.get('product_series').get('url'), 'http://web1');
+ Y.Assert.areEqual(
+ tsconfig.get('branch').get('text'), 'lp:title2');
+ Y.Assert.isTrue(tsconfig.get('branch').get('user_authorized'));
+ Y.Assert.isTrue(tsconfig.get('autoimport').get('complete'));
+ Y.Assert.isTrue(
+ tsconfig.get('translations_usage').get('complete'));
+ Y.Assert.areSame(ctrl.get('source_package'), model.context);
+ },
+ test_update_from_model: function() {
+ var null_model = {
+ productseries: null,
+ product: null,
+ upstream_branch: null
+ };
+
+ var cache = {
+ product: {
+ translations_usage: test_ns.usage.launchpad,
+ resource_type_link: 'http://product'
+ },
+ productseries: {
+ title: 'title1',
+ web_link: 'http://web1',
+ translations_autoimport_mode: (
+ test_ns.autoimport_modes.import_translations),
+ resource_type_link: 'productseries'
+ },
+ upstream_branch: {
+ unique_name: 'title2',
+ web_link: 'http://web2',
+ resource_type_link: 'branch'
+ },
+ context: {
+ resource_type_link: 'http://sourcepackage'
+ },
+ user_can_change_branch: true
+ };
+ var ctrl = new TranslationSharingController();
+ var lp_client = new Y.lp.client.Launchpad();
+ var model = lp_client.wrap_resource(null, cache);
+ var import_overlay = {
+ loadFormContentAndRender: function() {},
+ render: function() {},
+ get: function(ignore) {
+ return Y.Node.create(
+ '<p><a href="http://fake";>fake</a></p>');
+ }
+ };
+ unlink_overlay = import_overlay;
+ usage_overlay = import_overlay;
+ ctrl.configure(
+ null_model, {}, unlink_overlay, import_overlay,
+ usage_overlay);
+ ctrl.update_from_model(model);
+ var tsconfig = ctrl.get('tsconfig');
+ Y.Assert.areEqual(
+ tsconfig.get('product_series').get('text'), 'title1');
+ Y.Assert.areEqual(
+ tsconfig.get('product_series').get('url'), 'http://web1');
+ Y.Assert.areEqual(
+ tsconfig.get('branch').get('text'), 'lp:title2');
+ Y.Assert.isTrue(tsconfig.get('branch').get('user_authorized'));
+ Y.Assert.isTrue(tsconfig.get('autoimport').get('complete'));
+ Y.Assert.isTrue(
+ tsconfig.get('translations_usage').get('complete'));
+ Y.Assert.areSame(ctrl.get('source_package'), model.context);
},
test_set_permissions: function(){
var ctrl = new TranslationSharingController();
@@ -324,6 +384,21 @@
Y.Assert.isTrue(check.get('complete'));
}
}));
+ suite.add(new Y.Test.Case({
+ name: 'setup',
+ test_show_success: function(){
+ var controller = new TranslationSharingController();
+ var check = controller.get('tsconfig').get('branch');
+ check.set('pending', true);
+ controller.update_check(check);
+ spinner = Y.one(controller.spinner_selector(check));
+ Y.Assert.isFalse(spinner.hasClass('unseen'));
+ io_handler = new IOHandler(controller, check);
+ io_handler.show_success();
+ Y.Assert.isFalse(check.get('pending'));
+ Y.Assert.isTrue(spinner.hasClass('unseen'));
+ }
+ }));
Y.lp.testing.Runner.run(suite);
