164038 - token.cgi: Cancel token messages should be moved into the templates

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Tobias Burnus]

At the present all token cancel messages are in token.cgi
(They are passed to Token::Cancel() and to the message.html.tmpl.
Left over from bug 159901)

Patch to come (after testing).

Comment 1

[Tobias Burnus]

Assign to me since I have a patch

Comment 2

[Tobias Burnus]

Attachment: v1: Patch token.cgi, cancel-token.txt.tmpl and messages.html.tmpl

Please review.

Comment 3

[Bradley Baetz (:bbaetz)]

Comment on attachment 96313 [details] [diff] [review]
v1: Patch token.cgi, cancel-token.txt.tmpl and messages.html.tmpl

This is all good, except:

>--- template/en/default/account/cancel-token.txt.tmpl	18 Apr 2002 18:56:15 -0000	1.1
>+++ template/en/default/account/cancel-token.txt.tmpl	22 Aug 2002 15:32:25 -0000

>+
>+  [% ELSE %]
>+    [% cancelaction %]
>+  [% END %]

Have this print an error message similar to how messages.html.tmpl works.

Comment 4

[Tobias Burnus]

Attachment: v2: Spit out an error message instead of the passed string

> Have this print an error message similar to how messages.html.tmpl works.
Done. Hope the wording is ok.

Comment 5

[Bradley Baetz (:bbaetz)]

Comment on attachment 96827 [details] [diff] [review]
v2: Spit out an error message instead of the passed string

The message_tags should be in alphabetical order. (whether _ comes before or
after letters doesn't matter; just make it consistant)

Comment 6

[Tobias Burnus]

Attachment: v3: Sort tags in messages.html.tmpl, DisplayError messages moved to user-error.html.tmpl

> The message_tags should be in alphabetical order. (whether _ comes before or
> after letters doesn't matter; just make it consistant)
I used: '_' comes before 'a'.

The messages.html.tmpl was rather unsorted, I corrected this too.

As a plus I moved the DisplayError messages to user-error.html.tmpl.

Comment 7

[Bradley Baetz (:bbaetz)]

Comment on attachment 97168 [details] [diff] [review]
v3: Sort tags in messages.html.tmpl, DisplayError messages moved to user-error.html.tmpl

This looks fine, except what are teh [% # --- ] for?

r=bbaetz x2 if you remove those

Do you have cvs access, or do you need me to check it in for you?

Comment 8

[Gervase Markham [:gerv]]

> As a plus I moved the DisplayError messages to user-error.html.tmpl.

Well, you can move them straight back again :-)

template/en/default/global/header.html.tmpl:  [% PROCESS
global/messages.html.tmpl %]

They are in that file for a reason, you know :-) Some of the messages get
included from header.html.tmpl and some from user-error.html.tmpl. So they have
to be in a single file which can be included from either.

Gerv

Comment 9

[Bradley Baetz (:bbaetz)]

Gerv - burnus moved the token error messages, not all of them....

Comment 10

[Gervase Markham [:gerv]]

All the messages should be in the same place. 

I don't want the thing we have currently with globals.pl and CGI.pl where you
are looking for a "global" function and always look in the wrong file first. 

I did think about this when I designed it, you know :-)

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

user-error is already separate - I'm not sure what you mean.

Comment 12

[Gervase Markham [:gerv]]

The "code error" error messages are in code-error.html.tmpl.
The "user error" error messages are _not_ in user-error.html.tmpl, as you might
expect. 

This is because we have multiple ways of displaying messages. One of them is to
use message.html.tmpl - it uses header.html.tmpl to display its message. Another
is to use header.html.tmpl "in passing", while also providing other content. A
third is to use user-error.html.tmpl.

So, the messages are all in message_s_.html.tmpl, which is included from
header.html.tmpl and user-error.html.tmpl. This is by design. It allows the
messages.html.tmpl file to be included from multiple places, and also for
messages to be used for non-HTML errors (in most cases) because the template
which defines them contains no "surrounding" HTML. 

The messages should all be kept together in this file, in one place, rather than
moved to either header.html.tmpl or user-error.html.tmpl.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

So, um... what _is_ in user-messages.html.tmpl?

And more importantly, why do we have three different ways of displaying errors?

(If the answer includes $vars->{headers_done}, then I'll need another reason,
since that is going to have to go away soonish - global vars are bad)

Comment 14

[Gervase Markham [:gerv]]

> So, um... what _is_ in user-messages.html.tmpl?

Neither I nor LXR have heard of that file.

> And more importantly, why do we have three different ways of displaying errors?

We have three different ways of displaying errors, and two of displaying messages. 
ThrowCodeError: for errors which are "this-shouldn't-happen" - includes an
exhortation to send the error to the admin, and may also be logged.
ThrowUserError: for errors which are, sort of, the user's fault - like an empty
compulsory form field
ThrowTemplateError: for template errors only; this falls back on print
statements as necessary.

All of these are necessary and useful.

We also have two ways of displaying messages. They are:
Displaying a page, but setting the "message" template variable to a key - that
will print the corresponding message at the top of the page. This message is
printed by header.html.tmpl.
Using message.html.tmpl - this is basically an empty shell (just header and
footer), with header.html.tmpl again displaying the message.

Currently, the strings for ThrowCodeError are in code-error.html.tmpl, and those
for ThrowUserError and the other messages are in message_s_.html.tmpl. 

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Well, currently (as in, whats in CVS right now) ThrowUserError uses
user-error.html.tmpl, which doesn't have any message tags defined in it (Except
for some default dummy ones). It does not include any message template.

This means that _all_ user errors just show the tag, not the text, which is
useless.....

Comment 16

[Gervase Markham [:gerv]]

Something's gone a bit wrong, then :-) I'll look at this ASAP.

Gerv

Comment 17

[Gervase Markham [:gerv]]

OK, I've fixed up the error stuff over in bug 166698. So that's probably broken
this patch :-|. When you refresh it, you should follow the scheme I posted to
the newsgroup about where to put stuff. It's very easy. Apologies for the confusion.

Gerv

Comment 18

[Gervase Markham [:gerv]]

burnus: ping? :-)

Gerv

Comment 19

[Tobias Burnus]

Attachment: v4: Rediff, DisplayError -> Throw{User,Code}Error

This templatizes all DisplayErrors in [tT]oken.{pm,cgi} but one
(ValidatePassword, in globals.pl).
I think I've split user/code error correctly.

account/cancel-token.txt.tmpl uses its own token--string translations.
I thought of using messages.html.tmpl for this, but I cannot share the strings
with ThrowUserError (which uses user-error.html.tmpl) and since furthemore some
strings are specific to mail I decided they would be better in
cancel-token.txt.tmpl.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 101070 [details] [diff] [review]
v4: Rediff, DisplayError -> Throw{User,Code}Error


>-    That account already exists.
>+    [% IF email %]
>+      [% email FILTER html %]
>+    [% ELSE %]
>+      That
>+    [% END %]
>+    account already exists.

Slightly dodgy grammar here in the IF case. 
Ideally, we'd get a message:
"There is already an account with the login name [e-mail]."
or
"There is already an account with the that login name."

Other than that, this is great. So I'll fix that one nit and check it in.

Gerv

Comment 21

[Gervase Markham [:gerv]]

Fixed.

Checking in Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.16; previous revision: 1.15
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/account/cancel-token.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/cancel-token.txt.tmpl,v
 <--  cancel-token.txt.tmpl
new revision: 1.2; previous revision: 1.1
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.14; previous revision: 1.13
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.9; previous revision: 1.8
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.12; previous revision: 1.11
done

Gerv