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

RESOLVED FIXED in Bugzilla 3.6

Status

()

--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({ue})

3.3.4
Bugzilla 3.6
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 379130 [details] [diff] [review]
patch, v1
Attachment #379130 - Flags: review?(wicked)
Attachment #379130 - Flags: review?(ghendricks)
Attachment #379130 - Flags: review?(wicked)
Attachment #379130 - Flags: review?(ghendricks)
Attachment #379130 - Flags: review-
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.
(Assignee)

Comment 3

10 years ago
Created attachment 379433 [details] [diff] [review]
patch, v2
Attachment #379130 - Attachment is obsolete: true
Attachment #379433 - Flags: review?(wicked)
Attachment #379433 - Flags: review?(wicked) → review+
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!
(Assignee)

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 5

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Duplicate of this bug: 276728

Updated

9 years ago
Keywords: relnote

Comment 7

9 years ago
Added to the release notes in bug 547466.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.