Merge pull request #429 from Skud/tomatoes

Tomatoes - scientific name upload bugfix

app/models/crop.rb

@@ -33,11 +33,6 @@ class Crop < ActiveRecord::Base
       :message => 'is not a valid English Wikipedia URL'
     }
 
-  def Crop.random
-    @crop = Crop.offset(rand(Crop.count)).first
-    return @crop
-  end
-
   def to_s
     return name
   end
@@ -130,52 +125,55 @@ def Crop.interesting
 # - en_wikipedia_url (required)
 # - parent (name, optional)
 
-  def Crop.create_from_csv(row, definitely_new=false)
+  def Crop.create_from_csv(row)
     name,scientific_name,en_wikipedia_url,parent = row
 
-    @cropbot = Member.find_by_login_name('cropbot')
-    raise "cropbot account not found: run rake db:seed" unless @cropbot
+    cropbot = Member.find_by_login_name('cropbot')
+    raise "cropbot account not found: run rake db:seed" unless cropbot
+
+    crop = Crop.find_or_create_by_name(name)
+    crop.update_attributes(
+      :en_wikipedia_url => en_wikipedia_url,
+      :creator_id => cropbot.id
+    )
 
-    if definitely_new then
-      @crop = Crop.create(
-        :name => name,
-        :en_wikipedia_url => en_wikipedia_url,
-        :creator_id => @cropbot.id
-      )
-    else
-      @crop = Crop.find_or_create_by_name(name)
-      @crop.update_attributes(
-        :en_wikipedia_url => en_wikipedia_url,
-        :creator_id => @cropbot.id
-      )
-    end
     if parent
-      @parent = Crop.find_by_name(parent)
-      if @parent
-        @crop.update_attributes(:parent_id => @parent.id)
+      parent = Crop.find_by_name(parent)
+      if parent
+        crop.update_attributes(:parent_id => parent.id)
       else
         logger.warn("Warning: parent crop #{parent} not found")
       end
     end
 
-    unless @crop.scientific_names.exists?(:scientific_name => scientific_name)
-      @sn = ''
-      if scientific_name
-        @sn = scientific_name
-      elsif @crop.parent
-        @sn = @crop.parent.scientific_names.first.scientific_name
-      end
+    crop.add_scientific_name_from_csv(scientific_name)
 
-      if @sn
-        @crop.scientific_names.create(
-          :scientific_name => @sn,
-          :creator_id => @cropbot.id
-        )
+  end
+
+  def add_scientific_name_from_csv(scientific_name)
+    name_to_add = nil
+    if ! scientific_name.blank? # i.e. we actually passed one in, which isn't a given
+      name_to_add = scientific_name
+    elsif parent && parent.default_scientific_name
+      name_to_add = parent.default_scientific_name
+    else
+      logger.warn("Warning: no scientific name (not even on parent crop) for #{self}")
+    end
+
+    if name_to_add
+      if scientific_names.exists?(:scientific_name => name_to_add)
+        logger.warn("Warning: skipping duplicate scientific name #{name_to_add} for #{self}")
       else
-        logger.warn("Warning: no scientific name (not even on parent crop) for #{@crop}")
-      end
+        cropbot = Member.find_by_login_name('cropbot')
+        raise "cropbot account not found: run rake db:seed" unless cropbot
 
+        scientific_names.create(
+          :scientific_name => name_to_add,
+          :creator_id => cropbot.id
+        )
+      end
     end
+
   end
 
   # Crop.search(string)

db/seeds.rb

@@ -29,7 +29,7 @@ def load_crops
   Dir.glob("#{source_path}/crops*.csv").each do |crop_file|
     puts "Loading crops from #{crop_file}..."
     CSV.foreach(crop_file) do |row|
-      Crop.create_from_csv(row, definitely_new=true)
+      Crop.create_from_csv(row)
     end
   end
   puts "Finished loading crops"

spec/factories/member.rb

@@ -11,6 +11,11 @@
     show_email false
     bio 'I love seeds'
 
+    # cropbot is needed for certain tests, eg. Crop.create_from_csv
+    factory :cropbot do
+      login_name 'cropbot'
+    end
+
     factory :no_tos_member do
       tos_agreement false
     end

spec/models/crop_spec.rb

@@ -343,6 +343,116 @@
     end
   end
 
+  context "csv loading" do
+
+    before(:each) do
+      # don't use 'let' for this -- we need to actually create it,
+      # regardless of whether it's used.
+      @cropbot = FactoryGirl.create(:cropbot)
+    end
+
+    it "adds a scientific name to a crop that has none" do
+      tomato = FactoryGirl.create(:tomato)
+      expect(tomato.scientific_names.size).to eq 0
+      tomato.add_scientific_name_from_csv("Foo bar")
+      expect(tomato.scientific_names.size).to eq 1
+      expect(tomato.default_scientific_name).to eq "Foo bar"
+    end
+
+    it "picks up scientific name from parent crop if available" do
+      parent = FactoryGirl.create(:crop, :name => 'parent crop')
+      parent.add_scientific_name_from_csv("Parentis cropis")
+      parent.save
+      parent.reload
+
+      tomato = FactoryGirl.create(:tomato, :parent => parent)
+      expect(tomato.parent).to eq parent
+      expect(tomato.parent.default_scientific_name).to eq "Parentis cropis"
+
+      tomato.add_scientific_name_from_csv('')
+      expect(tomato.default_scientific_name).to eq "Parentis cropis"
+    end
+
+    it "doesn't add a duplicate scientific name" do
+      tomato = FactoryGirl.create(:tomato)
+      expect(tomato.scientific_names.size).to eq 0
+      tomato.add_scientific_name_from_csv("Foo bar")
+      expect(tomato.scientific_names.size).to eq 1
+      tomato.add_scientific_name_from_csv("Foo bar")
+      expect(tomato.scientific_names.size).to eq 1 # shouldn't increase
+      tomato.add_scientific_name_from_csv("Baz quux")
+      expect(tomato.scientific_names.size).to eq 2
+    end
+
+    it "doesn't add a duplicate scientific name from parent" do
+      parent = FactoryGirl.create(:crop, :name => 'parent')
+      parent.add_scientific_name_from_csv("Parentis cropis")
+      parent.save
+      parent.reload
+
+      tomato = FactoryGirl.create(:tomato, :parent => parent)
+      expect(tomato.scientific_names.size).to eq 0
+      tomato.add_scientific_name_from_csv('')
+      expect(tomato.scientific_names.size).to eq 1 # picks up parent SN
+      tomato.add_scientific_name_from_csv('')
+      expect(tomato.scientific_names.size).to eq 1 # shouldn't increase now
+    end
+
+
+    it "loads the simplest possible crop" do
+      tomato_row = "tomato,,http://en.wikipedia.org/wiki/Tomato"
+
+      CSV.parse(tomato_row) do |row|
+        Crop.create_from_csv(row)
+      end
+
+      loaded = Crop.last
+      expect(loaded.name).to eq "tomato"
+      expect(loaded.en_wikipedia_url).to eq 'http://en.wikipedia.org/wiki/Tomato'
+      expect(loaded.creator).to eq @cropbot
+    end
+
+    it "loads a crop with a scientific name" do
+      tomato_row = "tomato,Solanum lycopersicum,http://en.wikipedia.org/wiki/Tomato"
+
+      CSV.parse(tomato_row) do |row|
+        Crop.create_from_csv(row)
+      end
+
+      loaded = Crop.last
+      expect(loaded.name).to eq "tomato"
+      expect(loaded.scientific_names.size).to eq 1
+      expect(loaded.scientific_names.last.scientific_name).to eq "Solanum lycopersicum"
+    end
+
+    it "loads a crop with a parent" do
+      parent = FactoryGirl.create(:crop, :name => 'parent')
+      tomato_row = "tomato,,http://en.wikipedia.org/wiki/Tomato,parent"
+
+      CSV.parse(tomato_row) do |row|
+        Crop.create_from_csv(row)
+      end
+
+      loaded = Crop.last
+      expect(loaded.parent).to eq parent
+    end
+
+    it "doesn't add unnecessary duplicate crops" do
+      tomato_row = "tomato,Solanum lycopersicum,http://en.wikipedia.org/wiki/Tomato"
+
+      CSV.parse(tomato_row) do |row|
+        Crop.create_from_csv(row)
+      end
+
+      loaded = Crop.last
+      expect(loaded.name).to eq "tomato"
+      expect(loaded.en_wikipedia_url).to eq 'http://en.wikipedia.org/wiki/Tomato'
+      expect(loaded.creator).to eq @cropbot
+
+    end
+
+  end
+
   context "crop-post association" do
     let!(:tomato) { FactoryGirl.create(:tomato) }
     let!(:maize) { FactoryGirl.create(:maize) }