364216 - Flag::retarget() is broken

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Frédéric Buclin]

Flag::retarget() contains these lines:

            _validate($flag, $flag->type, $flag->status, $flag->setter,
                      $requestee, $is_private);

which doesn't match what _validate() expects:

sub _validate {
    my ($flag, $flag_type, $status, $requestees, $private_attachment,
        $bug_id, $attach_id) = @_;

The last two arguments are optional, so this is fine. The problem is $flag->setter which should not be here. Or said otherwise, maybe it should be here, but _validate() always assumes the flag setter is Bugzilla->user.

Comment 1

[Frédéric Buclin]

This must be fixed before we release 2.23.4, or 3.0 RC1 at the latest.

Comment 2

[Max Kanat-Alexander]

Yes, this is major enough to be a blocker, since it can cause dataloss.

Comment 3

[Frédéric Buclin]

While fixing this bug, I realized that Flag::target() itself was broken. The call to _validate() is only one part of the problem.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 249242 [details] [diff] [review]
patch, v1

This seems reasonable and correct. r=myk

Comment 6

[Frédéric Buclin]

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.79; previous revision: 1.78
done

Comment 7

[Frédéric Buclin]

Wow, I forgot to retarget attachment flags. And I realized that there are bugs in the code anyway. Reopening!

Comment 8

[Frédéric Buclin]

Attachment: patch (part 2), v1

I hope this fixes all issues.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 250359 [details] [diff] [review]
patch (part 2), v1

>+    my $flags = Bugzilla::Flag->new_from_list($flag_ids);
>+    foreach my $flag (@$flags) {
>+        my $is_retargetted = retarget($flag, $bug);
>+        clear($flag, $bug, $flag->attachment) unless $is_retargetted;

I wonder if there's a way to not have to define a temporary variable here, i.e. something like:

    foreach my $flag (@{Bugzilla::Flag->new_from_list($flag_ids)}) {
        my $is_retargetted = retarget($flag, $bug);
        clear($flag, $bug, $flag->attachment) unless $is_retargetted;

Otherwise this looks good. r=myk

Comment 10

[Frédéric Buclin]

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.49; previous revision: 1.48
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.80; previous revision: 1.79
done