490930 - Always store attachments locally if they are over X size, don't ever display "Big File" checkbox

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Max Kanat-Alexander]

The "Big File" checkbox is confusing to users for various reasons. pyrzak's students' research pointed out that it has a note next to it about data getting purged--that's something the user has no control over, so it probably shouldn't even be displayed to them.

Also, the user isn't in a position to decide whether the attachment should be stored locally or in the database--that's more something that the administrator should be deciding.

So we should probably have a single parameter called something like big_file_size, and any file over that size is automatically stored locally. It can default to some enormous number so that files are never stored locally, by default. Setting it to 0 means that all files are stored locally and never stored in the database.

Comment 1

[Frédéric Buclin]

The problem is that we cannot know in advance whether the attachment will be large or not. And the way we get the data is very different depending on the answer to this question.

But I agree that this "Big File" checkbox should never be displayed.

Comment 3

[Max Kanat-Alexander]

Actually, I think 0 should mean "always store in the database", since a *negative* number would logically mean "always store on the disk".

Comment 4

[Frédéric Buclin]

<LpSolit> mkanat: to fix it, we can no longer use $data = <$fh>, because it slurps the whole attachment at once. I would always have to store it in a tempfile first, before deciding what to do with it
<mkanat> LpSolit: Well, actually, we already always store it in a temp file first.
<mkanat> LpSolit: Because that's what CGI.pm does with it.
<mkanat> LpSolit: So that's OK, I think. 

Let's do this. :)

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

Both parameters are still required as you don't want to store files of arbitrary size. Now that I moved the check of the file size in _check_data() (as it should be), I can copy the whole temporary file at once using File::Copy::copy(). This should be faster than looping over each line of the file.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 456331 [details] [diff] [review]
patch, v1

This is an awesome cleanup, and looks great. :-)

Comment 7

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified attachment.cgi
modified post_bug.cgi
modified Bugzilla/Attachment.pm
modified docs/en/xml/using.xml
modified js/attachment.js
modified template/en/default/admin/params/attachment.html.tmpl
modified template/en/default/attachment/createformcontents.html.tmpl
modified template/en/default/global/code-error.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7297.