launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03419
[Merge] lp:~abentley/launchpad/translation-sharing-details-permissions into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/translation-sharing-details-permissions into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #758920 in Launchpad itself: "On translate sharing details page, usage settings behaves badly with permission issues"
https://bugs.launchpad.net/launchpad/+bug/758920
For more details, see:
https://code.launchpad.net/~abentley/launchpad/translation-sharing-details-permissions/+merge/58746
= Summary =
Fix bug #758920 and other permission-related issues.
== Proposed fix ==
Disable checklist items if user does not have permission to change them.
== Pre-implementation notes ==
None
== Implementation details ==
Provide new user_authorized flag on CheckItem, and do not enable checkitems if user is not authorized.
To ensure consistent behaviour at initialization and update time, implement TranslationSharingController.set_permissions() that acts on a dict of values and sets user_authorized.
Call set_permissions from both configure() and select_productseries().
Extract getSharingDetailPermissions from setPackagingReturnSharingDetailPermissions(). Use getSharingDetailPermissions to configure the IJSONRequestCache. Add can_change_product_series to getSharingDetailPermissions Update tests.
Driveby: remove non-existent SourcePackageTranslationSharingStatus from __all__.
== Tests ==
firefox lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.html
bin/test -t SharingDetailPermissions -t test_cache_contents_no_productseries
== Demo and Q/A ==
For a sourcepackage that has no packaging link and you do not have permissions on, go to +sharing-details. Select a project series. The change/delete project series items should be shown. Log in as another user. The change/delete project series items should not be shown.
Similarly, if you have access to the product/productseries, you should be able to edit the remaining items. Otherwise, no.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/sourcepackage.py
lib/lp/registry/tests/test_sourcepackage.py
lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js
lib/lp/translations/browser/tests/test_sharing_details.py
lib/lp/registry/model/sourcepackage.py
lib/lp/translations/javascript/sourcepackage_sharing_details.js
lib/lp/translations/browser/sourcepackage.py
--
https://code.launchpad.net/~abentley/launchpad/translation-sharing-details-permissions/+merge/58746
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/translation-sharing-details-permissions into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/sourcepackage.py'
--- lib/lp/registry/interfaces/sourcepackage.py 2011-04-20 16:54:48 +0000
+++ lib/lp/registry/interfaces/sourcepackage.py 2011-04-21 20:16:31 +0000
@@ -212,11 +212,7 @@
@export_write_operation()
@operation_for_version('devel')
def setPackagingReturnSharingDetailPermissions(productseries, owner):
- """Like setPackaging(), but returns a dictionary which says
- if the current user can change the series' target branch,
- if he can change the translation usage settings of the
- product and if he can change the translation synchronisation
- setting of the series.
+ """Like setPackaging(), but returns getSharingDetailPermissions().
This method is intended for AJAX usage on the +sharing-details
page.
@@ -227,6 +223,16 @@
def deletePackaging():
"""Delete the packaging for this sourcepackage."""
+ def getSharingDetailPermissions(self):
+ """Return a dictionary of user permissions for +sharing-details page.
+
+ This shows whether the user can change
+ - The project series
+ - The project series target branch
+ - The project series autoimport mode
+ - The project translation usage setting
+ """
+
def getSuiteSourcePackage(pocket):
"""Return the `ISuiteSourcePackage` for this package in 'pocket'.
=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py 2011-04-20 15:41:17 +0000
+++ lib/lp/registry/model/sourcepackage.py 2011-04-21 20:16:31 +0000
@@ -549,14 +549,35 @@
owner):
"""See `ISourcePackage`."""
self.setPackaging(productseries, owner)
+ return self.getSharingDetailPermissions()
+
+ def getSharingDetailPermissions(self):
user = getUtility(ILaunchBag).user
- return {
- 'user_can_change_branch': user.canWrite(productseries, 'branch'),
- 'user_can_change_translation_usage':
- user.canWrite(productseries.product, 'translations_usage'),
- 'user_can_change_translations_autoimport_mode':
- user.canWrite(productseries, 'translations_autoimport_mode'),
- }
+ productseries = self.productseries
+ permissions = {
+ 'user_can_change_product_series': False,
+ 'user_can_change_branch': False,
+ 'user_can_change_translation_usage': False,
+ 'user_can_change_translations_autoimport_mode': False}
+ if user is None:
+ pass
+ elif productseries is None:
+ permissions['user_can_change_product_series'] = user.canAccess(
+ self, 'setPackaging')
+ else:
+ permissions.update({
+ 'user_can_change_product_series':
+ self.direct_packaging.userCanDelete(),
+ 'user_can_change_branch':
+ user.canWrite(productseries, 'branch'),
+ 'user_can_change_translation_usage':
+ user.canWrite(
+ productseries.product, 'translations_usage'),
+ 'user_can_change_translations_autoimport_mode':
+ user.canWrite(
+ productseries, 'translations_autoimport_mode'),
+ })
+ return permissions
def deletePackaging(self):
"""See `ISourcePackage`."""
=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py 2011-04-20 15:41:17 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py 2011-04-21 20:16:31 +0000
@@ -340,26 +340,60 @@
packaging_owner.canWrite(
productseries.product, 'translations_usage'))
expected = {
- 'user_can_change_branch': False,
- 'user_can_change_translation_usage': False,
- 'user_can_change_translations_autoimport_mode': False,
- }
- self.assertEqual(expected, permissions)
-
- def test_setPackagingReturnSharingDetailPermissions__series_owner(self):
+ 'user_can_change_product_series': True,
+ 'user_can_change_branch': False,
+ 'user_can_change_translation_usage': False,
+ 'user_can_change_translations_autoimport_mode': False,
+ }
+ self.assertEqual(expected, permissions)
+
+ def test_getSharingDetailPermissions__ordinary_user(self):
+ """An ordinary user cannot set the series' branch or translation
+ synchronisation settings, or the translation usage settings of the
+ product.
+ """
+ packaging = self.factory.makePackagingLink()
+ sourcepackage = packaging.sourcepackage
+ productseries = packaging.productseries
+ with person_logged_in(packaging.owner):
+ permissions = sourcepackage.getSharingDetailPermissions()
+ self.assertEqual(productseries, sourcepackage.productseries)
+ self.assertFalse(
+ packaging.owner.canWrite(productseries, 'branch'))
+ self.assertFalse(
+ packaging.owner.canWrite(
+ productseries, 'translations_autoimport_mode'))
+ self.assertFalse(
+ packaging.owner.canWrite(
+ productseries.product, 'translations_usage'))
+ expected = {
+ 'user_can_change_product_series': True,
+ 'user_can_change_branch': False,
+ 'user_can_change_translation_usage': False,
+ 'user_can_change_translations_autoimport_mode': False,
+ }
+ self.assertEqual(expected, permissions)
+
+ def makeDistinctOwnerProductSeries(self):
+ # Ensure productseries owner is distinct from product owner.
+ return self.factory.makeProductSeries(
+ owner=self.factory.makePerson())
+
+ def test_getSharingDetailPermissions__series_owner(self):
"""A product series owner can create a packaging link, and he can
set the series' branch or translation syncronisation settings,
but he cannot set the translation usage settings of the product.
"""
- series_owner = self.factory.makePerson()
- sourcepackage = self.factory.makeSourcePackage()
- product = self.factory.makeProduct()
+ productseries = self.makeDistinctOwnerProductSeries()
+ series_owner = productseries.owner
+ # Ensure productseries owner is distinct from product owner.
productseries = self.factory.makeProductSeries(
- product=product, owner=series_owner)
+ owner=series_owner)
with person_logged_in(series_owner):
- permissions = (
- sourcepackage.setPackagingReturnSharingDetailPermissions(
- productseries, series_owner))
+ packaging = self.factory.makePackagingLink(
+ productseries=productseries, owner=series_owner)
+ sourcepackage = packaging.sourcepackage
+ permissions = sourcepackage.getSharingDetailPermissions()
self.assertEqual(productseries, sourcepackage.productseries)
self.assertTrue(series_owner.canWrite(productseries, 'branch'))
self.assertTrue(
@@ -369,25 +403,25 @@
series_owner.canWrite(
productseries.product, 'translations_usage'))
expected = {
+ 'user_can_change_product_series': True,
'user_can_change_branch': True,
'user_can_change_translation_usage': False,
'user_can_change_translations_autoimport_mode': True,
}
self.assertEqual(expected, permissions)
- def test_setPackagingReturnSharingDetailPermissions__product_owner(self):
- """A product series owner can create a packaging link, and he can
- set the series' branch and the translation syncronisation settings,
- and the translation usage settings of the product.
+ def test_getSharingDetailPermissions__product_owner(self):
+ """A product owner can create a packaging link, and he can set the
+ series' branch and the translation syncronisation settings, and the
+ translation usage settings of the product.
"""
- sourcepackage = self.factory.makeSourcePackage()
- product = self.factory.makeProduct()
- productseries = self.factory.makeProductSeries(
- product=product, owner=self.factory.makePerson())
+ productseries = self.makeDistinctOwnerProductSeries()
+ product = productseries.product
with person_logged_in(product.owner):
- permissions = (
- sourcepackage.setPackagingReturnSharingDetailPermissions(
- productseries, product.owner))
+ packaging = self.factory.makePackagingLink(
+ productseries=productseries, owner=product.owner)
+ sourcepackage = packaging.sourcepackage
+ permissions = sourcepackage.getSharingDetailPermissions()
self.assertEqual(productseries, sourcepackage.productseries)
self.assertTrue(product.owner.canWrite(productseries, 'branch'))
self.assertTrue(
@@ -397,12 +431,58 @@
product.owner.canWrite(
productseries.product, 'translations_usage'))
expected = {
+ 'user_can_change_product_series': True,
'user_can_change_branch': True,
'user_can_change_translation_usage': True,
'user_can_change_translations_autoimport_mode': True,
}
self.assertEqual(expected, permissions)
+ def test_getSharingDetailPermissions_change_product(self):
+ """Test user_can_change_product_series.
+
+ Until a Packaging is created, anyone can change product series.
+ Afterward, random people cannot change product series.
+ """
+ sourcepackage = self.factory.makeSourcePackage()
+ person1 = self.factory.makePerson()
+ person2 = self.factory.makePerson()
+
+ def can_change_product_series():
+ return sourcepackage.getSharingDetailPermissions()[
+ 'user_can_change_product_series']
+ with person_logged_in(person1):
+ self.assertTrue(can_change_product_series())
+ with person_logged_in(person2):
+ self.assertTrue(can_change_product_series())
+ self.factory.makePackagingLink(
+ sourcepackage=sourcepackage, owner=person1)
+ with person_logged_in(person1):
+ self.assertTrue(can_change_product_series())
+ with person_logged_in(person2):
+ self.assertFalse(can_change_product_series())
+
+ def test_getSharingDetailPermissions_no_product_series(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ expected = {
+ 'user_can_change_product_series': True,
+ 'user_can_change_branch': False,
+ 'user_can_change_translation_usage': False,
+ 'user_can_change_translations_autoimport_mode': False}
+ self.assertEqual(
+ expected, sourcepackage.getSharingDetailPermissions())
+
+ def test_getSharingDetailPermissions_no_user(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ expected = {
+ 'user_can_change_product_series': False,
+ 'user_can_change_branch': False,
+ 'user_can_change_translation_usage': False,
+ 'user_can_change_translations_autoimport_mode': False}
+ logout()
+ self.assertEqual(
+ expected, sourcepackage.getSharingDetailPermissions())
+
class TestSourcePackageWebService(WebServiceTestCase):
=== modified file 'lib/lp/translations/browser/sourcepackage.py'
--- lib/lp/translations/browser/sourcepackage.py 2011-04-14 15:20:37 +0000
+++ lib/lp/translations/browser/sourcepackage.py 2011-04-21 20:16:31 +0000
@@ -8,7 +8,6 @@
__all__ = [
'SourcePackageTranslationsExportView',
'SourcePackageTranslationsView',
- 'SourcePackageTranslationSharingStatus',
]
@@ -145,6 +144,7 @@
'upstream_branch': self.upstream_branch,
'product': self.product,
})
+ cache.objects.update(self.context.getSharingDetailPermissions())
@property
def branch_link(self):
=== modified file 'lib/lp/translations/browser/tests/test_sharing_details.py'
--- lib/lp/translations/browser/tests/test_sharing_details.py 2011-04-14 15:20:37 +0000
+++ lib/lp/translations/browser/tests/test_sharing_details.py 2011-04-21 20:16:31 +0000
@@ -340,6 +340,10 @@
def test_cache_contents_no_productseries(self):
objects = self.getCacheObjects()
self.assertIs(None, objects['productseries'])
+ self.assertIn('user_can_change_product_series', objects)
+ self.assertIn('user_can_change_branch', objects)
+ self.assertIn('user_can_change_translation_usage', objects)
+ self.assertIn('user_can_change_translations_autoimport_mode', objects)
def test_cache_contents_no_branch(self):
self.configureSharing()
=== modified file 'lib/lp/translations/javascript/sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/sourcepackage_sharing_details.js 2011-04-18 13:39:13 +0000
+++ lib/lp/translations/javascript/sourcepackage_sharing_details.js 2011-04-21 20:16:31 +0000
@@ -19,12 +19,16 @@
// True if this item is enabled.
enabled: {
getter: function(){
+ if (!this.get('user_authorized')){
+ return false;
+ }
if (Y.Lang.isNull(this.get('dependency'))){
return true;
}
return this.get('dependency').get('complete');
}
},
+ user_authorized: {value: false},
// The HTML identifier of the item.
identifier: null,
pending: {value: false}
@@ -132,7 +136,7 @@
{identifier: 'upstream-sync', dependency: branch});
this.set('autoimport', autoimport);
var configuration = new CheckItem(
- {identifier: 'configuration'});
+ {identifier: 'configuration', user_authorized: true});
this.set('configuration', configuration);
this.set(
'all_items', [product_series, usage, branch, autoimport]);
@@ -313,6 +317,24 @@
this.replace_productseries(config.productseries);
this.replace_product(config.product);
this.set_branch(config.upstream_branch);
+ this.set_permissions(config);
+ },
+ set_permissions: function(permissions){
+ var usage = this.get('tsconfig').get('translations_usage');
+ usage.set(
+ 'user_authorized', permissions.user_can_change_translation_usage);
+ var branch = this.get('tsconfig').get('branch');
+ branch.set('user_authorized', permissions.user_can_change_branch);
+ var autoimport = this.get('tsconfig').get('autoimport');
+ autoimport.set(
+ 'user_authorized',
+ permissions.user_can_change_translations_autoimport_mode);
+ var product_series = this.get('tsconfig').get('product_series');
+ if (permissions.user_can_change_product_series !== undefined){
+ product_series.set(
+ 'user_authorized',
+ permissions.user_can_change_product_series);
+ }
},
set_productseries: function(productseries) {
if (Y.Lang.isValue(productseries)){
@@ -381,9 +403,11 @@
var source_package = that.get('source_package');
config.parameters = {
productseries: productseries_summary.api_uri};
- source_package.named_post('setPackaging', config);
+ source_package.named_post(
+ 'setPackagingReturnSharingDetailPermissions', config);
}
- function get_productseries(config) {
+ function get_productseries(config, permissions) {
+ that.set_permissions(permissions);
lp_client.get(productseries_summary.api_uri, config);
}
function cache_productseries(config, productseries) {
=== modified file 'lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js'
--- lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js 2011-04-18 13:39:13 +0000
+++ lib/lp/translations/javascript/tests/test_sourcepackage_sharing_details.js 2011-04-21 20:16:31 +0000
@@ -25,6 +25,7 @@
test_translations_usage_enabled: function() {
var sharing_config = new TranslationSharingConfig();
var usage = sharing_config.get('translations_usage');
+ usage.set('user_authorized', true);
Y.Assert.isFalse(usage.get('enabled'));
sharing_config.get('product_series').set_link('ps', 'http://');
Y.Assert.isTrue(usage.get('enabled'));
@@ -33,9 +34,11 @@
var sharing_config = new TranslationSharingConfig();
var product_series = sharing_config.get('product_series');
Y.Assert.isFalse(product_series.get('complete'));
- Y.Assert.isFalse(sharing_config.get('branch').get('enabled'));
+ var branch = sharing_config.get('branch');
+ branch.set('user_authorized', true);
+ Y.Assert.isFalse(branch.get('enabled'));
product_series.set_link('ps', 'http://');
- Y.Assert.isTrue(sharing_config.get('branch').get('enabled'));
+ Y.Assert.isTrue(branch.get('enabled'));
},
test_set_branch: function() {
var lp_client = new Y.lp.client.Launchpad();
@@ -51,7 +54,9 @@
},
test_autoimport: function() {
var sharing_config = new TranslationSharingConfig();
- Y.Assert.isFalse(sharing_config.get('autoimport').get('enabled'));
+ var autoimport = sharing_config.get('autoimport');
+ autoimport.set('user_authorized', true);
+ Y.Assert.isFalse(autoimport.get('enabled'));
sharing_config.get('branch').set_link('br', 'http://foo');
Y.Assert.isTrue(sharing_config.get('autoimport').get('enabled'));
},
@@ -77,12 +82,12 @@
Y.Assert.isTrue(lci.get('complete'));
},
test_CheckItem_enabled: function() {
- var ci = new CheckItem();
+ var ci = new CheckItem({user_authorized: true});
Y.Assert.isTrue(ci.get('enabled'));
},
test_CheckItem_enabled_dependency: function(){
var lci = new LinkCheckItem();
- var ci = new CheckItem({dependency: lci});
+ var ci = new CheckItem({dependency: lci, user_authorized: true});
Y.Assert.isFalse(ci.get('enabled'));
lci.set_link('text', 'http://example.com');
Y.Assert.isTrue(ci.get('enabled'));
@@ -91,6 +96,12 @@
var ci = new CheckItem({identifier: 'id1'});
Y.Assert.areEqual('id1', ci.get('identifier'));
},
+ test_CheckItem_user_authorized: function() {
+ var ci = new CheckItem({user_authorized: true});
+ Y.Assert.isTrue(ci.get('enabled'));
+ ci.set('user_authorized', false);
+ Y.Assert.isFalse(ci.get('enabled'));
+ },
test_configure_empty: function() {
var ctrl = new TranslationSharingController();
var cache = {
@@ -120,7 +131,8 @@
},
context: {
resource_type_link: 'http://sourcepackage'
- }
+ },
+ user_can_change_branch: true
};
var ctrl = new TranslationSharingController();
var lp_client = new Y.lp.client.Launchpad();
@@ -138,6 +150,7 @@
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'));
@@ -149,13 +162,69 @@
self_link: 'http://foo',
resource_type_link: 'http://foo_type'
},
- bar: null
+ bar: null,
+ baz: true
};
var lp_client = new Y.lp.client.Launchpad();
cache = test_ns.convert_cache(lp_client, cache);
Y.Assert.isNull(cache.bar);
+ Y.Assert.isTrue(cache.baz);
Y.Assert.areEqual('http://foo', cache.foo.get('self_link'));
},
+ test_set_permissions: function(){
+ var ctrl = new TranslationSharingController();
+ var config = ctrl.get('tsconfig');
+ var overall = config.get('configuration');
+ Y.Assert.isTrue(overall.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: true,
+ user_can_change_branch: true,
+ user_can_change_translations_autoimport_mode: true,
+ user_can_change_product_series: true
+ });
+ var usage = config.get('translations_usage');
+ Y.Assert.isTrue(usage.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: false,
+ user_can_change_branch: true,
+ user_can_change_translations_autoimport_mode: true,
+ user_can_change_product_series: true
+ });
+ Y.Assert.isFalse(usage.get('user_authorized'));
+ var branch = config.get('branch');
+ Y.Assert.isTrue(branch.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: false,
+ user_can_change_branch: false,
+ user_can_change_translations_autoimport_mode: true,
+ user_can_change_product_series: true
+ });
+ Y.Assert.isFalse(branch.get('user_authorized'));
+ var autoimport = config.get('autoimport');
+ Y.Assert.isTrue(autoimport.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: false,
+ user_can_change_branch: false,
+ user_can_change_translations_autoimport_mode: false,
+ user_can_change_product_series: true
+ });
+ Y.Assert.isFalse(autoimport.get('user_authorized'));
+ var product_series = config.get('product_series');
+ Y.Assert.isTrue(product_series.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: false,
+ user_can_change_branch: false,
+ user_can_change_translations_autoimport_mode: false,
+ user_can_change_product_series: false
+ });
+ Y.Assert.isFalse(product_series.get('user_authorized'));
+ ctrl.set_permissions({
+ user_can_change_translation_usage: false,
+ user_can_change_branch: false,
+ user_can_change_translations_autoimport_mode: false
+ });
+ Y.Assert.isFalse(product_series.get('user_authorized'));
+ },
test_update_branch: function(){
var complete = Y.one('#branch-complete');
var incomplete = Y.one('#branch-incomplete');
@@ -203,6 +272,7 @@
var incomplete = Y.one('#branch-incomplete');
var ctrl = new TranslationSharingController();
var branch = ctrl.get('tsconfig').get('branch');
+ branch.set('user_authorized', true);
ctrl.update_check(branch);
Y.Assert.isTrue(incomplete.hasClass('lowlight'));
var product_series = ctrl.get('tsconfig').get('product_series');
@@ -215,6 +285,7 @@
'#upstream-sync-incomplete-spinner');
var ctrl = new TranslationSharingController();
var autoimport = ctrl.get('tsconfig').get('autoimport');
+ autoimport.set('user_authorized', true);
var branch = ctrl.get('tsconfig').get('branch');
branch.set_link('a', 'b');
var incomplete_picker = Y.one(
