diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb
index 1b98db82c016..d32a96f3dbf5 100644
--- a/app/services/journals/create_service.rb
+++ b/app/services/journals/create_service.rb
@@ -428,13 +428,15 @@ def insert_storable_sql
storages_file_links_journals (
journal_id,
file_link_id,
- link_name
+ link_name,
+ storage_name
)
SELECT
#{id_from_inserted_journal_sql},
file_links.id,
- file_links.origin_name
- FROM file_links
+ file_links.origin_name,
+ storages.name
+ FROM file_links left join storages ON file_links.storage_id = storages.id
WHERE
#{only_if_created_sql}
AND file_links.container_id = :journable_id
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 571845d734ec..5280a0232e0d 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2915,12 +2915,16 @@ en:
text_work_packages_destroy_confirmation: "Are you sure you want to delete the selected work package(s)?"
text_work_packages_ref_in_commit_messages: "Referencing and fixing work packages in commit messages"
text_journal_added: "%{label} %{value} added"
+ text_journal_attachment_added: "%{label} %{value} added as attachment"
+ text_journal_attachment_deleted: "%{label} %{old} removed as attachment"
text_journal_changed_plain: "%{label} changed from %{old} %{linebreak}to %{new}"
text_journal_changed_no_detail: "%{label} updated"
text_journal_changed_with_diff: "%{label} changed (%{link})"
text_journal_deleted: "%{label} deleted (%{old})"
text_journal_deleted_subproject: "%{label} %{old}"
text_journal_deleted_with_diff: "%{label} deleted (%{link})"
+ text_journal_file_link_added: "%{label} link to %{value} (%{storage}) added"
+ text_journal_file_link_deleted: "%{label} link to %{old} (%{storage}) removed"
text_journal_of: "%{label} %{value}"
text_journal_set_to: "%{label} set to %{value}"
text_journal_set_with_diff: "%{label} set (%{link})"
diff --git a/db/migrate/20230725165505_add_storage_name_to_storages_file_links_journals.rb b/db/migrate/20230725165505_add_storage_name_to_storages_file_links_journals.rb
new file mode 100644
index 000000000000..febcd3c9c929
--- /dev/null
+++ b/db/migrate/20230725165505_add_storage_name_to_storages_file_links_journals.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2023 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-2013 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 COPYRIGHT and LICENSE files for more details.
+#++
+
+class AddStorageNameToStoragesFileLinksJournals < ActiveRecord::Migration[7.0]
+ def change
+ add_column :storages_file_links_journals, :storage_name, :string
+ end
+end
diff --git a/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb b/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb
new file mode 100644
index 000000000000..1908ef5911c3
--- /dev/null
+++ b/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2023 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-2013 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 COPYRIGHT and LICENSE files for more details.
+#++
+
+class AddFileLinkJournalsToExistingContainers < ActiveRecord::Migration[7.0]
+ def up
+ system_user = SystemUser.first
+ containers = Storages::FileLink.includes(:container).map(&:container).uniq
+
+ containers.each do |container|
+ next unless container.class.journaled?
+
+ Journals::CreateService.new(container, system_user).call(notes: "File link activity added by a system update")
+ end
+ end
+
+ def down; end
+end
diff --git a/lib/open_project/journal_formatter/attachment.rb b/lib/open_project/journal_formatter/attachment.rb
index 9b323b8c4fb5..d900aec15909 100644
--- a/lib/open_project/journal_formatter/attachment.rb
+++ b/lib/open_project/journal_formatter/attachment.rb
@@ -44,11 +44,19 @@ def render(key, values, options = { html: true })
value = format_html_attachment_detail(key.to_s.sub('attachments_', ''), value)
end
- render_binary_detail_text(label, value, old_value)
+ render_attachment_detail_text(label, value, old_value)
end
private
+ def render_attachment_detail_text(label, value, old_value)
+ if value.blank?
+ I18n.t(:text_journal_attachment_deleted, label:, old: old_value)
+ else
+ I18n.t(:text_journal_attachment_added, label:, value:)
+ end
+ end
+
def label(_key)
Attachment.model_name.human
end
diff --git a/lib/open_project/journal_formatter/file_link.rb b/lib/open_project/journal_formatter/file_link.rb
index 93503b0339bd..8fa66e4e1f93 100644
--- a/lib/open_project/journal_formatter/file_link.rb
+++ b/lib/open_project/journal_formatter/file_link.rb
@@ -32,27 +32,64 @@ class OpenProject::JournalFormatter::FileLink < JournalFormatter::Base
include OpenProject::ObjectLinking
def render(key, values, options = { html: true })
- id = key.to_s.sub('file_links_', '')
- label, old_value, value = format_details(id, values)
+ id = key.to_s.sub('file_links_', '').to_i
+ label, old_value, value, storage = format_details(id, values)
if options[:html]
- label, old_value, value = *format_html_details(label, old_value, value)
+ label, old_value, value = format_html_details(label, old_value, value)
value = format_html_file_link_detail(id, value)
end
- render_binary_detail_text(label, value, old_value)
+ render_file_link_detail_text(label, value, old_value, storage)
end
private
+ def format_details(id, values)
+ old_value, current_value = values
+ [
+ label(nil),
+ old_value&.fetch('link_name'),
+ current_value&.fetch('link_name'),
+ storage(id, old_value, current_value)
+ ]
+ end
+
+ def storage(id, old, current)
+ non_nil_value = [old, current].compact.first
+ file_link = file_link_for(id, non_nil_value)
+
+ non_nil_value['storage_name'] || file_link&.storage&.name || I18n.t('storages.unknown_storage')
+ end
+
+ def render_file_link_detail_text(label, value, old_value, storage)
+ if value.blank?
+ I18n.t(:text_journal_file_link_deleted, label:, old: old_value, storage:)
+ else
+ I18n.t(:text_journal_file_link_added, label:, value:, storage:)
+ end
+ end
+
# Based this off the Attachment formatter. Not sure if it is the best approach
- def label(_key) = Storages::FileLink.model_name.human
+ def label(_key) = I18n.t('activerecord.models.file_link')
+
+ def file_link_for(key, value)
+ value.present? && ::Storages::FileLink.find_by(id: key)
+ end
def format_html_file_link_detail(key, value)
- if value.present? && file_link = ::Storages::FileLink.find_by(id: key.to_i)
+ if file_link = file_link_for(key, value)
link_to_file_link(file_link, only_path: false)
elsif value.present?
value
end
end
+
+ def link_to_file_link(file_link, options = {})
+ text = options.delete(:text) || file_link.origin_name
+
+ link_to text,
+ url_to_file_link(file_link, only_path: options.delete(:only_path) { true }),
+ options
+ end
end
diff --git a/lib/open_project/object_linking.rb b/lib/open_project/object_linking.rb
index 2dc213bceeac..e16af57663f7 100644
--- a/lib/open_project/object_linking.rb
+++ b/lib/open_project/object_linking.rb
@@ -79,14 +79,6 @@ def link_to_attachment(attachment, options = {})
options
end
- def link_to_file_link(file_link, options = {})
- text = options.delete(:text) || file_link.origin_name
-
- link_to text,
- url_to_file_link(file_link, only_path: options.delete(:only_path) { true }),
- options
- end
-
# Generates a link to a SCM revision
# Options:
# * :text - Link text (default to the formatted revision)
diff --git a/lib_static/plugins/acts_as_journalized/lib/acts/journalized/file_link_journal_differ.rb b/lib_static/plugins/acts_as_journalized/lib/acts/journalized/file_link_journal_differ.rb
new file mode 100644
index 000000000000..9b0bbefd20cf
--- /dev/null
+++ b/lib_static/plugins/acts_as_journalized/lib/acts/journalized/file_link_journal_differ.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2023 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-2013 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 COPYRIGHT and LICENSE files for more details.
+#++
+
+module Acts::Journalized
+ module FileLinkJournalDiffer
+ class << self
+ def get_changes_to_file_links(predecessor, storable_journals)
+ if predecessor.nil?
+ storable_journals.each_with_object({}) do |journal, hash|
+ change_key = "file_links_#{journal.file_link_id}"
+ new_values = { link_name: journal.link_name, storage_name: journal.storage_name }
+ hash[change_key] = [nil, new_values]
+ end
+ else
+ current_storables = storable_journals.map(&:attributes)
+ previous_storables = predecessor.storable_journals.map(&:attributes)
+
+ changes_on_file_links(previous_storables, current_storables)
+ end
+ end
+
+ def changes_on_file_links(previous, current)
+ ids = all_file_link_ids(previous, current)
+
+ cleanup_changes(
+ pair_changes(ids, previous, current)
+ ).transform_keys! { |key| "file_links_#{key}" }
+ end
+
+ def all_file_link_ids(previous, current)
+ current.pluck('file_link_id') | previous.pluck('file_link_id')
+ end
+
+ def cleanup_changes(changes) = changes.reject { |_, (first, last)| first == last }
+
+ def pair_changes(ids, previous, current)
+ ids.index_with do |id|
+ [select_journals(previous.select { |attributes| attributes['file_link_id'] == id }),
+ select_journals(current.select { |attributes| attributes['file_link_id'] == id })]
+ end
+ end
+
+ def select_journals(journals)
+ return if journals.empty?
+
+ journals.sort.map { |hash| hash.slice('link_name', 'storage_name') }.last
+ end
+ end
+ end
+end
diff --git a/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb b/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb
index f157f3814f15..10d6e9e78201 100644
--- a/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb
+++ b/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb
@@ -63,13 +63,9 @@ def get_changes
if has_file_links?
@changes.merge!(
- ::Acts::Journalized::JournableDiffer.association_changes(
+ ::Acts::Journalized::FileLinkJournalDiffer.get_changes_to_file_links(
predecessor,
- self,
- 'storable_journals',
- 'file_links',
- :file_link_id,
- :link_name
+ storable_journals
)
)
end
diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml
index 8dc1ffa8bd5f..3083831789ad 100644
--- a/modules/storages/config/locales/en.yml
+++ b/modules/storages/config/locales/en.yml
@@ -18,7 +18,7 @@ en:
activerecord:
models:
- file_link: "File link"
+ file_link: "File"
storages/storage: "Storage"
attributes:
storages/storage:
@@ -58,6 +58,7 @@ en:
too_many_elements_created_at_once: "Too many elements created at once. Expected %{max} at most, got %{actual}."
storages:
+ unknown_storage: "Unknown storage"
buttons:
done_continue_setup: "Done. Continue setup"
done_complete_setup: "Done, complete setup"
diff --git a/spec/lib/journal_formatter/attachment_spec.rb b/spec/lib/journal_formatter/attachment_spec.rb
index 1af4d6f26754..d7017aff8cc1 100644
--- a/spec/lib/journal_formatter/attachment_spec.rb
+++ b/spec/lib/journal_formatter/attachment_spec.rb
@@ -40,25 +40,19 @@ def self.default_url_options
{ only_path: true }
end
- let(:klass) { OpenProject::JournalFormatter::Attachment }
- let(:instance) { klass.new(journal) }
- let(:id) { 1 }
- let(:journal) do
- OpenStruct.new(id:)
- end
+ let(:journal) { instance_double(Journal, id: 1) }
let(:user) { create(:user) }
- let(:attachment) do
- create(:attachment,
- author: user)
- end
+ let(:attachment) { create(:attachment, author: user) }
let(:key) { "attachments_#{attachment.id}" }
+ subject(:instance) { described_class.new(journal) }
+
describe '#render' do
describe 'WITH the first value being nil, and the second an id as string' do
it 'adds an attachment added text' do
link = "#{Setting.protocol}://#{Setting.host_name}/api/v3/attachments/#{attachment.id}/content"
- expect(instance.render(key, [nil, attachment.id.to_s]))
- .to eq(I18n.t(:text_journal_added,
+ expect(instance.render(key, [nil, attachment.filename.to_s]))
+ .to eq(I18n.t(:text_journal_attachment_added,
label: "#{I18n.t(:'activerecord.models.attachment')}",
value: "#{attachment.filename}"))
end
@@ -72,8 +66,8 @@ def self.default_url_options
it 'adds an attachment added text' do
link = "#{Setting.protocol}://#{Setting.host_name}/blubs/api/v3/attachments/#{attachment.id}/content"
- expect(instance.render(key, [nil, attachment.id.to_s]))
- .to eq(I18n.t(:text_journal_added,
+ expect(instance.render(key, [nil, attachment.filename.to_s]))
+ .to eq(I18n.t(:text_journal_attachment_added,
label: "#{I18n.t(:'activerecord.models.attachment')}",
value: "#{attachment.filename}"))
end
@@ -82,34 +76,34 @@ def self.default_url_options
describe 'WITH the first value being an id as string, and the second nil' do
let(:expected) do
- I18n.t(:text_journal_deleted,
+ I18n.t(:text_journal_attachment_deleted,
label: "#{I18n.t(:'activerecord.models.attachment')}",
- old: "#{attachment.id}")
+ old: "#{attachment.filename}")
end
- it { expect(instance.render(key, [attachment.id.to_s, nil])).to eq(expected) }
+ it { expect(instance.render(key, [attachment.filename.to_s, nil])).to eq(expected) }
end
describe "WITH the first value being nil, and the second an id as a string
WITH specifying not to output html" do
let(:expected) do
- I18n.t(:text_journal_added,
+ I18n.t(:text_journal_attachment_added,
label: I18n.t(:'activerecord.models.attachment'),
- value: attachment.id)
+ value: attachment.filename)
end
- it { expect(instance.render(key, [nil, attachment.id.to_s], html: false)).to eq(expected) }
+ it { expect(instance.render(key, [nil, attachment.filename.to_s], html: false)).to eq(expected) }
end
describe "WITH the first value being an id as string, and the second nil,
WITH specifying not to output html" do
let(:expected) do
- I18n.t(:text_journal_deleted,
+ I18n.t(:text_journal_attachment_deleted,
label: I18n.t(:'activerecord.models.attachment'),
- old: attachment.id)
+ old: attachment.filename)
end
- it { expect(instance.render(key, [attachment.id.to_s, nil], html: false)).to eq(expected) }
+ it { expect(instance.render(key, [attachment.filename.to_s, nil], html: false)).to eq(expected) }
end
end
end
diff --git a/spec/lib/journal_formatter/file_link_spec.rb b/spec/lib/journal_formatter/file_link_spec.rb
index ed09fa58b40a..9208cec7d9e8 100644
--- a/spec/lib/journal_formatter/file_link_spec.rb
+++ b/spec/lib/journal_formatter/file_link_spec.rb
@@ -31,22 +31,47 @@
require 'spec_helper'
RSpec.describe OpenProject::JournalFormatter::FileLink do
- let(:work_package) { create(:work_package) }
+ let(:work_package) { build(:work_package) }
let(:journal) { instance_double(Journal, journable: work_package) }
- let(:file_link) { create(:file_link, container: work_package) }
+ let(:file_link) { create(:file_link, container: work_package, storage: build(:storage)) }
let(:key) { "file_links_#{file_link.id}" }
+ let(:changes) { { "link_name" => file_link.origin_name, "storage_name" => file_link.storage.name } }
+
subject(:instance) { described_class.new(journal) }
describe '#render' do
- context 'having the first value being nil, and the second an file link name as string' do
+ context 'having the origin_name as nil' do
+ let(:changes) { { "link_name" => file_link.origin_name, "storage_name" => nil } }
+
+ it 'if the storage name is nil it tries to find it out looking at the file link' do
+ link = "#{Setting.protocol}://#{Setting.host_name}/api/v3/file_links/#{file_link.id}/open"
+
+ expect(instance.render(key, [nil, changes]))
+ .to eq(I18n.t(:text_journal_file_link_added,
+ label: "#{I18n.t('activerecord.models.file_link')}",
+ value: "#{file_link.origin_name}",
+ storage: file_link.storage.name))
+ end
+
+ it 'if the storage name is nil and the file link does not exist, "Unknown storage" is used' do
+ expect(instance.render('file_links_12', [changes, nil]))
+ .to eq(I18n.t(:text_journal_file_link_deleted,
+ label: "#{I18n.t('activerecord.models.file_link')}",
+ old: "#{file_link.origin_name}",
+ storage: I18n.t('storages.unknown_storage')))
+ end
+ end
+
+ context 'having the first value being nil, and the second an hash of properties' do
context 'as HTML' do
it 'adds a file link added text' do
link = "#{Setting.protocol}://#{Setting.host_name}/api/v3/file_links/#{file_link.id}/open"
- expect(instance.render(key, [nil, file_link.origin_name]))
- .to eq(I18n.t(:text_journal_added,
- label: "#{I18n.t(:'activerecord.models.file_link')}",
- value: "#{file_link.origin_name}"))
+ expect(instance.render(key, [nil, changes]))
+ .to eq(I18n.t(:text_journal_file_link_added,
+ label: "#{I18n.t('activerecord.models.file_link')}",
+ value: "#{file_link.origin_name}",
+ storage: file_link.storage.name))
end
context 'with a configured relative url root' do
@@ -54,41 +79,47 @@
it 'adds an file link added text' do
link = "#{Setting.protocol}://#{Setting.host_name}/blubs/api/v3/file_links/#{file_link.id}/open"
- expect(instance.render(key, [nil, file_link.origin_name]))
- .to eq(I18n.t(:text_journal_added,
- label: "#{I18n.t(:'activerecord.models.file_link')}",
- value: "#{file_link.origin_name}"))
+ expect(instance.render(key, [nil, changes]))
+ .to eq(I18n.t(:text_journal_file_link_added,
+ label: "#{I18n.t('activerecord.models.file_link')}",
+ value: "#{file_link.origin_name}",
+ storage: file_link.storage.name))
end
end
end
context 'as plain text' do
it 'adds a file link added text' do
- message = I18n.t(:text_journal_added,
- label: I18n.t(:'activerecord.models.file_link'),
- value: file_link.id)
+ message = I18n.t(:text_journal_file_link_added,
+ label: I18n.t('activerecord.models.file_link'),
+ value: file_link.origin_name,
+ storage: file_link.storage.name)
- expect(instance.render(key, [nil, file_link.id.to_s], html: false)).to eq(message)
+ expect(instance.render(key, [nil, changes], html: false)).to eq(message)
end
end
end
- context 'having the first value being an id as string, and the second nil' do
+ context 'having the first value being an props hash, and the second nil' do
context 'as HTML' do
it 'adds a file link remove text' do
- message = I18n.t(:text_journal_deleted,
- label: "#{I18n.t(:'activerecord.models.file_link')}",
- old: "#{file_link.id}")
+ message = I18n.t(:text_journal_file_link_deleted,
+ label: "#{I18n.t('activerecord.models.file_link')}",
+ old: "#{file_link.origin_name}",
+ storage: file_link.storage.name)
- expect(instance.render(key, [file_link.id.to_s, nil])).to eq(message)
+ expect(instance.render(key, [changes, nil])).to eq(message)
end
end
context 'as plain text' do
it 'adds a file link removed' do
- message = I18n.t(:text_journal_deleted, label: Storages::FileLink.model_name.human, old: file_link.id)
+ message = I18n.t(:text_journal_file_link_deleted,
+ label: I18n.t('activerecord.models.file_link'),
+ old: file_link.origin_name,
+ storage: file_link.storage.name)
- expect(instance.render(key, [file_link.id.to_s, nil], html: false)).to eq(message)
+ expect(instance.render(key, [changes, nil], html: false)).to eq(message)
end
end
end
diff --git a/spec/models/work_package/work_package_acts_as_journalized_spec.rb b/spec/models/work_package/work_package_acts_as_journalized_spec.rb
index 19fd69aa1e9c..8e7d5662628f 100644
--- a/spec/models/work_package/work_package_acts_as_journalized_spec.rb
+++ b/spec/models/work_package/work_package_acts_as_journalized_spec.rb
@@ -441,11 +441,20 @@
subject(:journal_details) { work_package.last_journal.details }
it { is_expected.to have_key file_link_id }
- it { expect(journal_details[file_link_id]).to eq([nil, file_link.origin_name]) }
+
+ it {
+ expect(journal_details[file_link_id])
+ .to eq([nil, { 'link_name' => file_link.origin_name, 'storage_name' => nil }])
+ }
end
- context 'when attachment saved w/o change' do
- it { expect { file_link.save! }.not_to change(Journal, :count) }
+ context 'when file link saved w/o change' do
+ it {
+ expect do
+ file_link.save
+ work_package.save_journals
+ end.not_to change(Journal, :count)
+ }
end
end