Merge ~ilasc/launchpad:add-delete-comment-ui into launchpad:master

Proposed by Ioana Lasc
Status: Merged
Approved by: Ioana Lasc
Approved revision: 9e7b120d76e23d3341cf9953a95d3858f0c6bec3
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:add-delete-comment-ui
Merge into: launchpad:master
Diff against target: 268 lines (+148/-4)
6 files modified
lib/lp/answers/templates/questionmessage-display.pt (+4/-0)
lib/lp/bugs/templates/bugcomment-box.pt (+4/-1)
lib/lp/code/templates/codereviewcomment-header.pt (+4/-1)
lib/lp/services/messages/javascript/messages.edit.js (+79/-1)
lib/lp/services/messages/javascript/tests/test_messages.edit.html (+2/-0)
lib/lp/services/messages/javascript/tests/test_messages.edit.js (+55/-1)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+406012@code.launchpad.net

Commit message

Add delete comment UI

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

This basically all looks good as far as it goes, thanks! As discussed on Mattermost, though, I think we need to wire up some kind of confirmation here before landing this: it feels a little too easy to accidentally delete messages right now, either due to not having very good pointer coordination or by not understanding what the icon means.

review: Needs Fixing
Revision history for this message
Ioana Lasc (ilasc) :
82c1799... by Ioana Lasc

Shorten revision list heading

9e7b120... by Ioana Lasc

Add confirmation dialog to delete comment

Revision history for this message
Ioana Lasc (ilasc) wrote :

Added a dialog popup for confirmation prior to actual deletion, screenshots here:

https://chat.canonical.com/canonical/pl/8xh5eduda7839pd9juhubd9bze
https://chat.canonical.com/canonical/pl/643y4suakpgfmyn7d4o9pbjcrc

MP now ready for another look.

Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/answers/templates/questionmessage-display.pt b/lib/lp/answers/templates/questionmessage-display.pt
2index 49973ab..fa39275 100644
3--- a/lib/lp/answers/templates/questionmessage-display.pt
4+++ b/lib/lp/answers/templates/questionmessage-display.pt
5@@ -59,6 +59,10 @@
6 <img class="sprite edit action-icon editable-message-edit-btn"
7 tal:condition="view/can_edit"/>
8 </td>
9+ <td>
10+ <img class="sprite remove action-icon editable-message-delete-btn"
11+ tal:condition="view/can_edit"/>
12+ </td>
13 <td class="bug-comment-index">
14 <a
15 tal:content="string: #${context/display_index}" />
16diff --git a/lib/lp/bugs/templates/bugcomment-box.pt b/lib/lp/bugs/templates/bugcomment-box.pt
17index 1e1882a..c9f1d60 100644
18--- a/lib/lp/bugs/templates/bugcomment-box.pt
19+++ b/lib/lp/bugs/templates/bugcomment-box.pt
20@@ -92,7 +92,10 @@
21 <img class="sprite edit action-icon editable-message-edit-btn"
22 tal:condition="view/can_edit"/>
23 </td>
24-
25+ <td>
26+ <img class="sprite remove action-icon editable-message-delete-btn"
27+ tal:condition="view/can_edit"/>
28+ </td>
29 <td class="bug-comment-index">
30 <a itemprop="url"
31 tal:attributes="href comment/fmt:url"
32diff --git a/lib/lp/code/templates/codereviewcomment-header.pt b/lib/lp/code/templates/codereviewcomment-header.pt
33index 3511b9f..8ddea58 100644
34--- a/lib/lp/code/templates/codereviewcomment-header.pt
35+++ b/lib/lp/code/templates/codereviewcomment-header.pt
36@@ -59,7 +59,10 @@
37 <img class="sprite edit action-icon editable-message-edit-btn"
38 tal:condition="view/can_edit"/>
39 </td>
40-
41+ <td>
42+ <img class="sprite remove action-icon editable-message-delete-btn"
43+ tal:condition="view/can_edit"/>
44+ </td>
45 <td class="bug-comment-index">
46 <a itemprop="url"
47 tal:attributes="href context/fmt:url">#</a>
48diff --git a/lib/lp/services/messages/javascript/messages.edit.js b/lib/lp/services/messages/javascript/messages.edit.js
49index 56762e8..48c9b8a 100644
50--- a/lib/lp/services/messages/javascript/messages.edit.js
51+++ b/lib/lp/services/messages/javascript/messages.edit.js
52@@ -39,6 +39,15 @@ YUI.add('lp.services.messages.edit', function(Y) {
53 "Please try again in a few minutes."
54 );
55
56+ module.confirm_delete_message = (
57+ "Are you sure you want to delete this "+
58+ "comment and all its revisions?");
59+
60+ // Making it easier to mock on tests.
61+ module.confirm = function(msg) {
62+ return confirm(msg);
63+ };
64+
65 module.htmlify_msg = function(text) {
66 text = text.replace(/&/g, "&amp;");
67 text = text.replace(/</g, "&lt;");
68@@ -70,6 +79,20 @@ YUI.add('lp.services.messages.edit', function(Y) {
69 this.lp_client.named_post(msg_url, 'editContent', config);
70 };
71
72+ module.deleteMessageContent = function(
73+ msg_path, on_success, on_failure) {
74+ var msg_url = "/api/devel" + msg_path;
75+ if (module.confirm(module.confirm_delete_message)) {
76+ var config = {
77+ on: {
78+ success: on_success,
79+ failure: on_failure
80+ }
81+ };
82+ this.lp_client.named_post(msg_url, 'deleteContent', config);
83+ }
84+ };
85+
86 module.showNotification = function(container, msg, can_dismiss) {
87 can_dismiss = can_dismiss || false;
88 // Clean up previous notification.
89@@ -168,6 +191,41 @@ YUI.add('lp.services.messages.edit', function(Y) {
90 });
91 };
92
93+ // What to do when a message is deleted in the backend.
94+ module.onMessageDeleted = function(elements, baseurl) {
95+ elements.msg_body_text.getDOMNode().innerHTML = '';
96+ // if after a delete the user wants to edit the empty
97+ // message placeholder, they won't be able to
98+ // unless we enable the text area and update button
99+ elements.textarea.getDOMNode().disabled = false;
100+ elements.update_btn.getDOMNode().disabled = false;
101+ module.hideLoading(elements.container);
102+ elements.last_edit.getDOMNode().innerHTML = (
103+ ' <a href="#" class="editable-message-last-edit-link">' +
104+ '(message and all revisions deleted a moment ago):' +
105+ '</a>');
106+
107+ // Wire click handler to the newly created
108+ // "message deleted / last edit" button.
109+ var last_edit_btn = elements.container.one(
110+ '.editable-message-last-edit-link');
111+ last_edit_btn.on('click', function(e) {
112+ e.preventDefault();
113+ module.onLastEditClick(elements, baseurl);
114+ });
115+ };
116+
117+ // What to do when a message fails to delete on the backend.
118+ module.onMessageDeleteError = function(elements, err) {
119+ // When something goes wrong at the backend, re-enable
120+ // UI elements and display an error.
121+ module.showNotification(
122+ elements.container,
123+ module.msg_edit_error_notification, true);
124+ elements.textarea.getDOMNode().disabled = false;
125+ elements.update_btn.getDOMNode().disabled = false;
126+ };
127+
128 // What to do when a message fails to update on the backend.
129 module.onMessageSaveError = function(elements, err) {
130 // When something goes wrong at the backend, re-enable
131@@ -179,6 +237,21 @@ YUI.add('lp.services.messages.edit', function(Y) {
132 elements.update_btn.getDOMNode().disabled = false;
133 };
134
135+ // What to do when a user clicks the delete button on a msg.
136+ module.onDeleteClick = function(elements, baseurl) {
137+ module.showLoading(elements.container);
138+ var textarea = elements.textarea.getDOMNode();
139+ textarea.disabled = true;
140+ elements.delete_btn.getDOMNode().disabled = true;
141+
142+ module.deleteMessageContent(
143+ baseurl,
144+ function(err) {
145+ module.onMessageDeleted(elements, baseurl);
146+ },
147+ function(err) { module.onMessageDeleteError(elements, err); }
148+ );
149+ };
150 module.fillMessageRevisions = function(elements, revisions) {
151 // Position the message revision list element.
152 revisions = revisions.reverse();
153@@ -255,7 +328,8 @@ YUI.add('lp.services.messages.edit', function(Y) {
154 "update_btn": container.one('.editable-message-update-btn'),
155 "cancel_btn": container.one('.editable-message-cancel-btn'),
156 "last_edit": container.one('.editable-message-last-edit-date'),
157- "last_edit_btn": container.one('.editable-message-last-edit-link')
158+ "last_edit_btn": container.one('.editable-message-last-edit-link'),
159+ "delete_btn": container.one('.editable-message-delete-btn')
160 };
161 // If the msg body or the msg form are not defined, don't try to do
162 // anything else.
163@@ -290,6 +364,10 @@ YUI.add('lp.services.messages.edit', function(Y) {
164 elements.cancel_btn.on('click', function(e) {
165 module.onEditCancelClick(elements);
166 });
167+
168+ elements.delete_btn.on('click', function(e) {
169+ module.onDeleteClick(elements, baseurl);
170+ });
171 };
172
173 module.setup = function() {
174diff --git a/lib/lp/services/messages/javascript/tests/test_messages.edit.html b/lib/lp/services/messages/javascript/tests/test_messages.edit.html
175index 9d394c2..ce4d0df 100644
176--- a/lib/lp/services/messages/javascript/tests/test_messages.edit.html
177+++ b/lib/lp/services/messages/javascript/tests/test_messages.edit.html
178@@ -98,6 +98,7 @@ GNU Affero General Public License version 3 (see the file LICENSE).
179 The message is above :)
180 </div>
181 <img class="sprite edit action-icon editable-message-edit-btn" />
182+ <img class="sprite remove action-icon editable-message-delete-btn" />
183
184 <div class="editable-message-form">
185 <textarea></textarea>
186@@ -141,6 +142,7 @@ GNU Affero General Public License version 3 (see the file LICENSE).
187 The message is above :)
188 </div>
189 <img class="sprite edit action-icon editable-message-edit-btn" />
190+ <img class="sprite remove action-icon editable-message-delete-btn" />
191
192 <div class="editable-message-form">
193 <textarea></textarea>
194diff --git a/lib/lp/services/messages/javascript/tests/test_messages.edit.js b/lib/lp/services/messages/javascript/tests/test_messages.edit.js
195index d9644f0..0bcccb3 100644
196--- a/lib/lp/services/messages/javascript/tests/test_messages.edit.js
197+++ b/lib/lp/services/messages/javascript/tests/test_messages.edit.js
198@@ -71,6 +71,10 @@ YUI.add('lp.services.messages.edit.test', function(Y) {
199 this.containers[0].one(".editable-message-update-btn"),
200 this.containers[1].one(".editable-message-update-btn")
201 ];
202+ this.delete_btns = [
203+ this.containers[0].one(".editable-message-delete-btn"),
204+ this.containers[1].one(".editable-message-delete-btn")
205+ ];
206
207 for(var i=0 ; i<this.containers.length ; i++) {
208 this.msg_texts[i].getDOMNode().innerHTML = (
209@@ -235,8 +239,58 @@ YUI.add('lp.services.messages.edit.test', function(Y) {
210 revisions_container.one(
211 '.message-revision-close').simulate('click');
212 Y.Assert.areSame('none', revisions_container.getStyle('display'));
213- }
214+ },
215+
216+ test_delete_comment: function() {
217+ module.setup();
218+ module.lp_client.io_provider = new Y.lp.testing.mockio.MockIo();
219+ module.confirm = function(){
220+ return 'True';
221+ };
222+ // Delete the comment
223+ this.delete_btns[1].simulate('click');
224+ module.lp_client.io_provider.success({
225+ responseText:'null',
226+ responseHeaders: {'Content-Type': 'application/json'}
227+ });
228+
229+ // Check message was deleted
230+ Y.Assert.areSame(
231+ "",
232+ this.msg_texts[1].getDOMNode().innerHTML);
233+
234+ // All forms should be released.
235+ Y.Assert.isFalse(this.textareas[1].getDOMNode().disabled);
236+ Y.Assert.isFalse(this.update_btns[1].getDOMNode().disabled);
237+ Y.Assert.isFalse(this.textareas[0].getDOMNode().disabled);
238+ Y.Assert.isFalse(this.update_btns[0].getDOMNode().disabled);
239+
240+ // Check forms and msg bodies visibility are back to normal.
241+ assertDisplayStyle(this.msg_bodies[0], 'block');
242+ assertDisplayStyle(this.msg_forms[0], 'none');
243+ assertDisplayStyle(this.msg_bodies[1], 'block');
244+ assertDisplayStyle(this.msg_forms[1], 'none');
245
246+ // Check that the request was made correctly.
247+ var last_request = module.lp_client.io_provider.last_request;
248+ Y.Assert.areSame("/api/devel/message/2", last_request.url);
249+ Y.Assert.areSame("POST", last_request.config.method);
250+ Y.Assert.areSame(
251+ "ws.op=deleteContent",
252+ last_request.config.data);
253+
254+ // Check that the "last edit" header changed.
255+ Y.Assert.areSame(":", this.last_edit[0].getDOMNode().innerHTML);
256+ Y.Assert.areSame(
257+ ' <a href="#" class="editable-message-last-edit-link">' +
258+ '(message and all revisions deleted a moment ago):</a>',
259+ this.last_edit[1].getDOMNode().innerHTML);
260+
261+ // Check the pop-up revisions list is empty after deleting
262+ var revisions = this.revision_history_lists[1].all(
263+ ".message-revision-item");
264+ Y.Assert.areSame(0, revisions.size());
265+ }
266 };
267
268 suite.add(new Y.Test.Case(TestMessageEdit));

Subscribers

People subscribed via source and target branches

to status/vote changes: