launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08617
[Merge] lp:~wallyworld/launchpad/sharing-cleanup into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-cleanup into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-cleanup/+merge/109269
== Implementation ==
A cleanup branch which adds consistency to the sharing service apis and adjusts the feature flags.
This is branch precedes work to address issues with the sharing jobs and will be landed as part of that work.
Remove disclosure.access_mirror_triggers.removed since the model code copes fine if the triggers are st6ill there and so no guard is needed.
Add disclosure.unsubscribe_jobs.enabled to guard the creation of sharing jobs until they are debugged. A quick fix was done previously to use disclosure.enhanced_sharing.writable
== Tests ==
No new tests, just fix existing ones.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/model/tests/test_bugtask.py
lib/lp/bugs/tests/test_bugvisibility.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
lib/lp/registry/tests/test_sharingjob.py
lib/lp/registry/tests/test_teammembership.py
lib/lp/services/features/flags.py
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-cleanup/+merge/109269
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-cleanup into lp:launchpad.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-06-07 06:04:46 +0000
+++ lib/lp/bugs/model/bug.py 2012-06-08 02:35:21 +0000
@@ -842,16 +842,13 @@
# Ensure that the subscription has been flushed.
Store.of(sub).flush()
- # Grant the subscriber access if they can't see the bug (if the
- # database triggers aren't going to do it for us).
- trigger_flag = 'disclosure.access_mirror_triggers.removed'
- if bool(getFeatureFlag(trigger_flag)):
- service = getUtility(IService, 'sharing')
- bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
- if not bugs:
- service.ensureAccessGrants(
- subscribed_by, person, bugs=[self],
- ignore_permissions=True)
+ # Grant the subscriber access if they can't see the bug.
+ service = getUtility(IService, 'sharing')
+ bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
+ if not bugs:
+ service.ensureAccessGrants(
+ person, subscribed_by, bugs=[self],
+ ignore_permissions=True)
# In some cases, a subscription should be created without
# email notifications. suppress_notify determines if
@@ -1809,7 +1806,7 @@
self._reconcileAccess()
self.updateHeat()
- flag = 'disclosure.enhanced_sharing.writable'
+ flag = 'disclosure.unsubscribe_jobs.enabled'
if bool(getFeatureFlag(flag)):
# As a result of the transition, some subscribers may no longer
# have access to the bug. We need to run a job to remove any such
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2012-06-07 06:04:46 +0000
+++ lib/lp/bugs/model/bugtask.py 2012-06-08 02:35:21 +0000
@@ -1187,7 +1187,7 @@
self.maybeConfirm()
# END TEMPORARY BIT FOR BUGTASK AUTOCONFIRM FEATURE FLAG.
- flag = 'disclosure.enhanced_sharing.writable'
+ flag = 'disclosure.unsubscribe_jobs.enabled'
if bool(getFeatureFlag(flag)):
# As a result of the transition, some subscribers may no longer
# have access to the parent bug. We need to run a job to remove any
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2012-06-07 05:59:10 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2012-06-08 02:35:21 +0000
@@ -3342,10 +3342,9 @@
def setUp(self):
self.useFixture(FeatureFixture({
- 'disclosure.enhanced_sharing.writable': 'true',
+ 'disclosure.unsubscribe_jobs.enabled': 'true',
'jobs.celery.enabled_classes':
'RemoveBugSubscriptionsJob',
- 'disclosure.access_mirror_triggers.removed': 'true',
}))
self.useFixture(disable_trigger_fixture())
super(TestTransitionsRemovesSubscribersJob, self).setUp()
=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py 2012-06-07 05:59:10 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py 2012-06-08 02:35:21 +0000
@@ -19,8 +19,6 @@
LEGACY_VISIBILITY_FLAG = {
u"disclosure.enhanced_sharing.writable": "true",
u"disclosure.legacy_subscription_visibility.enabled": u"true"}
-TRIGGERS_REMOVED_FLAG = {
- u"disclosure.access_mirror_triggers.removed": u"true"}
class TestPublicBugVisibility(TestCaseWithFactory):
@@ -132,13 +130,12 @@
def test_subscribeGrantsVisibilityWithTriggersRemoved(self):
# When a user is subscribed to a bug, they are granted access. In this
# test, the database triggers are removed and so model code is used.
- with FeatureFixture(TRIGGERS_REMOVED_FLAG):
- with self.disable_trigger_fixture:
- user = self.factory.makePerson()
- self.assertFalse(self.bug.userCanView(user))
- with celebrity_logged_in('admin'):
- self.bug.subscribe(user, self.owner)
- self.assertTrue(self.bug.userCanView(user))
+ with self.disable_trigger_fixture:
+ user = self.factory.makePerson()
+ self.assertFalse(self.bug.userCanView(user))
+ with celebrity_logged_in('admin'):
+ self.bug.subscribe(user, self.owner)
+ self.assertTrue(self.bug.userCanView(user))
def test_subscribeGrantsVisibilityUsingTriggers(self):
# When a user is subscribed to a bug, they are granted access. In this
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-06-04 04:19:31 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-06-08 02:35:21 +0000
@@ -111,17 +111,17 @@
key_type=Choice(vocabulary=InformationType),
value_type=Choice(vocabulary=SharingPermission)))
@operation_for_version('devel')
- def sharePillarInformation(pillar, sharee, permissions, user):
+ def sharePillarInformation(pillar, sharee, user, permissions):
"""Ensure sharee has the grants for information types on a pillar.
:param pillar: the pillar for which to grant access
:param sharee: the person or team to grant
+ :param user: the user making the request
:param permissions: a dict of {InformationType: SharingPermission}
if SharingPermission is ALL, then create an access policy grant
if SharingPermission is SOME, then remove any access policy grants
if SharingPermission is NONE, then remove all grants for the access
policy
- :param user: the user making the request
"""
@export_write_operation()
@@ -136,8 +136,8 @@
"""Remove a sharee from a pillar.
:param pillar: the pillar from which to remove access
- :param user: the user making the request
:param sharee: the person or team to remove
+ :param user: the user making the request
:param information_types: if None, remove all access, otherwise just
remove the specified access_policies
"""
@@ -152,12 +152,12 @@
branches=List(
Reference(schema=IBranch), title=_('Branches'), required=False))
@operation_for_version('devel')
- def revokeAccessGrants(pillar, user, sharee, branches=None, bugs=None):
+ def revokeAccessGrants(pillar, sharee, user, branches=None, bugs=None):
"""Remove a sharee's access to the specified artifacts.
:param pillar: the pillar from which to remove access
- :param user: the user making the request
:param sharee: the person or team for whom to revoke access
+ :param user: the user making the request
:param bugs: the bugs for which to revoke access
:param branches: the branches for which to revoke access
"""
@@ -171,11 +171,11 @@
branches=List(
Reference(schema=IBranch), title=_('Branches'), required=False))
@operation_for_version('devel')
- def ensureAccessGrants(user, sharee, branches=None, bugs=None):
+ def ensureAccessGrants(sharee, user, branches=None, bugs=None):
"""Ensure a sharee has an access grant to the specified artifacts.
- :param user: the user making the request
:param sharee: the person or team for whom to grant access
+ :param user: the user making the request
:param bugs: the bugs for which to grant access
:param branches: the branches for which to grant access
"""
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2012-06-07 05:59:10 +0000
+++ lib/lp/registry/model/teammembership.py 2012-06-08 02:35:21 +0000
@@ -388,7 +388,7 @@
_fillTeamParticipation(self.person, self.team)
elif old_status in ACTIVE_STATES:
_cleanTeamParticipation(self.person, self.team)
- flag = 'disclosure.enhanced_sharing.writable'
+ flag = 'disclosure.unsubscribe_jobs.enabled'
if bool(getFeatureFlag(flag)):
# A person has left the team so they may no longer have access
# to some artifacts shared with the team. We need to run a job
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-06-04 04:19:31 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-06-08 02:35:21 +0000
@@ -68,9 +68,7 @@
def write_enabled(self):
return (
bool(getFeatureFlag(
- 'disclosure.enhanced_sharing.writable') or
- bool(getFeatureFlag(
- 'disclosure.access_mirror_triggers.removed'))))
+ 'disclosure.enhanced_sharing.writable')))
def getSharedArtifacts(self, pillar, person, user):
"""See `ISharingService`."""
@@ -223,7 +221,7 @@
return result
@available_with_permission('launchpad.Edit', 'pillar')
- def sharePillarInformation(self, pillar, sharee, permissions, user):
+ def sharePillarInformation(self, pillar, sharee, user, permissions):
"""See `ISharingService`."""
# We do not support adding sharees to project groups.
@@ -282,7 +280,7 @@
# call the deletePillarSharee method directly.
if len(info_types_for_nothing) > 0:
self.deletePillarSharee(
- pillar, user, sharee, info_types_for_nothing)
+ pillar, sharee, user, info_types_for_nothing)
# Return sharee data to the caller.
ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
@@ -294,7 +292,7 @@
return sharee
@available_with_permission('launchpad.Edit', 'pillar')
- def deletePillarSharee(self, pillar, user, sharee,
+ def deletePillarSharee(self, pillar, sharee, user,
information_types=None):
"""See `ISharingService`."""
@@ -336,7 +334,7 @@
pillar, sharee, user, information_types=information_types)
@available_with_permission('launchpad.Edit', 'pillar')
- def revokeAccessGrants(self, pillar, user, sharee, branches=None,
+ def revokeAccessGrants(self, pillar, sharee, user, branches=None,
bugs=None):
"""See `ISharingService`."""
@@ -361,11 +359,11 @@
getUtility(IRemoveGranteeSubscriptionsJobSource).create(
pillar, sharee, user, bugs=bugs, branches=branches)
- def ensureAccessGrants(self, user, sharee, branches=None, bugs=None,
- **kwargs):
+ def ensureAccessGrants(self, sharee, user, branches=None, bugs=None,
+ ignore_permissions=False):
"""See `ISharingService`."""
- if not self.write_enabled:
+ if not ignore_permissions and not self.write_enabled:
raise Unauthorized("This feature is not yet enabled.")
artifacts = []
@@ -373,7 +371,6 @@
artifacts.extend(branches)
if bugs:
artifacts.extend(bugs)
- ignore_permissions = kwargs.get('ignore_permissions', False)
if not ignore_permissions:
# The user needs to have launchpad.Edit permission on all supplied
# bugs and branches or else we raise an Unauthorized exception.
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-06-05 02:03:44 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-06-08 02:35:21 +0000
@@ -430,7 +430,7 @@
InformationType.PROPRIETARY: SharingPermission.NOTHING}
with FeatureFixture(WRITE_FLAG):
sharee_data = self.service.sharePillarInformation(
- pillar, sharee, permissions, grantor)
+ pillar, sharee, grantor, permissions)
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
policy_grant_source = getUtility(IAccessPolicyGrantSource)
[grant] = policy_grant_source.findByPolicy(policies)
@@ -460,8 +460,8 @@
login_person(owner)
self.assertRaises(
AssertionError, self.service.sharePillarInformation,
- project_group, sharee,
- {InformationType.USERDATA: SharingPermission.ALL}, owner)
+ project_group, sharee, owner,
+ {InformationType.USERDATA: SharingPermission.ALL})
def test_updateProductSharee(self):
# Users with launchpad.Edit can add sharees.
@@ -490,7 +490,7 @@
grant.policy.type: SharingPermission.SOME}
with FeatureFixture(WRITE_FLAG):
sharee_data = self.service.sharePillarInformation(
- pillar, sharee, permissions, self.factory.makePerson())
+ pillar, sharee, self.factory.makePerson(), permissions)
self.assertIsNone(sharee_data)
def _assert_sharePillarInformationUnauthorized(self, pillar):
@@ -501,8 +501,8 @@
user = self.factory.makePerson()
self.assertRaises(
Unauthorized, self.service.sharePillarInformation,
- pillar, sharee,
- {InformationType.USERDATA: SharingPermission.ALL}, user)
+ pillar, sharee, user,
+ {InformationType.USERDATA: SharingPermission.ALL})
def test_sharePillarInformationAnonymous(self):
# Anonymous users are not allowed.
@@ -527,8 +527,8 @@
user = self.factory.makePerson()
self.assertRaises(
Unauthorized, self.service.sharePillarInformation,
- product, sharee,
- {InformationType.USERDATA: SharingPermission.ALL}, user)
+ product, sharee, user,
+ {InformationType.USERDATA: SharingPermission.ALL})
def _assert_deletePillarSharee(self, pillar, types_to_delete=None):
access_policies = getUtility(IAccessPolicySource).findByPillar(
@@ -550,7 +550,7 @@
# Delete data for a specific information type.
with FeatureFixture(WRITE_FLAG):
self.service.deletePillarSharee(
- pillar, pillar.owner, grantee, types_to_delete)
+ pillar, grantee, pillar.owner, types_to_delete)
# Assemble the expected data for the remaining access grants for
# grantee.
expected_data = []
@@ -606,7 +606,7 @@
with FeatureFixture(WRITE_FLAG):
self.assertRaises(
Unauthorized, self.service.deletePillarSharee,
- pillar, pillar.owner, [InformationType.USERDATA])
+ pillar, pillar.owner, pillar.owner, [InformationType.USERDATA])
def test_deletePillarShareeAnonymous(self):
# Anonymous users are not allowed.
@@ -629,7 +629,7 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.deletePillarSharee,
- product, product.owner, [InformationType.USERDATA])
+ product, product.owner, product.owner, [InformationType.USERDATA])
def _assert_deleteShareeRemoveSubscriptions(self,
types_to_delete=None):
@@ -666,7 +666,7 @@
# Delete data for specified information types or all.
with FeatureFixture(WRITE_FLAG):
self.service.deletePillarSharee(
- product, product.owner, grantee, types_to_delete)
+ product, grantee, product.owner, types_to_delete)
with block_on_job(self):
transaction.commit()
@@ -734,7 +734,7 @@
with FeatureFixture(WRITE_FLAG):
self.service.revokeAccessGrants(
- pillar, pillar.owner, grantee, bugs=bugs, branches=branches)
+ pillar, grantee, pillar.owner, bugs=bugs, branches=branches)
with block_on_job(self):
transaction.commit()
@@ -788,7 +788,7 @@
with FeatureFixture(WRITE_FLAG):
self.assertRaises(
Unauthorized, self.service.revokeAccessGrants,
- product, product.owner, sharee, bugs=[bug])
+ product, sharee, product.owner, bugs=[bug])
def test_revokeAccessGrantsAnonymous(self):
# Anonymous users are not allowed.
@@ -812,7 +812,7 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.revokeAccessGrants,
- product, product.owner, sharee, bugs=[bug])
+ product, sharee, product.owner, bugs=[bug])
def _assert_ensureAccessGrants(self, user, bugs, branches,
grantee=None):
@@ -821,7 +821,7 @@
grantee = self.factory.makePerson()
with FeatureFixture(WRITE_FLAG):
self.service.ensureAccessGrants(
- user, grantee, bugs=bugs, branches=branches)
+ grantee, user, bugs=bugs, branches=branches)
# Check that grantee has expected access grants.
shared_bugs = []
@@ -877,7 +877,7 @@
# Create an existing access grant.
grantee = self.factory.makePerson()
with FeatureFixture(WRITE_FLAG):
- self.service.ensureAccessGrants(owner, grantee, bugs=[bug])
+ self.service.ensureAccessGrants(grantee, owner, bugs=[bug])
# Test with a new bug as well as the one for which access is already
# granted.
self._assert_ensureAccessGrants(owner, [bug, bug2], None, grantee)
@@ -917,7 +917,7 @@
login_person(owner)
self.assertRaises(
Unauthorized, self.service.ensureAccessGrants,
- product.owner, sharee, bugs=[bug])
+ sharee, product.owner, bugs=[bug])
def test_getSharedArtifacts(self):
# Test the getSharedArtifacts method.
@@ -1137,10 +1137,10 @@
return self._named_post(
'sharePillarInformation', pillar=pillar_uri,
sharee=self.grantee_uri,
+ user=self.grantor_uri,
permissions={
InformationType.USERDATA.title:
- SharingPermission.ALL.title},
- user=self.grantor_uri)
+ SharingPermission.ALL.title})
class TestLaunchpadlib(ApiTestMixin, TestCaseWithFactory):
=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py 2012-06-05 02:03:44 +0000
+++ lib/lp/registry/tests/test_sharingjob.py 2012-06-08 02:35:21 +0000
@@ -412,9 +412,6 @@
layer = DatabaseFunctionalLayer
def setUp(self):
- self.useFixture(FeatureFixture({
- 'disclosure.access_mirror_triggers.removed': 'true',
- }))
self.useFixture(disable_trigger_fixture())
super(TestRunViaCron, self).setUp()
@@ -490,7 +487,6 @@
self.useFixture(FeatureFixture({
'jobs.celery.enabled_classes':
'RemoveBugSubscriptionsJob',
- 'disclosure.access_mirror_triggers.removed': 'true',
}))
self.useFixture(disable_trigger_fixture())
super(RemoveBugSubscriptionsJobTestCase, self).setUp()
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py 2012-06-07 05:59:10 +0000
+++ lib/lp/registry/tests/test_teammembership.py 2012-06-08 02:35:21 +0000
@@ -998,7 +998,7 @@
def setUp(self):
self.useFixture(FeatureFixture({
- 'disclosure.enhanced_sharing.writable': 'true',
+ 'disclosure.unsubscribe_jobs.enabled': 'true',
'jobs.celery.enabled_classes':
'RemoveGranteeSubscriptionsJob',
}))
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2012-06-03 23:11:40 +0000
+++ lib/lp/services/features/flags.py 2012-06-08 02:35:21 +0000
@@ -288,10 +288,10 @@
'',
'Sharing management',
''),
- ('disclosure.access_mirror_triggers.removed',
+ ('disclosure.unsubscribe_jobs.enabled',
'boolean',
- ('If true, the database triggers which cause subscribing to grant'
- 'access to a bug or branch have been removed.'),
+ ('If true, the jobs to unsubscribe users who lose access to bugs'
+ 'and branches are run.'),
'',
'',
''),
Follow ups
