Merge lp:~nigelbabu/launchpad/patch-edit-684548 into lp:launchpad

[Nigel Babu]

[r=danilo][bug=684548] Adding an edit button in the inline comments for attachments

= Description =
The 'edit' URL for most attachments is hard to find since its only next to the patch link in the right portlet. This fix will ensure that URL shows up in the bug comment as well.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/templates/bugcomment-box.pt

[Данило Шеган (danilo)]

As discussed on IRC, the test lib/lp/bugs/stories/bugattachments/xx-attachments-to-bug-report.txt would need fixing, and you should at least extend it to provide a check for the contents of the link. (You already talked with a few people about starting a unit test, but came up empty: it'd still be desireable to do that, but I won't block on it for such a tiny change like this)

Also, I believe parentheses next to each other in eg. "attachment.tar (edit) (17 bytes...)" look very bad. You should use CSS classes "sprite edit" for the edit link (with most likely no text at all: but I suggest you try out a few different options and see how it works out).

review: Needs Fixing

[Nigel Babu (nigelbabu)]

Personally, I think its much nicer to use edit, since that's what is used on portlet s well. Another reason being, adding the "edit sprite" classes puts the sprite on the left side where there's the attachment/patch icon.

[Данило Шеган (danilo)]

The fact that the portlet uses something out of date is not a reason to introduce more out-of-dateness. If it appears all the way to the left, you are putting them on the wrong A tag (i.e. it looks nice if I do something like http://pastebin.ubuntu.com/679537/).

When you remove the link text though, you'll probably want to introduce a CSS class on the ID link so you can easily find it with find_tags_by_class or similar.

Also note that your reformatting of the doctest headers (lint probably complained) is slightly incorrect. "\n" is an actual new line indicator, and you should make the follow-up line be of the same length as the text above it. Also, since this is RST (http://en.wikipedia.org/wiki/ReStructuredText), you should use different characters for different levels of headings, eg.

File Attachments of the Bug Report
==================================
...
Patch attachments are shown before non-patch attachments
--------------------------------------------------------
...

[Данило Шеган (danilo)]

This now looks good, thanks for all the improvements!

review: Approve

=== modified file 'lib/lp/bugs/stories/bugattachments/xx-attachments-to-bug-report.txt'
--- lib/lp/bugs/stories/bugattachments/xx-attachments-to-bug-report.txt	2011-06-28 15:04:29 +0000
+++ lib/lp/bugs/stories/bugattachments/xx-attachments-to-bug-report.txt	2011-09-01 11:25:29 +0000
@@ -1,4 +1,5 @@
-= File Attachments of the Bug Report =
+File Attachments of the Bug Report
+==================================
 
 File attachments of the bug report itself are displayed in the list of
 comments. Since the text of this comment is already displayed as the
@@ -52,8 +53,14 @@
     >>> link = browser.getLink('sample data')
     >>> print link.url
     http://bugs.launchpad.dev/jokosher/+bug/11/+attachment/1/+files/test.txt
-
-== Patch attachments are shown before non-patch attachments ==
+    >>> edit_link = find_tags_by_class(browser.contents, 'edit sprite')[1]
+    >>> print edit_link
+    <a alt="edit" class="sprite edit"...
+    ...href="/jokosher/+bug/11/+attachment/1"></a>
+
+
+Patch attachments are shown before non-patch attachments
+--------------------------------------------------------
 
     >>> login("[e-mail]")
     >>> launchbag.clear()
@@ -70,4 +77,4 @@
     ...a patch...
     ...Bug attachments...
     ...sample data...
-    ... 
+    ...

=== modified file 'lib/lp/bugs/templates/bugcomment-box.pt'
--- lib/lp/bugs/templates/bugcomment-box.pt	2011-05-31 14:44:29 +0000
+++ lib/lp/bugs/templates/bugcomment-box.pt	2011-09-01 11:25:29 +0000
@@ -61,6 +61,7 @@
         <a tal:attributes="href python: view.proxiedUrlOfLibraryFileAlias(attachment)"
             tal:content="attachment/title"
             class="sprite download-icon">foo.txt</a>
+        <a tal:attributes="href attachment/fmt:url" alt="edit" class="sprite edit"></a>
         (<span
            tal:replace="attachment/libraryfile/content/filesize/fmt:bytes" />,
         <span tal:replace="attachment/libraryfile/mimetype" />)
@@ -71,6 +72,7 @@
       <li tal:repeat="attachment comment/patches" class="download-attachment">
         <a tal:attributes="href python: view.proxiedUrlOfLibraryFileAlias(attachment)"
            tal:content="attachment/title" class="sprite haspatch-icon">foo.txt</a>
+        <a tal:attributes="href attachment/fmt:url" alt="edit" class="sprite edit"></a>
         (<span
            tal:replace="attachment/libraryfile/content/filesize/fmt:bytes" />,
         <span tal:replace="attachment/libraryfile/mimetype" />)