Merge lp:~wallyworld/launchpad/sharing-picker-title-959417 into lp:launchpad

[Ian Booth]

Improve the title and step title displayed by the sharee picker and fix the re-selection of a sharee.

== Implementation ==

The branch fixes 2 issues with the sharee picker:
1. Improve title and step title
2. After selecting a sharee, if the Back button is used and a new sharee is selected, the old sharee was still being used.

The picker displays different text for title and step title depending on if a new sharee is being added or if an existing sharee is being updated.

New sharee:
Share with a user or team
Select sharing policies for Fred

Edit sharee:
Update sharing policies
Update sharing policies for Fred

A bit of refactoring was done to move the code to render the step 2 content to a separate method.

== Tests ==

Update test_shareepicker.js to test the new title/steptitle behaviour and add a new test for the sharee selection issue.

== Lint ==

Linting changed files:
  lib/lp/registry/javascript/sharing/shareepicker.js
  lib/lp/registry/javascript/sharing/tests/test_shareepicker.js

[Curtis Hovey (sinzui)]

thank you.

review: Approve (code)

=== modified file 'lib/lp/registry/javascript/sharing/shareepicker.js'
--- lib/lp/registry/javascript/sharing/shareepicker.js	2012-03-21 03:18:56 +0000
+++ lib/lp/registry/javascript/sharing/shareepicker.js	2012-03-27 02:40:57 +0000
@@ -52,6 +52,7 @@
         }
         this.set('information_types', information_types);
         this.set('sharing_permissions', sharing_permissions);
+        this.step_one_header = this.get('headerContent');
         var self = this;
         this.subscribe('save', function (e) {
             e.halt();
@@ -63,6 +64,8 @@
             var data = e.details[Y.lazr.picker.Picker.SAVE_RESULT];
             switch(step_nr) {
                 case 1:
+                    data.sharee_name = data.title;
+                    delete data.title;
                     self._display_step_two(data);
                     break;
                 case 2:
@@ -95,6 +98,7 @@
     },
 
     _display_step_one: function() {
+        this.set('headerContent', this.step_one_header);
         this.set(
             'steptitle',
             'Search for user or exclusive team with whom to share');
@@ -105,67 +109,79 @@
         this._fade_in(step_one_content, step_two_content);
     },
 
+    _render_step_two: function(back_enabled, allowed_permissions) {
+        var step_two_html = [
+            '<div class="picker-content-two transparent">',
+            '<div class="step-links">',
+            '<a class="prev js-action" href="#">Back</a>',
+            '<button class="next lazr-pos lazr-btn"></button>',
+            '<a class="next js-action" href="#">Select</a>',
+            '</div></div>'
+            ].join(' ');
+        var self = this;
+        var step_two_content = Y.Node.create(step_two_html);
+        // Remove the back link if required.
+        if (Y.Lang.isBoolean(back_enabled) && !back_enabled ) {
+            step_two_content.one('a.prev').remove(true);
+        } else {
+            step_two_content.one('a.prev').on('click', function(e) {
+                e.halt();
+                self._display_step_one();
+            });
+        }
+        // By default, we only show All or Nothing.
+        if (!Y.Lang.isValue(allowed_permissions)) {
+            allowed_permissions = ['ALL', 'NOTHING'];
+        }
+        var sharing_permissions = [];
+        Y.Array.each(this.get('sharing_permissions'),
+                function(permission) {
+            if (Y.Array.indexOf(
+                    allowed_permissions, permission.value) >=0) {
+                sharing_permissions.push(permission);
+            }
+        });
+        var policy_selector = self._make_policy_selector(sharing_permissions);
+        step_two_content.one('div.step-links')
+            .insert(policy_selector, 'before');
+        step_two_content.all('input[name^=field.permission]')
+                .on('click', function(e) {
+            self._disable_select_if_all_info_choices_nothing(step_two_content);
+        });
+        return step_two_content;
+    },
+
     _display_step_two: function(data) {
-        var title = Y.Lang.substitute('Select sharing policies for {name}',
-            {name: data.title});
-        this.set('steptitle', title);
+        if (Y.Lang.isValue(data.title)) {
+            this.set(
+                'headerContent',
+                Y.Node.create('<h2></h2>').set('text', data.title));
+        }
+        var steptitle = data.steptitle;
+        if (!Y.Lang.isValue(steptitle)) {
+            steptitle = Y.Lang.substitute(
+                'Select sharing policies for {name}',
+                {name: data.sharee_name});
+        }
+        this.set('steptitle', steptitle);
         this.set('progress', 75);
         var contentBox = this.get('contentBox');
         var step_one_content = contentBox.one('.yui3-widget-bd');
         var step_two_content = contentBox.one('.picker-content-two');
         if (step_two_content === null) {
-            var step_two_html = [
-                '<div class="picker-content-two transparent">',
-                '<div class="step-links">',
-                '<a class="prev js-action" href="#">Back</a>',
-                '<button class="next lazr-pos lazr-btn"></button>',
-                '<a class="next js-action" href="#">Select</a>',
-                '</div></div>'
-                ].join(' ');
-            step_two_content = Y.Node.create(step_two_html);
-            var self = this;
-            // Remove the back link if required.
-            if (Y.Lang.isBoolean(data.back_enabled)
-                    && !data.back_enabled ) {
-                step_two_content.one('a.prev').remove(true);
-            } else {
-                step_two_content.one('a.prev').on('click', function(e) {
-                    e.halt();
-                    self._display_step_one();
-                });
-            }
-            // Wire up the next (ie submit) links.
-            step_two_content.all('.next').on('click', function(e) {
-                e.halt();
-                // Only submit if at least one info type is selected.
-                if (!self._all_info_choices_unticked(step_two_content)) {
-                    self.fire('save', data, 2);
-                }
-            });
-            // By default, we only show All or Nothing.
-            var allowed_permissions = ['ALL', 'NOTHING'];
-            if (Y.Lang.isValue(data.allowed_permissions)) {
-                allowed_permissions = data.allowed_permissions;
-            }
-            var sharing_permissions = [];
-            Y.Array.each(this.get('sharing_permissions'),
-                    function(permission) {
-                if (Y.Array.indexOf(
-                        allowed_permissions, permission.value) >=0) {
-                    sharing_permissions.push(permission);
-                }
-            });
-            var policy_selector = self._make_policy_selector(
-                sharing_permissions);
-            step_two_content.one('div.step-links')
-                .insert(policy_selector, 'before');
+            step_two_content = this._render_step_two(
+                data.back_enabled, data.allowed_permissions);
             step_one_content.insert(step_two_content, 'after');
-            step_two_content.all('input[name^=field.permission]')
-                    .on('click', function(e) {
-                self._disable_select_if_all_info_choices_unticked(
-                    step_two_content);
-            });
         }
+        // Wire up the next (ie submit) links.
+        step_two_content.detach('click');
+        step_two_content.delegate('click', function(e) {
+            e.halt();
+            // Only submit if at least one info type is selected.
+            if (!this._all_info_choices_nothing(step_two_content)) {
+                this.fire('save', data, 2);
+            }
+        }, '.next', this);
         // Initially set all radio buttons to Nothing.
         step_two_content.all('input[name^=field.permission][value=NOTHING]')
                 .each(function(radio_button) {
@@ -183,7 +199,7 @@
                 }
             });
         }
-        this._disable_select_if_all_info_choices_unticked(step_two_content);
+        this._disable_select_if_all_info_choices_nothing(step_two_content);
         this._fade_in(step_two_content, step_one_content);
     },
 
@@ -193,7 +209,7 @@
      * @return {Boolean}
      * @private
      */
-    _all_info_choices_unticked: function(content) {
+    _all_info_choices_nothing: function(content) {
         var all_unticked = true;
         content.all('input[name^=field.permission]')
                 .each(function(info_node) {
@@ -209,8 +225,8 @@
      * @param content
      * @private
      */
-    _disable_select_if_all_info_choices_unticked: function(content) {
-        var disable_links = this._all_info_choices_unticked(content);
+    _disable_select_if_all_info_choices_nothing: function(content) {
+        var disable_links = this._all_info_choices_nothing(content);
         content.all('.next').each(function(node) {
             if (disable_links) {
                 node.addClass('invalid-link');
@@ -328,8 +344,12 @@
         }
         switch (config.first_step) {
             case 2:
+                var steptitle = Y.Lang.substitute(
+                    'Update sharing policies for {name}',
+                    {name: config.sharee.person_name});
                 var data = {
-                    title: config.sharee.person_name,
+                    title: 'Update sharing policies',
+                    steptitle: steptitle,
                     api_uri: config.sharee.person_uri,
                     sharee_permissions: config.sharee_permissions,
                     allowed_permissions: config.allowed_permissions,

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareepicker.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareepicker.js	2012-03-21 03:18:56 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareepicker.js	2012-03-27 02:40:57 +0000
@@ -57,7 +57,7 @@
                 anim_duration: 0,
                 progressbar: true,
                 progress: 50,
-                headerContent: "<h2>Grant access</h2>",
+                headerContent: "<h2>Share with a user or team</h2>",
                 steptitle: "Search for user or exclusive team " +
                             "with whom to share",
                 zIndex: 1000,
@@ -96,6 +96,9 @@
             this.picker.set('results', this.vocabulary);
             this.picker.render();
             var cb = this.picker.get('contentBox');
+            Y.Assert.areEqual(
+                'Share with a user or team',
+                this.picker.get('headerContent').get('text'));
             var steptitle = cb.one('.contains-steptitle h2').getContent();
             Y.Assert.areEqual(
                 'Search for user or exclusive team with whom to share',
@@ -110,7 +113,10 @@
             // The first step ui should be hidden.
             Y.Assert.isTrue(cb.one('.yui3-widget-bd').hasClass('unseen'));
             // The step title should be updated according to the selected
-            // person.
+            // person. The title should remain unchanged.
+            Y.Assert.areEqual(
+                'Share with a user or team',
+                this.picker.get('headerContent').get('text'));
             steptitle = cb.one('.contains-steptitle h2').getContent();
             Y.Assert.areEqual(
                 'Select sharing policies for Fred', steptitle);
@@ -172,9 +178,13 @@
                 sharee_permissions: {'P1': 'ALL'}
             });
             var cb = this.picker.get('contentBox');
+            // Check the title and step title are correct.
             var steptitle = cb.one('.contains-steptitle h2').getContent();
             Y.Assert.areEqual(
-                'Select sharing policies for Fred', steptitle);
+                'Update sharing policies',
+                this.picker.get('headerContent').get('text'));
+            Y.Assert.areEqual(
+                'Update sharing policies for Fred', steptitle);
             // By default, selections only for ALL and NOTHING are available
             // (and no others).
             Y.Assert.isNotNull(cb.one('input[value=ALL]'));
@@ -244,7 +254,10 @@
             Y.Assert.areEqual(50, this.picker.get('progress'));
             // The first step ui should be visible.
             Y.Assert.isFalse(cb.one('.yui3-widget-bd').hasClass('unseen'));
-            // The step title should be updated.
+            // The title and step title should be updated.
+            Y.Assert.areEqual(
+                'Share with a user or team',
+                this.picker.get('headerContent').get('text'));
             var steptitle = cb.one('.contains-steptitle h2').getContent();
             Y.Assert.areEqual(
                 'Search for user or exclusive team with whom to share',
@@ -288,6 +301,47 @@
             Y.Assert.areEqual('~/fred', selected_result.api_uri);
         },
 
+        // Test that a new sharee can be selected after click the Back button
+        // and the new sharee is correctly used in the final result.
+        test_sharee_reselection: function() {
+            var selected_result;
+            this.picker = this._create_picker(
+                {
+                    save: function(result) {
+                        selected_result = result;
+                    }
+                }
+            );
+            // Select a person to trigger transition to next step.
+            this.picker.set('results', this.vocabulary);
+            this.picker.render();
+            this.picker.get('boundingBox').one(
+                '.yui3-picker-results li:nth-child(1)').simulate('click');
+            var cb = this.picker.get('contentBox');
+            var step_two_content = cb.one('.picker-content-two');
+            var back_link = step_two_content.one('a.prev');
+            back_link.simulate('click');
+            // Select a different person.
+            this.picker.get('boundingBox').one(
+                '.yui3-picker-results li:nth-child(2)').simulate('click');
+            // Select an access policy.
+            step_two_content
+                .one('input[name=field.permission.P2][value="ALL"]')
+                .simulate('click');
+            var select_link = step_two_content.one('a.next');
+            select_link.simulate('click');
+            // Check the results.
+            Y.Assert.areEqual(
+                3, Y.Object.size(selected_result.selected_permissions));
+            Y.Assert.areEqual(
+                selected_result.selected_permissions.P1, 'NOTHING');
+            Y.Assert.areEqual(
+                selected_result.selected_permissions.P2, 'ALL');
+            Y.Assert.areEqual(
+                selected_result.selected_permissions.P3, 'NOTHING');
+            Y.Assert.areEqual('~/frieda', selected_result.api_uri);
+        },
+
         // When no info types are selected, the Submit links are disabled.
         test_no_selected_info_types: function() {
             var save_called = false;