bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Templatise GetCommandMenu

RESOLVED FIXED in Bugzilla 2.16



16 years ago
5 years ago


(Reporter: gerv, Assigned: gerv)


Bugzilla 2.16
Dependency tree / graph



(1 attachment, 2 obsolete attachments)

GetCommandMenu is, via %commandmenu%, included in Param('footerhtml'), which is
itself called from the footer.html.tmpl script.


Fixing this mess properly is somewhat complicated. But templatising the
GetCommandMenu function will, at least, allow localisers to DTRT.

This is the _last_ templatisation bug I know about - I swear :-)

Target Milestone: --- → Bugzilla 2.16
I've got this half done, but it's now bed time :-)

Assignee: justdave → gerv
*** Bug 140320 has been marked as a duplicate of this bug. ***
We need to continue to support both templatised footers and non-templatised
footers (I assume some of the latter are left).  I think the footer should short
circuit the current path and use the command menu template directly rather than
using the GetCommandMenu path which should eventually die.
Priority: -- → P5
Changing this would involve:

- Templatising the footer properly, including the command menu stuff
- Changing PutFooter to use the new footer template
- removing the DB calls from GetCommandMenu, and putting them either in a sub
which is passed in to the footer, or in some always-executed code.
- Removing the footer param

I suppose that's not too much more work. I need to do some more investigation here.


Nearly there... :-) My current plan (please criticise) is to get
quietly_check_login to call a sub GetUserInfo, which returns a data structure of
info about the user - their groups, stored queries etc. This gets put into
$vars->{'user'}. So, all templates, including the footer, can get this info
without asking for it again.

This means that (either soon or later), we can optimise a lot of instances where
we currently get the same information twice in a single CGI - e.g. for the body
and the footer.

Blocks: 140781

Comment 6

16 years ago
This means that if the user info gets more complex over time, we will do
unnecessary work in getting it from the database, even if only few of it is used
in the footer template. But this is the general drawback of the 'push' model we
are using: We need to supply all the data to the template even if it doesn't
need it.
> This means that if the user info gets more complex over time, we will do
> unnecessary work in getting it from the database, even if only few of it is used
> in the footer template. 

Not necessarily; we can trade off whether we want to get it every time, or just
get it for the templates which need it (which is an extra DB access).

But bear in mind, at the moment, we must read the user's entry in the profiles
table a load of times for a given CGI - and every time we call UserInGroup, we
are doing a DB call. Getting all this stuff at the beginning will definitely be
a win, when we convert to using it rather than asking the DB again.

There are a lot of fixes that we could and should be making, but what's the
simplest and lowest-risk fix that would stop this bug from blocking the release?
 At this point in the development cycle no other fix should go in.  It sounds
like that fix is the templatization of GetCommandMenu, so that's what this bug
should be about.  The rest of it, while laudable, is 2.18 material.
The actual code is the same - it's just a question of where we put it. I'll
produce the patch and you can tell me what you think.

Created attachment 81563 [details] [diff] [review]
Patch v.1

This patch templatises GetCommandMenu inside footerhtml. It makes the code to
set up the relevant info about the user be called from quietly_check_login,
which seems a sensible place. It makes a couple of changes to keep the footer
consistent when the info in it changes after quietly_check_login is called.

In passing, it fixes 
+[% CALL SyncAnyPendingShadowChanges() IF SyncAnyPendingShadowChanges %]
and also adds comments to footer.html.tmpl to help admins migrating from a
Param-based system.

Blocks: 140437
Comment on attachment 81563 [details] [diff] [review]
Patch v.1

>Index: CGI.pl

>+    my ($userid) = (@_);

Nit: unnecessary parentheses.

>Index: globals.pl

>+    $vars->{'use_votes'} = $::anyvotesallowed;

You've put this in GenerateVersionTable, which only runs once an hour
instead of every time.	Also, you don't use it in footer.html.tmpl,
preferring votesallowed instead (which never gets defined).  Note that
sidebar.cgi defines anyvotesallowed, while bug_form.pl uses use_votes.

>Index: template/en/default/global/footer.html.tmpl

>+[%# Migration note: this whole file corresponds to the old Param 'footerhtml' %]

That param needs to remain in order for non-user-facing scripts to use it.
Does this mean installations will need to customize both the param and the
template for the change to happen across all scripts?  I'm not necessarily
opposed to this, but we will need to spell this out if it's what we're doing.

Also, this code should be broken up into multiple templates just as it was
in params.  That makes it much easier to customize with minimal forking.

Other than that, the code looks fine, but it doesn't work in all cases.  
When I am logged in, and I run a query, the command menu on buglist.cgi 
looks like the menu of a non-logged-in user.  When I click the "log in" 
button on that page, however, I go back to a query page with a logged-in 
user command menu.
Attachment #81563 - Flags: review-
I'm not convinced we really need to templatize the "template" parameters in the
first place.  It helps with localization, but it makes installations customize
those values in two places (once in the template parameters, once in the
template files) if they want to change all scripts.  If we do want to do it,
however, we should do it for the template parameters in header.html.tmpl as well.
Hmm, I see that PutHeader and PutFooter now process the header.html.tmpl and
footer.html.tmpl templates, respectively, so making installations customize the
header and footer in two different places is not a problem.  Ok, so the only
problem is that the parameters in header.html.tmpl should also be templatized if
the ones in footer.html.tmpl are.
Also, I get the following error running checksetup.pl after applying the patch:

Variable "$vars" is not imported at globals.pl line 489 (#1)
    (F) While "use strict" in effect, you referred to a global variable that
    you apparently thought was imported from another module, because
    something else of the same name (usually a subroutine) is exported by
    that module.  It usually means you put the wrong funny character on the
    front of your variable.

Uncaught exception from user code:
        Uncaught exception from user code:
        Global symbol "$vars" requires explicit package name at globals.pl line 489.
BEGIN not safe after errors--compilation aborted at globals.pl line 630.
        require globals.pl called at ./checksetup.pl line 558
Compilation failed in require at ./checksetup.pl line 558.
Created attachment 81997 [details] [diff] [review]
Patch v.1

Nice review :-) All issues addressed, except:

> Note that
> sidebar.cgi defines anyvotesallowed, while bug_form.pl uses use_votes.

Indeed. Our template APIs have many duplications and inconsistencies. I hope to
be able to clean them up soon; either before 2.16 or after. Obviously, before
is preferable, but we can't hold the release for it.

> Other than that, the code looks fine, but it doesn't work in all cases.  

This was due to a namespace clash on $vars->{'user'}. This patch fixes both
instances of this.

> Also, this code should be broken up into multiple templates just as it was
> in params.  That makes it much easier to customize with minimal forking.

I'm not convinced that there's much we can do here. We could put the command
menu in a separate template - but that wasn't customisable before anyway. I
think we'll find that people customise the footer and header so much that
there's not much constant stuff we can abstract out, and we just risk annoying
people by making them maintain two templates instead of one.

Attachment #81563 - Attachment is obsolete: true
Keywords: patch, review
Comment on attachment 81997 [details] [diff] [review]
Patch v.1

>+# Populate a hash with information about this user. 
>+sub GetUserInfo {
>+    my $userid = (@_);

>+    SendSQL("select name, (bit & $::usergroupset) != 0 from groups");

You can't take a $userid in, but use $::usergroupset. Either just use $::user
directly, or grab
the groupset in the previous query on the profiles table.

/me mutters a bit about the mini race condition here. Its harmless though, so
don't bother with

>Index: globals.pl
>RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
>retrieving revision 1.165
>diff -u -r1.165 globals.pl
>--- globals.pl	29 Apr 2002 19:37:52 -0000	1.165
>+++ globals.pl	2 May 2002 07:28:55 -0000

>@@ -1571,6 +1570,13 @@
>         # filter should be used for a full URL that may have
>         # characters that need encoding.
>         url_quote => \&url_quote ,
>+        no_break => sub

You need a comment here describing this filter. You also need to add a dummy
filter to checksetup
and the template tests

>+        {
>+            my ($var) = @_;            
>+            $var =~ s/ /\&nbsp;/g;

Do you need a /m, too ? 

>@@ -1753,6 +1759,8 @@
>     # User Agent - useful for detecting in templates
>     'user_agent' => $ENV{'HTTP_USER_AGENT'} ,
>+    'use_votes' => $::anyvotesallowed,

Sigh. This has to be a global var, doesn't it?

>Index: buglist.cgi
>RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v
>retrieving revision 1.166
>diff -u -r1.166 buglist.cgi
>--- buglist.cgi	26 Apr 2002 18:17:04 -0000	1.166
>+++ buglist.cgi	2 May 2002 07:28:57 -0000
>@@ -1124,6 +1124,7 @@
>         $vars->{'title'} = "OK, query saved.";
>         $vars->{'message'} = "OK, you have a new query named <code>$name</code>";
>         $vars->{'url'} = "query.cgi";
>+        push(@{$vars->{'user'}{'queries'}}, {name => $name});

What if I'm replacing an existing query, by reusing the name? Won't this cause
it to appear twice?

>         $vars->{'link'} = "Go back to the query page.";
>         $template->process("global/message.html.tmpl", $vars)
>           || ThrowTemplateError($template->error());

>Index: template/en/default/global/footer.html.tmpl
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/footer.html.tmpl,v
>retrieving revision 1.4
>diff -u -r1.4 footer.html.tmpl
>--- template/en/default/global/footer.html.tmpl	25 Apr 2002 22:35:14 -0000	1.4
>+++ template/en/default/global/footer.html.tmpl	2 May 2002 07:28:58 -0000

>+[%# Migration note: this section corresponds to %commandmenu% in 'footerhtml' %]

I agree with myk - this should be a separate template. Some people have it in
the header, and theres
no need for them to have to copy and paste it. Theres no styling, just a table,
so it could probably
be reused fairly easily.

>-[% CALL SyncAnyPendingShadowChanges() %]
>+[% CALL SyncAnyPendingShadowChanges() IF SyncAnyPendingShadowChanges %]

hmm. Does this work? Where do you push this to the templates?
Attachment #81997 - Flags: review-
Oh, and one other note - while you're doing this, can you please fix the bug
where the "Preset queries" header appears even if you have no preset quries, and
the my bugs link is off? It should be one more if statement with the template.
Created attachment 82557 [details] [diff] [review]
Patch v.2

All review comments addressed, except:

> >+	    {
> >+		my ($var) = @_; 	   
> >+		$var =~ s/ /\&nbsp;/g;
> Do you need a /m, too ? 

"Let ^ and $ match next to embedded \n"? Don't think so...

> >-[% CALL SyncAnyPendingShadowChanges() %]
> >+[% CALL SyncAnyPendingShadowChanges() IF SyncAnyPendingShadowChanges %]
> hmm. Does this work? Where do you push this to the templates?

Yes. Line 1760 of globals.pl, in the global template definition :-)

Attachment #81997 - Attachment is obsolete: true
Comment on attachment 82557 [details] [diff] [review]
Patch v.2

r= justdave

thought I have to wonder if it might be useful to leave blurbhtml as a real
param instead of in the template since it's frequently used for administrative
Attachment #82557 - Flags: review+
Comment on attachment 82557 [details] [diff] [review]
Patch v.2


Whilst it would be nice to be user-editable, anyone with editparams putting up
administrative notices probably has shell access to the box anyway. If we get
bugs, we can re-add it, I suppose.
Attachment #82557 - Flags: review+
Blocks: 141639
No longer blocks: 141639

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.152; previous revision: 1.151
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.167; previous revision: 1.166
Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.17; previous revision: 1.16
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.168; previous revision: 1.167
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.3; previous revision: 1.2
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.143; previous revision: 1.142
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.35; previous revision: 1.34
Checking in template/en/default/global/footer.html.tmpl;
 <--  footer.html.tmpl
new revision: 1.5; previous revision: 1.4
RCS file:
Checking in template/en/default/global/useful-links.html.tmpl;
 <--  useful-links.html.tmpl
initial revision: 1.1
Checking in template/en/default/bug/votes/list-for-user.html.tmpl;
 <--  list-for-user.html.tmpl
new revision: 1.6; previous revision: 1.5
Checking in template/en/default/list/edit-multiple.html.tmpl;
 <--  edit-multiple.html.tmpl
new revision: 1.4; previous revision: 1.3
Checking in t/004template.t;
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v  <--  004template.t
new revision: 1.14; previous revision: 1.13

Last Resolved: 16 years ago
Resolution: --- → FIXED
the tree is burning...
checked in the missing comma on line 844 of checksetup.pl
<cough> Er... it's not burning any more :-) Both bustages fixed.

QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.