-
Notifications
You must be signed in to change notification settings - Fork 359
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
Improved RIS export #3954
Improved RIS export #3954
Conversation
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.
Thanks, @damien-git -- this looks like a significant improvement. I have a few suggested adjustments/improvements; see below.
module/VuFind/src/VuFind/RecordDriver/Feature/MarcAdvancedTrait.php
Outdated
Show resolved
Hide resolved
themes/root/templates/RecordDriver/AbstractBase/export-ris.phtml
Outdated
Show resolved
Hide resolved
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.
Thanks, @damien-git and @meganschanz! This is coming along nicely, though there are a few more details that may need tweaking...
themes/root/templates/RecordDriver/AbstractBase/export-ris.phtml
Outdated
Show resolved
Hide resolved
themes/root/templates/RecordDriver/AbstractBase/export-ris.phtml
Outdated
Show resolved
Hide resolved
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.
Thanks for the progress, @damien-git. See below for one new question raised by the latest EDS refactoring.
module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/EDSTest.php
Outdated
Show resolved
Hide resolved
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.
Thanks for the ongoing progress, @meganschanz and @damien-git! See below for a few more suggestions, mostly trivial and related to comments. (I also left a separate comment earlier today before this review regarding the extraction of EDS subjects).
module/VuFind/src/VuFind/RecordDriver/Feature/MarcAdvancedTrait.php
Outdated
Show resolved
Hide resolved
module/VuFind/src/VuFind/RecordDriver/Feature/MarcAdvancedTrait.php
Outdated
Show resolved
Hide resolved
module/VuFind/src/VuFind/RecordDriver/Feature/MarcAdvancedTrait.php
Outdated
Show resolved
Hide resolved
module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/Feature/MarcAdvancedTraitTest.php
Outdated
Show resolved
Hide resolved
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.
One tiny comment adjustment, and then I think this is probably ready to merge.
Once this is merged, I suggest two separate follow-up PRs:
1.) Since this PR narrows the scope of getSummary (removing the Abstracts), I think we should add a separate 'Abstract' line to the RecordDataFormatter configuration as soon as possible, so users who upgrade don't lose visibility to any important information. Fortunately, we already have translations for an 'Abstract' label, so this can be done in a PR targeting the current dev branch without requiring new translation strings.
2.) The other notes you want to add should be introduced in a PR targeted against dev-11.0, since these will require new translations.
Please let me know if you have any questions or concerns. I'm running the full test suite on this branch right now and will report back when the run completes. I expect it should pass, and in that case, I think this is ready to merge once the trivial comment change below is addressed.
Tests are passing! |
Should be all set now after the comment fix (good catch). I agree that 2 PRs makes sense for the follow up work. I'll start working on those today. |
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.
Thanks again, @damien-git and @meganschanz! I'm merging this now and will also ensure that the dev branch is merged into dev-11.0 so everything is up to date and ready for the follow-up work.
This PR adds additional tags to the RIS export. A number of them are based on new SolrMarc functions, so they are specific to this driver.
New tags:
Updated tags:
There might be ways to improve the way this implementation works with other drivers...
TODO