DiscoveryReport templates #255
Conversation
Pull Request Test Coverage Report for Build 800
💛 - Coveralls |
There was a problem hiding this comment.
So as this stands, we have view code for the discovery report, but the view code isn't triggered by anything so it isn't actually rendered anywhere -- it's just to have a way to write tests?
and is the ultimate idea that we will have templates write to a file, and the discovery_report_job will tell discovery report to run, and that job or another will send an email with a url when the report is ready? (and the form submission response could also provide the same url, but the data isn't there yet)?
'cause if that what this is, I'd rather merge a PR that creates a file and tests what will be written to the file as these templates are sort of "test-y".
Might also be good to check with Ben if we want to produce a csv report or if there are any other "what the report should look like" things (I doubt it)
app/services/discovery_report.rb
Outdated
| fields.join(' , ') | ||
| end | ||
|
|
||
| # For use by template | ||
| def skipped_files | ||
| files = ['Thumbs.db', '.DS_Store'] # if these files are in the bundle directory but not in the manifest, they will be ignorned and not reported as missing | ||
| files << File.basename(content_md_creation[:smpl_manifest]) if using_smpl_manifest? | ||
| files << File.basename(manifest) if using_manifest? |
There was a problem hiding this comment.
I thought we were requiring a manifest file in all cases -- so the if clause can go, but the line should stay?
There was a problem hiding this comment.
^^ whoops - I guess the above is a "request changes"
There was a problem hiding this comment.
Thanks, incorporated 2 lines earlier in the initial array.
|
Yes, I think your projection of the ultimate goal is correct. But to be clear, this PR is not going to get into:
So there is no payoff to holding out for those things here. The next step (that could be in PR or subsequent) would be making the partial template for |
app/services/discovery_report.rb
Outdated
| fields.concat ['Num Files in CM Manifest', 'All CM files found'] if using_smpl_manifest? | ||
| fields.concat ['Duplicate Filenames?', 'DRUID', 'Registered?', 'APO exists?'] | ||
| fields << 'SourceID unique in DOR?' if using_manifest? | ||
| fields.concat ['Duplicate Filenames?', 'DRUID', 'Registered?', 'APO exists?', 'SourceID unique in DOR?'] |
There was a problem hiding this comment.
whoops - can get rid of "SourceID unique in DOR?" header -- sorry I didn't notice on first pass.
There was a problem hiding this comment.
Seems like we can get rid of headers completely, if we are making the report output JSON.
app/services/discovery_report.rb
Outdated
| fields.join(' , ') | ||
| end | ||
|
|
||
| # For use by template | ||
| def skipped_files | ||
| files = ['Thumbs.db', '.DS_Store'] # if these files are in the bundle directory but not in the manifest, they will be ignorned and not reported as missing | ||
| files = ['Thumbs.db', '.DS_Store', manifest] # if these files are in the bundle directory but not in the manifest, they will be ignorned and not reported as missing |
There was a problem hiding this comment.
looks good. slight preference for manifest first (and I think :manifest might need to get a delegation in bundle.db? Also the comment reads a little weird now (and has a typo) -- maybe just change it to a method comment?
Also remove `header` method. We won't be building pseudo-CSV w/ handcrafted header string anymore.
This is just to show how the object, template and partial fit together. Note: because of the Enumerator, we can use the form: ``` render partial: 'row', collection: @discovery_report.each_row ``` That knows to iterate of the Enumerator.
|
I'm cool w/ merging this as is, since from our conversations, I gather that what we are really after is more structured JSON output (not plaintext). |
|
merging. Assume you'll address the need to delegate :manifest in bundle.rb in another PR. |
These are stub templates, but show structure and test it. I don't mind if reviewers prefer to wait for them to be further fleshed out, but they are mergeable as is. Since there is some confusion about how these components might work together, this may be clarifying.
Note: because of the Enumerator, we can use the form:
That knows to iterate over the Enumerator.
We can also imagine other template outputs (JSON, HTML, etc.) based on the same structure.