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

Form submission refactor #1244

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Form submission refactor #1244

wants to merge 22 commits into from

Conversation

kasugaijin
Copy link
Collaborator

@kasugaijin kasugaijin commented Dec 11, 2024

🔗 Issue

Issue: #1232

  • changes associations so that adopter application belongs to person instead of form submission
  • removes broadcast on adopter applications
  • Adds a new form submission route scoped to the person to display their form submissions. User can click a given form submission to get its associated forms answers via turbo frame, from a new form submission scope route.
  • Adds policy class for these two resources
  • updates tests to handle new associations

We could consider pagination. I was thinking it would be useful to paginate the form submissions so we only ever have 3-5 rows in that table. But, I think it's better UI not to paginate form answers because if you want to go back to a given question/answer while reviewing details it will be annoying to do it with page links. I think it's easier to just scroll. Open to other thoughts on this, though. I suggest adding pagination in another issue.

formstuff

Empty State
image

✍️ Description

📷 Screenshots/Demos

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an endpoint we hit with turbo frame to load the form answers. I figured the policy is exactly the same as FormSubmission, so re-used that policy instead of creating a separate policy.

@kasugaijin kasugaijin marked this pull request as ready for review December 13, 2024 18:39
before_action :set_person

def index
@form_submissions = @person.form_submissions.order(created_at: :desc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

created_at will be different than csv_timestamp. Do we want to use the csv timestamp?

We will have to deal with form submissions without the csv timestamp(custom forms) as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that makes sense!

Copy link
Collaborator Author

@kasugaijin kasugaijin Dec 13, 2024

Choose a reason for hiding this comment

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

Second thoughts, I suppose it depends on how we want to define the FormSubmission. Is it a capture of when they filled out the Google Form, or when the staff imported the data? I think it would be more 'accurate' if I change the column name from Submitted to Imported, then use the created_at date. This will remove any ambiguity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how valuable the date of import is. For example, when a new org does their first import they may have multiple submissions from one person on separate dates. The import date would be the same day for all submissions.

When our custom forms is fully functional, then yes the date the form submission is created would make sense because that is when the form is submitted.

Copy link
Collaborator Author

@kasugaijin kasugaijin Dec 14, 2024

Choose a reason for hiding this comment

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

Ok if we go with the csv_timestamp, we have to assume it will always be available on FormSubmission, at least until we start using custom forms. We do not validate this attribute on the model or database. I think that now we create the FormSubmission in the CSV Import service, we should at least have a validation for csv_timestamp presence. When we start to use the custom forms, we can update this validation to only run on CSV imports. Thoughts?

Copy link
Collaborator Author

@kasugaijin kasugaijin Dec 14, 2024

Choose a reason for hiding this comment

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

I added a presence validation on FormSubmission.

I also updated to show both the csv_timestamp and the import date just so it's clear what each date is for. I also decided to show the HHMM for the csv_timestamp in case someone submits twice in one day - then the staff can tell which set of answers they are checking.

image

…bmitted at in case someone submits a form twice in one day, then it is easiser to determine which answers you are looking at
@kasugaijin kasugaijin requested a review from jmilljr24 December 14, 2024 22:23
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