Rework the attachments activity messages

app/services/journals/create_service.rb

@@ -396,13 +396,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

config/locales/en.yml

@@ -2921,12 +2921,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})"

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

lib/open_project/journal_formatter/attachment.rb

@@ -44,11 +44,20 @@ 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

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

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)

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

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

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"

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: "<strong>#{I18n.t(:'activerecord.models.attachment')}</strong>",
                         value: "<a href=\"#{link}\">#{attachment.filename}</a>"))
       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: "<strong>#{I18n.t(:'activerecord.models.attachment')}</strong>",
                           value: "<a href=\"#{link}\">#{attachment.filename}</a>"))
         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: "<strong>#{I18n.t(:'activerecord.models.attachment')}</strong>",
-               old: "<strike><i>#{attachment.id}</i></strike>")
+               old: "<strike><i>#{attachment.filename}</i></strike>")
       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