190226 - templatize editversions.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Dave Miller [:justdave]]

templatize editversions.cgi

Comment 1

[Jack Nerad]

Attachment: Revisions to edtiversions.cgi, and new templates.

A humble offering to the might bugzilla team.

Comment 2

[Dave Miller [:justdave]]

jeez, I'm gonna need to post mine here before you get too far along so we get
common ground on the coding style and everything at least...  :-)

The style I used for mine is blatently ripped off from editflagtypes.cgi and
attachment.cgi

Comment 3

[Jack Nerad]

Is it just a convention that the error "tags" for the user-error.tmpl.html use
lowercase, or is there some reason to pass the values to the template that way?

Comment 4

[Dave Miller [:justdave]]

Just a convention as far as I know...  Gerv?

Comment 5

[Gervase Markham [:gerv]]

It's a convention - underscore_separated_lower_case. Please don't break it :-)

Gerv

Comment 6

[Jack Nerad]

Attachment: Style Revisions

- attempted to copy style found in editflagtypes.cgi
- changed template/en/default/global/user-error.html.tmpl to reflect errors in
editversions.cgi
- modify FILTER directives from | to FILTER in templates

Comment 7

[Gervase Markham [:gerv]]

So here's the thing.

I'm sorry I haven't managed to comment on all this before we got this far,
because it might have saved someone some effort. 

The administration of Bugzilla often consists of doing the same things a lot on
different data sets. For example, compare the front page of the different edit*
cgis - basically, the same UI but different data. I think it's important that we
do the templatisation in a generic enough way that we don't end up with a load
of templates which are all very similar. Not only is that scenario a maintenance
nightmare (in terms of fixing bugs), but it makes it hard to even keep
everything consistent. It's also a greater localisation burden.

jnerad: I'm very pleased that you've taken up the torch here, because this is
something that has needed doing for a while but hasn't made it to the top of
anyone's list. But what I'd like to see is a survey of what all the admin CGIs
are trying to achieve, and discussion on how we can make that happen with as
much commonality as possible.

Have you been thinking about this much, or just taking each CGI as it comes?

Gerv

Comment 8

[Jack Nerad]

I've just been taking it one cgi at a time.  I'm really more about horsepower
than I am about elegance, as I'm sure you can tell, just trying not to step on
anybody's toes while contributing something.  

I've got an idea, but it could mean a _lot_ of work architecturally.

The trouble I run into with the bugzilla system is that all of the localization
is in the templates.  That's better than it being in the scripts, for sure, but
it seems like it should be somewhere else... like a configuration file.  You can
see the problem emerging in the user-error.tmpl, which is one huge if statement.
I can see it becoming a problem to maintain in the future (how many error
messages are there?). 

To abstract the templates involved, one could have keys and values while keeping
a single template, switching inside the template depending on what kind of
output you need. PHPNuke does something very similar.

e.g.

[% IF two_by_table %]
  <table>
    [% FOREACH message %]
      <tr>
        <td>[% label %]</td>
        <td>[% value %]</td>
      </tr>
    [% END %]
  </table>
[% END %]

then when message is packed in the script, it could get the label from the
config file.

$message->{label} = $config->{object}{version}{label};
$message->{value} = (db lookup pseudo code);
push @{$vars->{message}}, $message;

Then you could leave all your localization to the config files without ever
having to touch either the scripts or the code.  You could use a setup script to
 convert XML to a Storable.pm object, so to represent the above:
<?xml version="1.0" ?>
<config>
 <object name="version">
   <label>Version</label>
   <field name="Version">
     <type>text</type>
     <length>10</length>
     <maxlength>100</length>
   </field>
 </object>
</config>   

Then to customize it for Spanish, just change <label>Version</label> to
<label>Versión</label>, and rerun the setup script to create the serial object
(quicker than parsing XML each time).   

If that kind of generalization isn't what you had in mind, you are going to have
to school me on what 'survey' means.  I'm just a dumb English major.

Each of the screens in the admin scripts that I've come across could be handled
this way.  The editproducts.cgi is the toughest to do because it can link to
other scripts; however, those links can be handled this way, too: it just
complicates things a tad.

Comment 9

[Gervase Markham [:gerv]]

That's not really the sort of generalisation I was talking about :-)

We considered several solutions for localisation, balancing all the different
and competing requirements, and settled on keeping the text strings in the
templates. A solution like the one you propose was considered, but there are no
plans to change the route we've gone down.

What I mean is that, for example, if you look at editproducts and editkeywords,
you can see that you basically display some data in some columns, and give
operations to perform on it. I'd rather have one data display template than
five, all of which are copied and pasted from each other with a few tweaks.
There are other examples throughout admin of bits of UI which are very similar.

This does obviously mean that you have a containing template which defines the
specific details, and this will involve text strings passed in as variables. But
we aren't going to a full-blown text substitution system.

Gerv

Comment 10

[Jack Nerad]

No problem.

I think I understand what you mean.  Let's see if I do. 

Both editversions.cgi and editproducts.cgi have a function that lists all the
products and some information about them. You want a single template to produce
both tables.

Both editversions.cgi and editproducts.cgi have a function that displays data
about them prior to delete confirmation.  You want a single template to display
both sets of data.

Is that the right idea?

Comment 11

[Dave Miller [:justdave]]

Another thing I would encourage (I admit I haven't looked at your existing
patches yet) is to make use of the user-message architecture
($vars->{'message'}) to tell the user what they just did at the top of the item
list when displaying it again.

In other words, when I create a new version, the confirmation message saying
"version was added" instead of being on a separate page by itself like it is
now, should be in the message box at the top of the version list, and the
version list template should be displayed again.

Comment 12

[Jack Nerad]

Did I get the idea of generalization about right?  Shall I proceed under that
understanding?

Comment 13

[Dave Miller [:justdave]]

Yeah, that sounds like the general idea.

Maybe we should have a single "metadata" template for each editor (sort of like
fields-defs.html.tmpl, for example) that describes the basics for each editor
(what columns are displayed in the table, map the variable names to the row
arrays, etc), and the the templates that do the dirty work can all include the
one for the editor they get called from to get their setup details.

Or maybe I'm just blowing smoke :)

Comment 14

[Gervase Markham [:gerv]]

I think dave is on the right track. My point is that we need to think about this
more generally, and not just do it CGI by CGI.

Gerv

Comment 15

[Jack Nerad]

If editcomponents.cgi is called without parameters, it gives back a two column
table that has "edit milestone of," and "description."  

If editmilestones.cgi or editversions.cgi is called without parameters, it gives
back a three column table that has "edit version of," "description," and "Bugs"
(which appears to be "xyzzy" for all products).

If editproducts.cgi is called without parameters, it gives back an 8 column
table that has "edit product," "Description," "Status," "Votes per user," "Max
votes per bug," "votes to confirm," "bugs," (which appears to be an accurate
count) and "action" (which is apparently only delete).

If editkeywords.cgi is called without parameters, it gives back a 4 column table
that has columns "Edit keyword," "description," "bugs," and "action" (which
appears to be only "delete" for all keywords).

Is that all the scripts that deal with the "components" of a project or am I
missing some?

Would it be okay to settle on a single "no parameters" screen for each of these,
or is it necessary to the usability of the screens that they present different
information?

Comment 16

[Jack Nerad]

Still waiting on guidance.

Comment 17

[Vlad Dascalu]

Can we move along with this please?

I'd hate to see this as just another thing that was forgotten due to lack of
time in the b.m.org jungle :-)

Comment 18

[Jack Nerad]

Still waiting on guidance.

Comment 19

[Dave Miller [:justdave]]

oof.  Sorry, I'm just now catching up on about 2 months worth of bugmail. :)  I
have this flagged on my personal list, I'll try to come back to it this week.

Comment 20

[GavinS]

Comment on attachment 113159 [details] [diff] [review]
Style Revisions

[not an official review in any way, but:]

I think that more filtering is needed in some parts of the new templates.

e.g. product and description in list.html.tmpl, and version in select.html.tmpl
are displayed without any 'FILTER html'

Comment 21

[Christian Reis]

Comment on attachment 113159 [details] [diff] [review]
Style Revisions


Let's get on with this :-) As an initial note, justdave pointed this out to me:

<justdave_> a lot of our edit*.cgi stuff is editing different tables that look
alike.
<justdave_> a lot of them could be considated into a single cgi that takes
which table to edit as a parameter

This definitely applies to editversions, and since it's the first edit*.cgi
file being templatized, take care that it's done in a way that it's easy to
reuse the table template part.

>Index: editversions.cgi
> my $version = trim($::FORM{version} || '');
> my $action  = trim($::FORM{action}  || '');

Silly question: do these need to be detainted?

>-# product = '' -> Show nice list of versions
>+# product  = '' -> Show nice list of versions

(nit: not necessary, right?)

>+elsif (! $product         )   { no_product();      }
>+elsif (! $action          )   { no_action();       }

These functions should really be named to indicate what they do -- I thought
initially they were error functions. Maybe list_products() / select_versions()?

>+    print "Content-type: text/html\n\n";

Hmmm, this reminds me. This code should be updated to use Bugzila::CGI.

>+++ template/en/default/admin/versions/list.html.tmpl	Thu Jan 
>+  # Copyright (C) 1998 Netscape Communications Corporation. 
All
>+  # Rights Reserved.

Let's ask Gerv what the initial copyright assignee should be -- the consensus
on #mozwebtools seems to be that it should be you, though I'm not sure because
templates contain code that came from the original cgis.

>+          <a href="editversions.cgi?product=[% product FILTER uri 
>+                                               FILTER html %]">[% product %]</a>

FILTER html

>+          [%- IF description -%]
>+            [% description %]

same

>+          [%- IF bugs -%]
>+            [% bugs %]

Unsure -- same here?

>+++ template/en/default/admin/versions/select.html.tmpl	Thu Jan 30 20:57:03 2003

>+    [%- FOREACH v = all_version %]
>+      [% v_quote = v FILTER uri FILTER html %]
>+      <tr>
>+        <td valign="top"><a 
>+        href="editversions.cgi?product=[% prod_quote %]&version=[% v_quote %]&action=edit">
>+        <b>[% v %]</b></a>

Shouldn't it be v_quote here too?

I may have missed other FILTERs, be sure to check.

The code looks nice, thanks a lot!

Comment 22

[Christian Reis]

Gerv, see my copyright remark in the previous message? Thanks!

Comment 23

[chaduv]

> my $version = trim($::FORM{version} || '');
> my $action  = trim($::FORM{action}  || '');

>+elsif (! $product         )   { no_product();      }
>+elsif (! $action          )   { no_action();       }

I like what was done in editflag... force action to list if it wasn't passed, 
and define a 'list' sub rather than a no_action sub. It also allows you to call 
it explicitly (you don't allow for passing in no_action as the action arg :).

Looks good in general, mod bits requested by kiko.

Comment 24

[Gervase Markham [:gerv]]

If it contains significant amounts of code from other files, then the initial
developer should probably be the person from those files - which would probably
be Netscape. If it's basically all your own work, it's you.

The Original Code is usually "The Bugzilla Bug-Tracking System" (or some words
like that; check some source files) for Bugzilla.

Gerv

Comment 25

[Dave Miller [:justdave]]

moving all enhancements without current patches to 2.20

Comment 26

[Jouni Heikniemi]

All admin-page templatization bugs: Note that just-fixed bug 244265 implemented
a basic generic structure for writing out those typical
product/milestone/whatever selection tables that exist on most of the admin
pages. If you templatize one of those, please use the generic version. If it
doesn't have all the features you need, let's discuss expanding it with new
features.

Comment 27

[GavinS]

Attachment: templatise editversions.cgi v1

Comment 28

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 29

[Jouni Heikniemi]

Comment on attachment 159312 [details] [diff] [review]
templatise editversions.cgi v1

Gavin: First, sorry for being so slow. Second, I'm really impressed. I did find
the nit below on the last stretch, but all in all, a templatization patch with
just one comment nit is something really special - even for something as simple
as editversions. Great work! 

r=jouni

>+  # 'updated_XXX' variables are booleans, and are defined if the
>+  # 'XXX' field was updated during the edit just being handled.
>+  # Variables called just 'XXX' are strings, and are the _new_ contents
>+  # of the fields.
>+  # name & updated_name: the name of the version

This is a bit confusing since there's only one field; you could just explain
that specifically for those two variables :)

Comment 30

[GavinS]

Attachment: templatize editversions.cgi v2

Nit fixed. Thanks for the review.

Comment 31

[GavinS]

Any chance of approval for this????

Comment 32

[Dave Miller [:justdave]]

(In reply to comment #31)
> Any chance of approval for this????

This is being held for the creation of the bugzilla2.20 branch so it can be
checked in on the trunk after we branch.  That should happen within the next
week at the latest.

Comment 33

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 34

[Vlad Dascalu]

Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.29; previous revision: 1.28
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.82; previous revision: 1.81
done

RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/confirm-delete.html.tmpl,v
done
Checking in template/en/default/admin/versions/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/create.html.tmpl,v
done
Checking in template/en/default/admin/versions/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/create.html.tmpl,v
 <--  create.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/created.html.tmpl,v
done
Checking in template/en/default/admin/versions/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/created.html.tmpl,v
 <--  created.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/deleted.html.tmpl,v
done
Checking in template/en/default/admin/versions/deleted.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/deleted.html.tmpl,v
 <--  deleted.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/edit.html.tmpl,v
done
Checking in template/en/default/admin/versions/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/edit.html.tmpl,v
 <--  edit.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/footer.html.tmpl,v
done
Checking in template/en/default/admin/versions/footer.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/footer.html.tmpl,v
 <--  footer.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/list.html.tmpl,v
done
Checking in template/en/default/admin/versions/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/list.html.tmpl,v
 <--  list.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/select-product.html.tmpl,v
done
Checking in template/en/default/admin/versions/select-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/select-product.html.tmpl,v
 <--  select-product.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/updated.html.tmpl,v
done
Checking in template/en/default/admin/versions/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/updated.html.tmpl,v
 <--  updated.html.tmpl
initial revision: 1.1
done