-
Notifications
You must be signed in to change notification settings - Fork 121
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
Form submission refactor #1244
Changes from 15 commits
15dafa1
2c8d684
08d5ca1
b5bc2dd
263a92f
1c21e17
3b5d6fb
a5951d8
704fa5c
6c100df
285ca2c
efcaf6b
efb41ff
e8af96c
efc7508
0fa6693
022d69e
9bade6c
3ef2127
8806b4c
85b50fc
ba38ff4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
module Organizations | ||
module Staff | ||
class FormAnswersController < Organizations::BaseController | ||
before_action :context_authorize!, only: %i[index] | ||
before_action :set_form_submission | ||
|
||
def index | ||
@form_answers = @form_submission.form_answers.order(created_at: :desc) | ||
end | ||
|
||
private | ||
|
||
def set_form_submission | ||
@form_submission = FormSubmission.find_by(id: params[:form_submission_id]) | ||
end | ||
|
||
def context_authorize! | ||
authorize! FormAnswer, | ||
context: {organization: Current.organization}, | ||
with: Organizations::FormSubmissionPolicy | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
module Organizations | ||
module Staff | ||
class FormSubmissionsController < Organizations::BaseController | ||
layout "dashboard" | ||
|
||
before_action :context_authorize!, only: %i[index] | ||
before_action :set_person | ||
|
||
def index | ||
@form_submissions = @person.form_submissions.order(created_at: :desc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We will have to deal with form submissions without the csv timestamp(custom forms) as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that makes sense! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good! We can always add a user sort-able feature in the future. FYI this isn't a blocker for me, I just haven't had a chance to fully review the whole pr yet. My first thought on validating the csv_timestamp presence is that it makes sense. After thinking through the import more I'm not sure. I'm trying not to get too into the weeds on what people might do to their csv date, but as of right now the the import will work just fine if the timestamp column entry is nil (the header has to be present). So for example, the row could have valid data and a matching email but no timestamp ( maybe the row was added manually by someone?) I don't think I'd want to skip that row. Maybe a simpler approach is to conditionally display the timestamp for each submission in the view. That resolves the current issue as well as when the custom form is live. Edit: I would need to tweak the import service a tiny bit, Time.parse returns an error when nil. Also just saw the the validation that you added. No problem having it. I will update the csv_import. |
||
end | ||
|
||
private | ||
|
||
def set_person | ||
@person = Person.find_by(id: params[:person_id]) | ||
end | ||
|
||
def context_authorize! | ||
authorize! FormSubmission, | ||
context: {organization: Current.organization} | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
module Organizations | ||
class FormSubmissionPolicy < ApplicationPolicy | ||
pre_check :verify_organization! | ||
pre_check :verify_active_staff! | ||
|
||
alias_rule :index?, to: :manage? | ||
|
||
def manage? | ||
permission?(:view_form_submissions) | ||
end | ||
end | ||
end |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<%= turbo_frame_tag :form_answers do %> | ||
<h4>Form Answers (<%= @form_submission.created_at.strftime("%Y-%m-%d") %>)</h4> | ||
|
||
<% @form_answers.each do |form_answer| %> | ||
<div class="justify-content-md-between mb-2 gx-3"> | ||
<div class="card"> | ||
<div id="<%= dom_id form_answer %>"class="card-body d-flex flex-sm-row flex-column justify-content-between border-bottom"> | ||
<div class="d-flex align-items-center"> | ||
<div> | ||
<strong class="fs-4" >Q: <%= form_answer.question_snapshot %></strong> | ||
<p class="mb-0">A: <%= form_answer.value %> </p> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
<% end %> | ||
<% end %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
<div class="card"> | ||
<!-- table --> | ||
<div class="table-responsive overflow-y-hidden"> | ||
<table class="table mb-2 text-nowrap table-hover table-centered"> | ||
<thead> | ||
<tr> | ||
<th scope="col">Submitted</th> | ||
<th scope="col">Action</th> | ||
</tr> | ||
</thead> | ||
|
||
<tbody> | ||
<% form_submissions.each do |form_submission| %> | ||
<tr> | ||
<td> | ||
<p class="mb-0"><%= form_submission.created_at.strftime("%Y-%m-%d") %></p> | ||
</td> | ||
<td> | ||
<%= link_to "View Submission", staff_form_submission_form_answers_path(form_submission), data: { turbo_frame: "form_answers" } %> | ||
</td> | ||
</tr> | ||
<% end %> | ||
</tbody> | ||
</table> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
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.