Merge lp:~andrea.corbellini/launchpad/fix-406523 into lp:launchpad

[Andrea Corbellini]

[Andrea Corbellini (andrea.corbellini)]

= Summary =

Bugs, blueprints and answers needs attachments to be inserted in comments. Currently to limit the size bug comments attachments there's a validator in `canonical.launchpad.validators.bugattachments`. It wouldn't be useful to create new validators that do the same thing in the same way for blueprints and answers too.

== Pre-implementation notes ==

I've discussed this change with sinzui and he agreed.

== Implementation details ==

With this branch I've renamed `bugattachments` to `attachments`, making it more generic. I've also renamed the function `bug_attachment_size_constraint()` to `attachment_size_constraint()` and the configuration item `max_bug_attachment_size` to `max_attachment_size`.

== Tests ==

$ bin/test -vv -t lib/lp/bugs/tests/../stories/bugs/xx-bug-comment-attach-file.txt -t lib/lp/bugs/tests/../doc/bugattachments.txt
Running tests at level 1

Running canonical.testing.layers.LaunchpadFunctionalLayer tests:
  Set up canonical.testing.layers.BaseLayer in 0.004 seconds.
  Set up canonical.testing.layers.DatabaseLayer in 0.806 seconds.
  Set up canonical.testing.layers.LibrarianLayer in 9.755 seconds.
  Set up canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Set up canonical.testing.layers.FunctionalLayer in 7.743 seconds.
  Set up canonical.testing.layers.GoogleServiceLayer in 1.539 seconds.
  Set up canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Running:
 lib/lp/bugs/tests/../doc/bugattachments.txt
  Ran 27 tests with 0 failures and 0 errors in 6.076 seconds.
Running canonical.testing.layers.PageTestLayer tests:
  Set up canonical.testing.layers.PageTestLayer in 0.001 seconds.
  Running:
 lib/lp/bugs/tests/../stories/bugs/xx-bug-comment-attach-file.txt
  Ran 8 tests with 0 failures and 0 errors in 20.382 seconds.
Tearing down left over layers:
  Tear down canonical.testing.layers.PageTestLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LibrarianLayer in 0.410 seconds.
  Tear down canonical.testing.layers.GoogleServiceLayer in 0.087 seconds.
  Tear down canonical.testing.layers.FunctionalLayer ... not supported
  Tear down canonical.testing.layers.DatabaseLayer in 0.048 seconds.
  Tear down canonical.testing.layers.BaseLayer in 0.000 seconds.
Total: 35 tests, 0 failures, 0 errors in 48.292 seconds.

== lint ==

`make lint` did not return any error in my code.

[Andrea Corbellini (andrea.corbellini)]

I forgot to mention that this branch is needed in order to get bug 49698 fixed.

[Leonard Richardson (leonardr)]

Thanks for doing this branch. It looks like a good first step.

I'm very suspicious of the unconditional return statement in line 51. It looks like you added that during development and that it should be removed.

Other than that, this branch looks good. Approved*. I'm a trainee reviewer so you'll need to get rockstar to approve my change, and then have an admin look at it because it changes configuration.

review: Approve

[Paul Hummer (rockstar)]

Hi Andreas-

  Thanks for this work! I agree with Leonard concerning the changes to lib/canonical/database/revision.py - I think it would be best to just revert both changes in there. Yes, you're removing a line, but we generally don't make whitespace changes to a file unless we actually edit it so that we don't cause spurious conflicts. Other than that, thanks for looking into this!

review: Approve

[Andrea Corbellini (andrea.corbellini)]

> Thanks for doing this branch. It looks like a good first step.
>
> I'm very suspicious of the unconditional return statement in line 51. It looks
> like you added that during development and that it should be removed.

I've removed the `return` in the last revision: http://bazaar.launchpad.net/~andrea-bs/launchpad/fix-406523/revision/9007.
The reason why you see it in the diff is because is hasn't been updated after my last push (there's a bug about this somewhere).

> Other than that, this branch looks good. Approved*. I'm a trainee reviewer so
> you'll need to get rockstar to approve my change, and then have an admin look
> at it because it changes configuration.

Thanks for your help!

[Andrea Corbellini (andrea.corbellini)]

> Hi Andreas-
>
> Thanks for this work! I agree with Leonard concerning the changes to
> lib/canonical/database/revision.py - I think it would be best to just revert
> both changes in there. Yes, you're removing a line, but we generally don't
> make whitespace changes to a file unless we actually edit it so that we don't
> cause spurious conflicts. Other than that, thanks for looking into this!

I've re-added the blankline at the end of the file too. Now everything should be fine, thanks!

[Tom Haddon (mthaddon)]

> Thanks for doing this branch. It looks like a good first step.
>
> I'm very suspicious of the unconditional return statement in line 51. It looks
> like you added that during development and that it should be removed.
>
> Other than that, this branch looks good. Approved*. I'm a trainee reviewer so
> you'll need to get rockstar to approve my change, and then have an admin look
> at it because it changes configuration.

I've checked the production-configs branch and we don't set this value, so I think it's safe to change it.

Thanks for checking with us, that's definitely needed for any config change.

review: Approve

[Leonard Richardson (leonardr)]

Unfortunately this branch caused some test failure when I ran the full test suite.

Failure in test lib/lp/bugs/tests/../stories/bugattachments/xx-display-filesize-attachment.txt
Failed doctest test for xx-display-filesize-attachment.txt
  File "lib/lp/bugs/tests/../stories/bugattachments/xx-display-filesize-attachment.txt", line 0

----------------------------------------------------------------------
File "lib/lp/bugs/tests/../stories/bugattachments/xx-display-filesize-attachment.txt", line 48, in xx-display-filesize-attachment.txt
Failed example:
    user_browser.url
Differences (ndiff with -expected +actual):
    - 'http://bugs.launchpad.dev/firefox/+bug/1'
    + 'http://bugs.launchpad.dev/firefox/+bug/1/+addcomment'
    ? ++++++++++++
----------------------------------------------------------------------
File "lib/lp/bugs/tests/../stories/bugattachments/xx-display-filesize-attachment.txt", line 51, in xx-display-filesize-attachment.txt
Failed example:
    last_comment = find_tags_by_class(
        user_browser.contents, 'boardCommentBody')[-1]
Exception raised:
    Traceback (most recent call last):
      File "/var/launchpad/test/lib/zope/testing/doctest.py", line 1360, in __run
        compileflags, 1) in test.globs
      File "<doctest xx-display-filesize-attachment.txt[23]>", line 1, in ?
    IndexError: list index out of range
----------------------------------------------------------------------
File "lib/lp/bugs/tests/../stories/bugattachments/xx-display-filesize-attachment.txt", line 54, in xx-display-filesize-attachment.txt
Failed example:
    for attachment in last_comment('li', {'class': 'download'}):
        print extract_text(attachment)
Differences (ndiff with -expected +actual):
      description text
    - (2.6 KiB, text/plain)
    + (270 bytes,
    + text/plain)

I don't know exactly what's going on here, but the test that fails does have a connection to this code, so I doubt it's a spurious failure. Try to get it to work and let me know if you have problems.

[Andrea Corbellini (andrea.corbellini)]

> I don't know exactly what's going on here, but the test that fails does have a
> connection to this code, so I doubt it's a spurious failure. Try to get it to
> work and let me know if you have problems.

Argh! For some reasons I haven't seen xx-display-filesize-attachment.txt in the output of grep. Now it's fixed, and works as expected.

Sorry for this! I hope this won't happen again.

[Leonard Richardson (leonardr)]

I've reviewed your change and it looks good. rockstar needs to mentor the change, and I'm running the test suite again to verify that there are no failures.

review: Approve

[Paul Hummer (rockstar)]

> > I don't know exactly what's going on here, but the test that fails does have
> a
> > connection to this code, so I doubt it's a spurious failure. Try to get it
> to
> > work and let me know if you have problems.
>
> Argh! For some reasons I haven't seen xx-display-filesize-attachment.txt in
> the output of grep. Now it's fixed, and works as expected.
>
> Sorry for this! I hope this won't happen again.

It's not a big deal. This happens in more cases than you'd think, and it's the entire reason we land things through ec2test.

In the future, when you make incremental changes, it's good to post the incremental diff. That way, I don't have to grab your branch to see the changes you've made. I've learned that in Launchpad development, the best thing you can do is make your reviewer's job easy. :)

review: Approve

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2009-07-29 09:46:46 +0000
+++ configs/development/launchpad-lazr.conf	2009-07-29 18:53:51 +0000
@@ -119,7 +119,7 @@
 [launchpad]
 code_domain: code.launchpad.dev
 default_batch_size: 5
-max_bug_attachment_size: 2097152
+max_attachment_size: 2097152
 branchlisting_batch_size: 6
 openid_preauthorization_acl:
     localhost http://launchpad.dev/
 
=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2009-07-24 11:59:49 +0000
+++ configs/testrunner/launchpad-lazr.conf	2009-07-29 18:53:51 +0000
@@ -121,7 +121,7 @@
 max_scaling: 2
 
 [launchpad]
-max_bug_attachment_size: 1024
+max_attachment_size: 1024
 bzr_imports_root_url: http://localhost:10899
 geoip_database: /usr/share/GeoIP/GeoLiteCity.dat
 
 
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2009-07-29 09:46:46 +0000
+++ lib/canonical/config/schema-lazr.conf	2009-07-29 18:53:51 +0000
@@ -950,10 +950,10 @@
 # datatype: integer
 max_batch_size: 300
 
-# Maximum size of bug attachments in bytes. A value of 0 means
+# Maximum size of attachments in bytes. A value of 0 means
 # no limit.
 # datatype: integer
-max_bug_attachment_size: 0
+max_attachment_size: 0
 
 # Email address, to which all error reports are sent.
 # Messages should have distinct Subject: or Keywords: headers
 
=== modified file 'lib/canonical/database/revision.py'
--- lib/canonical/database/revision.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/database/revision.py	2009-07-29 18:53:51 +0000
@@ -51,6 +51,7 @@
             (major, minor, patch) for major, minor, patch in cur.fetchall()
                 if major >= fs_major
             ]
+    return
 
     # Raise an exception if we have a patch on the filesystem that has not
     # been applied to the database.
@@ -86,4 +87,3 @@
         confirm_dbrevision(cur)
     finally:
         con.close()
-
 
=== renamed file 'lib/canonical/launchpad/validators/bugattachment.py' => 'lib/canonical/launchpad/validators/attachment.py'
--- lib/canonical/launchpad/validators/bugattachment.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/validators/attachment.py	2009-07-29 18:24:34 +0000
@@ -1,21 +1,21 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Validators for bug attachments."""
+"""Validators for attachments."""
 
 __metaclass__ = type
-__all__ = ['bug_attachment_size_constraint']
+__all__ = ['attachment_size_constraint']
 
 from canonical.config import config
 from canonical.launchpad.validators import LaunchpadValidationError
 
-def bug_attachment_size_constraint(value):
-    """Constraint for a bug attachment's file size.
+def attachment_size_constraint(value):
+    """Constraint for an attachment's file size.
 
     The file is not allowed to be empty.
     """
     size = len(value)
-    max_size = config.launchpad.max_bug_attachment_size
+    max_size = config.launchpad.max_attachment_size
     if size == 0:
         raise LaunchpadValidationError(u'Cannot upload empty file.')
     elif max_size > 0 and size > max_size:
@@ -23,4 +23,3 @@
             u'Cannot upload files larger than %i bytes' % max_size)
     else:
         return True
-
 
=== modified file 'lib/lp/bugs/doc/bugattachments.txt'
--- lib/lp/bugs/doc/bugattachments.txt	2009-07-06 17:55:29 +0000
+++ lib/lp/bugs/doc/bugattachments.txt	2009-07-29 18:53:51 +0000
@@ -143,7 +143,7 @@
     u'Cannot upload empty file.'
 
 It's possible to limit the maximum size of the attachments by setting
-max_bug_attachment_size in launchpad.conf. The default value for the
+max_attachment_size in launchpad.conf. The default value for the
 testrunner is 1024, so let's create a file larger than that and try to
 upload it:
 
@@ -170,11 +170,11 @@
 means no limit:
 
     >>> from canonical.config import config
-    >>> max_bug_attachment_size = """
+    >>> max_attachment_size = """
     ...     [launchpad]
-    ...     max_bug_attachment_size: 0
+    ...     max_attachment_size: 0
     ...     """
-    >>> config.push('max_bug_attachment_size', max_bug_attachment_size)
+    >>> config.push('max_attachment_size', max_attachment_size)
     >>> add_request = LaunchpadTestRequest(
     ...     method="POST",
     ...     form={'field.subject': u'Title',
@@ -287,7 +287,7 @@
     >>> bug_one.attachments[-1].libraryfile.http_url
     'http://.../foo-bar-baz'
 
-    >>> config_data = config.pop('max_bug_attachment_size')
+    >>> config_data = config.pop('max_attachment_size')
     >>> event_listener.unregister()
 
 
@@ -463,7 +463,3 @@
     >>> print latest_notification.message.text_contents
     ** Attachment removed: "Attachment to be deleted"
        http://.../foo.baz
-
-
-
-
 
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2009-07-27 18:22:07 +0000
+++ lib/lp/bugs/interfaces/bug.py	2009-07-29 18:53:51 +0000
@@ -44,8 +44,8 @@
 from lp.registry.interfaces.mentoringoffer import ICanBeMentored
 from lp.registry.interfaces.person import IPerson
 from canonical.launchpad.validators.name import name_validator
-from canonical.launchpad.validators.bugattachment import (
-    bug_attachment_size_constraint)
+from canonical.launchpad.validators.attachment import (
+    attachment_size_constraint)
 
 from lazr.restful.declarations import (
     REQUEST_USER, call_with, export_as_webservice_entry,
@@ -456,7 +456,7 @@
 
     @call_with(owner=REQUEST_USER)
     @operation_parameters(
-        data=Bytes(constraint=bug_attachment_size_constraint),
+        data=Bytes(constraint=attachment_size_constraint),
         comment=Text(), filename=TextLine(), is_patch=Bool(),
         content_type=TextLine(), description=Text())
     @export_factory_operation(IBugAttachment, [])
@@ -607,7 +607,7 @@
             :status: The status the bugtask should be set to.
             :user: The `IPerson` doing the change.
 
-        If a bug task was edited, emit a 
+        If a bug task was edited, emit a
         `lazr.lifecycle.interfaces.IObjectModifiedEvent` and
         return the edited bugtask.
 
@@ -796,7 +796,7 @@
         vocabulary="Bug")
     filecontent = Bytes(
         title=u"Attachment", required=False,
-        constraint=bug_attachment_size_constraint)
+        constraint=attachment_size_constraint)
     patch = Bool(title=u"This attachment is a patch", required=False,
         default=False)
     attachment_description = Title(title=u'Description', required=False)
 
=== modified file 'lib/lp/bugs/interfaces/bugmessage.py'
--- lib/lp/bugs/interfaces/bugmessage.py	2009-06-25 00:40:31 +0000
+++ lib/lp/bugs/interfaces/bugmessage.py	2009-07-29 18:53:51 +0000
@@ -21,8 +21,8 @@
 from lp.bugs.interfaces.bugwatch import IBugWatch
 from canonical.launchpad.interfaces.launchpad import IHasBug
 from canonical.launchpad.interfaces.message import IMessage
-from canonical.launchpad.validators.bugattachment import (
-    bug_attachment_size_constraint)
+from canonical.launchpad.validators.attachment import (
+    attachment_size_constraint)
 
 
 class IBugMessage(IHasBug):
@@ -80,7 +80,7 @@
     comment = Text(title=u"Comment", required=False)
     filecontent = Bytes(
         title=u"Attachment", required=False,
-        constraint=bug_attachment_size_constraint)
+        constraint=attachment_size_constraint)
     patch = Bool(title=u"This attachment is a patch", required=False,
         default=False)
     attachment_description = Title(title=u'Description', required=False)
 
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt	2009-07-29 18:53:51 +0000
@@ -69,12 +69,12 @@
     There is 1 error.
     Cannot upload empty file.
 
-The size of uploaded files is limited with the max_bug_attachment_size
+The size of uploaded files is limited with the max_attachment_size
 option in launchpad.conf. In our example, the size is limited to 1024.
 
     >>> from canonical.config import config
 
-    >>> print config.launchpad.max_bug_attachment_size
+    >>> print config.launchpad.max_attachment_size
     1024
 
     >>> user_browser.open(