Skip to content
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

1033 external form upload #1038

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ def index
authorize! :external_form_upload,
context: {organization: Current.organization}
end

def create
authorize! :external_form_upload,
context: {organization: Current.organization}
files = params.require(:files)
files.each do |file|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

next if !file.is_a?(ActionDispatch::Http::UploadedFile)
import_service = Organizations::Importers::GoogleCsvImportService.new(file)
import_service.call
end
end
end
end
end
4 changes: 4 additions & 0 deletions app/policies/organizations/external_form_upload_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,9 @@ class ExternalFormUploadPolicy < ApplicationPolicy
def index?
permission?(:manage_external_form_uploads)
end

def create?
permission?(:manage_external_form_uploads)
end
end
end
32 changes: 0 additions & 32 deletions app/services/organizations/csv_import_service.rb

This file was deleted.

34 changes: 34 additions & 0 deletions app/services/organizations/importers/google_csv_import_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
require "csv"

module Organizations
module Importers
class GoogleCsvImportService
def initialize(file)
@file = file
@organization = Current.organization
end

def call
CSV.foreach(@file.to_path, headers: true, skip_blanks: true) do |row|
# Using Google Form headers
email = row["Email"].downcase
csv_timestamp = Time.parse(row["Timestamp"])

person = Person.find_by(email:, organization: @organization)
previous = FormSubmission.where(person:, csv_timestamp:)
next unless person && previous.empty?

ActiveRecord::Base.transaction do
form_submission = FormSubmission.create!(person:, csv_timestamp:)
row.each do |col|
next if col[0] == "Email" || col[0] == "Timestamp"

FormAnswer.create!(form_submission:,
question_snapshot: col[0], value: col[1])
end
end
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
<div>
<div>
<p class="fs-4">If you use a third party form service, like Google Forms, to provide questionnaires to potential adopters, and receive their answers, you can upload the CSV of data here to import the questions and responses to this application. This means the questionnaire data will live in one place, and you will be able to view it for a given adoption application at any time. Note that the adopter must have an account in this application using the same email address they used in the third party form.</p>
<%= render "organizations/staff/shared/attachment_form", instance: @external_form, title: 'Files', url: staff_external_form_upload_index_path(@pet), attachment_type: 'files' %>
<p class="mb-4">Current form service supported: Google Forms</p>
<%= render "organizations/staff/shared/attachment_form", instance: nil, title: 'Files', url: staff_external_form_upload_index_path, attachment_type: 'files' %>
</div>
</div>
</section>
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
resource :organization, only: %i[edit update]
resource :custom_page, only: %i[edit update]
resources :profile_reviews, only: [:show]
resources :external_form_upload, only: [:index]
resources :external_form_upload, only: %i[index create]

resources :pets do
resources :tasks
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
require "test_helper"

class Organizations::Staff::ExternalFormUploadControllerTest < ActionDispatch::IntegrationTest
setup do
@organization = ActsAsTenant.current_tenant
@file = fixture_file_upload("google_form_sample.csv", "text/csv")
@params = {files: [@file]}
@user = create(:admin, email: "[email protected]")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@user2 = create(:admin)
sign_in @user
end

test "Import from google_form_sample.csv" do
# FomAnswers and FormSubmissions should be created
assert_difference "FormAnswer.count", 4 do
assert_difference "FormSubmission.count", 1 do
post staff_external_form_upload_index_path, params: @params
end
end
# There should be a form submission for @user but not for @user2
# assert FormSubmission.exists?(person_id: @user.id)
assert_not FormSubmission.exists?(person_id: @user2.id)
end
end
2 changes: 2 additions & 0 deletions test/fixtures/files/google_form_sample.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Timestamp,Nombre,Email,Dirección,Número de teléfono,Comentarios
2024/10/05 7:14:21 p. m. GMT-4,dsf,[email protected],dsfsdf,sdfsdf,ds
6 changes: 3 additions & 3 deletions test/services/organizations/csv_import_service_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class CsvImportServiceTest < ActiveSupport::TestCase

assert_difference "FormSubmission.count" do
assert_difference("FormAnswer.count", + 7) do
Organizations::CsvImportService.new(@file).call
Organizations::Importers::GoogleCsvImportService.new(@file).call
end
end
end
Expand All @@ -48,7 +48,7 @@ class CsvImportServiceTest < ActiveSupport::TestCase
end

assert_no_difference "FormSubmission.count" do
Organizations::CsvImportService.new(@file).call
Organizations::Importers::GoogleCsvImportService.new(@file).call
end
end

Expand All @@ -58,7 +58,7 @@ class CsvImportServiceTest < ActiveSupport::TestCase
csv << @data
end
assert_difference "FormSubmission.count" do
Organizations::CsvImportService.new(@file).call
Organizations::Importers::GoogleCsvImportService.new(@file).call
end
end
end
Expand Down
Loading