editproducts.cgi needs templatizing

RESOLVED FIXED in Bugzilla 2.22

Status

()

--
enhancement
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: jnerad, Assigned: bugzilla)

Tracking

unspecified
Bugzilla 2.22
Dependency tree / graph

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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
Last Resolved: 16 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

16 years ago
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
(Reporter)

Comment 5

16 years ago
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 :)
(Reporter)

Comment 7

16 years ago
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;"
(Reporter)

Comment 9

16 years ago
Created attachment 112382 [details]
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.
(Reporter)

Comment 10

16 years ago
Created attachment 112383 [details] [diff] [review]
changes to editproducts.cgi to get add/edit/delete working with Template

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

Comment 11

16 years ago
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.
(Reporter)

Comment 12

16 years ago
Created attachment 112401 [details]
The templates for add/edit/delete products (gzipped tarball)

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

16 years ago
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-
(Reporter)

Comment 16

16 years ago
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
(Reporter)

Comment 18

16 years ago
Created attachment 112520 [details] [diff] [review]
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?
Attachment #112383 - Attachment is obsolete: true
Attachment #112401 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Attachment #112520 - Flags: review?
(Reporter)

Comment 19

16 years ago
Created attachment 112858 [details] [diff] [review]
editproducts.cgi templatization patch

- Fixes table problem in edit
- Fixes problem in delete confirmation
Attachment #112520 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Attachment #112858 - Flags: review?(gerv)

Comment 20

16 years ago
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.
(Reporter)

Comment 21

16 years ago
Created attachment 112863 [details] [diff] [review]
editproducts.cgi templatization patch (rev 4)

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

Updated

16 years ago
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
(Reporter)

Comment 23

16 years ago
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 #112858 - Flags: review?(gerv) → review-
Attachment #112863 - Flags: review?(gerv) → review-

Updated

16 years ago
Attachment #112520 - Flags: review?
*** Bug 216929 has been marked as a duplicate of this bug. ***
Blocks: 214907
Is supposed to be something left to do to get editproducts.cgi fully templatised?
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20

Comment 26

15 years ago
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

Updated

14 years ago
Blocks: 265898
No longer blocks: 265898
Depends on: 265898
(Assignee)

Updated

14 years ago
Assignee: justdave → bugzilla
Status: ASSIGNED → NEW
(Assignee)

Updated

14 years ago
Depends on: 280412
(Assignee)

Updated

14 years ago
Depends on: 282384
(Assignee)

Updated

14 years ago
Depends on: 289580
Created attachment 183053 [details] [diff] [review]
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.
Attachment #183053 - Flags: review?

Comment 29

14 years ago
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?

Updated

14 years ago
Depends on: 293524

Updated

14 years ago
Depends on: 294160

Comment 30

14 years ago
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-
(Assignee)

Comment 31

14 years ago
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

14 years ago
/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.

Updated

14 years ago
Depends on: 299753

Updated

13 years ago
Depends on: 302370
(Assignee)

Updated

13 years ago
Blocks: 266143
(Assignee)

Updated

13 years ago
Depends on: 306242

Updated

13 years ago
Blocks: 293524
No longer depends on: 293524
Depends on: 306757

Updated

13 years ago
Blocks: 299753
No longer depends on: 299753

Updated

13 years ago
Depends on: 307524

Comment 33

13 years ago
editproducts.cgi is now completely templatized! :)
Status: NEW → RESOLVED
Last Resolved: 16 years ago13 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.