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."""
> +        return self.context.type == BugAttachmentType.PATCH
> +
>      page_title = label
> 
> === modified file 'lib/lp/bugs/browser/bugmessage.py'
> --- lib/lp/bugs/browser/bugmessage.py	2009-08-28 14:31:15 +0000
> +++ lib/lp/bugs/browser/bugmessage.py	2010-01-12 10:15:35 +0000
> @@ -11,6 +11,7 @@
>  from StringIO import StringIO
>  
>  from zope.component import getUtility
> +from zope.contenttype import guess_content_type
>  
>  from lp.bugs.interfaces.bugmessage import IBugMessageAddForm
>  from lp.bugs.interfaces.bugwatch import IBugWatchSet
> @@ -88,6 +89,7 @@
>                  self.request.response.addNotification(
>                      "Thank you for your comment.")
>  
> +        self.next_url = canonical_url(self.context)
>          if file_:
>  
>              # Slashes in filenames cause problems, convert them to dashes
> @@ -102,15 +104,29 @@
>                  file_description = filename
>  
>              # Process the attachment.
> -            bug.addAttachment(
> +            # If the patch flag indicates a confict with the content,
> +            # override the flag and redirect the user to the bug
> +            # attachment edit page, where he can confirm his setting,
> +            # if he really wants to set the to the desired value.
> +            content_type, encoding = guess_content_type(
> +                name=filename, body=data['filecontent'])
> +            patch_flag_consistent = (
> +                (content_type != 'text/x-diff') ^ data['patch'])
> +            if not patch_flag_consistent:
> +                is_patch = content_type == 'text/x-diff'
> +            else:
> +                is_patch = data['patch']
> +            attachment = bug.addAttachment(
>                  owner=self.user, data=StringIO(data['filecontent']),
>                  filename=filename, description=file_description,
> -                comment=message, is_patch=data['patch'])
> +                comment=message, is_patch=is_patch)
> +
> +            if not patch_flag_consistent:
> +                self.next_url = canonical_url(attachment) + '?need_confirm=1'
>  

You've got a lot of similarities here with the code in
browser.bugattachment. Would it be reasonable for you to refactor the
commonalities into a Mixin class (e.g. BugPatchProcessinMixin) so that
you don't have this duplication of effort?

>              self.request.response.addNotification(
>                  "Attachment %s added to bug." % filename)
>  
> -        self.next_url = canonical_url(self.context)
>  
>      def shouldShowEmailMeWidget(self):
>          """Should the subscribe checkbox be shown?"""
> 
> === modified file 'lib/lp/bugs/interfaces/bugattachment.py'
> --- lib/lp/bugs/interfaces/bugattachment.py	2009-11-26 10:35:21 +0000
> +++ lib/lp/bugs/interfaces/bugattachment.py	2010-01-12 10:15:35 +0000
> @@ -133,5 +133,5 @@
>              "text/plain"),
>          required=True)
>      patch = Bool(
> -        title=u"This attachment is a patch",
> +        title=u"This attachment contains a solution (patch) for this bug",

Nice!

>          required=True, default=False)
> 
> === modified file 'lib/lp/bugs/interfaces/bugmessage.py'
> --- lib/lp/bugs/interfaces/bugmessage.py	2009-12-11 10:03:46 +0000
> +++ lib/lp/bugs/interfaces/bugmessage.py	2010-01-12 10:15:35 +0000
> @@ -81,8 +81,9 @@
>      filecontent = Bytes(
>          title=u"Attachment", required=False,
>          constraint=attachment_size_constraint)
> -    patch = Bool(title=u"This attachment is a patch", required=False,
> -        default=False)
> +    patch = Bool(
> +        title=u"This attachment contains a solution (patch) for this bug",
> +        required=False, default=False)
>      attachment_description = Title(title=u'Description', required=False)
>      email_me = Bool(
>          title=u"E-mail me about changes to this bug report",
> 
> === modified file 'lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt'
> --- lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt	2009-09-22 16:35:00 +0000
> +++ lib/lp/bugs/stories/bugattachments/10-add-bug-attachment.txt	2010-01-12 10:15:35 +0000
> @@ -70,3 +70,148 @@
>    ...   print li_tag.a.renderContents()
>    Some information
>    bar.txt
> +
> +We can also declare an attachment to be a patch.
> +
> +  >>> user_browser.open(
> +  ...     'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
> +
> +Leading and trailing whitespace are stripped from the description of the
> +attachment.
> +
> +  >>> foo_file.seek(0)
> +  >>> user_browser.getControl('Attachment').add_file(
> +  ...   foo_file, 'text/plain', 'foo.diff')
> +  >>> user_browser.getControl('Description').value = 'A fix for this bug.'
> +  >>> patch_control = user_browser.getControl(
> +  ...     'This attachment contains a solution (patch) for this bug')
> +  >>> patch_control.selected = True
> +  >>> user_browser.getControl(
> +  ...     name="field.comment").value = 'Added some information'
> +  >>> user_browser.getControl('Post Comment').click()
> +  >>> user_browser.url
> +  'http://bugs.launchpad.dev/firefox/+bug/1'
> +
> +If we add an attachment that looks like a patch but if we don't set
> +the flag "this attachment is a patch"...
> +
> +  >>> user_browser.open(
> +  ...     'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
> +  >>> foo_file.seek(0)
> +  >>> user_browser.getControl('Attachment').add_file(
> +  ...   foo_file, 'text/plain', 'foo2.diff')
> +  >>> user_browser.getControl('Description').value = 'More data'
> +  >>> patch_control = user_browser.getControl(
> +  ...     'This attachment contains a solution (patch) for this bug')
> +  >>> patch_control.selected = False
> +  >>> user_browser.getControl(
> +  ...     name="field.comment").value = 'Added even more information'
> +  >>> user_browser.getControl('Post Comment').click()
> +
> +...we are redirected to the "edit attachment" page...
> +
> +  >>> user_browser.url
> +  'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/4?need_confirm=1'
> +
> +...where we see a message that we should double-check if this file
> +is indeed not a patch.
> +
> +  >>> print extract_text(find_tags_by_class(
> +  ...     user_browser.contents, 'documentDescription')[0])
> +  An attachment can provide valuable information about the bug. It
> +  can also provide a direct solution to a bug - for example a patch
> +  that fixes the bug.
> +  This file looks like a patch, but you unselected the
> +  checkbox "This attachment contains a solution (patch) for this
> +  bug". Please unselect it again and click on "change",
> +  if you are sure that this file does not contain a patch.
> +  Patches normally describe changes to the source code which fix
> +  a bug. See also Launchpad help on Bug Attachments.
> +
> +Also, the flag "patch" is currently set.
> +
> +  >>> patch_control = user_browser.getControl(
> +  ...     'This attachment contains a solution (patch) for this bug')
> +  >>> patch_control.selected
> +  True
> +
> +We want indeed to declare the file as not being a patch, so we unselect
> +the "patch" checkbox again and submit the form.
> +
> +  >>> patch_control.selected = False
> +  >>> user_browser.getControl('Change').click()
> +
> +Now we are redirected to the main bug page, and the new file is
> +listed as an ordinary attachment.
> +
> +  >>> user_browser.url
> +  'http://bugs.launchpad.dev/firefox/+bug/1'
> +  >>> attachments = find_portlet(user_browser.contents, 'Bug attachments')
> +  >>> for li_tag in attachments.findAll('li', 'download-attachment'):
> +  ...   print li_tag.a.renderContents()
> +  Some information
> +  bar.txt
> +  More data
> +
> +Similary, if we add an attachment that does not look like a patch ans
> +if we set the "patch" flag for this attachment...
> +
> +  >>> user_browser.open(
> +  ...     'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment')
> +  >>> foo_file.seek(0)
> +  >>> user_browser.getControl('Attachment').add_file(
> +  ...   foo_file, 'text/plain', 'foo.png')
> +  >>> user_browser.getControl('Description').value = 'A better icon for foo'
> +  >>> patch_control = user_browser.getControl(
> +  ...     'This attachment contains a solution (patch) for this bug')
> +  >>> patch_control.selected = True
> +  >>> user_browser.getControl('Post Comment').click()
> +
> +...we are redirected to the bug attachment edit page...
> +
> +  >>> user_browser.url
> +  'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/5?need_confirm=1'
> +
> +...where we see a message asking us if we really ant to declare this file
> +as a patch.
> +
> +  >>> print extract_text(find_tags_by_class(
> +  ...     user_browser.contents, 'documentDescription')[0])
> +  An attachment can provide valuable information about the bug. It
> +  can also provide a direct solution to a bug - for example a patch
> +  that fixes the bug.
> +  This file does not look like a patch, but you selected the checkbox
> +  "This attachment contains a solution (patch) for this bug". Please
> +  select it again and click on "change", if you are sure that this
> +  file does indeed contain a patch.
> +  Please leave the field unchecked, if the attachment contains
> +  data that describes a bug, for example a screenshot or test
> +  data to reproduce a bug.
> +  Patches normally describe changes to the source code which fix
> +  a bug. See also Launchpad help on Bug Attachments.
> +
> +Also, the "patch" flag is not yet set.
> +
> +  >>> patch_control = user_browser.getControl(
> +  ...     'This attachment contains a solution (patch) for this bug')
> +  >>> patch_control.selected
> +  False
> +
> +Let's pretend that the file contains an improved icon, so we set
> +the "patch" flag again and save the changes.
> +
> +  >>> patch_control.selected = True
> +  >>> user_browser.getControl('Change').click()
> +
> +Now we are redirected to the main bug page..
> +
> +  >>> user_browser.url
> +  'http://bugs.launchpad.dev/firefox/+bug/1'
> +
> +...and the new attachment is listed as a patch.
> +
> +  >>> patches = find_portlet(user_browser.contents, 'Patches')
> +  >>> for li_tag in patches.findAll('li', 'download-attachment'):
> +  ...   print li_tag.a.renderContents()
> +  A fix for this bug.
> +  A better icon for foo
> 
> === modified file 'lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt'
> --- lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt	2009-12-10 15:12:22 +0000
> +++ lib/lp/bugs/stories/bugattachments/20-edit-bug-attachment.txt	2010-01-12 10:15:35 +0000
> @@ -32,18 +32,57 @@
>  We can edit the attachment to be a patch.
>  
>      >>> user_browser.getLink(url='+attachment/1').click()
> -    >>> user_browser.getControl('This attachment is a patch').selected = True
> -    >>> user_browser.getControl('Change').click()
> +    >>> patch_control = user_browser.getControl(
> +    ...     'This attachment contains a solution (patch) for this bug')
> +    >>> patch_control.selected = True
> +    >>> user_browser.getControl('Change').click()
> +
> +The server now checks if the attachment looks like a patch. In this case,
> +is doesn't, and we get the edit page again, with a message asking us if
> +the attachment should indeed be labeled as a patch
> +
> +    >>> user_browser.url
> +    'http://bugs.launchpad.dev/firefox/+bug/1/+attachment/1?need_confirm=1'
> +
> +    >>> print extract_text(find_tags_by_class(
> +    ...     user_browser.contents, 'documentDescription')[0])
> +    An attachment can provide valuable information about the bug. It
> +    can also provide a direct solution to a bug - for example a patch
> +    that fixes the bug.
> +    This file does not look like a patch, but you selected the checkbox
> +    "This attachment contains a solution (patch) for this bug". Please
> +    select it again and click on "change", if you are sure that this
> +    file does indeed contain a patch.
> +    Please leave the field unchecked, if the attachment contains
> +    data that describes a bug, for example a screenshot or test
> +    data to reproduce a bug.
> +    Patches normally describe changes to the source code which fix
> +    a bug. See also Launchpad help on Bug Attachments.
> +
> +We are sure that this file is indeed a patch, so let's activate the
> +currenty inactive "patch" checkbox again and submit the form.
> +
> +    >>> patch_control = user_browser.getControl(
> +    ...     'This attachment contains a solution (patch) for this bug')
> +    >>> patch_control.selected
> +    False
> +    >>> patch_control.selected = True
> +    >>> user_browser.getControl('Change').click()
> +
> +Now we are redirected to the main bug page...
>  
>      >>> user_browser.url
>      'http://bugs.launchpad.dev/firefox/+bug/1'
>  
> -The attachment that became a patch is now shown in the portlet "Patches"...
> +...the attachment that became a patch is now shown in the portlet
> +"Patches"...
>  
>      >>> patches = find_portlet(user_browser.contents, 'Patches')
>      >>> for li_tag in patches.findAll('li', 'download-attachment'):
>      ...   print li_tag.a.renderContents()
>      Another title
> +    A fix for this bug.
> +    A better icon for foo
>  
>  ...while it is gone from the portlet "Bug attachments".
>  
> @@ -51,6 +90,7 @@
>      >>> for li_tag in attachments.findAll('li', 'download-attachment'):
>      ...   print li_tag.a.renderContents()
>      bar.txt
> +    More data
>  
>  The content type of a patch is automatically changed to text/plain.
>  
> @@ -58,12 +98,13 @@
>      >>> user_browser.getControl('Content Type').value
>      'text/plain'
>  
> -Clicking the link "Add patch" in the patches portlet open the form
> +Clicking the link "Add patch" in the patches portlet opens the form
>  to add bug comments with the checkbox "This attachment is a patch"
>  enabled.
>  
>      >>> user_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
>      >>> user_browser.getLink('Add patch').click()
> -    >>> patch_checkbox = user_browser.getControl('This attachment is a patch')
> +    >>> patch_checkbox = user_browser.getControl(
> +    ...     'This attachment contains a solution (patch) for this bug')
>      >>> patch_checkbox.selected
>      True
> 
> === modified file 'lib/lp/bugs/templates/bug-attachment-edit.pt'
> --- lib/lp/bugs/templates/bug-attachment-edit.pt	2009-08-31 11:55:51 +0000
> +++ lib/lp/bugs/templates/bug-attachment-edit.pt	2010-01-12 10:15:35 +0000
> @@ -12,9 +12,39 @@
>      <div metal:use-macro="context/@@launchpad_form/form">
>  
>        <div metal:fill-slot="extra_info" class="documentDescription">
> -        An attachment can provide valuable information about the bug. It
> -        can also provide a direct solution to a bug - for example a patch
> -        that fixes the bug.
> +        <p>An attachment can provide valuable information about the bug. It
> +          can also provide a direct solution to a bug - for example a patch
> +          that fixes the bug.
> +        </p>
> +        <tal:is-confirmation condition="view/is_confirmation_step">
> +          <tal:is_patch condition="view/is_patch">
> +            <p><strong>This file looks like a patch, but you unselected the

We should use 'uncheck' rather than 'unselect' here, since it's a
checkbox.

> +              checkbox "This attachment contains a solution (patch) for this
> +              bug". Please unselect it again and click on "change",
> +              if you are sure that this file does not contain a patch.
> +            </strong></p>
> +          </tal:is_patch>
> +          <tal:is_not_patch condition="not: view/is_patch">
> +            <p><strong>This file does not look like a patch, but you
> +              selected the checkbox "This attachment contains a solution

And 'selected' here should be 'checked'.

> +              (patch) for this bug". Please select it again and
> +                click on "change", if you are sure that this file does
> +                indeed contain a patch.
> +            </strong></p>
> +            <p>Please leave the field unchecked, if the attachment contains
> +              data that describes a bug, for example a screenshot or test
> +              data to reproduce a bug.
> +            </p>
> +          </tal:is_not_patch>
> +          <p>Patches normally describe changes to the source code which fix
> +            a bug. See also
> +            <a href="https://help.launchpad.net/Bugs/BugAttchements">Launchpad
> +              help on Bug Attachments</a>.
> +          </p>
> +        </tal:is-confirmation>
> +      </div>
> +      <div metal:fill-slot="extra_bottom">
> +        <tal:hidden replace="structure view/confirmation_element" />
>        </div>
>      </div>
>    </div>
>