266159 - It's possible to create multiple flags by reloading, even if flag is not multiplicable

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Christian Reis]

It's possible to create multiple flags by reloading, even if flag is not
multiplicable. Just create a new flag, and on the confirmation page (Changes to
Attachment XXX submitted) reload as many times as you like.

We should raise an error saying that there is already a flag of that type
associated with that attachment.

Comment 1

[Frédéric Buclin]

I think the problem comes from Flag::FormToNewFlags() which does not check if
non-multiplicable flag types already have a flag set for the given bug/attachment.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Comment 3

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

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

>+    my $flag_types = Bugzilla::FlagType::match(
>+        { 'target_type'  => $target->{'type'},
>+          'product_id'   => $target->{'product_id'},
>+          'component_id' => $target->{'component_id'},
>+          'is_active'    => 1 });

Don't we already determine that the flag types submitted in the form are valid
for the given product and component?

>+        # Get the number of active flags of this type already set for this target.
>+        $flag_type->{'flag_count'} = count(
>+            { 'type_id'     => $type_id,
>+              'target_type' => $target->{'type'},
>+              'bug_id'      => $target->{'bug'}->{'id'},
>+              'attach_id'   => $target->{'attachment'}->{'id'},
>+              'is_active'   => 1 });

It would be better to use a local variable here instead of type->flag_count,
since the member property is known to mean "total flags for this type", and we
shouldn't confuse that by overloading the meaning with another one (once
FlagType becomes a true object, we probably won't create a new one every time
we want access to the same type, so overloading the meaning could also
introduce bugs in the future).

Otherwise this looks good.

Comment 4

[Frédéric Buclin]

Attachment: patch, v2

$flag_type->{'flag_count'} is now $has_flags.

Comment 5

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

Approved for checkin during 2.20 freeze.

Comment 6

[Frédéric Buclin]

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