244265 - Abstract out the typical admin page table to a separate template

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Jouni Heikniemi]

As discussed earlier in editkeywords.cgi templatization (bug 190223 comment 6
and onwards), most of the admin pages have a table structure displaying a list
of something (products, keywords, milestones, whatnot). The table code gets
repeated pretty often, so we could use an abstraction before doing too many
admin pages.

I'll attach a first stab soon. The patch does the following:

- add admin/table.html.tmpl, a generic structure for printing out tables on the
  admin pages. This could also be in global/; do you think it would be useful
  outside the admin domain? Theoretically there's nothing admin-specific here.

- change admin/keywords/list.html.tmpl to use this new table system
  Also fixes a single comment mistake in mentioned template (s/keyword_id/id/).

- Gets rid of the max_table_size. I'm ready to add it if we really need it, but
  it's not as necessary as it was before (more connection speed, better
  browsers), and I don't think I've ever seen an admin page with the splitting
  actually done. It's looks just like unnecessary clutter to me.

Comment 1

[Jouni Heikniemi]

Attachment: v1

Oh, I forgot to mention: this simplifies the output a bit: a missing keyword
description is no longer output as a red "missing". I didn't feel it to be
necessary, and didn't want to stuff the generic table structure with output
formatting just yet.

Comment 2

[Jouni Heikniemi]

Comment on attachment 149027 [details] [diff] [review]
v1

Joel, you promised to review anything templatization related ;-)

Comment 3

[Joel Peshkin]

Comment on attachment 149027 [details] [diff] [review]
v1

r=joel

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 149027 [details] [diff] [review]
v1

Can we use this opportunity to move the "Add" link to somewhere more visible?
It's really hard to discover down in the corner of the table.

I say we remove it from the table entirely, abolish the "footer", and instead
just have a convention about how the Add link should be placed in the base
page. Somewhere near the top, where it's easy to find.

- "literalcontent" seems unnecessarily verbose - just "content" would do IMO.
People can see it's an override.

- could "data" be an array of arrays? Which is easier and more efficient to get
out of the database, I wonder?

- Do table_header and table_footer need to be named blocks rather than just
inline? They are only used once.

- You don't seem to be using the "align" attribute you've put on the "Bugs"
column.

Gerv

Comment 5

[Jouni Heikniemi]

(In reply to comment #4)
> (From update of attachment 149027 [details] [diff] [review])
> Can we use this opportunity to move the "Add" link to somewhere more visible?
> It's really hard to discover down in the corner of the table.

Actually, I disagree. It's not the best of all places perhaps, but I couldn't
come up with a considerably better position for it with quick fiddling. The add
link being below the list perhaps hides it a bit, but I've never heard anyone
_not_ finding it after all. Usually making people see some examples gives them a
reason to think.

Anyway, I'm not opposed to moving the link if someone comes up with a good
proposal. However, I don't think we should hold this technical patch for that. 

> I say we remove it from the table entirely, abolish the "footer", and instead
> just have a convention about how the Add link should be placed in the base
> page. Somewhere near the top, where it's easy to find.

The footer could be useful for several other things, too, such as showing table
counts et al. Otoh, we don't have such an application ATM, and if you think
keeping the link in the "footer" sucks, we can get rid of it. In the patch
coming soon, I've positioned the link outside the table, but left it down below.
This is the same approach as in editflagtypes.cgi.

> - "literalcontent" seems unnecessarily verbose - just "content" would do IMO.
> People can see it's an override.

Done.

> - could "data" be an array of arrays? Which is easier and more efficient to 
> get out of the database, I wonder?

Array of arrays is probably faster, but there are several aspects why it sucks:

1) It creates a relation between visible field order in templates and field
order in SQL SELECT clause - or alternatively creates a column number mapping,
which is a really confusing thing to maintain. 

2) Not all data is pulled from SQL; constructing hashes manually is way better
than constructing lists with magically-order elements.

3) TT's dot operator syntax allows you to substitute objects instead of hashes,
so you could pass it Keyword.pm instances or whatever. 

4) With arrays, the %x% variable interpolation syntax in contentlink would
become _very_ unreadable indeed.

I've left this as it is.

> - Do table_header and table_footer need to be named blocks rather than just
> inline? They are only used once.

Not necessarily. I left them out as named blocks out of old convention. Changed
in the next patch, although I'm not sure whether the named blocks help more than
confuse (sometimes separating a once-called method/sub is also worth it etc.).
But let's go for simplicity this time.

> - You don't seem to be using the "align" attribute you've put on the "Bugs"
> column.

Good catch. Fixed.

Comment 6

[Jouni Heikniemi]

Attachment: v2

Comment 7

[Vlad Dascalu]

Comment on attachment 149183 [details] [diff] [review]
v2

Looks great to me! Nice job!

uri --> URI because it's an abbreviation.

Comment 8

[Jouni Heikniemi]

Attachment: v3

URI fixed in interface comments.

Comment 9

[GavinS]

Should there be something in there which says 'none', for the case when there is
no data?

Comment 10

[Vlad Dascalu]

Comment on attachment 152409 [details] [diff] [review]
v3

r=vladd

I don't object to keeping things simple the way they are now. A header without
any data looks acceptable.

Although I'd prefer to see a message like "No data found." or something like
that when the table contains no rows, like glob suggested.

Comment 11

[Vlad Dascalu]

I mean GavinS :)

Comment 12

[GavinS]

This is what I have in my local copy for the 'no data found' case, in case it can be squeezed into this 
patch. If not, I'll include it in my upcoming patch for editcomponnents templatisation...

[% IF data.size == 0 %]
  <tr><td colspan="[% columns.size %]" align="center"><i>&lt;none&gt;</i></td></tr>
[% END %]

Comment 13

[Jouni Heikniemi]

(In reply to comment #12)
> This is what I have in my local copy for the 'no data found' case, in 
> case it can be squeezed into this patch. If not, I'll include it in my 
> upcoming patch for editcomponnents templatisation...

Merits of this were discussed on IRC. I agree the current solution of just
showing the table headers isn't perhaps the best one, but I don't think adding
anything semantically empty "none" style stuff is going to help much.

When it's necessary to show a clear message about lack of entries, we might also
IF-branch in the calling template - I'm not certain on the merits this generic
table structure can offer wrt that need. Gavin, when you're implementing
something like this, please CC me so we can discuss it more. 

For now, I'm requesting approval to get this in. I have no particular desire to
push this for 2.18, so trunk-only approval is fine for me if we can branch soon.

Comment 14

[Jouni Heikniemi]

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.17; previous revision: 1.16
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.5; previous revision: 1.4
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/table.html.tmpl,v
done
Checking in template/en/default/admin/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/table.html.tmpl,v 
<--  table.html.tmpl
initial revision: 1.1
done