Skip to content

Conversation

@whomingbird
Copy link
Contributor

@whomingbird whomingbird commented Nov 17, 2025

@stuzart stuzart requested a review from Copilot November 18, 2025 10:43
@stuzart stuzart added this to the 1.17.2 milestone Nov 18, 2025
@stuzart stuzart moved this to In review in SEEK 1.17.x Nov 18, 2025
Copilot finished reviewing on behalf of stuzart November 18, 2025 10:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extends the publication type list by migrating from legacy SEEK publication types to DataCite-compliant types, adding support for a broader range of publication categories. The changes align the system with DataCite Metadata Schema v4.6 and provide better semantic clarity for publication classifications.

Key changes:

  • Migration of legacy publication types (e.g., "Journal" → "Journal Article", "InProceedings" → "Conference Paper", "Phd Thesis" → "Doctoral Thesis")
  • Addition of 25+ new DataCite publication types including Dataset, Software, Workflow, and Computational Notebook
  • Refactored publication type model to use YAML-driven configuration and dynamic method generation

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
app/models/publication_type.rb Refactored to use YAML-based type registry, BibTeX-to-DataCite mapping, and dynamic method generation for type accessors
app/models/publication.rb Updated validation and citation logic to use new publication type keys instead of deprecated is_*? methods; standardized quote style
db/seeds/005_publication_types.seeds.rb Implements migration from legacy types to DataCite types with YAML-based configuration
config/default_data/publication_types.yml New YAML configuration defining all supported publication types with DataCite mappings
app/views/publications/new.html.erb Updated default selection from PublicationType.Journal to PublicationType.JournalArticle
test/fixtures/publication_types.yml Updated test fixtures to match new publication type names and keys
test/fixtures/files/bibtex/sample_publications.bib Added BibTeX sample file for testing publication type mapping
test/functional/publications_controller_test.rb Updated test assertions for new type names and added comprehensive import test
test/factories/publications.rb Updated factory definitions to use new publication type titles and keys
test/fixtures/json/responses/get_min_publication.json.erb Updated JSON fixture to reference "Journal Article" instead of "Journal"
test/fixtures/json/responses/get_max_publication.json.erb Updated JSON fixture to reference "Journal Article" instead of "Journal"
lib/tasks/seek_upgrades.rake Added publication types seed task to upgrade sequence

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@whomingbird whomingbird requested a review from stuzart November 18, 2025 11:18
@whomingbird whomingbird self-assigned this Nov 18, 2025
Comment on lines +347 to +348
case publication_type.key
when 'journalarticle'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these keys shouldn't hard-coded and used outside of PubliciationType . either use the accessor methods (like is_journal ), or move this logic inside PublicationType


if (%w[InCollection InProceedings].include? self.publication_type.title) && (bibtex_record[:booktitle].blank?)
errors.add(:base, "An #{self.publication_type.title} needs to have a booktitle.")
if (['Collection', 'Conference Paper'].include? self.publication_type.title) && (bibtex_record[:booktitle].blank?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't rely on the title, incase they change in future migrations

# Load expected publication types from YAML
def self.type_registry
@type_registry ||= begin
path = Rails.root.join('config/default_data/publication_types.yml')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use the yaml file, which is used for seeding. Fetch this info from the database where we know it is up to date

Comment on lines -87 to -93
def is_journal?
key == "article"
end

def is_book?
key == 'book'
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these accessor methods (and those below) are still needed, to avoid relying on hard-coded keys outside of PublicationType

Comment on lines +48 to +49
title: Diplom Thesis
key: diplomthesis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be Diploma thesis

key: diplomthesis

PhdThesis:
title: Doctoral Thesis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhD thesis is more widely used in UK and Europe

'Manual' => 'Text',
'Misc' => 'Other',
'Tech report' => 'Report',
'Unpublished' => 'Preprint'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the misspelling on Diploma will also need adding here


begin
type_registry.each_key do |title|
define_singleton_method(title.gsub(/\s+/, '')) do
Copy link
Member

@stuzart stuzart Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these methods, e.g PublicationType.Journal or PublicationType.Collection to get a specific type, ever actually used anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants