Add occurred at to incidents #292
Conversation
| @@ -166,6 +166,7 @@ def update | |||
| def incident_params | |||
| data = params.require(:incident).permit! | |||
| sup_report_attrs = data[:supervisor_incident_report_attributes] | |||
| @incident.driver_incident_report = IncidentReport.create( user_id: current_user.id ) if current_user.driver? | |||
There was a problem hiding this comment.
I feel uncomfortable creating an object in the params. If not for any reason other than the question: What if it doesn't work? There's no error handling here, that's done (as is the "Rails Way") in the create action.
There are a couple standard or at least more future-proof ways of handling the problem of "create a driver incident report also if the current user is a driver".
Since Incident accepts_nested_attributes_for IncidentReport, you could just handle this in the same way the supervisor incident report is, with incident_driver_incident_report_attributes. See the way sup_report_attrs is created and used.
What you want to end up with is data that can be used to create an object, not some data and also an already-created object.
| @@ -29,9 +29,6 @@ | |||
| %p | |||
| %strong= human_name :incident, :bus | |||
| = report.bus | |||
| %p | |||
| %strong= human_name :incident_report, :occurred_at | |||
| = report.occurred_at_readable | |||
There was a problem hiding this comment.
Would people still expect to see this here? Do we want to still display occurred at for the incident the report is associated with or..? This is a business logic question, I really don't know.
| Incident.all.each do |incident| | ||
| if incident.driver_incident_report? | ||
| occurred_at = incident.driver_incident_report.occurred_at | ||
| incident.update(occurred_at: occurred_at) |
There was a problem hiding this comment.
With a task that updates every object in the database like this, there's a (slight) concern for me that for some reason past incidents won't be valid for other reasons. This has happened before. So updating it won't work in that case. Do we want to skip validations here?
I moved the field occurred_at from incident_report to incidents. The driver now has has to input a date and time before being redirected to filling out their incident_report. This is part one of two PRs to close #288.
