launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-615763 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #615763 Use LibraryFileAliasWithParent in lp.bugs.browser.bugattachment.BugAttachmentEditView.updateContentType()
  https://bugs.launchpad.net/bugs/615763


This branch makes changing the content type of a bug attachment a bit simpler and more efficient. 

The content type is stored as an attribute of the LibraryFileAlias associtaed with the bugattachment. If a user changed the content type of a bug attachment, the old implementation of BugAttachmentEditView.change_action() downloaded the attachment's file content, created a new LibraryFileAlias instance having this content and finally associated the bug attachment with this new LFA.

This was necessary because LFA attributes are read-only, but since a few weeks we have a class LibraryFileAliasWithParent which allows to change the "restricted" flag. This branch makes another attribute of LFAWithParent, "mimetype", editable too. BugAttachmentEditView.change_action() now adapts the LFA to an LFAWithParent and changes the attribute "mimetype" there.

I also added a few unit tests for BugAttachmentEditView:

./bin/test -vvt test_bugattachment_edit_view

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-615763/+merge/35394
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-615763 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/zcml/librarian.zcml'
--- lib/canonical/launchpad/zcml/librarian.zcml	2010-09-06 15:14:17 +0000
+++ lib/canonical/launchpad/zcml/librarian.zcml	2010-09-14 12:26:12 +0000
@@ -16,7 +16,7 @@
     <allow interface="canonical.launchpad.interfaces.ILibraryFileAliasWithParent" />
     <require
       permission="launchpad.Edit"
-      set_attributes="restricted" />
+      set_attributes="mimetype restricted" />
   </class>
 
   <class class="canonical.launchpad.database.LibraryFileContent">

=== modified file 'lib/lp/bugs/browser/bugattachment.py'
--- lib/lp/bugs/browser/bugattachment.py	2010-09-06 07:44:21 +0000
+++ lib/lp/bugs/browser/bugattachment.py	2010-09-14 12:26:12 +0000
@@ -12,19 +12,21 @@
     'BugAttachmentURL',
     ]
 
-from cStringIO import StringIO
-
-from zope.component import getUtility
+from zope.component import (
+    getMultiAdapter,
+    getUtility,
+    )
 from zope.contenttype import guess_content_type
 from zope.interface import implements
 
 from canonical.launchpad.browser.librarian import (
     FileNavigationMixin,
     ProxiedLibraryFileAlias,
-    StreamOrRedirectLibraryFileAliasView,
     SafeStreamOrRedirectLibraryFileAliasView,
     )
-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.launchpad.interfaces.librarian import (
+    ILibraryFileAliasWithParent,
+    )
 from canonical.launchpad.webapp import (
     canonical_url,
     custom_widget,
@@ -163,7 +165,10 @@
             self.context.title = data['title']
 
         if self.context.libraryfile.mimetype != data['contenttype']:
-            self.updateContentType(data['contenttype'])
+            lfa_with_parent = getMultiAdapter(
+                (self.context.libraryfile, self.context),
+                ILibraryFileAliasWithParent)
+            lfa_with_parent.mimetype = data['contenttype']
 
     @action('Delete Attachment', name='delete')
     def delete_action(self, action, data):
@@ -174,20 +179,6 @@
             url=libraryfile_url, name=self.context.title))
         self.context.removeFromBug(user=self.user)
 
-    def updateContentType(self, new_content_type):
-        """Update the attachment content type."""
-        filealiasset = getUtility(ILibraryFileAliasSet)
-        old_filealias = self.context.libraryfile
-        # Download the file and upload it again with the new content
-        # type.
-        # XXX: Bjorn Tillenius 2005-06-30:
-        # It should be possible to simply create a new filealias
-        # with the same content as the old one.
-        old_content = old_filealias.read()
-        self.context.libraryfile = filealiasset.create(
-            name=old_filealias.filename, size=len(old_content),
-            file=StringIO(old_content), contentType=new_content_type)
-
     @property
     def label(self):
         return smartquote('Bug #%d - Edit attachment "%s"') % (

=== added file 'lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py	2010-09-14 12:26:12 +0000
@@ -0,0 +1,107 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import transaction
+from zope.security.interfaces import Unauthorized
+
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class TestBugAttachmentEditView(TestCaseWithFactory):
+    """Tests of traversal to and access of files of bug attachments."""
+
+    layer = LaunchpadFunctionalLayer
+
+    CHANGE_FORM_DATA = {
+        'field.title': 'new description',
+        'field.patch': 'on',
+        'field.contenttype': 'application/whatever',
+        'field.actions.change': 'Change',
+        }
+
+    def setUp(self):
+        super(TestBugAttachmentEditView, self).setUp()
+        self.bug_owner = self.factory.makePerson()
+        login_person(self.bug_owner)
+        self.bug = self.factory.makeBug(owner=self.bug_owner)
+        self.bugattachment = self.factory.makeBugAttachment(
+            bug=self.bug, filename='foo.diff', data='file content',
+            description='attachment description', content_type='text/plain',
+            is_patch=False)
+        # The Librarian server should know about the new file before
+        # we start the tests.
+        transaction.commit()
+
+    def test_change_action_public_bug(self):
+        # Properties of attachments for public bugs can be
+        # changed by every user.
+        user = self.factory.makePerson()
+        login_person(user)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
+        self.assertEqual('new description', self.bugattachment.title)
+        self.assertTrue(self.bugattachment.is_patch)
+        self.assertEqual(
+            'application/whatever', self.bugattachment.libraryfile.mimetype)
+
+    def test_change_action_private_bug(self):
+        # Subscribers of a private bug can edit attachments.
+        user = self.factory.makePerson()
+        self.bug.subscribe(user, self.bug_owner)
+        self.bug.setPrivate(True, self.bug_owner)
+        transaction.commit()
+        login_person(user)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
+        self.assertEqual('new description', self.bugattachment.title)
+        self.assertTrue(self.bugattachment.is_patch)
+        self.assertEqual(
+            'application/whatever', self.bugattachment.libraryfile.mimetype)
+
+    def test_change_action_private_bug_unauthorized(self):
+        # Other users cannot edit attachments of private bugs.
+        user = self.factory.makePerson()
+        self.bug.setPrivate(True, self.bug_owner)
+        transaction.commit()
+        login_person(user)
+        self.assertRaises(
+            Unauthorized, create_initialized_view, self.bugattachment,
+            name='+edit', form=self.CHANGE_FORM_DATA)
+
+    DELETE_FORM_DATA = {
+        'field.actions.delete': 'Delete Attachment',
+        }
+
+    def test_delete_action_public_bug(self):
+        # Bug attachments can be removed from a bug.
+        user = self.factory.makePerson()
+        login_person(user)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(0, self.bug.attachments.count())
+
+    def test_delete_action_private_bug(self):
+        # Subscribers of a private bug can delete attachments.
+        user = self.factory.makePerson()
+        self.bug.subscribe(user, self.bug_owner)
+        self.bug.setPrivate(True, self.bug_owner)
+        login_person(user)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(0, self.bug.attachments.count())
+
+    def test_delete_action_private_bug_unautorized(self):
+        # Other users cannot delete private bug attachments.
+        user = self.factory.makePerson()
+        self.bug.setPrivate(True, self.bug_owner)
+        login_person(user)
+        self.assertRaises(
+            Unauthorized, create_initialized_view, self.bugattachment,
+            name='+edit', form=self.DELETE_FORM_DATA)