launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01373
[Merge] lp:~michael.nelson/launchpad/652838-select-diffs-for-syncing into lp:launchpad
Michael Nelson has proposed merging lp:~michael.nelson/launchpad/652838-select-diffs-for-syncing into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#652838 Select multiple diffs for syncing
https://bugs.launchpad.net/bugs/652838
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).
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.
To test
=======
bin/test -vvm test_series_views
I'll upload a quick screencast soon.
--
https://code.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/652838-select-diffs-for-syncing into lp:launchpad.
=== 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 08:59:46 +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,47 @@
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)
+ if has_edit:
+ diffs_vocabulary = SimpleVocabulary(
+ [SimpleTerm(
+ difference,
+ difference.source_package_name.name,
+ difference.source_package_name.name)
+ for difference in self.cached_differences.batch])
+ choice = self.form_fields['selected_differences'].field.value_type
+ choice.vocabulary = diffs_vocabulary
+
+ @action(_("Sync sources"), name="sync", validator='validate_sync')
+ 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.')
=== 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 08:59:46 +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
@@ -64,7 +72,7 @@
class DistroSeriesLocalPackageDiffsTestCase(TestCaseWithFactory):
"""Test the distroseries +localpackagediffs view."""
- layer = LaunchpadZopelessLayer
+ layer = LaunchpadFunctionalLayer
def makeDerivedSeries(self, derived_name=None, parent_name=None):
# Helper that creates a derived distro series.
@@ -209,6 +217,153 @@
self.assertEqual(1, len(links))
self.assertEqual(difference.source_package_name.name, links[0].string)
+ def test_sync_option_for_non_editor(self):
+ # Non-editors do not see options to syncs.
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+ soup = BeautifulSoup(view())
+
+ checkbox = soup.find(
+ 'input', id='field.selected_differences.%s' % (
+ difference.source_package_name.name))
+ self.assertIs(None, checkbox)
+ button = soup.find('input', id='field.actions.sync')
+ self.assertIs(None, button)
+
+ def test_sync_option_for_editor(self):
+ # Editors are presented with options to perform syncs.
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ with person_logged_in(derived_series.owner):
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+ soup = BeautifulSoup(view())
+
+ checkbox = soup.find(
+ 'input', id='field.selected_differences.%s' % (
+ difference.source_package_name.name))
+ self.assertIsNot(None, checkbox)
+ button = soup.find('input', id='field.actions.sync')
+ self.assertEqual(
+ "Sync selected Lucid versions into Derilucid",
+ button['value'])
+
+ def test_selected_differences_field_for_non_editor(self):
+ # Non-editors do not get a selected_differences field
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+
+ self.assertIs(None, view.widgets.get('selected_differences'))
+
+ def test_selected_differences_field_for_editor(self):
+ # Editors see a selected diffs field with the correct
+ # vocabulary.
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ source_package_name_str='my-src-name',
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ with person_logged_in(derived_series.owner):
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+
+ widget = view.widgets['selected_differences']
+ self.assertEqual(
+ ['my-src-name'],
+ widget.vocabulary.by_token.keys())
+
+ def test_sync_notification_on_success(self):
+ # Syncing one or more diffs results in a stub notification.
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ source_package_name_str='my-src-name',
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ 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.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ source_package_name_str='my-src-name',
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ 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.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ source_package_name_str='my-src-name',
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ 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 08:59:46 +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="context/required:launchpad.Edit"
+ 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
