-
Notifications
You must be signed in to change notification settings - Fork 0
SCC-5105: MARC endpoint #608
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
base: main
Are you sure you want to change the base?
Conversation
lib/marc-serializer.js
Outdated
| .filter(Boolean) | ||
|
|
||
| return { | ||
| unserialized: bib, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will remove but it's helpful for testing
yossariano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
| @@ -0,0 +1,166 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're putting the rest of the codebase to shame with all this detailed documentation! Nicely done though it definitely helps make it easy to understand what is going on.
| const parallels = MarcSerializer.findParallelFields(bib, field) | ||
| parallels.forEach((p) => { | ||
| Object.assign(p, MarcSerializer.buildSourceWithMasking(p, matchingRule)) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an effort to link parallel fields to primary fields so that they can be visually linked on the front-end? Aren't we just rendering 880s as is?
Incidentally I'm not seeing known 880 values come through when viewing/api/v0.1/discovery/resources/b22144813.marclocally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this may be wrong then– I'm trying to find the parallels of suppressed fields and suppress them, but otherwise pass parallels through
lib/marc-serializer.js
Outdated
| bib: { | ||
| id: bib.id, | ||
| nyplSource: bib.nyplSource, | ||
| fields: suppressedVarFields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this variable is misnamed, but something here does not seem right. Why are you returning something called suppressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, renamed to serialized to mean all the processed varfields
lib/marc-serializer.js
Outdated
|
|
||
| if (!linkNumbers.length) return [] | ||
|
|
||
| return bib.varFields.filter((f) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm finding this chunk hard to grok. Can you use some more descriptive variable names and/or pull some of these array method callbacks into named functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken down into some more chunks
| subfields: (field.subfields || []).map((subfield) => { | ||
| let content = subfield.content | ||
| if ( | ||
| (rule.subfieldSpec.directive === 'include' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is in the original code, but do we know why this is logically here? If the directive says include but it is not present in the subfield spec, wouldn't we just skip it? Wouldn't this add [redacted] for fields that don't have content? @nonword maybe you have some insight?
Adds MARC endpoint which should:
and return the serialized bib.
good test:
b22144813