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

851 create form submission model #856

Merged

Conversation

Gabe-Torres
Copy link
Contributor

🔗 Issue

(#851)

✍️ Description

Create form_submission model

  • create test for form_submission model
  • create the foreign key for has many relationship between form_submission and submitted_answer
  • create the foreign key for has many relationship between form_submission and adopter_application
  • update submitted_answer model test to include new association with form_submission
  • update adopter_applicaiton model test to include new association with form_submission
    • Added an associations context block
    • this also involved creating a form_submission factory that can be called within tests

📷 Screenshots/Demos

… and adopter applications

- create form_submission factory
- add associations context block in adopter_application test
@Gabe-Torres Gabe-Torres marked this pull request as draft June 28, 2024 23:39
@kasugaijin
Copy link
Collaborator

kasugaijin commented Jun 30, 2024

@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.

@application = create(:adopter_application, form_submission: @form_submission)
end

context "associations" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I guess this was missing before! Thanks for adding.

@Gabe-Torres Gabe-Torres marked this pull request as ready for review July 2, 2024 04:03
@Gabe-Torres
Copy link
Contributor Author

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?

@Gabe-Torres Gabe-Torres requested a review from kasugaijin July 2, 2024 04:19
@kasugaijin
Copy link
Collaborator

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.

#
class FormSubmission < ApplicationRecord
belongs_to :person
belongs_to :organization
Copy link
Collaborator

@kasugaijin kasugaijin Jul 7, 2024

Choose a reason for hiding this comment

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

Check out other resources that belong to an organization (most of them) and you'll see we use the acts as tenant helper here. This is important for setting the tenancy, or scope, for a given record i.e., which org it belongs to, and is used by the acts as tenant scoping logic elsewhere.

acts_as_tenant(:organization)

@kasugaijin
Copy link
Collaborator

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

@Gabe-Torres
Copy link
Contributor Author

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

@Gabe-Torres Gabe-Torres requested a review from kasugaijin July 9, 2024 17:52
@kasugaijin
Copy link
Collaborator

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

These comments give context to what the FormSubmission class does
@Gabe-Torres
Copy link
Contributor Author

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

@kasugaijin kasugaijin merged commit 9dc380d into rubyforgood:main Jul 10, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants