Merge branch 'circular_subcollections' of github.com:innoq/iqvoc into…

… circular_subcollections

app/controllers/collections_controller.rb

@@ -61,10 +61,6 @@ def update
 
     if @collection.update_attributes(params[:concept])
       flash[:notice] = I18n.t("txt.controllers.collections.save.success")
-      if @collection.circular_errors.length > 0:
-        flash[:error] = I18n.t("txt.controllers.collections.circular_error") %
-            @collection.circular_errors.map { |c| c.label }.join("\n")
-      end
       redirect_to collection_path(@collection, :lang => I18n.locale)
     else
       flash.now[:error] = I18n.t("txt.controllers.collections.save.error")

app/controllers/search_results_controller.rb

@@ -1,6 +1,6 @@
 class SearchResultsController < ApplicationController
   skip_before_filter :require_user
-  
+
   def index
     authorize! :read, Concept::Base
 
@@ -22,17 +22,17 @@ def index
       return invalid_search(I18n.t('txt.controllers.search_results.insufficient_data')) if params[:query].blank? && params[:collection_origin].blank?
 
       params[:languages] << nil if params[:languages].is_a?(Array) && params[:languages].include?("none")
-      
+
       # Decide whether to search a specific class or ALL classes
       unless params[:type] == 'all'
         unless type_class_index = Iqvoc.searchable_class_names.map(&:parameterize).index(params[:type].parameterize)
           raise "'#{params[:type]}' is not a valid / configured searchable class! Must be one of " + Iqvoc.searchable_class_names.join(', ')
         end
         @klass = Iqvoc.searchable_class_names[type_class_index].constantize
       end
-      
+
       query_size = params[:query].split(/\r\n/).size
-      
+
       # @klass is only available if we're going to search using a specific class
       # it's not available if we're searching within all classes
       if @klass
@@ -46,9 +46,12 @@ def index
       else
         @multi_query = true
         # all names (including collection labels)
-        @results = Iqvoc.searchable_classes.select{ |klass| (klass < Labeling::Base) }.map{ |klass| klass.single_query(params) }.flatten
+        @results = Iqvoc.searchable_classes.
+            select { |klass| (klass < Labeling::Base) }.
+            map { |klass| klass.single_query(params) }.
+            flatten.uniq
       end
-      
+
       respond_to do |format|
         format.html
         format.ttl { @multi_query ? render('search_results/unpaged/index.iqrdf') : render('search_results/paged/index.iqrdf') }
@@ -57,9 +60,9 @@ def index
 
     end
   end
-  
+
   protected
-  
+
   def invalid_search(msg=nil)
     flash.now[:error] = msg || I18n.t('txt.controllers.search_results.query_invalid')
     render :action => 'index', :status => 422

app/models/collection/base.rb

@@ -31,12 +31,6 @@ class Collection::Base < Concept::Base
   #  :allow_destroy => true,
   #  :reject_if => Proc.new { |attrs| attrs[:value].blank? }
 
-  attr_writer :circular_errors
-  def circular_errors
-    @circular_errors = [] if not @circular_errors # XXX: hack because initialize didn't seem to work
-    @circular_errors
-  end
-
   after_save :regenerate_concept_members, :regenerate_collection_members
 
   before_validation(:on => :create) do
@@ -53,6 +47,7 @@ def circular_errors
 
   validates_uniqueness_of :origin
   validates :origin, :presence => true, :length => { :minimum => 2 }
+  validate :circular_subcollections
 
   def self.note_class_names
     ['Note::SKOS::Definition']
@@ -110,11 +105,7 @@ def regenerate_collection_members
     existing_collection_origins = collection_members.map{ |m| m.collection.origin }.uniq
     (@member_collection_origins - existing_collection_origins).each do |new_origin|
       Iqvoc::Collection.base_class.where(:origin => new_origin).each do |c|
-        if not c.subcollections.all.include?(self)
-          collection_members.create!(:target_id => c.id)
-        else
-          self.circular_errors.push c
-        end
+        collection_members.create!(:target_id => c.id)
       end
     end
     collection_members.
@@ -132,4 +123,13 @@ def label
   #   notes.select{ |note| note.class.name == note_class }
   # end
 
+  def circular_subcollections
+    Iqvoc::Collection.base_class.by_origin(@member_collection_origins).each do |subcollection|
+      if subcollection.subcollections.all.include?(self)
+        errors.add(:base,
+            I18n.t("txt.controllers.collections.circular_error") % subcollection.label)
+      end
+    end
+  end
+
 end

app/views/collections/edit.html.erb

@@ -1,3 +1,5 @@
+<%= error_messages_for @collection %>
+
 <h3><%= Collection::Base.model_name.human %></h3>
 
 <%= form_for @collection, :as => 'concept',

app/views/search_results/index.html.erb

@@ -68,7 +68,7 @@
     <h3><%= t('txt.views.search_results.search_results') %></h3>
     <p><%= t('txt.views.search_results.no_results_found') %></p>
     <% end %>
-    
+
     <dl id="search_results">
     <% @results.each do |result| %>
       <% if @klass %>

test/factories.rb

@@ -5,6 +5,10 @@
   c.narrower_relations{ |narrower_relations| [narrower_relations.association(:narrower_relation)]}
 end
 
+Factory.define :collection, :class => Iqvoc::Collection.base_class do |c|
+  c.sequence(:origin) { |n| "_100000#{n}" }
+end
+
 Factory.define :stupid_broader_relation, :class => Iqvoc::Concept.broader_relation_class do |rel|
 end
 
@@ -21,6 +25,10 @@
   lab.target { |target| target.association(:pref_label) }
 end
 
+Factory.define :alt_labeling, :class => Iqvoc::Concept.further_labeling_classes.first.first do |lab| # XXX: .first.first hacky!?
+  # TODO: should define target (YAGNI?)
+end
+
 Factory.sequence(:label_number) { |n| n + 1 }
 
 Factory.define :pref_label, :class => Iqvoc::Concept.pref_labeling_class.label_class do |l|

test/integration/collection_circularity_test.rb

@@ -1,42 +1,37 @@
-require 'test_helper'
+require 'integration_test_helper'
 
 class CollectionCircularityTest < ActionDispatch::IntegrationTest
-  @@klass = Iqvoc::Collection.base_class
-
-  setup do
-    # create a user
-    password = "FooBar"
-    user = User.new(:forename => "John", :surname => "Doe",
-        :email => "foo@example.org",
-        :password => password, :password_confirmation => password,
-        :active => true, :role => "administrator")
-    user.save
-
-    auth = Base64::encode64("%s:%s" % [user.email, user.password])
-    @env = { "HTTP_AUTHORIZATION" => "Basic " + auth }
-  end
 
   test "circular sub-collection references are rejected during update" do
-    coll1 = @@klass.new
-    coll1.save
-    coll2 = @@klass.new
-    coll2.save
+    coll1 = Factory.create(:collection)
+    coll2 = Factory.create(:collection)
+
+    login("administrator")
 
     # add coll2 as subcollection of coll1
-    uri = collection_path(:id => coll1.origin, :lang => "de", :format => "html")
-    params = { "concept[inline_member_collection_origins]" => "%s," % coll2.origin }
-    put_via_redirect uri, params, @env
+    visit edit_collection_path(coll1, :lang => "de", :format => "html")
+    fill_in "concept_inline_member_collection_origins",
+        :with => "%s," % coll2.origin
+    click_button "Speichern"
 
-    assert_response :success
-    assert_equal 1, @@klass.by_origin(coll1.origin).first.subcollections.count
+    assert page.has_no_css?(".flash_error")
+    assert page.has_content?(I18n.t("txt.controllers.collections.save.success"))
+    assert page.has_link?(coll2.label.to_s,
+        :href => collection_path(coll2, :lang => "de", :format => "html"))
 
     # add coll1 as subcollection of coll2
-    uri = collection_path(:id => coll2.origin, :lang => "de", :format => "html")
-    params = { "concept[inline_member_collection_origins]" => coll1.origin }
-    put_via_redirect uri, params, @env
-
-    assert_response :success
-    assert_equal 0, @@klass.by_origin(coll2.origin).first.subcollections.count
-    assert_equal flash[:error], I18n.t("txt.controllers.collections.circular_error") % coll1.label
+    visit edit_collection_path(coll2, :lang => "de", :format => "html")
+    fill_in "concept_inline_member_collection_origins",
+        :with => "%s," % coll1.origin
+    click_button "Speichern"
+
+    assert page.has_css?(".flash_error")
+    assert page.has_css?("#concept_edit")
+    assert page.source.include?( # XXX: page.has_content? didn't work
+        I18n.t("txt.controllers.collections.circular_error") % coll1.label)
+
+    # ensure coll1 is not a subcollection of coll2
+    visit collection_path(coll2, :lang => "de", :format => "html")
+    assert page.has_no_link?(coll1.label.to_s)
   end
 end

test/integration/search_results_test.rb

@@ -0,0 +1,90 @@
+require 'test_helper'
+require 'integration_test_helper'
+
+
+class SearchTest < ActionDispatch::IntegrationTest
+
+  setup do
+    # create a few XL labels
+    labels = {}
+    {
+      "terra" => "la",
+      "earth" => "en",
+      "Erde" => "de",
+      "sol" => "la",
+      "sun" => "en",
+      "Sonne" => "de",
+      "inhabited" => "en",
+      "bewohnt" => "de",
+      "uninhabited" => "en",
+      "unbewohnt" => "de"
+    }.map { |name, lang|
+      label = Factory.create(:xllabel, :origin => "_%s" % name,
+          :language => lang, :value => name, :published_at => Time.now)
+      labels[name] = label
+    }
+
+    # create a few concepts
+    [
+      ["Erde", "earth"],
+      ["Sonne", "sun"]
+    ].each { |pref, alt|
+        concept = Factory.create(:concept, :origin => "_%s" % pref,
+            :labelings => [], :narrower_relations => [], # avoid creating additional concepts/label[ing]s
+            :published_at => Time.now)
+        Factory.create(:pref_labeling, :owner => concept, :target => labels[pref])
+        Factory.create(:alt_labeling, :owner => concept, :target => labels[alt])
+    }
+
+    # create a few collections
+    [
+      ["bewohnt", "inhabited"],
+      ["unbewohnt", "uninhabited"]
+    ].each { |pref, alt|
+        collection = Factory.create(:collection)
+        Factory.create(:pref_labeling, :owner => collection, :target => labels[pref])
+        Factory.create(:alt_labeling, :owner => collection, :target => labels[alt])
+    }
+
+    # confirm environment, just to be safe (i.e. ensure that factories,
+    # for example, don't introduce unexpected side-effects)
+    assert_equal 10, Label::Base.all.count
+    assert_equal 8, Labeling::Base.all.count
+    assert_equal 2, Iqvoc::Concept.base_class.all.count
+    assert_equal 2, Iqvoc::Collection.base_class.all.count
+    assert_equal 4, Concept::Base.all.count
+  end
+
+  test "HTML search returns matches" do
+    uri = search_path(:lang => "de", :format => "html")
+    uri += "?l[]=de&l[]=en" # doesn't fit into params hash
+    params = {
+      "q" => "Erde",
+      "qt" => "exact", # match type
+      "t" => "all", # search type
+      "c" => "" # collection
+    }
+    params.each { |key, value|
+      uri += "&%s=%s" % [key, value] # XXX: hacky and brittle (e.g. lack of URL-encoding)
+    }
+
+    visit uri
+
+    assert page.has_css?("#search_results dt", :count => 1)
+  end
+
+  test "RDF search returns matches" do
+    uri = search_path(:lang => "de", :format => "rdf")
+    uri += "?l[]=de&l[]=en" # doesn't fit into params hash
+    params = {
+      "q" => "Erde",
+      "qt" => "exact", # match type
+      "t" => "all", # search type
+      "c" => "" # collection
+    }
+    get uri, params
+
+    assert_match /<sdc:result rdf:resource.*#result1"\/>/, @response.body
+    assert_no_match /#result2"\/>/, @response.body
+  end
+end

test/unit/collection_label_test.rb

@@ -2,12 +2,8 @@
 
 class CollectionLabelTest < ActiveSupport::TestCase
 
-  setup do
-    @klass = Iqvoc::Collection.base_class
-  end
-
   test "can have multiple labels" do
-    my_coll = @klass.new
+    my_coll = Factory.build(:collection)
     # add a few label[ing]s
     origin = 0
     Iqvoc::Concept.labeling_classes.each { |lnclass, langs|
@@ -28,7 +24,7 @@ class CollectionLabelTest < ActiveSupport::TestCase
   end
 
   test "must have a preferred label" do
-    my_coll = @klass.new
+    my_coll = Factory.build(:collection)
 
     # add an alternative label
     lnclass = Labeling::SKOSXL::AltLabel
@@ -55,7 +51,7 @@ class CollectionLabelTest < ActiveSupport::TestCase
   end
 
   test "does not need more than one label" do
-    my_coll = @klass.new
+    my_coll = Factory.build(:collection)
 
     # add a preferred label
     lnclass = Labeling::SKOSXL::PrefLabel
@@ -71,7 +67,7 @@ class CollectionLabelTest < ActiveSupport::TestCase
   end
 
   test "can have multiple preferred labels" do
-    my_coll = @klass.new
+    my_coll = Factory.build(:collection)
 
     # add multiple preferred labels
     lnclass = Labeling::SKOSXL::PrefLabel