Closed Bug 99215 Opened 23 years ago Closed 17 years ago

Attachments have no midair collision protection

Categories

(Bugzilla :: Attachments & Requests, defect)

2.15
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: dbaron, Assigned: LpSolit)

References

Details

Attachments

(2 files, 1 obsolete file)

(I'm reporting this in whatever version of bugzilla http://bugzilla.mozilla.org/
runs as of 10 minutes ago.)

Changes to attachment status have no midair collision detection.  This causes
problems like the one seen in
http://bugzilla.mozilla.org/show_activity.cgi?id=93164 at 2001-09-10 17:02.

Steps to reproduce:
 * Click the "Edit" link for one attachment in one browser
 * do the same in another browser
 * check "has review" in the first, and submit
 * check "has super-review" in the second, and submit

Result:
 * the attachment no longer has review

Expected result alternative #1:
 * midair collision report

Expected result alternative #2:
 * the attachment has both review and super-review

I prefer #1 because of the possibility of withdrawing review for a bug (or the
possibility of one person marking an attachment obsolete as another is
reviewing) -- if it were not possible to unset the states once they were set
then #2 would be preferable.
Version: 2.14 → 2.15
I'll agree here, but do we want midair protection over the whole bug?  Two
create attachments currently clash ... I think a change to one attachment should
clash with a change to another attachment, because it's all the same bug.
The purpose of midair protection is to prevent one user's changes from
overwriting another's, not to prevent two people from changing the same bug at
once.  Simultaneous changes to two different attachments at the same time do not
have the potential to collide in midair and thus do not need midair protection.
AIUI the purpose of midair is to prevent a second person making a change
unnecessary after the first change.  There could be situations where this could
occur on separate attachments or between an attachment and the bug.

If what you say was true, we would have attempted to make midair collisions not
occur when different fields on the same bug were touched.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
(side note)It is sometimes useful not to clash, eg someone adding themselves to
the cc list shouldn't clash with a summary change.

If there are no fields in common (or the fields are lits, like
cc/dependancy/etc), I'd like a "keep going" option on the midair screen. Thats a
separate bug, though
Nominating for 2.16 blocker - this really isn't ideal.

Gerv
Severity: normal → blocker
Let me get this straight...  we want midair collisions to occur when two people
edit the same attachment, but not when people edit different attachments or when
one person edits the bug while someone else is editing an attachment?

So basically, we need a timestamp field for last modified on each attachment
record as well?
We should do the quick fix and just update the bug's timestamp.  Luckily, this
is also the right fix.
MattyT: are you able to produce a patch to that effect?

Gerv
Well, commenting should check the bug while attachment statues should check the
attachment. However, in practice can't we just check the bug? An extra midair
when editing a different attachment won't hurt, and is probably sufficiently
rare that its not worth adding an extra tiemstamp field to the attachment table.

The obsolete stuff from creating an attachment is sort of protected, because it
errors out if one of the attachments is already obsolete. (Although of course it
doesn't do any locking, so its not really safe. Setting isobsolete on an
attachment which is already obsolete won't hurt, though)

Should creating two different attachments midair?

Theres attachments.creation_ts - is that really the last-modified time? It looks
that way, and thats what the header in Attachment.pm::List says...
Creating two attachments should definitely midair - all duplicate actions should
midair because they can be mistakes.
myk: see my laste paragraph in comment 9. If what we're doing is correct, then I
can just use that.

Do note that going back after having done "edit attachment as comment" in
mozilla will lose your comments. I'm not sure if there is a mozilla bug filed on
that.
Yes, attachments.creation_ts is indeed to the last modified time.
Would be very nice, but no patch, not a stopper, and probably a fair bit of work
to check all the cases.

->2.18
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Component: Creating/Changing Bugs → attachment and request management
OS: Linux → All
Hardware: PC → All
Definitely not a blocker.

Gerv
Severity: blocker → major
All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Summary: attachment status changes have no midair collision protection → flag changes have no midair collision protection
This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to nobody@bugzilla.org or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.
Target Milestone: Bugzilla 2.20 → ---
*** Bug 212180 has been marked as a duplicate of this bug. ***
*** Bug 233393 has been marked as a duplicate of this bug. ***
(In reply to comment #12)
> Yes, attachments.creation_ts is indeed to the last modified time.

Oh really? Actually, creation_ts is the creation date of the attachment and an
additional field similar to delta_ts is required in the attachments table in
order to fix this bug correctly.

It does not make sense to midair when the bug or another attachment is being
modified at the same time. And marking an attachment as obsolete when submitting
a new attachment should update the delta_ts-like field of the obsoleted
attachment in order to correctly reflect the change.

We are frozen and 2.20rc1 should be released this week. So pushing it to 2.22,
unfortunately.
Assignee: myk → LpSolit
Severity: major → normal
Priority: P1 → --
Target Milestone: --- → Bugzilla 2.22
I won't have time to play with it before the 2.22 freeze
Target Milestone: Bugzilla 2.22 → ---
QA Contact: mattyt-bugzilla → default-qa
Attachments actually have working independent creation and modification dates now (they didn't the last time there was major discussion on this bug) so this should actually be easily doable now.  Too much change to shoehorn into 3.0 though, we're frozen.
Target Milestone: --- → Bugzilla 3.2
Here's another recent example.
I received a review request on a patch.  So I went to the bug, clicked
on the "details" link for the patch, spent about 20 minutes writing a 
review and setting the review flag on the attachment details page, and 
then I clicked submit.

While I had been writing that review, the author of the patch found a bug
in the patch I was reviewing, and submitted another patch, and marked the
patch I was reviewing obsolete, which cancelled the review request to me
as well as to another reviewer.  But I did not know any of that had happened.

So, when I submitted my review, submitting the contents of the details
page, I effectively undid his recent changes.  My submission removed the
obsolete flag from the patch I reviewed, and it restored the review request
to the other reviewer for the patch I had reviewed.  I received no warning
from the b.m.o web server about any of this.  

I learned about the collision when I received the email from b.m.o, telling
me of the changes I had made.  There I saw that it appeared at I had 
unobsoleted the patch, and that I had requested that another reviewer 
also review that patch. 
Summary: flag changes have no midair collision protection → Attachments have no midair collision protection
Attached patch patch, v1 (obsolete) — Splinter Review
Attachment #280247 - Flags: review?(wicked)
Comment on attachment 280247 [details] [diff] [review]
patch, v1

Max, could you look at the DB and upgrade parts of the patch?
Myk, could you look at the attachment part?
Attachment #280247 - Flags: review?(myk)
Attachment #280247 - Flags: review?(mkanat)
Comment on attachment 280247 [details] [diff] [review]
patch, v1

>         INDEXES => [
>             attachments_bug_id_idx => ['bug_id'],
>             attachments_creation_ts_idx => ['creation_ts'],
>+            attachments_modif_time_idx => ['modification_time'],

  Index names *must* contain the full name of the first field.

>Index: Bugzilla/Install/DB.pm
> [snip]
>+sub _fix_attachment_modification_date {
>+    my $dbh = Bugzilla->dbh;
>+    $dbh->bz_add_column('attachments', 'modification_time', {TYPE => 'DATETIME'});
>+
>+    my $attachments_with_no_modif_time =
>+      $dbh->selectcol_arrayref('SELECT attach_id FROM attachments
>+                                WHERE modification_time IS NULL');

  Just check if the column exists, and then add it and do these things if it doesn't exist.

>+    print "Setting the modification time for attachments having none...\n";
>+    foreach my $attach_id (@$attachments_with_no_modif_time) {
>+        my $time = $dbh->selectrow_array($sth_select, undef, $attach_id);
>+        if ($time) {
>+            $sth_update->execute($time, $attach_id);
>+        }
>+        else {
>+            # The attachment has never been edited. Use the creation time.
>+            $sth_sync->execute($attach_id);
>+        }

  Wait, if this is set to creation_ts when the attachment hasn't been modified, then why is modified_time allowed to be NULL? Either it should be NOT NULL or NULL should represent "never modified". (I prefer the second, but if the first makes sense, that's fine too. You could always put that logic into Attachment.pm instead of into the DB, though.)
Attachment #280247 - Flags: review?(mkanat) → review-
Attached patch patch, v2Splinter Review
mkanat: DB + upgrade parts; myk: attachment part.
Attachment #280247 - Attachment is obsolete: true
Attachment #280247 - Flags: review?(wicked)
Attachment #280247 - Flags: review?(myk)
Attachment #282039 - Flags: review?(myk)
Attachment #282039 - Flags: review?(mkanat)
Comment on attachment 282039 [details] [diff] [review]
patch, v2

Yeah, the DB part looks fine.

At the bottom of the upgrade code, you might want to make that foreach loop a bit clearer, for people who aren't perl gurus.
Attachment #282039 - Flags: review?(mkanat) → review+
Attachment #282039 - Flags: review?(justdave)
Blocks: 401007
Comment on attachment 282039 [details] [diff] [review]
patch, v2

ok, this works as designed on the trunk in my testing. My only concern is the text "This will cause all of the above changes to be overwritten" next to the Submit anyway button, but we have the same text on the bug midair page, too, and I'm sure there's another bug on that somewhere (because it's not always accurate)
Attachment #282039 - Flags: review?(myk)
Attachment #282039 - Flags: review?(justdave)
Attachment #282039 - Flags: review+
They want this on b.m.o, but we're not upgrading to tip yet, which means we get to backport this patch.  It *almost* applies cleanly on the 3.0 branch, and the one conflict was painless to apply manually, however there must be API changes somewhere between 3.0 and trunk that affect this because it doesn't work on the branch (no errors anywhere, but the midair is never triggered).

For the record, I don't want a 3.0 backport committed to CVS, it does schema changes, and I don't want schema changes on the branch unless necessary for a security issue.
Flags: approval+
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.137; previous revision: 1.136
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.394; previous revision: 1.393
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.51; previous revision: 1.50
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.218; previous revision: 1.217
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.90; previous revision: 1.89
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.109; previous revision: 1.108
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.49; previous revision: 1.48
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/midair.html.tmpl,v
done
Checking in template/en/default/attachment/midair.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/midair.html.tmpl,v  <--  midair.html.tmpl
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: relnote
Resolution: --- → FIXED
The DB index is omitted when upgrading, making Tinderbox to fail. Oops!
Attachment #290760 - Flags: review?(mkanat)
mkanat, could you review the 2nd fix about the missing index?
Comment on attachment 290760 [details] [diff] [review]
add missing index when upgrading, v1

Looks good to me.
Attachment #290760 - Flags: review?(mkanat) → review+
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.44; previous revision: 1.43
done
Blocks: 423346
Added to the Bugzilla 3.2 release notes in bug 432331.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.