494369 - Do not throw an error if the 'order' parameter contains invalid columns for buglists

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Frédéric Buclin]

As said in bug 491467 comment 5, I want to convert errors thrown when an invalid column is detected in the LASTORDER cookie or in the order list passed from the form to messages displayed at the top of the buglist.

The problem is not critical enough to frighten the user with such errors.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Teemu Mannermaa (:wicked)]

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

Awesome! There's just few concerns about the changes.

>Index: buglist.cgi
>===================================================================
>+            if (scalar @invalid_fragments) {
>+                if ($order_from_cookie) {
>+                    $cgi->remove_cookie('LASTORDER');
>+                    $vars->{'message'} = 'invalid_column_name_cookie';

I'm not sure having two Set-Cookies for same cookie (first this one to remove and later one to set actual order used) in a response will work for all browsers. (Will they all choose the latter one and set the cookie or will they end up removing the cookie?) This does work for Firefox but I think you could just remove that remove_cookie since 1) you don't anymore say that cookie's removed, 2) it most likely gets sets later anyway and 3) it doesn't matter that much anymore if the invalid cookie gets used again now that it's a message rather than a code error.

>Index: template/en/default/global/messages.html.tmpl
>===================================================================
>+  [% ELSIF message_tag == "invalid_column_name_cookie" %]
>+    The custom sort order specified in your cookie contained some invalid
..
>+  [% ELSIF message_tag == "invalid_column_name_form" %]
>+    The custom sort order specified in your form submission contained some

Nit: This "cookie" and "form submission" sound so technical and I doubt there's any added value stating these anymore. So you could just remove this separation and simplify code quite a bit..

The message would be better if worded like "The custom sort order specified (in your (cookie|form submission)) contains one or more invalid.." to not bother with plurality issues when there's only one invalid columns.

Comment 3

[Frédéric Buclin]

Attachment: patch, v2

Comment 4

[Teemu Mannermaa (:wicked)]

Comment on attachment 379433 [details] [diff] [review]
patch, v2

>Index: template/en/default/global/messages.html.tmpl
>===================================================================
>+  [% ELSIF message_tag == "invalid_column_name" %]
>+    The custom sort order specified in your form submission contains one or more

Please, remove the "in your form submission" part too. I'm sorry if my previous review comment was vague regarding this since I was not sure you'd implement my nit as well.

Thanks for this patch, I think this change is awesome!

Comment 5

[Frédéric Buclin]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.398; previous revision: 1.397
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <-- code-error.html.tmpl
new revision: 1.112; previous revision: 1.111
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.85; previous revision: 1.84
done

Comment 7

[Max Kanat-Alexander]

Added to the release notes in bug 547466.