Conversation
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
…y_name Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
… compatibility Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Co-authored-by: fbacall <503373+fbacall@users.noreply.github.com>
Remove the data migration for now, needs to be safer/better. * This saves having to centrally manage People and provide ways of correcting metadata etc.
Too complex trying to manage both split names and full names.
There was a problem hiding this comment.
Pull request overview
This PR migrates authors and contributors from simple string arrays to a new Person model with name and orcid fields. It introduces a reusable HasPeople concern with a has_person_role macro, updates Bioschemas output to include ORCID identifiers, adds a custom display_people helper with profile/ORCID links, and implements PersonLinkWorker to automatically link Person records to TeSS profiles by ORCID.
Changes:
- New
Personmodel andHasPeople/HasOrcidconcerns, plus database migrations migrating existing string data - Updated controllers, serializers, views, and Bioschemas generator to use the new structured
Personmodel - Comprehensive new test coverage for the
Personmodel, worker, and updated ingestor/controller tests
Reviewed changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/person.rb |
New Person model with ORCID support, autocomplete query, and profile auto-linking |
app/models/concerns/has_people.rb |
New concern with has_person_role macro and flexible setter accepting strings/hashes/Person objects |
app/models/concerns/has_orcid.rb |
New concern extracting ORCID normalization shared between Profile and Person |
app/models/profile.rb |
Includes HasOrcid concern; triggers PersonLinkWorker after ORCID authentication |
app/models/material.rb / workflow.rb / learning_path.rb |
Include HasPeople, use has_person_role for authors/contributors |
app/workers/person_link_worker.rb |
Background worker that re-saves Person records by ORCID to update profile links |
lib/bioschemas/generator.rb |
Adds @id/identifier (ORCID URL) to Person schema output |
app/helpers/materials_helper.rb |
New display_people helper rendering profile/ORCID links |
app/controllers/*_controller.rb |
Updated *_params to permit structured authors/contributors and people_attributes |
app/serializers/*_serializer.rb |
Serializes authors/contributors as display name arrays via people helper |
db/migrate/202602*.rb – 202603*.rb |
Migrations to create people table, migrate legacy data, rename columns |
db/schema.rb |
Reflects new people table and renamed deprecated_* columns |
test/models/person_test.rb |
New comprehensive tests for Person model |
test/workers/person_link_worker_test.rb |
New tests for PersonLinkWorker |
test/models/profile_test.rb |
Updated to assert PersonLinkWorker job is enqueued on ORCID change |
test/controllers/materials_controller_test.rb / workflows_controller_test.rb / learning_paths_controller_test.rb |
New tests for structured/legacy author update endpoints |
test/unit/ingestors/*_test.rb |
Updated assertions to use display_name/ORCID on Person objects |
test/fixtures/people.yml |
New fixture file for Person records |
app/views/common/_people_form.html.erb / _person_form.html.erb |
New people sub-form partials for the UI |
app/assets/javascripts/people.js |
New JS for dynamic people form (autocomplete, add/remove) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Fixes matching on blank ORCID etc.
(even though the migration up/down is fine without)
… author-improvements
|
Check. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| <label class="ml-2 delete-person-btn delete-nested-resource-btn"> | ||
| <i class="icon icon-sm cross-icon icon-greyscale"></i> | ||
| <%= hidden_field_tag "#{field_name_prefix}[_destroy]", '0', autocomplete: 'off' %> | ||
| <%= check_box_tag "#{field_name_prefix}[_destroy]", '1', false, class: 'destroy-attribute', autocomplete: 'off' %> | ||
| </label> |
There was a problem hiding this comment.
The delete button in the person form (_person_form.html.erb) relies on Rails' accepts_nested_attributes_for behavior (via a _destroy field) to remove a person, but the model uses a custom setter (set_people_for_role) rather than accepts_nested_attributes_for. The _destroy param is not included in the strong parameters (authors: [:name, :orcid]) and is not handled by set_people_for_role. When a user clicks the delete button, the name and ORCID fields still have their values and are still submitted (the JS only visually fades the form), so set_people_for_role will match by name/orcid and retain the person rather than removing them. The _destroy mechanism either needs to be added to the strong parameters and handled by set_people_for_role, or the JS should clear the name/orcid fields when the delete checkbox is checked so the blank entry is filtered out by reject(&:blank?).
app/models/concerns/has_people.rb
Outdated
| current_people = send(role_name).to_a | ||
| to_keep = [] | ||
|
|
||
| value = value.values if value.respond_to?(:keys) && value.keys.first.match(/\A\d+\z/) |
There was a problem hiding this comment.
On line 29, if value is an empty hash ({}), then value.keys.first returns nil, and calling nil.match(/\A\d+\z/) raises a NoMethodError. This could occur if an empty form submission sends authors={}. Consider adding a guard: check value.keys.first&.match(...) or value.any? && value.keys.first.match(...).
| # Authors is now an array of objects with id, name, orcid | ||
| response_authors = JSON.parse(response.body)['authors'] | ||
| assert_equal test_material.authors.size, response_authors.size, 'authors count not matched' | ||
| response_authors.each_with_index do |author_json, i| | ||
| expected_author = test_material.authors[i] | ||
| assert_equal expected_author.display_name, author_json, "author #{i} name not matched" | ||
| end | ||
| # Contributors is now an array of objects with id, name, orcid |
There was a problem hiding this comment.
The comment on line 294 says "Authors is now an array of objects with id, name, orcid" but the actual JSON response (as confirmed by app/views/materials/show.json.jbuilder which uses @material.authors.map(&:display_name)) is an array of strings, not objects. The same misleading comment appears on line 301 for contributors. The comments should say "Authors is now an array of display name strings".
| @@ -0,0 +1,25 @@ | |||
| <% index ||= 'replace-me' %> <%# This is so we can render a blank version of this sub-form in the page, %> | |||
| <%# which can be dynamically cloned using JavaScript to add more People to the main form %> | |||
| <% field_name_prefix = "#{field_prefix}[#{index}]" %> <%# This format is dictated by "accepts_nested_attributes_for" %> | |||
There was a problem hiding this comment.
The comment on line 3 states that the field name format "is dictated by accepts_nested_attributes_for", but the model does not use accepts_nested_attributes_for for people — it uses a custom setter (set_people_for_role in the HasPeople concern). The comment should be updated to accurately describe the custom setter mechanism.
| def self.query(query, limit = nil) | ||
| q = select(:name, :orcid, :profile_id).starting_with(query).distinct | ||
| q = q.limit(limit) if limit | ||
| q.order(name: :asc, orcid: :asc) | ||
| end |
There was a problem hiding this comment.
The Person.query method uses lower(name) LIKE 'query%' for autocomplete but there is no index on the name column (or lower(name)) in the people table. As the people table grows (one row per person-resource association), this autocomplete query will do a full table scan, which can become slow. Consider adding a functional index on lower(name) or at minimum a regular index on name in the migration.
| def display_people(resource, attribute) | ||
| display_attribute(resource, attribute) do |values| | ||
| html = values.map do |person| | ||
| if person.profile | ||
| link_to(person.profile.full_name, person.profile) | ||
| elsif person.orcid.present? | ||
| image_tag('ORCID-iD_icon_vector.svg', size: 16) + ' ' + | ||
| external_link(person.display_name, person.orcid_url) | ||
| else | ||
| person.display_name | ||
| end | ||
| end | ||
| safe_join(html, ', ') | ||
| end |
There was a problem hiding this comment.
The display_people helper iterates over each person and calls person.profile, but the has_person_role association doesn't eager-load profiles. This causes N+1 database queries when a resource has multiple authors or contributors. The has_person_role macro should add includes: :profile to the association scope, or the display_people helper should receive a collection that is already preloaded with profiles.
| belongs_to :profile, optional: true | ||
| belongs_to :resource, polymorphic: true | ||
|
|
||
| validates :resource, :role, :name, presence: true |
There was a problem hiding this comment.
The Person model does not validate the format of the orcid field (unlike Profile which has validates :orcid, orcid: true, allow_blank: true). Without this validation, malformed ORCID strings can be stored, which will produce invalid Bioschemas @id and identifier values in the JSON-LD output via orcid_url. Adding validates :orcid, orcid: true, allow_blank: true (matching the Profile model) would prevent this.
…hors/contributors can be removed
Summary of changes
authorsandcontributorsforMaterial,WorkflowandLearningPathfrom string arrays to ahas_manyrelationship with a new genericPersonmodel containingfull_name, andorcidfieldsHasPeopleconcern with ahas_person_rolemacro for DRY code that can be applied to any model and rolePersonobjects or strings)Still to do
WorkflowandLearningPathcontributorsandauthorsPersonobjects being needlessly destroyed/recreated when a scraper "updates" a resource.Motivation and context
The string arrays were limiting metadata quality and prevented linking to external author identifiers. Directly linking to authors/contributors to their ORCID/TeSS Profile opens up opportunities for better credit/recognition for people working in the training ecosystem. Additionally makes it easier to link other "roles" to resources in the future, e.g. instructors for events, maintainers for learning paths etc.
Screenshots
Checklist
to license it to the TeSS codebase under the
BSD license.