190224 - templatize editmilestones.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Dave Miller [:justdave]]

templatize editmilestones.cgi

Comment 1

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 2

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

[GavinS]

Attachment: editmilestones templatisation v1

1) Added check for milestone size when updating and creating

2) Changed size and maxlength fields on forms from 64 to 20, as per DB spec

3) Moved TestProduct and CheckProduct to Bugzilla/Util.pm
3.1) (in this file and other admin cgis)
3.2) and converted to DBI

4) Added '.length$' to valid TT vmethods allowed when testing filtering. OK to
do?

5) Removed %::FORM

6) Ported to DBI

7) And did the templatisation

Comment 4

[GavinS]

Comment on attachment 156289 [details] [diff] [review]
editmilestones templatisation v1

Jouni: Any chance of a review?This should be a little bit easier and quicker
than the editcomponents one...

Comment 5

[Jouni Heikniemi]

Comment on attachment 156289 [details] [diff] [review]
editmilestones templatisation v1

Sorry to make you the first victim of my new split-the-patches campaign, but
I'm in a flu so I have extra energy for whining ;-) See
<http://www.heikniemi.net/hc/archives/000109.html> for some of my thoughts on
these überpatches.

>1) Added check for milestone size when updating and creating
>2) Changed size and maxlength fields on forms from 64 to 20, as per DB spec
>3) Moved TestProduct and CheckProduct to Bugzilla/Util.pm
>3.1) (in this file and other admin cgis)
>3.2) and converted to DBI
>4) Added '.length$' to valid TT vmethods allowed when testing filtering. OK to do?
>5) Removed %::FORM
>6) Ported to DBI
>7) And did the templatisation

Of these, only 4, 5 and 7 are really tightly integrated to templatization.
Anyway, I'm ready to get 1-2 and even 6 in at the same shot, if only because
you're practically rewriting the whole file anyway. Number 3 is unrelated.
Worse, it changes several files, adding the bitrot probability. Also, it
touches a heavily used code library (Bugzilla:Util), an operation that needs
totally different kind of review attention than a "simple" templatization.

Please get issue 3 into a separate patch and re-request review; I'll try to
respond as quickly as possible.

Comment 6

[GavinS]

Attachment: editmilestones templatisation v2

I've moved item 3) to another bug, I will try and apply your (very sensible)
split-the-patches approach to future patches...

Comment 7

[GavinS]

Attachment: editmilestones templatisation v3

Noticed that I need to add  2 * $sth->finish

Comment 8

[Jouni Heikniemi]

Comment on attachment 157274 [details] [diff] [review]
editmilestones templatisation v3

When deleting a milestone:

Insecure dependency in parameter 4 of DBI::db=HASH(0x8677e2c)->do method call
while running with -T switch at /var/www/bugzilla/tip/editmilestones.cgi line
418


The bug deletion logic is insane, but let's not worry about it... Anyway, I'd
naturally think deleting a milestone should cause all bugs on it to be moved to
the default milestone instead of being deleted (with allowbugdeletions), but
that's food for another bug.

>Index: template/en/default/filterexceptions.pl
>===================================================================

>-# Safe vmethods                   - [% foo.size %]
>+# Safe vmethods                   - [% foo.size %] [% foo.length %]

Mention push here while you're at it.

>+++ template/en/default/admin/milestones/confirm-delete.html.tmpl	Sat Aug  7 19:42:42 2004

>+     href="buglist.cgi?terget_milestone=[% name FILTER url_quote %]&product=

target, not terget.

>+    outstanding for this milestone.  You must reassign 

... or "move"? Reassigning is for owners...


>+    <p>Sorry; '[% name FILTER html %]'  is the default milestone for product '

Nit: Double space after latter apostrophe. Also, rather "Sorry, but" than
"Sorry;".

>+      <form method="post" action="editmilestones.cgi">
>+      <input type="submit" value="Yes, delete">
>+      <input type="hidden" name="action" value="delete">
>+      <input type="hidden" name="product" value="[% product FILTER html %]">
>+      <input type="hidden" name="milestone" value="[% name FILTER html %]">
>+      </form>

Nit: Indent the inputs by two spaces. O:-)

>+++ template/en/default/admin/milestones/created.html.tmpl	Wed Aug  4 21:13:15 2004

>+[% title = BLOCK %]Adding new Milestone of Product
>+                   '[% product FILTER html %]'[% END %]

I'd say "for" instead of "of"... But anything goes.

>+<p>The milestone '<a title="Edit milestone '[% name FILTER html %]' of 
>+   product '[% product FILTER html %]'" href="editmilestones.cgi?action=edit&amp;product=

Nit: This line exceeds 80 chars. Enter before href?


>+++ template/en/default/admin/milestones/deleted.html.tmpl	Thu Aug  5 22:37:34 2004

>+<p>Flag inclusions and exclusions deleted.</p>

Hmm... Really? These aren't related to milestones. Copy/paste?

>+++ template/en/default/admin/milestones/edit.html.tmpl	Wed Aug  4 21:13:37 2004

>+[%# INTERFACE:
>+  # name: string; The name of the milestone
>+  #
>+  # description: string; Component description, may be empty
>+  #
>+  # initialowner: string; initial owner, may be empty
>+  #
>+  # initialqacontact: string; initial qa contact, may be empty
>+  #
>+  # product: string; The product the milestone belongs to
>+  #
>+  # bug_count: number; number of bugs belonging to the milestone
>+  #%]

This interface comment has been pasted from elsewhere...

>+   <input type="hidden" name="milestoneold" value="[% name FILTER html %]">
>+   <input type="hidden" name="sortkeyold" value="[% sortkey FILTER html %]">
>+   <input type="hidden" name="action" value="update">
>+   <input type="hidden" name="product" value="[% product FILTER html %]">
>+   <input type="submit" value="Update">

Nit: Unindent by one space.

>+++ template/en/default/admin/milestones/footer.html.tmpl	Wed Aug 11 21:43:13 2004

>+  # no_XXX_link: flag; if defined, then don't show the corresponding 
>+  #                    link. Supported parameters are:

Just use "boolean" as the type... There is a theoretic risk somebody thinks
"flag" actually means a Bugzilla::Flag object.

>+  <a title="Add a milestone to product '[% product FILTER html %]'"
>+     href="editmilestones.cgi?action=add&product=
>+          [%- product FILTER html %]">Add</a> a milestone. 

&amp; and url_quote in the href.

>+  title="Edit Milestone '[% name FILTER html %]' of product '
>+         [%+ product FILTER html %]'"

Don't disable pre_chomp in the product, it causes ugliness.

>+  Edit other milestones of product <a 
>+  title="Choose a milestone from product '[% product FILTER html %]' to edit"
>+  href="editmilestones.cgi?product=
>+  [%- product FILTER url_quote %]">'[% product FILTER html %]'</a>.

That title is pretty unnecessary...

>+  Edit product <a 
>+  title="Edit Product '[% product FILTER html %]'"
>+  href="editproducts.cgi?action=edit&amp;product=
>+  [%- product FILTER url_quote %]">'[% product FILTER html %]'</a>.

As is this; if you want to emphasize the link's meaning, use _Edit product Xxx_
as the link text instead of just Xxx. Only use title when you want to convey
information outside the link context.

>+++ template/en/default/admin/milestones/list.html.tmpl	Wed Aug  4 21:14:05 2004

>+[% PROCESS admin/table.html.tmpl
>+     columns = columns
>+     data = milestones
>+     footer = footer_row
>+%]

The admin table template no longer has support for the footer row.

>+++ template/en/default/admin/milestones/select-product.html.tmpl	Wed Aug  4 21:14:22 2004

>+       heading => "Edit milestone of..."

Nit: Milestone_s_ of...

>+++ template/en/default/admin/milestones/create.html.tmpl	Thu Aug  5 22:43:43 2004

>+[% title = BLOCK %]Add Milestone  to Product '[% product FILTER html %]'[% END %]

Nit: Double space before "to".

Comment 9

[GavinS]

Attachment: editmilestones templatisation v4

(In reply to comment #8)
> Insecure dependency in parameter 4 of DBI::db=HASH(0x8677e2c)->do method call

> while running with -T switch at /var/www/bugzilla/tip/editmilestones.cgi line

> 418

Fixed

> The bug deletion logic is insane, but let's not worry about it... Anyway, I'd

> naturally think deleting a milestone should cause all bugs on it to be moved
to
> the default milestone instead of being deleted (with allowbugdeletions), but
> that's food for another bug.

Agreed


> >+++ template/en/default/admin/milestones/created.html.tmpl	  Wed Aug  4
21:13:15 2004
> 
> >+[% title = BLOCK %]Adding new Milestone of Product
> >+		       '[% product FILTER html %]'[% END %]
> 
> I'd say "for" instead of "of"... But anything goes.

The consensus in other admin pages is 'of', so I'll leave that for now.



> >+  title="Edit Milestone '[% name FILTER html %]' of product '
> >+	     [%+ product FILTER html %]'"
> 
> Don't disable pre_chomp in the product, it causes ugliness.

Done, ans also fixed this in a a couple of other places

> >+++ template/en/default/admin/milestones/list.html.tmpl	  Wed Aug  4
21:14:05 2004
> 
> >+[% PROCESS admin/table.html.tmpl
> >+	 columns = columns
> >+	 data = milestones
> >+	 footer = footer_row
> >+%]
> 
> The admin table template no longer has support for the footer row.

Removed. This is still used in keywords and components templates, so
I'll try and remove those sometime as well (but in a new bug!)

And all the other issues should be sorted...

Comment 10

[Jouni Heikniemi]

Comment on attachment 157347 [details] [diff] [review]
editmilestones templatisation v4

Looking pretty good! Fix the nits below, and I think that does it.


>+++ template/en/default/admin/milestones/created.html.tmpl	Sun Aug 29 11:01:40 2004

>+  # name: string; the name of the newly created miestones

s/miestones/milestone

>+  # product: string; the name of the product the milestones belongs to

s/milestones/milestone

>+++ template/en/default/admin/milestones/footer.html.tmpl	Sun Aug 29 18:46:08 2004

>+[% IF !no_add_milestone_link %]

Perhaps "UNLESS no_add_milestone_link"? (ditto for the latter similar ones in
the file; you don't have to change this, but if you do, change them all).

>+++ template/en/default/admin/milestones/list.html.tmpl	Sun Aug 29 11:09:59 2004

>+  # milestones: array of hashes having the properties:

"having the _following_ properties"

>+  # product: string; the name of the product we are editing milstones for

s/milstones/milestones/

>+++ template/en/default/admin/milestones/select-product.html.tmpl	Sun Aug 29 11:10:42 2004

>+  # products: array of hashes having the properties:

Again, "following".

>+++ template/en/default/admin/milestones/create.html.tmpl	Sun Aug 29 11:11:08 2004

>+  # product: string; name of product

"name of the product the milestone is being created for" (or something similar)

Comment 11

[GavinS]

(In reply to comment #10)
> (From update of attachment 157347 [details] [diff] [review])
> Looking pretty good! Fix the nits below, and I think that does it.

All those issues should be sorted. Sorry about some of them - they were pretty lame

> >+++ template/en/default/admin/milestones/footer.html.tmpl      Sun Aug 29 18:46:08 2004
> 
> >+[% IF !no_add_milestone_link %]
> 
> Perhaps "UNLESS no_add_milestone_link"? (ditto for the latter similar ones in
> the file; you don't have to change this, but if you do, change them all).

I've changed the 2 simple IF's, but left the middle one, which wouldn't get any simpler

Comment 12

[GavinS]

Attachment: editmilestones templatisation v5

Comment 13

[Jouni Heikniemi]

Comment on attachment 157531 [details] [diff] [review]
editmilestones templatisation v5

r=jouni

Comment 14

[Dave Miller [:justdave]]

This patch doesn't apply cleanly because the diff header for created.html.tmpl
is corrupted.  changing the 0,1 0,45 to 0,0 0,44 fixed it.


Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.14; previous revision: 1.13
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.23; previous revision: 1.22
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/confirm-delete.html.tmpl,v
done
Checking in template/en/default/admin/milestones/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/create.html.tmpl,v
done
Checking in template/en/default/admin/milestones/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/create.html.tmpl,v
 <--  create.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/created.html.tmpl,v
done
Checking in template/en/default/admin/milestones/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/created.html.tmpl,v
 <--  created.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/deleted.html.tmpl,v
done
Checking in template/en/default/admin/milestones/deleted.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/deleted.html.tmpl,v
 <--  deleted.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/edit.html.tmpl,v
done
Checking in template/en/default/admin/milestones/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/edit.html.tmpl,v
 <--  edit.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/footer.html.tmpl,v
done
Checking in template/en/default/admin/milestones/footer.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/footer.html.tmpl,v
 <--  footer.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/list.html.tmpl,v
done
Checking in template/en/default/admin/milestones/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/list.html.tmpl,v
 <--  list.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/select-product.html.tmpl,v
done
Checking in template/en/default/admin/milestones/select-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/select-product.html.tmpl,v
 <--  select-product.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/updated.html.tmpl,v
done
Checking in template/en/default/admin/milestones/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/updated.html.tmpl,v
 <--  updated.html.tmpl
initial revision: 1.1
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.71; previous revision: 1.70
done