launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11585
[Merge] lp:~wallyworld/launchpad/sharing-policies-ajax-update into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-policies-ajax-update into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1043368 in Launchpad itself: "SharingService.getInformationTypes must respect sharing policies"
https://bugs.launchpad.net/launchpad/+bug/1043368
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-policies-ajax-update/+merge/122603
== Implementation ==
When the +sharing view is used to update a bug or branch sharing policy, the allowed information types which can be shared may change. This has a number of significant flow on effects on the underlying view model and the internals of the various widgets used. I tried to dynamically update the view to account for these changes but it quickly got messy, especially if batching is involved in the grantee rendering. So the easiest approach is simply to reload the entire page when a sharing policy is updated.
The sharing policies were updated using a lp client patch operation. I changed this to a new exported sharing service method updatePillarSharingPolicies. This makes everything consistent in the underlying implementation, including permission checks and api usage.
The TAL used for the +sharing page was also improved to properly render the sharing policy data when the page is first loaded. This avoids the nasty flicker as the javascript creates the widgets and makes the page refresh after saving the sharing policies almost unnoticeable.
== Tests ==
Update pillarsharingview yui tests
Add tests for new sharing service updatePillarSharingPolicies method
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/javascript/sharing/pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
lib/lp/registry/templates/pillar-sharing.pt
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-policies-ajax-update/+merge/122603
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-policies-ajax-update into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-09-03 11:22:25 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-09-04 00:12:21 +0000
@@ -31,6 +31,8 @@
from lp.bugs.interfaces.bug import IBug
from lp.code.interfaces.branch import IBranch
from lp.registry.enums import (
+ BranchSharingPolicy,
+ BugSharingPolicy,
InformationType,
SharingPermission,
)
@@ -239,3 +241,18 @@
:param bugs: the bugs for which to grant access
:param branches: the branches for which to grant access
"""
+
+ @export_write_operation()
+ @operation_parameters(
+ pillar=Reference(IPillar, title=_('Pillar'), required=True),
+ branch_sharing_policy=Choice(vocabulary=BranchSharingPolicy),
+ bug_sharing_policy=Choice(vocabulary=BugSharingPolicy))
+ @operation_for_version('devel')
+ def updatePillarSharingPolicies(pillar, branch_sharing_policy=None,
+ bug_sharing_policy=None):
+ """Update the sharing policies for a pillar.
+
+ :param pillar: the pillar to update
+ :param branch_sharing_policy: the new branch sharing policy
+ :param bug_sharing_policy: the new bug sharing policy
+ """
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-08-30 02:52:33 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-09-04 00:12:21 +0000
@@ -303,6 +303,8 @@
var value_key = artifact_type + '_sharing_policy';
var error_handler = new Y.lp.client.ErrorHandler();
error_handler.showError = function(error_msg) {
+ actionicon.addClass('edit');
+ actionicon.removeClass('spinner');
var portlet_id = '#' + artifact_type + 'sharing-policy-portlet';
Y.lp.app.errors.display_error(
Y.one(portlet_id), error_msg);
@@ -321,53 +323,30 @@
this._update_policy_portlet(artifact_type, value);
var pillar_uri = LP.cache.context.self_link;
var actionicon = widget.get("actionicon");
+ var parameters = {
+ pillar: pillar_uri};
+ parameters[value_key] = value;
var y_config = {
on: {
start: function() {
actionicon.removeClass('edit');
actionicon.addClass('spinner');
},
- end: function() {
- actionicon.addClass('edit');
- actionicon.removeClass('spinner');
- },
success: function() {
- self.save_sharing_policy_success(
- widget, artifact_type, value);
+ self._reload();
},
failure: error_handler.getFailureHandler()
},
- parameters: {
- branch_sharing_policy: value
- }
+ parameters: parameters
};
- var patch_data = {};
- patch_data[value_key] = value;
- this.get('lp_client').patch(pillar_uri, patch_data, y_config);
+ this.get('lp_client').named_post(
+ '/+services/sharing', 'updatePillarSharingPolicies',
+ y_config);
},
- /**
- * The sharing policy save operation has succeeded.
- * @param widget
- * @param artifact_type
- * @param value
- */
- save_sharing_policy_success: function(widget, artifact_type, value) {
- var value_key = artifact_type + '_sharing_policy';
- LP.cache.context[value_key] = value;
- widget._showSucceeded();
- // Update the popup widget - we may need to hide the edit link.
- var choice_items = this.getSharingPolicyInformation(artifact_type);
- widget.set('items', choice_items);
- if (choice_items.length <= 1) {
- var sharing_policy_row = Y.one(
- '#' + artifact_type + '-sharing-policy-row');
- var contentBox = sharing_policy_row.one(
- '#' + artifact_type + '-sharing-policy');
- var editicon = contentBox.one('a.editicon');
- editicon.addClass('hidden');
- widget.set('clickable_content', false);
- }
+ // Override for testing.
+ _reload: function() {
+ window.location.reload();
},
// A common error handler for XHR operations.
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-08-31 04:34:32 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-09-04 00:12:21 +0000
@@ -55,7 +55,7 @@
{index: '0', value: 'BRSP1',
title: 'Branch Policy 1',
description: 'Branch Policy 1 description'}
- ],
+ ]
}
};
this.mockio = new Y.lp.testing.mockio.MockIo();
@@ -556,11 +556,11 @@
'a[href=#' + title + ' Policy 1]');
permission_choice.simulate('click');
Y.Assert.areEqual(
- '/api/devel/~pillar',
+ '/api/devel/+services/sharing',
this.mockio.last_request.url);
Y.Assert.areEqual(
- '{"' + artifact_type + '_sharing_policy":"' +
- title + ' Policy 1"}',
+ 'ws.op=updatePillarSharingPolicies&pillar=~pillar&' +
+ artifact_type + '_sharing_policy=' + title + '%20Policy%201',
this.mockio.last_request.config.data);
},
@@ -578,36 +578,18 @@
this.view = this._create_Widget();
this.view.render();
this._assert_sharing_policy_save('bug', 'Bug');
- var data = {
- resource_type_link: 'product',
- self_link: 'api/devel/~pillar',
- bug_sharing_policy: 'Bug Policy 1'};
- this.mockio.last_request.successJSON(data);
- // Check the cached context.
- Y.Assert.areEqual(
- window.LP.cache.context.bug_sharing_policy,
- 'Bug Policy 1');
- // Check the portlet.
- Y.Assert.areEqual(
- 'Bug Policy 1',
- Y.one('#bug-sharing-policy .value').get('text'));
- Y.Assert.areEqual(
- 'Bug Policy 1 description',
- Y.one('#bug-sharing-policy-description').get('text'));
+ var reload_called = false;
+ this.view._reload = function() {
+ reload_called = true;
+ };
+ this.mockio.last_request.successJSON({});
+ Y.Assert.isTrue(reload_called);
},
test_bug_sharing_policy_save_success: function() {
this._assert_bug_sharing_policy_save_success();
},
- // There is more than one policy choice available for selection so
- // the edit link is visible.
- test_bug_policy_edit_link_visible_after_save: function() {
- this._assert_bug_sharing_policy_save_success();
- var permission_choice = Y.one('#bug-sharing-policy a.edit');
- Y.Assert.isFalse(permission_choice.hasClass('hidden'));
- },
-
// When a failure occurs, the client retains the existing data.
test_bug_sharing_policy_save_failure: function() {
window.LP.cache.context.bug_sharing_policy = null;
@@ -640,36 +622,18 @@
this.view = this._create_Widget();
this.view.render();
this._assert_sharing_policy_save('branch', 'Branch');
- var data = {
- resource_type_link: 'product',
- self_link: 'api/devel/~pillar',
- branch_sharing_policy: 'Branch Policy 1'};
- this.mockio.last_request.successJSON(data);
- // Check the cached context.
- Y.Assert.areEqual(
- window.LP.cache.context.branch_sharing_policy,
- 'Branch Policy 1');
- // Check the portlet.
- Y.Assert.areEqual(
- 'Branch Policy 1',
- Y.one('#branch-sharing-policy .value').get('text'));
- Y.Assert.areEqual(
- 'Branch Policy 1 description',
- Y.one('#branch-sharing-policy-description').get('text'));
+ var reload_called = false;
+ this.view._reload = function() {
+ reload_called = true;
+ };
+ this.mockio.last_request.successJSON({});
+ Y.Assert.isTrue(reload_called);
},
test_branch_sharing_policy_save_success: function() {
this._assert_branch_sharing_policy_save_success();
},
- // There is more than one policy choice available for selection so
- // the edit link is visible.
- test_branch_policy_edit_link_visible_after_save: function() {
- this._assert_branch_sharing_policy_save_success();
- var permission_choice = Y.one('#branch-sharing-policy a.edit');
- Y.Assert.isTrue(permission_choice.hasClass('hidden'));
- },
-
// When a failure occurs, the client retains the existing data.
test_branch_sharing_policy_save_failure: function() {
this.view = this._create_Widget();
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-09-03 11:22:25 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-09-04 00:12:21 +0000
@@ -542,3 +542,17 @@
missing_artifacts = set(artifacts) - set(artifacts_with_grants)
getUtility(IAccessArtifactGrantSource).grant(
list(product(missing_artifacts, grantees, [user])))
+
+ @available_with_permission('launchpad.Edit', 'pillar')
+ def updatePillarSharingPolicies(self, pillar, branch_sharing_policy=None,
+ bug_sharing_policy=None):
+ if not branch_sharing_policy and not bug_sharing_policy:
+ return None
+ # Only Products have sharing policies.
+ if not IProduct.providedBy(pillar):
+ raise ValueError(
+ "Sharing policies are only supported for products.")
+ if branch_sharing_policy:
+ pillar.setBranchSharingPolicy(branch_sharing_policy)
+ if bug_sharing_policy:
+ pillar.setBugSharingPolicy(bug_sharing_policy)
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-09-03 11:22:25 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-09-04 00:12:21 +0000
@@ -1098,6 +1098,49 @@
login_person(anyone)
self._assert_ensureAccessGrantsUnauthorized(anyone)
+ def test_updatePillarBugSharingPolicy(self):
+ # updatePillarSharingPolicies works for bugs.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ self.factory.makeCommercialSubscription(product)
+ login_person(owner)
+ self.service.updatePillarSharingPolicies(
+ product,
+ bug_sharing_policy=BugSharingPolicy.PROPRIETARY)
+ self.assertEqual(
+ BugSharingPolicy.PROPRIETARY, product.bug_sharing_policy)
+
+ def test_updatePillarBranchSharingPolicy(self):
+ # updatePillarSharingPolicies works for branches.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ self.factory.makeCommercialSubscription(product)
+ login_person(owner)
+ self.service.updatePillarSharingPolicies(
+ product,
+ branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
+ self.assertEqual(
+ BranchSharingPolicy.PROPRIETARY, product.branch_sharing_policy)
+
+ def _assert_updatePillarSharingPoliciesUnauthorized(self, user):
+ # updatePillarSharingPolicies raises an Unauthorized exception if the
+ # user is not permitted to do so.
+ product = self.factory.makeProduct()
+ self.assertRaises(
+ Unauthorized, self.service.updatePillarSharingPolicies,
+ product, BranchSharingPolicy.PUBLIC, BugSharingPolicy.PUBLIC)
+
+ def test_updatePillarSharingPoliciesAnonymous(self):
+ # Anonymous users are not allowed.
+ login(ANONYMOUS)
+ self._assert_updatePillarSharingPoliciesUnauthorized(ANONYMOUS)
+
+ def test_updatePillarSharingPoliciesAnyone(self):
+ # Unauthorized users are not allowed.
+ anyone = self.factory.makePerson()
+ login_person(anyone)
+ self._assert_updatePillarSharingPoliciesUnauthorized(anyone)
+
def test_getSharedArtifacts(self):
# Test the getSharedArtifacts method.
owner = self.factory.makePerson()
=== modified file 'lib/lp/registry/templates/pillar-sharing.pt'
--- lib/lp/registry/templates/pillar-sharing.pt 2012-08-23 00:35:25 +0000
+++ lib/lp/registry/templates/pillar-sharing.pt 2012-09-04 00:12:21 +0000
@@ -42,20 +42,32 @@
tal:condition="view/branch_sharing_policies">
<td id="branch-sharing-policy">
Branches sharing policy:
- <strong><span class="value"></span></strong>
+ <strong><span class="value"
+ tal:content="context/branch_sharing_policy/title|string:Legacy policy"
+ ></span></strong>
<a class="editicon sprite edit action-icon" href="#"
style="padding-bottom: 0;">Edit</a>
- <div id="branch-sharing-policy-description" class="formHelp"></div>
+ <div id="branch-sharing-policy-description" class="formHelp"
+ tal:content="context/branch_sharing_policy/description|
+ string:Legacy project sharing policy will continue to be used until
+ a new policy is configured."
+ ></div>
</td>
</tr>
<tr id="bug-sharing-policy-row"
tal:condition="view/bug_sharing_policies">
<td id="bug-sharing-policy">
Bugs sharing policy:
- <strong><span class="value"></span></strong>
+ <strong><span class="value"
+ tal:content="context/bug_sharing_policy/title|string:Legacy policy"
+ ></span></strong>
<a class="editicon sprite edit action-icon" href="#"
style="padding-bottom: 0;">Edit</a>
- <div id="bug-sharing-policy-description" class="formHelp"></div>
+ <div id="bug-sharing-policy-description" class="formHelp"
+ tal:content="context/bug_sharing_policy/description|
+ string:Legacy project sharing policy will continue to be used until
+ a new policy is configured."
+ ></div>
</td>
</tr>
</table>
Follow ups
