-
Notifications
You must be signed in to change notification settings - Fork 66
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
[WIP] Benefits disability migrate 0781 to lighthouse #18923
Conversation
Generated by 🚫 Danger |
48174aa
to
259865f
Compare
e90d846
to
d050e9c
Compare
326ae14
to
3362eab
Compare
3362eab
to
1360169
Compare
1360169
to
77f0ae9
Compare
@@ -184,6 +199,7 @@ def get_evss_claim_metadata(pdf_path, form_id) | |||
end | |||
|
|||
def create_document_data(evss_claim_id, upload_data) | |||
# AJ todo - should this be a LH doc? |
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 ApiProvider takes care of this under the hood but for BDD I left the original code outside of the killswitch toggle which does create an EVSSClaimDocument
still
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.
Ok, thanks! I'll leave it as is.
let(:path_to_0781a_fixture) { 'spec/fixtures/pdf_fill/21-0781a/simple.pdf' } | ||
let(:sample_0781a_file_read) { File.read(path_to_0781a_fixture) } | ||
let(:parsed_0781_form) { JSON.parse(submission.form_to_json(Form526Submission::FORM_0781))['form0781'] } | ||
let(:parsed_0781a_form) { JSON.parse(submission.form_to_json(Form526Submission::FORM_0781))['form0781a'] } |
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 is just a personal opinion but I'd split the test setup and tests into separate context blocks for 0781 and 0781a to keep this a little more organized
|
||
allow_any_instance_of(LighthouseSupplementalDocumentUploadProvider).to receive(:generate_upload_document).and_return(lighthouse_0781_document) | ||
|
||
allow(File).to receive(:delete).and_return(nil) |
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 is another one. Yeah I feel like it's such an anti pattern to have the test know all of these internal details/we have to maintain this, but I think it's just a problem with the design of all of the document uploads
77f0ae9
to
84c8363
Compare
6daf82f
to
ee63b97
Compare
Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.
Summary
Related issue(s)
Testing done
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
Acceptance criteria
Requested Feedback
(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?