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.

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

[58105] Added update_item and delete_branch methods to HierarchicalItemService #16939

Merged
merged 4 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ module CustomFields
module Hierarchy
class GenerateRootContract < Dry::Validation::Contract
params do
required(:hierarchy_root)
required(:custom_field).filled(type?: CustomField)
end

rule(:hierarchy_root) do
key.failure("Hierarchical root already set") unless value.nil?
rule(:custom_field) do
key.failure("must have field format 'hierarchy'") if value.field_format != "hierarchy"
key.failure("cannot be defined") if value.hierarchy_root.present?
end
end
end
Expand Down
10 changes: 2 additions & 8 deletions app/contracts/custom_fields/hierarchy/insert_item_contract.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,13 @@ class InsertItemContract < Dry::Validation::Contract
config.messages.backend = :i18n

params do
required(:parent).filled
required(:parent).filled(type?: CustomField::Hierarchy::Item)
required(:label).filled(:string)
optional(:short).filled(:string)
end

rule(:parent) do
if value.is_a?(CustomField::Hierarchy::Item)
unless value.persisted?
key.failure("Parent must exist")
end
else
key.failure("Parent must be of type 'Item'")
end
key.failure("must exist") unless value.persisted?
end

rule(:label) do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,24 @@

module CustomFields
module Hierarchy
class ServiceInitializationContract < Dry::Validation::Contract
class UpdateItemContract < Dry::Validation::Contract
params do
required(:field_format).filled(:string)
required(:item).filled(type?: CustomField::Hierarchy::Item)
optional(:label).filled(:string)
optional(:short).filled(:string)
end

rule(:field_format) do
key.failure("Custom field must have field format 'hierarchy'") if value != "hierarchy"
rule(:item) do
key.failure("must exist") if value.new_record?
key.failure("must not be a root item") if value.root?
Kharonus marked this conversation as resolved.
Show resolved Hide resolved
end

rule(:label) do
next unless key?

if CustomField::Hierarchy::Item.exists?(parent_id: values[:item].parent_id, label: value)
key.failure("must be unique at the same hierarchical level")
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def new

def create
::CustomFields::Hierarchy::HierarchicalItemService
.new(@custom_field)
.new
.insert_item(**item_input)
.either(
->(_) { update_via_turbo_stream(component: ItemsComponent.new(custom_field: @custom_field)) },
Expand Down
2 changes: 1 addition & 1 deletion app/models/custom_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class CustomField < ApplicationRecord

has_one :hierarchy_root,
class_name: "CustomField::Hierarchy::Item",
dependent: :delete, # todo: cascade into children with service
dependent: :destroy,
inverse_of: "custom_field"

acts_as_list scope: [:type]
Expand Down
2 changes: 1 addition & 1 deletion app/models/custom_field/hierarchy/item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,5 @@ class CustomField::Hierarchy::Item < ApplicationRecord
self.table_name = "hierarchical_items"

belongs_to :custom_field
has_closure_tree order: "sort_order", numeric_order: true
has_closure_tree order: "sort_order", numeric_order: true, dependent: :destroy
end
2 changes: 1 addition & 1 deletion app/services/custom_fields/create_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def after_perform(call)
if cf.is_a?(ProjectCustomField)
add_cf_to_visible_columns(cf)
elsif cf.field_format_hierarchy?
CustomFields::Hierarchy::HierarchicalItemService.new(cf).generate_root
CustomFields::Hierarchy::HierarchicalItemService.new.generate_root(cf)
end

call
Expand Down
98 changes: 78 additions & 20 deletions app/services/custom_fields/hierarchy/hierarchical_item_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,47 +33,105 @@ module Hierarchy
class HierarchicalItemService
include Dry::Monads[:result]

def initialize(custom_field)
validation = ServiceInitializationContract.new.call(field_format: custom_field.field_format)
# rubocop:disable Rails/DeprecatedActiveModelErrorsMethods
raise ArgumentError, "Invalid custom field: #{validation.errors.to_h}" if validation.failure?
# rubocop:enable Rails/DeprecatedActiveModelErrorsMethods

@custom_field = custom_field
end

def generate_root
# Generate the root item for the CustomField of type hierarchy
# @param custom_field [CustomField] custom field of type hierarchy
# @return [Success(CustomField::Hierarchy::Item), Failure(Dry::Validation::Result), Failure(ActiveModel::Errors)]
def generate_root(custom_field)
CustomFields::Hierarchy::GenerateRootContract
.new
.call(hierarchy_root: @custom_field.hierarchy_root)
.call(custom_field:)
.to_monad
.bind { create_root_item }
.bind { |validation| create_root_item(validation[:custom_field]) }
end

# Insert a new node on the hierarchy tree.
# @param parent [CustomField::Hierarchy::Item] the parent of the node
# @param label [String] the node label/name that must be unique at the same tree level
# @param short [String] an alias for the node
# @return [Success(CustomField::Hierarchy::Item), Failure(Dry::Validation::Result), Failure(ActiveModel::Errors)]
def insert_item(parent:, label:, short: nil)
CustomFields::Hierarchy::InsertItemContract
.new
.call({ parent:, label:, short: }.compact)
.to_monad
.bind { |validation| create_child_item(validation) }
.bind { |validation| create_child_item(validation:) }
end

# Updates an item/node
# @param item [CustomField::Hierarchy::Item] the item to be updated
# @param label [String] the node label/name that must be unique at the same tree level
# @param short [String] an alias for the node
# @return [Success(CustomField::Hierarchy::Item), Failure(Dry::Validation::Result), Failure(ActiveModel::Errors)]
def update_item(item:, label: nil, short: nil)
CustomFields::Hierarchy::UpdateItemContract
.new
.call({ item:, label:, short: }.compact)
.to_monad
.bind { |attributes| update_item_attributes(item:, attributes:) }
end

# Delete an entire branch of the hierarchy/tree
# @param item [CustomField::Hierarchy::Item] the parent of the node
# @return [Success(CustomField::Hierarchy::Item), Failure(Symbol), Failure(ActiveModel::Errors)]
def delete_branch(item:)
return Failure(:item_is_root) if item.root?

item.destroy ? Success() : Failure(item.errors)
end

# Gets all nodes in a tree from the item/node back to the root.
# Ordered from root to leaf
# @param item [CustomField::Hierarchy::Item] the parent of the node
# @return [Success(Array<CustomField::Hierarchy::Item>)]
def get_branch(item:)
Success(item.ancestors.reverse)
end

# Move an item/node to a new parent item/node
# @param item [CustomField::Hierarchy::Item] the parent of the node
# @param new_parent [CustomField::Hierarchy::Item] the new parent of the node
# @return [Success(CustomField::Hierarchy::Item)]
def move_item(item:, new_parent:)
Success(new_parent.append_child(item))
Kharonus marked this conversation as resolved.
Show resolved Hide resolved
end

# Reorder the item along its siblings.
# @param item [CustomField::Hierarchy::Item] the parent of the node
# @param new_sort_order [Integer] the new parent of the node
# @return [Success(CustomField::Hierarchy::Item)]
def reorder_item(item:, new_sort_order:)
old_item = item.siblings.where(sort_order: new_sort_order).first
Success(old_item.prepend_sibling(item))
end

def soft_delete_item(item)
# Soft delete the item and children
raise NotImplementedError
end

private

def create_root_item
item = CustomField::Hierarchy::Item.create(custom_field: @custom_field)
return Failure(item.errors) unless item.persisted?
def create_root_item(custom_field)
item = CustomField::Hierarchy::Item.create(custom_field.hierarchy_root)
return Failure(item.errors) if item.new_record?

Success(item)
end

def create_child_item(validation)
item = CustomField::Hierarchy::Item
.create(parent: validation[:parent], label: validation[:label], short: validation[:short])
return Failure(item.errors) unless item.persisted?
def create_child_item(validation:)
item = validation[:parent].children.create(label: validation[:label], short: validation[:short])
Copy link
Member

Choose a reason for hiding this comment

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

🟡 Hmm, not sure, but I do not like this change. What is the benefit? Before the code was very clear, that a new Item is created. Now I must understand, that the item to create is referenced here as .children.

🔴 Does this even work, if the created item is not the only child?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because that's how closure_tree works. It keeps intermediate representations of the tree for lookup, so we need to go through its code.

From another perspective, it is clearer to the random RoR dev as children.create maps to "I'm creating some associated record to the caller". :P

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm, that this does not break, if the parent already has persisted children, when adding a new one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It works. Why? Because .children is an association proxy that don't override anything but ensure that the fields for the association are correctly set - and in case of closure tree, sets up the rest of the fields and optimizations.

return Failure(item.errors) if item.new_record?

Success(item)
end

def update_item_attributes(item:, attributes:)
if item.update(label: attributes[:label], short: attributes[:short])
Success(item)
else
Failure(item.errors)
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
let(:custom_field) { create(:custom_field, field_format: "hierarchy", hierarchy_root: nil) }

it "is valid" do
result = subject.call(hierarchy_root: custom_field.hierarchy_root)
result = subject.call(custom_field:)
expect(result).to be_success
end
end
Expand All @@ -48,19 +48,17 @@
let(:custom_field) { create(:custom_field, field_format: "hierarchy", hierarchy_root:) }

it "is invalid" do
result = subject.call(hierarchy_root: custom_field.hierarchy_root)
result = subject.call(custom_field:)
expect(result).to be_failure
# rubocop:disable Rails/DeprecatedActiveModelErrorsMethods
expect(result.errors.to_h).to include(hierarchy_root: ["Hierarchical root already set"])
# rubocop:enable Rails/DeprecatedActiveModelErrorsMethods
expect(result.errors[:custom_field]).to match_array("cannot be defined")
end
end

context "when inputs are valid" do
let(:custom_field) { create(:custom_field, field_format: "hierarchy", hierarchy_root: nil) }

it "creates a success result" do
[
{ hierarchy_root: nil }
].each { |params| expect(subject.call(params)).to be_success }
expect(subject.call(custom_field:)).to be_success
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
it "is invalid" do
result = subject.call(params)
expect(result).to be_failure
expect(result.errors.to_h).to include(parent: ["Parent must be of type 'Item'"])
expect(result.errors.to_h).to include(parent: ["must be CustomField::Hierarchy::Item"])
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,65 +30,83 @@

require "rails_helper"

RSpec.describe CustomFields::Hierarchy::ServiceInitializationContract do
RSpec.describe CustomFields::Hierarchy::UpdateItemContract do
subject { described_class.new }

# rubocop:disable Rails/DeprecatedActiveModelErrorsMethods
describe "#call" do
context "when field_format is 'hierarchy'" do
let(:params) { { field_format: "hierarchy" } }
let(:vader) { create(:hierarchy_item) }
let(:luke) { create(:hierarchy_item, label: "luke", short: "ls", parent: vader) }
let(:leia) { create(:hierarchy_item, label: "leia", short: "lo", parent: vader) }

before do
luke
leia
end

context "when all required fields are valid" do
it "is valid" do
result = subject.call(params)
expect(result).to be_success
[
{ item: luke, label: "Luke Skywalker", short: "LS" },
{ item: luke, label: "Luke Skywalker" },
{ item: luke, short: "LS" },
{ item: luke, short: "lo" },
{ item: luke }
].each { |params| expect(subject.call(params)).to be_success }
end
end

context "when field_format is not 'hierarchy'" do
let(:params) { { field_format: "text" } }
context "when item is a root item" do
let(:params) { { item: vader } }

it "is invalid" do
it("is invalid") do
result = subject.call(params)
expect(result).to be_failure
expect(result.errors.to_h).to include(field_format: ["Custom field must have field format 'hierarchy'"])
expect(result.errors.to_h).to include(item: ["must not be a root item"])
end
end

context "when field_format is missing" do
let(:params) { {} }
context "when item is not of type 'Item'" do
let(:invalid_item) { create(:custom_field) }
let(:params) { { item: invalid_item } }

it "is invalid" do
it("is invalid") do
result = subject.call(params)
expect(result).to be_failure
expect(result.errors.to_h).to include(field_format: ["is missing"])
expect(result.errors.to_h).to include(item: ["must be CustomField::Hierarchy::Item"])
end
end

context "when field_format is nil" do
let(:params) { { field_format: nil } }
context "when item is not persisted" do
let(:item) { build(:hierarchy_item, parent: vader) }
let(:params) { { item: } }

it "is invalid" do
result = subject.call(params)
expect(result).to be_failure
expect(result.errors.to_h).to include(field_format: ["must be filled"])
expect(result.errors.to_h).to include(item: ["must exist"])
end
end

context "when inputs are valid" do
it "creates a success result" do
[
{ field_format: "hierarchy" }
].each { |params| expect(subject.call(params)).to be_success }
context "when the label already exist in the same hierarchy level" do
let(:params) { { item: luke, label: "leia" } }

it "is invalid" do
result = subject.call(params)
expect(result).to be_failure
expect(result.errors.to_h).to include(label: ["must be unique at the same hierarchical level"])
end
end

context "when inputs are invalid" do
it "creates a failure result" do
context "when fields are invalid" do
it "is invalid" do
[
{},
{ field_format: "text" },
{ field_format: nil },
{ field_format: 42 }
{ item: nil },
{ item: luke, label: nil },
{ item: luke, label: 42 },
{ item: luke, short: nil },
{ item: luke, short: 42 }
].each { |params| expect(subject.call(params)).to be_failure }
end
end
Expand Down
Loading