140435 - Templatise GetCommandMenu

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P5)

Description

[Gervase Markham [:gerv]]

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

<sigh>.

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 :-)

Gerv

Comment 1

[Gervase Markham [:gerv]]

I've got this half done, but it's now bed time :-)

Gerv

Comment 2

[Matthew Tuck [:CodeMachine]]

*** Bug 140320 has been marked as a duplicate of this bug. ***

Comment 3

[Matthew Tuck [:CodeMachine]]

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.

Comment 4

[Gervase Markham [:gerv]]

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.

Gerv

Comment 5

[Gervase Markham [:gerv]]

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.

Gerv

Comment 6

[Andreas Franke (gone)]

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.

Comment 7

[Gervase Markham [:gerv]]

> 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.

Gerv

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 9

[Gervase Markham [:gerv]]

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.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Attachment: 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.

Gerv

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 15

[Gervase Markham [:gerv]]

Attachment: 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.

Gerv

Comment 16

[Bradley Baetz (:bbaetz)]

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
locking.

>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/ /\ /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?

Comment 17

[Bradley Baetz (:bbaetz)]

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.

Comment 18

[Gervase Markham [:gerv]]

Attachment: Patch v.2

All review comments addressed, except:

> >+	    {
> >+		my ($var) = @_; 	   
> >+		$var =~ s/ /\ /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 :-)

Gerv

Comment 19

[Dave Miller [:justdave]]

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
notices.

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 82557 [details] [diff] [review]
Patch v.2

r=bbaetz.

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.

Comment 21

[Gervase Markham [:gerv]]

Fixed.

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

Gerv

Comment 22

[Dave Miller [:justdave]]

the tree is burning...

Comment 23

[Dave Miller [:justdave]]

checked in the missing comma on line 844 of checksetup.pl

Comment 24

[Gervase Markham [:gerv]]

<cough> Er... it's not burning any more :-) Both bustages fixed.

Gerv