Looks fine, I have a few comments in the diff below. > === modified file 'lib/lp/registry/browser/tests/test_distroseries.py' > --- lib/lp/registry/browser/tests/test_distroseries.py 2011-05-05 05:22:20 +0000 > +++ lib/lp/registry/browser/tests/test_distroseries.py 2011-05-05 15:21:22 +0000 > @@ -27,10 +27,10 @@ > from zope.security.proxy import removeSecurityProxy > > from canonical.config import config > -from canonical.database.constants import UTC_NOW > from canonical.database.sqlbase import flush_database_caches > from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities > from canonical.launchpad.testing.pages import find_tag_by_id > +from canonical.launchpad.webapp.authorization import check_permission > from canonical.launchpad.webapp.batching import BatchNavigator > from canonical.launchpad.webapp.publisher import canonical_url > from canonical.testing.layers import ( > @@ -48,8 +48,7 @@ > DistroSeriesDifferenceStatus, > DistroSeriesDifferenceType, > ) > -from lp.registry.interfaces.pocket import PackagePublishingPocket > -from lp.registry.interfaces.series import SeriesStatus > +from lp.registry.interfaces.person import IPersonSet > from lp.services.features import ( > get_relevant_feature_controller, > getFeatureFlag, > @@ -61,15 +60,18 @@ > getFeatureStore, > ) > from lp.soyuz.enums import ( > + ArchivePermissionType, > PackagePublishingStatus, > SourcePackageFormat, > ) > +from lp.soyuz.interfaces.component import IComponentSet > from lp.soyuz.interfaces.distributionjob import ( > IInitialiseDistroSeriesJobSource, > ) > from lp.soyuz.interfaces.sourcepackageformat import ( > ISourcePackageFormatSelectionSet, > ) > +from lp.soyuz.model.archivepermission import ArchivePermission > from lp.testing import ( > celebrity_logged_in, > feature_flags, > @@ -113,7 +115,7 @@ > # Helper function to create a valid DSD. > distroseries = self.factory.makeDistroSeries( > parent_series=self.factory.makeDistroSeries()) > - ds_diff = self.factory.makeDistroSeriesDifference( > + self.factory.makeDistroSeriesDifference( > derived_series=distroseries, difference_type=difference_type) > return create_initialized_view(distroseries, '+index') > > @@ -176,11 +178,11 @@ > derived_series, > '+index', > principal=self.simple_user) > - html = view() > + html_content = view() > > self.assertEqual( > None, getFeatureFlag('soyuz.derived-series-ui.enabled')) > - self.assertThat(html, Not(portlet_header)) > + self.assertThat(html_content, Not(portlet_header)) > > def test_differences_portlet_all_differences(self): > # The difference portlet shows the differences with the parent > @@ -212,9 +214,9 @@ > # XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides > # self.features to NullFeatureController. > view.request.features = get_relevant_feature_controller() > - html = view() > + html_content = view() > > - self.assertThat(html, portlet_display) > + self.assertThat(html_content, portlet_display) > > def test_differences_portlet_no_differences(self): > # The difference portlet displays 'No differences' if there is no > @@ -238,9 +240,9 @@ > # XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides > # self.features to NullFeatureController. > view.request.features = get_relevant_feature_controller() > - html = view() > + html_content = view() > > - self.assertThat(html, portlet_display) > + self.assertThat(html_content, portlet_display) > > def test_differences_portlet_initialising(self): > # The difference portlet displays 'The series is initialising.' if > @@ -248,7 +250,7 @@ > set_derived_series_ui_feature_flag(self) > derived_series = self._setupDifferences('deri', 'sid', 0, 0, 0) > job_source = getUtility(IInitialiseDistroSeriesJobSource) > - job = job_source.create(derived_series.parent, derived_series) > + job_source.create(derived_series.parent, derived_series) > portlet_display = soupmatchers.HTMLContains( > soupmatchers.Tag( > 'Derived series', 'h2', > @@ -266,10 +268,10 @@ > # XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides > # self.features to NullFeatureController. > view.request.features = get_relevant_feature_controller() > - html = view() > + html_content = view() > > self.assertTrue(derived_series.is_initialising) > - self.assertThat(html, portlet_display) > + self.assertThat(html_content, portlet_display) > > > class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory): > @@ -508,9 +510,13 @@ > # fairly static. However, with some DistroSeriesDifferences the query > # count will be higher, but it should remain the same no matter how > # many differences there are. > - login_person(self.simple_user) > derived_series = self.factory.makeDistroSeries( > parent_series=self.factory.makeDistroSeries()) > + ArchivePermission( > + archive=derived_series.main_archive, person=self.simple_user, > + component=getUtility(IComponentSet)["main"], > + permission=ArchivePermissionType.QUEUE_ADMIN) > + login_person(self.simple_user) > > def add_differences(num): > for index in xrange(num): > @@ -1005,12 +1011,14 @@ > [resolved_diff], filtered_view.cached_differences.batch) > > def _setUpDSD(self, src_name='src-name', versions=None, > - difference_type=None): > + difference_type=None, distribution=None): > # Helper to create a derived series with fixed names and proper > # source package format selection along with a DSD. > parent_series = self.factory.makeDistroSeries(name='warty') > + if distribution == None: > + distribution = self.factory.makeDistribution('deribuntu') > derived_series = self.factory.makeDistroSeries( > - distribution=self.factory.makeDistribution(name='deribuntu'), > + distribution=distribution, > name='derilucid', parent_series=parent_series) > self._set_source_selection(derived_series) > self.factory.makeDistroSeriesDifference( > @@ -1022,33 +1030,45 @@ > set_derived_series_ui_feature_flag(self) > return derived_series, parent_series, sourcepackagename > > - def test_canPerformSync_non_editor(self): > - # Non-editors do not see options to sync. > - derived_series = self.factory.makeDistroSeries( > - name='derilucid', parent_series=self.factory.makeDistroSeries( > - name='lucid')) > - self.factory.makeDistroSeriesDifference( > - derived_series=derived_series) > - > - set_derived_series_ui_feature_flag(self) > + def test_canPerformSync_anon(self): > + # Anonymous users cannot sync packages. > + derived_series, _, _ = self._setUpDSD() > + view = create_initialized_view( > + derived_series, '+localpackagediffs') > + > + self.assertFalse(view.canPerformSync()) > + > + def test_canPerformSync_non_anon_no_perm_dest_archive(self): > + # Logged-in users with no permission on the destination archive > + # are not presented with options to perform syncs. > + derived_series, _, _ = self._setUpDSD() > with person_logged_in(self.factory.makePerson()): > view = create_initialized_view( > derived_series, '+localpackagediffs') derived_series, _, _ = self._setUpDSD() scans a little weirdly. You might try derived_series = self._setUpDSD()[0] or (derived_series, ignore1, ignore2) = self._setUpDSD() > > - self.assertFalse(view.canPerformSync()) > - > - def test_canPerformSync_editor(self): > - # Editors are presented with options to perform syncs. > - derived_series = self.factory.makeDistroSeries( > - name='derilucid', parent_series=self.factory.makeDistroSeries( > - name='lucid')) > - self.factory.makeDistroSeriesDifference( > - derived_series=derived_series) > - > - set_derived_series_ui_feature_flag(self) > - with person_logged_in(derived_series.owner): > + self.assertFalse(view.canPerformSync()) > + > + def _setUpPersonWithPerm(self, derived_series): > + # Helper to create a person with an upload permission on the > + # series' archive. > + person = self.factory.makePerson() > + ArchivePermission( > + archive=derived_series.main_archive, person=person, > + component=getUtility(IComponentSet)["main"], > + permission=ArchivePermissionType.QUEUE_ADMIN) > + return person > + > + def test_canPerformSync_non_anon(self): > + # Logged-in users with a permission on the destination archive > + # are presented with options to perform syncs. > + # Note that a more fine-grained perm check is done on each > + # synced package. > + derived_series, _, _ = self._setUpDSD() > + person = self._setUpPersonWithPerm(derived_series) > + with person_logged_in(person): > view = create_initialized_view( > derived_series, '+localpackagediffs') > + > self.assertTrue(view.canPerformSync()) > > def _syncAndGetView(self, derived_series, person, sync_differences, > @@ -1063,136 +1083,179 @@ > }) > return view > > - def test_sync_notification_on_success(self): > - # Syncing one or more diffs results in a stub notification. > - versions = { > - 'base': '1.0', > - 'derived': '1.0derived1', > - 'parent': '1.0-1', > - } > - parent_series = self.factory.makeDistroSeries(name='warty') > - derived_distro = self.factory.makeDistribution(name='deribuntu') > - derived_series = self.factory.makeDistroSeries( > - distribution=derived_distro, name='derilucid', > - parent_series=parent_series) > - self._set_source_selection(derived_series) > - difference = self.factory.makeDistroSeriesDifference( > - source_package_name_str='my-src-name', > - derived_series=derived_series, versions=versions) > - > - # The inital state is that 1.0-1 is not in the derived series. > - pubs = derived_series.main_archive.getPublishedSources( > - name='my-src-name', version=versions['parent'], > - distroseries=derived_series).any() > - self.assertIs(None, pubs) > - > - # Now, sync the source from the parent using the form. > - set_derived_series_ui_feature_flag(self) > - with person_logged_in(derived_series.owner): > - view = create_initialized_view( > - derived_series, '+localpackagediffs', > - method='POST', form={ > - 'field.selected_differences': [ > - difference.source_package_name.name, > - ], > - 'field.actions.sync': 'Sync', > - }) > - > - # The parent's version should now be in the derived series: > - pub = derived_series.main_archive.getPublishedSources( > - name='my-src-name', version=versions['parent'], > - distroseries=derived_series).one() > + def test_sync_error_nothing_selected(self): > + # An error is raised when a sync is requested without any selection. > + derived_series, _, _ = self._setUpDSD() > + person = self._setUpPersonWithPerm(derived_series) > + view = self._syncAndGetView(derived_series, person, []) > + > + self.assertEqual(1, len(view.errors)) > + self.assertEqual( > + 'No differences selected.', view.errors[0]) > + > + def test_sync_error_invalid_selection(self): > + # An error is raised when an invalid difference is selected. > + derived_series, _, _ = self._setUpDSD('my-src-name') > + person = self._setUpPersonWithPerm(derived_series) > + view = self._syncAndGetView( > + derived_series, person, ['some-other-name']) > + > + self.assertEqual(2, len(view.errors)) > + self.assertEqual( > + 'No differences selected.', view.errors[0]) > + self.assertEqual( > + 'Invalid value', view.errors[1].error_name) > + > + def test_sync_error_no_perm_dest_archive(self): > + # A user without upload rights on the destination archive cannot > + # sync packages. > + derived_series, _, _ = self._setUpDSD('my-src-name') > + person = self._setUpPersonWithPerm(derived_series) > + view = self._syncAndGetView( > + derived_series, person, ['my-src-name']) > + > + self.assertEqual(1, len(view.errors)) > + self.assertTrue( > + "The signer of this package has no upload rights to this " > + "distribution's primary archive" in view.errors[0]) > + > + def makePersonWithComponentPermission(self, archive, component=None): > + person = self.factory.makePerson() > + if component is None: > + component = self.factory.makeComponent() > + removeSecurityProxy(archive).newComponentUploader( > + person, component) > + return person, component > + > + def test_sync_success_perm_component(self): > + # A user with upload rights on the destination component > + # can sync packages. > + derived_series, parent_series, sourcepackagename = self._setUpDSD( > + 'my-src-name') > + person, _ = self.makePersonWithComponentPermission( > + derived_series.main_archive, > + derived_series.getSourcePackage( > + sourcepackagename).latest_published_component) > + view = self._syncAndGetView( > + derived_series, person, ['my-src-name']) > + > + self.assertEqual(0, len(view.errors)) > + > + def test_sync_error_no_perm_component(self): > + # A user without upload rights on the destination component > + # will get an error when he syncs packages to this component. > + derived_series, parent_series, sourcepackagename = self._setUpDSD( > + 'my-src-name') > + person, another_component = self.makePersonWithComponentPermission( > + derived_series.main_archive) > + view = self._syncAndGetView( > + derived_series, person, ['my-src-name']) > + > + self.assertEqual(1, len(view.errors)) > + self.assertTrue( > + "Signer is not permitted to upload to the " > + "component" in view.errors[0]) > + > + def assertPackageCopied(self, series, src_name, version, view): > + # Helper to check that a package has been copied. > + # The new version should now be in the destination series: > + pub = series.main_archive.getPublishedSources( > + name=src_name, version=version, > + distroseries=series).one() > self.assertIsNot(None, pub) > - self.assertEqual(versions['parent'], pub.sourcepackagerelease.version) > + self.assertEqual(version, pub.sourcepackagerelease.version) > > # The view should show no errors, and the notification should > - # confirm the sync worked. > + # confirm the sync worked: I think the period was actually the right punctuation here. > === modified file 'lib/lp/soyuz/interfaces/archive.py' > --- lib/lp/soyuz/interfaces/archive.py 2011-05-05 00:47:27 +0000 > +++ lib/lp/soyuz/interfaces/archive.py 2011-05-05 15:21:22 +0000 > @@ -616,6 +616,9 @@ > :param component: The `Component` being uploaded to. > :param pocket: The `PackagePublishingPocket` of 'distroseries' being > uploaded to. > + :param strict_component: True if access to the specific component for > + the package is needed to upload to it. If False, then access to > + any component will do. > :return: The reason for not being able to upload, None otherwise. > """ > > @@ -632,7 +635,7 @@ > :param distroseries: The upload's target distro series. > :param strict_component: True if access to the specific component for > the package is needed to upload to it. If False, then access to > - any package will do. > + any component will do. > :return: CannotUploadToArchive if 'person' cannot upload to the > archive, > None otherwise. > @@ -1173,12 +1176,22 @@ > @operation_returns_collection_of(Interface) > @export_read_operation() > def getComponentsForQueueAdmin(person): > - """Return `IArchivePermission` for the person's queue admin components > + """Return `IArchivePermission` for the person's queue admin > + components. > > - :param person: An `IPerson` > + :param person: An `IPerson`. > :return: A list of `IArchivePermission` records. > """ > > + def hasAnyPermission(person): > + """Whether or not this person has any permission at all on this > + archive. > + > + :param person: The `IPerson` for whom the check is performed. > + :return: A boolean indicating if the person has any permission on this > + archive at all. > + """ > + > def getPackageDownloadCount(bpr, day, country): > """Get the `IBinaryPackageDownloadCount` with the given key.""" > > @@ -1212,6 +1225,7 @@ > class IArchiveAppend(Interface): > """Archive interface for operations restricted by append privilege.""" > > + @call_with(person=REQUEST_USER) > @operation_parameters( > source_names=List( > title=_("Source package names"), > @@ -1227,8 +1241,8 @@ > @export_write_operation() > # Source_names is a string because exporting a SourcePackageName is > # rather nonsensical as it only has id and name columns. > - def syncSources(source_names, from_archive, to_pocket, > - to_series=None, include_binaries=False): > + def syncSources(source_names, from_archive, to_pocket, to_series=None, > + include_binaries=False, person=None): > """Synchronise (copy) named sources into this archive from another. > > It will copy the most recent PUBLISHED versions of the named > @@ -1246,6 +1260,7 @@ > :param include_binaries: optional boolean, controls whether or not > the published binaries for each given source should also be > copied along with the source. > + :param person: the `IPerson` who requests the sync. > > :raises NoSuchSourcePackageName: if the source name is invalid > :raises PocketNotFound: if the pocket name is invalid > @@ -1253,6 +1268,7 @@ > :raises CannotCopy: if there is a problem copying. > """ > > + @call_with(person=REQUEST_USER) > @operation_parameters( > source_name=TextLine(title=_("Source package name")), > version=TextLine(title=_("Version")), > @@ -1271,7 +1287,7 @@ > # we should consider either changing this method or adding a new one > # that takes that object instead. > def syncSource(source_name, version, from_archive, to_pocket, > - to_series=None, include_binaries=False): > + to_series=None, include_binaries=False, person=None): > """Synchronise (copy) a single named source into this archive. > > Copy a specific version of a named source to the destination > @@ -1285,6 +1301,7 @@ > :param include_binaries: optional boolean, controls whether or not > the published binaries for each given source should also be > copied along with the source. > + :param person: the `IPerson` who requests the sync. > > :raises NoSuchSourcePackageName: if the source name is invalid > :raises PocketNotFound: if the pocket name is invalid It looks to me like syncSource isn't necessary; couldn't you just pass one source_name to syncSources? Obviously this isn't a problem in your branch, but it may be worth filing a bug to capture the issue?