Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add citations for SCSB items #4427

Merged
merged 6 commits into from
Oct 10, 2024
Merged

Add citations for SCSB items #4427

merged 6 commits into from
Oct 10, 2024

Conversation

maxkadel
Copy link
Contributor

@maxkadel maxkadel commented Oct 8, 2024

I think this PR delivers value, but there are some things I'd like help improving.

Connected to #4012

  • Right now, it is only tested for Books, I think we should add tests for other types of items, especially journals.
  • It requires a second call to Solr, which seems like it shouldn't be necessary, but I've spent a lot of time in the catalog controller trying to add the _citation_ fields so they are in the solr document at this point, but haven't succeeded yet. (See Reduce calls to Solr for CiteProc citations #4429 for follow-up ticket to resolve this)
  • Probably a lot of the methods in Blacklight::Document::Mla can be a) Private and b) moved into an abstract method for other CiteProc citations (APA, Chicago) - done
  • Right now we have to take apart existing Solr fields to get the publication information - see Store publication fields separately for CiteProc citations bibdata#2515 for follow-up ticket to add separate Solr fields so we do not have to process them here but can use them directly from Solr.
  • For some reason I can't get the author name to show up correctly with the comma between the family name and the given name. I've added a pending test for this issue. (I'm wondering if this is because of the combining accent in the example? - it does work correctly in development for SCSB-8953469) - I have created a fork that has a test, this seems to only affect names ending in decomposed accents. This is a bug in CiteProc - see https://github.com/pulibrary/citeproc/tree/names_with_final_accents
image

@maxkadel maxkadel marked this pull request as draft October 8, 2024 13:17
@maxkadel maxkadel force-pushed the i4012_add_citation_scsb branch from 14b5621 to ea75017 Compare October 8, 2024 13:26
@maxkadel maxkadel force-pushed the i4012_add_citation_scsb branch from 623adf3 to bbea05f Compare October 8, 2024 13:47
@coveralls
Copy link

coveralls commented Oct 8, 2024

Coverage Status

coverage: 96.812% (+0.3%) from 96.546%
when pulling df3938c on i4012_add_citation_scsb
into e3143b9 on main.


class Orangelight::Document::CitationComponent < Blacklight::Document::CitationComponent
DEFAULT_FORMATS = {
'blacklight.citation.mla': :export_as_mla,
Copy link
Member

Choose a reason for hiding this comment

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

We need a custom routine for each citation form we want to support, correct? I like that this approach would route all types of records through the same routines. We definitely don't want some records to go through blacklight-marc and others to go through citeproc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the next step is to add the other citation formats. Right now if it's an Alma record it does use the Marc version, but we could change that once we've gotten the CiteProc code more refined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket to route all types of records through the same routines - #4428

@maxkadel maxkadel changed the title Add MLA citation for SCSB items Add citations for SCSB items Oct 8, 2024
- Moves shared CiteProc methods into their own module
@maxkadel maxkadel force-pushed the i4012_add_citation_scsb branch from 8032f55 to 1c20768 Compare October 8, 2024 16:20
@maxkadel maxkadel marked this pull request as ready for review October 9, 2024 07:46
Copy link
Member

@sandbergja sandbergja left a comment

Choose a reason for hiding this comment

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

Thanks, @maxkadel ! 🦄 I saw one very small thing in the Gemfile, the actual code looks great to me.

Gemfile Outdated Show resolved Hide resolved

require 'rails_helper'

RSpec.describe Blacklight::Document::Apa, citation: true do
Copy link
Member

Choose a reason for hiding this comment

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

Woohoo citation tag!

# frozen_string_literal: true

# Creates an html APA citation for non-Marc records
module Blacklight::Document::Apa
Copy link
Member

Choose a reason for hiding this comment

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

I like this structure with the 3 modules for the 3 citation styles

Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
@maxkadel maxkadel force-pushed the i4012_add_citation_scsb branch from a649dbd to df3938c Compare October 10, 2024 06:45
@maxkadel maxkadel requested a review from sandbergja October 10, 2024 07:10
Copy link
Member

@sandbergja sandbergja left a comment

Choose a reason for hiding this comment

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

Woohoo! Thanks, @maxkadel 🦄

@sandbergja sandbergja merged commit 849803f into main Oct 10, 2024
13 checks passed
@sandbergja sandbergja deleted the i4012_add_citation_scsb branch October 10, 2024 15:09
@maxkadel maxkadel mentioned this pull request Oct 11, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants