← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wallyworld/launchpad/more-accesspolicy-service-2 into lp:launchpad

 

Ian Booth has proposed merging lp:~wallyworld/launchpad/more-accesspolicy-service-2 into lp:launchpad with lp:~wallyworld/launchpad/more-accesspolicy-service as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #933792 in Launchpad itself: "+sharing does not show who the project shares with"
  https://bugs.launchpad.net/launchpad/+bug/933792

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/more-accesspolicy-service-2/+merge/95093

== Implementation ==

Part 2 of the work to "wire up" the +sharing view. This branch adds a new addProductObserver() method on the access policy service and hooks in the disclosure person picker so that the selected person and sharing permissions are sent to the server. The observer table is updated by inserting the newly created observer.

Next work will be to make the back end service calls actually do something real.

== Demo and QA ==

Open the +sharing page eg http://launchpad.dev/firefox/+sharing
Use the +Share link to open the disclosure picker and choose someone new to share with.
An XHR call should be made to the access policy service addProductObserver api.
The observer table should be updated with the new observer as the first row.

== Tests ==

Add new yui tests for the ProductSharingView and ObserverTableWidget.
Also a bit of refactoring.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/enums.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/interfaces/accesspolicyservice.py
  lib/lp/registry/javascript/disclosure/disclosure_person_picker.js
  lib/lp/registry/javascript/disclosure/observer_table_widget.js
  lib/lp/registry/javascript/disclosure/product_sharing.js
  lib/lp/registry/javascript/disclosure/tests/test_disclosure_person_picker.js
  lib/lp/registry/javascript/disclosure/tests/test_observer_table_widget.js
  lib/lp/registry/javascript/disclosure/tests/test_product_sharing_view.js
  lib/lp/registry/services/accesspolicyservice.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/more-accesspolicy-service-2/+merge/95093
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/more-accesspolicy-service-2 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-02-29 03:51:19 +0000
@@ -2,7 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for product views."""
-from lp.app.interfaces.services import IService
 
 __metaclass__ = type
 
@@ -15,6 +14,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import ServiceUsage
+from lp.app.interfaces.services import IService
 from lp.registry.browser.product import ProductLicenseMixin
 from lp.registry.interfaces.product import (
     IProductSet,

=== modified file 'lib/lp/registry/enums.py'
--- lib/lp/registry/enums.py	2012-02-21 05:51:06 +0000
+++ lib/lp/registry/enums.py	2012-02-29 03:51:19 +0000
@@ -9,6 +9,7 @@
     'DistroSeriesDifferenceStatus',
     'DistroSeriesDifferenceType',
     'PersonTransferJobType',
+    'SharingPermission',
     ]
 
 from lazr.enum import (
@@ -59,6 +60,31 @@
         """)
 
 
+class SharingPermission(DBEnumeratedType):
+    """Sharing permission.
+
+    The level of access granted for a particular access policy.
+    """
+
+    NOTHING = DBItem(1, """
+        Nothing
+
+        Revoke all bug and branch subscriptions.
+        """)
+
+    ALL = DBItem(2, """
+        All
+
+        Share all bugs and branches.
+        """)
+
+    SOME = DBItem(3, """
+        Some
+
+        Share bug and branch subscriptions.
+        """)
+
+
 class DistroSeriesDifferenceStatus(DBEnumeratedType):
     """Distribution series difference status.
 

=== modified file 'lib/lp/registry/interfaces/accesspolicyservice.py'
--- lib/lp/registry/interfaces/accesspolicyservice.py	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/interfaces/accesspolicyservice.py	2012-02-29 03:51:19 +0000
@@ -10,6 +10,8 @@
     'IAccessPolicyService',
     ]
 
+from zope.schema import Choice
+
 from lazr.restful.declarations import (
     export_as_webservice_entry,
     export_read_operation,
@@ -21,6 +23,10 @@
 
 from lp import _
 from lp.app.interfaces.services import IService
+from lp.registry.enums import (
+    AccessPolicyType,
+    SharingPermission,
+    )
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import IProduct
 
@@ -48,6 +54,17 @@
     @export_write_operation()
     @operation_parameters(
         product=Reference(IProduct, title=_('Product'), required=True),
+        observer=Reference(IPerson, title=_('Observer'), required=True),
+        access_policy=Choice(vocabulary=AccessPolicyType),
+        sharing_permission=Choice(vocabulary=SharingPermission))
+    @operation_for_version('devel')
+    def addProductObserver(product, observer, access_policy,
+                              sharing_permission):
+        """Add an observer with the access policy to a product."""
+
+    @export_write_operation()
+    @operation_parameters(
+        product=Reference(IProduct, title=_('Product'), required=True),
         observer=Reference(IPerson, title=_('Observer'), required=True))
     @operation_for_version('devel')
     def deleteProductObserver(product, observer):

=== modified file 'lib/lp/registry/javascript/disclosure/disclosure_person_picker.js'
--- lib/lp/registry/javascript/disclosure/disclosure_person_picker.js	2012-02-22 05:22:13 +0000
+++ lib/lp/registry/javascript/disclosure/disclosure_person_picker.js	2012-02-29 03:51:19 +0000
@@ -26,15 +26,15 @@
     */
     access_policies: {
         value: []
+    },
+    // Override for testing
+    anim_duration: {
+        value: 1
     }
 };
 
 
 Y.extend(DisclosurePicker, Y.lazr.picker.Picker, {
-
-    // Override for testing
-    _anim_duratrion: 1,
-
     initializer: function(config) {
         DisclosurePicker.superclass.initializer.apply(this, arguments);
         var access_policies = [];
@@ -42,9 +42,6 @@
             if (config.access_policies !== undefined) {
                 access_policies = config.access_policies;
             }
-            if (config.anim_duratrion !== undefined) {
-                this._anim_duratrion = config.anim_duratrion;
-            }
         }
         this.set('access_policies', access_policies);
         var self = this;
@@ -75,7 +72,8 @@
             return;
         }
         old_content.addClass('unseen');
-        if (this._anim_duratrion === 0) {
+        var anim_duratrion = this.get('anim_duration');
+        if (anim_duratrion === 0) {
             return;
         }
         content_node.addClass('transparent');
@@ -83,7 +81,7 @@
         var fade_in = new Y.Anim({
             node: content_node,
             to: {opacity: 1},
-            duration: this._anim_duratrion
+            duration: anim_duratrion
         });
         fade_in.run();
     },
@@ -159,7 +157,7 @@
             '{{#policies}}',
             '    <tr>',
             '      <td rowspan="2"><input type="radio"',
-            '        value="{{value}}"',
+            '        value="{{title}}"',
             '        name="field.visibility"',
             '        id="field.visibility.{{index}}"',
             '        class="radioType">',

=== modified file 'lib/lp/registry/javascript/disclosure/observer_table_widget.js'
--- lib/lp/registry/javascript/disclosure/observer_table_widget.js	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/javascript/disclosure/observer_table_widget.js	2012-02-29 03:51:19 +0000
@@ -26,7 +26,7 @@
 
 ObserverTableWidget.ATTRS = {
     // The duration for various animations eg row deletion.
-    animation_duration: {
+    anim_duration: {
         value: 1
     },
     // The list of observers to display.
@@ -51,6 +51,10 @@
     observer_table_template: {
         value: null
     },
+    // The handlebars template for the observer table rows.
+    observer_row_template: {
+        value: null
+    },
     // The handlebars template for the each access policy item.
     observer_policy_template: {
         value: null
@@ -63,6 +67,8 @@
         this.set(
             'observer_table_template', this._observer_table_template());
         this.set(
+            'observer_row_template', this._observer_row_template());
+        this.set(
             'observer_policy_template', this._observer_policy_template());
         this.publish(REMOVE_OBSERVER);
     },
@@ -87,31 +93,37 @@
             '    </thead>',
             '    <tbody id="observer-table-body">',
             '        {{#observers}}',
-            '        <tr id="permission-{{name}}"><td>',
-            '            <a href="{{web_link}}" class="sprite person">' +
-            '                                  {{display_name}}',
-            '            <span class="formHelp">{{role}}</span></a>',
-            '        </td>',
-            '        <td id="remove-{{name}}">',
-            '            <a title="Share nothing with this user"',
-            '               href="#" class="sprite remove">',
-            '            </a>',
-            '        </td>',
-            '        <td id="td-permission-{{name}}">',
-            '            <span class="sortkey">1</span>',
-            '            <ul class="horizontal">',
-            '               {{>observer_access_policies}}',
-            '            </ul>',
-            '        </td>',
-            '        <td></td>',
-            '        <td><span class="formHelp">No items shared</span>',
-            '        </td>',
-            '        </tr>',
+            '        {{>observer_row}}',
             '        {{/observers}}',
             '    </tbody>',
             '</table>'].join(' ');
     },
 
+    _observer_row_template: function() {
+        return [
+            '<tr id="permission-{{name}}"><td>',
+            '    <a href="{{web_link}}" class="sprite person">' +
+            '                          {{display_name}}',
+            '    <span class="formHelp">{{role}}</span></a>',
+            '</td>',
+            '<td id="remove-{{name}}">',
+            '    <a title="Share nothing with this user"',
+            '       href="#" class="sprite remove"' +
+            '        data-self_link="{{self_link}}">',
+            '    </a>',
+            '</td>',
+            '<td id="td-permission-{{name}}">',
+            '    <span class="sortkey">1</span>',
+            '    <ul class="horizontal">',
+            '       {{>observer_access_policies}}',
+            '    </ul>',
+            '</td>',
+            '<td></td>',
+            '<td><span class="formHelp">No items shared</span>',
+            '</td>',
+            '</tr>'].join(' ');
+    },
+
     _observer_policy_template: function() {
         return [
            '{{#access_policies}}',
@@ -127,7 +139,7 @@
     // Render the popup widget to pick the sharing permission for an
     // access policy.
     renderObserverPolicy: function(
-            table_node, observer, policy, current_value) {
+            observer, policy, current_value) {
         var access_policy_types = this.get('access_policy_types');
         var sharing_permissions = this.get('sharing_permissions');
         var choice_items = [];
@@ -136,15 +148,15 @@
                 '<strong>{policy_name}:</strong> {permission_name}';
             choice_items.push({
                 value: permission.value,
-                name: permission.name,
+                name: permission.title,
                 source_name: Y.Lang.substitute(source_name,
                     {policy_name: access_policy_types[policy],
-                     permission_name: permission.name})
+                     permission_name: permission.title})
             });
         });
 
         var id = 'permission-'+observer.name;
-        var observer_row = table_node.one('#' + id);
+        var observer_row = this.get('observer_table').one('#' + id);
         var permission_node = observer_row.one('#td-' + id);
         var contentBox = permission_node.one('#' + policy + '-permission');
         var value_location = contentBox.one('.value');
@@ -165,67 +177,98 @@
     },
 
     // Render the access policy values for the observers.
-    renderSharingInfo: function(table_node) {
+    renderSharingInfo: function() {
         var observers = this.get('observers');
         var self = this;
         Y.Array.forEach(observers, function(observer) {
-            var observer_policies = observer.permissions;
-            var policy;
-            for (policy in observer_policies) {
-                if (observer_policies.hasOwnProperty(policy)) {
-                    self.renderObserverPolicy(
-                        table_node, observer, policy,
-                        observer_policies[policy]);
-                }
+            self.renderObserverSharingInfo(observer);
+        });
+    },
+
+    // Render the access policy values for an observer.
+    renderObserverSharingInfo: function(observer) {
+        var observer_policies = observer.permissions;
+        var policy;
+        for (policy in observer_policies) {
+            if (observer_policies.hasOwnProperty(policy)) {
+                this.renderObserverPolicy(
+                    observer, policy, observer_policies[policy]);
             }
-        });
+        }
     },
 
     renderUI: function() {
         var partials = {
             observer_access_policies:
-                this.get('observer_policy_template')
+                this.get('observer_policy_template'),
+            observer_row: this.get('observer_row_template')
         };
         var observers = this.get('observers');
-        Y.Array.forEach(observers, function(observer) {
-            var observer_policies = observer.permissions;
-            var policy_values = [];
-            var policy;
-            for (policy in observer_policies) {
-                if (observer_policies.hasOwnProperty(policy)) {
-                    var policy_value = {policy: policy};
-                    policy_values.push(policy_value);
-                }
-            }
-            observer.access_policies = policy_values;
-        });
-
+        this._prepareObserverDisplayData(observers);
         var html = Y.lp.mustache.to_html(
             this.get('observer_table_template'),
             {observers: observers}, partials);
         var table_node = Y.Node.create(html);
         this.get('observer_table').replace(table_node);
-        this.renderSharingInfo(table_node);
+        this.renderSharingInfo();
     },
 
     bindUI: function() {
-        var observers = this.get('observers');
-
+        // Bind the delete observer links.
         var self = this;
-        // Bind the delete observer link.
-        Y.Array.forEach(observers, function(observer) {
-            var link_id = 'remove-' + observer.name;
-            var delete_link = self.get('observer_table')
-                .one('td#' + link_id + ' a');
-            delete_link.on('click', function(e) {
-                e.preventDefault();
-                self.fire(REMOVE_OBSERVER, delete_link, observer.self_link);
-            });
-        });
+        this.get('observer_table').delegate('click', function(e) {
+            e.preventDefault();
+            var delete_link = e.currentTarget;
+            var observer_link = delete_link.getAttribute('data-self_link');
+            self.fire(REMOVE_OBSERVER, delete_link, observer_link);
+        }, 'td[id^=remove-] a');
     },
 
     syncUI: function() { },
 
+    // Transform the observer access policy data from the model into a form
+    // that can be used with the handlebars template.
+    _prepareObserverDisplayData: function(observers) {
+        Y.Array.forEach(observers, function(observer) {
+            var observer_policies = observer.permissions;
+            var policy_values = [];
+            var policy;
+            for (policy in observer_policies) {
+                if (observer_policies.hasOwnProperty(policy)) {
+                    var policy_value = {policy: policy};
+                    policy_values.push(policy_value);
+                }
+            }
+            observer.access_policies = policy_values;
+        });
+    },
+
+    // Add a new observer to the table.
+    addObserver: function(observer) {
+        this._prepareObserverDisplayData([observer]);
+        var partials = {
+            observer_access_policies:
+                this.get('observer_policy_template')
+        };
+        var row_html = Y.lp.mustache.to_html(
+            this.get('observer_row_template'), observer, partials);
+        var new_table_row = Y.Node.create(row_html);
+        var first_row = this.get('observer_table')
+            .one('tbody>:first-child');
+        var row_node;
+        if (Y.Lang.isValue(first_row)) {
+            row_node = first_row.insertBefore(new_table_row, first_row);
+        } else {
+            row_node = this.get('observer_table').one('tbody')
+                .appendChild(new_table_row);
+        }
+        this.renderObserverSharingInfo(observer);
+        var anim_duration = this.get('anim_duration');
+        var anim = Y.lp.anim.green_flash(
+            {node: row_node, duration:anim_duration});
+        anim.run();
+    },
+
     // Delete the specified observer from the table.
     deleteObserver: function(observer) {
         var table_row = this.get('observer_table')
@@ -233,7 +276,7 @@
         if (!Y.Lang.isValue(table_row)) {
             return;
         }
-        var anim_duration = this.get('animation_duration');
+        var anim_duration = this.get('anim_duration');
         if (anim_duration === 0 ) {
             table_row.remove(true);
             return;
@@ -252,6 +295,6 @@
 namespace.ObserverTableWidget = ObserverTableWidget;
 
 }, "0.1", { "requires": [
-    'node', 'collection', 'lazr.choiceedit',
+    'node', 'event', 'collection', 'lazr.choiceedit',
     'lp.mustache', 'lp.registry.disclosure'] });
 

=== modified file 'lib/lp/registry/javascript/disclosure/product_sharing.js'
--- lib/lp/registry/javascript/disclosure/product_sharing.js	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/javascript/disclosure/product_sharing.js	2012-02-29 03:51:19 +0000
@@ -43,6 +43,7 @@
         } else {
             config = {};
         }
+        var self = this;
         var new_config = Y.merge(config, {
             align: {
                 points: [Y.WidgetPositionAlign.CC,
@@ -54,7 +55,9 @@
             zIndex: 1000,
             visible: false,
             access_policies: LP.cache.access_policies,
-            save: this.saveSharingSelection
+            save: function(result) {
+                self.saveSharingSelection(result);
+            }
         });
         var disclosure_picker =
             new Y.lp.registry.disclosure.DisclosurePicker(new_config);
@@ -93,8 +96,7 @@
         var otns = Y.lp.registry.disclosure.observertable;
         observer_table.subscribe(
             otns.ObserverTableWidget.REMOVE_OBSERVER, function(e) {
-                self.performRemoveObserver(
-                    observer_table, e.details[0], e.details[1]);
+                self.performRemoveObserver(e.details[0], e.details[1]);
         });
     },
 
@@ -127,7 +129,14 @@
         }
     },
 
-    removeObserverSuccess: function(observer_table, person_uri) {
+    /**
+     * The server call to remove the specified observer has succeeded.
+     * Update the model and view.
+     * @method performRemoveObserver
+     * @param person_uri
+     */
+    removeObserverSuccess: function(person_uri) {
+        var observer_table = this.get('observer_table');
         var observer_data = LP.cache.observer_data;
         Y.Array.some(observer_data, function(observer, index) {
             if (observer.self_link === person_uri) {
@@ -138,7 +147,13 @@
         });
     },
 
-    performRemoveObserver: function(observer_table, delete_link, person_uri) {
+    /**
+     * Make a server call to remove the specified observer.
+     * @method performRemoveObserver
+     * @param delete_link
+     * @param person_uri
+     */
+    performRemoveObserver: function(delete_link, person_uri) {
         var error_handler = new Y.lp.client.ErrorHandler();
         var product_uri = LP.cache.context.self_link;
         var self = this;
@@ -147,7 +162,7 @@
                 start: Y.bind(self._showDeleteSpinner, namespace, delete_link),
                 end: Y.bind(self._hideDeleteSpinner, namespace, delete_link),
                 success: function() {
-                    self.removeObserverSuccess(observer_table, person_uri);
+                    self.removeObserverSuccess(person_uri);
                 },
                 failure: error_handler.getFailureHandler()
             },
@@ -160,9 +175,72 @@
             '/+services/accesspolicy', 'deleteProductObserver', y_config);
     },
 
-    saveSharingSelection: function(result) {
-        Y.log(result.access_policy);
-        Y.log(result.api_uri);
+    /**
+     * Show a spinner for a sharing update operation.
+     *
+     * @method _showSharingSpinner
+     */
+    _showSharingSpinner: function() {
+        var spinner_node = Y.Node.create(
+        '<img class="spinner" src="/@@/spinner" alt="Saving..." />');
+        var sharing_header = Y.one('#observer-table th:nth-child(2)');
+        sharing_header.appendChild(spinner_node, sharing_header);
+    },
+
+    /**
+     * Hide the sharing spinner.
+     *
+     * @method _hideSharingSpinner
+     */
+    _hideSharingSpinner: function() {
+        var spinner = Y.one('#observer-table th .spinner');
+        if (spinner !== null) {
+            spinner.remove();
+        }
+    },
+
+    /**
+     * The server call to add the specified observer has succeeded.
+     * Update the model and view.
+     * @method saveSharingSelectionSuccess
+     * @param observer
+     */
+    saveSharingSelectionSuccess: function(observer) {
+        var observer_table = this.get('observer_table');
+        var observer_data = LP.cache.observer_data;
+        observer_data.splice(0, 0, observer);
+        observer_table.addObserver(observer);
+    },
+
+    /**
+     * Make a server call to add the specified observer and access policy.
+     * @method performRemoveObserver
+     * @param selection_result the disclosure picker
+     */
+    saveSharingSelection: function(selection_result) {
+        var error_handler = new Y.lp.client.ErrorHandler();
+        var product_uri = LP.cache.context.self_link;
+        var person_uri = Y.lp.client.normalize_uri(selection_result.api_uri);
+        person_uri = Y.lp.client.get_absolute_uri(person_uri);
+        var self = this;
+        var y_config =  {
+            on: {
+                start: Y.bind(self._showSharingSpinner, namespace),
+                end: Y.bind(self._hideSharingSpinner, namespace),
+                success: function(observer_entry) {
+                    self.saveSharingSelectionSuccess(observer_entry.getAttrs());
+                },
+                failure: error_handler.getFailureHandler()
+            },
+            parameters: {
+                product: product_uri,
+                observer: person_uri,
+                access_policy: selection_result.access_policy,
+                sharing_permission: 'All'
+            }
+        };
+        this.get('lp_client').named_post(
+            '/+services/accesspolicy', 'addProductObserver', y_config);
     }
 });
 

=== modified file 'lib/lp/registry/javascript/disclosure/tests/test_disclosure_person_picker.js'
--- lib/lp/registry/javascript/disclosure/tests/test_disclosure_person_picker.js	2012-02-22 05:22:13 +0000
+++ lib/lp/registry/javascript/disclosure/tests/test_disclosure_person_picker.js	2012-02-29 03:51:19 +0000
@@ -109,7 +109,7 @@
             // policy type.
             Y.Array.each(this.access_policies, function(policy) {
                 var rb = step_two_content.one(
-                    'input[value=' + policy.value + ']');
+                    'input[value="' + policy.title + '"]');
                 Y.Assert.isNotNull(rb);
             });
             // The first policy button should be selected.
@@ -167,10 +167,10 @@
             var cb = this.picker.get('contentBox');
             var step_two_content = cb.one('.picker-content-two');
             // Select an access policy.
-            step_two_content.one('input[value=P2]').simulate('click');
+            step_two_content.one('input[value="Policy 2"]').simulate('click');
             var select_link = step_two_content.one('a.next');
             select_link.simulate('click');
-            Y.Assert.areEqual('P2', selected_result.access_policy);
+            Y.Assert.areEqual('Policy 2', selected_result.access_policy);
             Y.Assert.areEqual('~/fred', selected_result.api_uri);
         }
     }));

=== modified file 'lib/lp/registry/javascript/disclosure/tests/test_observer_table_widget.js'
--- lib/lp/registry/javascript/disclosure/tests/test_observer_table_widget.js	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/javascript/disclosure/tests/test_observer_table_widget.js	2012-02-29 03:51:19 +0000
@@ -20,10 +20,10 @@
             'permissions': {'P1': 's1', 'P3': 's3'}}
             ];
             this.sharing_permissions = [
-                {'value': 's1', 'name': 'S1',
-                 'title': 'Sharing 1'},
-                {'value': 's2', 'name': 'S2',
-                 'title': 'Sharing 2'}
+                {'value': 's1', 'title': 'S1',
+                 'description': 'Sharing 1'},
+                {'value': 's2', 'title': 'S2',
+                 'description': 'Sharing 2'}
             ];
             this.access_policies = {
                 'P1': 'Policy 1',
@@ -39,7 +39,7 @@
         _create_Widget: function() {
             var ns = Y.lp.registry.disclosure.observertable;
             return new ns.ObserverTableWidget({
-                animation_duration: 0.001,
+                anim_duration: 0.001,
                 observers: this.observer_data,
                 sharing_permissions: this.sharing_permissions,
                 access_policy_types: this.access_policies
@@ -60,33 +60,56 @@
                 "Observer table failed to be instantiated");
         },
 
+        // The given observer is correctly rendered.
+        _test_observer_rendered: function(observer) {
+            // The observer row
+            Y.Assert.isNotNull(
+                Y.one('#observer-table tr[id=permission-'
+                      + observer.name + ']'));
+            // The delete link
+            Y.Assert.isNotNull(
+                Y.one('#observer-table td[id=remove-'
+                      + observer.name + '] a'));
+            // The access policy sharing permissions
+            var permission;
+            for (permission in observer.permissions) {
+                if (observer.permissions.hasOwnProperty(permission)) {
+                    Y.Assert.isNotNull(
+                        Y.one('#observer-table td[id=td-permission-'
+                              + observer.name + '] ul li '
+                              + 'span[id='+permission+'-permission] '
+                              + 'span.value'));
+                }
+            }
+        },
+
         // The observer table is correctly rendered.
         test_render: function() {
             this.observer_table = this._create_Widget();
             this.observer_table.render();
+            var self = this;
             Y.Array.each(this.observer_data, function(observer) {
-                // The observer row
-                Y.Assert.isNotNull(
-                    Y.one('#observer-table tr[id=permission-'
-                          + observer.name + ']'));
-                // The delete link
-                Y.Assert.isNotNull(
-                    Y.one('#observer-table td[id=remove-'
-                          + observer.name + '] a'));
-                // The access policy sharing permissions
-                var permission;
-                for (permission in observer.permissions) {
-                    if (observer.permissions.hasOwnProperty(permission)) {
-                        Y.Assert.isNotNull(
-                            Y.one('#observer-table td[id=td-permission-'
-                                  + observer.name + '] ul li '
-                                  + 'span[id='+permission+'-permission] '
-                                  + 'span.value'));
-                    }
-                }
+                self._test_observer_rendered(observer);
             });
         },
 
+        // The addObserver call adds the observer to the table.
+        test_observer_add: function() {
+            this.observer_table = this._create_Widget();
+            this.observer_table.render();
+            var new_observer = {
+                'name': 'joe', 'display_name': 'Joe Smith',
+                'role': '(Maintainer)', web_link: '~joe',
+                'self_link': '~joe',
+                'permissions': {'P1': 's2'}};
+            this.observer_table.addObserver(new_observer);
+            var self = this;
+            this.wait(function() {
+                    self._test_observer_rendered(new_observer);
+                }, 60
+            );
+        },
+
         // When the delete link is clicked, the correct event is published.
         test_observer_delete_click: function() {
             this.observer_table = this._create_Widget();

=== modified file 'lib/lp/registry/javascript/disclosure/tests/test_product_sharing_view.js'
--- lib/lp/registry/javascript/disclosure/tests/test_product_sharing_view.js	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/javascript/disclosure/tests/test_product_sharing_view.js	2012-02-29 03:51:19 +0000
@@ -26,16 +26,19 @@
                     'permissions': {'P1': 's1', 'P3': 's3'}}
                     ],
                     sharing_permissions: [
-                        {'value': 's1', 'name': 'S1',
-                         'title': 'Sharing 1'},
-                        {'value': 's2', 'name': 'S2',
-                         'title': 'Sharing 2'}
+                        {'value': 's1', 'title': 'S1',
+                         'description': 'Sharing 1'},
+                        {'value': 's2', 'title': 'S2',
+                         'description': 'Sharing 2'}
                     ],
-                    access_policies: {
-                        'P1': 'Policy 1',
-                        'P2': 'Policy 2',
-                        'P3': 'Policy 3'
-                    }
+                    access_policies: [
+                        {index: '0', value: 'P1', title: 'Policy 1',
+                         description: 'Policy 1 description'},
+                        {index: '1', value: 'P2', title: 'Policy 2',
+                         description: 'Policy 2 description'},
+                        {index: '2', value: 'P3', title: 'Policy 3',
+                         description: 'Policy 3 description'}
+                    ]
                 }
             };
         },
@@ -93,9 +96,7 @@
             var performRemove_called = false;
             var self = this;
             this.view.performRemoveObserver = function(
-                observer_table, delete_link, person_uri) {
-                Y.Assert.areEqual(
-                    self.view.get('observer_table'), observer_table);
+                    delete_link, person_uri) {
                 Y.Assert.areEqual('~fred', person_uri);
                 Y.Assert.areEqual(delete_link_to_click, delete_link);
                 performRemove_called = true;
@@ -119,18 +120,13 @@
             this.view.render();
             var removeObserverSuccess_called = false;
             var self = this;
-            this.view.removeObserverSuccess = function(
-                    observer_table, person_uri) {
-                Y.Assert.areEqual(
-                    self.view.get('observer_table'), observer_table);
+            this.view.removeObserverSuccess = function(person_uri) {
                 Y.Assert.areEqual('~fred', person_uri);
                 removeObserverSuccess_called = true;
             };
             var delete_link =
                 Y.one('#observer-table td[id=remove-fred] a');
-            this.view.performRemoveObserver(
-                this.view.get('observer_table'),
-                delete_link, '~fred');
+            this.view.performRemoveObserver(delete_link, '~fred');
             Y.Assert.areEqual(
                 '/api/devel/+services/accesspolicy',
                 mockio.last_request.url);
@@ -138,13 +134,13 @@
                 'ws.op=deleteProductObserver&product=~product' +
                     '&observer=~fred',
                 mockio.last_request.config.data);
-            mockio.last_request.successJSON('');
+            mockio.last_request.successJSON({});
             Y.Assert.isTrue(removeObserverSuccess_called);
         },
 
         // The removeObserver callback updates the model and table.
         test_removeObserverSuccess: function() {
-            this.view = this._create_Widget({animation_duration: 0.001});
+            this.view = this._create_Widget({anim_duration: 0.001});
             this.view.render();
             var deleteObserver_called = false;
             var expected_observer = window.LP.cache.observer_data[0];
@@ -153,12 +149,93 @@
                 Y.Assert.areEqual(expected_observer, observer);
                 deleteObserver_called = true;
             };
-            this.view.removeObserverSuccess(observer_table, '~fred');
+            this.view.removeObserverSuccess('~fred');
             Y.Assert.isTrue(deleteObserver_called);
             Y.Array.each(window.LP.cache.observer_data,
                 function(observer) {
                     Y.Assert.areNotEqual('fred', observer.name);
             });
+        },
+
+        // The performAddObserver method makes the expected XHR calls.
+        test_performAddObserver: function() {
+            var mockio = new Y.lp.testing.mockio.MockIo();
+            var lp_client = new Y.lp.client.Launchpad({
+                io_provider: mockio
+            });
+            this.view = this._create_Widget({
+                lp_client: lp_client,
+                anim_duration: 0
+            });
+            this.view.render();
+            var saveSharingSelectionSuccess_called = false;
+            var self = this;
+            this.view.saveSharingSelectionSuccess = function(observer) {
+                Y.Assert.areEqual('joe', observer.name);
+                saveSharingSelectionSuccess_called = true;
+            };
+            // Use the picker to select a new observer and access policy.
+            var disclosure_picker = this.view.get('disclosure_picker');
+            var picker_results = [
+                {"value": "joe", "title": "Joe", "css": "sprite-person",
+                    "description": "joe@xxxxxxxxxxx", "api_uri": "~/joe",
+                    "metadata": "person"}];
+            Y.one('#add-observer-link').simulate('click');
+            disclosure_picker.set('results', picker_results);
+            disclosure_picker.get('boundingBox').one(
+                '.yui3-picker-results li:nth-child(1)').simulate('click');
+            var cb = disclosure_picker.get('contentBox');
+            var step_two_content = cb.one('.picker-content-two');
+            step_two_content.one('input[value="Policy 2"]').simulate('click');
+            var select_link = step_two_content.one('a.next');
+            select_link.simulate('click');
+            // Selection made using the picker, now check the results.
+            Y.Assert.areEqual(
+                '/api/devel/+services/accesspolicy',
+                mockio.last_request.url);
+            var person_uri = Y.lp.client.normalize_uri('~/joe');
+            person_uri = Y.lp.client.get_absolute_uri(person_uri);
+            var expected_url;
+            expected_url = Y.lp.client.append_qs(
+                expected_url, 'ws.op', 'addProductObserver');
+            expected_url = Y.lp.client.append_qs(
+                expected_url, 'product', '~product');
+            expected_url = Y.lp.client.append_qs(
+                expected_url, 'observer', person_uri);
+            expected_url = Y.lp.client.append_qs(
+                expected_url, 'access_policy', 'Policy 2');
+            expected_url = Y.lp.client.append_qs(
+                expected_url, 'sharing_permission', 'All');
+            Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
+            mockio.last_request.successJSON({
+                'resource_type_link': 'entity',
+                'name': 'joe',
+                'self_link': '~joe'});
+            Y.Assert.isTrue(saveSharingSelectionSuccess_called);
+        },
+
+        // The saveSharingSelectionSuccess callback updates the model and table.
+        test_saveSharingSelectionSuccess: function() {
+            this.view = this._create_Widget({anim_duration: 0.001});
+            this.view.render();
+            var addObserver_called = false;
+            var new_observer = {
+                'name': 'joe'
+            };
+            var observer_table = this.view.get('observer_table');
+            observer_table.addObserver = function(observer) {
+                Y.Assert.areEqual(new_observer, observer);
+                addObserver_called = true;
+            };
+            this.view.saveSharingSelectionSuccess(new_observer);
+            Y.Assert.isTrue(addObserver_called);
+            var model_updated = false;
+            Y.Array.some(window.LP.cache.observer_data,
+                function(observer) {
+                    model_updated = 'joe' === observer.name;
+                    return model_updated;
+            });
+            Y.Assert.isTrue(model_updated);
         }
     }));
 

=== modified file 'lib/lp/registry/services/accesspolicyservice.py'
--- lib/lp/registry/services/accesspolicyservice.py	2012-02-29 03:51:19 +0000
+++ lib/lp/registry/services/accesspolicyservice.py	2012-02-29 03:51:19 +0000
@@ -14,7 +14,10 @@
 from zope.component import getUtility
 from zope.interface import implements
 
-from lp.registry.enums import AccessPolicyType
+from lp.registry.enums import (
+    AccessPolicyType,
+    SharingPermission,
+    )
 from lp.registry.interfaces.accesspolicyservice import IAccessPolicyService
 from lp.registry.interfaces.person import IPersonSet
 
@@ -48,15 +51,14 @@
 
     def getSharingPermissions(self):
         """See `IAccessPolicyService`."""
-        # TODO - use proper model class
-        sharing_permissions = [
-            {'value': 'all', 'name': 'All',
-             'title': 'share bug and branch subscriptions'},
-            {'value': 'some', 'name': 'Some',
-             'title': 'share bug and branch subscriptions'},
-            {'value': 'nothing', 'name': 'Nothing',
-             'title': 'revoke all bug and branch subscriptions'}
-        ]
+        sharing_permissions = []
+        for permission in SharingPermission:
+            item = dict(
+                value=permission.token,
+                title=permission.title,
+                description=permission.value.description
+            )
+            sharing_permissions.append(item)
         return sharing_permissions
 
     def getProductObservers(self, product):
@@ -70,15 +72,28 @@
             resource = EntryResource(person, request)
             person_data = resource.toDataForJSON()
             permissions = {
-                'PUBLICSECURITY': 'some',
-                'EMBARGOEDSECURITY': 'all'
+                'PUBLICSECURITY': 'SOME',
+                'EMBARGOEDSECURITY': 'ALL'
             }
             if id > 2:
-                permissions['USERDATA'] = 'some'
+                permissions['USERDATA'] = 'SOME'
             person_data['permissions'] = permissions
             result.append(person_data)
         return result
 
+    def addProductObserver(self, product, observer, access_policy,
+                              sharing_permission):
+        """See `IAccessPolicyService`."""
+        # TODO - implement this
+        request = get_current_web_service_request()
+        resource = EntryResource(observer, request)
+        person_data = resource.toDataForJSON()
+        permissions = {
+            access_policy.name: sharing_permission.name,
+        }
+        person_data['permissions'] = permissions
+        return person_data
+
     def deleteProductObserver(self, product, observer):
         """See `IAccessPolicyService`."""
         # TODO - implement this