launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03325
[Merge] lp:~rvb/launchpad/perm-comment-758938 into lp:launchpad
Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/perm-comment-758938 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #758938 in Launchpad itself: "Allow derived series comments from anyone who is an uploader"
https://bugs.launchpad.net/launchpad/+bug/758938
For more details, see:
https://code.launchpad.net/~rvb/launchpad/perm-comment-758938/+merge/57712
This branch modifies how the permission to have lp.Edit on a DSD is calculated. Now it's equivalent to have lp.View on the related distribution. The goal is to allow (much) more people to comment on DSDs, blacklist DSDs and request package diffs.
= Tests =
(modified tests)
./bin/test -cvv test_distroseriesdifference_views test_comment_for_display_provides_icomment
./bin/test -cvv test_distroseriesdifference_views test_show_edit_options_non_ajax
./bin/test -cvv test_distroseriesdifference_views test_show_edit_options_editor
./bin/test -cvv test_distroseriesdifference_views test_source_diff_rendering_diff
./bin/test -cvv test_distroseriesdifference_views test_source_diff_rendering_diff_no_link
./bin/test -cvv test_distroseriesdifference_views test_parent_source_diff_rendering_diff_no_link
./bin/test -cvv test_distroseriesdifference_views test_parent_source_diff_rendering_diff
./bin/test -cvv test_distroseriesdifference_views test_blacklist_options
./bin/test -cvv test_distroseriesdifference_views test_package_diff_request_link
./bin/test -cvv test_distroseriesdifference_views test_comments_rendered
./bin/test -cvv test_distroseriesdifference_webservice test_blacklist
./bin/test -cvv test_distroseriesdifference_webservice test_unblacklist
./bin/test -cvv test_distroseriesdifference_webservice test_addComment
./bin/test -cvv test_distroseriesdifference_webservice test_requestDiffs
./bin/test -cvv test_distroseriesdifference_webservice test_requestPackageDiffs_exception
./bin/test -cvv test_distroseriesdifference_webservice test_package_diffs
./bin/test -cvv test_distroseriesdifference test_addComment
./bin/test -cvv test_distroseriesdifference test_blacklist_default
./bin/test -cvv test_distroseriesdifference test_blacklist_all
./bin/test -cvv test_distroseriesdifference test_unblacklist
./bin/test -cvv test_distroseriesdifference test_unblacklist_resolved
= QA =
On a local instance or on dogfood:
- Login in as a non priviledged user (name12 on a local instance)
- Access the +localpackagediffs page:
https://dogfood.launchpad.net/ubuntu/natty/+localpackagediffs
https://launchpad.dev/deribuntu/deriwarty/+localpackagediffs
- make sure a non priviledged user (hence with lp.View on the distribution) can:
- blacklist packages
- add comments
- request packages diffs
--
https://code.launchpad.net/~rvb/launchpad/perm-comment-758938/+merge/57712
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/perm-comment-758938 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2011-03-30 13:33:35 +0000
+++ lib/canonical/launchpad/security.py 2011-04-14 15:42:37 +0000
@@ -31,11 +31,11 @@
from canonical.launchpad.interfaces.librarian import (
ILibraryFileAliasWithParent,
)
+from canonical.launchpad.interfaces.message import IMessage
from canonical.launchpad.interfaces.oauth import (
IOAuthAccessToken,
IOAuthRequestToken,
)
-from canonical.launchpad.interfaces.message import IMessage
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.interfaces import (
IAuthorization,
@@ -58,8 +58,7 @@
from lp.blueprints.interfaces.sprint import ISprint
from lp.blueprints.interfaces.sprintspecification import ISprintSpecification
from lp.bugs.interfaces.bugtarget import IOfficialBugTagTargetRestricted
-from lp.bugs.interfaces.structuralsubscription import (
- IStructuralSubscription)
+from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
from lp.buildmaster.interfaces.builder import (
IBuilder,
IBuilderSet,
@@ -114,6 +113,9 @@
IDistributionSourcePackage,
)
from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseriesdifference import (
+ IDistroSeriesDifferenceEdit,
+ )
from lp.registry.interfaces.entitlement import IEntitlement
from lp.registry.interfaces.gpg import IGPGKey
from lp.registry.interfaces.irc import IIrcID
@@ -192,9 +194,7 @@
IPackageUploadQueue,
)
from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
-from lp.translations.interfaces.customlanguagecode import (
- ICustomLanguageCode,
- )
+from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
from lp.translations.interfaces.languagepack import ILanguagePack
from lp.translations.interfaces.pofile import IPOFile
from lp.translations.interfaces.potemplate import IPOTemplate
@@ -1019,6 +1019,16 @@
usedfor = ICountry
+class EditDistroSeriesDifference(AuthorizationBase):
+ """Anyone with lp.View on the distribution can edit a DSD."""
+ permission = 'launchpad.Edit'
+ usedfor = IDistroSeriesDifferenceEdit
+
+ def checkAuthenticated(self, user):
+ return check_permission(
+ 'launchpad.View', self.obj.derived_series.distribution)
+
+
class SeriesDrivers(AuthorizationBase):
"""Drivers can approve or decline features and target bugs.
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-04-07 20:37:22 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-04-14 15:42:37 +0000
@@ -16,6 +16,7 @@
import transaction
from zope.component import getUtility
+from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.launchpad.webapp.testing import verifyObject
from canonical.testing import LaunchpadFunctionalLayer
@@ -42,7 +43,6 @@
person_logged_in,
TestCaseWithFactory,
)
-from zope.security.proxy import removeSecurityProxy
from lp.testing.views import create_initialized_view
@@ -60,9 +60,10 @@
def test_comment_for_display_provides_icomment(self):
# The DSDDisplayComment browser object provides IComment.
ds_diff = self.factory.makeDistroSeriesDifference()
- owner = ds_diff.derived_series.owner
- with person_logged_in(owner):
- comment = ds_diff.addComment(owner, "I'm working on this.")
+
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ comment = ds_diff.addComment(person, "I'm working on this.")
comment_for_display = DistroSeriesDifferenceDisplayComment(comment)
self.assertTrue(verifyObject(IComment, comment_for_display))
@@ -143,7 +144,7 @@
ds_diff = self.factory.makeDistroSeriesDifference()
# Without JS, even editors don't see blacklist options.
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra')
self.assertFalse(view.show_edit_options)
@@ -154,7 +155,7 @@
ds_diff = self.factory.makeDistroSeriesDifference()
request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra', request=request)
self.assertTrue(view.show_edit_options)
@@ -278,7 +279,7 @@
ds_diff = self.factory.makeDistroSeriesDifference(
set_base_version=True)
- with person_logged_in(ds_diff.derived_series.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.package_diff = self.factory.makePackageDiff()
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
@@ -299,7 +300,7 @@
(PackageDiffStatus.PENDING, 'PENDING'),
(PackageDiffStatus.FAILED, 'FAILED')]
for status, css_class in statuses_and_classes:
- with person_logged_in(ds_diff.derived_series.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.package_diff = self.factory.makePackageDiff(
status=status)
@@ -323,7 +324,7 @@
(PackageDiffStatus.PENDING, 'PENDING'),
(PackageDiffStatus.FAILED, 'FAILED')]
for status, css_class in statuses_and_classes:
- with person_logged_in(ds_diff.derived_series.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.parent_package_diff = self.factory.makePackageDiff(
status=status)
@@ -353,7 +354,7 @@
ds_diff = self.factory.makeDistroSeriesDifference(
set_base_version=True)
- with person_logged_in(ds_diff.derived_series.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.parent_package_diff = self.factory.makePackageDiff()
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
@@ -378,10 +379,10 @@
def test_comments_rendered(self):
# If there are comments on the difference, they are rendered.
ds_diff = self.factory.makeDistroSeriesDifference()
- owner = ds_diff.derived_series.owner
- with person_logged_in(owner):
- ds_diff.addComment(owner, "I'm working on this.")
- ds_diff.addComment(owner, "Here's another comment.")
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ ds_diff.addComment(person, "I'm working on this.")
+ ds_diff.addComment(person, "Here's another comment.")
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
soup = BeautifulSoup(view())
@@ -392,10 +393,17 @@
1, len(soup.findAll('pre', text="Here's another comment.")))
def test_blacklist_options(self):
- # blacklist options are presented to editors.
+ # blacklist options are presented to the users with
+ # lp.View on the distroseries.
ds_diff = self.factory.makeDistroSeriesDifference()
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
+ self.assertTrue(
+ check_permission('launchpad.Edit', ds_diff))
+ self.assertTrue(
+ check_permission(
+ 'launchpad.View',
+ ds_diff.derived_series.parent))
request = LaunchpadTestRequest(
HTTP_X_REQUESTED_WITH='XMLHttpRequest')
view = create_initialized_view(
@@ -433,7 +441,8 @@
def test_package_diff_request_link(self):
# The link to compute package diffs is only shown to
- # a user with lp.Edit persmission.
+ # a user with lp.Edit permission (i.e. lp.View on the
+ # distribution).
ds_diff = self.factory.makeDistroSeriesDifference(
set_base_version=True)
package_diff_request_matcher = soupmatchers.HTMLContains(
@@ -443,12 +452,12 @@
'\s*Compute differences from last common version\s*')))
with person_logged_in(self.factory.makePerson()):
- view = create_initialized_view(
- ds_diff, '+listing-distroseries-extra')
- self.assertFalse(view.show_package_diffs_request_link)
- self.assertThat(view(), Not(package_diff_request_matcher))
-
- with celebrity_logged_in('admin'):
+ self.assertTrue(
+ check_permission('launchpad.Edit', ds_diff))
+ self.assertTrue(
+ check_permission(
+ 'launchpad.View',
+ ds_diff.derived_series.parent))
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra')
self.assertThat(view(), package_diff_request_matcher)
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2011-03-10 04:00:08 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2011-04-14 15:42:37 +0000
@@ -3,17 +3,13 @@
__metaclass__ = type
+from lazr.restfulclient.errors import BadRequest
import transaction
-
-from lazr.restfulclient.errors import (
- BadRequest,
- Unauthorized,
- )
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.webapp.publisher import canonical_url
from canonical.testing import AppServerLayer
-from canonical.launchpad.webapp.publisher import canonical_url
from lp.registry.enum import DistroSeriesDifferenceStatus
from lp.registry.interfaces.distroseriesdifference import (
IDistroSeriesDifferenceSource,
@@ -40,18 +36,11 @@
self.assertTrue(
ws_diff.self_link.endswith(ds_diff_path))
- def test_blacklist_not_public(self):
- # The blacklist method is not publically available.
- ds_diff = self.factory.makeDistroSeriesDifference()
- ws_diff = ws_object(self.factory.makeLaunchpadService(), ds_diff)
-
- self.assertRaises(Unauthorized, ws_diff.blacklist)
-
def test_blacklist(self):
# The blacklist method can be called by people with edit access.
ds_diff = self.factory.makeDistroSeriesDifference()
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.derived_series.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
result = ws_diff.blacklist()
transaction.commit()
@@ -63,20 +52,12 @@
DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
ds_diff.status)
- def test_unblacklist_not_public(self):
- # The unblacklist method is not publically available.
- ds_diff = self.factory.makeDistroSeriesDifference(
- status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
- ws_diff = ws_object(self.factory.makeLaunchpadService(), ds_diff)
-
- self.assertRaises(Unauthorized, ws_diff.unblacklist)
-
def test_unblacklist(self):
# The unblacklist method can be called by people with edit access.
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
result = ws_diff.unblacklist()
transaction.commit()
@@ -92,7 +73,7 @@
# Comments can be added via the API
ds_diff = self.factory.makeDistroSeriesDifference()
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
result = ws_diff.addComment(comment='Hey there')
@@ -118,7 +99,7 @@
'parent': parent_changelog,
})
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
result = ws_diff.requestPackageDiffs()
transaction.commit()
@@ -138,7 +119,7 @@
})
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
self.assertRaises(
BadRequest, ws_diff.requestPackageDiffs)
@@ -152,7 +133,7 @@
naked_dsdiff.parent_package_diff = self.factory.makePackageDiff()
ws_diff = ws_object(self.factory.makeLaunchpadService(
- ds_diff.owner), ds_diff)
+ self.factory.makePerson()), ds_diff)
self.assertIs(None, ws_diff.package_diff_url)
self.assertIsNot(None, ws_diff.parent_package_diff_url)
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2011-04-12 13:39:33 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2011-04-14 15:42:37 +0000
@@ -38,7 +38,6 @@
)
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.person import IPerson
-from lp.registry.interfaces.role import IHasOwner
from lp.registry.interfaces.sourcepackagename import ISourcePackageName
from lp.soyuz.enums import PackageDiffStatus
from lp.soyuz.interfaces.distroseriessourcepackagerelease import (
@@ -48,7 +47,7 @@
from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
-class IDistroSeriesDifferencePublic(IHasOwner, Interface):
+class IDistroSeriesDifferencePublic(Interface):
"""The public interface for distro series differences."""
id = Int(title=_('ID'), required=True, readonly=True)
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-04-13 14:44:31 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-04-14 15:42:37 +0000
@@ -9,7 +9,6 @@
from storm.store import Store
import transaction
from zope.component import getUtility
-from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.webapp.authorization import check_permission
@@ -301,9 +300,10 @@
ds_diff = self.factory.makeDistroSeriesDifference(
source_package_name_str="foonew")
- with person_logged_in(ds_diff.owner):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
dsd_comment = ds_diff.addComment(
- ds_diff.owner, "Wait until version 2.1")
+ person, "Wait until version 2.1")
self.assertEqual(ds_diff, dsd_comment.distro_series_difference)
@@ -312,31 +312,23 @@
# most recent comment first.
ds_diff = self.factory.makeDistroSeriesDifference()
- with person_logged_in(ds_diff.owner):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
dsd_comment = ds_diff.addComment(
- ds_diff.owner, "Wait until version 2.1")
+ person, "Wait until version 2.1")
dsd_comment_2 = ds_diff.addComment(
- ds_diff.owner, "Wait until version 2.1")
+ person, "Wait until version 2.1")
self.assertEqual(
[dsd_comment_2, dsd_comment], list(ds_diff.getComments()))
- def test_addComment_not_public(self):
- # Comments cannot be added with launchpad.View.
- ds_diff = self.factory.makeDistroSeriesDifference()
- person = self.factory.makePerson()
-
- with person_logged_in(person):
- self.assertTrue(check_permission('launchpad.View', ds_diff))
- self.assertFalse(check_permission('launchpad.Edit', ds_diff))
- self.assertRaises(Unauthorized, getattr, ds_diff, 'addComment')
-
def test_addComment_for_owners(self):
# Comments can be added by any of the owners of the derived
# series.
ds_diff = self.factory.makeDistroSeriesDifference()
- with person_logged_in(ds_diff.owner):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
self.assertTrue(check_permission('launchpad.Edit', ds_diff))
diff_comment = ds_diff.addComment(
ds_diff.derived_series.owner, "Boo")
@@ -371,21 +363,11 @@
sorted([packageset.name for packageset in packagesets]),
[packageset.name for packageset in ds_diff.getPackageSets()])
- def test_blacklist_not_public(self):
- # Differences cannot be blacklisted without edit access.
- ds_diff = self.factory.makeDistroSeriesDifference()
- person = self.factory.makePerson()
-
- with person_logged_in(person):
- self.assertTrue(check_permission('launchpad.View', ds_diff))
- self.assertFalse(check_permission('launchpad.Edit', ds_diff))
- self.assertRaises(Unauthorized, getattr, ds_diff, 'blacklist')
-
def test_blacklist_default(self):
# By default the current version is blacklisted.
ds_diff = self.factory.makeDistroSeriesDifference()
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.blacklist()
self.assertEqual(
@@ -396,7 +378,7 @@
# All versions are blacklisted with the all=True param.
ds_diff = self.factory.makeDistroSeriesDifference()
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.blacklist(all=True)
self.assertEqual(
@@ -408,7 +390,7 @@
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.unblacklist()
self.assertEqual(
@@ -429,7 +411,7 @@
status=PackagePublishingStatus.PENDING,
version='1.0')
- with person_logged_in(ds_diff.owner):
+ with person_logged_in(self.factory.makePerson()):
ds_diff.unblacklist()
self.assertEqual(
@@ -624,8 +606,9 @@
'parent': parent_changelog,
})
- with person_logged_in(ds_diff.owner):
- ds_diff.requestPackageDiffs(ds_diff.owner)
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ ds_diff.requestPackageDiffs(person)
self.assertEqual(
'1.2', ds_diff.package_diff.to_source.version)
@@ -654,8 +637,9 @@
'parent': parent_changelog,
})
- with person_logged_in(ds_diff.owner):
- ds_diff.requestPackageDiffs(ds_diff.owner)
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ ds_diff.requestPackageDiffs(person)
self.assertIs(None, ds_diff.package_diff)
self.assertIsNot(None, ds_diff.parent_package_diff)
@@ -674,11 +658,12 @@
'derived': changelog_lfa,
'parent': changelog_lfa,
})
- with person_logged_in(ds_diff.owner):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
self.assertRaisesWithContent(
DistroSeriesDifferenceError,
"Can not generate package diffs for a resolved difference.",
- ds_diff.requestPackageDiffs, ds_diff.owner)
+ ds_diff.requestPackageDiffs, person)
def test_package_diff_urls_none(self):
# URLs to the package diffs are only present when the diffs
