Closed Bug 190196 Opened 22 years ago Closed 19 years ago

editproducts.cgi needs templatizing

Categories

(Bugzilla :: Administration, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: jnerad, Assigned: bugzilla)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
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)
Status: UNCONFIRMED → RESOLVED
Closed: 22 years ago
Resolution: --- → WORKSFORME
In 2.17.3.  My version of editproducts.cgi is definitely NOT templatized.
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.
Severity: trivial → enhancement
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Summary: Needs templatizing → editproducts.cgi needs templatizing
Target Milestone: --- → Bugzilla 2.18
confirming, accepting (I'm already working on some of this stuff anyway)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
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.
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 :)
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?
it's not in Bugzilla itself, that's imported by "use Template;"
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.
This is the counterpart to the templates (attachment 112382 [details])
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.
- passes the no-tabs test in runtests.sh
- correct default values in add
Attachment #112382 - Attachment is obsolete: true
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 :)
Attachment #112401 - Attachment description: The templates for add/edit/delete products → The templates for add/edit/delete products (gzipped tarball)
Attachment #112401 - Flags: review?(gerv)
Joel, CCing you since you added the two existing templates to this file :)
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;
Attachment #112401 - Flags: review-
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.

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
- 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?
Attachment #112383 - Attachment is obsolete: true
Attachment #112401 - Attachment is obsolete: true
Attachment #112520 - Flags: review?
- Fixes table problem in edit
- Fixes problem in delete confirmation
Attachment #112520 - Attachment is obsolete: true
Attachment #112858 - Flags: review?(gerv)
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.
Sorry for all the spam.  I'll get it right one day.
Attachment #112863 - Flags: review?(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
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 ;).
Attachment #112401 - Flags: review?(gerv)
Attachment #112858 - Flags: review?(gerv) → review-
Attachment #112863 - Flags: review?(gerv) → review-
Attachment #112520 - Flags: review?
*** Bug 216929 has been marked as a duplicate of this bug. ***
Is supposed to be something left to do to get editproducts.cgi fully templatised?
No longer blocks: 214907
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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.
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Blocks: 265898
No longer blocks: 265898
Depends on: 265898
Assignee: justdave → bugzilla
Status: ASSIGNED → NEW
Depends on: 280412
Depends on: 282384
Depends on: 289580
I decided to split the patches to be easy to review. Would be wonderful if we
can land these templatizing as soon as possible.
Attachment #183053 - Flags: review?
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?
Depends on: 293524
Depends on: 294160
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 &amp; .

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.
Attachment #183053 - Flags: review? → review-
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
/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.
Depends on: 299753
Depends on: 302370
Blocks: 266143
Depends on: 306242
Blocks: 293524
No longer depends on: 293524
Depends on: 306757
Blocks: 299753
No longer depends on: 299753
Depends on: 307524
editproducts.cgi is now completely templatized! :)
Status: NEW → RESOLVED
Closed: 22 years ago19 years ago
Resolution: --- → 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.

Attachment

General

Creator:
Created:
Updated:
Size: