Fix/journals on direct uploads (#8933)

* linting

* use Carrierwave's replacement for filename

This avoids having the name altered by Carrierwave later on in the Attachments::FinishDirectUploadJob job where the file is fetched and then the attachment is stored.

* separate cases for no attachment and no local file

* correct journalizing on direct upload

In case the container of an attachment is journalized, the container needs to have added a journal to it when an attachment is uploaded. Without that, the attachment is only picked up in the journals once the container is altered again (possibly by a different user). This will then lead to a history which does not reflect the actual upload.

app/workers/attachments/finish_direct_upload_job.rb

@@ -43,14 +43,45 @@ class Attachments::FinishDirectUploadJob < ApplicationJob
     end
 
     begin
-      attachment.downloads = 0
-      attachment.set_file_size local_file unless attachment.filesize && attachment.filesize > 0
-      attachment.set_content_type local_file unless attachment.content_type.present?
-      attachment.set_digest local_file unless attachment.digest.present?
-
-      attachment.save! if attachment.changed?
+      set_attributes_from_file(attachment, local_file)
+      save_attachment(attachment)
+      journalize_container(attachment)
     ensure
       File.unlink(local_file.path) if File.exist?(local_file.path)
     end
   end
+
+  private
+
+  def set_attributes_from_file(attachment, local_file)
+    attachment.downloads = 0
+    attachment.set_file_size local_file unless attachment.filesize && attachment.filesize > 0
+    attachment.set_content_type local_file unless attachment.content_type.present?
+    attachment.set_digest local_file unless attachment.digest.present?
+  end
+
+  def save_attachment(attachment)
+    User.execute_as(attachment.author) do
+      attachment.save! if attachment.changed?
+    end
+  end
+
+  def journalize_container(attachment)
+    journable = attachment.container
+
+    return unless journable&.class&.journaled?
+
+    # Touching the journable will lead to the journal created next having its own timestamp.
+    # That timestamp will not adequately reflect the time the attachment was uploaded. This job
+    # right here might be executed way later than the time the attachment was uploaded. Ideally,
+    # the journals would be created bearing the time stamps of the attachment's created_at.
+    # This remains a TODO.
+    # But with the timestamp update in place as it is, at least the collapsing of aggregated journals
+    # from days before with the newly uploaded attachment is prevented.
+    journable.touch
+
+    Journals::CreateService
+      .new(journable, attachment.author)
+      .call
+  end
 end

spec/workers/attachments/finish_direct_upload_job_integration_spec.rb

@@ -0,0 +1,113 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe Attachments::FinishDirectUploadJob, 'integration', type: :job do
+  let!(:pending_attachment) do
+    FactoryBot.create(:attachment,
+                      downloads: -1,
+                      digest: '',
+                      container: container)
+  end
+
+  let(:job) { described_class.new }
+
+  shared_examples_for 'turning pending attachment into a standard attachment' do
+    it do
+      job.perform(pending_attachment.id)
+
+      attachment = Attachment.find(pending_attachment.id)
+
+      expect(attachment.downloads)
+        .to eql(0)
+      expect(attachment.content_type)
+        .to eql('application/binary')
+      expect(attachment.digest)
+        .to eql("9473fdd0d880a43c21b7778d34872157")
+    end
+  end
+
+  shared_examples_for "adding a journal to the attachment in the name of the attachment's author" do
+    it do
+      job.perform(pending_attachment.id)
+
+      journals = Attachment.find(pending_attachment.id).journals
+
+      expect(journals.count)
+        .to eql(2)
+
+      expect(journals.last.user)
+        .to eql(pending_attachment.author)
+    end
+  end
+
+  context 'for a journalized container' do
+    let!(:container) { FactoryBot.create(:work_package) }
+    let!(:container_timestamp) { container.updated_at }
+
+    it_behaves_like 'turning pending attachment into a standard attachment'
+    it_behaves_like "adding a journal to the attachment in the name of the attachment's author"
+
+    it "adds a journal to the container in the name of the attachment's author" do
+      job.perform(pending_attachment.id)
+
+      journals = container.journals.reload
+
+      expect(journals.count)
+        .to eql(2)
+
+      expect(journals.last.user)
+        .to eql(pending_attachment.author)
+
+      expect(journals.last.created_at > container_timestamp)
+        .to be_truthy
+
+      container.reload
+
+      expect(container.lock_version)
+        .to eql 1
+    end
+  end
+
+  context 'for a non journalized container' do
+    let!(:container) { FactoryBot.create(:wiki_page) }
+
+    it_behaves_like 'turning pending attachment into a standard attachment'
+    it_behaves_like "adding a journal to the attachment in the name of the attachment's author"
+  end
+
+  context 'for a nil container' do
+    let!(:container) { nil }
+
+    it_behaves_like 'turning pending attachment into a standard attachment'
+    it_behaves_like "adding a journal to the attachment in the name of the attachment's author"
+  end
+end