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
1=== modified file 'lib/lp/bugs/browser/bug.py'
2--- lib/lp/bugs/browser/bug.py 2009-11-26 14:50:55 +0000
3+++ lib/lp/bugs/browser/bug.py 2009-12-09 16:34:16 +0000
4@@ -52,6 +52,7 @@
5 from canonical.launchpad.interfaces._schema_circular_imports import IBug
6 from canonical.launchpad.webapp.interfaces import ILaunchBag, NotFoundError
7 from lp.bugs.interfaces.bug import IBugSet
8+from lp.bugs.interfaces.bugattachment import BugAttachmentType
9 from lp.bugs.interfaces.bugtask import (
10 BugTaskSearchParams, BugTaskStatus, IBugTask, IFrontPageBugTaskSearch)
11 from lp.bugs.interfaces.bugwatch import IBugWatchSet
12@@ -233,7 +234,7 @@
13
14 def addcomment(self):
15 """Return the 'Comment or attach file' Link."""
16- text = 'Add an attachment'
17+ text = 'Add attachment or patch'
18 return Link('+addcomment', text, icon='add')
19
20 def addbranch(self):
21@@ -461,6 +462,20 @@
22 else:
23 return 'subscribed-false %s' % dup_class
24
25+ @property
26+ def regular_attachments(self):
27+ """The list of bug attachments that are not patches."""
28+ return [attachment
29+ for attachment in self.context.attachments
30+ if attachment.type != BugAttachmentType.PATCH]
31+
32+ @property
33+ def patches(self):
34+ """The list of bug attachments that are patches."""
35+ return [attachment
36+ for attachment in self.context.attachments
37+ if attachment.type == BugAttachmentType.PATCH]
38+
39
40 class BugView(LaunchpadView, BugViewMixin):
41 """View class for presenting information about an `IBug`.
42
43=== modified file 'lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt'
44--- lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2009-08-31 20:03:29 +0000
45+++ lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2009-12-09 16:34:17 +0000
46@@ -1,44 +1,60 @@
47 We can also edit the attachment details, let's navigate to that page.
48
49- >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
50- >>> user_browser.getLink(url='+attachment/1').click()
51- >>> user_browser.url
52- 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1'
53+ >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
54+ >>> user_browser.getLink(url='+attachment/1').click()
55+ >>> user_browser.url
56+ 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1'
57
58- >>> 'Bug #1 - Edit attachment' in user_browser.contents
59- True
60+ >>> 'Bug #1 - Edit attachment' in user_browser.contents
61+ True
62
63 There's also an option to cancel, which takes you back to the bug
64 page, maintaining the firefox context.
65
66- >>> user_browser.getLink('Cancel')
67- <Link text='Cancel' url='http://bugs.launchpad.dev/firefox/+bug/1'>
68+ >>> user_browser.getLink('Cancel')
69+ <Link text='Cancel' url='http://bugs.launchpad.dev/firefox/+bug/1'>
70
71 After editing the attachment details (we leave some leading and trailing
72 whitespace to test that's correctly stripped)...
73
74- >>> user_browser.getControl('Title').value = ' Another title '
75- >>> user_browser.getControl('Content Type').value = 'text/html'
76- >>> user_browser.getControl('Change').click()
77+ >>> user_browser.getControl('Title').value = ' Another title '
78+ >>> user_browser.getControl('Content Type').value = 'text/html'
79+ >>> user_browser.getControl('Change').click()
80
81 ...we're redirected to the bug page
82
83- >>> user_browser.url
84- 'http://bugs.launchpad.dev/firefox/+bug/1'
85-
86- >>> 'Another title' in user_browser.contents
87- True
88-
89-And if we edit it to be a patch the content type will be automatically
90-changed to be text/plain.
91-
92- >>> user_browser.getLink(url='+attachment/1').click()
93- >>> user_browser.getControl('This attachment is a patch').selected = True
94- >>> user_browser.getControl('Change').click()
95-
96- >>> user_browser.url
97- 'http://bugs.launchpad.dev/firefox/+bug/1'
98-
99- >>> user_browser.getLink(url='+attachment/1').click()
100- >>> user_browser.getControl('Content Type').value
101- 'text/plain'
102+ >>> user_browser.url
103+ 'http://bugs.launchpad.dev/firefox/+bug/1'
104+
105+ >>> 'Another title' in user_browser.contents
106+ True
107+
108+We can edit the attachment to be a patch.
109+
110+ >>> user_browser.getLink(url='+attachment/1').click()
111+ >>> user_browser.getControl('This attachment is a patch').selected = True
112+ >>> user_browser.getControl('Change').click()
113+
114+ >>> user_browser.url
115+ 'http://bugs.launchpad.dev/firefox/+bug/1'
116+
117+The attachment that became a patch is now shown in the portlet "Patches"...
118+
119+ >>> patches = find_portlet(user_browser.contents, 'Patches')
120+ >>> for li_tag in patches.findAll('li', 'download-attachment'):
121+ ... print li_tag.a.renderContents()
122+ Another title
123+
124+...while it is gone from the portlet "Bug attachments".
125+
126+ >>> attachments = find_portlet(user_browser.contents, 'Bug attachments')
127+ >>> for li_tag in attachments.findAll('li', 'download-attachment'):
128+ ... print li_tag.a.renderContents()
129+ bar.txt
130+
131+The content type of a patch is automatically changed to text/plain.
132+
133+ >>> user_browser.getLink(url='+attachment/1').click()
134+ >>> user_browser.getControl('Content Type').value
135+ 'text/plain'
136+
137
138=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
139--- lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-09-23 11:10:00 +0000
140+++ lib/lp/bugs/stories/bugs/xx-bug-index.txt 2009-12-09 16:34:17 +0000
141@@ -127,10 +127,10 @@
142 >>> print user_browser.getLink('List open bugs').url
143 http://launchpad.dev/ubuntu/+source/mozilla-firefox/+bugs
144
145-There's also a link on the page that will take the user to the "Add an
146-attachment" page, for use when JavaScript isn't available.
147+There's also a link on the page that will take the user to the "Add
148+attachment or patch" page, for use when JavaScript isn't available.
149
150- >>> print user_browser.getLink('Add an attachment').url
151+ >>> print user_browser.getLink('Add attachment or patch').url
152 http://bugs.launchpad.dev/ubuntu/+source/.../+bug/1/+addcomment
153
154
155
156=== modified file 'lib/lp/bugs/templates/bug-portlet-attachments.pt'
157--- lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-09-22 16:35:00 +0000
158+++ lib/lp/bugs/templates/bug-portlet-attachments.pt 2009-12-09 16:34:17 +0000
159@@ -2,26 +2,50 @@
160 xmlns:tal="http://xml.zope.org/namespaces/tal"
161 xmlns:metal="http://xml.zope.org/namespaces/metal"
162 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
163- class="portlet" id="portlet-attachments"
164- tal:condition="context/attachments">
165- <h2>Bug attachments</h2>
166- <ul>
167- <li class="download-attachment"
168- tal:repeat="attachment context/attachments">
169- <a tal:attributes="href attachment/libraryfile/http_url"
170- tal:content="attachment/title"
171- class="sprite download-icon">
172- Attachment Title
173- </a>
174- <small>
175- (<a tal:attributes="href attachment/fmt:url">edit</a>)
176- </small>
177- </li>
178- </ul>
179- <ul>
180- <li>
181- <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
182- class="sprite add">Add</a>
183- </li>
184- </ul>
185+ tal:omit-tag="">
186+ <div tal:condition="view/regular_attachments" class="portlet"
187+ id="portlet-attachments">
188+ <h2>Bug attachments</h2>
189+ <ul>
190+ <li class="download-attachment"
191+ tal:repeat="attachment view/regular_attachments">
192+ <a tal:attributes="href attachment/libraryfile/http_url"
193+ tal:content="attachment/title"
194+ class="sprite download-icon">
195+ Attachment Title
196+ </a>
197+ <small>
198+ (<a tal:attributes="href attachment/fmt:url">edit</a>)
199+ </small>
200+ </li>
201+ </ul>
202+ <ul>
203+ <li>
204+ <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
205+ class="sprite add">Add attachment</a>
206+ </li>
207+ </ul>
208+ </div>
209+ <div tal:condition="view/patches" class="portlet" id="portlet-patches">
210+ <h2>Patches</h2>
211+ <ul>
212+ <li class="download-attachment"
213+ tal:repeat="attachment view/patches">
214+ <a tal:attributes="href attachment/libraryfile/http_url"
215+ tal:content="attachment/title"
216+ class="sprite download-icon">
217+ Attachment Title
218+ </a>
219+ <small>
220+ (<a tal:attributes="href attachment/fmt:url">edit</a>)
221+ </small>
222+ </li>
223+ </ul>
224+ <ul>
225+ <li>
226+ <a tal:attributes="href view/current_bugtask/fmt:url/+addcomment"
227+ class="sprite add">Add patch</a>
228+ </li>
229+ </ul>
230+ </div>
231 </div>