225703 - Partial templatization for editkeywords.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Vlad Dascalu]

Partial templatization for editkeywords.cgi.

Comment 1

[Vlad Dascalu]

Attachment: Partial templatization

Smaller patches are easier to review.

If someone likes a complete rewrite instead of a partial patch, let me know.

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 135486 [details] [diff] [review]
Partial templatization

r=gerv, but it should have a second review. Looks mostly OK to me. I have a few
language nits, some of which may well have been present in the original.

>+[% IF action == "update" %]
>+  [% title = "Update keyword" %]
>+[% ELSIF action == "delete" %]
>+  [% title = "Delete keyword" %]
>+[% END %]
>+
>+[% PROCESS global/header.html.tmpl %]
>+
>+Keyword [% name %] [%+action %]d.

This is rather inconvenient for localisers. Can you spell it out explicitly
with an IF test?

>+
>+<p>
>+  <b>You have deleted or modified a keyword. You must
>+  rebuild the keyword cache!</b><br>

A bit excitable! "After you have finished deleting or modifying keywords, you
need to rebuild the keyword cache." would do.

>+  You can rebuild the cache using sanitycheck.cgi.

This sentence is unnecessary.

>+  On very large installations of Bugzilla,<br>
>+  this can take several minutes.

"Warning: on a very..."

>+</p>
>+
>+<p>
>+  <b><a href="sanitycheck.cgi?rebuildkeywordcache=1">Rebuild
>+  cache</a></b>

Rebuild _keyword_ cache.

>+</p>
>+
>+<p>Back to the <a href="query.cgi">query page</a> or <a href="editkeywords.cgi">edit</a> more keywords.</p>

"_Go_ back to the..." (here and in other templates.)

Gerv

Comment 3

[Vlad Dascalu]

Comment on attachment 135486 [details] [diff] [review]
Partial templatization

Canceling request review on obsolete patch.

Comment 4

[Vlad Dascalu]

Attachment: Ver 2

Thanks for the comments!

This version addresses Gerv's suggestions.

Comment 5

[Vlad Dascalu]

Comment on attachment 135511 [details] [diff] [review]
Ver 2

Gerv, can you do the second review? Or did you mean someone else?

Comment 6

[Vlad Dascalu]

Note to myself: The <br> from the "Warning: on a very large installation of
Bugzilla,<br>" needs to go away because I eliminated an unnecessary sentence
before it.

Comment 7

[Gervase Markham [:gerv]]

I meant someone else :-)

Gerv

Comment 8

[Vlad Dascalu]

Attachment: Version 3

Eliminating <br>, and doing Bugzilla --> [% terms.Bugzilla %].

Comment 9

[Vlad Dascalu]

Comment on attachment 135521 [details] [diff] [review]
Version 3

Since kiko seems away, anybody should feel free to take upon this r? thing :)

Comment 10

[Jouni Heikniemi]

Templatization patches... oh the pain :-) I'll try to check this out tomorrow
morning. 

-> patch author

Comment 11

[Jouni Heikniemi]

Comment on attachment 135521 [details] [diff] [review]
Version 3

>Smaller patches are easier to review.
>If someone likes a complete rewrite instead of a partial patch, let me know.

I agree, and a partial patch is fine with me. Anyway, you should clearly say
when you're templatizing "the rest of editkeywords", because otherwise stuff
will get missed (like now, when there are plenty of comments but I'll skip most
because this is just a partial patch and it's not that fair to require new
thing in review phase). There has to be the moment when the editkeywords page
in its entirety is reviewed.


>Index: editkeywords.cgi

>+            $template->process("admin/keywords/confirm-delete.html.tmpl",
>+              $vars) || ThrowTemplateError($template->error());

Nit: This wrapping bites. Make $vars aligned to "admin", with something like

$template->process("foo",
		   $vars)
  || ThrowTemplateError...



>+++ template/en/default/admin/keywords/add-keyword.html.tmpl

>+<p>Go back to the <a href="query.cgi">query page</a> or <a href="editkeywords.cgi">edit</a> other keywords.</p>

This line exceeds 80 chars. Also, please make the latter link text encompass
the whole phrase "edit other keywords". For this stuff, the rule of thumb is
that link texts should be understandable without context (text browsers can
collect the links and show them separately). Usually that's not worth doing,
but in this case it's so easy...



>+++ template/en/default/admin/keywords/confirm-delete.html.tmpl

I see we've slipped a bit recently on the template documentation part (and even
all of the old stuff may not be well documented anyway), but I see no good
reason for not having an interface comment explaining the contents of the $vars
array. See for example the global/header template for an example of a good if
comment.

If Gerv, who's sort of a template czar here, says the comment is not necessary,
I'll reconsider. Anyway, for now, I believe he just forgot. O:-) I also suggest
you add an interface doc part to templates that don't have variables (just say
"none" or something similar), so that it's easy to tell apart an interfaceless
template from one without documentation.


>+  There are [% bugs %] [%+terms.bugs %] which have this keyword set.

I have a problem with a "bug_count" being called "bugs". I suggest renaming
that variable.

Also, the sentence is grammatically wrong when there's only one bug.

>+  <input type="hidden" name="id" value="[% id %]">

Here, let's go for "keyword_id" or a similar template variable (I don't care
about the form field name) for the sake of clarity.

>+<p>Go back to the <a href="query.cgi">query page</a> or <a href="editkeywords.cgi">edit</a> other keywords.</p>

Wrap and refine link text, as before. :-)



>+++ template/en/default/admin/keywords/rebuild-cache.html.tmpl


>+<a href="editkeywords.cgi">edit</a> more keywords.</p>

And finally, this link text should be "edit more keywords".

Comment 12

[Vlad Dascalu]

Attachment: Version 4

Great comments! This version addresses Jouni's concerns.

I plan to start working on the rest of templatization for editkeywords.cgi as
soon as I get this checked into the CVS.

Comment 13

[Vlad Dascalu]

Attachment: Version 5

(missed one $template ident last time)

Comment 14

[Jouni Heikniemi]

Comment on attachment 135634 [details] [diff] [review]
Version 5

>+++ template/en/default/admin/keywords/confirm-delete.html.tmpl

>+  # bug_count: number. The number of bugs containing specified keyword.

Nit: "number of bugs with the keyword" or something like that; "containing"
sounds like a string search.

>+<p>
>+  There are [% bug_count %]
>+  
>+  [% IF bug_count == 1 %]
>+    [%+terms.bug %]
>+  [% ELSE %]
>+    [%+terms.bugs %]
>+  [% END %]
>+  
>+  which have this keyword set.

It's not just "bug" or "bugs", it's also the verb forms. 

"There is one bug with this keyword set" 
OR
"There are X bugs with this keyword set"

(or "which has/have this keyword set", I don't care :-)

Fix that, and r=jouni.

Comment 15

[Vlad Dascalu]

Attachment: Version 6

Guess we aren't going to make it to no 17 after all :)

Comment 16

[Jouni Heikniemi]

Comment on attachment 135635 [details] [diff] [review]
Version 6

>+    There is one bug with this keyword set.
		   ---
[% terms.bug %] 


Get approval and fix that before you check it in, I'm done with this.

Comment 17

[Jouni Heikniemi]

Comment on attachment 135635 [details] [diff] [review]
Version 6

Nevermind that "ok"; this seems to fail 008filter tests. You'll probably want
to edit filterexceptions.pl to mark the numeric fields as safe and check out
the couple of occasions of html filtering the keyword. 

Sorry for not noticing this earlier :-(

Comment 18

[Vlad Dascalu]

Attachment: Version 7

I forgot about FILTER html, sorry. :)

This is fixed now, and all 9 tests are passed. :)

Comment 19

[Jouni Heikniemi]

I'd prefer to have numeric values marked safe in filterexceptions.pl for
clarity, and then filter only stuff that really needs the filtering (the keyword
itself). However, I can't seem to find a real documentation on what are the real
intentions behind filterexceptions.pl - what should be there and what should
just be filtered? We should probably have rules for this. 

Ccing Gerv for comment.

Comment 20

[Gervase Markham [:gerv]]

The real intention is to avoid cross-site scripting holes :-)

If it's a numeric value, you can either filter it, or mark it safe. Filtering is
preferred if there is any doubt whatsoever that the value may have come from the
user's browser. If it's a direct database read of a numeric field, then you can
mark it safe.

Gerv

Comment 21

[Jouni Heikniemi]

Very well. In that case, I think we should mark bug counts and keyword numeric
ids as safe in filterexceptions rather than filtering them at output. That's
semantically more clear - to me, |123 FILTER html| sounds just like |"123"| - an
overly complex presentation of an integer.

Besides, using filterexceptions is also good for consistency - many (perhaps
most) of the other templates do this, too. 

Comment 22

[Jouni Heikniemi]

Comment on attachment 135637 [details] [diff] [review]
Version 7

I don't think there's going to be any more of this reviewers@ consensus stuff
on the template names. I'd say let's go for what Myk suggested: add-keyword
becomes create, otherwise the names seem good.

Comment 23

[Vlad Dascalu]

Attachment: Version 8

This should do it. :)

Comment 24

[Vlad Dascalu]

Ran runtests.pl on it and it passes all safety-checks.

Comment 25

[Jouni Heikniemi]

Comment on attachment 135833 [details] [diff] [review]
Version 8

r=jouni

Comment 26

[Christian Reis]

I have a question. The delete action used to have a &RebuildCacheWarning call at
the end of it; I don't see one here for that specific action (it's there for
update, though). Was it forgotten, or left out intentionally, or have I not had
enough sleep?

If it's missing, maybe the proper way to do it is implement a BLOCK that you can
add as necessary? 

Comment 27

[Vlad Dascalu]

The RebuildCacheWarning sub was removed in the diff:

-sub RebuildCacheWarning {
-
-    print "<BR><BR><B>You have deleted or modified a keyword. You must rebuild
the keyword cache!<BR></B>";
-    print "You can rebuild the cache using sanitycheck.cgi. On very large
installations of Bugzilla,<BR>";
-    print "This can take several minutes.<BR><BR><B><A
HREF=\"sanitycheck.cgi?rebuildkeywordcache=1\">Rebuild cache</A><BR></B>";
 }


It contained only HTML tags, that have been moved in the rebuild-cache.html.tmpl
template. The template mainly says "keyword updated" / "keyword deleted", but
its main purpose is to notify after that message about the need to rebuild the
keyword cache.

The template rebuild-cache.html.tmpl is called both after an update and after a
delete. I don't see any difference between "update" and "delete" regarding the
call for the rebuild-cache.html.tmpl template.

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 135833 [details] [diff] [review]
Version 8

Looks fine to me.

Gerv

Comment 29

[Vlad Dascalu]

RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/create.html.tmpl,v
done
Checking in template/en/default/admin/keywords/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/create.html.tmpl,v
 <--  create.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/rebuild-cache.html.tmpl,v
done
Checking in template/en/default/admin/keywords/rebuild-cache.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/rebuild-cache.html.tmpl,v
 <--  rebuild-cache.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.12; previous revision: 1.11
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.17; previous revision: 1.16
done

Thanks!

Comment 30

[Christian Reis]

Comment on attachment 135833 [details] [diff] [review]
Version 8

Yeah, I was sleepy. It was pretty hard to find in the diff, too ;)