-
Notifications
You must be signed in to change notification settings - Fork 4
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
Guard against nil tempfiles in MARC export job #914
Conversation
1a0644e
to
7e4b138
Compare
@JPrevost Interestingly, this didn't come up in local testing. I'm planning to create some dummy theses in the PR build to confirm that this fixes the problem. |
05aba70
to
338bdba
Compare
6547f1a
to
906506a
Compare
f58d8ef
to
347958d
Compare
@JPrevost Just confirmed that this is ready for review after much ado. If you'd like to test yourself, I created two fake-published theses that should export successfully using the MarcExportJob. I haven't yet run the full DspacePublicationResultsJob but I will probably do that before merging. (I ran into issues with the pipeline handles, which are fixed now.) |
Why these changes are being introduced: If a tempfile has already been closed and unlinked, the `ensure` block in MarcExportJob#perform will throw an error. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-546 How this addresses that need: This checks if the tempfile exists before attempting to close it. Side effects of this change: Adds rubyzip to gemfile. Rubyzip is already used by other gems, and it can be called in local development, but bootsnap seems to require it to be loaded explicitly.
a2b21cc
to
a0caeca
Compare
Why these changes are being introduced:
If a tempfile has already been closed and unlinked, the
ensure
block in MarcExportJob#perform will throw an error.
Relevant ticket(s):
How this addresses that need:
This checks if the tempfile exists before attempting to close it.
Side effects of this change:
Adds rubyzip to gemfile. Rubyzip is already used by other gems, and
it can be called in local development, but bootsnap seems to require
it to be loaded explicitly.
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?
YES (sort of -- we are explicitly adding
rubygems
to the gemfile, whereas it was previously installed by other dependencies)