launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01189
[Merge] lp:~michael.nelson/launchpad/644328-blacklist-ui into lp:launchpad
Michael Nelson has proposed merging lp:~michael.nelson/launchpad/644328-blacklist-ui into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Overview
========
This branch implements the UI for blacklisting source package differences for derived distro series.
See the LEP here:
https://dev.launchpad.net/LEP/DerivativeDistributions
and the specific mockup here:
https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png
Currently I'm just after a UI review before I go and clean up the JS code (and create template fragments). You can view a 1min demo of the UI here:
http://people.canonical.com/~michaeln/tmp/644328-blacklist-ui.ogv
(hrm, I need to sort those differences by name).
To demo locally:
================
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs
--
https://code.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/644328-blacklist-ui into lp:launchpad.
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in 2010-09-21 16:02:23 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-09-23 13:53:45 +0000
@@ -2419,7 +2419,7 @@
background-repeat: no-repeat;
background-position:right center;
}
-table#packages_list tr.superseded {
+table#packages_list tr.superseded, tr.blacklisted {
background-color: #eee;
}
ul.latest-ppa-updates li:nth-child(odd) {
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2010-09-21 11:17:25 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2010-09-23 13:53:45 +0000
@@ -59,3 +59,28 @@
self.assertEqual(
DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
ds_diff.status)
+
+ def test_unblacklist_not_public(self):
+ # The unblacklist method is not publically available.
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+ ws_diff = ws_object(self.factory.makeLaunchpadService(), ds_diff)
+
+ self.assertRaises(Unauthorized, ws_diff.unblacklist)
+
+ def test_unblacklist(self):
+ # The unblacklist method can be called by people with edit access.
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+ ws_diff = ws_object(self.factory.makeLaunchpadService(
+ ds_diff.owner), ds_diff)
+
+ result = ws_diff.unblacklist()
+ transaction.commit()
+
+ utility = getUtility(IDistroSeriesDifferenceSource)
+ ds_diff = utility.getByDistroSeriesAndName(
+ ds_diff.derived_series, ds_diff.source_package_name.name)
+ self.assertEqual(
+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+ ds_diff.status)
=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py 2010-09-21 08:42:29 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py 2010-09-23 13:53:45 +0000
@@ -73,15 +73,6 @@
name=derived_name, parent_series=parent)
return derived_series
- def makeControllerInScopes(self, scopes):
- """Make a controller that will report it's in the given scopes."""
- call_log = []
-
- def scope_cb(scope):
- call_log.append(scope)
- return scope in scopes
- return FeatureController(scope_cb), call_log
-
def setDerivedSeriesUIFeatureFlag(self):
# Helper to set the feature flag enabling the derived series ui.
ignore = getFeatureStore().add(FeatureFlag(
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-20 15:00:11 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-23 13:53:45 +0000
@@ -17,10 +17,12 @@
export_as_webservice_entry,
export_write_operation,
exported,
+ operation_parameters,
)
from lazr.restful.fields import Reference
from zope.interface import Interface
from zope.schema import (
+ Bool,
Choice,
Int,
TextLine,
@@ -135,6 +137,8 @@
def addComment(owner, comment):
"""Add a comment on this difference."""
+ @operation_parameters(
+ all=Bool(title=_("All"), required=False))
@export_write_operation()
def blacklist(all=False):
"""Blacklist this version or all versions of this source package.
@@ -143,6 +147,13 @@
be blacklisted or just the current (default).
"""
+ @export_write_operation()
+ def unblacklist():
+ """Removes this difference from the blacklist.
+
+ The status will be updated based on the versions.
+ """
+
class IDistroSeriesDifference(IDistroSeriesDifferencePublic,
IDistroSeriesDifferenceEdit):
=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-09-15 15:05:11 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-09-23 13:53:45 +0000
@@ -11,24 +11,108 @@
var namespace = Y.namespace('lp.registry.distroseriesdifferences_details');
+/**
+ * Create one Launchpad client that will be used with multiple requests.
+ */
+var lp_client = new LP.client.Launchpad();
+
/*
* Setup the expandable rows for each difference.
*
* @method setup_expandable_rows
*/
namespace.setup_expandable_rows = function() {
- var start_update = function(uri, container) {
+
+ var blacklist_handler = function(e, api_uri, source_name) {
+ e.preventDefault();
+ var blacklist_options_container = this;
+ var blacklist_all = false;
+
+ // XXX - move all blacklist options/text generation to
+ // template fragment.
+ var in_progress_msg = ['Blacklisted until a new version ',
+ 'is published.'].join('');
+ var success_msg = 'Blacklisted until a new version is published.';
+ if (e.target.hasClass('blacklist-all')) {
+ blacklist_all = true;
+ in_progress_msg = 'Blacklisting all future versions.';
+ success_msg = 'Blacklisted permanently.';
+ }
+ var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+ in_progress_msg)
+ var blacklist_options = blacklist_options_container.removeChild(
+ blacklist_options_container.one('div'));
+ blacklist_options_container.insert(in_progress_message, 'replace');
+
+ var diff_rows = Y.all('tr.' + source_name);
+
+ var config = {
+ on: {
+ success: function(updated_entry, args) {
+ Y.each(diff_rows, function(diff_row) {
+ var fade_to_gray = new Y.Anim({
+ node: diff_row,
+ to: { backgroundColor: '#EEEEEE'},
+ });
+ fade_to_gray.run();
+ });
+
+ blacklist_options_container.insert(
+ success_msg, 'replace');
+ Y.lazr.anim.green_flash({
+ node:success_msg}).run();
+ },
+ failure: function(id, response) {
+ blacklist_options_container.insert(
+ blacklist_options, 'replace');
+ }
+ },
+ parameters: {
+ all: blacklist_all
+ }
+ };
+
+ // The context passed when this event was created is a simple
+ // object with the uri.
+ lp_client.named_post(api_uri, 'blacklist', config);
+
+ }
+ var setup_blacklist_options = function(container, source_name, source_version) {
+ var blacklist_options = Y.Node.create([
+ '<div>',
+ ' <a href="#" class="js-action blacklist-all">Blacklist ',
+ ' all versions of ' + source_name,
+ ' </a><br/>',
+ ' <a href="#" class="js-action blacklist-version">Blacklist ' + source_name,
+ ' ' + source_version + '</a>',
+ '</div>',
+ ].join(''));
+ container.insert(blacklist_options, 'replace')
+ var api_uri = [
+ LP.client.cache.context.self_link,
+ '+difference',
+ source_name
+ ].join('/')
+ Y.on('click', blacklist_handler, blacklist_options.all('a'),
+ container, api_uri, source_name);
+ };
+
+ var start_update = function(uri, container, source_name, source_version) {
var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
'Fetching difference details ...')
- container.insert(in_progress_message, 'replace');
+ container.one('div.diff-extra-container').insert(
+ in_progress_message, 'replace');
var config = {
on: {
'success': function(transaction_id, response, args) {
- args.container.set(
- 'innerHTML', response.responseText);
- // Change to insert(,'replace)
+ args.container.one(
+ 'div.diff-extra-container').insert(
+ response.responseText, 'replace');
+ setup_blacklist_options(args.container.one(
+ 'div.blacklist-options'), source_name,
+ source_version);
},
'failure': function(transaction_id, response, args){
var retry_handler = function(e) {
@@ -64,11 +148,18 @@
// Only insert if there isn't already a container row there.
next_row = row.next();
if (next_row == null || !next_row.hasClass('diff-extra')) {
- details_row = Y.Node.create(
- '<table><tr class="diff-extra unseen"><td colspan="5"></td></tr></table>').one('tr');
+ var source_name = row.one('a.toggle-extra').get('text');
+ var details_row = Y.Node.create([
+ '<table><tr class="diff-extra unseen ' + source_name + '">',
+ ' <td colspan="5">',
+ ' <div class="blacklist-options" style="float:right"></div>',
+ ' <div class="diff-extra-container"></div>',
+ ' </td></tr></table>'
+ ].join('')).one('tr');
row.insert(details_row, 'after');
var uri = toggle.get('href');
- start_update(uri, details_row.one('td'));
+ var version = row.one('a.derived-version').get('text');
+ start_update(uri, details_row.one('td'), source_name, version);
} else {
details_row = next_row
}
@@ -83,4 +174,4 @@
};
-}, "0.1", {"requires": ["io-base", "lp.soyuz.base"]});
+}, "0.1", {"requires": ["io-base", "lp.soyuz.base", "lazr.anim"]});
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2010-09-21 11:17:25 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2010-09-23 13:53:45 +0000
@@ -272,3 +272,8 @@
self.status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
else:
self.status = DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT
+
+ def unblacklist(self):
+ """See `IDistroSeriesDifference`."""
+ self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ self.update()
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-16 09:32:35 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-23 13:53:45 +0000
@@ -44,7 +44,8 @@
<tal:difference repeat="difference differences/batch">
<tr tal:define="parent_source_pub difference/parent_source_pub;
source_pub difference/source_pub;
- signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;">
+ 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>
@@ -52,7 +53,8 @@
<tal:replace
replace="parent_source_pub/sourcepackagerelease/version"/></a>
</td>
- <td><a tal:attributes="href source_pub/sourcepackagerelease/fmt:url">
+ <td><a tal:attributes="href source_pub/sourcepackagerelease/fmt:url"
+ class="derived-version">
<tal:replace
replace="source_pub/sourcepackagerelease/version"/></a>
</td>
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-20 15:00:11 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-23 13:53:45 +0000
@@ -369,6 +369,39 @@
DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
ds_diff.status)
+ def test_unblacklist(self):
+ # Unblacklisting will return to NEEDS_ATTENTION by default.
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+
+ with person_logged_in(ds_diff.owner):
+ ds_diff.unblacklist()
+
+ self.assertEqual(
+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+ ds_diff.status)
+
+ def test_unblacklist_resolved(self):
+ # Status is resolved when unblacklisting a now-resolved difference.
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ versions={
+ 'derived': '0.9',
+ 'parent': '1.0',
+ },
+ status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
+ new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
+ sourcepackagename=ds_diff.source_package_name,
+ distroseries=ds_diff.derived_series,
+ status=PackagePublishingStatus.PENDING,
+ version='1.0')
+
+ with person_logged_in(ds_diff.owner):
+ ds_diff.unblacklist()
+
+ self.assertEqual(
+ DistroSeriesDifferenceStatus.RESOLVED,
+ ds_diff.status)
+
def test_source_package_name_unique_for_derived_series(self):
# We cannot create two differences for the same derived series
# for the same package.
Follow ups
