1033 external form upload #1038
Conversation
|
@kasugaijin There is an assertion that is commented. This is because it works perfectly when tested individually. But when using |
| @organization = ActsAsTenant.current_tenant | ||
| @file = fixture_file_upload("google_form_sample.csv", "text/csv") | ||
| @params = {files: [@file]} | ||
| @user = create(:admin, email: "[email protected]") |
There was a problem hiding this comment.
We should be creating an adopter here, instead of admin
There are also some changes happening in this PR #997
- a Person can have multiple form submissions
- a form submission will be created on a person when a new adopter signs up
This means the asserstions on lines 20 and 21 will become unnecessary. We'll want to just test two cases
- when a form submission does not have any form answers (and no CSV timestamp), assert that new form answers are created
- when a form submission does have a csv timestamp matching the timestamp in the CSV, no new Form Answers are created
I think the best thing to do, here, is wait for that PR to get merged, then finish these tests off. Thoughts?
There was a problem hiding this comment.
Ok, I understand lines 20 and 21 can be removed.
On the other assertions I'm verifying that FormSubmissions and FormAnswers are being created after the upload csv. Where we have 4 answers and questions/cells and 1 form was uploaded so one FormSubmission was created.
As I understood we may need to add a test for when a csv file matches an existing csv_timestamp entry... Nothing should happen. But all of these are subject to change after PR #997
Then I will remove the unnecessary assertions, address controller comments, and wait for your instructions on the tests.
| authorize! :external_form_upload, | ||
| context: {organization: Current.organization} | ||
| files = params.require(:files) | ||
| files.each do |file| |
There was a problem hiding this comment.
I think we should only really allow 1 upload here...it doesn't make sense for more than one file.
Given we are using app/views/organizations/staff/shared/_attachment_form.html.erb partial, we could make the value for the multiple attribute an argument that gets passed to the partial. So in this case, it would be false and in the other place the partial is used, on the pet, it would be true. Thoughts?
Then we can get rid of this loop here.
Also, curious of the benefit of checking file.is_a?(ActionDispatch::Http::UploadedFile)
There was a problem hiding this comment.
file.is_a?(ActionDispatch::Http::UploadedFile) It is because the received parameter contains other things like "". Maybe It will be unnecessary when using just one file.
I don't understand completely the use case due to a lack of background knowledge of the project. If you think a single file is the way to go, I will implement the suggested changes.
There was a problem hiding this comment.
Yes this import service is making the assumption there is only one third party form, and therefore one CSV to upload. It doesn’t currently handle the case where adopter data lives across multiple forms. So it would be good to only allow one file for now.
Modify external form controller to support just one file upload correct controller test
🔗 Issue
#1033
✍️ Description
📷 Screenshots/Demos