Accessibility Checker #1627
Accessibility Checker #1627
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| resource.file_data['metadata']['size'] < FILE_SIZE_LIMIT | ||
| end | ||
|
|
||
| def fetch_accessibility_report |
There was a problem hiding this comment.
Method fetch_accessibility_report has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
| file_resource.file_data['metadata']['size'] < FILE_SIZE_LIMIT | ||
| end | ||
|
|
||
| def fetch_accessibility_report |
There was a problem hiding this comment.
Method fetch_accessibility_report has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.
… model scope. Tests for this. Also, using .create! when storeing acccessibility check results so an error is raised if one already exists. The associated model should be deleted entirely if we want to try again.
|
|
||
| def client_secret | ||
| if Rails.env.test? | ||
| 'adobe_client_secret' |
There was a problem hiding this comment.
Something to consider (probably down the line) – we could consider having distinct env files for different environments, so that this sort of logic wouldn't be in the code itself.
For example, direnv/direnv#320 (comment)
There was a problem hiding this comment.
Ah okay. I didn't know you could do that with direnv. I wonder if there's a way to switch contexts depending on an ENV variable? From what I understand in that issue, you still need to manually switch contexts.
| # my_asset = MyAsset.new(resource) | ||
| # my_asset.perform_operations | ||
| # | ||
| # @yield The block to run additional operations on the asset. |
There was a problem hiding this comment.
Really helpful to include an example like this, thanks!
|
|
||
| def asset_id | ||
| @asset_id ||= asset_upload_uri_and_asset_id['assetID'] | ||
| end |
There was a problem hiding this comment.
Could this and upload_uri potentially get set during initialize as well?
There was a problem hiding this comment.
Yeah, I wasn't sure what would be better. They both require similar amounts of code, and result in similar memoization. I think with the way I have it, the value won't be stored until it's needed, which probably makes the code infinitesimally more efficient.
| AccessibilityCheckResult.create!(file_resource: resource, | ||
| detailed_report: { | ||
| error: 'Accessibility check failed: Polling time limit exceeded' | ||
| }) |
There was a problem hiding this comment.
I noticed code climate's Complexity warning, but I'm not seeing a lot of obvious places to trim this method down. One very minor thing could be to pull lines 48-51 into some kind of createTimeoutResult method.
There was a problem hiding this comment.
Yeah, this one might be hard to clean up.
| 'Passed manually' => 0, | ||
| 'Skipped' => 0 | ||
| } | ||
| }) |
There was a problem hiding this comment.
This feels brittle to me, as even slight changes to how Adobe formats their responses would require test rewirites. I'm not sure what the best alternative, but I'd be content know we got a result containing 'Detailed Report'.
There was a problem hiding this comment.
The request is mocked with VCR/Webmock here spec/fixtures/vcr_cassettes/AdobePdf_AccessibilityChecker/_call/happy_path/fetches_the_accessibility_report.yml, so it won't break unless that is turned off. If we do turn that off and run a live test, it would be good to have it break so we can make changes. I added an issue to further handle the format and storing of the report here #1637 .
| # frozen_string_literal: true | ||
|
|
||
| module AdobePdf | ||
| # TODO: This whole module can potentially be decoupled from scholarsphere. |
There was a problem hiding this comment.
I'm glad we have this on our todo list. Crossing fingers for reusability!
ajkiessl
dismissed stale reviews from Smullz622 and jlandiseigsti
via
78ca26b
closes #1602
Module for pulling a file from S3, sending it up to Adobe through the AdobePdf API, accessibility checking, storing the accessibility report, and deleting the file in Adobe. There's some over lap with #1613 with storing the report, but it looks like the conflicts are minimal. I also threw a small accessibility fix in here to make the three required fields for culminating experiences required in the view.
I added an issue (#1638) to decouple the AdobePdf module from ScholarSphere. It's not a priority, but it might be helpful architecturally, and if we want to use this elsewhere. I also added an issue (#1637) to use setters for storing the report from Adobe. Again, not a priority, but it could help us keep track of changes Adobe makes to the report's structure.