-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have a single request. :D otherwise: nice finds, and quick fix. Cool!
@@ -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(" / ") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. =)
There was a problem hiding this comment.
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
1491047
to
f60f654
Compare
f60f654
to
0587d1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, convincing argument
The CustomFieldFormatter didn't know how to handle the hierarchy type fields, therefore rendered the incorrect representation of the changes.
Here is a screenshot of the updated behaviour