Merge pull request #859 from ElixirTeSS/node-country-fix

Validate node country

app/helpers/nodes_helper.rb

@@ -27,11 +27,11 @@ def node_staff_list(staff, show_role = true, link: true)
   end
 
   def country_flag(country_code, options = {})
-    image_tag "nodes/flags/#{country_code}.png", options
+    image_tag("nodes/flags/#{country_code}.png", options) if country_code.present?
   end
 
   def countries_options_for_select
-    Node::COUNTRIES.map {|k, v| [v + " (#{k})", k] }.to_h
+    Node::COUNTRIES.map {|k, v| [v + " (#{k})", k] }.sort_by { |o| o[0] }
   end
 
   def elixir_node_icon(opts = {})

app/models/node.rb

@@ -1,4 +1,6 @@
 class Node < ApplicationRecord
+  MEMBER_STATUS = ['Member', 'Observer']
+  COUNTRIES = JSON.parse(File.read(File.join(Rails.root, 'config', 'data', 'countries.json')))
 
   include PublicActivity::Common
   include LogParameterChanges
@@ -26,6 +28,7 @@ class Node < ApplicationRecord
 
   validates :name, presence: true, uniqueness: true
   validates :home_page, format: { with: URI.regexp }, if: Proc.new { |a| a.home_page.present? }
+  validates :country_code, inclusion: { in: COUNTRIES.keys, allow_blank: true }
   # validate :has_training_coordinator
 
   alias_attribute(:title, :name)
@@ -50,9 +53,6 @@ class Node < ApplicationRecord
     # :nocov:
   end
 
-  MEMBER_STATUS = ['Member', 'Observer']
-  COUNTRIES = JSON.parse(File.read(File.join(Rails.root, 'config', 'data', 'countries.json')))
-
   def related_events
     Event.where(id: provider_event_ids | event_ids)
   end

config/data/countries.json

@@ -248,5 +248,6 @@
   "ID": "Indonesia",
   "UA": "Ukraine",
   "QA": "Qatar",
-  "MZ": "Mozambique"
-}
+  "MZ": "Mozambique",
+  "EMBL-EBI": "EMBL-EBI"
+}

test/controllers/nodes_controller_test.rb

@@ -8,7 +8,7 @@ class NodesControllerTest < ActionController::TestCase
     @node = nodes(:good)
     @node_attributes = {
         carousel_images: '',
-        country_code: 'FN',
+        country_code: 'FI',
         home_page: 'http://www.example.com', #institutions: '',
         member_status: 'Member',
         name: 'Finnland',
@@ -123,26 +123,26 @@ class NodesControllerTest < ActionController::TestCase
 
     patch :update, params: {
         id: @node,
-        node: { carousel_images: @node.carousel_images, country_code: ':)',
+        node: { carousel_images: @node.carousel_images, country_code: 'EE',
                 home_page: @node.home_page, #institutions: @node.institutions,
                 member_status: @node.member_status, name: @node.name,
                 twitter: @node.twitter
         }
     }
     assert_redirected_to node_path(assigns(:node))
-    assert_equal ':)', assigns(:node).country_code
+    assert_equal 'EE', assigns(:node).country_code
   end
 
   test "should not allow update if non-admin, non-owner" do
     sign_in users(:another_regular_user)
 
-    patch :update, params: { id: @node, node: { country_code: ':)' } }
+    patch :update, params: { id: @node, node: { country_code: 'EE' } }
 
     assert_response :forbidden
   end
 
   test "should not allow update if not logged-in" do
-    patch :update, params: { id: @node, node: { country_code: ':)' } }
+    patch :update, params: { id: @node, node: { country_code: 'EE' } }
 
     assert_redirected_to new_user_session_path
   end
@@ -229,4 +229,14 @@ class NodesControllerTest < ActionController::TestCase
 
     assert_redirected_to nodes_path
   end
+
+  test 'should cope with node without country' do
+    n = Node.create(user: users(:admin), name: 'No Country Node')
+    assert n.valid?
+    assert_nil n.country_code
+
+    get :index, params: { sort: 'new' }
+
+    assert_select 'h4', text: 'No Country Node'
+  end
 end

test/fixtures/files/node_test_data.json

@@ -2,7 +2,7 @@
   "nodes": [
     {
       "name": "Narnia",
-      "country_code": "NN",
+      "country_code": "DE",
       "member_status": "Member",
       "home_page": "",
       "image_url": "",

test/models/node_test.rb

@@ -32,18 +32,18 @@ class NodeTest < ActiveSupport::TestCase
     nodes = Node.load_from_hash(hash)
     node_ids = nodes.map(&:id).sort
     narnia = nodes.first
-    assert_equal 'NN', narnia.country_code
+    assert_equal 'DE', narnia.country_code
 
     updated_hash = node_data_hash
-    updated_hash['nodes'].first['country_code'] = 'XY'
+    updated_hash['nodes'].first['country_code'] = 'ES'
     updated_nodes = []
     assert_no_difference('Node.count') do
       assert_no_difference('StaffMember.count') do
         updated_nodes = Node.load_from_hash(updated_hash)
       end
     end
     assert_equal node_ids, updated_nodes.map(&:id).sort
-    assert_equal 'XY', narnia.reload.country_code
+    assert_equal 'ES', narnia.reload.country_code
   end
 
   test 'can update staff via seed data' do
@@ -64,7 +64,7 @@ class NodeTest < ActiveSupport::TestCase
 
   test 'can load countries data' do
     countries_hash = {}
-    assert_difference('countries_hash.keys.size', 250) do
+    assert_difference('countries_hash.keys.size', 251) do
       countries_hash = JSON.parse(File.read(File.join(Rails.root, 'config', 'data', 'countries.json')))
     end
   end
@@ -109,6 +109,18 @@ class NodeTest < ActiveSupport::TestCase
     assert_equal [m1, m2], node.related_materials.to_a.sort_by(&:title)
   end
 
+  test 'validates country code' do
+    node = nodes(:good)
+    assert node.valid?
+
+    node.country_code = 'XZ'
+    refute node.valid?
+    assert node.errors.added?(:country_code, :inclusion, value: 'XZ')
+
+    node.country_code = nil
+    assert node.valid?
+  end
+
   private
 
   def node_data_hash