From f37b9d1d1bb73e803eae2ab322ddb11ca49432dc Mon Sep 17 00:00:00 2001 From: Marcello Rocha Date: Fri, 6 Dec 2024 14:54:55 +0100 Subject: [PATCH 1/2] Makes the custom field formatter aware of hierarchy fields --- app/models/custom_field/hierarchy/item.rb | 4 + .../journal_formatter/custom_field/plain.rb | 32 +++-- .../journal_formatter/custom_field_spec.rb | 110 +++++++++++++++--- 3 files changed, 122 insertions(+), 24 deletions(-) diff --git a/app/models/custom_field/hierarchy/item.rb b/app/models/custom_field/hierarchy/item.rb index 7bfd41af2159..3490e7009ef3 100644 --- a/app/models/custom_field/hierarchy/item.rb +++ b/app/models/custom_field/hierarchy/item.rb @@ -37,4 +37,8 @@ class CustomField::Hierarchy::Item < ApplicationRecord scope :including_children, -> { includes(children: :children) } def to_s = short.nil? ? label : "#{label} (#{short})" + + def ancestry_path + self_and_ancestors.filter_map(&:to_s).reverse.join(" / ") + end end diff --git a/lib/open_project/journal_formatter/custom_field/plain.rb b/lib/open_project/journal_formatter/custom_field/plain.rb index 4eae0a2665d5..e16264700700 100644 --- a/lib/open_project/journal_formatter/custom_field/plain.rb +++ b/lib/open_project/journal_formatter/custom_field/plain.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -38,7 +40,9 @@ def format_details(key, values) label = label_for_custom_field(custom_field) old_value, value = if custom_field - get_formatted_values(custom_field, values) + coisa = get_formatted_values(custom_field, values) + Rails.logger.error "[MARCELLOGGER] #{coisa.inspect}" + coisa else [values.first, values.last] end @@ -53,6 +57,8 @@ def get_formatted_values(custom_field, values) def get_modifier_function(custom_field) case custom_field.field_format + when "hierarchy" + :find_item_value when "list" :find_list_value when "user" @@ -65,6 +71,8 @@ def get_modifier_function(custom_field) end def formatted_values(custom_field, values, modifier_fn) + Rails.logger.error "[MARCELLOGGER] #{values.inspect} #{modifier_fn.inspect} #{custom_field.inspect}" + values.map { |value| formatted_value(custom_field, value, modifier_fn) } end @@ -79,13 +87,23 @@ def find_user_value(value, _custom_field) # Lookup any visible user we can find user_lookup = Principal - .in_visible_project_or_me(User.current) - .where(id: ids) - .index_by(&:id) + .in_visible_project_or_me(User.current) + .where(id: ids) + .index_by(&:id) ids_to_names(ids, user_lookup) end + def find_item_value(value, _custom_field) + ids = value.split(",").map(&:to_i) + + CustomField::Hierarchy::Item.where(id: ids).map do |item| + next I18n.t(:label_deleted_custom_option) unless ids.include?(item.id) + + item.ancestry_path + end.join(", ") + end + def find_list_value(value, custom_field) ids = value.split(",").map(&:to_i) @@ -105,9 +123,9 @@ def find_version_value(value, _custom_field) # Lookup visible versions we can find version_lookup = Version - .visible(User.current) - .where(id: ids) - .index_by(&:id) + .visible(User.current) + .where(id: ids) + .index_by(&:id) ids_to_names(ids, version_lookup) end diff --git a/spec/lib/journal_formatter/custom_field_spec.rb b/spec/lib/journal_formatter/custom_field_spec.rb index 2398bd319a94..e86d3fb90999 100644 --- a/spec/lib/journal_formatter/custom_field_spec.rb +++ b/spec/lib/journal_formatter/custom_field_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -50,8 +52,8 @@ def url_helper = Rails.application.routes.url_helpers allow(CustomField).to receive(:find_by).and_return(nil) allow(CustomField) .to receive(:find_by) - .with(id: custom_field.id) - .and_return(custom_field) + .with(id: custom_field.id) + .and_return(custom_field) end context "with html requested by default" do @@ -196,8 +198,8 @@ def url_helper = Rails.application.routes.url_helpers allow(wherestub) .to receive(:where) - .with(id: [user1.id, user2.id]) - .and_return(visible_users) + .with(id: [user1.id, user2.id]) + .and_return(visible_users) end describe "with two visible users" do @@ -248,37 +250,37 @@ def url_helper = Rails.application.routes.url_helpers allow(custom_field) .to receive(:custom_options) - .and_return(cf_options) + .and_return(cf_options) allow(cf_options) .to receive(:where) - .with(id: [1, 2]) - .and_return(old_options) + .with(id: [1, 2]) + .and_return(old_options) allow(cf_options) .to receive(:where) - .with(id: [3, 4]) - .and_return(new_options) + .with(id: [3, 4]) + .and_return(new_options) allow(old_options) .to receive(:order) - .with(:position) - .and_return(old_options) + .with(:position) + .and_return(old_options) allow(new_options) .to receive(:order) - .with(:position) - .and_return(new_options) + .with(:position) + .and_return(new_options) allow(old_options) .to receive(:pluck) - .with(:id, :value) - .and_return(old_custom_option_names) + .with(:id, :value) + .and_return(old_custom_option_names) allow(new_options) .to receive(:pluck) - .with(:id, :value) - .and_return(new_custom_option_names) + .with(:id, :value) + .and_return(new_custom_option_names) end describe "with both values being a comma separated list of ids" do @@ -395,4 +397,78 @@ def url_helper = Rails.application.routes.url_helpers end end end + + context "for hierarchy custom field" do + shared_let(:custom_field) { create(:hierarchy_wp_custom_field) } + shared_let(:service) { CustomFields::Hierarchy::HierarchicalItemService.new } + shared_let(:root) { service.generate_root(custom_field).value! } + shared_let(:luke) { service.insert_item(parent: root, label: "luke", short: "LS").value! } + shared_let(:mara) { service.insert_item(parent: luke, label: "mara").value! } + + describe "first value being nil and second value a string" do + let(:values) { [nil, mara.id.to_s] } + let(:formatted_value) { mara.ancestry_path } + let(:expected) do + I18n.t(:text_journal_set_to, + label: "#{custom_field.name}", + value: "#{formatted_value}") + end + + it { expect(rendered).to be_html_eql(expected) } + end + + describe "first value being a string and second value being nil" do + let(:values) { [mara.id.to_s, nil] } + let(:formatted_value) { mara.ancestry_path } + let(:expected) do + I18n.t(:text_journal_deleted, + label: "#{custom_field.name}", + old: "#{formatted_value}") + end + + it { expect(rendered).to be_html_eql(expected) } + end + + context "with multiple values" do + describe "first value being nil and second value a string" do + let(:values) { [nil, [luke.id, mara.id].join(",")] } + let(:formatted_value) { [luke.ancestry_path, mara.ancestry_path].join(", ") } + let(:expected) do + I18n.t(:text_journal_set_to, + label: "#{custom_field.name}", + value: "#{formatted_value}") + end + + it { expect(rendered).to be_html_eql(expected) } + end + + describe "first value being a string and second value being nil" do + let(:values) { [[luke.id, mara.id].join(","), nil] } + let(:formatted_value) { [luke.ancestry_path, mara.ancestry_path].join(", ") } + let(:expected) do + I18n.t(:text_journal_deleted, + label: "#{custom_field.name}", + old: "#{formatted_value}") + end + + it { expect(rendered).to be_html_eql(expected) } + end + + describe "both values being strings" do + let(:values) { [[luke.id, mara.id].join(","), luke.id.to_s] } + let(:original_value) { [luke.ancestry_path, mara.ancestry_path].join(", ") } + let(:formatted_value) { luke.ancestry_path } + + let(:expected) do + I18n.t(:text_journal_changed_plain, + label: "#{custom_field.name}", + linebreak: "", + old: "#{original_value}", + new: "#{formatted_value}") + end + + it { expect(rendered).to be_html_eql(expected) } + end + end + end end From 0587d1f63bdeef8143fd1efd6ff4a4a7512ae34b Mon Sep 17 00:00:00 2001 From: Marcello Rocha Date: Fri, 6 Dec 2024 15:05:58 +0100 Subject: [PATCH 2/2] Remove logging statements :facepalm: --- lib/open_project/journal_formatter/custom_field/plain.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/open_project/journal_formatter/custom_field/plain.rb b/lib/open_project/journal_formatter/custom_field/plain.rb index e16264700700..a0ed17f05b27 100644 --- a/lib/open_project/journal_formatter/custom_field/plain.rb +++ b/lib/open_project/journal_formatter/custom_field/plain.rb @@ -40,9 +40,7 @@ def format_details(key, values) label = label_for_custom_field(custom_field) old_value, value = if custom_field - coisa = get_formatted_values(custom_field, values) - Rails.logger.error "[MARCELLOGGER] #{coisa.inspect}" - coisa + get_formatted_values(custom_field, values) else [values.first, values.last] end @@ -71,8 +69,6 @@ def get_modifier_function(custom_field) end def formatted_values(custom_field, values, modifier_fn) - Rails.logger.error "[MARCELLOGGER] #{values.inspect} #{modifier_fn.inspect} #{custom_field.inspect}" - values.map { |value| formatted_value(custom_field, value, modifier_fn) } end