Closed Bug 306638 Opened 15 years ago Closed 13 years ago

Midair with product change while adding an attachment silently clears flags

Categories

(Bugzilla :: Attachments & Requests, defect)

2.19.1
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: neil, Assigned: timello)

References

Details

(Whiteboard: [wanted-bmo])

Attachments

(2 files, 3 obsolete files)

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
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?
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.
That won't help Muggins creating an attachment with the now wrong flags...
(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."
(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.
Summary: Midair with product change silently clears flags → Midair with product change while adding an attachment silently clears flags
Assignee: attach-and-request → timello
Status: NEW → ASSIGNED
LpSolit,
This patch just warn the user. The warning message is a mkanat's suggestion.
Attachment #281893 - Flags: review?(LpSolit)
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.
It applies cleanly, but uses variables that don't exist on the branch.
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.
Attachment #281893 - Flags: review?(LpSolit) → review-
Attached patch The fixes. (obsolete) — Splinter Review
Attachment #281893 - Attachment is obsolete: true
Attachment #283081 - Flags: review?(LpSolit)
Attached patch Some other fixes. (obsolete) — Splinter Review
Attachment #283081 - Attachment is obsolete: true
Attachment #283413 - Flags: review?(LpSolit)
Attachment #283081 - Flags: review?(LpSolit)
Whiteboard: [wanted-bmo]
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.
Attachment #283413 - Flags: review?(LpSolit) → review+
Target Milestone: --- → Bugzilla 3.2
Attachment #283413 - Attachment is obsolete: true
Attachment #284150 - Flags: review?(LpSolit)
Comment on attachment 284150 [details] [diff] [review]
Warning the user when editing existent flags.

Thanks for the patch. r=LpSolit
Attachment #284150 - Flags: review?(LpSolit) → review+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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)
Blocks: 413258
You need to log in before you can comment on or make changes to this bug.