launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04322
lp:~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-overlay into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-overlay into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-overlay/+merge/68647
This adds an overlay into DistroSeries:+localpackagediffs that allows
the user to only show packages which are within a given set of
packagesets.
- When the new JavaScript runs it changes the "Package-sets" table
header into a clickable js-action that pops up a form overlay
containing a single widget.
- When this overlay is dismissed, nothing happens.
- When this overlay is okayed, the search form (above the table) is
changed to include field.packageset fields, then the form is
submitted.
It's somewhat/fairly hacky right now, and I have firm plans to improve
this, but I want to get this out there and seek feedback. The page
it's on it feature flagged so we're not making this generally
available yet.
Note: the changes lib/lp/app/javascript/formoverlay/formoverlay.js are
all lint fixes. I happened to be passing and couldn't resist :-/
--
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-overlay/+merge/68647
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-overlay into lp:launchpad.
=== modified file 'lib/lp/app/javascript/formoverlay/formoverlay.js'
--- lib/lp/app/javascript/formoverlay/formoverlay.js 2011-06-29 14:56:15 +0000
+++ lib/lp/app/javascript/formoverlay/formoverlay.js 2011-07-21 09:36:36 +0000
@@ -84,8 +84,8 @@
* @type string
* @static
*/
-FormOverlay.FORM_HEADER_TEMPLATE = '<div class="' + FormOverlay.C_FORM_HEADER +
- '" />';
+FormOverlay.FORM_HEADER_TEMPLATE =
+ '<div class="' + FormOverlay.C_FORM_HEADER + '" />';
/**
* Static html template to use for creating the form element.
@@ -307,7 +307,8 @@
body_node.appendChild(this.form_header_node);
body_node.appendChild(this.form_node);
- this.setStdModContent(Y.WidgetStdMod.BODY, body_node, Y.WidgetStdMod.REPLACE);
+ this.setStdModContent(
+ Y.WidgetStdMod.BODY, body_node, Y.WidgetStdMod.REPLACE);
},
/**
@@ -405,7 +406,8 @@
++select_idx) {
option = elem.options[select_idx];
if (option.selected) {
- addData(elem.name, getOptionValue(option));
+ addData(elem.name,
+ getOptionValue(option));
}
}
}
@@ -443,7 +445,7 @@
* @method showError
*/
showError: function(error_msgs){
- if (typeof(error_msgs) == "string"){
+ if (Y.Lang.isString(error_msgs)) {
error_msgs = [error_msgs];
}
var error_html = "The following errors were encountered: <ul>";
@@ -494,8 +496,8 @@
}
var cfg = {
on: {success: on_success, failure: on_failure},
- arguments: this
- }
+ "arguments": this
+ };
io_provider.io(url, cfg);
}
});
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2011-07-18 18:28:27 +0000
+++ lib/lp/registry/browser/distroseries.py 2011-07-21 09:36:36 +0000
@@ -93,9 +93,7 @@
DistroSeriesDifferenceStatus,
DistroSeriesDifferenceType,
)
-from lp.registry.interfaces.distroseries import (
- IDistroSeries,
- )
+from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.distroseriesdifference import (
IDistroSeriesDifferenceSource,
)
@@ -112,6 +110,7 @@
IDistroSeriesDifferenceJobSource,
)
from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
+from lp.soyuz.interfaces.packageset import IPackagesetSet
from lp.soyuz.interfaces.queue import IPackageUploadSet
from lp.soyuz.model.queue import PackageUploadQueue
from lp.translations.browser.distroseries import (
@@ -1028,6 +1027,20 @@
return None
@property
+ def specified_packagesets_filter(self):
+ """If specified, return Packagesets given in the GET form data."""
+ packageset_ids = (
+ self.request.query_string_params.get("field.packageset", []))
+ packageset_ids = set(
+ int(packageset_id) for packageset_id in packageset_ids
+ if packageset_id.isdigit())
+ packagesets = getUtility(IPackagesetSet).getBySeries(self.context)
+ packagesets = set(
+ packageset for packageset in packagesets
+ if packageset.id in packageset_ids)
+ return None if len(packagesets) == 0 else packagesets
+
+ @property
def specified_package_type(self):
"""If specified, return the package type filter from the GET form
data.
@@ -1059,7 +1072,8 @@
IDistroSeriesDifferenceSource).getForDistroSeries(
self.context, difference_type=self.differences_type,
name_filter=self.specified_name_filter,
- status=status, child_version_higher=child_version_higher)
+ status=status, child_version_higher=child_version_higher,
+ packagesets=self.specified_packagesets_filter)
return BatchNavigator(differences, self.request)
@cachedproperty
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py 2011-07-07 20:07:51 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-07-21 09:36:36 +0000
@@ -596,10 +596,11 @@
def test_submit_sets_previous_series(self):
# Creating a new series when one already exists should set the
# previous_series.
- old_series = self.factory.makeDistroSeries(self.distribution,
- version='11.10')
- older_series = self.factory.makeDistroSeries(self.distribution,
- version='11.04')
+ old_series = self.factory.makeDistroSeries(
+ self.distribution, version='11.10')
+ # A yet older series.
+ self.factory.makeDistroSeries(
+ self.distribution, version='11.04')
old_time = utc_now() - timedelta(days=5)
removeSecurityProxy(old_series).datereleased = old_time
distroseries = self.createNewDistroseries()
@@ -2079,6 +2080,59 @@
'http://127.0.0.1?start=1&batch=1',
view.action_url)
+ def test_specified_packagesets_filter_none_specified(self):
+ # specified_packagesets_filter is None when there are no
+ # field.packageset parameters in the query.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.factory.makeDistroSeriesDifference()
+ person = dsd.derived_series.owner
+ with person_logged_in(person):
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string='')
+ self.assertIs(None, view.specified_packagesets_filter)
+
+ def test_specified_packagesets_filter_specified(self):
+ # specified_packagesets_filter returns a collection of Packagesets
+ # when there are field.packageset query parameters.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.factory.makeDistroSeriesDifference()
+ person = dsd.derived_series.owner
+ packageset1 = self.factory.makePackageset(
+ distroseries=dsd.derived_series)
+ packageset2 = self.factory.makePackageset(
+ distroseries=dsd.derived_series)
+ with person_logged_in(person):
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string='field.packageset=%d&field.packageset=%d' % (
+ packageset1.id, packageset2.id))
+ self.assertContentEqual(
+ [packageset1, packageset2],
+ view.specified_packagesets_filter)
+
+ def test_search_for_packagesets(self):
+ # If packagesets are supplied in the query the resulting batch will
+ # only contain packages in the given packagesets.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.factory.makeDistroSeriesDifference()
+ person = dsd.derived_series.owner
+ packageset = self.factory.makePackageset(
+ owner=person, distroseries=dsd.derived_series)
+ # The package is not in the packageset so the batch will be empty.
+ with person_logged_in(person):
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string='field.packageset=%d' % packageset.id)
+ self.assertEqual(0, len(view.cached_differences.batch))
+ # The batch will contain the package once it has been added to the
+ # packageset.
+ packageset.add((dsd.source_package_name,))
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string='field.packageset=%d' % packageset.id)
+ self.assertEqual(1, len(view.cached_differences.batch))
+
class TestDistroSeriesNeedsPackagesView(TestCaseWithFactory):
"""Test the distroseries +needs-packaging view."""
=== added file 'lib/lp/registry/javascript/distroseries/differences.js'
--- lib/lp/registry/javascript/distroseries/differences.js 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/distroseries/differences.js 2011-07-21 09:36:36 +0000
@@ -0,0 +1,181 @@
+/**
+ * Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * DistroSeries Initialization.
+ *
+ * @module lp.registry.distroseries
+ * @submodule differences
+ */
+
+YUI.add('lp.registry.distroseries.differences', function(Y) {
+
+Y.log('loading lp.registry.distroseries.differences');
+
+var namespace = Y.namespace('lp.registry.distroseries.differences'),
+ formwidgets = Y.lp.app.formwidgets,
+ widgets = Y.lp.registry.distroseries.widgets;
+
+var PACKAGESET_FIELD = "field.packageset",
+ PACKAGESET_FIELD_SELECTOR = "input[name=" + PACKAGESET_FIELD + "]";
+
+
+/**
+ * Return an array of the packageset IDs that are configured in the
+ * current window's query string.
+ *
+ * @param {String} qs The query string, typically obtained from
+ * window.location.search.
+ */
+var get_packagesets_in_query = function(qs) {
+ var query = Y.QueryString.parse(qs.replace(/^[?]/, ""));
+ /* Y.QueryString.parse() tries to be helpful and convert
+ numeral strings into numbers... but we don't want that,
+ so we have to convert back again. */
+ var packagesets = query[PACKAGESET_FIELD];
+ var n2s = function(n) { return n.toString(10); };
+ /* Y.QueryString.parse() tries to be even more helpful by
+ returning multiple values in an array but single values *not*
+ in an array... I wonder why I'm using Y.QueryString.parse() at
+ all. */
+ if (Y.Lang.isArray(packagesets)) {
+ return packagesets.map(n2s);
+ }
+ else if (Y.Lang.isValue(packagesets)) {
+ return [n2s(packagesets)];
+ }
+ else {
+ return [];
+ }
+};
+
+
+/**
+ * Wire up a packageset picker that updates the given form.
+ *
+ * @param {Y.Node} origin The node that, when clicked, should activate
+ * the picker.
+ * @param {Y.Node} form The form that the picker should update.
+ */
+var connect_packageset_picker = function(origin, form) {
+ /* XXX: GavinPanella 2011-07-19 bug=???: PackagesetPickerWidget
+ needs to render into a container. This should not be
+ required. */
+ var picker_table =
+ Y.Node.create("<table><tbody /></table>");
+ var picker =
+ new widgets.PackagesetPickerWidget()
+ .set("name", "packagesets")
+ .set("size", 5)
+ .set("multiple", true)
+ .render(picker_table.one("tbody"));
+
+ /* XXX: GavinPanella 2011-07-19 bug=???:
+ PackagesetPickerWidget.add_distroseries() accepts only this
+ odd-looking object. It would be more convenient and less
+ surprising if it also accepted an lp.client.Entry. */
+ picker.add_distroseries({
+ api_uri: LP.cache.context.self_link,
+ title: LP.cache.context.title,
+ value: LP.cache.context.self_link
+ });
+
+ /* Buttons */
+ var submit_button = Y.Node.create(
+ '<button type="submit" class="lazr-pos lazr-btn" />')
+ .set("text", "OK");
+ var cancel_button = Y.Node.create(
+ '<button type="button" class="lazr-neg lazr-btn" />')
+ .set("text", "Cancel");
+
+ /* When the form overlay is submitted the search filter form is
+ modified and submitted. */
+ var submit_callback = function(data) {
+ // Remove all packagesets information previously recorded.
+ form.all(PACKAGESET_FIELD_SELECTOR).remove();
+ if (data.packagesets !== undefined) {
+ Y.each(data.packagesets, function(packageset) {
+ form.append(
+ Y.Node.create('<input type="hidden" />')
+ .set("name", PACKAGESET_FIELD)
+ .set("value", packageset));
+ });
+ }
+ form.submit();
+ };
+
+ /* Form overlay. */
+ var overlay = new Y.lazr.FormOverlay({
+ align: {
+ /* Align the centre of the overlay with the centre of the
+ origin node. */
+ node: origin,
+ points: [
+ Y.WidgetPositionAlign.CC,
+ Y.WidgetPositionAlign.CC
+ ]
+ },
+ headerContent: "<h2>Select package sets</h2>",
+ form_content: picker_table,
+ form_submit_button: submit_button,
+ form_cancel_button: cancel_button,
+ form_submit_callback: submit_callback,
+ visible: false
+ });
+ overlay.render();
+
+ var reposition_overlay = function() {
+ /* Trigger alignment and constrain to the viewport. Should
+ these not be automatic? Perhaps a bad interaction between
+ widget-position-align and widget-position-constrain? Only
+ reposition when overlay is visible. */
+ if (overlay.get("visible")) {
+ overlay.set("align", overlay.get("align"));
+ overlay.constrain(null, true);
+ }
+ };
+ overlay.after("visibleChange", reposition_overlay);
+ Y.on("windowresize", reposition_overlay);
+
+ var packagesets_in_query =
+ get_packagesets_in_query(window.location.search);
+ var initialize_picker = function() {
+ /* Set the current selection from the query string. Only
+ initialize when overlay is visible. */
+ if (overlay.get("visible")) {
+ picker.set("choice", packagesets_in_query);
+ }
+ };
+ /* XXX: GavinPanella 2011-07-20 bug=???: We should be able to
+ listen to choicesChange events from the picker widget but
+ they're not fired consistently. Instead we initialize when
+ showing the overlay, which is prone to a race condition (it may
+ update the selection before the packageset picker has been
+ populated with choices. */
+ overlay.after("visibleChange", initialize_picker);
+
+ /* Convert the content of the origin into a js-action link, and
+ show the overlay when it's clicked. I would rather not do this
+ with innerHTML, but I can't figure out how to get YUI3 to wrap
+ the contents of a node with another node, *including text
+ nodes*. Also, node.get("text") is broken; given
+ <a>foo<b/>bar</a>, a.get("text") will return "foobar". */
+ var link = Y.Node.create('<a class="js-action sprite edit" />');
+ link.set("innerHTML", origin.get("innerHTML"));
+ link.on("click", function() { overlay.show(); });
+ origin.empty().append(link);
+};
+
+
+// Exports.
+namespace.connect_packageset_picker = connect_packageset_picker;
+
+
+}, "0.1", {
+ "requires": [
+ "node",
+ "querystring-parse",
+ "lazr.formoverlay",
+ "lp.app.formwidgets",
+ "lp.registry.distroseries.widgets"
+ ]});
=== modified file 'lib/lp/registry/javascript/distroseries/widgets.js'
--- lib/lp/registry/javascript/distroseries/widgets.js 2011-07-18 10:50:01 +0000
+++ lib/lp/registry/javascript/distroseries/widgets.js 2011-07-21 09:36:36 +0000
@@ -367,8 +367,8 @@
* distroseries to the possible choices.
*
* @method add_distroseries
- * @param {Object} The distroseries to add ({value:distroseries_id),
- * api_uri:distroseries_uri}).
+ * @param {Object} distroseries The distroseries to add
+ * ({value:distroseries_id, api_uri:distroseries_uri}).
*/
add_distroseries: function(distroseries) {
var path = distroseries.api_uri + "/architectures";
@@ -468,6 +468,9 @@
* @property distroSeries
*/
distroSeries: {
+ /* XXX: GavinPanella 2011-07-20 bug=???: This widget
+ handles multiple distroseries so this attribute is
+ defunct. */
setter: function(value, name) {
var distro_series_uri = Y.lp.client.get_absolute_uri(value);
var on = {
@@ -495,7 +498,12 @@
* Add a distroseries: add its packagesets to the packageset picker.
*
* @method add_distroseries
- * @param {Object} distroseries The distroseries object.
+ * @param {Object} distroseries An object describing a
+ * distroseries, containing three properties: api_uri, title
+ * and value. The first two are what you might expect. The
+ * last is simply a unique term for referencing the
+ * distroseries which is required when calling
+ * remove_distroseries.
*/
add_distroseries: function(distroseries) {
var distro_series_uri = Y.lp.client.get_absolute_uri(
@@ -522,6 +530,14 @@
* @method clean_display
*/
clean_display: function() {
+ /* XXX: GavinPanella 2011-07-20 bug=???: The list of choices
+ should be set via this.set("choices", ...) and the setter
+ should ensure that the selection is maintained. Doing it
+ via the back door means that the choicesChange event is not
+ fired. Rather than fire it explicity we should DTRT. */
+ /* XXX: GavinPanella 2011-07-18 bug=???: This does not update
+ this._distroseries and can thus leave the widget in an
+ invalid state. */
this.fieldNode.empty();
this.fieldNode.append(Y.Node.create("<div />")
.set('text', '[No package sets to select from yet!]'));
@@ -534,6 +550,9 @@
* @method init_select
*/
init_select: function() {
+ /* XXX: GavinPanella 2011-07-20 bug=???: This method will not
+ be necessary once this.set("choices", ...) is used
+ consistently. */
var select = this.fieldNode.one('select');
if (select === null) {
select = Y.Node.create("<select />");
@@ -555,6 +574,11 @@
* ({text: choice_text, value: choice_value, data: choice_data}).
*/
add_choice: function(choice) {
+ /* XXX: GavinPanella 2011-07-20 bug=???: The list of choices
+ should be set via this.set("choices", ...) and the setter
+ should ensure that the selection is maintained. Doing it
+ via the back door means that the choicesChange event is not
+ fired. Rather than fire it explicity we should DTRT. */
var select = this.init_select();
var option = Y.Node.create("<option />");
option.set("value", choice.value)
@@ -612,6 +636,12 @@
remove_distroseries: function(distroseries_id) {
this._packagesets[distroseries_id].forEach(
function(packageset) {
+ /* XXX: GavinPanella 2011-07-20 bug=???: The list of
+ choices should be set via this.set("choices", ...)
+ and the setter should ensure that the selection is
+ maintained. Doing it via the back door means that
+ the choicesChange event is not fired. Rather than
+ fire it explicity we should DTRT. */
this.fieldNode.one(
'option[value="' + packageset.get("id") + '"]').remove();
}, this);
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2011-06-13 19:41:40 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2011-07-21 09:36:36 +0000
@@ -38,7 +38,7 @@
<div metal:use-macro="context/@@launchpad_form/form">
- <div tal:condition="differences/batch" metal:fill-slot="widgets">
+ <div metal:fill-slot="widgets">
<tal:navigation_top
replace="structure differences/@@+navigation-links-upper" />
<table class="listing">
@@ -61,10 +61,12 @@
<a tal:attributes="href context/fmt:url"
tal:content="series_name">Deriwarty</a> version
</th>
- <th tal:condition="view/show_parent_packagesets">
+ <th tal:condition="view/show_parent_packagesets"
+ class="parent-package-sets">
Parent package-sets
</th>
- <th tal:condition="view/show_packagesets">
+ <th tal:condition="view/show_packagesets"
+ class="package-sets">
Package-sets
</th>
<th>Last uploaded</th>
@@ -202,6 +204,18 @@
Y.on('domready', diff_module.setup_expandable_rows);
});
</script>
+<script type="text/javascript">
+ LPS.use("lp.registry.distroseries.differences", function(Y) {
+ Y.on("domready", function() {
+ var form = Y.one("form#distroseries-localdiff-search-filter");
+ var differences = Y.lp.registry.distroseries.differences;
+ // Probably only one, but check for more just in case.
+ Y.all("th.package-sets").each(function(header) {
+ differences.connect_packageset_picker(header, form);
+ });
+ });
+ });
+</script>
</div>
