180652 - Marking an attachment as obsolete should cancel all unfulfilled requests

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P2)

Description

[Bradley Baetz (:bbaetz)]

I'm sure I've seen this somewhere before, but maybe it was just in IRC

If I replace an attachment with another, making the original obsolete, then any
original requests still in the '?' state should be dropped (or transferred to
the new one, perhaps optionally)

Furthermore, making a request (and possible even changing any flag) on an
obsolete attachment should be forbidden

Theres no point in asking someone to review an attachment which has a newer
version available.

Comment 1

[Boris Zbarsky [:bzbarsky]]

amen to this.  I just went through my review queue and 3 of the requests (that's
about 20%) were for obsolete attachments... and that was just since yesterday,
when I cleaned all such requests out.  ;)

Comment 2

[Dave Miller [:justdave]]

*** Bug 180736 has been marked as a duplicate of this bug. ***

Comment 3

[Bradley Baetz (:bbaetz)]

See bug 180833, which is going to migrate them along. This bug is still sort of
separate, in that we should be blocking requests on obsolete attachments anyway.

Comment 4

[:deanis74]

Personally I'd still like to see a request history, even for obsolete
attachments.  It's good to know for future reference, if nothing else.  What
about just not including them in request.cgi?  Or am I missing the point?

Comment 5

[Ran]

This should also resolve bug 180812 (also on the meta-bug)

Comment 6

[Dave Miller [:justdave]]

*** Bug 232217 has been marked as a duplicate of this bug. ***

Comment 7

[Frédéric Buclin]

Adding bug 237209 as this last one cannot be resolved if this particular point
is not discussed (and solved) first.

Comment 8

[James Ross]

While this is all well and good, should I file a new bug to get the request
queue to show the obsolete-ness of patches? I disagree that they should be
hidden, as they *do* have the requests set, and instead would rather see the
standard strikethrough effect.

Comment 9

[alanjstr]

What exactly are you trying to see?  Bugs that have obsolete patches with no
outstanding requests?

Comment 10

[James Ross]

I'm not trying to "see" anything, I just want obsolete patches to have the
strikethrough effect if they appear on the request queue.

Comment 11

[alanjstr]

Well, this directly conflicts with that, no?  I thought the plan was to
unclutter the request queue.  Maybe, instead of removing the requests, we toggle
in the review queue (via css or a button).

Comment 12

[James Ross]

Only partially. You can still set requests on obsolete patches after making them
obsolete, and I don't think you'd last long trying to prevent that. Also, I
don't think requests should be removed unconditionally, but I'm not sure of a
sensible way to do that, other than the case of the add attachment page (where a
"Also remove request flags" checkbox would be simple).

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I really need this.

I'd settle for just a checkbox on the attachment page next to each "obsolete
this attachment?", saying "... and clear requests?"

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> I really need this.

I won't have time to fix it in the following two weeks (mid-february). But this
is the next bug in my "to do" list! ;)

Comment 15

[Frédéric Buclin]

Attachment: patch, v1

OK, here is a first attempt.

All requests for the attachment being marked as obsolete are removed and new
requests on obsolete attachements are ignored, even if the display does not
forbid you to do so (because at the time of displaying an attachment, you still
don't know if it will be marked as obsolete or not; in the same way, if an
attachment marked as obsolete will be marked as non-obsolete or not).

Note that this patch only applies when editing an existing attachment, as
creating a new one and marking another attachment as obsolete should move
pending requests to the new attachment, see bug 180833.

Comment 16

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

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

This code assumes that all requests appear as fields in the UI, but that's not
guaranteed, and if some requests don't appear in the UI, either because we
change the UI in the future, because of a browser bug, or because a user
maliciously modifies their form, they won't get cancelled by the code in this
patch.

Instead of embedding this code in the processes that update existing flags and
add new ones, I suggest a separate chunk of code which runs after flags have
been updated/created.  This has the added advantages of being simpler (since
flag changes have already taken place at that point, so you don't have to
differentiate between new and existing flags) and letting users set flags while
obsoleting an attachment, which is impossible with the current patch.

Comment 17

[Frédéric Buclin]

Attachment: patch, v2

This patch is much better than the previous one: it makes no assumption about
the UI, and applies for both the creation of a new attachment and updating an
existing attachment.

Comment 18

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

Comment on attachment 176308 [details] [diff] [review]
patch, v2

>Index: attachment.cgi

>+      # If the obsolete attachment has request flags, deactivate them.
>+      Bugzilla::Flag::DeactivateRequests($::FORM{'bugid'}, $obsolete_id, $sql_timestamp);

We should cancel these requests as part of the process of updating flags on the
attachment.  It doesn't matter when a user obsoletes an attachment while
creating a new one, but if the user obsoletes an existing attachment and
simultaneously sets/clears a non-request flag on it, the flag change and the
canceling of unfulfilled requests would be recorded separately in the activity
log, but they should be recorded together as the complete set of changes to the
attachment's flags.


>Index: Bugzilla/Flag.pm

>+sub snapshot {

Excellent factoring out of code.


>+sub update_activity {
>+    my ($bug_id, $attach_id, $timestamp, $old_summaries, $new_summaries) = @_;

...

>     if ($removed ne $added) {
>+        $attach_id ||= 'NULL';

Nit: this should happen right after initializing $attach_id in the first line
of the function.


>+sub DeactivateRequests {

Nit: the requests are getting cancelled, not deactivated, so CancelRequests
would be a better name for this.


>+    return unless (scalar(@$request_ids));

Nit: works, but more readable when not implicitly interpreted as boolean and
when conditional reflects reason *to* return rather than reason not to, i.e.:

    return if scalar(@$request_ids) == 0;

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> We should cancel these requests as part of the process of updating flags on the
> attachment.  It doesn't matter when a user obsoletes an attachment while
> creating a new one, but if the user obsoletes an existing attachment and
> simultaneously sets/clears a non-request flag on it, the flag change and the
> canceling of unfulfilled requests would be recorded separately in the activity
> log, but they should be recorded together as the complete set of changes to the
> attachment's flags.

myk, looking at this problem more closely, I still think this patch does it
right. If a user sets a flag to '?' and marks the patch as obsolete at the same
time, this flag will be cancelled right after it has been created, as expected.
But meanwhile an email is sent to the requestee and this bit of information
should be kept in the activity table. If we don't update the activity table, our
process is inconsistent as the requestee would receive an email which would not
be mentioned in the activity table. Then, in order to be consistent, this email
should not be sent to the requestee at all or this flag should not be created.
But doing this is much more invasive, see my previous patch.

In order to have a full solution, we would need to merge these two patches. Do
we really want this?

myk, what I suggest is to accept this patch as is for 2.20 (we freeze in two
days) and eventually open a new bug to optimize this (minor) "problem". Does it
sound reasonable to you?

Comment 20

[Frédéric Buclin]

Attachment: patch, v3

I finally found a solution which fixes the remaining problem. When obsoleting a
patch, all new requests are ignored, thus preventing any requestee to receive
an email. This way, we can safely ignore these flags when updating the activity
table (indeed, these flags are not created).

Moreover, we do not distinguish anymore request flags removed by the user and
the ones removed when obsoleting attachments, as requested by myk. :)

Comment 21

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

Comment on attachment 177321 [details] [diff] [review]
patch, v3

With this patch, all flag changes appear in a single line in the activity log,
which is great.  This patch is almost perfect except for one thing: we
shouldn't ignore requests the user creates while obsoleting an attachment.

The reason we cancel requests when a user obsoletes an attachment is not
because requests are prohibited on obsolete attachments, it's because we make
the reasonable assumption that users will usually want such requests cancelled,
so we save them time by doing it for them.

Although b.m.o's flags are the kind that would probably never be requested on
an obsolete attachment, we don't know how other installations use flags and
"obsolete".  Some installations may have flags that apply to obsolete
attachments, or "obsolete" may not mean exactly the same thing on some
installations, and "obsolete" attachments may still see some activity.

Unlike product/component-specific flags, which we have to prohibit on
attachments that move out of the specific products/components for which the
flag is valid, we don't have to prohibit requests on obsolete attachments, and
some installations should have a use for it, so we should continue to enable
users to set such flags.

Basically, the goal here is to save our users time by doing the most likely
thing for them automatically.  We don't want to second-guess them, though.  If
a user wants to request a flag on an obsolete attachment, whether she's
requesting it while obsoleting the attachment or afterwards, we should let her.

Comment 22

[alanjstr]

So, to sound like a typical Firefox user, how about a preference/constant for that?

if (remove_flags_on_obsolete) {}

default it to true and have a deployment note.

Comment 23

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

We could have a pref to turn this on, but the behavior being turned on should
still be to cancel pending requests when an attachment is obsoleted but not
cancel or ignore requests explicitly set on the attachment after the attachment
is obsoleted or while it is being obsoleted.

Comment 24

[Frédéric Buclin]

Attachment: patch, v4

Comment 25

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

Comment on attachment 177435 [details] [diff] [review]
patch, v4

>Index: attachment.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v
>retrieving revision 1.74
>diff -u -r1.74 attachment.cgi
>--- attachment.cgi	9 Mar 2005 16:18:03 -0000	1.74
>+++ attachment.cgi	14 Mar 2005 23:36:58 -0000
>@@ -968,15 +968,13 @@
>   # Make existing attachments obsolete.
>   my $fieldid = GetFieldID('attachments.isobsolete');
>   foreach my $obsolete_id (@{$::MFORM{'obsolete'}}) {
>+      # If the obsolete attachment has request flags, cancel them.
>+      # This call must be done before updating the 'attachments' table.
>+      Bugzilla::Flag::CancelRequests($::FORM{'bugid'}, $obsolete_id, $sql_timestamp);

Nit: actually, CancelRequests doesn't need to be done first in this case,
because there's no way for users to request flags on other attachments while
attaching a new file.  It still makes sense to cancel requests first here,
since that keeps it consistent with other ways of changing attachments, but
it's not necessary.


>+
>       SendSQL("UPDATE attachments SET isobsolete = 1 WHERE attach_id = $obsolete_id");
>       SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, fieldid, removed, added) 
>                VALUES ($::FORM{'bugid'}, $obsolete_id, $::userid, $sql_timestamp, $fieldid, '0', '1')");
>-      # If the obsolete attachment has pending flags, migrate them to the new attachment.
>-      if (Bugzilla::Flag::count({ 'attach_id' => $obsolete_id ,
>-                                  'status' => 'pending',
>-                                  'is_active' => 1 })) {
>-        Bugzilla::Flag::migrate($obsolete_id, $attachid, $timestamp);
>-      }
>   }
> 
>   # Assign the bug to the user, if they are allowed to take it
>@@ -1150,8 +1148,17 @@
>   my $quotedcontenttype = SqlQuote($::FORM{'contenttype'});
>   my $quotedfilename = SqlQuote($::FORM{'filename'});
> 
>+  # Figure out when the changes were made.
>+  SendSQL("SELECT NOW()");
>+  my $timestamp = FetchOneColumn();
>+    
>+  # Update flags. These calls must be done before updating the
>+  # 'attachments' table due to the deletion of request flags
>+  # on attachments being obsoleted.

Nit:

Update flags.  We have to do this before committing changes
to attachments so that we can delete pending requests if the user
is obsoleting this attachment without deleting any requests
the user submits at the same time.


>+    # Cancel old request flags if we are obsoleting an attachment.

Nit: old request flags -> pending requests


>+    foreach my $flag_id (@$flag_ids) {
>+        clear($flag_id);
>+    }

Nit: per convention with short conditionals:

    foreach my $flag_id (@$flag_ids) { clear($flag_id) }

Looks good, works well. r=myk

Comment 26

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

I think ultimately we will want this not to cancel any pending requests if the
user makes any flag changes, or possibly never to cancel pending requests from
the edit attachment page.  It's clear that users want to cancel pending requests
on attachments being obsoleted by new attachments.  It's unclear whether they
want this on the edit attachment form, especially when they are setting flags
and/or making requests, at which point they clearly have some vision for how
flags on the attachment should look and are probably setting them the way they want.

But we can work that out in another bug.

Comment 27

[Shane H. W. Travis]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.75; previous revision: 1.74
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.30; previous revision: 1.29
done

Comment 28

[Frédéric Buclin]

Attachment: Fix nits

Fix nits reported by myk.

Comment 29

[Max Kanat-Alexander]

*** Bug 293502 has been marked as a duplicate of this bug. ***

Comment 30

[Max Kanat-Alexander]

Added to the release notes in bug 286274.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

It appears that when a patch is obsoleted, and the review requests for it are
cancelled, that no emails are sent to the reviewers telling them of the
cancellations.  Is that as intended?  

Comment 32

[Frédéric Buclin]

Attachment: doc patch, v1

Comment 33

[Colin Ogilvie [:cso]]

Comment on attachment 254751 [details] [diff] [review]
doc patch, v1

>+      Note that marking an attachment as obsolete automatically cancels all
>+      pending requests for this attachment.

this -> the

r=me

Comment 34

[Frédéric Buclin]

tip:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.64; previous revision: 1.63
done

2.22.2:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.37.2.18; previous revision: 1.37.2.17
done

2.20.4:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.20; previous revision: 1.33.2.19
done