From 2c07cf981c3d205e4a1c38d761cab5ee3de67b49 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Wed, 17 Sep 2025 17:07:14 +0200 Subject: [PATCH 1/2] [#67462] Add filterable tree view for parent selection - https://community.openproject.org/work_packages/67462 - change parent by selecting from existing hierarchy - show error if same item or same parent is selected, of if multiple items are selected --- ...ange_item_parent_dialog_component.html.erb | 17 +++++--- .../change_item_parent_dialog_component.rb | 35 ++++++++++++++- .../hierarchy/items_controller.rb | 43 ++++++++++++++++--- config/locales/en.yml | 2 + 4 files changed, 85 insertions(+), 12 deletions(-) diff --git a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.html.erb b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.html.erb index c03a235ccc8..dc43525e528 100644 --- a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.html.erb +++ b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.html.erb @@ -28,18 +28,25 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= - render Primer::Alpha::Dialog.new(id: dialog_id, title: I18n.t(:label_change_parent)) do |dialog| + render( + Primer::Alpha::Dialog.new( + id: dialog_id, + title: I18n.t(:label_change_parent), + size: :medium_portrait + ) + ) do |dialog| dialog.with_body do - primer_form_with(**form_arguments) do |f| - helpers.render_inline_form(f) do |form| - form.text_field(name: :new_parent, label: "New Parent") + primer_form_with(**form_arguments) do |form| + render( + Primer::OpenProject::FilterableTreeView.new(form_arguments: { builder: form, name: :new_parent }) + ) do |tree_view| + add_sub_tree(tree_view, hierarchy_items) end end end dialog.with_footer(show_divider: true) do concat(render(Primer::Beta::Button.new(data: { close_dialog_id: dialog_id })) { I18n.t(:button_cancel) }) - concat( render( Primer::Beta::Button.new( diff --git a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb index d75daaa5b96..baf5f82ee48 100644 --- a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb +++ b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb @@ -48,17 +48,48 @@ module Admin def form_arguments { - form_id:, + id: form_id, url: change_parent_custom_field_item_path(custom_field_id: @custom_field.id, id: @hierarchy_item.id), model: form_model, method: :post } end + def hierarchy_items + hashed_hierarchy = @custom_field.hierarchy_root.hash_tree + hashed_hierarchy.nil? ? {} : hashed_hierarchy.first[1] + end + + def add_sub_tree(tree, hierarchy_hash) + hierarchy_hash.each do |item, child_hash| + if child_hash.empty? + tree.with_leaf(**item_options(item)) + else + expanded = current?(item) || child_hash.any? { |child, _| current?(child) } + + tree.with_sub_tree(expanded: expanded, **item_options(item)) do |sub_tree| + add_sub_tree(sub_tree, child_hash) + end + end + end + end + private def form_model - CustomField::Hierarchy::Forms::NewParentFormModel.new(new_parent: nil) + CustomField::Hierarchy::Forms::NewParentFormModel.new(new_parent: []) + end + + def item_options(item) + { + label: item.label, + current: current?(item), + value: item.id + } + end + + def current?(item) + item.id == @hierarchy_item.id end end end diff --git a/app/controllers/admin/custom_fields/hierarchy/items_controller.rb b/app/controllers/admin/custom_fields/hierarchy/items_controller.rb index b5fd7ba86b5..0793930f9d1 100644 --- a/app/controllers/admin/custom_fields/hierarchy/items_controller.rb +++ b/app/controllers/admin/custom_fields/hierarchy/items_controller.rb @@ -33,6 +33,7 @@ module Admin module Hierarchy class ItemsController < ApplicationController include OpTurbo::ComponentStream + include Dry::Monads[:result] layout :admin_or_frame_layout model_object CustomField @@ -102,12 +103,26 @@ module Admin redirect_to(custom_field_item_path(@custom_field, @active_item.parent), status: :see_other) end - def change_parent - permitted = params.expect(custom_field_hierarchy_forms_new_parent_form_model: [:new_parent]) - new_parent = CustomField::Hierarchy::Item.including_children.find(permitted[:new_parent]) - item_service.move_item(item: @active_item, new_parent:) + def change_parent # rubocop:disable Metrics/AbcSize + input = params.require(:custom_field_hierarchy_forms_new_parent_form_model).require(:new_parent) - redirect_to(custom_field_item_path(@custom_field, new_parent), status: :see_other) + parse_parent_input(input).bind do |new_parent| + validate_new_parent(new_parent).bind do + item_service.move_item(item: @active_item, new_parent:) + end + end.either( + ->(result) do + redirect_to( + custom_field_item_path(@custom_field, result.parent), + status: :see_other, + notice: I18n.t(:notice_successful_update) + ) + end, + ->(error) do + render_error_flash_message_via_turbo_stream(message: error) + respond_with_turbo_streams(&:html) + end + ) end def destroy @@ -184,6 +199,24 @@ module Admin end end + def parse_parent_input(new_parent_input) + case new_parent_input + in [new_parent] + input = MultiJson.load(new_parent, symbolize_keys: true)[:value] + new_parent = CustomField::Hierarchy::Item.including_children.find(input) + Success(new_parent) + else + Failure("Invalid input: #{new_parent_input}") + end + end + + def validate_new_parent(new_parent) + return Failure(I18n.t(:notice_change_parent_same_target)) if @active_item.parent.id == new_parent.id + return Failure(I18n.t(:notice_change_parent_to_self)) if @active_item.id == new_parent.id + + Success() + end + def find_model_object @object = CustomField.hierarchy_root_and_children.find(params[:custom_field_id]) @custom_field = @object diff --git a/config/locales/en.yml b/config/locales/en.yml index bfb6a03876a..d3086b67927 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3735,6 +3735,8 @@ en: notice_auth_stage_wrong_stage: "Expected to finish authentication stage '%{expected}', but '%{actual}' returned." notice_auth_stage_error: "Authentication stage '%{stage}' failed." notice_can_t_change_password: "This account uses an external authentication source. Impossible to change the password." + notice_change_parent_same_target: "The selected item is already the parent." + notice_change_parent_to_self: "Cannot change parent to itself." notice_custom_options_deleted: "Option '%{option_value}' and its %{num_deleted} occurrences were deleted." notice_email_error: "An error occurred while sending mail (%{value})" notice_email_sent: "An email was sent to %{value}" From 256a078486b9bc2fe49011749ccea0580d309106 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Mon, 22 Sep 2025 14:48:04 +0200 Subject: [PATCH 2/2] [#67462] code cleanup after code review - removed locale keys that would never be caused by user error - display root element in change parent dialog --- .../change_item_parent_dialog_component.rb | 6 +++-- .../hierarchy/items_controller.rb | 27 ++++++++++++------- config/locales/en.yml | 2 -- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb index baf5f82ee48..d424ee57919 100644 --- a/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb +++ b/app/components/admin/custom_fields/hierarchy/change_item_parent_dialog_component.rb @@ -57,7 +57,9 @@ module Admin def hierarchy_items hashed_hierarchy = @custom_field.hierarchy_root.hash_tree - hashed_hierarchy.nil? ? {} : hashed_hierarchy.first[1] + hashed_hierarchy.keys.first.label = @custom_field.name + + hashed_hierarchy end def add_sub_tree(tree, hierarchy_hash) @@ -67,7 +69,7 @@ module Admin else expanded = current?(item) || child_hash.any? { |child, _| current?(child) } - tree.with_sub_tree(expanded: expanded, **item_options(item)) do |sub_tree| + tree.with_sub_tree(expanded:, **item_options(item)) do |sub_tree| add_sub_tree(sub_tree, child_hash) end end diff --git a/app/controllers/admin/custom_fields/hierarchy/items_controller.rb b/app/controllers/admin/custom_fields/hierarchy/items_controller.rb index 0793930f9d1..03de873e437 100644 --- a/app/controllers/admin/custom_fields/hierarchy/items_controller.rb +++ b/app/controllers/admin/custom_fields/hierarchy/items_controller.rb @@ -103,14 +103,14 @@ module Admin redirect_to(custom_field_item_path(@custom_field, @active_item.parent), status: :see_other) end - def change_parent # rubocop:disable Metrics/AbcSize - input = params.require(:custom_field_hierarchy_forms_new_parent_form_model).require(:new_parent) - - parse_parent_input(input).bind do |new_parent| + def change_parent + result = parse_parent_input(new_parent_params).bind do |new_parent| validate_new_parent(new_parent).bind do item_service.move_item(item: @active_item, new_parent:) end - end.either( + end + + result.either( ->(result) do redirect_to( custom_field_item_path(@custom_field, result.parent), @@ -162,6 +162,10 @@ module Admin input end + def new_parent_params + params.require(:custom_field_hierarchy_forms_new_parent_form_model).require(:new_parent) + end + def create_contract case @custom_field.field_format when "hierarchy" @@ -203,16 +207,21 @@ module Admin case new_parent_input in [new_parent] input = MultiJson.load(new_parent, symbolize_keys: true)[:value] - new_parent = CustomField::Hierarchy::Item.including_children.find(input) - Success(new_parent) + new_parent = CustomField::Hierarchy::Item.including_children.find_by(id: input) + + if new_parent.present? + Success(new_parent) + else + Failure("Cannot find parent with id: #{input}") + end else Failure("Invalid input: #{new_parent_input}") end end def validate_new_parent(new_parent) - return Failure(I18n.t(:notice_change_parent_same_target)) if @active_item.parent.id == new_parent.id - return Failure(I18n.t(:notice_change_parent_to_self)) if @active_item.id == new_parent.id + return Failure("Parent must not be the same as before.") if @active_item.parent.id == new_parent.id + return Failure("Parent must not be the current item.") if @active_item.id == new_parent.id Success() end diff --git a/config/locales/en.yml b/config/locales/en.yml index 4dc11e2865b..d8cb22f457f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3749,8 +3749,6 @@ en: notice_auth_stage_wrong_stage: "Expected to finish authentication stage '%{expected}', but '%{actual}' returned." notice_auth_stage_error: "Authentication stage '%{stage}' failed." notice_can_t_change_password: "This account uses an external authentication source. Impossible to change the password." - notice_change_parent_same_target: "The selected item is already the parent." - notice_change_parent_to_self: "Cannot change parent to itself." notice_custom_options_deleted: "Option '%{option_value}' and its %{num_deleted} occurrences were deleted." notice_email_error: "An error occurred while sending mail (%{value})" notice_email_sent: "An email was sent to %{value}"