remove the "delete" link for the default milestone

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
User Interface
--
minor
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Frédéric Buclin, Assigned: GavinS)

Tracking

2.20
Bugzilla 2.22
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

13 years ago
GavinS, interested in taking this bug? It looks like a bit hard to fix, right?
(Assignee)

Comment 2

13 years ago
(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?)
Assignee: myk → bugzilla
(Assignee)

Comment 3

13 years ago
Created attachment 192597 [details] [diff] [review]
add ability to override cells in the admin table template v1
Attachment #192597 - Flags: review?
(Reporter)

Comment 4

13 years ago
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.
Attachment #192597 - Flags: review?(LpSolit)
(Reporter)

Comment 5

13 years ago
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! :)
Attachment #192597 - Flags: review?(LpSolit)
Attachment #192597 - Flags: review?
Attachment #192597 - Flags: review-
(Reporter)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 6

13 years ago
Created attachment 194095 [details] [diff] [review]
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
Attachment #192597 - Attachment is obsolete: true
Attachment #194095 - Flags: review?(LpSolit)
(Reporter)

Comment 7

13 years ago
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
Attachment #194095 - Flags: review?(LpSolit) → review+
(Reporter)

Updated

13 years ago
Flags: approval?
Flags: approval? → approval+
(Reporter)

Comment 8

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 304267
You need to log in before you can comment on or make changes to this bug.