Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds handling of Hierarchy fields to the JournalFormatters #17387

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Adds handling of Hierarchy fields to the JournalFormatters #17387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f37b9d1
Makes the custom field formatter aware of hierarchy fields
mereghost Dec 6, 2024
0587d1f
Remove logging statements :facepalm:
mereghost Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/models/custom_field/hierarchy/item.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(" / ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Let's extract the gem methods to the persistence layer, so, instead of calling self_and_ancestors directly from model, I'd say we introduce a simple method in the service wrapping it, like we did with descendants:

      # Gets all descendant nodes in a tree starting from the item/node.
      # @param item [CustomField::Hierarchy::Item] the node
      # @param include_self [Boolean] flag
      # @return [Success(Array<CustomField::Hierarchy::Item>)]
      def get_descendants(item:, include_self: true)
        if include_self
          Success(item.self_and_descendants)
        else
          Success(item.descendants)
        end
      end

Copy link
Contributor Author

@mereghost mereghost Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I don't see the advantage in this case.

We might differ on how we see the Service, tho. I see it as a wrapper to hide the implementation details of the tree from the outside code a mix of wrapper and syntactic sugar over it.

This here is no different of asking the label or short or ID of the record.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd describe the difference as follows:

The service is the abstraction of the persistence from the domain layer. In this case, you'd be right, finding an item by ID is also a direct access.
(Now the famous) BUT: to be pragmatic, I do not intend to root EVERYTHING through it, but actually everything closure tree related. This way, we could replace the dependency without touching dozens of code files, but just the one.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. In this case, we would need to change the actual model anyway so moot point. =)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, eyes opened now.... The model is actually more on persistence layer, sadly :D Makes no sense, using the service in here, as actually the service is using the model and the closure tree methods on it. So, If we would have imports, we would have a circular dependency even.

You are right, I'm wrong

end
end
26 changes: 20 additions & 6 deletions lib/open_project/journal_formatter/custom_field/plain.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
Expand Down Expand Up @@ -53,6 +55,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"
Expand All @@ -79,13 +83,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)

Expand All @@ -105,9 +119,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
Expand Down
110 changes: 93 additions & 17 deletions spec/lib/journal_formatter/custom_field_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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: "<strong>#{custom_field.name}</strong>",
value: "<i>#{formatted_value}</i>")
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: "<strong>#{custom_field.name}</strong>",
old: "<strike><i>#{formatted_value}</i></strike>")
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: "<strong>#{custom_field.name}</strong>",
value: "<i>#{formatted_value}</i>")
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: "<strong>#{custom_field.name}</strong>",
old: "<strike><i>#{formatted_value}</i></strike>")
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: "<strong>#{custom_field.name}</strong>",
linebreak: "",
old: "<i>#{original_value}</i>",
new: "<i>#{formatted_value}</i>")
end

it { expect(rendered).to be_html_eql(expected) }
end
end
end
end
Loading