launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/localdiff-sync-bug-739525 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #739525 in Launchpad itself: "Connect the "sync" button on +localpackagediffs to some useful code"
  https://bugs.launchpad.net/launchpad/+bug/739525

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/localdiff-sync-bug-739525/+merge/54496

= Summary =
Derived Distros feature:
Connect the "Synchronise" button on the +localpackagediffs form to code that will actually perform the diff.

== Implementation details ==
The button was originally left doing nothing, with a stub function.  This is because we thought that we needed a job runner to do the synchronisation as package syncing is a fairly db-intensive operation.  However, recent improvements to the package copier infrastructure have made it much faster, so we want to try out this direct copying method inside a webapp request (which is exactly how the PPA copying page works).

The code does a very simple call to IArchive.syncSourceS() to perform the sync.  Most of the changes in the branch are test changes.

If we ever need to make this an asynchronous operation, we'll do it in tandem with the PPA copying code, but for now we'll see how it goes like this.

== Tests ==
bin/test -cvv test_series_views

== Demo and Q/A ==
See https://dogfood.launchpad.net/deribuntu/dangerous/+localpackagediffs
(the button doesn't do anything there yet but you can click it to get an idea of how it works)


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/testing/factory.py
  lib/lp/registry/browser/distroseries.py
  lib/lp/registry/browser/tests/test_series_views.py

./lib/lp/testing/factory.py
     265: E501 line too long (84 characters)
     265: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/localdiff-sync-bug-739525/+merge/54496
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/localdiff-sync-bug-739525 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-03-08 11:56:40 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-03-23 11:30:56 +0000
@@ -90,6 +90,9 @@
 from lp.services.worlddata.interfaces.country import ICountry
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.soyuz.browser.packagesearch import PackageSearchViewBase
+from lp.soyuz.interfaces.archive import (
+    CannotCopy,
+    )
 from lp.soyuz.interfaces.queue import IPackageUploadSet
 from lp.translations.browser.distroseries import (
     check_distroseries_translations_viewable,
@@ -618,19 +621,27 @@
     @action(_("Sync Sources"), name="sync", validator='validate_sync',
             condition='canPerformSync')
     def sync_sources(self, action, data):
-        """Mark the diffs as syncing and request the sync.
+        """Synchronise packages from the parent series to this one."""
+        # XXX We're doing a direct copy sync here as an interim measure
+        # until we work out if it's fast enough to work reliably.  If it
+        # isn't, we need to implement a way of flagging sources 'to be
+        # synced' and write a job runner to do it in the background.
 
-        Currently this is a stub operation, the details of which will
-        be implemented later.
-        """
         selected_differences = data['selected_differences']
         diffs = [
             diff.source_package_name.name
                 for diff in selected_differences]
 
-        self.request.response.addNotification(
-            "The following sources would have been synced if this "
-            "wasn't just a stub operation: " + ", ".join(diffs))
+        try:
+            self.context.main_archive.syncSources(
+                diffs, from_archive=self.context.parent_series.main_archive,
+                to_pocket='Release', to_series=self.context.name)
+        except CannotCopy, e:
+            self.request.response.addErrorNotification("Cannot copy: %s" % e)
+        else:
+            self.request.response.addNotification(
+                "The following sources were synchronized: " +
+                ", ".join(diffs))
 
         self.next_url = self.request.URL
 

=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2011-03-21 09:15:49 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2011-03-23 11:30:56 +0000
@@ -268,7 +268,7 @@
     layer = LaunchpadFunctionalLayer
 
     def test_batch_filtered(self):
-        # The name_filter parameter allows to filter packages by name.
+        # The name_filter parameter allows filtering of packages by name.
         set_derived_series_ui_feature_flag(self)
         derived_series = self.factory.makeDistroSeries(
             name='derilucid', parent_series=self.factory.makeDistroSeries(
@@ -346,19 +346,27 @@
                 derived_series, '+localpackagediffs')
             self.assertTrue(view.canPerformSync())
 
-    # XXX 2010-10-29 michaeln bug=668334
-    # The following three tests pass locally but there is a bug with
-    # per-thread features when running with a larger subset of tests.
-    # These should be re-enabled once the above bug is fixed.
-    def disabled_test_sync_notification_on_success(self):
+    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_series = self.factory.makeDistroSeries(
-            name='derilucid', parent_series=self.factory.makeDistroSeries(
-                name='lucid'))
+            name='derilucid', parent_series=parent_series)
         difference = self.factory.makeDistroSeriesDifference(
             source_package_name_str='my-src-name',
-            derived_series=derived_series)
-
+            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(
@@ -370,16 +378,25 @@
                     '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()
+        self.assertIsNot(None, pub)
+        self.assertEqual(versions['parent'], pub.sourcepackagerelease.version)
+
+        # The view should show no errors, and the notification should
+        # confirm the sync worked.
         self.assertEqual(0, len(view.errors))
         notifications = view.request.response.notifications
         self.assertEqual(1, len(notifications))
         self.assertEqual(
-            "The following sources would have been synced if this wasn't "
-            "just a stub operation: my-src-name",
+            "The following sources were synchronized: my-src-name",
             notifications[0].message)
+        # 302 is a redirect back to the same page.
         self.assertEqual(302, view.request.response.getStatus())
 
-    def disabled_test_sync_error_nothing_selected(self):
+    def test_sync_error_nothing_selected(self):
         # An error is raised when a sync is requested without any selection.
         derived_series = self.factory.makeDistroSeries(
             name='derilucid', parent_series=self.factory.makeDistroSeries(
@@ -401,7 +418,7 @@
         self.assertEqual(
             'No differences selected.', view.errors[0])
 
-    def disabled_test_sync_error_invalid_selection(self):
+    def test_sync_error_invalid_selection(self):
         # An error is raised when an invalid difference is selected.
         derived_series = self.factory.makeDistroSeries(
             name='derilucid', parent_series=self.factory.makeDistroSeries(

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-03-18 10:31:56 +0000
+++ lib/lp/testing/factory.py	2011-03-23 11:30:56 +0000
@@ -245,6 +245,7 @@
     PackagePublishingPriority,
     PackagePublishingStatus,
     PackageUploadStatus,
+    SourcePackageFormat,
     )
 from lp.soyuz.interfaces.archive import (
     default_name_by_purpose,
@@ -261,6 +262,7 @@
     BinaryPackageFile,
     SourcePackageReleaseFile,
     )
+from lp.soyuz.interfaces.sourcepackageformat import ISourcePackageFormatSelectionSet
 from lp.soyuz.model.packagediff import PackageDiff
 from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.testing import (
@@ -2266,6 +2268,13 @@
             description=self.getUniqueString(),
             parent_series=parent_series, owner=distribution.owner)
         series.status = status
+
+        # Set up source package format selection so that copying will
+        # work with the default dsc_format used in
+        # makeSourcePackageRelease.
+        getUtility(ISourcePackageFormatSelectionSet).add(
+            series, SourcePackageFormat.FORMAT_1_0)
+
         return ProxyFactory(series)
 
     def makeUbuntuDistroRelease(self, version=None,