launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04526
[Merge] lp:~rvb/launchpad/bug-798522 into lp:launchpad
Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/bug-798522 into lp:launchpad with lp:~rvb/launchpad/api-add-comment-bug-798522 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798522 in Launchpad itself: "https://launchpad.net/ubuntu/oneiric/+localpackagediffs should require a comment when blacklisting a package"
https://bugs.launchpad.net/launchpad/+bug/798522
For more details, see:
https://code.launchpad.net/~rvb/launchpad/bug-798522/+merge/70309
This is the UI side of the fix for bug 798522. The goal is to better track blackling/unblacklisting actions on DSDs. When someone changes the status of a dsd, an overlay pops up to let him enter an optional comment. Once the round trip to the server is done, the comment is added to the list of comments and replaces the last comment slot.
= Tests =
No tests for this because the Javascript is almost untested and almost untestable now, this will be fixed by ~rvb/launchpad/widgetify-bug-796669 and lp:~rvb/launchpad/blacklist-bug-796669 (which depend on this branch).
= Q/A =
Open up a DSD row on
https://dogfood.launchpad.net/ubuntu/oneiric/+localpackagediffs and change the status, fill the pop up that should appear: the new comment should then appear at the bottom of the list and contain the comment plus a description of the status changed. The "last comment" slot should also be updated.
--
https://code.launchpad.net/~rvb/launchpad/bug-798522/+merge/70309
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/bug-798522 into lp:launchpad.
=== modified file 'lib/lp/registry/javascript/distroseries/differences.js'
--- lib/lp/registry/javascript/distroseries/differences.js 2011-08-01 12:38:05 +0000
+++ lib/lp/registry/javascript/distroseries/differences.js 2011-08-04 08:09:43 +0000
@@ -193,7 +193,7 @@
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. */
+ populated with choices). */
overlay.after("visibleChange", initialize_picker);
/* Linkify the origin and show the overlay when it's clicked. */
=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js 2011-06-21 14:34:49 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js 2011-08-04 08:09:43 +0000
@@ -17,19 +17,86 @@
var lp_client = new Y.lp.client.Launchpad();
/*
+ * XXX: rvb 2011-08-01 bug=796669: At present this module it is
+ * function-passing spaghetti. The duct-tape is getting frayed.
+ * It ought to be recomposed as widgets or something a bit more objecty so
+ * it can be unit tested without having to set-up the world each time.
+ */
+
+/*
* Setup the expandable rows for each difference.
*
* @method setup_expandable_rows
*/
namespace.setup_expandable_rows = function() {
- var blacklist_handler = function(e, dsd_link, source_name) {
+ var blacklist_handler = function(e, dsd_link, source_name,
+ latest_comment_container,
+ add_comment_placeholder) {
// We only want to select the new radio if the update is
// successful.
e.preventDefault();
var blacklist_options_container = this.ancestor('div');
-
- // Disable all the inputs
+ blacklist_comment_overlay(
+ e, dsd_link, source_name, latest_comment_container,
+ add_comment_placeholder, blacklist_options_container);
+ };
+
+ var blacklist_comment_overlay = function(e, dsd_link, source_name,
+ latest_comment_container,
+ add_comment_placeholder,
+ blacklist_options_container) {
+ var comment_form = Y.Node.create("<form />")
+ .appendChild(Y.Node.create("<textarea></textarea>")
+ .set("name", "comment")
+ .set("rows", "3")
+ .set("cols", "60"));
+
+ /* 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");
+
+ var submit_callback = function(data) {
+ overlay.hide();
+ var comment = "";
+ if (data.comment !== undefined) {
+ comment = data.comment[0];
+ }
+ blacklist_submit_handler(
+ e, dsd_link, source_name, comment, latest_comment_container,
+ add_comment_placeholder, blacklist_options_container);
+
+ };
+ var origin = blacklist_options_container.one('.blacklist-options');
+ var overlay = new Y.lazr.FormOverlay({
+ align: {
+ /* Align the centre of the overlay with the centre of the
+ node containing the blacklist options. */
+ node: origin,
+ points: [
+ Y.WidgetPositionAlign.CC,
+ Y.WidgetPositionAlign.CC
+ ]
+ },
+ headerContent: "<h2>Add an optional comment</h2>",
+ form_content: comment_form,
+ form_submit_button: submit_button,
+ form_cancel_button: cancel_button,
+ form_submit_callback: submit_callback,
+ visible: true
+ });
+ overlay.render();
+ };
+
+ var blacklist_submit_handler = function(e, dsd_link, source_name,
+ comment, latest_comment_container,
+ add_comment_placeholder,
+ blacklist_options_container) {
+ // Disable all the inputs.
blacklist_options_container.all('input').set('disabled', 'disabled');
e.target.prepend('<img src="/@@/spinner" />');
@@ -58,7 +125,10 @@
}
fade_to_gray.run();
});
- },
+ add_comment(
+ updated_entry, add_comment_placeholder,
+ latest_comment_container);
+ },
failure: function(id, response) {
blacklist_options_container.one('img').remove();
blacklist_options_container.all(
@@ -66,7 +136,8 @@
}
},
parameters: {
- all: blacklist_all
+ all: blacklist_all,
+ comment: comment
}
};
@@ -79,13 +150,20 @@
* api uri.
*
* @param blacklist_options {Node} The node containing the blacklist
- * options.
+ * options.
* @param source_name {string} The name of the source to update.
+ * @param dsd_link {string} The uri for the distroseriesdifference object.
+ * @param latest_comment_container {Node} The node containing the last
+ * comment.
+ * @param add_comment_placeholder {Node} The node containing the "add
+ * comment" slot.
*/
var setup_blacklist_options = function(
- blacklist_options, source_name, dsd_link) {
+ blacklist_options, source_name, dsd_link, latest_comment_container,
+ add_comment_placeholder) {
Y.on('click', blacklist_handler, blacklist_options.all('input'),
- blacklist_options, dsd_link, source_name);
+ blacklist_options, dsd_link, source_name,
+ latest_comment_container, add_comment_placeholder);
};
/**
@@ -93,7 +171,6 @@
*
* @param comment_entry {lp.client.Entry} An object representing
* a DistroSeriesDifferenceComment.
- *
* @param placeholder {Node}
* The node in which to put the latest comment HTML fragment. The
* contents of this node will be replaced.
@@ -118,18 +195,51 @@
};
/**
- * Handle the add comment event.
+ * This method adds a comment in the UI. It appends a comment to the
+ * list of comments and updates the latest comments slot.
+ *
+ * @param comment_entry {Comment} A comment as returns by the api.
+ * @param add_comment_placeholder {Node} The node that contains the
+ * relevant comment fields.
+ * @param latest_comment_placeholder {Node} The node that contains the
+ * latest comment.
+ */
+ var add_comment = function(comment_entry, add_comment_placeholder,
+ latest_comment_placeholder) {
+ // Grab the XHTML representation of the comment
+ // and prepend it to the list of comments.
+ var config = {
+ on: {
+ success: function(comment_html) {
+ var comment_node = Y.Node.create(comment_html);
+ add_comment_placeholder.insert(comment_node, 'before');
+ var reveal = Y.lazr.effects.slide_out(comment_node);
+ reveal.on("end", function() {
+ Y.lazr.anim.green_flash(
+ {node: comment_node}).run();
+ });
+ reveal.run();
+ }
+ },
+ accept: Y.lp.client.XHTML
+ };
+ lp_client.get(comment_entry.get('self_link'), config);
+ update_latest_comment(comment_entry, latest_comment_placeholder);
+ };
+
+ /**
+ * Handle the add comment event triggered by the 'add comment' form.
*
* This method adds a comment via the API and update the UI.
*
* @param comment_form {Node} The node that contains the relevant comment
- * fields.
+ * fields.
+ * @param latest_comment_placeholder {Node} The node that contains the
+ * latest comment.
* @param api_uri {string} The uri for the distroseriesdifference to which
- * the comment is to be added.
- *
- * @param cb_success {function}
- * Called when a comment has successfully been added. (Deferreds would
- * be awesome right about now.)
+ * the comment is to be added.
+ * @param cb_success {function} Called when a comment has successfully
+ * been added. (Deferreds would be awesome right about now.)
*/
var add_comment_handler = function(
comment_form, latest_comment_placeholder, api_uri, cb_success) {
@@ -144,26 +254,9 @@
}
var success_handler = function(comment_entry) {
- // Grab the XHTML representation of the comment
- // and prepend it to the list of comments.
- var config = {
- on: {
- success: function(comment_html) {
- var comment_node = Y.Node.create(comment_html);
- comment_form.insert(comment_node, 'before');
- var reveal = Y.lazr.effects.slide_out(comment_node);
- reveal.on("end", function() {
- Y.lazr.anim.green_flash(
- {node: comment_node}).run();
- });
- reveal.run();
- }
- },
- accept: Y.lp.client.XHTML
- };
- lp_client.get(comment_entry.get('self_link'), config);
+ add_comment(
+ comment_entry, comment_form, latest_comment_placeholder);
comment_form.one('textarea').set('value', '');
- update_latest_comment(comment_entry, latest_comment_placeholder);
cb_success();
};
var failure_handler = function(id, response) {
@@ -205,9 +298,9 @@
* the event handlers.
*
* @param placeholder {Node} The node that is to contain the comment
- * fields.
+ * fields.
* @param api_uri {string} The uri for the distroseriesdifference to which
- * the comment is to be added.
+ * the comment is to be added.
*/
var setup_add_comment = function(
placeholder, latest_comment_placeholder, api_uri) {
@@ -294,11 +387,26 @@
parent_distro_name,
parent_series_name
].join('/');
+ var latest_comment_container =
+ args.master_container.one('td.latest-comment-fragment');
+ // The add comment slot is only available when the user has the
+ // right to add comments.
+ var add_comment_placeholder =
+ args.container.one('div.add-comment-placeholder');
+ if (add_comment_placeholder !== null) {
+ setup_add_comment(
+ add_comment_placeholder,
+ latest_comment_container,
+ api_uri);
+ }
// The blacklist slot with a class 'blacklist-options' is only
// available when the user has the right to blacklist.
var blacklist_slot = args.container.one('div.blacklist-options');
if (blacklist_slot !== null) {
- setup_blacklist_options(blacklist_slot, source_name, api_uri);
+ setup_blacklist_options(
+ blacklist_slot, source_name, api_uri,
+ latest_comment_container,
+ add_comment_placeholder);
}
// If the user has not the right to blacklist, we disable
// the blacklist slot.
@@ -308,16 +416,6 @@
disabled_blacklist_slot
.all('input').set('disabled', 'disabled');
}
- // The add comment slot is only available when the user has the
- // right to add comments.
- var add_comment_placeholder =
- args.container.one('div.add-comment-placeholder');
- if (add_comment_placeholder !== null) {
- setup_add_comment(
- add_comment_placeholder,
- args.master_container.one('td.latest-comment-fragment'),
- api_uri);
- }
// Set-up diffs and the means to request them.
namespace.setup_packages_diff_states(args.container, api_uri);
};
@@ -470,9 +568,8 @@
* the package diff object's status.
*
* @param container {Node} The container node displaying this package
-* diff information.
-*
-* @dsd_link {string} The uri for the distroseriesdifference object.
+* diff information.
+* @param dsd_link {string} The uri for the distroseriesdifference object.
*/
namespace.setup_pending_package_diff = function(container, dsd_link) {
var parent = container.hasClass('parent');
@@ -538,8 +635,7 @@
* Add a button to start package diff computation.
*
* @param row {Node} The container node for this package extra infos.
-*
-* @dsd_link {string} The uri for the distroseriesdifference object.
+* @param dsd_link {string} The uri for the distroseriesdifference object.
*/
namespace.setup_request_derived_diff = function(container, dsd_link) {
// Setup handler for diff requests. There should either zero or
@@ -555,8 +651,7 @@
* - Start pollers for pending packages diffs.
*
* @param row {Node} The container node for this package extra infos.
-*
-* @dsd_link {string} The uri for the distroseriesdifference object.
+* @param dsd_link {string} The uri for the distroseriesdifference object.
*/
namespace.setup_packages_diff_states = function(container, dsd_link) {
// Attach pollers for pending packages diffs.
@@ -571,9 +666,8 @@
* Helper method to add a message node inside the placeholder.
*
* @param container {Node} The container in which to look for the
-* placeholder.
-*
-* @msg_node {Node} The message node to add.
+* placeholder.
+* @param msg_node {Node} The message node to add.
*/
namespace.add_msg_node = function(container, msg_node) {
container.one('.package-diff-placeholder')
@@ -585,8 +679,7 @@
* Start package diff computation. Update package diff status to PENDING.
*
* @param row {Node} The container node for this package diff.
-*
-* @dsd_link {string} The uri for the distroseriesdifference object.
+* @param dsd_link {string} The uri for the distroseriesdifference object.
*/
var compute_package_diff = function(container, dsd_link) {
var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
=== modified file 'lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js 2011-07-08 05:12:39 +0000
+++ lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js 2011-08-04 08:09:43 +0000
@@ -165,7 +165,7 @@
Assert.isTrue(package_diff.hasClass('COMPLETED'));
}, dsd_details.poll_interval);
}, dsd_details.poll_interval);
- }
+ }
};
suite.add(new Y.Test.Case(testPackageDiffUpdate));
=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-06-22 10:48:40 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-08-04 08:09:43 +0000
@@ -20,20 +20,7 @@
</metal:macro-parent>
</metal:macros>
- <div
- tal:attributes="class view/blacklist_options_css_class"
- style="float:right">
- <dl>
- <dt>Ignored:</dt>
- <dd>
- <form>
- <tal:replace
- replace="structure view/widgets/blacklist_options" />
- </form>
- </dd>
- </dl>
- </div>
-
+ <div style="float: left;">
<dl>
<dt>Binary descriptions:</dt>
<dd><ul>
@@ -124,6 +111,22 @@
</tal:package-diffs>
</dl>
+ </div>
+ <div
+ tal:attributes="class view/blacklist_options_css_class"
+ style="float:left">
+ <dl>
+ <dt>Ignored:</dt>
+ <dd>
+ <form>
+ <tal:replace
+ replace="structure view/widgets/blacklist_options" />
+ </form>
+ </dd>
+ </dl>
+ </div>
+
+ <div style="clear:both;"> </div>
<strong>Comments:</strong>
<tal:conversation replace="structure view/@@+render"/>
