If a user doesn't have access to change target milestone, it gets set back to default.

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
9 years ago
9 years ago

People

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

Tracking

({regression})

Bugzilla 3.2
regression
Bug Flags:
approval +
approval3.2 +
blocking3.2 +

Details

Attachments

(1 attachment)

997 bytes, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
In check_can_change_field() in Bug.pm, I added some code near the top so that only users with "editcomponents" privs on a product can change the target milestone:

    if ($field eq 'target_milestone') {
        $$PrivilegesRequired = 3;
        return $user->in_group('editcomponents', $self->{'product_id'})
    }

Now, if a user who does not have this privilege saves any changes to a bug, the target milestone for that bug gets reset back to the default value (---).

I'm not sure if this is a regression or if it's always been like this, but I think I'm doing something supported, and the behaviour seems obviously broken (to me).
Flags: blocking3.2?

Updated

9 years ago
Flags: blocking3.2? → blocking3.2+
Keywords: qawanted

Updated

9 years ago
Target Milestone: --- → Bugzilla 3.2

Comment 1

9 years ago
I'm assuming this is 3.2rc2, yes?
(Assignee)

Comment 2

9 years ago
I can reproduce the problem. I have a bug with TM = 1.3, and doing nothing besides clicking "Commit", I get:

You tried to change the Target Milestone field from ---  to 1.3  , but only a user with the required permissions may change that field. 


As I don't try to edit the TM, and it was already set to 1.3, I shouldn't get this error.
Flags: testcase?
Keywords: qawanted
(Assignee)

Comment 3

9 years ago
About the testcase flag, we will only implement a script if it's doable without altering check_can_change_field().
(Assignee)

Comment 4

9 years ago
First, the good news: the problem is specific to milestones. Other fields are not affected by this bug.

Now the bad news: the error comes from set_product(). This method has the following test:

    if ($product_changed && Bugzilla->usage_mode == USAGE_MODE_BROWSER) {

As you don't edit the product, $product_changed is false and so we jump to:

    else {
        ....
        if ($self->check_can_change_field('target_milestone', 0, 1)) {
            $self->set_target_milestone($tm_name);
        }
        else {
            # Have to set this directly to bypass the validators.
            $self->{target_milestone} = $product->default_milestone;
        }
    }

So the last check is wrong. If you don't edit the milestone AND you don't have privs to edit it anyway, it falls back to the default milestone instead of first making sure the current milestone is valid (which is probably the case as the bug is still in the same product).

Note that you can only trigger this problem if you edit check_can_change_field() to use different privs to edit the product and the target milestone, which explains why our Selenium scripts do not catch this issue -> severity 'normal' rather than 'major'. Still a blocker, though.
Severity: major → normal
Depends on: 452896
Flags: testcase?
Keywords: regression
(Assignee)

Comment 5

9 years ago
Created attachment 348994 [details] [diff] [review]
patch, v1

If the product changed and you are not allowed to choose the milestone, use the default one. In all other cases, check the given milestone passed by the user. If the user is not allowed to select it, 'undef' is passed to set_product() which automatically sets $tm_name to the current milestone, meaning that this test will pass, as desired. So all 8 combinations (web/API + product changed/unchanged + milestone edition allowed/forbidden) work as expected now.
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
Attachment #348994 - Flags: review?(mkanat)

Comment 6

9 years ago
Comment on attachment 348994 [details] [diff] [review]
patch, v1

That looks right to me.
Attachment #348994 - Flags: review?(mkanat) → review+

Updated

9 years ago
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 7

9 years ago
tip:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.268; previous revision: 1.267
done

3.2rc2:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.18; previous revision: 1.241.2.17
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.