Launchpad itself

Merge lp:~adeuring/launchpad/bug-172501 into lp:launchpad/db-devel

  1. bug-172501
  2. Merge into db-devel
Proposed by Abel Deuring
Status: Merged
Approved by: Graham Binns
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/bug-172501
Merge into: lp:launchpad/db-devel
Diff against target: 574 lines (+358/-28)
9 files modified
lib/lp/bugs/browser/bugattachment.py (+98/-9)
lib/lp/bugs/browser/bugmessage.py (+23/-4)
lib/lp/bugs/browser/configure.zcml (+7/-0)
lib/lp/bugs/interfaces/bugattachment.py (+8/-1)
lib/lp/bugs/interfaces/bugmessage.py (+3/-2)
lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt (+134/-0)
lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt (+40/-12)
lib/lp/bugs/templates/bug-attachment-confirm-is-patch.pt (+40/-0)
lib/lp/bugs/templates/bug-attachment-edit.pt (+5/-0)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-172501
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Martin Albisetti (community) ui Approve
Review via email: mp+18187@code.launchpad.net

This proposal supersedes a proposal from 2010-01-12.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote : Posted in a previous version of this proposal
Download full text (3.3 KiB)

This branch fixes bug 172501: reject non-code patch attachements.

As discussed in that bug, non-code patches are not simply rejected. Instead, if a user labels an attachment as being a patch and if the file does not look like being a patch, he is redirected to the attachment edit page, where a message tells him that Launchpad thinks that the file does not seem to be a patch. Also, the patch flag is not yet set. If the user thinks that the file is indeed a patch, he can set the flag again and LP will finally set the flag on a click on the "change" button.

Similary, if a file is uploaded that looks like a patch but that is not marked as a patch, the patch flag is set by Launchpad and the user is redirected to the attachment edit page, where he can unset the flag, if he thinks that it makes sense.

The detection of the file content is unfortunately overly simple. It relies function zope.contenttype.guess_content_type(), which in turn relies mostly on Python's mimetypes.guess_type().

guess_type() simply looks at the filename suffix; while guess_content_type() accepts a parameter "body", it is not used if guess_type() finds a mime type based on the filename.

So, a "mis-named" file is not properly detected, which may lead to some confusion. I considered to call file(1) or to make some other use of the "magic" database, but running "file some-diff-output.diff" returned several results like "ASCII text", "UTF8 text", "C code" but nothing related to diff output.

Implementation:

The methods BugMessage.save_action() and BugAttachmentEditView.change_action() now make the consistency check described above. If they find an inconistency, they set next_url to canonical_url(attachment)+'?need_confirm=1'.

If the bug attachment edit page is called with the GET parameter need_confirm, and additional message is displayed, and a hidden form field named "confirmation_step" is added. The latter is used by BugAttachmentEditView.change_action() to avoid that next_url is set again to the edit view, when a user confirms his setting of the "patch" flag.

The label of "patch" flag is now a bit more verbose (the text was proposed by Karl).

The template lib/lp/bugs/templates/bug-attachment-edit.pt displays some additional explanations, when the page is loaded with the ?need_conform" GET parameter. This text also contians a link to https://help.launchpad.net/Bugs/BugAttchements , which tries to give even more explataions of what a patch is. (I suspect that this page could be improved...)

test: ./bin/test -vv -f lp.bugs stories.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/browser/bugattachment.py
  lib/lp/bugs/browser/bugmessage.py
  lib/lp/bugs/interfaces/bugattachment.py
  lib/lp/bugs/interfaces/bugmessage.py
  lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt
  lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt
  lib/lp/bugs/templates/bug-attachment-edit.pt

== Pylint notices ==

lib/lp/bugs/interfaces/bugattachment.py
    19: [F0401] Unable to import 'lazr.enum' (No module named enum)
...

Read more...

Revision history for this message
Graham Binns (gmb) wrote : Posted in a previous version of this proposal
Download full text (19.4 KiB)

Hi Abel,

This is a nice branch - thanks for working on it.

I've got a few comments and I think that the patch detection logic could
benefit from being factored out into a mixin class, so I'm marking this
as needs-fixing.

> === modified file 'lib/lp/bugs/browser/bugattachment.py'
> --- lib/lp/bugs/browser/bugattachment.py 2009-11-26 10:35:21 +0000
> +++ lib/lp/bugs/browser/bugattachment.py 2010-01-12 10:15:35 +0000
> @@ -84,7 +84,27 @@
> else:
> new_type = BugAttachmentType.UNSPECIFIED
> if new_type != self.context.type:
> - self.context.type = new_type
> + filename = self.context.libraryfile.filename
> + file_content = self.context.libraryfile.read()
> + guessed_type, encoding = guess_content_type(
> + name=filename, body=file_content)
> + # We expect that users set data['patch'] to True only for
> + # real patch data, indicated by guessed_content_type ==
> + # 'text/x-diff'. If there are inconsistencies, we don't
> + # the value automatically, but show the user this form

This should read "we don't set the value automatically."

> + # again with an explanation when the flag data['patch'] should
> + # be used.

And I think this would read better as "with an explanation of when the
flag...".

> + new_type_consistent_with_guessed_type = (
> + (new_type == BugAttachmentType.PATCH) ^
> + (guessed_type != 'text/x-diff'))

I dislike the use of the logical XOR here, but I can't think of a better
way to express this. Please add a comment above this line to explain why
an XOR is needed.

> + is_confirmation_step = self.request.form_ng.getOne(
> + 'confirmation_step') is not None

This is inconsistent with the is_confirmation_step @property below. Is
there a reason why you don't use that here?

> + if new_type_consistent_with_guessed_type or is_confirmation_step:
> + self.context.type = new_type
> + else:
> + self.next_url = (canonical_url(self.context) +
> + '?need_confirm=1')
>
> if data['title'] != self.context.title:
> self.context.title = data['title']
> @@ -122,4 +142,24 @@
> return smartquote('Bug #%d - Edit attachment "%s"') % (
> self.context.bug.id, self.context.title)
>
> + @property
> + def is_confirmation_step(self):
> + """Is the page is displayed to confirm an unexpected "patch" value?"""
> + return self.request.get('need_confirm') is not None

See above re: consistency of is_confirmation_step usage.

> + @property
> + def confirmation_element(self):
> + """An extra hidden input field for the patch flag confirmation step.
> + """
> + if self.is_confirmation_step:
> + return ('<input type="hidden" name="confirmation_step" '
> + 'value="1"/>')
> + else:
> + return ''
> +
> + @property
> + def is_patch(self):
> + """True if this attachment contains a patch, else False."""
> + ...

review: Needs Fixing (code)
Revision history for this message
Abel Deuring (adeuring) wrote : Posted in a previous version of this proposal
Download full text (20.5 KiB)

Hi Graham,

thanks for your review!

On 12.01.2010 12:58, Graham Binns wrote:
> Review: Needs Fixing code
> Hi Abel,
>
> This is a nice branch - thanks for working on it.
>
> I've got a few comments and I think that the patch detection logic could
> benefit from being factored out into a mixin class, so I'm marking this
> as needs-fixing.
>
>> === modified file 'lib/lp/bugs/browser/bugattachment.py'
>> --- lib/lp/bugs/browser/bugattachment.py 2009-11-26 10:35:21 +0000
>> +++ lib/lp/bugs/browser/bugattachment.py 2010-01-12 10:15:35 +0000
>> @@ -84,7 +84,27 @@
>> else:
>> new_type = BugAttachmentType.UNSPECIFIED
>> if new_type != self.context.type:
>> - self.context.type = new_type
>> + filename = self.context.libraryfile.filename
>> + file_content = self.context.libraryfile.read()
>> + guessed_type, encoding = guess_content_type(
>> + name=filename, body=file_content)
>> + # We expect that users set data['patch'] to True only for
>> + # real patch data, indicated by guessed_content_type ==
>> + # 'text/x-diff'. If there are inconsistencies, we don't
>> + # the value automatically, but show the user this form
>
> This should read "we don't set the value automatically."

oops, fixed.

>
>> + # again with an explanation when the flag data['patch'] should
>> + # be used.
>
> And I think this would read better as "with an explanation of when the
> flag...".

fixed.

>
>> + new_type_consistent_with_guessed_type = (
>> + (new_type == BugAttachmentType.PATCH) ^
>> + (guessed_type != 'text/x-diff'))
>
> I dislike the use of the logical XOR here, but I can't think of a better
> way to express this. Please add a comment above this line to explain why
> an XOR is needed.

Changed.

>
>> + is_confirmation_step = self.request.form_ng.getOne(
>> + 'confirmation_step') is not None
>
> This is inconsistent with the is_confirmation_step @property below. Is
> there a reason why you don't use that here?

ouch, a stupid name conflict... Here, we want to figure out, if the form
data which the user submitted comes from the confirmation step, while
the property below is used to render HTML data for the confirmation
form. I renamed the property to "rendering_confirmation_step".

>
>> + if new_type_consistent_with_guessed_type or is_confirmation_step:
>> + self.context.type = new_type
>> + else:
>> + self.next_url = (canonical_url(self.context) +
>> + '?need_confirm=1')
>>
>> if data['title'] != self.context.title:
>> self.context.title = data['title']
>> @@ -122,4 +142,24 @@
>> return smartquote('Bug #%d - Edit attachment "%s"') % (
>> self.context.bug.id, self.context.title)
>>
>> + @property
>> + def is_confirmation_step(self):
>> + """Is the page is displayed to confirm an unexpected "patch" value?"""
>> + return self.request.get('need_confirm') is not None
>
> See above re: consistenc...

Revision history for this message
Graham Binns (gmb) : Posted in a previous version of this proposal
review: Approve (code)
Revision history for this message
Martin Albisetti (beuno) wrote : Posted in a previous version of this proposal

I really like this change you're doing, it will make the experience so much nicer.
I do think this is an interaction we want to be very careful about, since it's easy for people to misunderstand what happened.

The first thing would be to reduce the amount of text on the page by, say, a million times (or as close to that as possible). The more text on there, the less people will read at all.
My proposal:
- Show the file name of the uploaded file with maybe a green check, to feed back that the upload was successful
- I think this is better phrased as a question rather than an edit form:

This looks like a patch (<small>what is a patch</small>)
<b>Is this a patch?</b>
[_yes_] [no]

------------------

This doesn't look like a patch (<small>what is a patch</small>)
<b>Is this a patch?</b>
[yes] [_no_]

review: Needs Fixing (ui)
Revision history for this message
Abel Deuring (adeuring) wrote :
Download full text (4.0 KiB)

I'm re-submitting this branch for review, because Martin
suggested in his UI review to use simplified view where
the user must confirm that a bug attachment is indeed a patch,
if Launchpad believes it is not.

When a user sets the "patch" flag to an value considered to be
inconsistent by Launchpad, a click to the "change bugattachment"
and "create new bug comment" buttons leads him to a new view
"+confirm-is-patch", where he must explicitly set his choice
of the "patch" flag again.

As a drive-by, I also fixed bug 182286 ("Content Type:" on bug
attachment page is editable even when it has no effect). I simply
removed the code that unconditionally sets teh content type
of patches to "text/plain". As noted by Kjell Braden in a comment
on bug 182286, this is sometimes simply wrong. The alternative would
have been to carry over the overriding of the content type setting
for patches to the new view class BugAttachmentPatchConfirmationView.
That would have been a bit more effort for a questionaable feature
than to fix bug 182286 which simply meant that I had to remove a few
lines of code and tests.

The implementation is quite straightforward:

- a new template and a new view class for the page "+confirm-is-patch"
- a mixin class BugAttachmentContentCheck with some methods to check
  if the content type as guessed by Launchpad matches the patch flag
  selection made by the user. This mixin class is used in
  BugAttachmentEditView and in BugMessageAddFormView.

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

Demo/QA:

  - Create a new bug attachment with a filename like "123.diff"
    but do not check the "patch" flag
  - Ensure that you are led to the "+confirm-is-patch" view.
  - Ensure that the "patch" flag is currently set for this attachment:
    On the main bug page, it should appear in the "patches" portlet.
  - choose "no" in the confirmation view and click "change"
  - The attachment should now appear in the "attachments" portlet of
    the main bug page.

  - Create a new bug attachment with a filename like "456.png" and
    check the "patch" flag.
  - Ensure that you are led to the "+confirm-is-patch" view.
  - Ensure that the "patch" flag is currently _not_ set for this
    attachment: On the main bug page, it should appear in the
    "attachments" portlet.
  - choose "yes" in the confirmation view and click "change"
  - The attachment should now appear in the "patches" portlet of
    the main bug page.

  - Change the "patch" flag of the two new attachments in their "edit"
    view. If the new value looks wrong to LP, you are again led to the
    "+confirm-is-patch" view.

screenshots:

http://people.canonical.com/~adeuring/confirm-patch-1.png
  Add diff file without setting the "patch" flag
http://people.canonical.com/~adeuring/confirm-patch-2.png
  confirmation view for the new attachment
http://people.canonical.com/~adeuring/confirm-patch-3.png
  Add a PNG file and check the "patch" flag
http://people.canonical.com/~adeuring/confirm-patch-4.png
  confirmation view for the new attachment/patch
http://people.canonical.com/~adeuring/confirm-patch-5.png
  confirmation view shown when the "patch" flag is set to a value which
  ...

Read more...

Revision history for this message
Martin Albisetti (beuno) wrote :

This is much much better, thanks for re-working this Abel.

The only suggestion I'd leave is that "Is this a patch" should end with a question mark instead of a semi-colon. It may be a restriction of Launchpad forms, if it is, "Is this a patch?:" maybe be better.

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

On 28.01.2010 15:26, Martin Albisetti wrote:
> Review: Approve ui
> This is much much better, thanks for re-working this Abel.
>
> The only suggestion I'd leave is that "Is this a patch" should end with a question mark instead of a semi-colon. It may be a restriction of Launchpad forms, if it is, "Is this a patch?:" maybe be better.

yes, I think too that a question mark would be better. The ':' is
generated by LPFormView -- but "Is this a patch ?:" would look quite
ugly, I think, so I'd prefer to stay with the simple ':'.

Revision history for this message
Martin Albisetti (beuno) wrote :

I agree it's a bit ugly, but the question mark is a key symbol there :)
I'm not going to block on it, I'll leave it up to you.

Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugattachment.py'
2--- lib/lp/bugs/browser/bugattachment.py 2009-11-26 10:35:21 +0000
3+++ lib/lp/bugs/browser/bugattachment.py 2010-02-02 14:56:19 +0000
4@@ -1,3 +1,4 @@
5+
6 # Copyright 2009 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9@@ -5,6 +6,7 @@
10
11 __metaclass__ = type
12 __all__ = [
13+ 'BugAttachmentContentCheck',
14 'BugAttachmentSetNavigation',
15 'BugAttachmentEditView',
16 'BugAttachmentURL',
17@@ -14,12 +16,15 @@
18
19 from zope.interface import implements
20 from zope.component import getUtility
21+from zope.contenttype import guess_content_type
22
23-from canonical.launchpad.webapp import canonical_url, GetitemNavigation
24+from canonical.launchpad.webapp import (
25+ canonical_url, custom_widget, GetitemNavigation)
26 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
27 from canonical.launchpad.webapp.interfaces import ILaunchBag
28 from lp.bugs.interfaces.bugattachment import (
29- BugAttachmentType, IBugAttachmentEditForm, IBugAttachmentSet)
30+ BugAttachmentType, IBugAttachmentEditForm,
31+ IBugAttachmentIsPatchConfirmationForm, IBugAttachmentSet)
32 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
33 from canonical.launchpad.webapp.launchpadform import (
34 action, LaunchpadFormView)
35@@ -27,6 +32,33 @@
36
37 from canonical.lazr.utils import smartquote
38
39+from canonical.widgets.itemswidgets import LaunchpadBooleanRadioWidget
40+
41+
42+class BugAttachmentContentCheck:
43+ """A mixin class that checks the consistency of patch flag and file type.
44+ """
45+
46+ def guessContentType(self, filename, file_content):
47+ """Guess the content type a file with the given anme and content."""
48+ guessed_type, encoding = guess_content_type(
49+ name=filename, body=file_content)
50+ return guessed_type
51+
52+ def attachmentTypeConsistentWithContentType(
53+ self, patch_flag_set, filename, file_content):
54+ """Return True iff patch_flag is consistent with filename and content.
55+ """
56+ guessed_type = self.guessContentType(filename, file_content)
57+ # An XOR of "is the patch flag selected?" with "is the
58+ # guessed type not a diff?" tells us if the type selected
59+ # by the user matches the guessed type.
60+ return (patch_flag_set ^ (guessed_type != 'text/x-diff'))
61+
62+ def nextUrlForInconsistentPatchFlags(self, attachment):
63+ """The next_url value used for an inconistent patch flag."""
64+ return canonical_url(attachment) + '/+confirm-is-patch'
65+
66
67 class BugAttachmentSetNavigation(GetitemNavigation):
68
69@@ -57,12 +89,11 @@
70 return u"+attachment/%d" % self.context.id
71
72
73-class BugAttachmentEditView(LaunchpadFormView):
74+class BugAttachmentEditView(LaunchpadFormView, BugAttachmentContentCheck):
75 """Edit a bug attachment."""
76
77 schema = IBugAttachmentEditForm
78 field_names = ['title', 'patch', 'contenttype']
79- label = "Change bug attachment information"
80
81 def __init__(self, context, request):
82 LaunchpadFormView.__init__(self, context, request)
83@@ -84,15 +115,27 @@
84 else:
85 new_type = BugAttachmentType.UNSPECIFIED
86 if new_type != self.context.type:
87- self.context.type = new_type
88+ filename = self.context.libraryfile.filename
89+ file_content = self.context.libraryfile.read()
90+ # We expect that users set data['patch'] to True only for
91+ # real patch data, indicated by guessed_content_type ==
92+ # 'text/x-diff'. If there are inconsistencies, we don't
93+ # set the value automatically. Instead, we lead the user to
94+ # another form where we ask him if he is sure about his
95+ # choice of the patch flag.
96+ new_type_consistent_with_guessed_type = (
97+ self.attachmentTypeConsistentWithContentType(
98+ new_type == BugAttachmentType.PATCH, filename,
99+ file_content))
100+ if new_type_consistent_with_guessed_type:
101+ self.context.type = new_type
102+ else:
103+ self.next_url = self.nextUrlForInconsistentPatchFlags(
104+ self.context)
105
106 if data['title'] != self.context.title:
107 self.context.title = data['title']
108
109- # If it's a patch, 'text/plain' is always used.
110- if (self.context.type == BugAttachmentType.PATCH
111- and data['contenttype'] != 'text/plain'):
112- data['contenttype'] = 'text/plain'
113 if self.context.libraryfile.mimetype != data['contenttype']:
114 self.updateContentType(data['contenttype'])
115
116@@ -123,3 +166,49 @@
117 self.context.bug.id, self.context.title)
118
119 page_title = label
120+
121+
122+class BugAttachmentPatchConfirmationView(LaunchpadFormView):
123+ """Confirmation of the "patch" flag setting.
124+
125+ If the user sets the "patch" flag to a value that is inconsistent
126+ with the result of a call of guess_content_type() for this
127+ attachment, we show this view to ask the user if he is sure
128+ about his selection.
129+ """
130+
131+ schema = IBugAttachmentIsPatchConfirmationForm
132+
133+ custom_widget('patch', LaunchpadBooleanRadioWidget)
134+
135+ def __init__(self, context, request):
136+ LaunchpadFormView.__init__(self, context, request)
137+ self.next_url = self.cancel_url = (
138+ canonical_url(ICanonicalUrlData(context).inside))
139+
140+ def initialize(self):
141+ super(BugAttachmentPatchConfirmationView, self).initialize()
142+ self.widgets['patch'].setRenderedValue(self.is_patch)
143+
144+ @property
145+ def label(self):
146+ return smartquote('Bug #%d - Confirm attachment type of "%s"') % (
147+ self.context.bug.id, self.context.title)
148+
149+ page_title = label
150+
151+ @action('Change', name='change')
152+ def change_action(self, action, data):
153+ current_patch_setting = self.context.type == BugAttachmentType.PATCH
154+ if data['patch'] != current_patch_setting:
155+ if data['patch']:
156+ self.context.type = BugAttachmentType.PATCH
157+ #xxxxxxxxxx adjust content type!
158+ # xxx use mixin, together with BugAttachmnetEditView
159+ else:
160+ self.context.type = BugAttachmentType.UNSPECIFIED
161+
162+ @property
163+ def is_patch(self):
164+ """True if this attachment contains a patch, else False."""
165+ return self.context.type == BugAttachmentType.PATCH
166
167=== modified file 'lib/lp/bugs/browser/bugmessage.py'
168--- lib/lp/bugs/browser/bugmessage.py 2009-08-28 14:31:15 +0000
169+++ lib/lp/bugs/browser/bugmessage.py 2010-02-02 14:56:19 +0000
170@@ -12,13 +12,14 @@
171
172 from zope.component import getUtility
173
174+from lp.bugs.browser.bugattachment import BugAttachmentContentCheck
175 from lp.bugs.interfaces.bugmessage import IBugMessageAddForm
176 from lp.bugs.interfaces.bugwatch import IBugWatchSet
177 from canonical.launchpad.webapp import action, canonical_url
178 from canonical.launchpad.webapp import LaunchpadFormView
179
180
181-class BugMessageAddFormView(LaunchpadFormView):
182+class BugMessageAddFormView(LaunchpadFormView, BugAttachmentContentCheck):
183 """Browser view class for adding a bug comment/attachment."""
184
185 schema = IBugMessageAddForm
186@@ -88,6 +89,7 @@
187 self.request.response.addNotification(
188 "Thank you for your comment.")
189
190+ self.next_url = canonical_url(self.context)
191 if file_:
192
193 # Slashes in filenames cause problems, convert them to dashes
194@@ -102,15 +104,32 @@
195 file_description = filename
196
197 # Process the attachment.
198- bug.addAttachment(
199+ # If the patch flag is not consistent with the result of
200+ # the guess made in attachmentTypeConsistentWithContentType(),
201+ # we use the guessed type and lead the user to a page
202+ # where he can override the flag value, if Luanchpad's
203+ # guess is wrong.
204+ patch_flag_consistent = (
205+ self.attachmentTypeConsistentWithContentType(
206+ data['patch'], filename, data['filecontent']))
207+ if not patch_flag_consistent:
208+ guessed_type = self.guessContentType(
209+ filename, data['filecontent'])
210+ is_patch = guessed_type == 'text/x-diff'
211+ else:
212+ is_patch = data['patch']
213+ attachment = bug.addAttachment(
214 owner=self.user, data=StringIO(data['filecontent']),
215 filename=filename, description=file_description,
216- comment=message, is_patch=data['patch'])
217+ comment=message, is_patch=is_patch)
218+
219+ if not patch_flag_consistent:
220+ self.next_url = self.nextUrlForInconsistentPatchFlags(
221+ attachment)
222
223 self.request.response.addNotification(
224 "Attachment %s added to bug." % filename)
225
226- self.next_url = canonical_url(self.context)
227
228 def shouldShowEmailMeWidget(self):
229 """Should the subscribe checkbox be shown?"""
230
231=== modified file 'lib/lp/bugs/browser/configure.zcml'
232--- lib/lp/bugs/browser/configure.zcml 2010-01-18 21:44:59 +0000
233+++ lib/lp/bugs/browser/configure.zcml 2010-02-02 14:56:19 +0000
234@@ -718,6 +718,13 @@
235 name="+edit"
236 permission="launchpad.AnyPerson"
237 template="../templates/bug-attachment-edit.pt"/>
238+ <browser:page
239+ for="lp.bugs.interfaces.bugattachment.IBugAttachment"
240+ class="lp.bugs.browser.bugattachment.BugAttachmentPatchConfirmationView"
241+ facet="bugs"
242+ name="+confirm-is-patch"
243+ permission="launchpad.AnyPerson"
244+ template="../templates/bug-attachment-confirm-is-patch.pt"/>
245 <browser:navigation
246 module="lp.bugs.browser.bugattachment"
247 classes="
248
249=== modified file 'lib/lp/bugs/interfaces/bugattachment.py'
250--- lib/lp/bugs/interfaces/bugattachment.py 2010-01-20 17:09:40 +0000
251+++ lib/lp/bugs/interfaces/bugattachment.py 2010-02-02 14:56:19 +0000
252@@ -12,6 +12,7 @@
253 'IBugAttachment',
254 'IBugAttachmentSet',
255 'IBugAttachmentEditForm',
256+ 'IBugAttachmentIsPatchConfirmationForm',
257 ]
258
259 from zope.interface import Interface
260@@ -136,5 +137,11 @@
261 "text/plain"),
262 required=True)
263 patch = Bool(
264- title=u"This attachment is a patch",
265+ title=u"This attachment contains a solution (patch) for this bug",
266 required=True, default=False)
267+
268+
269+class IBugAttachmentIsPatchConfirmationForm(Interface):
270+ """Schema used to confirm the setting of the "patch" flag."""
271+
272+ patch = Bool(title=u"Is this file a patch", required=True, default=False)
273
274=== modified file 'lib/lp/bugs/interfaces/bugmessage.py'
275--- lib/lp/bugs/interfaces/bugmessage.py 2009-12-11 10:03:46 +0000
276+++ lib/lp/bugs/interfaces/bugmessage.py 2010-02-02 14:56:19 +0000
277@@ -81,8 +81,9 @@
278 filecontent = Bytes(
279 title=u"Attachment", required=False,
280 constraint=attachment_size_constraint)
281- patch = Bool(title=u"This attachment is a patch", required=False,
282- default=False)
283+ patch = Bool(
284+ title=u"This attachment contains a solution (patch) for this bug",
285+ required=False, default=False)
286 attachment_description = Title(title=u'Description', required=False)
287 email_me = Bool(
288 title=u"E-mail me about changes to this bug report",
289
290=== modified file 'lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt'
291--- lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt 2009-09-22 16:35:00 +0000
292+++ lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt 2010-02-02 14:56:19 +0000
293@@ -70,3 +70,137 @@
294 ... print li_tag.a.renderContents()
295 Some information
296 bar.txt
297+
298+We can also declare an attachment to be a patch.
299+
300+ >>> user_browser.open(
301+ ... 'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
302+
303+Leading and trailing whitespace are stripped from the description of the
304+attachment.
305+
306+ >>> foo_file.seek(0)
307+ >>> user_browser.getControl('Attachment').add_file(
308+ ... foo_file, 'text/plain', 'foo.diff')
309+ >>> user_browser.getControl('Description').value = 'A fix for this bug.'
310+ >>> patch_control = user_browser.getControl(
311+ ... 'This attachment contains a solution (patch) for this bug')
312+ >>> patch_control.selected = True
313+ >>> user_browser.getControl(
314+ ... name="field.comment").value = 'Added some information'
315+ >>> user_browser.getControl('Post Comment').click()
316+ >>> user_browser.url
317+ 'http://bugs.launchpad.dev/firefox/+bug/1'
318+
319+If we add an attachment that looks like a patch but if we don't set
320+the flag "this attachment is a patch"...
321+
322+ >>> user_browser.open(
323+ ... 'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
324+ >>> foo_file.seek(0)
325+ >>> user_browser.getControl('Attachment').add_file(
326+ ... foo_file, 'text/plain', 'foo2.diff')
327+ >>> user_browser.getControl('Description').value = 'More data'
328+ >>> patch_control = user_browser.getControl(
329+ ... 'This attachment contains a solution (patch) for this bug')
330+ >>> patch_control.selected = False
331+ >>> user_browser.getControl(
332+ ... name="field.comment").value = 'Added even more information'
333+ >>> user_browser.getControl('Post Comment').click()
334+
335+...we are redirected to a page...
336+
337+ >>> user_browser.url
338+ 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/4/+confirm-is-patch'
339+
340+...where we see a message that we should double-check if this file
341+is indeed not a patch.
342+
343+ >>> print extract_text(find_tags_by_class(
344+ ... user_browser.contents, 'documentDescription')[0])
345+ This file looks like a patch.
346+ What is a patch?
347+
348+Also, we have "yes"/"no" radio buttons to answer the question "Is this a
349+patch?". The currently selected radio button is "yes".
350+
351+ >>> patch_control_yes = user_browser.getControl('yes')
352+ >>> patch_control_yes.selected
353+ True
354+ >>> patch_control_no = user_browser.getControl('no')
355+ >>> patch_control_no.selected
356+ False
357+
358+We want indeed to declare the file as not being a patch, so we unselect
359+the "patch" checkbox again and submit the form.
360+
361+ >>> patch_control_no.selected = True
362+ >>> user_browser.getControl('Change').click()
363+
364+Now we are redirected to the main bug page, and the new file is
365+listed as an ordinary attachment.
366+
367+ >>> user_browser.url
368+ 'http://bugs.launchpad.dev/firefox/+bug/1'
369+ >>> attachments = find_portlet(user_browser.contents, 'Bug attachments')
370+ >>> for li_tag in attachments.findAll('li', 'download-attachment'):
371+ ... print li_tag.a.renderContents()
372+ Some information
373+ bar.txt
374+ More data
375+
376+Similary, if we add an attachment that does not look like a patch and
377+if we set the "patch" flag for this attachment...
378+
379+ >>> user_browser.open(
380+ ... 'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
381+ >>> foo_file.seek(0)
382+ >>> user_browser.getControl('Attachment').add_file(
383+ ... foo_file, 'text/plain', 'foo.png')
384+ >>> user_browser.getControl('Description').value = 'A better icon for foo'
385+ >>> patch_control = user_browser.getControl(
386+ ... 'This attachment contains a solution (patch) for this bug')
387+ >>> patch_control.selected = True
388+ >>> user_browser.getControl('Post Comment').click()
389+
390+...we are redirected to the page where we must confirm that this attachment
391+is indeed a patch.
392+
393+ >>> user_browser.url
394+ 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/5/+confirm-is-patch'
395+
396+...where we see a message asking us if we really ant to declare this file
397+as a patch.
398+
399+ >>> print extract_text(find_tags_by_class(
400+ ... user_browser.contents, 'documentDescription')[0])
401+ This file does not look like a patch.
402+ What is a patch?
403+
404+Also, the "patch" flag is not yet set.
405+
406+ >>> patch_control_yes = user_browser.getControl('yes')
407+ >>> patch_control_yes.selected
408+ False
409+ >>> patch_control_no = user_browser.getControl('no')
410+ >>> patch_control_no.selected
411+ True
412+
413+Let's pretend that the file contains an improved icon, so we set
414+the "patch" flag again and save the changes.
415+
416+ >>> patch_control_yes.selected = True
417+ >>> user_browser.getControl('Change').click()
418+
419+Now we are redirected to the main bug page...
420+
421+ >>> user_browser.url
422+ 'http://bugs.launchpad.dev/firefox/+bug/1'
423+
424+...and the new attachment is listed as a patch.
425+
426+ >>> patches = find_portlet(user_browser.contents, 'Patches')
427+ >>> for li_tag in patches.findAll('li', 'download-attachment'):
428+ ... print li_tag.a.renderContents()
429+ A fix for this bug.
430+ A better icon for foo
431
432=== modified file 'lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt'
433--- lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2009-12-10 15:12:22 +0000
434+++ lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt 2010-02-02 14:56:19 +0000
435@@ -32,18 +32,50 @@
436 We can edit the attachment to be a patch.
437
438 >>> user_browser.getLink(url='+attachment/1').click()
439- >>> user_browser.getControl('This attachment is a patch').selected = True
440- >>> user_browser.getControl('Change').click()
441+ >>> patch_control = user_browser.getControl(
442+ ... 'This attachment contains a solution (patch) for this bug')
443+ >>> patch_control.selected = True
444+ >>> user_browser.getControl('Change').click()
445+
446+The server now checks if the attachment looks like a patch. In this case,
447+is doesn't, and we get the edit page again, with a message asking us if
448+the attachment should indeed be labeled as a patch
449+
450+ >>> user_browser.url
451+ 'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1/+confirm-is-patch'
452+
453+ >>> print extract_text(find_tags_by_class(
454+ ... user_browser.contents, 'documentDescription')[0])
455+ This file does not look like a patch.
456+ What is a patch?
457+
458+We are sure that this file is indeed a patch, so let's activate the
459+currenty inactive "yes" radion button of the question "Is this a patch?"
460+again and submit the form.
461+
462+ >>> patch_control_yes = user_browser.getControl('yes')
463+ >>> patch_control_yes.selected
464+ False
465+ >>> patch_control_no = user_browser.getControl('no')
466+ >>> patch_control_no.selected
467+ True
468+ >>> patch_control_yes.selected = True
469+ >>> user_browser.getControl('Change').click()
470+
471+Now we are redirected to the main bug page...
472
473 >>> user_browser.url
474 'http://bugs.launchpad.dev/firefox/+bug/1'
475
476-The attachment that became a patch is now shown in the portlet "Patches"...
477+...the attachment that became a patch is now shown in the portlet
478+"Patches"...
479
480 >>> patches = find_portlet(user_browser.contents, 'Patches')
481 >>> for li_tag in patches.findAll('li', 'download-attachment'):
482 ... print li_tag.a.renderContents()
483 Another title
484+ A fix for this bug.
485+ A better icon for foo
486
487 ...while it is gone from the portlet "Bug attachments".
488
489@@ -51,19 +83,15 @@
490 >>> for li_tag in attachments.findAll('li', 'download-attachment'):
491 ... print li_tag.a.renderContents()
492 bar.txt
493-
494-The content type of a patch is automatically changed to text/plain.
495-
496- >>> user_browser.getLink(url='+attachment/1').click()
497- >>> user_browser.getControl('Content Type').value
498- 'text/plain'
499-
500-Clicking the link "Add patch" in the patches portlet open the form
501+ More data
502+
503+Clicking the link "Add patch" in the patches portlet opens the form
504 to add bug comments with the checkbox "This attachment is a patch"
505 enabled.
506
507 >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
508 >>> user_browser.getLink('Add patch').click()
509- >>> patch_checkbox = user_browser.getControl('This attachment is a patch')
510+ >>> patch_checkbox = user_browser.getControl(
511+ ... 'This attachment contains a solution (patch) for this bug')
512 >>> patch_checkbox.selected
513 True
514
515=== added file 'lib/lp/bugs/templates/bug-attachment-confirm-is-patch.pt'
516--- lib/lp/bugs/templates/bug-attachment-confirm-is-patch.pt 1970-01-01 00:00:00 +0000
517+++ lib/lp/bugs/templates/bug-attachment-confirm-is-patch.pt 2010-02-02 14:56:19 +0000
518@@ -0,0 +1,40 @@
519+<html
520+ xmlns="http://www.w3.org/1999/xhtml"
521+ xmlns:tal="http://xml.zope.org/namespaces/tal"
522+ xmlns:metal="http://xml.zope.org/namespaces/metal"
523+ xmlns:i18n="http://xml.zope.org/namespaces/i18n"
524+ metal:use-macro="view/macro:page/main_only"
525+ i18n:domain="malone"
526+>
527+<body>
528+ <div metal:fill-slot="main">
529+
530+ <div metal:use-macro="context/@@launchpad_form/form">
531+
532+ <div metal:fill-slot="extra_info" class="documentDescription">
533+ <tal:is_patch condition="view/is_patch">
534+ <p>
535+ <strong>This file looks like a patch.</strong>
536+ <small>
537+ <a href="https://help.launchpad.net/Bugs/BugAttchements">
538+ What is a patch?
539+ </a>
540+ </small>
541+ </p>
542+ </tal:is_patch>
543+ <tal:is_not_patch condition="not: view/is_patch">
544+ <p>
545+ <strong>This file does not look like a patch.
546+ </strong>
547+ <small>
548+ <a href="https://help.launchpad.net/Bugs/BugAttchements">
549+ What is a patch?
550+ </a>
551+ </small>
552+ </p>
553+ </tal:is_not_patch>
554+ </div>
555+ </div>
556+ </div>
557+</body>
558+</html>
559
560=== modified file 'lib/lp/bugs/templates/bug-attachment-edit.pt'
561--- lib/lp/bugs/templates/bug-attachment-edit.pt 2009-08-31 11:55:51 +0000
562+++ lib/lp/bugs/templates/bug-attachment-edit.pt 2010-02-02 14:56:19 +0000
563@@ -15,6 +15,11 @@
564 An attachment can provide valuable information about the bug. It
565 can also provide a direct solution to a bug - for example a patch
566 that fixes the bug.
567+ <small>
568+ <a href="https://help.launchpad.net/Bugs/BugAttchements">
569+ What is a patch?
570+ </a>
571+ </small>
572 </div>
573 </div>
574 </div>

Subscribers

People subscribed via source and target branches

to status/vote changes: