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

ETQ dev, je ne veux plus avoir de relation entre les champs et leur parent #10873

Merged
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
19 changes: 2 additions & 17 deletions app/models/champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,17 @@ class Champ < ApplicationRecord
include ChampConditionalConcern
include ChampsValidateConcern

self.ignored_columns += [:type_de_champ_id]
self.ignored_columns += [:type_de_champ_id, :parent_id]

attr_readonly :stable_id

belongs_to :dossier, inverse_of: false, touch: true, optional: false
belongs_to :parent, class_name: 'Champ', optional: true
has_many_attached :piece_justificative_file

# We declare champ specific relationships (Champs::CarteChamp, Champs::SiretChamp and Champs::RepetitionChamp)
# here because otherwise we can't easily use includes in our queries.
has_many :geo_areas, -> { order(:created_at) }, dependent: :destroy, inverse_of: :champ
belongs_to :etablissement, optional: true, dependent: :destroy
has_many :champs, foreign_key: :parent_id, inverse_of: :parent

delegate :procedure, to: :dossier

Expand Down Expand Up @@ -79,13 +77,8 @@ def type_de_champ
delegate :used_by_routing_rules?, to: :type_de_champ

scope :updated_since?, -> (date) { where('champs.updated_at > ?', date) }
scope :public_only, -> { where(private: false) }
scope :private_only, -> { where(private: true) }
scope :root, -> { where(parent_id: nil) }
scope :prefilled, -> { where(prefilled: true) }

before_create :set_dossier_id, if: :needs_dossier_id?
before_validation :set_dossier_id, if: :needs_dossier_id?
before_save :cleanup_if_empty
before_save :normalize
after_update_commit :fetch_external_data_later
Expand Down Expand Up @@ -232,7 +225,7 @@ def update_with_external_data!(data:)
end

def clone(fork = false)
champ_attributes = [:parent_id, :private, :row_id, :type, :stable_id, :stream]
champ_attributes = [:private, :row_id, :type, :stable_id, :stream]
value_attributes = fork || !private? ? [:value, :value_json, :data, :external_id] : []
relationships = fork || !private? ? [:etablissement, :geo_areas] : []

Expand Down Expand Up @@ -265,14 +258,6 @@ def html_id
type_de_champ.html_id(row_id)
end

def needs_dossier_id?
!dossier_id && parent_id
end

def set_dossier_id
self.dossier_id = parent.dossier_id
end

def cleanup_if_empty
if fetch_external_data? && persisted? && external_id_changed?
self.data = nil
Expand Down
6 changes: 1 addition & 5 deletions app/models/champs/repetition_champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ def row_ids
end

def add_row(updated_by:)
# TODO: clean this up when parent_id is deprecated
row_id, added_champs = dossier.repetition_add_row(type_de_champ, updated_by:)
self.champs << added_champs
dossier.champs.reload if dossier.persisted?
row_id
dossier.repetition_add_row(type_de_champ, updated_by:)
end

def remove_row(row_id, updated_by:)
Expand Down
17 changes: 5 additions & 12 deletions app/models/concerns/dossier_champs_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def project_champs_private
end

def project_rows_for(type_de_champ)
[] if !type_de_champ.repetition?
return [] if !type_de_champ.repetition?

children = revision.children_of(type_de_champ)
row_ids = repetition_row_ids(type_de_champ)
Expand Down Expand Up @@ -84,7 +84,7 @@ def update_champs_attributes(attributes, scope, updated_by:)
end

def repetition_row_ids(type_de_champ)
[] if !type_de_champ.repetition?
return [] if !type_de_champ.repetition?

stable_ids = revision.children_of(type_de_champ).map(&:stable_id)
champs.filter { _1.stable_id.in?(stable_ids) && _1.row_id.present? }
Expand All @@ -98,10 +98,10 @@ def repetition_add_row(type_de_champ, updated_by:)

row_id = ULID.generate
types_de_champ = revision.children_of(type_de_champ)
# TODO: clean this up when parent_id is deprecated
added_champs = types_de_champ.map { _1.build_champ(row_id:, updated_by:) }
self.champs += types_de_champ.map { _1.build_champ(row_id:, updated_by:) }
champs.reload if persisted?
@champs_by_public_id = nil
[row_id, added_champs]
row_id
end

def repetition_remove_row(type_de_champ, row_id, updated_by:)
Expand Down Expand Up @@ -158,13 +158,6 @@ def champ_with_attributes_for_update(type_de_champ, row_id, updated_by:)
attributes[:data] = nil
end

parent = revision.parent_of(type_de_champ)
if parent.present?
attributes[:parent] = champs.find { _1.stable_id == parent.stable_id }
else
attributes[:parent] = nil
end

@champs_by_public_id = nil

[champ, attributes]
Expand Down
32 changes: 7 additions & 25 deletions app/models/concerns/dossier_clone_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@
kopy.state = Dossier.states.fetch(:brouillon)
kopy.champs = cloned_champs.values.map do |(_, champ)|
champ.dossier = kopy
champ.parent = cloned_champs[champ.parent_id].second if champ.child?
champ
end
end
Expand Down Expand Up @@ -152,33 +151,16 @@
def apply_diff(diff)
champs_index = (champs_for_revision(scope: :public) + diff[:added]).index_by(&:public_id)

diff[:added].each do |champ|
if champ.child?
champ.update_columns(dossier_id: id, parent_id: champs_index.fetch(champ.parent.public_id).id)
else
champ.update_column(:dossier_id, id)
end
end
diff[:added].each { _1.update_column(:dossier_id, id) }

champs_to_remove = []
# a bit of a hack to work around unicity index
remove_group_id = ULID.generate
diff[:updated].each do |champ|
old_champ = champs_index.fetch(champ.public_id)
champs_to_remove << old_champ

if champ.child?
# we need to do that in order to avoid a foreign key constraint
old_champ.update(row_id: nil)
champ.update_columns(dossier_id: id, parent_id: champs_index.fetch(champ.parent.public_id).id)
else
champ.update_column(:dossier_id, id)
end
champs_index.fetch(champ.public_id).update(row_id: remove_group_id)
champ.update_column(:dossier_id, id)

Check warning on line 160 in app/models/concerns/dossier_clone_concern.rb

View check run for this annotation

Codecov / codecov/patch

app/models/concerns/dossier_clone_concern.rb#L159-L160

Added lines #L159 - L160 were not covered by tests
end

champs_to_remove += diff[:removed]
children_champs_to_remove, root_champs_to_remove = champs_to_remove.partition(&:child?)

children_champs_to_remove.each(&:destroy!)
Champ.where(parent_id: root_champs_to_remove.map(&:id)).destroy_all
root_champs_to_remove.each(&:destroy!)
Champ.where(row_id: remove_group_id).destroy_all
diff[:removed].each(&:destroy!)
end
end
30 changes: 11 additions & 19 deletions app/models/concerns/dossier_rebase_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ def rebase
.tap { _1.default = Champ.none }

# remove champ
children_champ, root_champ = changes_by_op[:remove].partition(&:child?)
children_champ.each { champs_by_stable_id[_1.stable_id].destroy_all }
root_champ.each { champs_by_stable_id[_1.stable_id].destroy_all }
changes_by_op[:remove].each { champs_by_stable_id[_1.stable_id].destroy_all }

# update champ
changes_by_op[:update].each { apply(_1, champs_by_stable_id[_1.stable_id]) }
Expand All @@ -78,8 +76,6 @@ def rebase
# add champ (after changing dossier revision to avoid errors)
changes_by_op[:add]
.map { target_coordinates_by_stable_id[_1.stable_id] }
# add parent champs first so we can then add children
.sort_by { _1.child? ? 1 : 0 }
.each { add_new_champs_for_revision(_1) }
end

Expand Down Expand Up @@ -119,28 +115,24 @@ def apply(change, champs)

def add_new_champs_for_revision(target_coordinate)
if target_coordinate.child?
# If this type de champ is a child, we create a new champ for each row of the parent
parent_stable_id = target_coordinate.parent.stable_id

champs.filter { _1.stable_id == parent_stable_id }.each do |champ_repetition|
if champ_repetition.champs.present?
champ_repetition.champs.map(&:row_id).uniq.each do |row_id|
champs << create_champ(target_coordinate, champ_repetition, row_id:)
end
elsif target_coordinate.parent.mandatory?
champs << create_champ(target_coordinate, champ_repetition, row_id: ULID.generate)
row_ids = repetition_row_ids(target_coordinate.parent.type_de_champ)

if row_ids.present?
row_ids.each do |row_id|
create_champ(target_coordinate, row_id:)
end
elsif target_coordinate.parent.mandatory?
create_champ(target_coordinate, row_id: ULID.generate)
end
else
create_champ(target_coordinate, self)
create_champ(target_coordinate)
end
end

def create_champ(target_coordinate, parent, row_id: nil)
target_coordinate
def create_champ(target_coordinate, row_id: nil)
self.champs << target_coordinate
.type_de_champ
.build_champ(rebased_at: Time.zone.now, row_id:)
.tap { parent.champs << _1 }
end

def purge_piece_justificative_file(champ)
Expand Down
12 changes: 4 additions & 8 deletions app/models/dossier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,7 @@ class Dossier < ApplicationRecord

has_one_attached :justificatif_motivation

has_many :champs
# We have to remove champs in a particular order - champs with a reference to a parent have to be
# removed first, otherwise we get a foreign key constraint error.
has_many :champs_to_destroy, -> { order(:parent_id) }, class_name: 'Champ', inverse_of: false, dependent: :destroy

has_many :champs, dependent: :destroy
has_many :commentaires, inverse_of: :dossier, dependent: :destroy
has_many :preloaded_commentaires, -> { includes(:dossier_correction, piece_jointe_attachments: :blob) }, class_name: 'Commentaire', inverse_of: :dossier

Expand Down Expand Up @@ -1160,12 +1156,12 @@ def build_default_champs

def build_default_champs_for(types_de_champ)
self.champs << types_de_champ.flat_map do |type_de_champ|
champ = type_de_champ.build_champ(dossier: self)
if type_de_champ.repetition? && (type_de_champ.private? || type_de_champ.mandatory?)
row_id = ULID.generate
parent = type_de_champ.build_champ(dossier: self)
[parent] + revision.children_of(type_de_champ).map { _1.build_champ(dossier: self, parent:, row_id:) }
[champ] + revision.children_of(type_de_champ).map { _1.build_champ(dossier: self, row_id:) }
else
type_de_champ.build_champ(dossier: self)
champ
end
end
end
Expand Down
12 changes: 0 additions & 12 deletions app/models/dossier_preloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,20 +81,8 @@ def load_dossier(dossier, champs, pj_template: false)
end
dossier.association(:champs).target = champs

# remove once parent_id is deprecated
champs_by_parent_id = champs.group_by(&:parent_id)

champs.each do |champ|
champ.association(:dossier).target = dossier

# remove once parent_id is deprecated
if champ.repetition?
children = champs_by_parent_id.fetch(champ.id, [])
children.each do |child|
child.association(:parent).target = champ
end
champ.association(:champs).target = children
end
end

# We need to do this because of the check on `Etablissement#champ` in
Expand Down

This file was deleted.

8 changes: 8 additions & 0 deletions db/migrate/20240321081721_remove_champs_foreign_key.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

class RemoveChampsForeignKey < ActiveRecord::Migration[7.0]
def change
remove_foreign_key :champs, column: :parent_id
remove_index :champs, :parent_id
end
end
2 changes: 0 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@
t.index ["dossier_id", "stream", "stable_id", "row_id"], name: "index_champs_on_dossier_id_and_stream_and_stable_id_and_row_id", unique: true
t.index ["dossier_id"], name: "index_champs_on_dossier_id"
t.index ["etablissement_id"], name: "index_champs_on_etablissement_id"
t.index ["parent_id"], name: "index_champs_on_parent_id"
t.index ["row_id"], name: "index_champs_on_row_id"
t.index ["stable_id"], name: "index_champs_on_stable_id"
t.index ["type"], name: "index_champs_on_type"
Expand Down Expand Up @@ -1264,7 +1263,6 @@
add_foreign_key "avis", "experts_procedures"
add_foreign_key "batch_operations", "instructeurs"
add_foreign_key "bulk_messages", "procedures"
add_foreign_key "champs", "champs", column: "parent_id"
add_foreign_key "champs", "dossiers"
add_foreign_key "champs", "etablissements"
add_foreign_key "champs", "types_de_champ"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
let(:types_de_champ_public) { [] }
let(:types_de_champ_private) { [] }
let(:dossier) { create(:dossier, :with_populated_champs, procedure:) }
let(:champ) { dossier.champs.first }
let(:champ) { (dossier.project_champs_public + dossier.project_champs_private).first }

let(:component) { described_class.new(form: nil, champ:) }

Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/instructeurs/dossiers_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1037,7 +1037,7 @@
primary_value: 'primary',
secondary_value: 'secondary'
},
champ_repetition.champs.first.public_id => {
champ_repetition.rows.first.first.public_id => {
value: 'text'
},
champ_drop_down_list.public_id => {
Expand All @@ -1054,7 +1054,7 @@
expect(champ_linked_drop_down_list.primary_value).to eq('primary')
expect(champ_linked_drop_down_list.secondary_value).to eq('secondary')
expect(champ_datetime.value).to eq(Time.zone.parse('2019-12-21T13:17:00').iso8601)
expect(champ_repetition.champs.first.value).to eq('text')
expect(champ_repetition.rows.first.first.value).to eq('text')
expect(champ_drop_down_list.value).to eq('other value')
expect(dossier.reload.last_champ_private_updated_at).to eq(now)
expect(response).to have_http_status(200)
Expand Down
4 changes: 2 additions & 2 deletions spec/factories/champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,8 @@

evaluator.rows.times do
row_id = ULID.generate
champ_repetition.champs << types_de_champ.map do |type_de_champ|
attrs = { dossier: champ_repetition.dossier, parent: champ_repetition, private: champ_repetition.private?, stable_id: type_de_champ.stable_id, row_id: }
champ_repetition.dossier.champs << types_de_champ.map do |type_de_champ|
attrs = { dossier: champ_repetition.dossier, private: champ_repetition.private?, stable_id: type_de_champ.stable_id, row_id: }
build(:"champ_do_not_use_#{type_de_champ.type_champ}", **attrs)
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/graphql/annotation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@
end

it 'add row' do
expect(annotation.champs.size).to eq(4)
expect(annotation.row_ids.size).to eq(2)
expect(data).to eq(dossierModifierAnnotationAjouterLigne: {
annotation: {
id: annotation.to_typed_id
},
errors: nil
})
expect(annotation.reload.champs.size).to eq(6)
expect(annotation.reload.row_ids.size).to eq(3)
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/lib/recovery/dossier_life_cycle_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def cleanup_export_file

expect(reloaded_dossier.champs.count).not_to be(0)

expect(repetition(reloaded_dossier).champs.map(&:type)).to match_array(["Champs::PieceJustificativeChamp"])
expect(repetition(reloaded_dossier).rows.flatten.map(&:type)).to match_array(["Champs::PieceJustificativeChamp"])
expect(pj_champ(reloaded_dossier).piece_justificative_file).to be_attached
expect(carte(reloaded_dossier).geo_areas).to be_present

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
stars.update(value: 4)
end

let(:representation) { described_class.new(libelle, dossier.champs[0].reload.rows) }
let(:representation) { described_class.new(libelle, champ_repetition.rows) }

describe '#to_s' do
it 'returns a key-value representation' do
Expand Down
Loading
Loading