Merge lp:~julian-edwards/launchpad/localdiff-sync-bug-739525 into lp:launchpad

[Julian Edwards]

[r=danilo][bug=739525] Derived Distros: Connect the sync button on +localpackagediffs to code that will actually do the sync

= 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.

[Данило Шеган (danilo)]

<danilos> bigjools, branch sounds like a nice feature, but you've got a single lint issue so please fix it (I understand it may not be from your changes :)
 looking at the code now
<bigjools> :/
<bigjools> it was my lint
<danilos> bigjools, I am sad to see a message like "The following sources *would have been* synced if..." go from the real UI
<bigjools> :)
<wgrant> bigjools: Have you worked out how permissions are going to work?
<bigjools> no
<wgrant> Heh
<danilos> bigjools, r=me, but I'd also remove the "XXX" from a comment about a thing you are not really going to fix unless it proves to be broken

review: Approve

=== 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 17:35:11 +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."""
+        # 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 17:35:11 +0000
@@ -33,6 +33,12 @@
     getFeatureFlag,
     install_feature_controller,
     )
+from lp.soyuz.interfaces.sourcepackageformat import (
+    ISourcePackageFormatSelectionSet,
+    )
+from lp.soyuz.enums import (
+    SourcePackageFormat,
+    )
 from lp.testing import (
     TestCaseWithFactory,
     login_person,
@@ -267,8 +273,15 @@
 
     layer = LaunchpadFunctionalLayer
 
+    def _set_source_selection(self, series):
+        # 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)
+
     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 +359,28 @@
                 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)
+        self._set_source_selection(derived_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 +392,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,11 +432,12 @@
         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(
                 name='lucid'))
+        self._set_source_selection(derived_series)
         difference = self.factory.makeDistroSeriesDifference(
             source_package_name_str='my-src-name',
             derived_series=derived_series)

=== 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 17:35:11 +0000
@@ -2266,6 +2266,7 @@
             description=self.getUniqueString(),
             parent_series=parent_series, owner=distribution.owner)
         series.status = status
+
         return ProxyFactory(series)
 
     def makeUbuntuDistroRelease(self, version=None,