-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Allows volunteer assignment on case creation #5880
Allows volunteer assignment on case creation #5880
Conversation
@wandergithub Tagging you as you requested |
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.
We should do this as nested attributes. It will provide additional ACID guarantees and the implementation will be simpler.
You are gonna want to:
- allow cases to accept nested attributes for case assignments
- set up nested fields in the views
- permit those parameters
after that ActiveRecord save, update, etc will handle all the creation and updates for you.
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.
Sorry for the long turnaround time. Was out of town.
All done now.
@kenny-luong feel free to pop in for office hours tuesdays https://discord.gg/qJcw2RZH8Q at 8EST if you feel stuck. |
That's good to know, thank you. I'll make the changes as soon as I get back home. 👍 |
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 looks way simpler with the nested attributes.
I also think we could simplify how this is test a bit. I think you could test basically all the functionality in a single system test:
make a new case
select a volunteer during that case creation
check that on the show page for the case that volunteer is assigned
I think that will result in less tests to maintain an it covers the most critical workflow of the whole feature.
|
||
create_params = casa_case_params | ||
create_params = create_params.except(:court_dates_attributes) if court_date_unknown? | ||
create_params = create_params.except(:case_assignments_attributes) unless assigned_volunteer? |
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.
Why do we need to exclude case assignment attributes in this situation? Is some issue created if we don't?
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.
It might be a symptom of another issue but if you don't exclude these attributes (case assignment and court date), and they are left blank on the form, there will be an error.
Though I think this can be fixed by updating the model to do something like this
class CasaCase
# ...
accepts_nested_attributes_for :case_assignments, reject_if: proc { |attributes| attributes['volunteer_id'].blank? }
# ...
end
I grabbed this from here: https://stackoverflow.com/questions/56096562/how-to-do-optional-nested-forms-with-form-for-in-rails-5
app/views/casa_cases/_form.html.erb
Outdated
<%= caf.select :volunteer_id, | ||
@casa_case.unassigned_volunteers.map { |v| [v.display_name, v.id] }, | ||
prompt: "Please select a volunteer" %> |
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.
You could use collection_select
for this
https://apidock.com/rails/v5.2.3/ActionView/Helpers/FormOptionsHelper/collection_select
I've updated one of the existing system tests to account for this. Let me know if that is not sufficient. |
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.
Looks good to me! Nice job! ❤️
create_params = casa_case_params | ||
create_params = create_params.except(:court_dates_attributes) if court_date_unknown? | ||
create_params.except(:empty_court_date) |
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.
create_params = casa_case_params | |
create_params = create_params.except(:court_dates_attributes) if court_date_unknown? | |
create_params.except(:empty_court_date) | |
create_params = casa_case_params.except(:empty_court_date) | |
create_params = create_params.except(:court_dates_attributes) if court_date_unknown? |
🤷♂️
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.
Might have been a bad call on my part since now the tests are failing 🙂
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.
Ah yeah this is just an ordering problem. I'll fix it now. No big deal.
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.
I've reverted the commit.
@@ -48,6 +48,7 @@ class CasaCase < ApplicationRecord | |||
accepts_nested_attributes_for :casa_case_contact_types | |||
accepts_nested_attributes_for :court_dates | |||
accepts_nested_attributes_for :volunteers | |||
accepts_nested_attributes_for :case_assignments, reject_if: proc { |attributes| attributes["volunteer_id"].blank? } |
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 makes more sense to me compared to having the if statement at the controller level.
I am not sure which is better
accepts_nested_attributes_for :case_assignments, reject_if: proc { |attributes| attributes["volunteer_id"].blank? }
# or
accepts_nested_attributes_for :case_assignments, reject_if: :all_blank
fe02ec6
to
4fb4b0c
Compare
What github issue is this PR for, if any?
Resolves #5757
What changed, and why?
Major Changes
/casa_cases/new
Minor Changes
/casa_cases/:id/edit
to "Assign a Volunteer to Case"/casa_cases/:id/edit
to include the caret iconHow is this tested? (please write tests!) 💖💪
Added specs to test the flow and updated existing specs to support this
Manual testing includes:
Screenshots please :)