Closed
Bug 99215
Opened 23 years ago
Closed 17 years ago
Attachments have no midair collision protection
Categories
(Bugzilla :: Attachments & Requests, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: dbaron, Assigned: LpSolit)
References
Details
Attachments
(2 files, 1 obsolete file)
14.39 KB,
patch
|
mkanat
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
773 bytes,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
(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.
Reporter | ||
Updated•23 years ago
|
Version: 2.14 → 2.15
Comment 1•23 years ago
|
||
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.
Comment 2•23 years ago
|
||
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.
Comment 3•23 years ago
|
||
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.
Updated•23 years ago
|
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Comment 4•23 years ago
|
||
(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
Comment 5•22 years ago
|
||
Nominating for 2.16 blocker - this really isn't ideal. Gerv
Severity: normal → blocker
Comment 6•22 years ago
|
||
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?
Comment 7•22 years ago
|
||
We should do the quick fix and just update the bug's timestamp. Luckily, this is also the right fix.
Comment 8•22 years ago
|
||
MattyT: are you able to produce a patch to that effect? Gerv
Comment 9•22 years ago
|
||
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...
Comment 10•22 years ago
|
||
Creating two attachments should definitely midair - all duplicate actions should midair because they can be mistakes.
Comment 11•22 years ago
|
||
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.
Comment 12•22 years ago
|
||
Yes, attachments.creation_ts is indeed to the last modified time.
Comment 13•22 years ago
|
||
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
Updated•21 years ago
|
OS: Linux → All
Hardware: PC → All
Comment 15•20 years ago
|
||
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
Updated•20 years ago
|
Summary: attachment status changes have no midair collision protection → flag changes have no midair collision protection
Comment 16•19 years ago
|
||
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 → ---
Assignee | ||
Comment 17•19 years ago
|
||
*** Bug 212180 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 18•19 years ago
|
||
*** Bug 233393 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 19•19 years ago
|
||
(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
Assignee | ||
Comment 20•19 years ago
|
||
I won't have time to play with it before the 2.22 freeze
Target Milestone: Bugzilla 2.22 → ---
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Comment 22•18 years ago
|
||
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
Comment 24•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Summary: flag changes have no midair collision protection → Attachments have no midair collision protection
Assignee | ||
Comment 25•17 years ago
|
||
Attachment #280247 -
Flags: review?(wicked)
Assignee | ||
Comment 26•17 years ago
|
||
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 27•17 years ago
|
||
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-
Assignee | ||
Comment 28•17 years ago
|
||
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 29•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Attachment #282039 -
Flags: review?(justdave)
Comment 30•17 years ago
|
||
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+
Comment 31•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: approval+
Assignee | ||
Comment 32•17 years ago
|
||
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
Assignee | ||
Comment 33•17 years ago
|
||
The DB index is omitted when upgrading, making Tinderbox to fail. Oops!
Attachment #290760 -
Flags: review?(mkanat)
Assignee | ||
Comment 34•17 years ago
|
||
mkanat, could you review the 2nd fix about the missing index?
Comment 35•17 years ago
|
||
Comment on attachment 290760 [details] [diff] [review] add missing index when upgrading, v1 Looks good to me.
Attachment #290760 -
Flags: review?(mkanat) → review+
Assignee | ||
Comment 36•17 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•