Add HasPeople concern, apply to contributors like authors

Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>

Author: Copilot

app/models/concerns/has_people.rb

@@ -0,0 +1,60 @@
+module HasPeople
+  extend ActiveSupport::Concern
+
+  included do
+    has_many :person_links, as: :resource, dependent: :destroy
+    has_many :people, through: :person_links
+    accepts_nested_attributes_for :person_links, allow_destroy: true, reject_if: :all_blank
+  end
+
+  class_methods do
+    # Define a person role association (e.g., :authors, :contributors)
+    # This creates the association and a custom setter that accepts strings, hashes, or Person objects
+    def has_person_role(role_name, role_key: role_name.to_s.singularize)
+      # Define the association
+      has_many role_name, -> { where(person_links: { role: role_key }) }, through: :person_links, source: :person
+
+      # Define custom setter that accepts strings (legacy), hashes, or Person objects
+      define_method("#{role_name}=") do |value|
+        set_people_for_role(value, role_key)
+      end
+    end
+  end
+
+  private
+
+  # Set people for a specific role, accepting various input formats
+  def set_people_for_role(value, role)
+    return if value.nil?
+
+    # Convert to array if needed
+    people_array = Array(value).reject(&:blank?)
+
+    # Remove existing links for this role
+    person_links.where(role: role).destroy_all
+
+    people_array.each do |person_data|
+      if person_data.is_a?(String)
+        # Legacy format: parse string into first_name and last_name
+        parts = person_data.strip.split(/\s+/, 2)
+        first_name = parts.length > 1 ? parts[0] : ''
+        last_name = parts.length > 1 ? parts[1] : parts[0]
+
+        person = Person.find_or_create_by!(first_name: first_name, last_name: last_name)
+        person_links.build(person: person, role: role)
+      elsif person_data.is_a?(Hash)
+        # Hash format from API
+        first_name = person_data[:first_name] || person_data['first_name'] || ''
+        last_name = person_data[:last_name] || person_data['last_name'] || ''
+        orcid = person_data[:orcid] || person_data['orcid']
+
+        person = Person.find_or_create_by!(first_name: first_name, last_name: last_name)
+        person.update!(orcid: orcid) if orcid.present?
+        person_links.build(person: person, role: role)
+      elsif person_data.is_a?(Person)
+        # Person object
+        person_links.build(person: person_data, role: role)
+      end
+    end
+  end
+end

app/models/material.rb

@@ -21,6 +21,7 @@ class Material < ApplicationRecord
   include HasDifficultyLevel
   include HasEdamTerms
   include InSpace
+  include HasPeople
 
   if TeSS::Config.solr_enabled
     # :nocov:
@@ -33,7 +34,9 @@ class Material < ApplicationRecord
       text :authors do
         authors.map(&:full_name)
       end
-      text :contributors
+      text :contributors do
+        contributors.map(&:full_name)
+      end
       text :target_audience
       text :keywords
       text :resource_type
@@ -66,7 +69,9 @@ class Material < ApplicationRecord
       string :keywords, multiple: true
       string :fields, multiple: true
       string :resource_type, multiple: true
-      string :contributors, multiple: true
+      string :contributors, multiple: true do
+        contributors.map(&:full_name)
+      end
       string :content_provider do
         content_provider.try(:title)
       end
@@ -106,45 +111,9 @@ class Material < ApplicationRecord
 
   has_many :stars, as: :resource, dependent: :destroy
 
-  has_many :person_links, as: :resource, dependent: :destroy
-  has_many :people, through: :person_links
-  has_many :authors, -> { where(person_links: { role: 'author' }) }, through: :person_links, source: :person
-  accepts_nested_attributes_for :person_links, allow_destroy: true, reject_if: :all_blank
-
-  # Custom setter for authors that accepts both strings (legacy API) and Person objects
-  def authors=(value)
-    return if value.nil?
-    
-    # Convert to array if needed
-    authors_array = Array(value).reject(&:blank?)
-    
-    # Remove existing author links
-    person_links.where(role: 'author').destroy_all
-    
-    authors_array.each do |author|
-      if author.is_a?(String)
-        # Legacy format: parse string into first_name and last_name
-        parts = author.strip.split(/\s+/, 2)
-        first_name = parts.length > 1 ? parts[0] : ''
-        last_name = parts.length > 1 ? parts[1] : parts[0]
-        
-        person = Person.find_or_create_by!(first_name: first_name, last_name: last_name)
-        person_links.build(person: person, role: 'author')
-      elsif author.is_a?(Hash)
-        # Hash format from API
-        first_name = author[:first_name] || author['first_name'] || ''
-        last_name = author[:last_name] || author['last_name'] || ''
-        orcid = author[:orcid] || author['orcid']
-        
-        person = Person.find_or_create_by!(first_name: first_name, last_name: last_name)
-        person.update!(orcid: orcid) if orcid.present?
-        person_links.build(person: person, role: 'author')
-      elsif author.is_a?(Person)
-        # Person object
-        person_links.build(person: author, role: 'author')
-      end
-    end
-  end
+  # Use HasPeople concern for authors and contributors
+  has_person_role :authors, role_key: 'author'
+  has_person_role :contributors, role_key: 'contributor'
 
   # Remove trailing and squeezes (:squish option) white spaces inside the string (before_validation):
   # e.g. "James     Bond  " => "James Bond"
@@ -155,10 +124,10 @@ def authors=(value)
   validates :other_types, presence: true, if: proc { |m| m.resource_type.include?('other') }
   validates :keywords, length: { maximum: 20 }
 
-  clean_array_fields(:keywords, :fields, :contributors,
+  clean_array_fields(:keywords, :fields,
                      :target_audience, :resource_type, :subsets)
 
-  update_suggestions(:keywords, :contributors, :target_audience,
+  update_suggestions(:keywords, :target_audience,
                      :resource_type)
 
   def description=(desc)
@@ -257,7 +226,7 @@ def to_oai_dc
       xml.tag!('dc:title', title)
       xml.tag!('dc:description', description)
       authors.each { |a| xml.tag!('dc:creator', a.full_name) }
-      contributors.each { |a| xml.tag!('dc:contributor', a) }
+      contributors.each { |c| xml.tag!('dc:contributor', c.full_name) }
       xml.tag!('dc:publisher', content_provider.title) if content_provider
 
       xml.tag!('dc:format', 'text/html')

app/serializers/material_serializer.rb

@@ -5,7 +5,7 @@ class MaterialSerializer < ApplicationSerializer
 
              :doi, :licence, :version, :status,
 
-             :contact, :contributors,
+             :contact,
 
              :difficulty_level, :target_audience, :prerequisites, :syllabus, :learning_objectives, :subsets,
 
@@ -19,4 +19,5 @@ class MaterialSerializer < ApplicationSerializer
   has_many :collections
   has_many :events
   has_many :authors, serializer: PersonSerializer
+  has_many :contributors, serializer: PersonSerializer
 end

app/views/common/_extra_metadata.html.erb

@@ -60,7 +60,7 @@
 <% end %>
 
 <%= display_attribute(resource, :authors) { |values| values.map(&:full_name).join(', ') } if resource.respond_to?(:authors) %>
-<%= display_attribute(resource, :contributors) { |values| values.join(', ') } if resource.respond_to?(:contributors) %>
+<%= display_attribute(resource, :contributors) { |values| values.map(&:full_name).join(', ') } if resource.respond_to?(:contributors) %>
 <%= display_attribute(resource, :remote_created_date) if resource.respond_to?(:remote_created_date) %>
 <%= display_attribute(resource, :remote_updated_date) if resource.respond_to?(:remote_updated_date) %>
 <%= display_attribute(resource, :scientific_topics) { |values| values.map { |x| x.preferred_label }.join(', ') } %>

app/views/materials/_form.html.erb

@@ -86,8 +86,22 @@
   </div>
 
   <!-- Field: Contributors -->
-  <%= f.multi_input :contributors, suggestions_url: people_autocomplete_suggestions_path, title: t('materials.hints.contributors'),
-                    visibility_toggle: TeSS::Config.feature['materials_disabled'] %>
+  <div class="form-group" id="person-contributor">
+    <%= f.label :contributors %>
+    <%= f.field_lock :contributors %>
+
+    <div id="person-contributor-list">
+      <% @material.person_links.where(role: 'contributor').each_with_index do |person_link, index| %>
+        <%= render partial: 'common/person_form',
+                   locals: { form_name: 'material', index: "contributor_#{index}", person_link: person_link, role: 'contributor' } %>
+      <% end %>
+    </div>
+
+    <a href="#" id="add-person-contributor" data-role="contributor" class="btn btn-icon">
+      <i class="icon icon-h4 plus-icon"></i>
+    </a>
+    <span class="help-inline-block help-block">Add contributor information</span>
+  </div>
 
   <!-- Field: Target Audience -->
   <%= f.multi_input :target_audience, label: 'Target audiences', errors: @material.errors[:target_audience],
@@ -214,6 +228,11 @@
              locals: { form_name: 'material', person_link: nil, role: 'author' } %>
 </div>
 
+<div id="person-contributor-template" style="display: none">
+  <%= render partial: 'common/person_form',
+             locals: { form_name: 'material', person_link: nil, role: 'contributor' } %>
+</div>
+
 <script type="text/javascript">
     function removeSuggestion(suggestion) {
         s = suggestion.replace(/[^a-zA-Z]/g, '');

app/views/materials/show.json.jbuilder

@@ -6,7 +6,7 @@ json.extract! @material, :id, :title, :url, :description,
 
               :doi, :licence, :version, :status,
 
-              :contact, :contributors,
+              :contact,
 
               :difficulty_level, :target_audience, :prerequisites, :syllabus, :learning_objectives, :subsets,
 
@@ -22,6 +22,7 @@ json.collections @material.collections.collect { |x| { title: x[:title], id: x[:
 json.events @material.events.collect { |x| { title: x[:title], id: x[:id] } }
 
 json.authors @material.authors.collect { |a| { id: a.id, first_name: a.first_name, last_name: a.last_name, orcid: a.orcid, full_name: a.full_name } }
+json.contributors @material.contributors.collect { |c| { id: c.id, first_name: c.first_name, last_name: c.last_name, orcid: c.orcid, full_name: c.full_name } }
 
 json.external_resources do
   @material.external_resources.each do |external_resource|

db/migrate/20251119095246_migrate_material_authors_data.rb

@@ -2,52 +2,70 @@ class MigrateMaterialAuthorsData < ActiveRecord::Migration[7.2]
   def up
     # Migrate existing authors from array to Person model
     Material.find_each do |material|
-      next if material.authors.blank?
-
-      material.authors.each do |author_name|
-        next if author_name.blank?
-
-        # Parse the name - assume "First Last" format
-        # Handle edge cases: single names, multiple spaces, etc.
-        parts = author_name.strip.split(/\s+/, 2)
-        
-        if parts.length == 1
-          # Single name - use as last name with empty first name
-          first_name = ''
-          last_name = parts[0]
-        else
-          first_name = parts[0]
-          last_name = parts[1] || ''
-        end
-
-        # Find or create person
-        person = Person.find_or_create_by!(
-          first_name: first_name,
-          last_name: last_name
-        )
-
-        # Create the association if it doesn't exist
-        PersonLink.find_or_create_by!(
-          resource: material,
-          person_id: person.id,
-          role: 'author'
-        )
-      end
+      # Migrate authors
+      migrate_people_role(material, 'authors', 'author') if material.respond_to?(:read_attribute) && material.read_attribute(:authors).present?
+
+      # Migrate contributors
+      migrate_people_role(material, 'contributors', 'contributor') if material.respond_to?(:read_attribute) && material.read_attribute(:contributors).present?
     end
   end
 
   def down
-    # Restore authors array from Person model
+    # Restore arrays from Person model
     Material.find_each do |material|
+      # Restore authors
       author_names = material.people.where(person_links: { role: 'author' }).map(&:full_name).compact
-      # Use raw SQL to avoid validation issues
       Material.connection.execute(
         "UPDATE materials SET authors = ARRAY[#{author_names.map { |n| "'#{n.gsub("'", "''")}'" }.join(',')}]::varchar[] WHERE id = #{material.id}"
       )
+
+      # Restore contributors
+      contributor_names = material.people.where(person_links: { role: 'contributor' }).map(&:full_name).compact
+      Material.connection.execute(
+        "UPDATE materials SET contributors = ARRAY[#{contributor_names.map { |n| "'#{n.gsub("'", "''")}'" }.join(',')}]::varchar[] WHERE id = #{material.id}"
+      )
     end
 
     # Clean up the new tables
     PersonLink.delete_all
     Person.delete_all
   end
+
+  private
+
+  def migrate_people_role(material, column_name, role)
+    # Get the raw array from database
+    people_array = material.read_attribute(column_name)
+    return if people_array.blank?
+
+    people_array.each do |person_name|
+      next if person_name.blank?
+
+      # Parse the name - assume "First Last" format
+      # Handle edge cases: single names, multiple spaces, etc.
+      parts = person_name.strip.split(/\s+/, 2)
+
+      if parts.length == 1
+        # Single name - use as last name with empty first name
+        first_name = ''
+        last_name = parts[0]
+      else
+        first_name = parts[0]
+        last_name = parts[1] || ''
+      end
+
+      # Find or create person
+      person = Person.find_or_create_by!(
+        first_name: first_name,
+        last_name: last_name
+      )
+
+      # Create the association if it doesn't exist
+      PersonLink.find_or_create_by!(
+        resource: material,
+        person_id: person.id,
+        role: role
+      )
+    end
+  end
 end

db/migrate/20251119095247_remove_authors_from_materials.rb

@@ -1,5 +1,6 @@
 class RemoveAuthorsFromMaterials < ActiveRecord::Migration[7.2]
   def change
     remove_column :materials, :authors, :string, array: true, default: []
+    remove_column :materials, :contributors, :string, array: true, default: []
   end
 end

lib/bioschemas/learning_resource_generator.rb

@@ -22,7 +22,13 @@ def self.bioschemas_profile
         author_hash
       } 
     }
-    property :contributor, -> (material) { material.contributors.map { |p| person(p) } }
+    property :contributor, -> (material) { 
+      material.contributors.map { |c| 
+        contributor_hash = { "@type" => "Person", "name" => c.full_name }
+        contributor_hash["@id"] = "https://orcid.org/#{c.orcid}" if c.orcid.present?
+        contributor_hash
+      } 
+    }
     property :provider, -> (material) { provider(material) }
     property :audience, -> (material) {
       material.target_audience.map { |audience| { '@type' => 'Audience', 'audienceType' => audience } }

test/controllers/materials_controller_test.rb

@@ -253,8 +253,7 @@ class MaterialsControllerTest < ActionController::TestCase
           date_published: test_material.date_published,
           doi: test_material.doi,
           subsets: test_material.subsets,
-          person_links_attributes: test_material.person_links.where(role: 'author').map { |pl| { role: 'author', person_attributes: { first_name: pl.person.first_name, last_name: pl.person.last_name, orcid: pl.person.orcid } } },
-          contributors: test_material.contributors,
+          person_links_attributes: test_material.person_links.map { |pl| { role: pl.role, person_attributes: { first_name: pl.person.first_name, last_name: pl.person.last_name, orcid: pl.person.orcid } } },
           prerequisites: test_material.prerequisites,
           syllabus: test_material.syllabus,
           learning_objectives: test_material.learning_objectives
@@ -300,7 +299,15 @@ class MaterialsControllerTest < ActionController::TestCase
       assert_equal expected_author.last_name, author_json['last_name'], "author #{i} last_name not matched"
       assert_equal expected_author.full_name, author_json['full_name'], "author #{i} full_name not matched"
     end
-    assert_equal test_material.contributors, JSON.parse(response.body)['contributors'], 'contributors not matched'
+    # Contributors is now an array of objects with id, first_name, last_name, orcid, full_name
+    response_contributors = JSON.parse(response.body)['contributors']
+    assert_equal test_material.contributors.size, response_contributors.size, 'contributors count not matched'
+    response_contributors.each_with_index do |contributor_json, i|
+      expected_contributor = test_material.contributors[i]
+      assert_equal expected_contributor.first_name, contributor_json['first_name'], "contributor #{i} first_name not matched"
+      assert_equal expected_contributor.last_name, contributor_json['last_name'], "contributor #{i} last_name not matched"
+      assert_equal expected_contributor.full_name, contributor_json['full_name'], "contributor #{i} full_name not matched"
+    end
     assert_equal test_material.subsets, JSON.parse(response.body)['subsets'], 'subsets not matched'
     assert_equal test_material.prerequisites, JSON.parse(response.body)['prerequisites'], 'prerequisites not matched'
     assert_equal test_material.syllabus, JSON.parse(response.body)['syllabus'], 'syllabus not matched'