302955 - remove the "delete" link for the default milestone

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Frédéric Buclin]

When the list of available milestones for a given product is displayed in
editmilestones.cgi, a link to delete the default milestone is displayed (as well
as for other milestones), but clicking on this link gives you an error that you
are not allowed to delete the default milestone. This link should not be
displayed for the default milestone!

Comment 1

[Frédéric Buclin]

GavinS, interested in taking this bug? It looks like a bit hard to fix, right?

Comment 2

[GavinS]

(In reply to comment #1)
> GavinS, interested in taking this bug? It looks like a bit hard to fix, right?

Yes, I will try to. It looked _Really_ easy at first, but then I remembered that
the admin/table.html.tmpl template produces the lists of milestones, so it is
not quite as simple as I originally hoped :(

I think the best way is to add (yet) another parameter to the table.html.tmpl
interface, which will allow for a per-row, per-column override. (unless anyone
has any simpler solutions?)

Comment 3

[GavinS]

Attachment: add ability to override cells in the admin table template v1

Comment 4

[Frédéric Buclin]

Comment on attachment 192597 [details] [diff] [review]
add ability to override cells in the admin table template v1

I will try to review this patch this week if nobody else wants to. This may fix
a similar problem we have in another bug.

Comment 5

[Frédéric Buclin]

Comment on attachment 192597 [details] [diff] [review]
add ability to override cells in the admin table template v1

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

> 'admin/table.html.tmpl' => [
>   'link_uri',
>-  'c.content'
>+  'content'
> ],

Nit: I think it is even better to remove 'c.content' from here and add 'FILTER
none' in admin/table.html.tmpl (appears in only one place!).


>Index: template/en/default/admin/table.html.tmpl

>+  #   Each column value mentioned in the 'columns' documentation
>+  #   above can be overwritten (apart from name and heading). To
>+  #   override a table-cell value 'xxx', specify a new 'xxx' value, and
>+  #   specify a 'override_xxx' value as well. e.g.
>+  #
>+  #    overrides.action = [ {
>+  #      match_value => "$row.name"
>+  #      match_field => 'name'
>+  #      override_content => 1
>+  #      content => "(Default milestone, so cannot be deleted)"
>+  #      override_contentlink => 1
>+  #      contentlink => undef
>+  #    } ]

Nit: I don't think this example is a good one, because it's too specific to our
case, and may appear confusing for other developers.


>+          [% SET contentlink = override.contentlink 
>+             IF override.override_contentlink %]

Nit: these 'SET' are useless. Are they used anywhere else among templates?? I
think it's the first time I see them (but I don't edit templates very often).
;)


>Index: template/en/default/admin/milestones/list.html.tmpl

> [% columns.push({
>+     name => "action"
>      heading => "Action"
>      content => "Delete"
>      contentlink => delete_contentlink
>-   }) %]
>+   })
>+%]

Nit: this is not really in relation with your patch, but I see no reason to
have this 'columns.push'. It's should simply be the third element of '[%
columns = [ ... ] %]'. Would you be nice enough to do this small cleanup? ;)


>+[% FOREACH row = milestones %]
>+  [% IF row.name == default_milestone %]
>+     [% overrides.action = [ {
>+         match_value => "$row.name"
>+         match_field => 'name'
>+         override_content => 1
>+         content => "(Default milestone, so cannot be deleted)"
>+         override_contentlink => 1
>+         contentlink => undef
>+    } ] %] 
>+
>+    [% LAST %]
>+  [% END %]
>+[% END %]

1) This FOREACH loop and IF test are useless. Replace "$row.name" by
"$default_milestone" to fix the problem!
2) "(Default milestone, so cannot be deleted)" is too long. The absence of a
link to delete this milestone is explicit enough, IMO. So I would write
"(Default milestone)" only.


I already tested your patch. It works great! :)

Comment 6

[GavinS]

Attachment: add ability to override cells in the admin table template v2

I couldn't make up a better example, so I removed the confusing one I had

Comment 7

[Frédéric Buclin]

Comment on attachment 194095 [details] [diff] [review]
add ability to override cells in the admin table template v2

Works fine. And I don't see a better idea to avoid the FOREACH loop in
admin.html.tmpl. So r=LpSolit

Comment 8

[Frédéric Buclin]

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.42; previous revision: 1.41
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.48; previous revision: 1.47
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
new revision: 1.6; previous revision: 1.5
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
new revision: 1.2; previous revision: 1.1
done