Launchpad itself

Merge lp:~adeuring/launchpad/bug-344054 into lp:launchpad

  1. bug-344054
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Approved by: Gavin Panella
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/bug-344054
Merge into: lp:launchpad
Diff against target: 231 lines (+111/-56)
4 files modified
lib/lp/bugs/browser/bug.py (+16/-1)
lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt (+46/-30)
lib/lp/bugs/stories/bugs/xx-bug-index.txt (+3/-3)
lib/lp/bugs/templates/bug-portlet-attachments.pt (+46/-22)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-344054
Reviewer Review Type Date Requested Status
Martin Albisetti (community) ui Approve
Curtis Hovey (community) ui* Approve
Gavin Panella (community) code Approve
Review via email: mp+15857@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :
Download full text (3.2 KiB)

This branch adds a separate portlet to show patches on a bug page; the portlet does no longer show them (bug 344054).

The implementation is quite straightfoward, I think:

  - the template lib/lp/bugs/templates/bug-portlet-attachments.pt now creates two portlets instead of one
  - class Bug has two new properties returning the list of patches and non-patch attachmentsm respectively
  - doc test for the new properties
  - changed pagetest.

test: ./bin/test -vv -f lp.bugs bugattachments

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/doc/bugattachments.txt
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt
  lib/lp/bugs/templates/bug-portlet-attachments.pt

== Pylint notices ==

lib/lp/bugs/interfaces/bug.py
    49: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    55: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    476: [C0322, IBug.addAttachment] Operator not preceded by a space
    comment=Text(), filename=TextLine(), is_patch=Bool(),
    ^
    content_type=TextLine(), description=Text())
    @export_factory_operation(IBugAttachment, [])
    def addAttachment(owner, data, comment, filename, is_patch=False,
    content_type=None, description=None):
    611: [C0322, IBug.getNominations] Operator not preceded by a space
    nominations=List(
    ^
    title=_("Nominations to search through."),
    value_type=Reference(schema=Interface), # IBugNomination
    required=False))
    @operation_returns_collection_of(Interface) # IBugNomination
    @export_read_operation()
    def getNominations(target=None, nominations=None):
    691: [C0322, IBug.markUserAffected] Operator not preceded by a space
    required=False, default=True))
    ^
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def markUserAffected(user, affected=True):
    706: [C0322, IBug.setCommentVisibility] Operator not preceded by a space
    required=True),
    ^
    visible=Bool(title=_('Show this comment?'), required=True))
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def setCommentVisibility(user, comment_number, visible):
    718: [C0322, IBug.linkHWSubmission] Operator not preceded by a space
    Interface, title=_('A HWDB submission'), required=True))
    ^
    @export_write_operation()
    def linkHWSubmission(submission):
    725: [C0322, IBug.unlinkHWSubmission] Operator not preceded by a space
    Interface, title=_('A HWDB submission'), required=True))
    ^
    @export_write_operation()
    def unlinkHWSubmission(submission):

lib/lp/bugs/model/bug.py
    25: [F0401] Unable to import 'email.Utils' (No module named Utils)
    38: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    40: [F0401] Unable to import 'lazr.lifecyc...

Read more...

Revision history for this message
Gavin Panella (allenap) wrote :

As discussed on IRC, the new properties are probably better off as
view properties.

  <allenap> adeuring: My gut feeling is that regular_attachments and
    patches should be view properties rather than model
    properties. Did you consider that?

  <adeuring> allenap: right, good idea. didn't think about that

  <allenap> adeuring: My feeling is that they should not be exported
    in any case. If you're a consumer of the API, I think it's better
    to get the list of all attachments and split it up from there; the
    criteria for a patch are unambiguous and easy to test. Exporting
    two (well, three) attachment fields, we also encourage people to
    make multiple API calls.

  <adeuring> allenap: yeah; I'll use browser methods

review: Needs Fixing
Revision history for this message
Gavin Panella (allenap) wrote :

Looks good :)

These are UI comments really, but the renamed "Add attachment or patch" links seem too prominent in the portlets. I wonder if we need them at all, or if just the green plus icon could be shown next to the portlet title, or if they could remain as simple "Add" or "Add another". Anyway, those are considerations for a UI reviewer really.

Should the titles remain as "Add attachment or patch", perhaps the link below the comment form in the body of the bugs page should also be changed to match? It currently says just "Add attachment".

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Abel.

I tried your branch. This is what I saw after uploading an attachment and a patch.
    http://people.canonical.com/~curtis/patch.png

I also noted that the comment form reads
    (+) Add an attachment
But the portlets reads
    (+) Add attachment or patch

Since the add attachment link is crafted, I think we should make the actions we expected the user to perform clear. We avoid the use of articles like 'a' and 'an' in our links, I think we should fix the link at the bottom of the page:

Page:
    (+) Add attachment or patch

Bug attachment portlet:
    (+) Add attachment

Patch portlet:
    (+) Add patch

review: Needs Information (ui)
Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Curtis,

changes applied.

Abel

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2009-12-09 11:14:16 +0000
+++ lib/lp/bugs/browser/bug.py 2009-12-09 16:25:58 +0000
@@ -234,7 +234,7 @@

     def addcomment(self):
         """Return the 'Comment or attach file' Link."""
- text = 'Add an attachment'
+ text = 'Add attachment or patch'
         return Link('+addcomment', text, icon='add')

     def addbranch(self):

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-09-23 11:10:00 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-12-09 16:30:45 +0000
@@ -127,10 +127,10 @@
     >>> print user_browser.getLink('List open bugs').url
     http://launchpad.dev/ubuntu/+source/mozilla-firefox/+bugs

-There's also a link on the page that will take the user to the "Add an
-attachment" page, for use when JavaScript isn't available.
+There's also a link on the page that will take the user to the "Add
+attachment or patch" page, for use when JavaScript isn't available.

- >>> print user_browser.getLink('Add an attachment').url
+ >>> print user_browser.getLink('Add attachment or patch').url
     http://bugs.launchpad.dev/ubuntu/+source/.../+bug/1/+addcomment

=== modified file 'lib/lp/bugs/templates/bug-portlet-attachments.pt'
--- lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-12-09 11:14:16 +0000
+++ lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-12-09 16:24:32 +0000
@@ -22,7 +22,7 @@
     <ul>
       <li>
         <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
- class="sprite add">Add attachment or patch</a>
+ class="sprite add">Add attachment</a>
       </li>
     </ul>
   </div>
@@ -44,7 +44,7 @@
     <ul>
       <li>
         <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
- class="sprite add">Add attachment or patch</a>
+ class="sprite add">Add patch</a>
       </li>
     </ul>
   </div>

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Abel.

This looks good. Martin or Michael should provide their opinion too.

review: Approve (ui*)
Revision history for this message
Martin Albisetti (beuno) wrote :

Curtis nailed it.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2009-11-26 14:50:55 +0000
+++ lib/lp/bugs/browser/bug.py 2009-12-09 16:34:16 +0000
@@ -52,6 +52,7 @@
52from canonical.launchpad.interfaces._schema_circular_imports import IBug52from canonical.launchpad.interfaces._schema_circular_imports import IBug
53from canonical.launchpad.webapp.interfaces import ILaunchBag, NotFoundError53from canonical.launchpad.webapp.interfaces import ILaunchBag, NotFoundError
54from lp.bugs.interfaces.bug import IBugSet54from lp.bugs.interfaces.bug import IBugSet
55from lp.bugs.interfaces.bugattachment import BugAttachmentType
55from lp.bugs.interfaces.bugtask import (56from lp.bugs.interfaces.bugtask import (
56 BugTaskSearchParams, BugTaskStatus, IBugTask, IFrontPageBugTaskSearch)57 BugTaskSearchParams, BugTaskStatus, IBugTask, IFrontPageBugTaskSearch)
57from lp.bugs.interfaces.bugwatch import IBugWatchSet58from lp.bugs.interfaces.bugwatch import IBugWatchSet
@@ -233,7 +234,7 @@
233234
234 def addcomment(self):235 def addcomment(self):
235 """Return the 'Comment or attach file' Link."""236 """Return the 'Comment or attach file' Link."""
236 text = 'Add an attachment'237 text = 'Add attachment or patch'
237 return Link('+addcomment', text, icon='add')238 return Link('+addcomment', text, icon='add')
238239
239 def addbranch(self):240 def addbranch(self):
@@ -461,6 +462,20 @@
461 else:462 else:
462 return 'subscribed-false %s' % dup_class463 return 'subscribed-false %s' % dup_class
463464
465 @property
466 def regular_attachments(self):
467 """The list of bug attachments that are not patches."""
468 return [attachment
469 for attachment in self.context.attachments
470 if attachment.type != BugAttachmentType.PATCH]
471
472 @property
473 def patches(self):
474 """The list of bug attachments that are patches."""
475 return [attachment
476 for attachment in self.context.attachments
477 if attachment.type == BugAttachmentType.PATCH]
478
464479
465class BugView(LaunchpadView, BugViewMixin):480class BugView(LaunchpadView, BugViewMixin):
466 """View class for presenting information about an `IBug`.481 """View class for presenting information about an `IBug`.
467482
=== modified file 'lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt'
--- lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2009-08-31 20:03:29 +0000
+++ lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2009-12-09 16:34:17 +0000
@@ -1,44 +1,60 @@
1We can also edit the attachment details, let's navigate to that page.1We can also edit the attachment details, let's navigate to that page.
22
3 >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')3 >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
4 >>> user_browser.getLink(url='+attachment/1').click()4 >>> user_browser.getLink(url='+attachment/1').click()
5 >>> user_browser.url5 >>> user_browser.url
6 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1'6 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1'
77
8 >>> 'Bug #1 - Edit attachment' in user_browser.contents8 >>> 'Bug #1 - Edit attachment' in user_browser.contents
9 True9 True
1010
11There's also an option to cancel, which takes you back to the bug11There's also an option to cancel, which takes you back to the bug
12page, maintaining the firefox context.12page, maintaining the firefox context.
1313
14 >>> user_browser.getLink('Cancel')14 >>> user_browser.getLink('Cancel')
15 <Link text='Cancel' url='http://bugs.launchpad.dev/firefox/+bug/1'>15 <Link text='Cancel' url='http://bugs.launchpad.dev/firefox/+bug/1'>
1616
17After editing the attachment details (we leave some leading and trailing17After editing the attachment details (we leave some leading and trailing
18whitespace to test that's correctly stripped)...18whitespace to test that's correctly stripped)...
1919
20 >>> user_browser.getControl('Title').value = ' Another title '20 >>> user_browser.getControl('Title').value = ' Another title '
21 >>> user_browser.getControl('Content Type').value = 'text/html'21 >>> user_browser.getControl('Content Type').value = 'text/html'
22 >>> user_browser.getControl('Change').click()22 >>> user_browser.getControl('Change').click()
2323
24...we're redirected to the bug page24...we're redirected to the bug page
2525
26 >>> user_browser.url26 >>> user_browser.url
27 'http://bugs.launchpad.dev/firefox/+bug/1'27 'http://bugs.launchpad.dev/firefox/+bug/1'
2828
29 >>> 'Another title' in user_browser.contents29 >>> 'Another title' in user_browser.contents
30 True30 True
3131
32And if we edit it to be a patch the content type will be automatically32We can edit the attachment to be a patch.
33changed to be text/plain.33
3434 >>> user_browser.getLink(url='+attachment/1').click()
35 >>> user_browser.getLink(url='+attachment/1').click()35 >>> user_browser.getControl('This attachment is a patch').selected = True
36 >>> user_browser.getControl('This attachment is a patch').selected = True36 >>> user_browser.getControl('Change').click()
37 >>> user_browser.getControl('Change').click()37
3838 >>> user_browser.url
39 >>> user_browser.url39 'http://bugs.launchpad.dev/firefox/+bug/1'
40 'http://bugs.launchpad.dev/firefox/+bug/1'40
4141The attachment that became a patch is now shown in the portlet "Patches"...
42 >>> user_browser.getLink(url='+attachment/1').click()42
43 >>> user_browser.getControl('Content Type').value43 >>> patches = find_portlet(user_browser.contents, 'Patches')
44 'text/plain'44 >>> for li_tag in patches.findAll('li', 'download-attachment'):
45 ... print li_tag.a.renderContents()
46 Another title
47
48...while it is gone from the portlet "Bug attachments".
49
50 >>> attachments = find_portlet(user_browser.contents, 'Bug attachments')
51 >>> for li_tag in attachments.findAll('li', 'download-attachment'):
52 ... print li_tag.a.renderContents()
53 bar.txt
54
55The content type of a patch is automatically changed to text/plain.
56
57 >>> user_browser.getLink(url='+attachment/1').click()
58 >>> user_browser.getControl('Content Type').value
59 'text/plain'
60
4561
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-09-23 11:10:00 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-12-09 16:34:17 +0000
@@ -127,10 +127,10 @@
127 >>> print user_browser.getLink('List open bugs').url127 >>> print user_browser.getLink('List open bugs').url
128 http://launchpad.dev/ubuntu/+source/mozilla-firefox/+bugs128 http://launchpad.dev/ubuntu/+source/mozilla-firefox/+bugs
129129
130There's also a link on the page that will take the user to the "Add an130There's also a link on the page that will take the user to the "Add
131attachment" page, for use when JavaScript isn't available.131attachment or patch" page, for use when JavaScript isn't available.
132132
133 >>> print user_browser.getLink('Add an attachment').url133 >>> print user_browser.getLink('Add attachment or patch').url
134 http://bugs.launchpad.dev/ubuntu/+source/.../+bug/1/+addcomment134 http://bugs.launchpad.dev/ubuntu/+source/.../+bug/1/+addcomment
135135
136136
137137
=== modified file 'lib/lp/bugs/templates/bug-portlet-attachments.pt'
--- lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-09-22 16:35:00 +0000
+++ lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-12-09 16:34:17 +0000
@@ -2,26 +2,50 @@
2 xmlns:tal="http://xml.zope.org/namespaces/tal"2 xmlns:tal="http://xml.zope.org/namespaces/tal"
3 xmlns:metal="http://xml.zope.org/namespaces/metal"3 xmlns:metal="http://xml.zope.org/namespaces/metal"
4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
5 class="portlet" id="portlet-attachments"5 tal:omit-tag="">
6 tal:condition="context/attachments">6 <div tal:condition="view/regular_attachments" class="portlet"
7 <h2>Bug attachments</h2>7 id="portlet-attachments">
8 <ul>8 <h2>Bug attachments</h2>
9 <li class="download-attachment"9 <ul>
10 tal:repeat="attachment context/attachments">10 <li class="download-attachment"
11 <a tal:attributes="href attachment/libraryfile/http_url"11 tal:repeat="attachment view/regular_attachments">
12 tal:content="attachment/title"12 <a tal:attributes="href attachment/libraryfile/http_url"
13 class="sprite download-icon">13 tal:content="attachment/title"
14 Attachment Title14 class="sprite download-icon">
15 </a>15 Attachment Title
16 <small>16 </a>
17 (<a tal:attributes="href attachment/fmt:url">edit</a>)17 <small>
18 </small>18 (<a tal:attributes="href attachment/fmt:url">edit</a>)
19 </li>19 </small>
20 </ul>20 </li>
21 <ul>21 </ul>
22 <li>22 <ul>
23 <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"23 <li>
24 class="sprite add">Add</a>24 <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
25 </li>25 class="sprite add">Add attachment</a>
26 </ul>26 </li>
27 </ul>
28 </div>
29 <div tal:condition="view/patches" class="portlet" id="portlet-patches">
30 <h2>Patches</h2>
31 <ul>
32 <li class="download-attachment"
33 tal:repeat="attachment view/patches">
34 <a tal:attributes="href attachment/libraryfile/http_url"
35 tal:content="attachment/title"
36 class="sprite download-icon">
37 Attachment Title
38 </a>
39 <small>
40 (<a tal:attributes="href attachment/fmt:url">edit</a>)
41 </small>
42 </li>
43 </ul>
44 <ul>
45 <li>
46 <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
47 class="sprite add">Add patch</a>
48 </li>
49 </ul>
50 </div>
27</div>51</div>