From 71009094a4ef3204482056e96a285ea449442b3c Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Mon, 4 Nov 2024 15:59:58 +0100 Subject: [PATCH] refactor(type_de_champ): improve and clean helper methods --- .../concerns/procedure_publish_concern.rb | 6 +- app/models/dossier_preloader.rb | 2 +- app/models/procedure_revision.rb | 118 ++++++++---------- .../procedure_revision_type_de_champ.rb | 8 +- app/models/type_de_champ.rb | 8 +- spec/models/champ_spec.rb | 5 +- spec/models/procedure_revision_spec.rb | 1 + spec/models/procedure_spec.rb | 5 +- 8 files changed, 74 insertions(+), 79 deletions(-) diff --git a/app/models/concerns/procedure_publish_concern.rb b/app/models/concerns/procedure_publish_concern.rb index 85e0fe48c..058d11b32 100644 --- a/app/models/concerns/procedure_publish_concern.rb +++ b/app/models/concerns/procedure_publish_concern.rb @@ -122,8 +122,8 @@ module ProcedurePublishConcern end def cleanup_types_de_champ_children! - draft_revision.types_de_champ.reject(&:repetition?).each do |type_de_champ| - draft_revision.remove_children_of(type_de_champ) - end + draft_revision.revision_types_de_champ + .filter(&:orphan?) + .each { draft_revision.remove_type_de_champ(_1.stable_id) } end end diff --git a/app/models/dossier_preloader.rb b/app/models/dossier_preloader.rb index 920ef00bf..6f5195dfa 100644 --- a/app/models/dossier_preloader.rb +++ b/app/models/dossier_preloader.rb @@ -39,7 +39,7 @@ class DossierPreloader def revisions(pj_template: false) @revisions ||= ProcedureRevision.where(id: @dossiers.pluck(:revision_id).uniq) - .includes(procedure: [], revision_types_de_champ: { parent: :type_de_champ }, types_de_champ_public: [], types_de_champ_private: [], types_de_champ: pj_template ? { piece_justificative_template_attachment: :blob } : []) + .includes(procedure: [], revision_types_de_champ: { parent_type_de_champ: [], types_de_champ: [] }, types_de_champ_public: [], types_de_champ_private: [], types_de_champ: pj_template ? { piece_justificative_template_attachment: :blob } : []) .index_by(&:id) end diff --git a/app/models/procedure_revision.rb b/app/models/procedure_revision.rb index 6ed3cce75..94f896d62 100644 --- a/app/models/procedure_revision.rb +++ b/app/models/procedure_revision.rb @@ -45,22 +45,26 @@ class ProcedureRevision < ApplicationRecord after_stable_id = params.delete(:after_stable_id) after_coordinate, _ = coordinate_and_tdc(after_stable_id) - siblings = siblings_for(parent_coordinate:, private_tdc: params[:private]) + type_de_champ = TypeDeChamp.new(params) - tdc = TypeDeChamp.new(params) - if tdc.save - # moving all the impacted tdc down - position = next_position_for(after_coordinate:, siblings:) - siblings.where("position >= ?", position).update_all("position = position + 1") + if type_de_champ.save + siblings = siblings_for(type_de_champ:, parent_coordinate:) + position = next_position_for(after_coordinate:) - # insertion of the new tdc - h = { type_de_champ: tdc, parent_id: parent_id, position: position } - revision_types_de_champ.create!(h) + transaction do + # moving all the impacted tdc down + siblings.where("position >= ?", position).update_all("position = position + 1") + + # insertion of the new tdc + revision_types_de_champ.create!(type_de_champ:, parent_id:, position:) + end + + revision_types_de_champ.reset end - tdc + type_de_champ rescue => e - TypeDeChamp.new.tap { |tdc| tdc.errors.add(:base, e.message) } + TypeDeChamp.new.tap { _1.errors.add(:base, e.message) } end def find_and_ensure_exclusive_use(stable_id) @@ -77,12 +81,17 @@ class ProcedureRevision < ApplicationRecord coordinate, _ = coordinate_and_tdc(stable_id) siblings = coordinate.siblings - if position > coordinate.position - siblings.where(position: coordinate.position..position).update_all("position = position - 1") - else - siblings.where(position: position..coordinate.position).update_all("position = position + 1") + transaction do + if position > coordinate.position + siblings.where(position: coordinate.position..position).update_all("position = position - 1") + else + siblings.where(position: position..coordinate.position).update_all("position = position + 1") + end + coordinate.update_column(:position, position) end - coordinate.update_column(:position, position) + + revision_types_de_champ.reset + coordinate.reload coordinate end @@ -90,16 +99,18 @@ class ProcedureRevision < ApplicationRecord coordinate, _ = coordinate_and_tdc(stable_id) siblings = coordinate.siblings - if position > coordinate.position - siblings.where(position: coordinate.position..position).update_all("position = position - 1") - coordinate.update_column(:position, position) - else - siblings.where(position: (position + 1)...coordinate.position).update_all("position = position + 1") - coordinate.update_column(:position, position + 1) + transaction do + if position > coordinate.position + siblings.where(position: coordinate.position..position).update_all("position = position - 1") + coordinate.update_column(:position, position) + else + siblings.where(position: (position + 1)...coordinate.position).update_all("position = position + 1") + coordinate.update_column(:position, position + 1) + end end + revision_types_de_champ.reset coordinate.reload - coordinate end @@ -110,13 +121,17 @@ class ProcedureRevision < ApplicationRecord return nil if coordinate.nil? children = children_of(tdc).to_a - coordinate.destroy - children.each(&:destroy_if_orphan) - tdc.destroy_if_orphan + transaction do + coordinate.destroy - coordinate.siblings.where("position >= ?", coordinate.position).update_all("position = position - 1") + children.each(&:destroy_if_orphan) + tdc.destroy_if_orphan + coordinate.siblings.where("position >= ?", coordinate.position).update_all("position = position - 1") + end + + revision_types_de_champ.reset coordinate end @@ -181,33 +196,11 @@ class ProcedureRevision < ApplicationRecord end def children_of(tdc) - if revision_types_de_champ.loaded? - parent_coordinate_id = revision_types_de_champ - .filter { _1.type_de_champ_id == tdc.id } - .map(&:id) - - revision_types_de_champ - .filter { _1.parent_id.in?(parent_coordinate_id) } - .sort_by(&:position) - .map(&:type_de_champ) - else - parent_coordinate_id = revision_types_de_champ.where(type_de_champ: tdc).select(:id) - - types_de_champ - .where(procedure_revision_types_de_champ: { parent_id: parent_coordinate_id }) - .order("procedure_revision_types_de_champ.position") - end + coordinate_for(tdc).types_de_champ end def parent_of(tdc) - revision_types_de_champ - .find { _1.type_de_champ_id == tdc.id }.parent&.type_de_champ - end - - def remove_children_of(tdc) - children_of(tdc).each do |child| - remove_type_de_champ(child.stable_id) - end + coordinate_for(tdc).parent_type_de_champ end def dependent_conditions(tdc) @@ -268,24 +261,17 @@ class ProcedureRevision < ApplicationRecord end end - def children_types_de_champ_as_json(tdcs_as_json, parent_tdcs) - parent_tdcs.each do |parent_tdc| - tdc_as_json = tdcs_as_json.find { |json| json["id"] == parent_tdc.stable_id } - tdc_as_json&.merge!(types_de_champ: children_of(parent_tdc).includes(piece_justificative_template_attachment: :blob).map(&:as_json_for_editor)) - end - end - - def siblings_for(parent_coordinate: nil, private_tdc: false) + def siblings_for(type_de_champ:, parent_coordinate: nil) if parent_coordinate parent_coordinate.revision_types_de_champ - elsif private_tdc + elsif type_de_champ.private? revision_types_de_champ_private else revision_types_de_champ_public end end - def next_position_for(siblings:, after_coordinate: nil) + def next_position_for(after_coordinate: nil) # either we are at the beginning of the list or after another item if after_coordinate.nil? # first element of the list, starts at 0 0 @@ -477,10 +463,12 @@ class ProcedureRevision < ApplicationRecord end def replace_type_de_champ_by_clone(coordinate) - cloned_type_de_champ = coordinate.type_de_champ.deep_clone do |original, kopy| - ClonePiecesJustificativesService.clone_attachments(original, kopy) + transaction do + cloned_type_de_champ = coordinate.type_de_champ.deep_clone do |original, kopy| + ClonePiecesJustificativesService.clone_attachments(original, kopy) + end + coordinate.update!(type_de_champ: cloned_type_de_champ) + cloned_type_de_champ end - coordinate.update!(type_de_champ: cloned_type_de_champ) - cloned_type_de_champ end end diff --git a/app/models/procedure_revision_type_de_champ.rb b/app/models/procedure_revision_type_de_champ.rb index 3963ae630..393c064f1 100644 --- a/app/models/procedure_revision_type_de_champ.rb +++ b/app/models/procedure_revision_type_de_champ.rb @@ -5,7 +5,9 @@ class ProcedureRevisionTypeDeChamp < ApplicationRecord belongs_to :type_de_champ belongs_to :parent, class_name: 'ProcedureRevisionTypeDeChamp', optional: true + has_one :parent_type_de_champ, through: :parent, source: :type_de_champ has_many :revision_types_de_champ, -> { ordered }, foreign_key: :parent_id, class_name: 'ProcedureRevisionTypeDeChamp', inverse_of: :parent, dependent: :destroy + has_many :types_de_champ, through: :revision_types_de_champ, source: :type_de_champ has_one :procedure, through: :revision scope :root, -> { where(parent: nil) } scope :ordered, -> { order(:position, :id) } @@ -19,6 +21,10 @@ class ProcedureRevisionTypeDeChamp < ApplicationRecord parent_id.present? end + def orphan? + child? && !parent_type_de_champ.repetition? + end + def first? position == 0 end @@ -33,7 +39,7 @@ class ProcedureRevisionTypeDeChamp < ApplicationRecord def siblings if child? - revision.revision_types_de_champ.where(parent_id:).ordered + parent.revision_types_de_champ elsif private? revision.revision_types_de_champ_private else diff --git a/app/models/type_de_champ.rb b/app/models/type_de_champ.rb index b380dc9fe..36904f1fe 100644 --- a/app/models/type_de_champ.rb +++ b/app/models/type_de_champ.rb @@ -336,7 +336,7 @@ class TypeDeChamp < ApplicationRecord end def child?(revision) - revision.revision_types_de_champ.find { _1.stable_id == stable_id }&.child? + revision.coordinate_for(self)&.child? end def filename_for_attachement(attachment_sym) @@ -403,10 +403,10 @@ class TypeDeChamp < ApplicationRecord end def level_for_revision(revision) - rtdc = revision.revision_types_de_champ.find { |rtdc| rtdc.stable_id == stable_id } + parent_type_de_champ = revision.parent_of(self) - if rtdc.child? - header_section_level_value.to_i + rtdc.parent.type_de_champ.current_section_level(revision) + if parent_type_de_champ.present? + header_section_level_value.to_i + parent_type_de_champ.current_section_level(revision) elsif header_section_level_value header_section_level_value.to_i else diff --git a/spec/models/champ_spec.rb b/spec/models/champ_spec.rb index 0f1a01ff5..61b554211 100644 --- a/spec/models/champ_spec.rb +++ b/spec/models/champ_spec.rb @@ -31,8 +31,9 @@ describe Champ do end context 'when repetition blank' do - let(:type_de_champ) { build(:type_de_champ_repetition) } - let(:champ) { Champs::RepetitionChamp.new(dossier: Dossier.new(revision: ProcedureRevision.new)) } + let(:procedure) { create(:procedure, types_de_champ_public: [{ type: :repetition, mandatory: false, children: [{ type: :text }] }]) } + let(:dossier) { create(:dossier, procedure:) } + let(:champ) { dossier.project_champs_public.find(&:repetition?) } it { expect(champ.blank?).to be(true) } end diff --git a/spec/models/procedure_revision_spec.rb b/spec/models/procedure_revision_spec.rb index 1d6856b1d..ec49a7134 100644 --- a/spec/models/procedure_revision_spec.rb +++ b/spec/models/procedure_revision_spec.rb @@ -830,6 +830,7 @@ describe ProcedureRevision do .revision_types_de_champ .where(type_de_champ: first_child) .update(type_de_champ: new_child) + new_draft.revision_types_de_champ.reload end it 'returns the children regarding the revision' do diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 50db50eb1..466435043 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -398,9 +398,8 @@ describe Procedure do expect(procedure.errors.messages_for(:draft_types_de_champ_public)).to include(invalid_repetition_error_message) new_draft = procedure.draft_revision - repetition = procedure.draft_revision.types_de_champ_public.find(&:repetition?) - parent_coordinate = new_draft.revision_types_de_champ.find_by(type_de_champ: repetition) - new_draft.revision_types_de_champ.create(type_de_champ: create(:type_de_champ), position: 0, parent: parent_coordinate) + repetition = new_draft.types_de_champ_public.find(&:repetition?) + new_draft.add_type_de_champ(type_champ: :text, libelle: 'Nom', parent_stable_id: repetition.stable_id) procedure.validate(:publication) expect(procedure.errors.messages_for(:draft_types_de_champ_public)).not_to include(invalid_repetition_error_message)