launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04422
lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873-ui into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873-ui into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798873 in Launchpad itself: "Being able to filter by subscribed packages for a given team or user."
https://bugs.launchpad.net/launchpad/+bug/798873
For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873-ui/+merge/69766
This branch makes it possible to click on the "Last changed" table
heading in +localpackagediffs and select a user or team using the
familiar person picker. The page is then reloaded with the result
limited to that person or team's changes.
View changes:
- DistroSeriesDifferenceBaseView has a new property,
specified_changed_by_filter, that returns the selected person or
teams. The value of this is passed into getForDistroSeries().
JavaScript and HTML changes:
- In lp.registry.distroseries.differences, a linkify() function was
extracted from connect_packageset_picker().
- New function connect_last_changed_picker() linkifies the given
origin node and wires up the person picker.
- Nothing in the differences modules was tested before - it is
somewhat experimental - so I added a test module. The person picker
stuff is much easier to test than the existing packageset picker
(the latter needs to make API calls during set-up) so it is tested
as well as all the module's functions. There are still no tests for
connect_packageset_picker().
- connect_last_changed_picker() is called from the template.
--
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873-ui/+merge/69766
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873-ui into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2011-07-28 18:35:44 +0000
+++ lib/lp/registry/browser/distroseries.py 2011-07-29 11:02:33 +0000
@@ -97,6 +97,7 @@
from lp.registry.interfaces.distroseriesdifference import (
IDistroSeriesDifferenceSource,
)
+from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
from lp.services.features import getFeatureFlag
@@ -1042,6 +1043,18 @@
return None if len(packagesets) == 0 else packagesets
@property
+ def specified_changed_by_filter(self):
+ """If specified, return Persons given in the GET form data."""
+ get_person_by_name = getUtility(IPersonSet).getByName
+ changed_by_names = set(
+ self.request.query_string_params.get("field.changed_by", ()))
+ changed_by = (
+ get_person_by_name(name) for name in changed_by_names)
+ changed_by = set(
+ person for person in changed_by if person is not None)
+ return None if len(changed_by) == 0 else changed_by
+
+ @property
def specified_package_type(self):
"""If specified, return the package type filter from the GET form
data.
@@ -1074,7 +1087,8 @@
self.context, difference_type=self.differences_type,
name_filter=self.specified_name_filter,
status=status, child_version_higher=child_version_higher,
- packagesets=self.specified_packagesets_filter)
+ packagesets=self.specified_packagesets_filter,
+ changed_by=self.specified_changed_by_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-28 18:35:44 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-07-29 11:02:33 +0000
@@ -9,6 +9,7 @@
import difflib
import re
from textwrap import TextWrapper
+from urllib import urlencode
from BeautifulSoup import BeautifulSoup
from lazr.restful.interfaces import IJSONRequestCache
@@ -2151,6 +2152,59 @@
query_string='field.packageset=%d' % packageset.id)
self.assertEqual(1, len(view.cached_differences.batch))
+ def test_specified_changed_by_filter_none_specified(self):
+ # specified_changed_by_filter is None when there are no
+ # field.changed_by 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_changed_by_filter)
+
+ def test_specified_changed_by_filter_specified(self):
+ # specified_changed_by_filter returns a collection of Person when
+ # there are field.changed_by query parameters.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.factory.makeDistroSeriesDifference()
+ person = dsd.derived_series.owner
+ changed_by1 = self.factory.makePerson()
+ changed_by2 = self.factory.makePerson()
+ with person_logged_in(person):
+ query_string = urlencode(
+ {"field.changed_by": (changed_by1.name, changed_by2.name)},
+ doseq=True)
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string=query_string)
+ self.assertContentEqual(
+ [changed_by1, changed_by2],
+ view.specified_changed_by_filter)
+
+ def test_search_for_changed_by(self):
+ # If changed_by is specified the query the resulting batch will only
+ # contain packages relating to those people or teams.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.factory.makeDistroSeriesDifference()
+ person = dsd.derived_series.owner
+ ironhide = self.factory.makePersonByName("Ironhide")
+ query_string = urlencode({"field.changed_by": ironhide.name})
+ # The package release is not from Ironhide so the batch will be empty.
+ with person_logged_in(person):
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string=query_string)
+ self.assertEqual(0, len(view.cached_differences.batch))
+ # The batch will contain the package once Ironhide has been
+ # associated with its release.
+ removeSecurityProxy(dsd.source_package_release).creator = ironhide
+ view = create_initialized_view(
+ dsd.derived_series, '+localpackagediffs', method='GET',
+ query_string=query_string)
+ self.assertEqual(1, len(view.cached_differences.batch))
+
class TestDistroSeriesNeedsPackagesView(TestCaseWithFactory):
"""Test the distroseries +needs-packaging view."""
=== modified file 'lib/lp/registry/javascript/distroseries/differences.js'
--- lib/lp/registry/javascript/distroseries/differences.js 2011-07-22 10:19:51 +0000
+++ lib/lp/registry/javascript/distroseries/differences.js 2011-07-29 11:02:33 +0000
@@ -2,7 +2,7 @@
* Copyright 2011 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE).
*
- * DistroSeries Initialization.
+ * DistroSeries Differences.
*
* @module lp.registry.distroseries
* @submodule differences
@@ -13,11 +13,14 @@
Y.log('loading lp.registry.distroseries.differences');
var namespace = Y.namespace('lp.registry.distroseries.differences'),
+ testspace = Y.namespace('lp.registry.distroseries.differences.test'),
formwidgets = Y.lp.app.formwidgets,
widgets = Y.lp.registry.distroseries.widgets;
var PACKAGESET_FIELD = "field.packageset",
- PACKAGESET_FIELD_SELECTOR = "input[name=" + PACKAGESET_FIELD + "]";
+ PACKAGESET_FIELD_SELECTOR = "input[name=" + PACKAGESET_FIELD + "]",
+ CHANGED_BY_FIELD = "field.changed_by",
+ CHANGED_BY_FIELD_SELECTOR = "input[name=" + CHANGED_BY_FIELD + "]";
/**
@@ -51,6 +54,49 @@
/**
+ * Return the first field.changed_by value from the current window's
+ * query string.
+ *
+ * @param {String} qs The query string, typically obtained from
+ * window.location.search.
+ */
+var get_changed_by_in_query = function(qs) {
+ var query = Y.QueryString.parse(qs.replace(/^[?]/, ""));
+ var changed_by = query[CHANGED_BY_FIELD];
+ if (Y.Lang.isArray(changed_by)) {
+ return changed_by[0];
+ }
+ else if (Y.Lang.isValue(changed_by)) {
+ return changed_by;
+ }
+ else {
+ return null;
+ }
+};
+
+
+/**
+ * Convert the content of the given node into a js-action link.
+ *
+ * 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".
+ *
+ * @param {Y.Node} node The node to linkify.
+ *
+ */
+var linkify = function(node) {
+ var link = Y.Node.create('<a class="js-action sprite edit" />');
+ link.set("innerHTML", node.get("innerHTML"));
+ node.empty().append(link);
+ return link;
+};
+
+
+/**
* Wire up a packageset picker that updates the given form.
*
* @param {Y.Node} origin The node that, when clicked, should activate
@@ -120,7 +166,7 @@
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
+ widget-position-align and widget-positionposition-constrain? Only
reposition when overlay is visible. */
if (overlay.get("visible")) {
overlay.set("align", overlay.get("align"));
@@ -147,28 +193,88 @@
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"));
+ /* Linkify the origin and show the overlay when it's clicked. */
+ var link = linkify(origin);
link.on("click", function() { overlay.show(); });
- origin.empty().append(link);
+};
+
+
+/**
+ * Wire up a person 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_last_changed_picker = function(origin, form) {
+ var config = {
+ picker_type: "person",
+ header: "Choose a person or a team",
+ visible: false
+ };
+ var picker = new Y.lp.app.picker.create("ValidPersonOrTeam", config);
+
+ /* Align the centre of the overlay with the centre of the origin
+ node. */
+ var align = {
+ node: origin,
+ points: [
+ Y.WidgetPositionAlign.CC,
+ Y.WidgetPositionAlign.CC
+ ]
+ };
+
+ /* lazr.picker.Picker sets centered to true each time the picker
+ is shown. Not happy. Killing it with fire^W preventDefault. */
+ picker.on("centeredChange", function(e) { e.preventDefault(); });
+
+ /* XXX: GavinPanella 2011-07-27 bug=817091: Alignment must be done
+ after creating the picker because lp.app.picker.create()
+ clobbers the passed configuration. */
+
+ /* Pre-fill the search box with an existing selection. */
+ picker.after("visibleChange", function(e) {
+ if (e.newVal) {
+ picker.set("align", align); // Arg.
+ var changed_by = get_changed_by_in_query(window.location.search);
+ if (Y.Lang.isValue(changed_by)) {
+ picker._search_input.set("value", changed_by);
+ }
+ }
+ });
+
+ /* When the picker is saved the search filter form is modified and
+ submitted. */
+ picker.on("save", function(e) {
+ form.all(CHANGED_BY_FIELD_SELECTOR).remove();
+ if (e.value) {
+ form.append(
+ Y.Node.create('<input type="hidden" />')
+ .set("name", CHANGED_BY_FIELD)
+ .set("value", e.value));
+ }
+ form.submit();
+ });
+
+ /* Linkify the origin and show the picker when it's clicked. */
+ linkify(origin).on("click", function(e) { picker.show(); });
+
+ return picker;
};
// Exports.
namespace.connect_packageset_picker = connect_packageset_picker;
-
-
-}, "0.1", {
- "requires": [
- "node",
- "querystring-parse",
- "lazr.formoverlay",
- "lp.app.formwidgets",
- "lp.registry.distroseries.widgets"
- ]});
+namespace.connect_last_changed_picker = connect_last_changed_picker;
+
+
+// Exports for testing.
+testspace.get_packagesets_in_query = get_packagesets_in_query;
+testspace.get_changed_by_in_query = get_changed_by_in_query;
+testspace.linkify = linkify;
+
+
+}, "0.1", {"requires": [
+ "lazr.formoverlay", "lp.app.formwidgets",
+ "lp.app.picker", "lp.registry.distroseries.widgets",
+ "node", "querystring-parse"]});
=== added file 'lib/lp/registry/javascript/distroseries/tests/test_differences.html'
--- lib/lp/registry/javascript/distroseries/tests/test_differences.html 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_differences.html 2011-07-29 11:02:33 +0000
@@ -0,0 +1,47 @@
+<!DOCTYPE
+ HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
+ "http://www.w3.org/TR/html4/strict.dtd";>
+<html>
+ <head>
+ <title>Launchpad DistroSeries Differences</title>
+
+ <!-- YUI and test setup -->
+ <link rel="stylesheet" href="../../../../app/javascript/testing/test.css" />
+ <script type="text/javascript"
+ src="../../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/testing/testrunner.js"></script>
+
+ <!-- Required modules -->
+ <script type="text/javascript"
+ src="../../../../app/javascript/client.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/activator/activator.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/anim/anim.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/lazr/lazr.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/overlay/overlay.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/picker/picker.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/picker/person_picker.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/picker/picker_patcher.js"></script>
+
+ <!-- The module under test -->
+ <script type="text/javascript"
+ src="../differences.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript"
+ src="test_differences.js"></script>
+
+ </head>
+ <body class="yui3-skin-sam">
+ <ul id="suites">
+ <li>lp.registry.distroseries.differences.test</li>
+ </ul>
+ </body>
+</html>
=== added file 'lib/lp/registry/javascript/distroseries/tests/test_differences.js'
--- lib/lp/registry/javascript/distroseries/tests/test_differences.js 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_differences.js 2011-07-29 11:02:33 +0000
@@ -0,0 +1,139 @@
+/**
+ * Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for DistroSeries Differences.
+ *
+ * @module lp.registry.distroseries.differences
+ * @submodule test
+ */
+
+YUI.add('lp.registry.distroseries.differences.test', function(Y) {
+
+ var namespace = Y.namespace('lp.registry.distroseries.differences.test');
+
+ var Assert = Y.Assert;
+ var ArrayAssert = Y.ArrayAssert;
+
+ var suite = new Y.Test.Suite("distroseries.differences Tests");
+ var differences = Y.lp.registry.distroseries.differences;
+
+ var TestFunctions = {
+ name: "TestFunctions",
+
+ test_get_packagesets_in_query: function() {
+ Assert.isFunction(namespace.get_packagesets_in_query);
+ },
+
+ test_get_packagesets_in_query_no_matching_parameters: function() {
+ ArrayAssert.itemsAreSame(
+ [], namespace.get_packagesets_in_query(""));
+ ArrayAssert.itemsAreSame(
+ [], namespace.get_packagesets_in_query("?"));
+ ArrayAssert.itemsAreSame(
+ [], namespace.get_packagesets_in_query("?foo=bar"));
+ },
+
+ test_get_packagesets_in_query_matching_parameters: function() {
+ ArrayAssert.itemsAreSame(
+ ["foo"], namespace.get_packagesets_in_query(
+ "field.packageset=foo"));
+ // A leading question mark is okay.
+ ArrayAssert.itemsAreSame(
+ ["foo"], namespace.get_packagesets_in_query(
+ "?field.packageset=foo"));
+ ArrayAssert.itemsAreSame(
+ ["foo", "bar"], namespace.get_packagesets_in_query(
+ "?field.packageset=foo&field.packageset=bar"));
+ },
+
+ test_get_packagesets_in_query_numeric_parameters: function() {
+ // All-digit parameters are still returned as strings.
+ ArrayAssert.itemsAreSame(
+ ["123"], namespace.get_packagesets_in_query(
+ "field.packageset=123"));
+ },
+
+ test_get_changed_by_in_query: function() {
+ Assert.isFunction(namespace.get_changed_by_in_query);
+ },
+
+ test_get_changed_by_in_query_no_matching_parameters: function() {
+ Assert.isNull(namespace.get_changed_by_in_query(""));
+ Assert.isNull(namespace.get_changed_by_in_query("?"));
+ Assert.isNull(namespace.get_changed_by_in_query("?foo=bar"));
+ },
+
+ test_get_changed_by_in_query_matching_parameters: function() {
+ Assert.areSame(
+ "foo", namespace.get_changed_by_in_query(
+ "field.changed_by=foo"));
+ // A leading question mark is okay.
+ Assert.areSame(
+ "foo", namespace.get_changed_by_in_query(
+ "?field.changed_by=foo"));
+ // Only the first changed_by parameter is returned.
+ Assert.areSame(
+ "foo", namespace.get_changed_by_in_query(
+ "?field.changed_by=foo&field.changed_by=bar"));
+ },
+
+ test_linkify: function() {
+ var target = Y.Node.create("<div>Foobar</div>");
+ var link = namespace.linkify(target);
+ Assert.isInstanceOf(Y.Node, link);
+ Assert.areSame(link, target.one("a"));
+ Assert.areSame("Foobar", link.get("text"));
+ Assert.isTrue(link.hasClass("js-action"));
+ }
+
+ };
+
+ suite.add(new Y.Test.Case(TestFunctions));
+
+ var TestLastChangedPicker = {
+ name: "TestLastChangedPicker",
+
+ setUp: function() {
+ var body = Y.one(document.body);
+ this.form = Y.Node.create("<form />").appendTo(body);
+ this.origin = Y.Node.create("<div>Origin</div>").appendTo(body);
+ this.picker = differences.connect_last_changed_picker(
+ this.origin, this.form);
+ },
+
+ tearDown: function() {
+ this.picker.get("boundingBox").remove(true);
+ this.origin.remove(true);
+ this.form.remove(true);
+ },
+
+ test_linkify_click_activates_picker: function() {
+ Assert.isFalse(this.picker.get("visible"));
+ this.origin.one("a").simulate("click");
+ Assert.isTrue(this.picker.get("visible"));
+ },
+
+ test_picker_save_updates_and_submits_form: function() {
+ var submitted = false;
+ this.form.submit = function(e) {
+ submitted = true;
+ };
+ this.picker.fire("save", {value: "foobar"});
+ Assert.isTrue(submitted);
+ var input = this.form.one("input");
+ Assert.isInstanceOf(Y.Node, input);
+ Assert.areSame("hidden", input.get("type"));
+ Assert.areSame("field.changed_by", input.get("name"));
+ Assert.areSame("foobar", input.get("value"));
+ }
+
+ };
+
+ suite.add(new Y.Test.Case(TestLastChangedPicker));
+
+ namespace.suite = suite;
+
+}, "0.1", {"requires": [
+ "lp.registry.distroseries.differences", "node",
+ "node-event-simulate", "test"]});
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2011-07-28 10:56:43 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2011-07-29 11:02:33 +0000
@@ -69,7 +69,7 @@
class="package-sets">
Package-sets
</th>
- <th>Last changed</th>
+ <th class="last-changed">Last changed</th>
<th>Latest comment</th>
</tr>
</thead>
@@ -233,6 +233,9 @@
Y.all("th.package-sets").each(function(header) {
differences.connect_packageset_picker(header, form);
});
+ Y.all("th.last-changed").each(function(header) {
+ differences.connect_last_changed_picker(header, form);
+ });
});
});
</script>
