190196 - editproducts.cgi needs templatizing

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Jack Nerad]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212

There's a whole lot of presentation layer stuff that should be in a template
rather than in a script.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Dave Miller [:justdave]]

In what?  Most of Bugzilla is already templatized, and there's already
individual bugs for the files that haven't been yet (which is just the admin
stuff at this point)

Comment 2

[Jack Nerad]

In 2.17.3.  My version of editproducts.cgi is definitely NOT templatized.

Comment 3

[Dave Miller [:justdave]]

and believe it or not, I can't find a specific bug for that particular file
(while there are for most of the others).  editproducts.cgi does make use of two
templates, but they were added features, and there weren't templates put in for
the existing stuff.  Might as well make this one it.

Comment 4

[Dave Miller [:justdave]]

confirming, accepting (I'm already working on some of this stuff anyway)

Comment 5

[Jack Nerad]

I've got some work done for this already, too, though I don't know where to put
the template files or what to name them.

Comment 6

[Dave Miller [:justdave]]

There's existing ones for editflagtypes.cgi that would be a good basis to follow...

I think they're in template/en/default/admin/flagtypes
create, edit, update, delete, and so forth.  There should already be a folder
within default/admin for products, because it already does have a couple
templates (for the group control-related stuff).

I haven't actually done anything to editproducts yet, but I have some of the
simple ones like editcomponents and editversions in progress, so if you have
stuff already for editproducts, feel free to post it :)

Comment 7

[Jack Nerad]

Sorry for all the questions... In the developers docs, it says to use 
$template->process("template.html.tmpl",$vars) ||
ThrowTemplateException($template->error)  

But I haven't been able to locate that method.  Where didn't I look?

Comment 8

[Dave Miller [:justdave]]

it's not in Bugzilla itself, that's imported by "use Template;"

Comment 9

[Jack Nerad]

Attachment: The templates for add/edit/delete products

gzipped tar file.  untar/unzip in bugzilla directory.  It puts the templates
add-edit.html.tmpl  delete-confirm.html.tmpl  list.html.tmpl
create.html.tmpl    deleted.html.tmpl into the
templates/en/default/admin/products directory of the bugzilla installation.

Comment 10

[Jack Nerad]

Attachment: changes to editproducts.cgi to get add/edit/delete working with Template

This is the counterpart to the templates (attachment 112382 [details])

Comment 11

[Jack Nerad]

Okay.  So there's my first stab at add/edit/delete editproducts.cgi.  I'm still
new to submitting patch sets, and I usually work alone, so I really appreciate
any hints (or curses :) about how to submit patches or which protocols to follow
when making changes like these.

Comment 12

[Jack Nerad]

Attachment: The templates for add/edit/delete products (gzipped tarball)

- passes the no-tabs test in runtests.sh
- correct default values in add

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 112401 [details]
The templates for add/edit/delete products (gzipped tarball)

Once you get a patch up, click Edit next to the attachment and set the "review"
box to "?" to request review on it.  If you're not sure who to give it to, just
leave the requestee box blank.	I'll fill in Gerv this time because we usually
give him the first crack at template reviews. :)  Fair warning: his reviews can
be harsh, but it's a good learning experience and we usually wind up with
better code as a result :)

Comment 14

[Dave Miller [:justdave]]

Joel, CCing you since you added the two existing templates to this file :)

Comment 15

[Tobias Burnus]

Comment on attachment 112401 [details]
The templates for add/edit/delete products (gzipped tarball)

(I had liked if the patch and the new files were in one single attachment and
especially not in a tar.gz format which makes it hard in the browser to
(re)view. You can do a cvs diff -u > foo and then a diff -u /dev/null newfile
>> foo to achieve that.)

Some remarks:
+    $vars->{title} = "Select Product";
+	 $status = $status ? 'closed' : 'open';
Those things really belong into the template, otherwise they cannot be
localized/customized.

+    $vars->{usetargetmilestone} = Param("usetargetmilestone");
You can use Param("usetargetmilestone") in the template directly.

You should also templatize those parts of editproducts if you are at it:
    unless ($prod) {
	print "Sorry, you haven't specified a product.";
	PutTrailer();
	exit;
You should change this to use ThrowUserError (or ThrowCodeError where
appropiate)
  sub EmitFormElements ($$$$$$$$)
This the output of this function should also be moved into a template.

	     <a href="editproducts.cgi?action=editgroupcontrols&product=[%
produ
Change the & into &amp;

Comment 16

[Jack Nerad]

I didn't know that trick about /dev/null.  That's a good one.  Thanks!

I'll do what you suggest with the ternary operator.  The title changes according
to how the script is called, and I reuse one of the templates.  Should I break
it out into two templates instead?

The EmitFormElements is just waiting to die.  All of it is in the templates.

Thanks for the review.

Comment 17

[Gervase Markham [:gerv]]

Review? What Tobias gave you wasn't a review, that was a quick glance. Just you
wait until I get my hands on your patch, young feller-me-lad. I'll show you what
a real review is.

Seriously... thanks for the patch :-) But as the man says, cvs diff -u is best,
with diffs against /dev/null appended. If you could provide one of those, I'll
get down to the review. 

Gerv

Comment 18

[Jack Nerad]

Attachment: editproducts.cgi templatization patch rev2

- Eliminated ternary operator in script in favor of IF statements in templates
- used Param('usetargetmilestone') in templates instead of in script
- Templatized some more of the script (still working on getting everything out)


I still haven't figured out the best way to set the title in a dual use
template.  Can somebody give me a clue there?

Comment 19

[Jack Nerad]

Attachment: editproducts.cgi templatization patch

- Fixes table problem in edit
- Fixes problem in delete confirmation

Comment 20

[Joel Peshkin]

Attachment 112858 [details] [diff] looks like it is only the template files and was done from one
of the template directories...

Try this....

from the top bugzilla directory (the one with checksetup.pl in it).....

cvs diff -u > patch.112858
diff -u /dev/null template/en/default/admin/product/add.html.tmpl >> patch.112858
diff -u /dev/null template/en/default/admin/product/del.html.tmpl >> patch.112858
etc....

The, the patch will apply properly.

Comment 21

[Jack Nerad]

Attachment: editproducts.cgi templatization patch (rev 4)

Sorry for all the spam.  I'll get it right one day.

Comment 22

[Gervase Markham [:gerv]]

jnerad: I'm sorry I haven't got to this review yet. It certainly won't be before
the weekend (I'm speaking at http://www.fosdem.org) and then 124174 is in the
queue first. Thanks for all your hard work; but, on the off-chance that I have,
er, extensive comments, you might want to hold off on templatising anything else
until one of your patches has been reviewed. :-)

Dave, myk - do either of you have time to look at this?

Gerv

Comment 23

[Jack Nerad]

Actually, if you wouldn't mind, I've submitted a patch for editversions.cgi that
I think is closer to what the project is looking for than this patch set.  It is
a smaller set, so easier for me to learn on.  I'd rather have that reviewed and
then just redo this one according to the feedback I get on that one. (bug#
190226).  It has the added benefit of being less work for the reviewer
(initially ;).

Comment 24

[Dave Miller [:justdave]]

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

Comment 25

[Oscar Manuel Gómez Senovilla]

Is supposed to be something left to do to get editproducts.cgi fully templatised?

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

[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 28

[Tiago Mello [:timello]]

Attachment: v1: add product templatizing.

I decided to split the patches to be easy to review. Would be wonderful if we
can land these templatizing as soon as possible.

Comment 29

[Christian Reis]

Tiago, if you are indeed proposing to split patches, you need to split this into
separate bugs and attach patches there. Also note the existence of bug 289580,
which is a part of what you need -- perhaps review that too for a start?

Comment 30

[Vlad Dascalu]

Comment on attachment 183053 [details] [diff] [review]
v1: add product templatizing.

This comes very close I think. Probably ready to go as soon as we reopen the
trunk, assuming the other parts are ready too :)

Unfortunately this introduces invalid HTML as output. In several places, in the
templates, & is used without having a valid follow-up reference to it.
Therefore you're attempting to encode an invalid value. If you wanted a plain
old "&" char (it looks this is the case), we should use & .

Therefore, all the places like this one:

+      <a href="editproducts.cgi?action=editgroupcontrols&product=[% productID
FILTER url_quote%]">

must be changed into:

+      <a href="editproducts.cgi?action=editgroupcontrols&amp;product=[%
productID FILTER url_quote%]">

Nit: The interface section should be complete, especially since you already
added the "- action: string" to it. We should specify in it the other variables
as well, especially those in the DEFAULT section.

Nit:

+		 <font color="red">description missing</font>

CSS is the way to go here. Adding a CSS class for missing would be cool, unless
my memory tells me we already have one for missing items.

Comment 31

[GavinS]

2 comments on attachment#183053 [details] [diff] [review] :

1) You seem to have removed the facility to change the case of a product. I think we want to be able to do 
this -- there are outstanding bugs to be able to change the case of component/version/keyword etc...

2) There is a admin/products/footer.html.tmpl, which would be nice to use, if possible, to provide a 
consistent~ish footer

Comment 32

[Frédéric Buclin]

/me would be really happy to see some collaboration and coordination between
Tiago and GavinS on editproducts.cgi templatization.

GavinS, any hope to see you in IRC? Your experience would be really useful here.
Tiago (under kiko and joel's pressure) tries to move this stuff into .pm files
in order to use product objects, ditto for components, etc..., see bug 294160.
i'm sure you could work together and share the work to do.

Comment 33

[Frédéric Buclin]

editproducts.cgi is now completely templatized! :)