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