1033 external form upload #1038

wandergithub · 2024-10-06T20:16:12Z

🔗 Issue

✍️ Description

Add a note for Google form support to description.
Add a route and create a method to use form upload service to process the csv file
adds test

📷 Screenshots/Demos

…al csv forms with google_csv_import_service

wandergithub · 2024-10-06T20:19:22Z

@kasugaijin There is an assertion that is commented. This is because it works perfectly when tested individually. But when using ./bin/rails test:all it fails. I don't know why the FormSubmission person_id is different from the user_id when tested with the mentioned command but it matches as it should when tested individually. I could not find where is it being affected.

kasugaijin · 2024-10-06T23:07:31Z

test/controllers/organizations/staff/external_form_upload_controller_test.rb

+    @organization = ActsAsTenant.current_tenant
+    @file = fixture_file_upload("google_form_sample.csv", "text/csv")
+    @params = {files: [@file]}
+    @user = create(:admin, email: "adopter1@alta.com")


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?

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.

@wandergithub FYI this PR has now been merged #997
So now person has many form submissions

@wandergithub let me know if you have any blockers on this :)

@kasugaijin Yeah, I would like to know which requirements or specifications changed on this PR. It's not clear to me.

app/controllers/organizations/staff/external_form_upload_controller.rb

Modify external form controller to support just one file upload correct controller test

kasugaijin · 2024-10-10T02:37:12Z

test/controllers/organizations/staff/external_form_upload_controller_test.rb

@@ -0,0 +1,21 @@
+require "test_helper"
+
+class Organizations::Staff::ExternalFormUploadControllerTest < ActionDispatch::IntegrationTest


Can we nest the modules instead please?

test/controllers/organizations/staff/external_form_upload_controller_test.rb

wandergithub · 2024-10-10T18:12:51Z

does not fail locally ??

kasugaijin · 2024-10-10T20:19:16Z

Some thoughts
https://www.loom.com/share/68033246a92b43a98e33fd72e466a625?sid=337cc0d5-0237-4e35-b6a3-d6b3328d3f72

i finish my thoughts here
https://www.loom.com/share/7fc7094d4733480a8fa876abb7c0ec24?sid=6b6341d3-1bb8-42ed-8dcf-cd7ebf0e9f7e

wandergithub · 2024-10-10T23:45:55Z

Ok, I will address it here in this PR.

wandergithub · 2024-10-14T19:20:33Z

I'm blocked, I cant make the factory for @person1 = create(:adopter, email: "adopter1@alta.com") to persist the person.
The person with the email "adoper1@..." is not being created. I've used :with_person to make changes to the factory but when debugging in the csv_service part of the code the person does not exist for some reason 🤯.

kasugaijin · 2024-10-14T19:46:44Z

I'm blocked, I cant make the factory for @person1 = create(:adopter, email: "adopter1@alta.com") to persist the person. The person with the email "adoper1@..." is not being created. I've used :with_person to make changes to the factory but when debugging in the csv_service part of the code the person does not exist for some reason 🤯.

Can you please push your latest changes up to this branch? I'd like to see the full picture before looking at why

kasugaijin · 2024-10-14T22:32:18Z

@wandergithub
I think I found the issues - see the video
I'd suggest refactoring the CSV import logic first, too.

I think you can ignore my comment about changing the email in the setup and the csv...that should not be an issue with the updates I suggest.

https://www.loom.com/share/d462da8a6a534cd0b75b831fcc593786?sid=4cf176c6-89f8-408d-8aa1-289c46fb3c7f

wandergithub · 2024-10-14T22:48:49Z

@kasugaijin Thank you! I really needed that. I will implement the new service logic first to get it out of the way, and then check out everything else.

kasugaijin · 2024-10-14T23:55:29Z

@kasugaijin Thank you! I really needed that. I will implement the new service logic first to get it out of the way, and then check out everything else.

I am glad it helped!

kasugaijin · 2024-10-15T00:01:05Z

I am just leaving a note for myself here - we should make sure we fully test the refactor of the CSV import service i.e.

it skips the row if the user exists and the timestamp matches that on the FormSubmisson
it updates the existing form submission if it is 'empty' i.e. does not have any answers (no timestamp saved)
it creates a new form submission and adds the form answers if there is no 'empty' form submission and the timestamp is different

@wandergithub You could add the above tests to this PR...but it has already changed a lot for you, so it might be better to not change the scope of this PR any further, and then tackle any of the missing above tests separately to keep things manageable (if you prefer).

- Updates users Factorie to create the person - fix external_form_upload_controller_tests

wandergithub · 2024-10-16T19:07:33Z

I am just leaving a note for myself here - we should make sure we fully test the refactor of the CSV import service i.e.

it skips the row if the user exists and the timestamp matches that on the FormSubmisson

it updates the existing form submission if it is 'empty' i.e. does not have any answers (no timestamp saved)

it creates a new form submission and adds the form answers if there is no 'empty' form submission and the timestamp is different

@wandergithub You could add the above tests to this PR...but it has already changed a lot for you, so it might be better to not change the scope of this PR any further, and then tackle any of the missing above tests separately to keep things manageable (if you prefer).

I added the last two since the first test already exists as the first one in csv_import_service_tests.rb. I think I addressed all concerns . I will be attentive to any comments.

app/services/organizations/importers/google_csv_import_service.rb

kasugaijin

Looking really good @wandergithub! Just one comment to DRY up the service object refactor.

test/controllers/organizations/staff/external_form_upload_controller_test.rb

kasugaijin

Approved but please change the variable names in the tests per my suggestion

kasugaijin · 2024-10-17T00:37:17Z

test/services/organizations/csv_import_service_test.rb

@@ -4,14 +4,14 @@
 module Organizations
  class CsvImportServiceTest < ActiveSupport::TestCase
    setup do
-      person = create(:person)
+      person = create(:adopter)


Same here re variable naming

test/controllers/organizations/staff/external_form_upload_controller_test.rb

kasugaijin

Nice!

wandergithub · 2024-10-17T00:49:28Z

I think that would do it 👍🏽

wandergithub added 4 commits October 6, 2024 13:39

adds route and controller method to use the importer to import extern…

27a9557

…al csv forms with google_csv_import_service

Merge branch 'main' into 1033-external-form-upload

0a3b161

adds test for external_form_upload#create

93f379b

Solve linter and test issues

e8c7720

kasugaijin reviewed Oct 6, 2024

View reviewed changes

app/controllers/organizations/staff/external_form_upload_controller.rb Outdated Show resolved Hide resolved

Adds multiple param to partial

06737d6

Modify external form controller to support just one file upload correct controller test

wandergithub requested a review from kasugaijin October 8, 2024 02:15

kasugaijin reviewed Oct 10, 2024

View reviewed changes

test/controllers/organizations/staff/external_form_upload_controller_test.rb Outdated Show resolved Hide resolved

Add new test and change to nested modules

66463fa

Merge branch 'main' into 1033-external-form-upload

53c25a1

add latest_form_submission

b2b3765

wandergithub added 2 commits October 16, 2024 13:35

- refactors google_csv_import_service

ad286e4

- Updates users Factorie to create the person - fix external_form_upload_controller_tests

Add csv_import_service_tests tests

a8d55bd

kasugaijin reviewed Oct 16, 2024

View reviewed changes

app/services/organizations/importers/google_csv_import_service.rb Outdated Show resolved Hide resolved

kasugaijin reviewed Oct 16, 2024

View reviewed changes

Apply Dry principle adding method

6db6688

kasugaijin reviewed Oct 17, 2024

View reviewed changes

test/controllers/organizations/staff/external_form_upload_controller_test.rb Outdated Show resolved Hide resolved

kasugaijin approved these changes Oct 17, 2024

View reviewed changes

kasugaijin reviewed Oct 17, 2024

View reviewed changes

test/controllers/organizations/staff/external_form_upload_controller_test.rb Outdated Show resolved Hide resolved

Improve naming

bf08e81

kasugaijin approved these changes Oct 17, 2024

View reviewed changes

admin variable

56366fe

kasugaijin merged commit c4a3c32 into rubyforgood:main Oct 17, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1033 external form upload #1038

1033 external form upload #1038

wandergithub commented Oct 6, 2024

wandergithub commented Oct 6, 2024

kasugaijin Oct 6, 2024

wandergithub Oct 7, 2024

kasugaijin Oct 9, 2024

kasugaijin Oct 9, 2024

wandergithub Oct 9, 2024

kasugaijin Oct 10, 2024

wandergithub commented Oct 10, 2024

kasugaijin commented Oct 10, 2024 •

edited

Loading

wandergithub commented Oct 10, 2024

wandergithub commented Oct 14, 2024

kasugaijin commented Oct 14, 2024

kasugaijin commented Oct 14, 2024 •

edited

Loading

wandergithub commented Oct 14, 2024

kasugaijin commented Oct 14, 2024

kasugaijin commented Oct 15, 2024

wandergithub commented Oct 16, 2024

kasugaijin left a comment

kasugaijin left a comment

kasugaijin Oct 17, 2024

kasugaijin left a comment

wandergithub commented Oct 17, 2024

		@@ -0,0 +1,21 @@
		require "test_helper"

		class Organizations::Staff::ExternalFormUploadControllerTest < ActionDispatch::IntegrationTest

1033 external form upload #1038

1033 external form upload #1038

Conversation

wandergithub commented Oct 6, 2024

🔗 Issue

✍️ Description

📷 Screenshots/Demos

wandergithub commented Oct 6, 2024

kasugaijin Oct 6, 2024

Choose a reason for hiding this comment

wandergithub Oct 7, 2024

Choose a reason for hiding this comment

kasugaijin Oct 9, 2024

Choose a reason for hiding this comment

kasugaijin Oct 9, 2024

Choose a reason for hiding this comment

wandergithub Oct 9, 2024

Choose a reason for hiding this comment

kasugaijin Oct 10, 2024

Choose a reason for hiding this comment

wandergithub commented Oct 10, 2024

kasugaijin commented Oct 10, 2024 • edited Loading

wandergithub commented Oct 10, 2024

wandergithub commented Oct 14, 2024

kasugaijin commented Oct 14, 2024

kasugaijin commented Oct 14, 2024 • edited Loading

wandergithub commented Oct 14, 2024

kasugaijin commented Oct 14, 2024

kasugaijin commented Oct 15, 2024

wandergithub commented Oct 16, 2024

kasugaijin left a comment

Choose a reason for hiding this comment

kasugaijin left a comment

Choose a reason for hiding this comment

kasugaijin Oct 17, 2024

Choose a reason for hiding this comment

kasugaijin left a comment

Choose a reason for hiding this comment

wandergithub commented Oct 17, 2024

kasugaijin commented Oct 10, 2024 •

edited

Loading

kasugaijin commented Oct 14, 2024 •

edited

Loading