Revert "Set status directly instead of in form data"

apps/src/code-studio/pd/form_components_func/FormController.jsx

@@ -294,13 +294,12 @@ const FormController = props => {
    *
    * @returns {Object}
    */
-  const serializeFormData = (formData, status) => {
+  const serializeFormData = formData => {
     if (!formData) {
       throw new Error(`formData cannot be undefined`);
     }
     return {
       form_data: formData,
-      status: status,
       ...serializeAdditionalData()
     };
   };
@@ -319,13 +318,16 @@ const FormController = props => {
   };
 
   const makeRequest = applicationStatus => {
+    const dataWithStatus = {...data, status: applicationStatus};
+    setData(dataWithStatus);
+
     const ajaxRequest = (method, endpoint) =>
       $.ajax({
         method: method,
         url: endpoint,
         contentType: 'application/json',
         dataType: 'json',
-        data: JSON.stringify(serializeFormData(data, applicationStatus))
+        data: JSON.stringify(serializeFormData(dataWithStatus))
       });
 
     return updatedApplicationId

apps/test/unit/code-studio/pd/form_components_func/FormControllerTest.js

@@ -180,7 +180,6 @@ describe('FormController', () => {
     });
 
     describe('Saving', () => {
-      // [MEG] TODO: Refactor this to use savedStatus instead of form_data
       it('Overrides application status as incomplete to data', () => {
         const testData = {
           field1: 'value 1',
@@ -193,7 +192,7 @@ describe('FormController', () => {
         );
         form.findAll('Button')[1].props.onClick();
 
-        expect(getData(DummyPage1)).to.eql({...testData});
+        expect(getData(DummyPage1)).to.eql({...testData, status: 'incomplete'});
       });
 
       it('Disables the save button during save', () => {
@@ -376,7 +375,6 @@ describe('FormController', () => {
           expect(form.findOne('#submit').props.disabled).to.be.false;
         });
 
-        // [MEG] TODO: Refactor this to use savedStatus instead of form_data
         it('Overrides application status as unreviewed on submit', () => {
           const testData = {
             field1: 'value 1',
@@ -391,7 +389,8 @@ describe('FormController', () => {
           triggerSubmit();
 
           expect(getData(DummyPage3)).to.eql({
-            ...testData
+            ...testData,
+            status: 'unreviewed'
           });
         });
 

dashboard/app/controllers/api/v1/pd/application/teacher_applications_controller.rb

@@ -13,16 +13,10 @@ def new_form
 
     # PATCH /api/v1/pd/application/teacher/<applicationId>
     def update
-      form_data_hash = params.try(:[], :form_data)
-      if form_data_hash
-        form_data_json = form_data_hash.to_unsafe_h.to_json.strip_utf8mb4
-        @application.form_data_hash = JSON.parse(form_data_json)
-      end
+      form_data_hash = params.try(:[], :form_data) || {}
+      form_data_json = form_data_hash.to_unsafe_h.to_json.strip_utf8mb4
 
-      status = params.try(:[], :status)
-      if status
-        @application.status = status
-      end
+      @application.form_data_hash = JSON.parse(form_data_json)
 
       if @application.save
         render json: @application, status: :ok

dashboard/app/controllers/api/v1/pd/forms_controller.rb

@@ -4,17 +4,12 @@ def new_form
   end
 
   def create
-    form = new_form
+    form_data_hash = params.try(:[], :form_data) || {}
+    form_data_json = form_data_hash.to_unsafe_h.to_json.strip_utf8mb4
 
-    form_data_hash = params.try(:[], :form_data)
-    form_data_json = form_data_hash ? form_data_hash.to_unsafe_h.to_json.strip_utf8mb4 : {}.to_json
+    form = new_form
     form.form_data_hash = JSON.parse(form_data_json)
 
-    status = params.try(:[], :status)
-    if status
-      @application.status = status
-    end
-
     # Check for idempotence
     existing_form = form.check_idempotency
     return render json: {id: existing_form.id}, status: :ok if existing_form

dashboard/app/controllers/pd/application/teacher_application_controller.rb

@@ -24,7 +24,6 @@ def new
       if @application
         return render :submitted if @application.status != 'incomplete'
         @application_id = @application.try(:id)
-        @saved_status = @application.try(:status)
         @form_data = @application.try(:form_data)
       end
 
@@ -37,7 +36,6 @@ def new
           userId: current_user.id,
           schoolId: current_user.school_info&.school&.id,
           applicationId: @application_id,
-          savedStatus: @saved_status,
           savedFormData: @form_data
         }.to_json
       }

dashboard/app/models/concerns/pd/form.rb

@@ -69,9 +69,8 @@ def validate_required_fields
 
     hash = sanitize_and_trim_form_data_hash
 
-    # 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'
+    # No validation is required if the application is still in progress
+    return if hash && hash[:status] == 'incomplete'
 
     self.class.required_fields.each do |key|
       add_key_error(key) unless hash.key?(key)

dashboard/app/models/pd/application/application_base.rb

@@ -56,7 +56,10 @@ class ApplicationBase < ApplicationRecord
 
     after_initialize :set_type_and_year
 
-    before_validation -> {self.status = 'unreviewed' unless status}
+    # Set the status only if (a) the application is new, or (b) the applicant updates form_data.
+    # Avoid setting the status when an RP or admin changes the application status, which only happens
+    # from the application dashboard, not from changing form_data.
+    before_validation :set_status
     validate :status_is_valid_for_application_type
     validates_presence_of :type
     validates_presence_of :user_id, unless: proc {|application| application.application_type == PRINCIPAL_APPROVAL_APPLICATION}
@@ -67,6 +70,7 @@ class ApplicationBase < ApplicationRecord
     # An application either has an "incomplete" or "unreviewed" state when created.
     # After creation, an RP or admin can change the status to "accepted," which triggers update_accepted_data.
     before_save :update_accepted_date, if: :status_changed?
+    before_save :remove_status_from_form_data
 
     before_create :generate_application_guid, if: -> {application_guid.blank?}
     after_destroy :delete_unsent_email
@@ -85,6 +89,17 @@ class ApplicationBase < ApplicationRecord
       csa
     ).index_by(&:to_sym).freeze
 
+    def set_status
+      return unless new_record? || (sanitize_form_data_hash && sanitize_form_data_hash[:status])
+      self.status = 'unreviewed' if new_record?
+      self.status = sanitize_form_data_hash[:status] if sanitize_form_data_hash && sanitize_form_data_hash[:status]
+    end
+
+    def remove_status_from_form_data
+      return unless form_data_hash && form_data_hash["status"]
+      self.form_data = form_data_hash.except("status").to_json
+    end
+
     def set_type_and_year
       # Override in derived classes and set to valid values.
       # Setting them to nil here fails those validations and prevents this base class from being saved.

dashboard/test/controllers/api/v1/pd/application/teacher_applications_controller_test.rb

@@ -123,7 +123,7 @@ class TeacherApplicationsControllerTest < ::ActionController::TestCase
 
     test 'can submit an empty form if application is incomplete' do
       sign_in @applicant
-      put :create, params: {status: 'incomplete'}
+      put :create, params: {form_data: {status: 'incomplete'}}
 
       assert_equal 'incomplete', TEACHER_APPLICATION_CLASS.last.status
       assert_response :created
@@ -134,7 +134,7 @@ class TeacherApplicationsControllerTest < ::ActionController::TestCase
       Pd::Application::TeacherApplication.expects(:queue_email).never
 
       sign_in @applicant
-      put :create, params: {status: 'incomplete'}
+      put :create, params: {form_data: {status: 'incomplete'}}
       assert_response :created
     end
 
@@ -144,8 +144,7 @@ class TeacherApplicationsControllerTest < ::ActionController::TestCase
       original_data = application.form_data_hash
       original_school_info = @applicant.school_info
 
-      # Keep cs_total_course_hours because it is calculated on create or update
-      put :update, params: {id: application.id, status: 'incomplete', form_data: {"cs_total_course_hours": 80}}
+      put :update, params: {id: application.id, form_data: {status: 'incomplete'}}
       application.reload
       refute_equal original_data, application.form_data_hash
       assert_nil application.course

dashboard/test/controllers/pd/application/teacher_application_controller_test.rb

@@ -34,8 +34,8 @@ class TeacherApplicationControllerTest < ::ActionController::TestCase
 
     test 'teachers with an incomplete application have an application id and saved form data' do
       application = create :pd_teacher_application, form_data_hash: (
-        build :pd_teacher_application_hash
-      ), status: 'incomplete'
+        build :pd_teacher_application_hash, :incomplete
+      )
       sign_in application.user
       get :new
       assert_response :success
@@ -44,7 +44,6 @@ class TeacherApplicationControllerTest < ::ActionController::TestCase
       @script_data = assigns(:script_data)
       assert_equal application.id, JSON.parse(@script_data.dig(:props)).dig('applicationId')
       assert_equal application.form_data, JSON.parse(@script_data.dig(:props)).dig('savedFormData')
-      assert_equal application.status, JSON.parse(@script_data.dig(:props)).dig('savedStatus')
     end
 
     test 'teachers without an application have no id nor form data' do

dashboard/test/factories/pd_factories.rb

@@ -809,6 +809,10 @@
       school_type 'Public school'
     end
 
+    trait :incomplete do
+      status 'incomplete'
+    end
+
     trait :with_no_school do
       school(-1)
     end

dashboard/test/models/concerns/pd/form_test.rb

@@ -91,6 +91,14 @@ class Pd::FormTest < ActiveSupport::TestCase
     refute form.valid?
   end
 
+  test 'pd form does not check required fields if status is incomplete' do
+    expects(:dynamic_required_fields).never
+    form = DummyFormWithRequiredFields.new
+    form.form_data = {status: "incomplete"}.to_json
+
+    assert form.valid?
+  end
+
   test 'pd form enforces required fields' do
     form = DummyFormWithRequiredFields.new
 

dashboard/test/models/pd/application/application_base_test.rb

@@ -36,15 +36,22 @@ class ApplicationBaseTest < ActiveSupport::TestCase
       assert application.unreviewed?
     end
 
-    test 'can set a different status from the default' do
-      application = create TEACHER_APPLICATION_FACTORY, status: 'incomplete'
+    test 'form data overwrites default status' do
+      application = create TEACHER_APPLICATION_FACTORY, form_data: {status: 'incomplete'}.to_json
       application.update_status_timestamp_change_log(nil)
 
       assert application.incomplete?
       assert_equal application.sanitize_status_timestamp_change_log.length, 1
       assert application.sanitize_status_timestamp_change_log[0].value?('incomplete')
     end
 
+    test 'status is removed from form_data after status is set' do
+      application = create TEACHER_APPLICATION_FACTORY, form_data: {status: 'incomplete'}.to_json
+
+      assert application.incomplete?
+      assert_nil application.form_data_hash['status']
+    end
+
     test 'can update status' do
       application = create FACILITATOR_APPLICATION_FACTORY
       assert application.unreviewed?

dashboard/test/models/pd/application/teacher_application_test.rb

@@ -55,8 +55,8 @@ class TeacherApplicationTest < ActiveSupport::TestCase
       }
 
       application = create :pd_teacher_application, form_data_hash: (
-        build :pd_teacher_application_hash, custom_minutes_hours_weeks
-      ), status: 'incomplete'
+        build :pd_teacher_application_hash, :incomplete, custom_minutes_hours_weeks
+      )
 
       assert_equal 112, application.sanitize_form_data_hash[:cs_total_course_hours]
     end
@@ -70,8 +70,8 @@ class TeacherApplicationTest < ActiveSupport::TestCase
 
       %i(cs_how_many_minutes cs_how_many_days_per_week cs_how_many_weeks_per_year).each do |attribute|
         application = create :pd_teacher_application, form_data_hash: (
-          build :pd_teacher_application_hash, empty_minutes_hours_weeks.slice(attribute)
-        ), status: 'incomplete'
+          build :pd_teacher_application_hash, :incomplete, empty_minutes_hours_weeks.slice(attribute)
+        )
 
         assert_nil application.sanitize_form_data_hash[:cs_total_course_hours]
       end