Add publishers field to record type #810
Conversation
|
It's been a minute since I've added a field to the API, so please let me know if I've missed something here (particularly from the testing standpoint)! |
There was a problem hiding this comment.
Looks good. Let's confirm with DataEng whether they are mapping anything to the data or location fields before we merge. If they are not doing that yet, we might be better off removing them for now from the API spec and adding them in when data starts flowing into them.
| field :imprint, [String], null: true, deprecation_reason: 'Use `publicationInformation`' | ||
| field :publication_information, [String], description: 'Imprint information for item' | ||
| field :imprint, [String], null: true, deprecation_reason: 'Use `publishers`' | ||
| field :publication_information, [String], deprecation_reason: 'Use `publishers`' |
There was a problem hiding this comment.
We should probably do a deprecation method here to populate publication_information from a concatenation of the various publishers subfields if publication_information does not exist. If these words make no sense, let me know and I can better describe what this might look like to allow for the smoothest transition for any consumers of this field.
There was a problem hiding this comment.
Oh, good call. I've not implemented something like that, but it does make sense. I'm going to check with Graham first to see whether there's data in all subfields before proceeding any further.
There was a problem hiding this comment.
Take a look at the methods that follow the RecordType definitions to see how this works. If the method name matches the field, it'll run instead of auto mapping.
So in this case, you'll create a method named publication_information and have it return whatever makes sense. Probably just pass through @object['publication_information'] if it exists (until we fully rip that out of the data which may be soonish?) and construct a string of various concatenated sub fields from @object['publication'] if not. Or something like that.
There was a problem hiding this comment.
Ha you pushed code as I was making that comment. FWIW I think it's probably fine. I don't think publication_information is long for this world (I think it goes away after full harvests for each source) so we likely don't need the complexity I suggested.
There was a problem hiding this comment.
I was just about to ask. :) In that case, I'll re-request review.
app/graphql/types/record_type.rb
Outdated
| @@ -205,6 +212,10 @@ def publication_date | |||
| @object['dates']&.map { |date| date['value'] if date['kind'] == 'Publication date' }&.compact&.first | |||
| end | |||
|
|
|||
| def publication_information | |||
| @object['publishers']&.map { |publisher| publisher.values.join(', ') } | |||
There was a problem hiding this comment.
Would you mind joining on ; ? I worry that a publisher name or location could have commas and be weird-ish.
Publisher, Inc., December, 1, 1999, Boston, Ma vs Publisher, Inc.; December, 1, 1999; Boston, Ma.
Not a blocker though as both suck and it's a deprecated field lol
There was a problem hiding this comment.
Good suggestion. I went ahead and made that change. (Didn't feel like it made sense to request another review, but feel free to confirm if you like!)
Why these changes are being introduced: We recently [made the decision in transmogrifier](MITLibraries/transmogrifier#132) To map publication information to a new nested field, named `publishers` to avoid a naming conflict with the existing `publicationInformation` field. Relevant ticket(s): * [GDT-270](https://mitlibraries.atlassian.net/browse/GDT-270) * [GDT-218](https://mitlibraries.atlassian.net/browse/GDT-218) * [GDT-229](https://mitlibraries.atlassian.net/browse/GDT-229) * [GDT-271](https://mitlibraries.atlassian.net/browse/GDT-271) How this addresses that need: This adds the new `publishers` field and deprecates `publicationInformation`. Side effects of this change: A follow-on ticket is needed to add this field to the UI. (This is ticketed as GDT-271.)
jazairi
force-pushed
the
gdt-218-publishers-field
branch
from
82b7b9a
to
5010917
Compare
Why these changes are being introduced:
We recently made the decision in transmogrifier To map publication information to a new nested field, named
publishersto avoid a naming conflict with the existingpublicationInformationfield.Relevant ticket(s):
How this addresses that need:
This adds the new
publishersfield and deprecatespublicationInformation.Side effects of this change:
A follow-on ticket is needed to add this field to the UI. (This is ticketed as GDT-271.)
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO