launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/sharing-overlay-buttons into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #758929 in Launchpad itself: "On translate sharing details page, "Submit" button on AJAX overlays doesn't say what it's submitting"
  https://bugs.launchpad.net/launchpad/+bug/758929

For more details, see:
https://code.launchpad.net/~abentley/launchpad/sharing-overlay-buttons/+merge/57935

= Summary =
Fix bug #758929: On translate sharing details page, "Submit" button on AJAX overlays doesn't say what it's submitting

== Proposed fix ==
Use the checkmark or X submit/cancel buttons used on other AJAX forms.

== Pre-implementation notes ==
Discussed with jml.

== Implementation details ==
Extracted create_form_overlay from prepare so that setting submit/cancel buttons could be done in a single place.

== Tests ==
None

== Demo and Q/A ==
Go to +sharing-details for a sourcepackage.  Click on the bottom two edit icons.  The displayed form should show checkbox and X for submit and cancel, and they should work as expected.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js
  lib/lp/translations/browser/tests/test_sharing_details.py
  lib/lp/translations/javascript/sourcepackage_sharing_details.js
  lib/lp/translations/browser/sourcepackage.py
-- 
https://code.launchpad.net/~abentley/launchpad/sharing-overlay-buttons/+merge/57935
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/sharing-overlay-buttons into lp:launchpad.

=== modified file 'lib/lp/translations/browser/sourcepackage.py'
--- lib/lp/translations/browser/sourcepackage.py	2011-04-11 17:16:28 +0000
+++ lib/lp/translations/browser/sourcepackage.py	2011-04-15 18:50:39 +0000
@@ -151,7 +151,7 @@
         if self.has_upstream_branch:
             # Normally should use BranchFormatterAPI(branch).link(None), but
             # on this page, that information is redundant.
-            title = self.upstream_branch.unique_name
+            title = 'lp:' + self.upstream_branch.unique_name
             url = canonical_url(self.upstream_branch)
         else:
             title = ''

=== modified file 'lib/lp/translations/browser/tests/test_sharing_details.py'
--- lib/lp/translations/browser/tests/test_sharing_details.py	2011-04-11 21:52:14 +0000
+++ lib/lp/translations/browser/tests/test_sharing_details.py	2011-04-15 18:50:39 +0000
@@ -164,6 +164,11 @@
         self.configureSharing(set_upstream_branch=True)
         self.assertTrue(self.view.has_upstream_branch)
 
+    def test_branch_link_text(self):
+        self.configureSharing(set_upstream_branch=True)
+        expected_text = '>lp:%s</a>' % self.view.upstream_branch.unique_name
+        self.assertIn(expected_text, self.view.branch_link.escapedtext)
+
     def test_is_upstream_translations_enabled__no_packaging_link(self):
         # If the source package is not linked to an upstream series,
         # is_upstream_translations_enabled returns False.

=== modified file 'lib/lp/translations/javascript/sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/sourcepackage_sharing_details.js	2011-04-13 17:39:32 +0000
+++ lib/lp/translations/javascript/sourcepackage_sharing_details.js	2011-04-15 18:50:39 +0000
@@ -140,6 +140,35 @@
 namespace.TranslationSharingConfig = TranslationSharingConfig;
 
 
+/**
+ * Create a form overlay for a submittable form.
+ */
+function create_form_overlay(title_html, submit_callback){
+    var submit_button = Y.Node.create(
+        '<button type="submit" name="field.actions.create" ' +
+        'value="Save Changes" class="lazr-pos lazr-btn" ' +
+        '>OK</button>'
+        );
+    var cancel_button = Y.Node.create(
+        '<button type="button" name="field.actions.cancel" ' +
+        'class="lazr-neg lazr-btn" >Cancel</button>'
+        );
+    function hide_and_callback(){
+        form_overlay.hide();
+        submit_callback.apply(this, arguments);
+    }
+    var form_overlay = new Y.lazr.FormOverlay({
+        centered: true,
+        headerContent: title_html,
+        form_cancel_button: cancel_button,
+        form_submit_button: submit_button,
+        form_submit_callback: hide_and_callback,
+        visible: false
+    });
+    return form_overlay;
+}
+
+
 function form_url(entry, view_name) {
     entry_url = Y.lp.get_url_path(entry.get('web_link'));
     return entry_url + '/' + view_name + '/++form++';
@@ -321,7 +350,7 @@
         var check = this.get('tsconfig').get('branch');
         if (Y.Lang.isValue(branch)){
             check.set_link(
-                branch.get('unique_name'), branch.get('web_link'));
+                'lp:' + branch.get('unique_name'), branch.get('web_link'));
         }
         else {
             check.clear_link();
@@ -490,7 +519,7 @@
     add_activator(picker, '#branch-complete-picker a');
     var productseries_picker_config = {
         picker_activator: '#packaging-complete-picker a',
-        header : 'Select productseries',
+        header : 'Select project series',
         step_title: 'Search',
         save: Y.bind('select_productseries', sharing_controller),
         context: cache.product
@@ -499,17 +528,12 @@
         'ProductSeries', productseries_picker_config);
     /* Picker can't normally be activated by two different elements. */
     add_activator(productseries_picker, '#packaging-incomplete-picker a');
-    var import_overlay = new Y.lazr.FormOverlay({
-        headerContent: '<h2>Import settings<h2>',
-        centered: true,
-        visible: false
-    });
-    import_overlay.set('form_submit_callback', function(form_data) {
+    var import_overlay = create_form_overlay(
+        '<h2>Import settings<h2>', function(form_data) {
         Y.log(form_data['field.translations_autoimport_mode']);
         mode = enum_title(
             form_data, 'field.translations_autoimport_mode',
             namespace.autoimport_modes);
-        import_overlay.hide();
         var product_series = sharing_controller.get('productseries');
         product_series.set('translations_autoimport_mode', mode);
         var autoimport_check = sharing_controller.get(
@@ -533,14 +557,9 @@
     });
     var autoimport = sharing_controller.get('tsconfig').get('autoimport');
     sharing_controller.set_check_picker(autoimport, import_overlay);
-    var usage_overlay = new Y.lazr.FormOverlay({
-        headerContent: '<h2>Usage settings<h2>',
-        centered: true,
-        visible: false
-    });
     var usage = sharing_controller.get('tsconfig').get('translations_usage');
-    usage_overlay.set('form_submit_callback', function(form_data) {
-        usage_overlay.hide();
+    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);

=== modified file 'lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js	2011-04-07 15:39:20 +0000
+++ lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js	2011-04-15 18:50:39 +0000
@@ -37,6 +37,18 @@
             product_series.set_link('ps', 'http://');
             Y.Assert.isTrue(sharing_config.get('branch').get('enabled'));
         },
+        test_set_branch: function() {
+            var lp_client = new Y.lp.client.Launchpad();
+            var branch = lp_client.wrap_resource('http://example.com', {
+                unique_name: 'unique',
+                web_link: 'http://example.com',
+                resource_type_link: 'http://foo_type'
+            });
+            var sharing_controller = new TranslationSharingController();
+            sharing_controller.set_branch(branch);
+            var check = sharing_controller.get('tsconfig').get('branch');
+            Y.Assert.areEqual('lp:unique', check.get('text'));
+        },
         test_autoimport: function() {
             var sharing_config = new TranslationSharingConfig();
             Y.Assert.isFalse(sharing_config.get('autoimport').get('enabled'));
@@ -125,7 +137,7 @@
             Y.Assert.areEqual(
                 tsconfig.get('product_series').get('url'), 'http://web1');
             Y.Assert.areEqual(
-                tsconfig.get('branch').get('text'), 'title2');
+                tsconfig.get('branch').get('text'), 'lp:title2');
             Y.Assert.isTrue(tsconfig.get('autoimport').get('complete'));
             Y.Assert.isTrue(
                 tsconfig.get('translations_usage').get('complete'));