[HOLD] only shelve WAS files via ShelveJob #5076
Conversation
.rubocop_todo.yml
Outdated
| @@ -67,6 +68,7 @@ RSpec/Be: | |||
| RSpec/BeforeAfterAll: | |||
| Exclude: | |||
| - 'spec/services/digital_stacks_service_spec.rb' | |||
There was a problem hiding this comment.
Looks like there are TODOs for files that you deleted.
Is it possible to not add any new TODOs? Either disable the rules if will never enforce or add inline disables.
There was a problem hiding this comment.
Thanks for catching those deleted files. Disabled rules.
app/services/was_shelving_service.rb
Outdated
| end | ||
|
|
||
| def initialize(cocina_object) | ||
| raise ConfigurationError, 'Missing configuration Settings.stacks.local_workspace_root' if Settings.stacks.local_workspace_root.nil? |
There was a problem hiding this comment.
I would just raise StandardError here; no reason to create a special error class.
app/services/was_shelving_service.rb
Outdated
|
|
||
| def initialize(cocina_object) | ||
| raise ConfigurationError, 'Missing configuration Settings.stacks.local_workspace_root' if Settings.stacks.local_workspace_root.nil? | ||
| raise WasShelvingService::WasShelvingError, 'Missing structural' if cocina_object.structural.nil? |
There was a problem hiding this comment.
Why would this happen? (Also, I would put this in the shelve method instead of the initializer.)
There was a problem hiding this comment.
This was a possible situation that was being handled when shelving other files, but since there is really only one pathway for web archive files to get accessioned, seems extremely unlikely. I've taken it out.
app/services/was_shelving_service.rb
Outdated
|
|
||
| def initialize(cocina_object) | ||
| raise ConfigurationError, 'Missing configuration Settings.stacks.local_workspace_root' if Settings.stacks.local_workspace_root.nil? | ||
| raise WasShelvingService::WasShelvingError, 'Missing structural' if cocina_object.structural.nil? |
There was a problem hiding this comment.
Why would this happen? (Also, I would put this in the shelve method instead of the initializer.)
app/services/was_shelving_service.rb
Outdated
| attr_reader :cocina_object, :druid | ||
|
|
||
| def shelve | ||
| # determine destination web archiving stacks location |
There was a problem hiding this comment.
I'd suggest moving these into methods to make this method clearer.
There was a problem hiding this comment.
Agree that is an improvement. Moved the methods.
app/services/was_shelving_service.rb
Outdated
| "/web-archiving-stacks/data/collections/#{collection_druid.delete_prefix('druid:')}" | ||
| end | ||
|
|
||
| def files_for(cocina_object) |
There was a problem hiding this comment.
cocina_object is an instance variable so no need to pass in.
app/services/was_shelving_service.rb
Outdated
|
|
||
| raise WasShelvingService::WasShelvingError, 'Web archive object missing collection' unless collection_druid | ||
|
|
||
| "/web-archiving-stacks/data/collections/#{collection_druid.delete_prefix('druid:')}" |
There was a problem hiding this comment.
Should /web-archiving-stacks be hardcoded?
There was a problem hiding this comment.
I've added a setting for it.
There was a problem hiding this comment.
Also updated tests.
app/services/was_shelving_service.rb
Outdated
| Rails.logger.debug("[Was Shelve] Copying #{workspace_pathname} to #{stacks_pathname}") | ||
| FileUtils.cp workspace_pathname.to_s, stacks_pathname.to_s | ||
| # Change permissions | ||
| FileUtils.chmod 'u=rw,go=r', stacks_pathname.to_s |
There was a problem hiding this comment.
Why do we do this? (Can you add reason to L62?)
There was a problem hiding this comment.
I did some more testing, and I think that may not be a concern with web archiving stacks. Seems like the permissions were already as needed. I'll check again when testing on stage just to be sure.
There was a problem hiding this comment.
Checked on stage and permissions are being set to 644. No need to change them.
lwrubel
force-pushed
the
t5072-warc-shelving
branch
from
b0a7025
to
ae974e7
Compare
lwrubel
force-pushed
the
t5072-warc-shelving
branch
from
ae974e7
to
2a84daa
Compare
app/services/was_shelving_service.rb
Outdated
| end | ||
|
|
||
| def initialize(cocina_object) | ||
| raise StandardError, 'Missing configuration Settings.stacks.local_workspace_root' if Settings.stacks.local_workspace_root.nil? |
There was a problem hiding this comment.
There is a default value in the settings. Would this ever happen?
app/services/was_shelving_service.rb
Outdated
| Pathname(workspace_druid.content_dir(true)) | ||
| end | ||
|
|
||
| def files_for |
There was a problem hiding this comment.
I'd just call this filenames or similar. Usually it is x_for(some_parameter).
There was a problem hiding this comment.
Ah, I see. Thanks for the suggestion.
| end | ||
|
|
||
| def was_stacks_dir | ||
| # determine destination web archiving stacks directory |
There was a problem hiding this comment.
This should be memoized since it is used multiple times.
| end | ||
|
|
||
| def workspace_content_dir | ||
| # determine current workspace location of object's content file |
|
Be sure to squash when merging. |
Why was this change made? 馃
Resolves #5072 for shelving web archiving files only. HOLD because we must be using purl-fetcher for shelving all other files in production before merging.
How was this change tested? 馃え
Unit and integration tests on stage.