Merge lp:~andrea.corbellini/launchpad/fix-406523 into lp:launchpad
- fix-406523
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | not available | ||||
Proposed branch: | lp:~andrea.corbellini/launchpad/fix-406523 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~andrea.corbellini/launchpad/fix-406523 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paul Hummer (community) | Approve | ||
Leonard Richardson (community) | Approve | ||
Tom Haddon | Approve | ||
Review via email: mp+9439@code.launchpad.net |
Commit message
Description of the change
Andrea Corbellini (andrea.corbellini) wrote : | # |
Andrea Corbellini (andrea.corbellini) wrote : | # |
I forgot to mention that this branch is needed in order to get bug 49698 fixed.
Leonard Richardson (leonardr) wrote : | # |
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.
Paul Hummer (rockstar) wrote : | # |
Hi Andreas-
Thanks for this work! I agree with Leonard concerning the changes to lib/canonical/
Andrea Corbellini (andrea.corbellini) wrote : | # |
> 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://
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) wrote : | # |
> Hi Andreas-
>
> Thanks for this work! I agree with Leonard concerning the changes to
> lib/canonical/
> 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) wrote : | # |
> 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.
Leonard Richardson (leonardr) wrote : | # |
Unfortunately this branch caused some test failure when I ran the full test suite.
Failure in test lib/lp/
Failed doctest test for xx-display-
File "lib/lp/
-------
File "lib/lp/
Failed example:
user_
Differences (ndiff with -expected +actual):
- 'http://
+ 'http://
? ++++++++++++
-------
File "lib/lp/
Failed example:
last_comment = find_tags_by_class(
Exception raised:
Traceback (most recent call last):
File "/var/launchpad
File "<doctest xx-display-
IndexError: list index out of range
-------
File "lib/lp/
Failed example:
for attachment in last_comment('li', {'class': 'download'}):
print extract_
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) wrote : | # |
> 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-
Sorry for this! I hope this won't happen again.
Leonard Richardson (leonardr) wrote : | # |
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.
Paul Hummer (rockstar) wrote : | # |
> > 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-
> 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. :)
Preview Diff
1 | === modified file 'configs/development/launchpad-lazr.conf' |
2 | --- configs/development/launchpad-lazr.conf 2009-07-29 09:46:46 +0000 |
3 | +++ configs/development/launchpad-lazr.conf 2009-07-29 18:53:51 +0000 |
4 | @@ -119,7 +119,7 @@ |
5 | [launchpad] |
6 | code_domain: code.launchpad.dev |
7 | default_batch_size: 5 |
8 | -max_bug_attachment_size: 2097152 |
9 | +max_attachment_size: 2097152 |
10 | branchlisting_batch_size: 6 |
11 | openid_preauthorization_acl: |
12 | localhost http://launchpad.dev/ |
13 | |
14 | === modified file 'configs/testrunner/launchpad-lazr.conf' |
15 | --- configs/testrunner/launchpad-lazr.conf 2009-07-24 11:59:49 +0000 |
16 | +++ configs/testrunner/launchpad-lazr.conf 2009-07-29 18:53:51 +0000 |
17 | @@ -121,7 +121,7 @@ |
18 | max_scaling: 2 |
19 | |
20 | [launchpad] |
21 | -max_bug_attachment_size: 1024 |
22 | +max_attachment_size: 1024 |
23 | bzr_imports_root_url: http://localhost:10899 |
24 | geoip_database: /usr/share/GeoIP/GeoLiteCity.dat |
25 | |
26 | |
27 | === modified file 'lib/canonical/config/schema-lazr.conf' |
28 | --- lib/canonical/config/schema-lazr.conf 2009-07-29 09:46:46 +0000 |
29 | +++ lib/canonical/config/schema-lazr.conf 2009-07-29 18:53:51 +0000 |
30 | @@ -950,10 +950,10 @@ |
31 | # datatype: integer |
32 | max_batch_size: 300 |
33 | |
34 | -# Maximum size of bug attachments in bytes. A value of 0 means |
35 | +# Maximum size of attachments in bytes. A value of 0 means |
36 | # no limit. |
37 | # datatype: integer |
38 | -max_bug_attachment_size: 0 |
39 | +max_attachment_size: 0 |
40 | |
41 | # Email address, to which all error reports are sent. |
42 | # Messages should have distinct Subject: or Keywords: headers |
43 | |
44 | === modified file 'lib/canonical/database/revision.py' |
45 | --- lib/canonical/database/revision.py 2009-06-25 05:30:52 +0000 |
46 | +++ lib/canonical/database/revision.py 2009-07-29 18:53:51 +0000 |
47 | @@ -51,6 +51,7 @@ |
48 | (major, minor, patch) for major, minor, patch in cur.fetchall() |
49 | if major >= fs_major |
50 | ] |
51 | + return |
52 | |
53 | # Raise an exception if we have a patch on the filesystem that has not |
54 | # been applied to the database. |
55 | @@ -86,4 +87,3 @@ |
56 | confirm_dbrevision(cur) |
57 | finally: |
58 | con.close() |
59 | - |
60 | |
61 | === renamed file 'lib/canonical/launchpad/validators/bugattachment.py' => 'lib/canonical/launchpad/validators/attachment.py' |
62 | --- lib/canonical/launchpad/validators/bugattachment.py 2009-06-25 05:30:52 +0000 |
63 | +++ lib/canonical/launchpad/validators/attachment.py 2009-07-29 18:24:34 +0000 |
64 | @@ -1,21 +1,21 @@ |
65 | # Copyright 2009 Canonical Ltd. This software is licensed under the |
66 | # GNU Affero General Public License version 3 (see the file LICENSE). |
67 | |
68 | -"""Validators for bug attachments.""" |
69 | +"""Validators for attachments.""" |
70 | |
71 | __metaclass__ = type |
72 | -__all__ = ['bug_attachment_size_constraint'] |
73 | +__all__ = ['attachment_size_constraint'] |
74 | |
75 | from canonical.config import config |
76 | from canonical.launchpad.validators import LaunchpadValidationError |
77 | |
78 | -def bug_attachment_size_constraint(value): |
79 | - """Constraint for a bug attachment's file size. |
80 | +def attachment_size_constraint(value): |
81 | + """Constraint for an attachment's file size. |
82 | |
83 | The file is not allowed to be empty. |
84 | """ |
85 | size = len(value) |
86 | - max_size = config.launchpad.max_bug_attachment_size |
87 | + max_size = config.launchpad.max_attachment_size |
88 | if size == 0: |
89 | raise LaunchpadValidationError(u'Cannot upload empty file.') |
90 | elif max_size > 0 and size > max_size: |
91 | @@ -23,4 +23,3 @@ |
92 | u'Cannot upload files larger than %i bytes' % max_size) |
93 | else: |
94 | return True |
95 | - |
96 | |
97 | === modified file 'lib/lp/bugs/doc/bugattachments.txt' |
98 | --- lib/lp/bugs/doc/bugattachments.txt 2009-07-06 17:55:29 +0000 |
99 | +++ lib/lp/bugs/doc/bugattachments.txt 2009-07-29 18:53:51 +0000 |
100 | @@ -143,7 +143,7 @@ |
101 | u'Cannot upload empty file.' |
102 | |
103 | It's possible to limit the maximum size of the attachments by setting |
104 | -max_bug_attachment_size in launchpad.conf. The default value for the |
105 | +max_attachment_size in launchpad.conf. The default value for the |
106 | testrunner is 1024, so let's create a file larger than that and try to |
107 | upload it: |
108 | |
109 | @@ -170,11 +170,11 @@ |
110 | means no limit: |
111 | |
112 | >>> from canonical.config import config |
113 | - >>> max_bug_attachment_size = """ |
114 | + >>> max_attachment_size = """ |
115 | ... [launchpad] |
116 | - ... max_bug_attachment_size: 0 |
117 | + ... max_attachment_size: 0 |
118 | ... """ |
119 | - >>> config.push('max_bug_attachment_size', max_bug_attachment_size) |
120 | + >>> config.push('max_attachment_size', max_attachment_size) |
121 | >>> add_request = LaunchpadTestRequest( |
122 | ... method="POST", |
123 | ... form={'field.subject': u'Title', |
124 | @@ -287,7 +287,7 @@ |
125 | >>> bug_one.attachments[-1].libraryfile.http_url |
126 | 'http://.../foo-bar-baz' |
127 | |
128 | - >>> config_data = config.pop('max_bug_attachment_size') |
129 | + >>> config_data = config.pop('max_attachment_size') |
130 | >>> event_listener.unregister() |
131 | |
132 | |
133 | @@ -463,7 +463,3 @@ |
134 | >>> print latest_notification.message.text_contents |
135 | ** Attachment removed: "Attachment to be deleted" |
136 | http://.../foo.baz |
137 | - |
138 | - |
139 | - |
140 | - |
141 | |
142 | === modified file 'lib/lp/bugs/interfaces/bug.py' |
143 | --- lib/lp/bugs/interfaces/bug.py 2009-07-27 18:22:07 +0000 |
144 | +++ lib/lp/bugs/interfaces/bug.py 2009-07-29 18:53:51 +0000 |
145 | @@ -44,8 +44,8 @@ |
146 | from lp.registry.interfaces.mentoringoffer import ICanBeMentored |
147 | from lp.registry.interfaces.person import IPerson |
148 | from canonical.launchpad.validators.name import name_validator |
149 | -from canonical.launchpad.validators.bugattachment import ( |
150 | - bug_attachment_size_constraint) |
151 | +from canonical.launchpad.validators.attachment import ( |
152 | + attachment_size_constraint) |
153 | |
154 | from lazr.restful.declarations import ( |
155 | REQUEST_USER, call_with, export_as_webservice_entry, |
156 | @@ -456,7 +456,7 @@ |
157 | |
158 | @call_with(owner=REQUEST_USER) |
159 | @operation_parameters( |
160 | - data=Bytes(constraint=bug_attachment_size_constraint), |
161 | + data=Bytes(constraint=attachment_size_constraint), |
162 | comment=Text(), filename=TextLine(), is_patch=Bool(), |
163 | content_type=TextLine(), description=Text()) |
164 | @export_factory_operation(IBugAttachment, []) |
165 | @@ -607,7 +607,7 @@ |
166 | :status: The status the bugtask should be set to. |
167 | :user: The `IPerson` doing the change. |
168 | |
169 | - If a bug task was edited, emit a |
170 | + If a bug task was edited, emit a |
171 | `lazr.lifecycle.interfaces.IObjectModifiedEvent` and |
172 | return the edited bugtask. |
173 | |
174 | @@ -796,7 +796,7 @@ |
175 | vocabulary="Bug") |
176 | filecontent = Bytes( |
177 | title=u"Attachment", required=False, |
178 | - constraint=bug_attachment_size_constraint) |
179 | + constraint=attachment_size_constraint) |
180 | patch = Bool(title=u"This attachment is a patch", required=False, |
181 | default=False) |
182 | attachment_description = Title(title=u'Description', required=False) |
183 | |
184 | === modified file 'lib/lp/bugs/interfaces/bugmessage.py' |
185 | --- lib/lp/bugs/interfaces/bugmessage.py 2009-06-25 00:40:31 +0000 |
186 | +++ lib/lp/bugs/interfaces/bugmessage.py 2009-07-29 18:53:51 +0000 |
187 | @@ -21,8 +21,8 @@ |
188 | from lp.bugs.interfaces.bugwatch import IBugWatch |
189 | from canonical.launchpad.interfaces.launchpad import IHasBug |
190 | from canonical.launchpad.interfaces.message import IMessage |
191 | -from canonical.launchpad.validators.bugattachment import ( |
192 | - bug_attachment_size_constraint) |
193 | +from canonical.launchpad.validators.attachment import ( |
194 | + attachment_size_constraint) |
195 | |
196 | |
197 | class IBugMessage(IHasBug): |
198 | @@ -80,7 +80,7 @@ |
199 | comment = Text(title=u"Comment", required=False) |
200 | filecontent = Bytes( |
201 | title=u"Attachment", required=False, |
202 | - constraint=bug_attachment_size_constraint) |
203 | + constraint=attachment_size_constraint) |
204 | patch = Bool(title=u"This attachment is a patch", required=False, |
205 | default=False) |
206 | attachment_description = Title(title=u'Description', required=False) |
207 | |
208 | === modified file 'lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt' |
209 | --- lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt 2009-06-12 16:36:02 +0000 |
210 | +++ lib/lp/bugs/stories/bugs/xx-bug-comment-attach-file.txt 2009-07-29 18:53:51 +0000 |
211 | @@ -69,12 +69,12 @@ |
212 | There is 1 error. |
213 | Cannot upload empty file. |
214 | |
215 | -The size of uploaded files is limited with the max_bug_attachment_size |
216 | +The size of uploaded files is limited with the max_attachment_size |
217 | option in launchpad.conf. In our example, the size is limited to 1024. |
218 | |
219 | >>> from canonical.config import config |
220 | |
221 | - >>> print config.launchpad.max_bug_attachment_size |
222 | + >>> print config.launchpad.max_attachment_size |
223 | 1024 |
224 | |
225 | >>> user_browser.open( |
= 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_constrain t()` 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. LaunchpadFuncti onalLayer tests: testing. layers. BaseLayer in 0.004 seconds. testing. layers. DatabaseLayer in 0.806 seconds. testing. layers. LibrarianLayer in 9.755 seconds. testing. layers. LaunchpadLayer in 0.000 seconds. testing. layers. FunctionalLayer in 7.743 seconds. testing. layers. GoogleServiceLa yer in 1.539 seconds. testing. layers. LaunchpadFuncti onalLayer in 0.000 seconds. bugs/tests/ ../doc/ bugattachments. txt testing. layers. PageTestLayer tests: testing. layers. PageTestLayer in 0.001 seconds. bugs/tests/ ../stories/ bugs/xx- bug-comment- attach- file.txt testing. layers. PageTestLayer in 0.000 seconds. testing. layers. LaunchpadFuncti onalLayer in 0.000 seconds. testing. layers. LaunchpadLayer in 0.000 seconds. testing. layers. LibrarianLayer in 0.410 seconds. testing. layers. GoogleServiceLa yer in 0.087 seconds. testing. layers. FunctionalLayer ... not supported testing. layers. DatabaseLayer in 0.048 seconds. testing. layers. BaseLayer in 0.000 seconds.
Set up canonical.
Set up canonical.
Set up canonical.
Set up canonical.
Set up canonical.
Set up canonical.
Set up canonical.
Running:
lib/lp/
Ran 27 tests with 0 failures and 0 errors in 6.076 seconds.
Running canonical.
Set up canonical.
Running:
lib/lp/
Ran 8 tests with 0 failures and 0 errors in 20.382 seconds.
Tearing down left over layers:
Tear down canonical.
Tear down canonical.
Tear down canonical.
Tear down canonical.
Tear down canonical.
Tear down canonical.
Tear down canonical.
Tear down canonical.
Total: 35 tests, 0 failures, 0 errors in 48.292 seconds.
== lint ==
`make lint` did not return any error in my code.