Merge lp:~abentley/launchpad/translations-sharing into lp:launchpad

[Aaron Bentley]

+sharing-details uses load_model.

= 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

[j.c.sackett (jcsackett)]

This looks good.

I'm really impressed by the cleanup of all the methods that update parts of an object into a method that updates everything; this is very cool work.

[j.c.sackett (jcsackett)]

review: Approve

=== modified file 'lib/lp/translations/javascript/sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/sourcepackage_sharing_details.js	2011-07-28 15:32:42 +0000
+++ lib/lp/translations/javascript/sourcepackage_sharing_details.js	2011-07-28 15:32:44 +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-28 15:32:44 +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-28 15:32:42 +0000
+++ lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js	2011-07-28 15:32:44 +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);