If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Forbids the default milestone from being marked as inactive, and forbids to choose an inactive milestone as the default one

NEW
Unassigned

Status

()

Bugzilla
Administration
--
minor
5 years ago
2 years ago

People

(Reporter: Frédéric Buclin, Unassigned)

Tracking

Details

Attachments

(3 obsolete attachments)

(Reporter)

Description

5 years ago
You currently can mark the default milestone of a product as inactive, as well as you can choose an inactive target milestone as the new default one for a product. These scenarios should be forbidden as they can trigger unexpected results.

Updated

5 years ago
Assignee: administration → hugo.seabrook
Status: NEW → ASSIGNED

Comment 1

5 years ago
Created attachment 714863 [details] [diff] [review]
v1 patch
Attachment #714863 - Flags: review?(LpSolit)
(Reporter)

Updated

5 years ago
Target Milestone: --- → Bugzilla 4.4

Comment 2

5 years ago
This is targeted for Bugzilla 4.4, but hasn't been reviewed yet. Given that 4.4rc2 is out, should it be reviewed soon?
(Reporter)

Comment 3

5 years ago
(In reply to Hugo Seabrook from comment #2)
> This is targeted for Bugzilla 4.4, but hasn't been reviewed yet. Given that
> 4.4rc2 is out, should it be reviewed soon?

I was busy last week with security bugs and releasing 4.4rc2 & co. I will review your patch this week now that things are quiet again.
(Reporter)

Comment 4

5 years ago
Comment on attachment 714863 [details] [diff] [review]
v1 patch

>=== modified file 'template/en/default/global/user-error.html.tmpl'

>   [% ELSIF error == "milestone_is_default" %]
>-    [% title = "Default milestone not deletable" %]
>+    [% title = "Default milestone not deletable or be inactive" %]

Maybe: Default Milestone Cannot Be Deleted Nor Deactivated.


>+  [% ELSIF error == "product_inactive_defaultmilestone" %]
>+    [% title = "Default milestone cannot be inactive" %]

Let's write the title Like This (first letter uppercase).


>+    You must <a href="editmilestones.cgi?action=add&amp;product=[% product FILTER uri %]">
>+    make the milestone '[% milestone FILTER html %]' active</a> before

The link is incorrect. action=add is to create a new target milestone. You should rather use action=edit and include the milestone in the URL to edit it. Also I would prefer a message of the form:

  You must <a href="...">activate</a> the [% milestone FILTER html %] milestone before you can use it as the default milestone.


Now, as inactive milestones cannot be chosen as the default milestone, the select box in the Edit Product page shouldn't list inactive milestones in the Default Milestone field. Those are easy to exclude. And ideally, the "Enabled For Bugs" checkbox on the Edit Milestone page should be disabled for the default milestone.


Otherwise looks good.
Attachment #714863 - Flags: review?(LpSolit) → review-

Comment 5

4 years ago
Created attachment 753763 [details] [diff] [review]
v2 patch

Contains the suggested wording and link changes
Attachment #714863 - Attachment is obsolete: true
Attachment #753763 - Flags: review?(LpSolit)
(Reporter)

Comment 6

4 years ago
Comment on attachment 753763 [details] [diff] [review]
v2 patch

The last paragraph of my previous review about selecting deactivated milestones has not been addressed.
Attachment #753763 - Flags: review?(LpSolit) → review-
(Reporter)

Comment 7

4 years ago
Comment on attachment 753763 [details] [diff] [review]
v2 patch

>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+    You must <a href="editmilestones.cgi?action=edit&amp;milestone=[% milestone FILTER html %]&amp;product=[% product FILTER uri %]">

Also, you must use FILTER uri, not html, for 'milestone'.

Comment 8

4 years ago
Created attachment 771931 [details] [diff] [review]
v3 patch

Sorry about that. Hopefully have addressed the three remaining issues.

Regards,
Hugo.
Attachment #753763 - Attachment is obsolete: true
Attachment #771931 - Flags: review?(LpSolit)
(Reporter)

Comment 9

4 years ago
Comment on attachment 771931 [details] [diff] [review]
v3 patch

(I like to know who is really asking me to review his patches...)
Attachment #771931 - Flags: review?(LpSolit) → review?

Updated

4 years ago
Attachment #771931 - Flags: review? → review?(justdave)
(Reporter)

Comment 10

4 years ago
Comment on attachment 771931 [details] [diff] [review]
v3 patch

The isactive checkbox for default milestones is marked as disabled, which means that no value is passed to the CGI script and so it's seen as being false, which means disabled. If you edit the sortey (or edit nothing), an error is thrown that you tried to disable the default milestone. If you edit the name, no error is thrown and the milestone is disabled, which means that validation checks do not work as _check_is_active() allowed to mark the default milestone as inactive. $product->default_milestone contains the old milestone name and so if you rename the default milestone, it will accept to mark it as disabled (race condition).
Attachment #771931 - Flags: review?(justdave) → review-

Updated

4 years ago
Assignee: simon → administration

Updated

4 years ago
Attachment #771931 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Status: ASSIGNED → NEW
(Reporter)

Comment 11

2 years ago
Bugzilla 4.4 is now restricted to security fixes only.
Target Milestone: Bugzilla 4.4 → ---
You need to log in before you can comment on or make changes to this bug.