From 84683c111e16e19c94a71f36f1d66c0745ad33d8 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 14 May 2018 12:41:27 +0200 Subject: CleanUp: Add `#destroy_routes_outside_referential` This builds on `#destroy_vehicle_journeys_outside_referential` (which will soon be removed in favour of this new method). It destroys orphaned routes in a referential. Refs #6854 --- app/models/clean_up.rb | 5 +++++ spec/models/clean_up_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/models/clean_up.rb b/app/models/clean_up.rb index 0f73e07b2..cf8908263 100644 --- a/app/models/clean_up.rb +++ b/app/models/clean_up.rb @@ -105,6 +105,11 @@ class CleanUp < ApplicationModel Chouette::VehicleJourney.joins(:route).where(["routes.line_id not in (?)", line_ids]).destroy_all end + def destroy_routes_outside_referential + line_ids = referential.metadatas.pluck(:line_ids).flatten.uniq + Chouette::Route.where(['line_id not in (?)', line_ids]).destroy_all + end + def destroy_vehicle_journeys Chouette::VehicleJourney.where("id not in (select distinct vehicle_journey_id from time_tables_vehicle_journeys)").destroy_all end diff --git a/spec/models/clean_up_spec.rb b/spec/models/clean_up_spec.rb index afd2d8721..cf15ab934 100644 --- a/spec/models/clean_up_spec.rb +++ b/spec/models/clean_up_spec.rb @@ -264,4 +264,24 @@ RSpec.describe CleanUp, :type => :model do } end end + + describe "#destroy_routes_outside_referential" do + let(:line_referential) { create(:line_referential) } + let(:line) { create(:line, line_referential: line_referential) } + let(:metadata) { create(:referential_metadata, lines: [line]) } + let(:referential) { create(:workbench_referential, metadatas: [metadata]) } + let(:cleaner) { create(:clean_up, referential: referential) } + + it "destroys routes not in the the referential" do + route = create(:route) + + cleaner.destroy_routes_outside_referential + + expect(Chouette::Route.exists?(route.id)).to be false + + line.routes.each do |route| + expect(route).not_to be_destroyed + end + end + end end -- cgit v1.2.3 From c048a5a7e5295b7a84d87ed4e850affe598d5588 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 14 May 2018 12:58:56 +0200 Subject: CleanUp#destroy_routes_outside_referential: Test cascade Ensure that `JourneyPattern`s and `VehicleJourney`s associated with orphaned routes get deleted in cascade. Refs #6854 --- spec/models/clean_up_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/models/clean_up_spec.rb b/spec/models/clean_up_spec.rb index cf15ab934..be169abbb 100644 --- a/spec/models/clean_up_spec.rb +++ b/spec/models/clean_up_spec.rb @@ -283,5 +283,17 @@ RSpec.describe CleanUp, :type => :model do expect(route).not_to be_destroyed end end + + it "cascades destruction of vehicle journeys and journey patterns" do + vehicle_journey = create(:vehicle_journey) + + cleaner.destroy_routes_outside_referential + + expect(Chouette::Route.exists?(vehicle_journey.route.id)).to be false + expect( + Chouette::JourneyPattern.exists?(vehicle_journey.journey_pattern) + ).to be false + expect(Chouette::VehicleJourney.exists?(vehicle_journey)).to be false + end end end -- cgit v1.2.3 From 307052d83619ca63532f46e1326f83450daefa10 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 14 May 2018 17:42:53 +0200 Subject: CleanUp: Remove `#destroy_vehicle_journeys_outside_referential` This method is superseded by `#destroy_routes_outside_referential`, which will cascade deletion to include vehicle journeys. Refs #6854 --- app/models/clean_up.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/models/clean_up.rb b/app/models/clean_up.rb index cf8908263..f668b9fda 100644 --- a/app/models/clean_up.rb +++ b/app/models/clean_up.rb @@ -43,7 +43,7 @@ class CleanUp < ApplicationModel end end - destroy_vehicle_journeys_outside_referential + destroy_routes_outside_referential # Disabled for the moment. See #5372 # destroy_time_tables_outside_referential @@ -100,11 +100,6 @@ class CleanUp < ApplicationModel destroy_time_tables(time_tables) end - def destroy_vehicle_journeys_outside_referential - line_ids = referential.metadatas.pluck(:line_ids).flatten.uniq - Chouette::VehicleJourney.joins(:route).where(["routes.line_id not in (?)", line_ids]).destroy_all - end - def destroy_routes_outside_referential line_ids = referential.metadatas.pluck(:line_ids).flatten.uniq Chouette::Route.where(['line_id not in (?)', line_ids]).destroy_all -- cgit v1.2.3 From 4ca91f7ccbb71013ffe869be8d7f5cf344d9917a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 14 May 2018 18:40:33 +0200 Subject: CleanUp spec: Pass ids to `#exists?` call Looks like I temporarily forgot that this method takes an ID. Refs #6854 --- spec/models/clean_up_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/clean_up_spec.rb b/spec/models/clean_up_spec.rb index be169abbb..8a96437fb 100644 --- a/spec/models/clean_up_spec.rb +++ b/spec/models/clean_up_spec.rb @@ -291,9 +291,9 @@ RSpec.describe CleanUp, :type => :model do expect(Chouette::Route.exists?(vehicle_journey.route.id)).to be false expect( - Chouette::JourneyPattern.exists?(vehicle_journey.journey_pattern) + Chouette::JourneyPattern.exists?(vehicle_journey.journey_pattern.id) ).to be false - expect(Chouette::VehicleJourney.exists?(vehicle_journey)).to be false + expect(Chouette::VehicleJourney.exists?(vehicle_journey.id)).to be false end end end -- cgit v1.2.3 From 72fd54c38b958d0be9d608a676e8fef84691d088 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 14 May 2018 18:42:59 +0200 Subject: CleanUp: Add `#run_methods` This method is coupled with a new virtual attribute that can be used in the initializer like: CleanUp.new(methods: [:destroy_routes]) The method will run all methods specified in the `:methods` list. The plan is to replace the calls to `destroy_routes` etc. with a call to this method. The result will be a more configurable clean-up process, allowing users to selectively choose what to clean up by declaring what methods in the `CleanUp` model to call. Refs #6854 --- app/models/clean_up.rb | 8 ++++++++ spec/models/clean_up_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/app/models/clean_up.rb b/app/models/clean_up.rb index f668b9fda..4656cdb31 100644 --- a/app/models/clean_up.rb +++ b/app/models/clean_up.rb @@ -16,6 +16,8 @@ class CleanUp < ApplicationModel where(referential_id: referential.id) end + attr_accessor :methods + def end_date_must_be_greater_that_begin_date if self.end_date && self.date_type == 'between' && self.begin_date >= self.end_date errors.add(:base, I18n.t('activerecord.errors.models.clean_up.invalid_period')) @@ -54,6 +56,12 @@ class CleanUp < ApplicationModel end end + def run_methods + return if methods.nil? + + methods.each { |method| send(method) } + end + def destroy_time_tables_between time_tables = Chouette::TimeTable.where('end_date < ? AND start_date > ?', self.end_date, self.begin_date) self.destroy_time_tables(time_tables) diff --git a/spec/models/clean_up_spec.rb b/spec/models/clean_up_spec.rb index 8a96437fb..f0c3a3233 100644 --- a/spec/models/clean_up_spec.rb +++ b/spec/models/clean_up_spec.rb @@ -296,4 +296,24 @@ RSpec.describe CleanUp, :type => :model do expect(Chouette::VehicleJourney.exists?(vehicle_journey.id)).to be false end end + + describe "#run_methods" do + let(:cleaner) { create(:clean_up) } + + it "calls methods in the :methods attribute" do + cleaner = create( + :clean_up, + methods: [:destroy_routes_outside_referential] + ) + + expect(cleaner).to receive(:destroy_routes_outside_referential) + cleaner.run_methods + end + + it "doesn't do anything if :methods is nil" do + cleaner = create(:clean_up) + + expect { cleaner.run_methods }.not_to raise_error + end + end end -- cgit v1.2.3 From 3667a7d4976a9e69ae990df9ba681f679e0ee4fa Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 15 May 2018 12:10:50 +0200 Subject: CleanUp#clean: Replace `#destroy_*` calls with `#run_methods` It turns out that we don't care about destroying orphaned data. From Alban: > Quand on duplique un JDD, on lance un CleanUp, sauf que la partie > "suppression des données orphelines" n'intéresse personne au final. Replace the calls to `#destory_{vehicle_journeys,journey_patterns,routes}` with one to `#run_methods`. This change allows callers to run any cleanup methods defined in `CleanUp`. For example: CleanUp.new(methods: [:destory_routes]) But now by default, none of these destroy methods are run, saving us from doing extra work that it turns out we don't need to bother doing. Refs #6854 --- app/models/clean_up.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/clean_up.rb b/app/models/clean_up.rb index 4656cdb31..a708a77ea 100644 --- a/app/models/clean_up.rb +++ b/app/models/clean_up.rb @@ -49,9 +49,8 @@ class CleanUp < ApplicationModel # Disabled for the moment. See #5372 # destroy_time_tables_outside_referential - destroy_vehicle_journeys - destroy_journey_patterns - destroy_routes + # Run caller-specified cleanup methods + run_methods end end end -- cgit v1.2.3 From ceec2e53425f8a1c726f4903d03e268386cb598e Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 15 May 2018 15:24:29 +0200 Subject: CleanUp: Add `#destroy_empty` Gives us a single call site to trigger the destruction of vehicle journeys, journey patterns, and routes without content. This was previously done directly in the `#clean` method, but since it's not needed during referential duplication (only during merges), we don't want to enable it by default. Thus now to activate the same old functionality, you would create a `CleanUp` like this: CleanUp.new(methods: [:destroy_empty]) Refs #6854 --- app/models/clean_up.rb | 6 ++++++ spec/models/clean_up_spec.rb | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/app/models/clean_up.rb b/app/models/clean_up.rb index a708a77ea..9cf2389c9 100644 --- a/app/models/clean_up.rb +++ b/app/models/clean_up.rb @@ -124,6 +124,12 @@ class CleanUp < ApplicationModel Chouette::Route.where("id not in (select distinct route_id from journey_patterns)").destroy_all end + def destroy_empty + destroy_vehicle_journeys + destroy_journey_patterns + destroy_routes + end + def overlapping_periods self.end_date = self.begin_date if self.date_type != 'between' Chouette::TimeTablePeriod.where('(period_start, period_end) OVERLAPS (?, ?)', self.begin_date, self.end_date) diff --git a/spec/models/clean_up_spec.rb b/spec/models/clean_up_spec.rb index f0c3a3233..f39ca2f2b 100644 --- a/spec/models/clean_up_spec.rb +++ b/spec/models/clean_up_spec.rb @@ -297,6 +297,18 @@ RSpec.describe CleanUp, :type => :model do end end + describe "#destroy_empty" do + it "calls the appropriate destroy methods" do + cleaner = create(:clean_up) + + expect(cleaner).to receive(:destroy_vehicle_journeys) + expect(cleaner).to receive(:destroy_journey_patterns) + expect(cleaner).to receive(:destroy_routes) + + cleaner.destroy_empty + end + end + describe "#run_methods" do let(:cleaner) { create(:clean_up) } -- cgit v1.2.3