Merge lp:~michael.nelson/launchpad/652838-select-diffs-for-syncing into lp:launchpad/db-devel

[Michael Nelson]

Allow multiple differences to be selected for syncing.

Overview
========
This branch fixes bug 652838 by adding (non-js) check boxes and the sync button for syncing differences.

Note: it does not add the actual sync operation itself, but just a stub action. Julian will be organising the actual operation at at later point (obviously before the feature is enabled).

UI demo
=======
http://people.canonical.com/~michaeln/tmp/652838-select-diffs.ogv

mockup:
https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png

To demo locally:
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs

Details
=======
I had 2 issues, and one ongoing:

1) There doesn't seem to be a natural way to have a dynamic name for the action as required by the prototype at:

https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png

You'll see the solution in the view's initialize() method, but any better options would be gratefully accepted.

2) There doesn't seem to be a natural way to have dynamic vocabularies based on the view. I've looked at various ways that we've done this in the past, and none are ideal.

In translations a vocabulary factory is used (see lp.translations.browser.translationimportqueue.TranslationImportQueueView), but vocabulary factories are designed to be based on the context, not the view (they implement IContextSourceBinder), so translations adds an __init__ kwarg, which means the vocabulary factory can't be instantiated in the interface description as it normally would, which means no LaunchpadFormView with its schema etc.

In soyuz an extra field and widget is added to the form during view initialisation (ie. one that is not on the schema, see lp.soyuz.browser.archive.ArchiveSourceSelectionFormView)

I decided instead to include the field on the schema with an empty vocabulary, and then update the vocabulary during the view's setUpFields() call. But again, any better ideas are welcome.

3) Finally, two tests where I render the view and check tags are failing, it seems, because the traversal request/lp:person is returning None:
http://pastebin.ubuntu.com/506335/

I'm still looking into why this is. So far, tracked it down to the fact that, even within the person_logged_in context manager, the tests view.request.principal is zope.principalregistry.principalregistry.UnauthenticatedPrincipal.

To test
=======

bin/test -vvm test_series_views

I'll upload a quick screencast soon.

[Jeroen T. Vermeulen (jtv)]

It's just a crying shame that you have to go to these lengths to include dynamic information in your view. I looked for alternatives but didn't find any.

You mentioned that you'd talk to the zope people; I think that's a good idea. If only Zope's @action accepted a callable as an alternative to a string!

review: Approve (code)

[Guilherme Salgado (salgado)]

The UI changes look good to me. I think it'd be nice to have an extra checkbox for selecting all/none versions, though.

review: Approve (ui*)

[Michael Nelson (michael.nelson)]

> The UI changes look good to me. I think it'd be nice to have an extra
> checkbox for selecting all/none versions, though.

salgado: thanks. And yes, the all/none checkbox is included on the mockup, I hope to add it in a later branch, but it's not a priority right now.

[Curtis Hovey (sinzui)]

The behaviour is nice. Buttons should be in title case (except for subordinating conjunctions and commonly used prepositions):
    Sync selected %s versions into => Sync Selected %s Versions into %s

This may look odd since the interpolated names may not be title case, but the user did that, not us.

review: Approve (ui)

[Leonard Richardson (leonardr)]

FYI, I just did a branch with dynamically-named buttons:

https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/37590

Basically, in the action code I created my own Actions object and copied in new Action objects. My Actions object was based on a class Actions object containing generic actions.

[Gary Poster (gary)]

I asked Leonard to comment on your (1), which he did, above. formlib actions are unpleasantly limited.

For (2), context-based vocabularies are what I expected you to use, but I see they are not sufficient for some reason. Without looking at the code more carefully, I don't know why. In their original usage, views are expected to be only a presentation of the context/model, so the context should be sufficient. I'm not sure why views are more than a presentation here, but would be interested to hear why. For the short term though, unless there's a nice way to refactor to actually use the context, hacks are the order of the day. :-(

[Michael Nelson (michael.nelson)]

On Tue, Oct 5, 2010 at 6:18 PM, Gary Poster <email address hidden> wrote:
> I asked Leonard to comment on your (1), which he did, above.  formlib actions are unpleasantly limited.

Thanks Gary and Leonard.

>
> For (2), context-based vocabularies are what I expected you to use, but I see they are not sufficient for some reason.  Without looking at the code more carefully, I don't know why.  In their original usage, views are expected to be only a presentation of the context/model, so the context should be sufficient.  I'm not sure why views are more than a presentation here, but would be interested to hear why.  For the short term though, unless there's a nice way to refactor to actually use the context, hacks are the order of the day. :-(

In this case, the context is a distro series, the view is displaying a
batched (and soon to be filtered) list of items related to that
distroseries, which can be selected for an operation. Another similar
situation that comes to mind is copying packages for an archive. The
view's context is the archive, but the view lists the related batched
(and filtered) packages.

In both cases we've manually created a field with a vocabulary
matching the items listed on screen so that validation and the action
can only operate on the current batch of items. Does that make sense,
or is there a better way?

> --
> https://code.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
> You are the owner of lp:~michael.nelson/launchpad/652838-select-diffs-for-syncing.
>

=== modified file 'lib/canonical/launchpad/templates/launchpad-form.pt'
--- lib/canonical/launchpad/templates/launchpad-form.pt	2010-06-22 17:12:23 +0000
+++ lib/canonical/launchpad/templates/launchpad-form.pt	2010-10-05 15:06:16 +0000
@@ -76,9 +76,10 @@
       </div>
       <div class="actions" id="launchpad-form-actions"
            metal:define-slot="buttons">
-        <input tal:repeat="action view/actions"
-               tal:replace="structure action/render"
-          />
+        <tal:actions repeat="action view/actions">
+          <input tal:replace="structure action/render"
+                 tal:condition="action/available"/>
+        </tal:actions>
         <tal:has-cancel-link condition="view/cancel_url">
           <tal:or condition="view/has_available_actions">or&nbsp;</tal:or>
           <a tal:attributes="href view/cancel_url">Cancel</a>

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2010-09-21 08:42:29 +0000
+++ lib/lp/registry/browser/distroseries.py	2010-10-05 15:06:16 +0000
@@ -21,8 +21,12 @@
 from zope.component import getUtility
 from zope.event import notify
 from zope.formlib import form
+from zope.interface import Interface
 from zope.lifecycleevent import ObjectCreatedEvent
-from zope.schema import Choice
+from zope.schema import (
+    Choice,
+    List,
+    )
 from zope.schema.vocabulary import (
     SimpleTerm,
     SimpleVocabulary,
@@ -59,6 +63,7 @@
     stepthrough,
     stepto,
     )
+from canonical.widgets import LabeledMultiCheckBoxWidget
 from canonical.widgets.itemswidgets import LaunchpadDropdownWidget
 from lp.app.errors import NotFoundError
 from lp.blueprints.browser.specificationtarget import (
@@ -536,8 +541,18 @@
         return navigator
 
 
-class DistroSeriesLocalDifferences(LaunchpadView):
+class IDifferencesFormSchema(Interface):
+    selected_differences = List(
+        title=_('Selected differences'),
+        value_type=Choice(vocabulary=SimpleVocabulary([])),
+        description=_("Select the differences for syncing."),
+        required=True)
+
+
+class DistroSeriesLocalDifferences(LaunchpadFormView):
     """Present differences between a derived series and its parent."""
+    schema = IDifferencesFormSchema
+    custom_widget('selected_differences', LabeledMultiCheckBoxWidget)
 
     page_title = 'Local package differences'
 
@@ -546,6 +561,13 @@
         if not getFeatureFlag('soyuz.derived-series-ui.enabled'):
             self.request.response.redirect(canonical_url(self.context))
             return
+
+        # Update the label for sync action.
+        self.__class__.actions.byname['actions.sync'].label = (
+            "Sync Selected %s Versions into %s" % (
+                self.context.parent_series.displayname,
+                self.context.displayname,
+                ))
         super(DistroSeriesLocalDifferences, self).initialize()
 
     @property
@@ -563,3 +585,55 @@
         utility = getUtility(IDistroSeriesDifferenceSource)
         differences = utility.getForDistroSeries(self.context)
         return BatchNavigator(differences, self.request)
+
+    def setUpFields(self):
+        """Add the selected differences field.
+
+        As this field depends on other search/filtering field values
+        for its own vocabulary, we set it up after all the others.
+        """
+        super(DistroSeriesLocalDifferences, self).setUpFields()
+        has_edit = check_permission('launchpad.Edit', self.context)
+
+        terms = [
+            SimpleTerm(diff, diff.source_package_name.name,
+                diff.source_package_name.name)
+                for diff in self.cached_differences.batch]
+        diffs_vocabulary = SimpleVocabulary(terms)
+        choice = self.form_fields['selected_differences'].field.value_type
+        choice.vocabulary = diffs_vocabulary
+
+    @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.
+
+        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))
+
+        self.next_url = self.request.URL
+
+    def validate_sync(self, action, data):
+        """Validate selected differences."""
+        form.getWidgetsData(self.widgets, self.prefix, data)
+
+        if len(data.get('selected_differences', [])) == 0:
+            self.setFieldError(
+                'selected_differences', 'No differences selected.')
+
+    def canPerformSync(self, *args):
+        """Return whether a sync can be performed.
+
+        This method is used as a condition for the above sync action, as
+        well as directly in the template.
+        """
+        return check_permission('launchpad.Edit', self.context)

=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2010-09-21 15:52:01 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2010-10-05 15:06:16 +0000
@@ -1,6 +1,8 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 from BeautifulSoup import BeautifulSoup
@@ -14,7 +16,10 @@
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.launchpad.webapp.publisher import canonical_url
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+    LaunchpadZopelessLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
@@ -28,7 +33,10 @@
     getFeatureFlag,
     per_thread,
     )
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
 from lp.testing.views import create_initialized_view
 
 
@@ -61,40 +69,35 @@
         self.assertEqual(view.needs_linking, None)
 
 
+def set_derived_series_ui_feature_flag(test_case):
+    # Helper to set the feature flag enabling the derived series ui.
+    ignore = getFeatureStore().add(FeatureFlag(
+        scope=u'default', flag=u'soyuz.derived-series-ui.enabled',
+        value=u'on', priority=1))
+
+    # XXX Michael Nelson 2010-09-21 bug=631884
+    # Currently LaunchpadTestRequest doesn't set per-thread
+    # features.
+    def in_scope(value):
+        return True
+    per_thread.features = FeatureController(in_scope)
+
+    def reset_per_thread_features():
+        per_thread.features = None
+    test_case.addCleanup(reset_per_thread_features)
+
+
 class DistroSeriesLocalPackageDiffsTestCase(TestCaseWithFactory):
     """Test the distroseries +localpackagediffs view."""
 
     layer = LaunchpadZopelessLayer
 
-    def makeDerivedSeries(self, derived_name=None, parent_name=None):
-        # Helper that creates a derived distro series.
-        parent = self.factory.makeDistroSeries(name=parent_name)
-        derived_series = self.factory.makeDistroSeries(
-            name=derived_name, parent_series=parent)
-        return derived_series
-
-    def setDerivedSeriesUIFeatureFlag(self):
-        # Helper to set the feature flag enabling the derived series ui.
-        ignore = getFeatureStore().add(FeatureFlag(
-            scope=u'default', flag=u'soyuz.derived-series-ui.enabled',
-            value=u'on', priority=1))
-
-        # XXX Michael Nelson 2010-09-21 bug=631884
-        # Currently LaunchpadTestRequest doesn't set per-thread
-        # features.
-        def in_scope(value):
-            return True
-        per_thread.features = FeatureController(in_scope)
-
-        def reset_per_thread_features():
-            per_thread.features = None
-        self.addCleanup(reset_per_thread_features)
-
     def test_view_redirects_without_feature_flag(self):
         # If the feature flag soyuz.derived-series-ui.enabled is not set the
         # view simply redirects to the derived series.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
 
         self.assertIs(
             None, getFeatureFlag('soyuz.derived-series-ui.enabled'))
@@ -108,8 +111,9 @@
 
     def test_label(self):
         # The view label includes the names of both series.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
 
         view = create_initialized_view(
             derived_series, '+localpackagediffs')
@@ -122,8 +126,9 @@
     def test_batch_includes_needing_attention_only(self):
         # The differences attribute includes differences needing
         # attention only.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
         current_difference = self.factory.makeDistroSeriesDifference(
             derived_series=derived_series)
         old_difference = self.factory.makeDistroSeriesDifference(
@@ -138,8 +143,9 @@
 
     def test_batch_includes_different_versions_only(self):
         # The view contains differences of type DIFFERENT_VERSIONS only.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
         different_versions_diff = self.factory.makeDistroSeriesDifference(
             derived_series=derived_series)
         unique_diff = self.factory.makeDistroSeriesDifference(
@@ -155,10 +161,11 @@
 
     def test_template_includes_help_link(self):
         # The help link for popup help is included.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
 
-        self.setDerivedSeriesUIFeatureFlag()
+        set_derived_series_ui_feature_flag(self)
         view = create_initialized_view(
             derived_series, '+localpackagediffs')
 
@@ -169,14 +176,15 @@
 
     def test_diff_row_includes_last_comment_only(self):
         # The most recent comment is rendered for each difference.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
         difference = self.factory.makeDistroSeriesDifference(
             derived_series=derived_series)
         difference.addComment(difference.owner, "Earlier comment")
         difference.addComment(difference.owner, "Latest comment")
 
-        self.setDerivedSeriesUIFeatureFlag()
+        set_derived_series_ui_feature_flag(self)
         view = create_initialized_view(
             derived_series, '+localpackagediffs')
 
@@ -192,12 +200,13 @@
 
     def test_diff_row_links_to_extra_details(self):
         # The source package name links to the difference details.
-        derived_series = self.makeDerivedSeries(
-            parent_name='lucid', derived_name='derilucid')
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
         difference = self.factory.makeDistroSeriesDifference(
             derived_series=derived_series)
 
-        self.setDerivedSeriesUIFeatureFlag()
+        set_derived_series_ui_feature_flag(self)
         view = create_initialized_view(
             derived_series, '+localpackagediffs')
         soup = BeautifulSoup(view())
@@ -210,6 +219,115 @@
         self.assertEqual(difference.source_package_name.name, links[0].string)
 
 
+class DistroSeriesLocalPackageDiffsFunctionalTestCase(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    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'))
+        difference = self.factory.makeDistroSeriesDifference(
+            derived_series=derived_series)
+
+        set_derived_series_ui_feature_flag(self)
+        with person_logged_in(self.factory.makePerson()):
+            view = create_initialized_view(
+                derived_series, '+localpackagediffs')
+
+        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'))
+        difference = self.factory.makeDistroSeriesDifference(
+            derived_series=derived_series)
+
+        set_derived_series_ui_feature_flag(self)
+        with person_logged_in(derived_series.owner):
+            view = create_initialized_view(
+                derived_series, '+localpackagediffs')
+            self.assertTrue(view.canPerformSync())
+
+    def test_sync_notification_on_success(self):
+        # Syncing one or more diffs results in a stub notification.
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
+        difference = self.factory.makeDistroSeriesDifference(
+            source_package_name_str='my-src-name',
+            derived_series=derived_series)
+
+        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',
+                    })
+
+        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",
+            notifications[0].message)
+        self.assertEqual(302, view.request.response.getStatus())
+
+    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(
+                name='lucid'))
+        difference = self.factory.makeDistroSeriesDifference(
+            source_package_name_str='my-src-name',
+            derived_series=derived_series)
+
+        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': [],
+                    'field.actions.sync': 'Sync',
+                    })
+
+        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.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
+        difference = self.factory.makeDistroSeriesDifference(
+            source_package_name_str='my-src-name',
+            derived_series=derived_series)
+
+        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': ['some-other-name'],
+                    'field.actions.sync': 'Sync',
+                    })
+
+        self.assertEqual(2, len(view.errors))
+        self.assertEqual(
+            'No differences selected.', view.errors[0])
+        self.assertEqual(
+            'Invalid value', view.errors[1].error_name)
+
+
 class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
     """Test the series.milestone_batch_navigator attribute."""
 

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2010-09-28 14:42:41 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2010-10-05 15:06:16 +0000
@@ -27,7 +27,9 @@
              more about syncing from the parent series</a>).
       </p>
 
-      <div tal:condition="differences/batch">
+      <div metal:use-macro="context/@@launchpad_form/form">
+
+      <div tal:condition="differences/batch" metal:fill-slot="widgets">
         <tal:navigation_top
           replace="structure differences/@@+navigation-links-upper" />
           <table class="listing">
@@ -44,10 +46,18 @@
             <tal:difference repeat="difference differences/batch">
             <tr tal:define="parent_source_pub difference/parent_source_pub;
                             source_pub difference/source_pub;
+                            src_name difference/source_package_name/name;
                             signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;"
                 tal:attributes="class parent_source_pub/source_package_name">
-              <td><a tal:attributes="href difference/fmt:url" class="toggle-extra"
-                     tal:content="parent_source_pub/source_package_name">Foo</a>
+              <td>
+                <input tal:condition="view/canPerformSync"
+                  name="field.selected_differences" type="checkbox"
+                  tal:attributes="
+                    value src_name;
+                    id string:field.selected_differences.${src_name}"/>
+
+                <a tal:attributes="href difference/fmt:url" class="toggle-extra"
+                   tal:content="parent_source_pub/source_package_name">Foo</a>
               </td>
               <td><a tal:attributes="href parent_source_pub/sourcepackagerelease/fmt:url">
                     <tal:replace
@@ -80,7 +90,17 @@
             </tal:difference>
           </tbody>
         </table>
+        <tal:selectable_differences_end
+               define="widget nocall:view/widgets/selected_differences;
+                       field_name widget/context/__name__;
+                       error python:view.getFieldError(field_name);">
+          <input tal:attributes="name string:${widget/name}-empty-marker"
+                 type="hidden" value="1" />
+          <div class="error message" tal:condition="error"
+               tal:content="structure error">Error message</div>
+           </tal:selectable_differences_end>
       </div>
+    </div>
 <script type="text/javascript">
 LPS.use('lp.registry.distroseriesdifferences_details', function(Y) {
   diff_module = Y.lp.registry.distroseriesdifferences_details