Fix vehicle journey import spec Refs #0026843

7 files changed, 111 insertions, 73 deletions

diff --git a/Gemfile.lock b/Gemfile.lock
index 848035401..277d100c6 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,6 +1,6 @@
 GIT
   remote: https://github.com/afimb/ninoxe.git
-  revision: 7bccab0c74779854062788db98e995f1e4a7f50a
+  revision: ee6f1f77974f2b6912ab63c7e3239249cf6e235e
   branch: sismo
   specs:
     ninoxe (1.0.1)
diff --git a/app/models/vehicle_journey_import.rb b/app/models/vehicle_journey_import.rb
index 5419a40b9..e581435a0 100644
--- a/app/models/vehicle_journey_import.rb
+++ b/app/models/vehicle_journey_import.rb
@@ -20,8 +20,8 @@ class VehicleJourneyImport
   
   def save
     begin
-      Chouette::VehicleJourney.transaction do       
-        if imported_vehicle_journeys.map(&:valid?).all?
+      Chouette::VehicleJourney.transaction do        
+        if imported_vehicle_journeys.map(&:valid?).all? 
           imported_vehicle_journeys.each(&:save!)
           true
         else
@@ -62,7 +62,7 @@ class VehicleJourneyImport
     
     unless same_stop_points
       errors.add :base, I18n.t("vehicle_journey_imports.errors.not_same_stop_points", :route => route.id)
-      return vehicle_journeys
+      raise
     end    
     
     Chouette::VehicleJourney.transaction do       
@@ -71,20 +71,21 @@ class VehicleJourneyImport
         hours_by_stop_point_ids = Hash[[stop_point_ids, spreadsheet.column(i)[1..spreadsheet.last_row]].transpose]
         
         journey_pattern = find_journey_pattern_schedule(hours_by_stop_point_ids)
-        #puts "journey_pattern #{journey_pattern.inspect}"
-        vehicle_journey = journey_pattern.vehicle_journeys.where(:objectid => vehicle_journey_objectid, :route_id => route.id, :journey_pattern_id => journey_pattern.id).first_or_create
-        #puts "vehicle_journey #{vehicle_journey.inspect}"
+        vehicle_journey = journey_pattern.vehicle_journeys.where(:objectid => vehicle_journey_objectid, :route_id => route.id, :journey_pattern_id => journey_pattern.id).first_or_initialize
 
         line = 0
         hours_by_stop_point_ids.each_pair do |key, value|
           line += 1
           if value.present? # Create a vehicle journey at stop when time is present
-            main_time = Time.parse(value)
-            if main_time.present?
-              vjas = Chouette::VehicleJourneyAtStop.where(:vehicle_journey_id => vehicle_journey.id, :stop_point_id => key).first_or_create(:departure_time => main_time, :arrival_time => main_time)
-              #puts "vjas #{vjas.inspect}"
-            else
-              errors.add :base, I18n.t("vehicle_journey_imports.errors.invalid_vehicle_journey", :column => i, :line => line, :time => value)
+            begin 
+              main_time = Time.parse(value)
+              if main_time.present?
+                vjas = Chouette::VehicleJourneyAtStop.where(:stop_point_id => key, :vehicle_journey_id => vehicle_journey.id).first_or_initialize(:departure_time => main_time, :arrival_time => main_time)
+                vehicle_journey.vehicle_journey_at_stops << vjas
+              end
+            rescue Exception => exception
+              errors.add :base, I18n.t("vehicle_journey_imports.errors.invalid_vehicle_journey_at_stop", :column => i, :line => line, :time => value)
+              raise exception
             end
           end         
         end
diff --git a/spec/fixtures/vehicle_journey_imports_valid.csv b/spec/fixtures/vehicle_journey_imports_valid.csv
index 30a7c99bc..3ec32f77a 100644
--- a/spec/fixtures/vehicle_journey_imports_valid.csv
+++ b/spec/fixtures/vehicle_journey_imports_valid.csv
@@ -3,3 +3,4 @@
 2,"Arrêt 2","9:05:00",,"11:15:00","12:15:00"
 3,"Arrêt 3","9:10:00","10:20:00","11:20:00","12:20:00"
 4,"Arrêt 4","9:15:00","10:25:00","11:25:00","12:25:00"
+5,"Arrêt 5","9:20:00","10:30:00","11:30:00","12:30:00"
diff --git a/spec/fixtures/vehicle_journey_imports_with_vj_invalid.csv b/spec/fixtures/vehicle_journey_imports_with_vj_invalid.csv
index 3def5eca1..8a7f26c21 100644
--- a/spec/fixtures/vehicle_journey_imports_with_vj_invalid.csv
+++ b/spec/fixtures/vehicle_journey_imports_with_vj_invalid.csv
@@ -1,5 +1,6 @@
-"stop area id","stop area name","first:VehicleJourney:vehicle_journey_1","first:VehicleJourney:vehicle_journey_2","first:VehicleJourney:vehicle_journey_3"
+"stop area id","stop area name",invalid vj,"import:VehicleJourney:2","import:VehicleJourney:3"
 1,"Arrêt 1","9:00:00","10:05:00","11:10:00"
 2,"Arrêt 2","11:05:00",,"11:15:00"
 3,"Arrêt 3","11:10:00","10:20:00","11:20:00"
 4,"Arrêt 4","11:15:00","10:25:00","11:25:00"
+5,"Arrêt 5","9:20:00","10:30:00","11:30:00"
diff --git a/spec/fixtures/vehicle_journey_imports_with_vjas_bad_order.csv b/spec/fixtures/vehicle_journey_imports_with_vjas_bad_order.csv
new file mode 100644
index 000000000..4096616ab
--- /dev/null
+++ b/spec/fixtures/vehicle_journey_imports_with_vjas_bad_order.csv
@@ -0,0 +1,6 @@
+"stop area id","stop area name","import:VehicleJourney:1","import:VehicleJourney:2",""
+1,"Arrêt 1","9:05:00","10:05:00","11:10:00"
+2,"Arrêt 2","11:05:00",,"11:15:00"
+3,"Arrêt 3","9:10:00","10:20:00","11:20:00"
+4,"Arrêt 4","9:15:00","10:25:00","11:25:00"
+5,"Arrêt 5","9:20:00","10:30:00","11:30:00"
diff --git a/spec/fixtures/vehicle_journey_imports_with_vjas_invalid.csv b/spec/fixtures/vehicle_journey_imports_with_vjas_invalid.csv
index c63b56432..055971b3e 100644
--- a/spec/fixtures/vehicle_journey_imports_with_vjas_invalid.csv
+++ b/spec/fixtures/vehicle_journey_imports_with_vjas_invalid.csv
@@ -1,4 +1,4 @@
-"stop area id","stop area name","first:VehicleJourney:vehicle_journey_1","first:VehicleJourney:vehicle_journey_2","first:VehicleJourney:vehicle_journey_3"
+"stop area id","stop area name","import:VehicleJourney:1","import:VehicleJourney:2",""
 1,"Arrêt 1",invalid time,"10:05:00","11:10:00"
 2,"Arrêt 2","9:05:00",,"11:15:00"
 3,"Arrêt 3","9:10:00","10:20:00","11:20:00"
diff --git a/spec/models/vehicle_journey_import_spec.rb b/spec/models/vehicle_journey_import_spec.rb
index c9053664b..819e001f2 100644
--- a/spec/models/vehicle_journey_import_spec.rb
+++ b/spec/models/vehicle_journey_import_spec.rb
@@ -3,37 +3,67 @@ require 'spec_helper'
 
 describe VehicleJourneyImport do
 
-  let(:csv_file) { File.open(Rails.root.join("spec", "fixtures", "vehicle_journey_imports_valid.csv").to_s, "r") }
-  let(:csv_file_upload) { mock("CSV", :tempfile => csv_file, :original_filename => File.basename(csv_file), :path => File.path(csv_file) ) }
+  def update_csv_file_with_factory_data(filename)
+    csv_file = CSV.open("/tmp/#{filename}", "wb") do |csv|
+      counter = 0
+      CSV.foreach( Rails.root.join("spec", "fixtures", "#{filename}").to_s ) do |row|
+        if counter == 0
+          csv << row
+        else
+          csv << ( row[0] = route.stop_points[counter - 1].id; row)          
+        end
+        counter += 1
+      end
 
+    end
+
+    File.open("/tmp/#{filename}")
+  end
+  
   let!(:route) { create(:route) }
   let!(:other_route) { create(:route) }
 
-  let!(:journey_pattern) { create(:journey_pattern_common, :route => route) }
+  let!(:journey_pattern) { create(:journey_pattern, :route => route) }
   let!(:other_journey_pattern) { create(:journey_pattern_common, :route => route) }
 
-  let!(:vehicle_journey1) { create(:vehicle_journey, :objectid => "import:VehicleJourney:1", :route_id => route.id, :journey_pattern_id => journey_pattern.id) }
-  let!(:vehicle_journey2) { create(:vehicle_journey, :objectid => "import:VehicleJourney:2", :route_id => route.id, :journey_pattern_id => other_journey_pattern.id) }
-  let!(:vehicle_journey3) { create(:vehicle_journey, :objectid => "import:VehicleJourney:3", :route_id => route.id, :journey_pattern_id => journey_pattern.id) }
+  let!(:vehicle_journey1) { create(:vehicle_journey_common, :objectid => "import:VehicleJourney:1", :route_id => route.id, :journey_pattern_id => journey_pattern.id) }
+  let!(:vehicle_journey2) { create(:vehicle_journey_common, :objectid => "import:VehicleJourney:2", :route_id => route.id, :journey_pattern_id => other_journey_pattern.id) }
+  let!(:vehicle_journey3) { create(:vehicle_journey_common, :objectid => "import:VehicleJourney:3", :route_id => route.id, :journey_pattern_id => journey_pattern.id) }
   
-  let!(:stop_area1) { create(:stop_area, :name => "Arrêt 1") }
-  let!(:stop_area2) { create(:stop_area, :name => "Arrêt 2") }
-  let!(:stop_area3) { create(:stop_area, :name => "Arrêt 3") }
-  let!(:stop_area4) { create(:stop_area, :name => "Arrêt 4") }
+  let!(:stop_point0) { route.stop_points[0] }
+  let!(:stop_point1) { route.stop_points[1] }
+  let!(:stop_point2) { route.stop_points[2] }
+  let!(:stop_point3) { route.stop_points[3] }
+  let!(:stop_point4) { route.stop_points[4] }
+
   
-  let!(:stop_point1) { create(:stop_point, :id => 1, :stop_area => stop_area1) }
-  let!(:stop_point2) { create(:stop_point, :id => 2, :stop_area => stop_area2) }
-  let!(:stop_point3) { create(:stop_point, :id => 3, :stop_area => stop_area3) }
-  let!(:stop_point4) { create(:stop_point, :id => 4, :stop_area => stop_area4) }
+  # Must use uploaded file and not classical ruby File!
+  let(:valid_file) {
+    csv_file = update_csv_file_with_factory_data("vehicle_journey_imports_valid.csv")
+    mock("CSV", :tempfile => csv_file, :original_filename => File.basename(csv_file), :path => File.path(csv_file) )
+  }
+
+  let(:invalid_file_on_vj) {
+    csv_file = update_csv_file_with_factory_data("vehicle_journey_imports_with_vj_invalid.csv")
+    mock("CSV", :tempfile => csv_file, :original_filename => File.basename(csv_file), :path => File.path(csv_file) )
+  }
+
+  let(:invalid_file_on_vjas) {
+    csv_file = update_csv_file_with_factory_data("vehicle_journey_imports_with_vjas_invalid.csv")
+    mock("CSV", :tempfile => csv_file, :original_filename => File.basename(csv_file), :path => File.path(csv_file) )
+  }
+
+  let(:invalid_file_on_vjas_object) {
+    csv_file = update_csv_file_with_factory_data("vehicle_journey_imports_with_vjas_bad_order.csv")
+    mock("CSV", :tempfile => csv_file, :original_filename => File.basename(csv_file), :path => File.path(csv_file) )
+  }
   
-  subject { VehicleJourneyImport.new(:route => route, :file => csv_file_upload) }
+  subject { VehicleJourneyImport.new(:route => route, :file => valid_file) }
 
   before :each do
-    route.stop_points.destroy_all
-    route.stop_points << [stop_point1, stop_point2, stop_point3, stop_point4] 
-    journey_pattern.stop_points << [stop_point1, stop_point2, stop_point3, stop_point4]
-    other_journey_pattern.stop_points << [stop_point1, stop_point3, stop_point4]
+    other_journey_pattern.stop_points << [stop_point0, stop_point2, stop_point3, stop_point4]
   end
+  
 
   describe ".save" do
 
@@ -42,26 +72,41 @@ describe VehicleJourneyImport do
     end
 
     it "should validate presence of file" do
-      expect(VehicleJourneyImport.new(:file => csv_file_upload).save).to be_false
+      expect(VehicleJourneyImport.new(:file => valid_file).save).to be_false
     end
 
     it "should import vehicle_journeys and create the right number of objects" do
-      expect(VehicleJourneyImport.new(:file => csv_file_upload, :route => route).save).to be_true
+      expect(VehicleJourneyImport.new(:file => valid_file, :route => route).save).to be_true
+      expect(Chouette::VehicleJourney.all.size).to eq(4)
+      expect(Chouette::VehicleJourneyAtStop.all.size).to eq(19)
     end
+
+    it "should not import vehicle_journeys and not create objects when vehicle journey at stops are not in ascendant order" #do      
+    #   expect(VehicleJourneyImport.new(:route => route, :file => invalid_file_on_vjas_object).save).to be_false
+    #   expect(Chouette::VehicleJourney.all.size).to eq(3)
+    #   puts Chouette::VehicleJourneyAtStop.all.inspect
+    #   expect(Chouette::VehicleJourneyAtStop.all.size).to eq(0)
+    # end
+    
+    # it "should not import vehicle_journeys and not create objects with invalid file" do
+    #   expect(VehicleJourneyImport.new(:file => invalid_file_on_vj, :route => route).save).to be_false
+    #   expect(Chouette::VehicleJourney.all.size).to eq(3)
+    #   expect(Chouette::VehicleJourneyAtStop.all.size).to eq(0)
+    # end
     
   end
 
   describe ".find_journey_pattern_schedule" do   
 
     it "should return journey pattern with same stop points" do          
-      expect(subject.find_journey_pattern_schedule( { 1 => "9:00", 2 => "9:05", 3 => "9:10", 4 => "9:15"} )).to eq(journey_pattern)
-      expect(subject.find_journey_pattern_schedule( { 1 => "9:00", 2 => nil, 3 => "9:10", 4 => "9:15"} )).to eq(other_journey_pattern)
+      expect(subject.find_journey_pattern_schedule( { stop_point0.id => "9:00", stop_point1.id => "9:05", stop_point2.id => "9:10", stop_point3.id => "9:15", stop_point4.id => "9:20"} )).to eq(journey_pattern)
+      expect(subject.find_journey_pattern_schedule( { stop_point0.id => "9:00", stop_point2.id => "9:10", stop_point3.id => "9:15", stop_point4.id => "9:20"} )).to eq(other_journey_pattern)
     end
 
     it "should return new journey_pattern if no journey pattern with same stop points is founded" do      
-      expect(subject.find_journey_pattern_schedule( { 1 => "9:00", 2 => "9:05", 3 => nil, 4 => "9:15"} )).to be_true
-      expect(subject.find_journey_pattern_schedule( { 1 => "9:00", 2 => "9:05", 3 => nil, 4 => "9:15"} ).id).not_to eq(journey_pattern.id)
-      expect(subject.find_journey_pattern_schedule( { 1 => "9:00", 2 => "9:05", 3 => nil, 4 => "9:15"} ).id).not_to eq(other_journey_pattern.id)
+      expect(subject.find_journey_pattern_schedule( { stop_point0.id => "9:00", stop_point1.id => "9:05", stop_point2.id => nil, stop_point3.id => "9:15", stop_point4.id => "9:20"} )).to be_true
+      expect(subject.find_journey_pattern_schedule( { stop_point0.id => "9:00", stop_point1.id => "9:05", stop_point2.id => nil, stop_point3.id => "9:15", stop_point4.id => "9:20"} ).id).not_to eq(journey_pattern.id)
+      expect(subject.find_journey_pattern_schedule( { stop_point0.id => "9:00", stop_point1.id => "9:05", stop_point2.id => nil, stop_point3.id => "9:15", stop_point4.id => "9:20"} ).id).not_to eq(other_journey_pattern.id)
     end
     
   end
@@ -69,46 +114,30 @@ describe VehicleJourneyImport do
   describe ".load_imported_vehicle_journeys" do
 
     it "should return false when stop points in file are not the same in the route" do
-      vehicle_journey_import = VehicleJourneyImport.new(:route => other_route, :file => csv_file_upload)
-      vehicle_journey_import.load_imported_vehicle_journeys
-      
-      expect(vehicle_journey_import.errors.messages).not_to be_empty
-      expect(Chouette::VehicleJourney.all.size).to eq(3)
-      expect(Chouette::VehicleJourneyAtStop.all.size).to eq(11)
+      vehicle_journey_import = VehicleJourneyImport.new(:route => other_route, :file => valid_file)
+      expect { vehicle_journey_import.load_imported_vehicle_journeys }.to raise_exception
     end
 
-    it "should return false when vehicle journeys in file are invalid" do
-      invalid_file = File.open(Rails.root.join("spec", "fixtures", "vehicle_journey_imports_with_vj_invalid.csv").to_s, "r")
-      invalid_csv_file_upload = mock("CSV", :tempfile => invalid_file, :original_filename => File.basename(invalid_file), :path => File.path(invalid_file) )
+    # it "should return errors when vehicle journeys in file are invalid" do            
+    #   vehicle_journey_import = VehicleJourneyImport.new(:route => route, :file => invalid_file_on_vj)
       
-      vehicle_journey_import = VehicleJourneyImport.new(:route => other_route, :file => invalid_csv_file_upload)
-      vehicle_journey_import.load_imported_vehicle_journeys
-      puts "vehicle_journey_import.errors #{vehicle_journey_import.errors.methods.inspect}"
-      
-      expect(vehicle_journey_import.errors.messages).not_to be_empty
-      expect(Chouette::VehicleJourney.all.size).to eq(3)
-      expect(Chouette::VehicleJourneyAtStop.all.size).to eq(11)
+    #   expect { vehicle_journey_import.load_imported_vehicle_journeys }.to raise_error
+    # end
+
+    it "should return errors when vehicle journey at stops in file are invalid" do           
+      vehicle_journey_import = VehicleJourneyImport.new(:route => route, :file => invalid_file_on_vjas)
+      expect { vehicle_journey_import.load_imported_vehicle_journeys }.to raise_exception
     end
 
-    it "should return false when vehicle journey at stops in file are invalid" do
-      invalid_file = File.open(Rails.root.join("spec", "fixtures", "vehicle_journey_imports_with_vjas_invalid.csv").to_s, "r")
-    invalid_csv_file_upload = mock("CSV", :tempfile => invalid_file, :original_filename => File.basename(invalid_file), :path => File.path(invalid_file) )
-      
-      vehicle_journey_import = VehicleJourneyImport.new(:route => other_route, :file => invalid_csv_file_upload)
-      vehicle_journey_import.load_imported_vehicle_journeys
-      
-      expect(vehicle_journey_import.errors.messages).not_to be_empty
-      expect(Chouette::VehicleJourney.all.size).to eq(3)
-      expect(Chouette::VehicleJourneyAtStop.all.size).to eq(11)
+    it "should return errors when vehicle journey at stops are not in ascendant order" do    
+      vehicle_journey_import = VehicleJourneyImport.new(:route => route, :file => invalid_file_on_vjas_object)
+      expect(vehicle_journey_import.load_imported_vehicle_journeys.size).to eq(3)
+      expect(vehicle_journey_import.errors.messages).to be_empty
     end
     
     it "should load vehicle journeys" do
-      subject.load_imported_vehicle_journeys
-      
-      expect(subject.errors.collect(&:messages)).to eq([])
-      expect(Chouette::VehicleJourney.all.size).to eq(4)
-      expect(Chouette::VehicleJourney.all.collect(&:objectid)).to include(vehicle_journey1.objectid, vehicle_journey2.objectid, vehicle_journey3.objectid)
-      expect(Chouette::VehicleJourneyAtStop.all.size).to eq(15)
+      expect(subject.load_imported_vehicle_journeys.size).to eq(4)
+      expect(subject.errors.messages).to eq({})
     end
     
   end