Bug 99215: Attachments have no midair collision protection - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat r=justdave a=justdave

9 files changed, 167 insertions, 10 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index cc3e16893..e3fe39f3a 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -91,6 +91,7 @@ sub _retrieve {
         'attachments.submitter_id AS _attacher_id',
         Bugzilla->dbh->sql_date_format('attachments.creation_ts',
                                        '%Y.%m.%d %H:%i') . " AS attached",
+        'attachments.modification_time',
         'attachments.filename AS filename',
         'attachments.ispatch AS ispatch',
         'attachments.isurl AS isurl',
@@ -208,6 +209,21 @@ sub attached {
 
 =over
 
+=item C<modification_time>
+
+the date and time on which the attachment was last modified.
+
+=back
+
+=cut
+
+sub modification_time {
+    my $self = shift;
+    return $self->{modification_time};
+}
+
+=over
+
 =item C<filename>
 
 the name of the file the attacher attached
@@ -826,10 +842,10 @@ sub insert_attachment_for_bug {
     # Insert the attachment into the database.
     my $sth = $dbh->do(
         "INSERT INTO attachments
-            (bug_id, creation_ts, filename, description,
+            (bug_id, creation_ts, modification_time, filename, description,
              mimetype, ispatch, isurl, isprivate, submitter_id)
-         VALUES (?,?,?,?,?,?,?,?,?)", undef, ($bug->bug_id, $timestamp, $filename,
-              $description, $contenttype, $cgi->param('ispatch'),
+         VALUES (?,?,?,?,?,?,?,?,?,?)", undef, ($bug->bug_id, $timestamp, $timestamp,
+              $filename, $description, $contenttype, $cgi->param('ispatch'),
               $isurl, $isprivate, $user->id));
     # Retrieve the ID of the newly created attachment record.
     my $attachid = $dbh->bz_last_key('attachments', 'attach_id');
@@ -877,8 +893,9 @@ sub insert_attachment_for_bug {
         # This call must be done before updating the 'attachments' table.
         Bugzilla::Flag::CancelRequests($bug, $obsolete_attachment, $timestamp);
 
-        $dbh->do('UPDATE attachments SET isobsolete = 1 WHERE attach_id = ?',
-                 undef, $obsolete_attachment->id);
+        $dbh->do('UPDATE attachments SET isobsolete = 1, modification_time = ?
+                  WHERE attach_id = ?',
+                 undef, ($timestamp, $obsolete_attachment->id));
 
         $dbh->do('INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when,
                                              fieldid, removed, added)
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index a8f1ede5d..0a45daf14 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -2573,11 +2573,11 @@ sub format_comment {
 # Get the activity of a bug, starting from $starttime (if given).
 # This routine assumes ValidateBugID has been previously called.
 sub GetBugActivity {
-    my ($id, $starttime) = @_;
+    my ($bug_id, $attach_id, $starttime) = @_;
     my $dbh = Bugzilla->dbh;
 
     # Arguments passed to the SQL query.
-    my @args = ($id);
+    my @args = ($bug_id);
 
     # Only consider changes since $starttime, if given.
     my $datepart = "";
@@ -2587,6 +2587,12 @@ sub GetBugActivity {
         $datepart = "AND bugs_activity.bug_when > ?";
     }
 
+    my $attachpart = "";
+    if ($attach_id) {
+        push(@args, $attach_id);
+        $attachpart = "AND bugs_activity.attach_id = ?";
+    }
+
     # Only includes attachments the user is allowed to see.
     my $suppjoins = "";
     my $suppwhere = "";
@@ -2616,6 +2622,7 @@ sub GetBugActivity {
             ON profiles.userid = bugs_activity.who
          WHERE bugs_activity.bug_id = ?
                $datepart
+               $attachpart
                $suppwhere
       ORDER BY bugs_activity.bug_when";
 
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 1c6ee352e..61f894f83 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -372,6 +372,7 @@ use constant ABSTRACT_SCHEMA => {
                              PRIMARYKEY => 1},
             bug_id       => {TYPE => 'INT3', NOTNULL => 1},
             creation_ts  => {TYPE => 'DATETIME', NOTNULL => 1},
+            modification_time => {TYPE => 'DATETIME', NOTNULL => 1},
             description  => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
             mimetype     => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
             ispatch      => {TYPE => 'BOOLEAN'},
@@ -389,6 +390,7 @@ use constant ABSTRACT_SCHEMA => {
         INDEXES => [
             attachments_bug_id_idx => ['bug_id'],
             attachments_creation_ts_idx => ['creation_ts'],
+            attachments_modification_time_idx => ['modification_time'],
             attachments_submitter_id_idx => ['submitter_id', 'bug_id'],
         ],
     },
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index 9934e058f..485c771b1 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -515,6 +515,9 @@ sub update_table_definitions {
     # 2007-08-21 wurblzap@gmail.com - Bug 365378
     _make_lang_setting_dynamic();
 
+    # 2007-09-09 LpSolit@gmail.com - Bug 99215
+    _fix_attachment_modification_date();
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
@@ -2898,6 +2901,31 @@ sub _make_lang_setting_dynamic {
     }
 }
 
+sub _fix_attachment_modification_date {
+    my $dbh = Bugzilla->dbh;
+    if (!$dbh->bz_column_info('attachments', 'modification_time')) {
+        # Allow NULL values till the modification time has been set.
+        $dbh->bz_add_column('attachments', 'modification_time', {TYPE => 'DATETIME'});
+
+        print "Setting the modification time for attachments...\n";
+        $dbh->do('UPDATE attachments SET modification_time = creation_ts');
+
+        # Now force values to be always defined.
+        $dbh->bz_alter_column('attachments', 'modification_time',
+                              {TYPE => 'DATETIME', NOTNULL => 1});
+
+        # Update the modification time for attachments which have been modified.
+        my $attachments =
+          $dbh->selectall_arrayref('SELECT attach_id, MAX(bug_when) FROM bugs_activity
+                                    WHERE attach_id IS NOT NULL ' .
+                                    $dbh->sql_group_by('attach_id'));
+
+        my $sth = $dbh->prepare('UPDATE attachments SET modification_time = ?
+                                 WHERE attach_id = ?');
+        $sth->execute($_->[1], $_->[0]) foreach (@$attachments);
+    }
+}
+
 1;
 
 __END__
diff --git a/attachment.cgi b/attachment.cgi
index 23841571c..768653c31 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -460,6 +460,27 @@ sub update {
     $cgi->param('isobsolete', $cgi->param('isobsolete') ? 1 : 0);
     $cgi->param('isprivate', $cgi->param('isprivate') ? 1 : 0);
 
+    # Now make sure the attachment has not been edited since we loaded the page.
+    if (defined $cgi->param('delta_ts')
+        && $cgi->param('delta_ts') ne $attachment->modification_time)
+    {
+        ($vars->{'operations'}) =
+            Bugzilla::Bug::GetBugActivity($bug->id, $attachment->id, $cgi->param('delta_ts'));
+
+        # If the modification date changed but there is no entry in
+        # the activity table, this means someone commented only.
+        # In this case, there is no reason to midair.
+        if (scalar(@{$vars->{'operations'}})) {
+            $cgi->param('delta_ts', $attachment->modification_time);
+            $vars->{'attachment'} = $attachment;
+
+            print $cgi->header();
+            # Warn the user about the mid-air collision and ask them what to do.
+            $template->process("attachment/midair.html.tmpl", $vars)
+              || ThrowTemplateError($template->error());
+            exit;
+        }
+    }
     # If the submitter of the attachment is not in the insidergroup,
     # be sure that he cannot overwrite the private bit.
     # This check must be done before calling Bugzilla::Flag*::validate(),
@@ -507,11 +528,12 @@ sub update {
                     filename    = ?,
                     ispatch     = ?,
                     isobsolete  = ?,
-                    isprivate   = ?
+                    isprivate   = ?,
+                    modification_time = ?
             WHERE   attach_id   = ?",
             undef, ($description, $contenttype, $filename,
             $cgi->param('ispatch'), $cgi->param('isobsolete'), 
-            $cgi->param('isprivate'), $attachment->id));
+            $cgi->param('isprivate'), $timestamp, $attachment->id));
 
   my $updated_attachment = Bugzilla::Attachment->get($attachment->id);
   # Record changes in the activity table.
diff --git a/process_bug.cgi b/process_bug.cgi
index 99ee5ed57..a8df416cd 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -878,7 +878,7 @@ foreach my $id (@idlist) {
     if (defined $cgi->param('delta_ts') && $cgi->param('delta_ts') ne $delta_ts)
     {
         ($vars->{'operations'}) =
-            Bugzilla::Bug::GetBugActivity($id, $cgi->param('delta_ts'));
+            Bugzilla::Bug::GetBugActivity($id, undef, $cgi->param('delta_ts'));
 
         $vars->{'start_at'} = $cgi->param('longdesclength');
 
diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl
index 23e104d86..3796b5650 100644
--- a/template/en/default/attachment/edit.html.tmpl
+++ b/template/en/default/attachment/edit.html.tmpl
@@ -202,6 +202,7 @@
   <input type="hidden" name="id" value="[% attachment.id %]">
   <input type="hidden" name="action" value="update">
   <input type="hidden" name="contenttypemethod" value="manual">
+  <input type="hidden" name="delta_ts" value="[% attachment.modification_time FILTER html %]">
 
   <table class="attachment_info" width="100%">
 
diff --git a/template/en/default/attachment/midair.html.tmpl b/template/en/default/attachment/midair.html.tmpl
new file mode 100644
index 000000000..8cde9f2f5
--- /dev/null
+++ b/template/en/default/attachment/midair.html.tmpl
@@ -0,0 +1,76 @@
+[%# The contents of this file are subject to the Mozilla Public
+  # License Version 1.1 (the "License"); you may not use this file
+  # except in compliance with the License. You may obtain a copy of
+  # the License at http://www.mozilla.org/MPL/
+  #
+  # Software distributed under the License is distributed on an "AS
+  # IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+  # implied. See the License for the specific language governing
+  # rights and limitations under the License.
+  #
+  # The Original Code is the Bugzilla Bug Tracking System.
+  #
+  # The Initial Developer of the Original Code is Netscape Communications
+  # Corporation. Portions created by Netscape are
+  # Copyright (C) 1998 Netscape Communications Corporation. All
+  # Rights Reserved.
+  #
+  # Contributor(s): Myk Melez <myk@mozilla.org>
+  #                 Frédéric Buclin <LpSolit@gmail.com>
+  #%]
+
+[%# INTERFACE:
+  # operations: array; bug activity since the user last displayed the attachment form,
+  #   used by bug/activity/table.html.tmpl to display recent changes that will
+  #   be overwritten if the user submits these changes.  See that template
+  #   for further documentation.
+  # attachment: object; the attachment being changed.
+  #%]
+
+[%# The global Bugzilla->cgi object is used to obtain form variable values. %]
+[% USE Bugzilla %]
+[% cgi = Bugzilla.cgi %]
+
+[% PROCESS global/variables.none.tmpl %]
+[% PROCESS global/header.html.tmpl title = "Mid-air collision!" %]
+
+<h1>Mid-air collision detected!</h1>
+
+<p>
+  Someone else has made changes to
+  <a href="attachment.cgi?id=[% attachment.id %]&amp;action=edit">attachment [% attachment.id %]</a>
+  of [% "$terms.bug $attachment.bug_id" FILTER bug_link(attachment.bug_id) FILTER none %]
+  at the same time you were trying to. The changes made were:
+</p>
+
+<p>
+  [% PROCESS "bug/activity/table.html.tmpl" incomplete_data=0 %]
+</p>
+
+[% IF cgi.param("comment") %]
+<p>
+  Your comment was:<br>
+  <blockquote><pre>[% cgi.param("comment") FILTER wrap_comment FILTER html %]</pre></blockquote>
+</p>
+[% END %]
+
+<p>
+You have the following choices:
+</p>
+
+<ul>
+  <li>
+    <form method="post" action="attachment.cgi">
+      [% PROCESS "global/hidden-fields.html.tmpl" exclude="^Bugzilla_(login|password)$" %]
+      <input type="submit" id="process" value="Submit my changes anyway">
+        This will cause all of the above changes to be overwritten.
+    </form>
+  </li>
+  <li>
+    Throw away my changes, and
+    <a href="attachment.cgi?id=[% attachment.id %]&amp;action=edit">revisit
+    attachment [% attachment.id %]</a>
+  </li>
+</ul>
+
+[% PROCESS global/footer.html.tmpl %]
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index 80957e6c7..e2acdcbca 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -416,6 +416,10 @@
   'obsolete_attachments',
 ],
 
+'attachment/midair.html.tmpl' => [
+  'attachment.id',
+],
+
 'attachment/show-multiple.html.tmpl' => [
   'a.id',
   'flag.status'