306638 - Midair with product change while adding an attachment silently clears flags

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Create a new attachment
2 [details] [diff] [review]. Set some request flags
3. Someone changes the product
4. Submit the attachment

Expected results: message indicating that requests were no longer valid

Actual results: invalid requests completely ignored

Comment 1

[Frédéric Buclin]

The reason is in Flag::FormToNewFlags():

    # Get a list of active flag types available for this target.
    my $flag_types = Bugzilla::FlagType::match(
        { 'target_type'  => $target->{'type'},
          'product_id'   => $target->{'product_id'},
          'component_id' => $target->{'component_id'},
          'is_active'    => 1 });

    my @flags;
    foreach my $flag_type (@$flag_types) {
        my $type_id = $flag_type->{'id'};

        # We are only interested in flags the user tries to create.
        next unless scalar(grep { $_ == $type_id } @type_ids);


We first get a list of all flag types available for this product and component;
then we extract those for which the user wants new flags and create them. Flag
types unrelated to the product are (silently) ignored.

myk, is that something we want to fix?

Comment 2

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

Since we have a "confirm component, version, and target milestone" page, it
might be worth mentioning on it that some flags/requests will get deleted.

Comment 3

[[e-mail]]

That won't help Muggins creating an attachment with the now wrong flags...

Comment 4

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

(In reply to comment #3)
> That won't help Muggins creating an attachment with the now wrong flags...

Right.  We would need to put this info into the "attachment created" message,
something like:

"foo changed the bug while you were attaching the file.  The changes they made
were: ...  You tried to ask bar for baz on the attachment, but the baz flag
doesn't exist in the bug's new product/component, so I couldn't request the flag."

Comment 5

[Frédéric Buclin]

(In reply to comment #4)

> Right.  We would need to put this info into the "attachment created" message,
> something like:

I think that's something easy to do. I could have a look at it next week if
nobody else wants to.

Comment 6

[Tiago Mello [:timello]]

Attachment: Warns the user if the flags are no long valid

LpSolit,
This patch just warn the user. The warning message is a mkanat's suggestion.

Comment 7

[Dave Miller [:justdave]]

Just to be complete, this patch does not work on the 3.0 branch.  We'll have to backport it to 3.0 to use it on bmo.

Comment 8

[Dave Miller [:justdave]]

It applies cleanly, but uses variables that don't exist on the branch.

Comment 9

[Frédéric Buclin]

Comment on attachment 281893 [details] [diff] [review]
Warns the user if the flags are no long valid

No need to duplicate code. A much better approach is to let Bugzilla::Flag::FormToNewFlags() set $vars->{'message'} if it detects an unexpected flag. Bugzilla::Flag::process() can then return it to its caller.

Note that bug 99215 will warn you if the product/component changed, so this probably makes this feature less "critical".

About the warning being displayed. Don't call new_from_list() as the error message is wrong if a flagtype is being deleted while you try to set it. Just say that some flags couldn't be set, and suggest the user to check them. No need to mention which flags; the user can do it himself.

Comment 10

[Tiago Mello [:timello]]

Attachment: The fixes.

Comment 11

[Tiago Mello [:timello]]

Attachment: Some other fixes.

Comment 12

[Frédéric Buclin]

Comment on attachment 283413 [details] [diff] [review]
Some other fixes.

>Index: Bugzilla/Flag.pm

>     # Create new flags and update existing flags.
>-    my $new_flags = FormToNewFlags($bug, $attachment, $cgi);
>+    my $new_flags = FormToNewFlags($bug, $attachment, $cgi, $hr_vars);
>     foreach my $flag (@$new_flags) { create($flag, $bug, $attachment, $timestamp) }
>     modify($bug, $attachment, $cgi, $timestamp);

This change is correct for new flags, but if you edit an existing flag which won't be a valid one in the new product/component, the user will get no notification about it. Till now, no notification at all was sent about removed flags, but as you now warn the user about new flags being invalid, it would make sense to also warn the user about existing flags being invalid. To fix that, look at the following line (this line appears twice in Flag.pm):

  clear($flag, $bug, $flag->attachment) unless $is_retargetted;

You should change it to something like:

unless ($is_retargetted) {
    clear($flag, $bug, $flag->attachment);
    $hr_vars->{'message'} = 'flag_cleared';
}

and define a new message for it.

r=LpSolit anyway. But please fix my comment above and re-request review.

Comment 13

[Tiago Mello [:timello]]

Attachment: Warning the user when editing existent flags.

Comment 14

[Frédéric Buclin]

Comment on attachment 284150 [details] [diff] [review]
Warning the user when editing existent flags.

Thanks for the patch. r=LpSolit

Comment 15

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.133; previous revision: 1.132
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.190; previous revision: 1.189
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.390; previous revision: 1.389
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.50; previous revision: 1.49
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.87; previous revision: 1.86
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.65; previous revision: 1.64
done

Comment 16

[Dave Miller [:justdave]]

Attachment: Backport to 3.0 branch

This patch is backported to the 3.0 branch for use on bmo.  It's actually not very intrusive, so it might be safe to commit on the 3.0 branch, but I'll go for a second opinion on that before okaying it.  Probably needs someone to actually review it for accuracy of the backport before committing if we do that (I've tested it on bugzilla-stage though)