Closed Bug 180652 Opened 17 years ago Closed 15 years ago

Marking an attachment as obsolete should cancel all unfulfilled requests

Categories

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

2.17.1
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: bbaetz, Assigned: LpSolit)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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.  ;)
*** Bug 180736 has been marked as a duplicate of this bug. ***
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.
Depends on: 180833
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?
This should also resolve bug 180812 (also on the meta-bug)
*** Bug 232217 has been marked as a duplicate of this bug. ***
Assignee: myk → LpSolit
Status: NEW → ASSIGNED
Adding bug 237209 as this last one cannot be resolved if this particular point
is not discussed (and solved) first.
Blocks: 237209
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.
What exactly are you trying to see?  Bugs that have obsolete patches with no
outstanding requests?
I'm not trying to "see" anything, I just want obsolete patches to have the
strikethrough effect if they appear on the request queue.
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).
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).
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?"
(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! ;)
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.20
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #173702 - Flags: review?(myk)
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.
Attachment #173702 - Flags: review?(myk) → review-
No longer blocks: 237209
No longer depends on: 180833
Attached patch patch, v2 (obsolete) — Splinter Review
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.
Attachment #173702 - Attachment is obsolete: true
Attachment #176308 - Flags: review?(myk)
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;
Attachment #176308 - Flags: review?(myk) → review-
(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?
Attached patch patch, v3 (obsolete) — Splinter Review
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. :)
Attachment #176308 - Attachment is obsolete: true
Attachment #177321 - Flags: review?(myk)
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.
Attachment #177321 - Flags: review?(myk) → review-
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.
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.
Attached patch patch, v4Splinter Review
Attachment #177321 - Attachment is obsolete: true
Attachment #177435 - Flags: review?(myk)
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
Attachment #177435 - Flags: review?(myk) → review+
Flags: approval+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: documentation?
Keywords: relnote
Resolution: --- → FIXED
Attached patch Fix nitsSplinter Review
Fix nits reported by myk.
Attachment #177514 - Flags: review+
*** Bug 293502 has been marked as a duplicate of this bug. ***
Added to the release notes in bug 286274.
Keywords: relnote
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?  
Attached patch doc patch, v1Splinter Review
Attachment #254751 - Flags: review?(colin.ogilvie)
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
Attachment #254751 - Flags: review?(colin.ogilvie) → review+
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
Flags: documentation?
Flags: documentation2.22+
Flags: documentation2.20+
Flags: documentation+
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.