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.

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. ***

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

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! ;)

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 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.

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 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;

(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?

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 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.

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.

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

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

Fix nits reported by myk.

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

Added to the release notes in bug 286274.

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 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

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