-
Notifications
You must be signed in to change notification settings - Fork 9
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
Vamc 15446 spike vamc police data page #1753
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.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
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.
Following a pattern from drupal graphql static files here. Not sure if at some point we'll need any of these variables.
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.
Not really an official review. This is shaping up pretty nicely. Mainly just a couple questions to help me understand it better.
…sible in content-build at the moment
I think this is a funny message, considering this is not a vets-website PR. |
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.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
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.
The pull request looks good to me, I just have a few suggestions and questions before I'm ready to approve. Overall, it's looking good!
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
Outdated
Show resolved
Hide resolved
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
Outdated
Show resolved
Hide resolved
url | ||
.pathToFileURL(join(__dirname, 'vaPoliceData', 'police-events.csv')) | ||
.toString(), | ||
]), |
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.
This could maybe be simplified so we have an array of the two strings in each pathToFileURL
function, and we map them out to get this.
query: queryVAPoliceData(['police-contact.csv', 'police-events.csv']
.map(filename => url.pathToFileURL(join(__dirname, 'vaPoliceData', filename)))
),
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.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
Outdated
Show resolved
Hide resolved
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
Dismissed
Show dismissed
Hide dismissed
src/site/stages/build/drupal/static-data-files/vaPoliceData/index.js
Dismissed
Show dismissed
Hide dismissed
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.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
@@ -3,7 +3,8 @@ describe('Static Data Files Test', () => { | |||
/* This is not a visual test -- no accessibility involvement */ | |||
/* eslint-disable @department-of-veterans-affairs/axe-check-required */ | |||
it('has the EHR JSON static file', () => { | |||
cy.deleteFileOrFolder('../cypress/downloads/vamc-ehr.json'); | |||
cy.deleteFileOrDir('../cypress/downloads/vamc-ehr.json'); | |||
cy.fileOrDirExists('cypress/downloads/vamc-ehr.json').should('eq', false); |
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.
👏
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.
LGTM
Sharp eye 👀 Seems it's just copy/pasta |
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.
LGTM
Description
relates to department-of-veterans-affairs/va.gov-cms#15446
Creates a new process for queryType: "curl" in static-data-files
This allows an arbitrarily large number of files to be downloaded via curly/node-libcurl GET and a postProcess step that receives the data in an array and can process the resulting data in any manner needed.
It uses libcurl because node-fetch has problems using file:// URLs which we need for the police instance.
The police postProcess step writes the files to temporary files and reads them in with csvtojson to later process (this ticket only starts the structure, since it is unknown what the format of the CSVs will be at the moment).
Changes were made to the static file creation, in that if any of the files are not present, all are generated. Previously only a test was done to determine if the directory exists, but now that there are multiple processes generating files, it seemed appropriate to convert to a per-file test.
Testing done & Screenshots
Added cypress testing for the existence of all JSON static data files.
QA steps
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?
Acceptance criteria
Definition of done