Set status directly instead of in form data #44874
Conversation
| @@ -180,6 +180,7 @@ describe('FormController', () => { | |||
| }); | |||
|
|
|||
| describe('Saving', () => { | |||
| // [MEG] TODO: Refactor this to use savedStatus instead of form_data | |||
There was a problem hiding this comment.
I'm leaving this as a to-do because soon, we'll need to have a 'reopened' status if an RP reopens an application. We should check savedStatus for both of these cases.
| @@ -36,6 +37,7 @@ def new | |||
| userId: current_user.id, | |||
| schoolId: current_user.school_info&.school&.id, | |||
| applicationId: @application_id, | |||
| savedStatus: @saved_status, | |||
There was a problem hiding this comment.
This will be needed once an RP can reopen an application
| @@ -91,14 +91,6 @@ class Pd::FormTest < ActiveSupport::TestCase | |||
| refute form.valid? | |||
| end | |||
|
|
|||
| test 'pd form does not check required fields if status is incomplete' do | |||
There was a problem hiding this comment.
This is the most awkward part of using this approach ... to test, we'd need to create an application with a status, rather than keep it as a form because a form doesn't have a status. Similar tests already exist in TeacherApplicationTest, so rather than duplicate here, I didn't include a test.
| return if hash && hash[:status] == 'incomplete' | ||
| # No validation is required if an application is still in progress | ||
| # Check that status is defined because not all forms have status | ||
| return if defined?(status) && status == 'incomplete' |
There was a problem hiding this comment.
See form_test for more discussion of this
| form.form_data_hash = JSON.parse(form_data_json) | ||
|
|
||
| status = params.try(:[], :status) | ||
| if status |
There was a problem hiding this comment.
optional / for next time: I am not sure why the existing code is using params.try here, because params should always be defined in a controller method. params[:status] would be more clear.
There was a problem hiding this comment.
Ahh, good to know! I'll make a note of it and include in a future PR. Thanks.
To save teacher applications, I was originally passing in the status via the form_data: if the user hits "submit application," {"status": "unreviewed"} would be added to the form data, and if the user hits "save application," {"status": "incomplete"} would be added to the form data.
This presented some challenges:
Here, the status is set through a separate param in the frontend. Now we don't need to worry about removing form data. It seems cleaner to me, but I'm curious to hear y'all's thoughts!
See recording here: https://watch.screencastify.com/v/hA8VKZ573lGMF6I5oAou
Links
Testing story
Deployment strategy
Follow-up work
Need to add an eyes test for the application_dashboard. Will do this after (a) the "reopened" application status is set, and (b) RP's can't modify form data while an application is "incomplete" (maybe "reopened" too––not sure)
Privacy
Security
Caching
PR Checklist: