rubyforgood / pet-rescue

Pet Rescue is an application making it easy to link adopters/fosters with pets. We work with grassroots pet rescue organizations to understand how we can make the most impact.
MIT License
57 stars 95 forks source link

851 create form submission model #856

Closed Gabe-Torres closed 1 month ago

Gabe-Torres commented 1 month ago

🔗 Issue

(https://github.com/rubyforgood/pet-rescue/issues/851)

✍️ Description

Create form_submission model

📷 Screenshots/Demos

kasugaijin commented 1 month ago

@Gabe-Torres This is looking good so far! Like you said, the pipeline is not passing because the seeds are not loading due to an error - my immediate guess is that AdopterApplication and SubmittedAnswer do not have a FormSubmission to belong to. So it will be a matter of creating that object and passing it to its associations in both Baja and Alta seeds files. I usually like to just focus on one seed file first and get that working, then it's more or less a copy paste into the other file as they should be almost identical.

To your questions about adding in the additional components to get the tests to pass - ideally, yes. PRs should be complete and always pass the pipeline. This will likely mean updating the seeds to make sure associated objects exist, and to update any test setups that also need the object to exist. Now, if this is starting to look like a huge chunk of work, we can always help you out! Let me know what you think! Ask as many questions as you need.

I usually try to break things up more into smaller chunks. But, given this is addition of a model, we need to add it and make sure it's working as expected. So, it's a bit more work...but should not be too much more as it's just two associated models. Famous last words.

Gabe-Torres commented 1 month ago

context "associations" do

@Gabe-Torres This is looking good so far! Like you said, the pipeline is not passing because the seeds are not loading due to an error - my immediate guess is that AdopterApplication and SubmittedAnswer do not have a FormSubmission to belong to. So it will be a matter of creating that object and passing it to its associations in both Baja and Alta seeds files. I usually like to just focus on one seed file first and get that working, then it's more or less a copy paste into the other file as they should be almost identical.

To your questions about adding in the additional components to get the tests to pass - ideally, yes. PRs should be complete and always pass the pipeline. This will likely mean updating the seeds to make sure associated objects exist, and to update any test setups that also need the object to exist. Now, if this is starting to look like a huge chunk of work, we can always help you out! Let me know what you think! Ask as many questions as you need.

I usually try to break things up more into smaller chunks. But, given this is addition of a model, we need to add it and make sure it's working as expected. So, it's a bit more work...but should not be too much more as it's just two associated models. Famous last words.

Thank you for the feedback @kasugaijin ! I can get sidetracked sometimes, so it's good to know I was on the right track. I updated tests where needed and made some tweaks to the adopter_applications factory to account for the new association with form_submissions.

Regarding seeds – How are we planning to handle a form_submission instance? In my current version, I created one form_submission object that has an association with the organization at the top of the file, along with a singular person. The seeds create 10 adopter applications. Do you think it would make sense to also create 10 form_submissions, or would that suffice for our needs?

kasugaijin commented 1 month ago

context "associations" do

@Gabe-Torres This is looking good so far! Like you said, the pipeline is not passing because the seeds are not loading due to an error - my immediate guess is that AdopterApplication and SubmittedAnswer do not have a FormSubmission to belong to. So it will be a matter of creating that object and passing it to its associations in both Baja and Alta seeds files. I usually like to just focus on one seed file first and get that working, then it's more or less a copy paste into the other file as they should be almost identical. To your questions about adding in the additional components to get the tests to pass - ideally, yes. PRs should be complete and always pass the pipeline. This will likely mean updating the seeds to make sure associated objects exist, and to update any test setups that also need the object to exist. Now, if this is starting to look like a huge chunk of work, we can always help you out! Let me know what you think! Ask as many questions as you need. I usually try to break things up more into smaller chunks. But, given this is addition of a model, we need to add it and make sure it's working as expected. So, it's a bit more work...but should not be too much more as it's just two associated models. Famous last words.

Thank you for the feedback @kasugaijin ! I can get sidetracked sometimes, so it's good to know I was on the right track. I updated tests where needed and made some tweaks to the adopter_applications factory to account for the new association with form_submissions.

Regarding seeds – How are we planning to handle a form_submission instance? In my current version, I created one form_submission object that has an association with the organization at the top of the file, along with a singular person. The seeds create 10 adopter applications. Do you think it would make sense to also create 10 form_submissions, or would that suffice for our needs?

So the benefit of the FormSubmission is that we should be able to associated multiple adopter applications with a single FormSubmission, so let's go with that in the seeds for now.

kasugaijin commented 1 month ago

@Gabe-Torres Looking great! Just a few comments.

Gabe-Torres commented 1 month ago

Awesome! I changed the belongs_to association to acts_as_tenant. Thank you @kasugaijin. I think everything is updated and ready to go!

kasugaijin commented 1 month ago

@Gabe-Torres approved, but please add the comments to the class per my comment above.

Gabe-Torres commented 1 month ago

Added! Apologies, I had added the comments before merging the most recent main and must of not added them while sorting conflicts.