Use New Status Field to Maintain InProgressForm Data for Dependents Application

[tfink419]

Summary

• In case of failure, InProgressForm is no longer deleted and now a status field is updated and the forms are not shown if the field is set
• Code is split into 3 PR's
  • Add Status Column to InProgressForm #15873
  • Data Migration of InProgressForm Status Column #16094
  • Use New Status Field to Maintain InProgressForm Data for Dependents Application #16097

Related issue(s)

department-of-veterans-affairs/va.gov-team/issues/77171

Testing done

• New code is covered by unit tests
• In progress form would be deleted immediately, now it is saved until submission succeeds or fails the final time
• Submit a dependents change application and check if the form still appears immediately after submission, and that it will reappear after the status is changed back (normally after failures)

Screenshots

Pending Submission:

Processing Submission:

What areas of the site does it impact?

Dependents application submission

Acceptance criteria

• ~ I fixed|updated|added unit tests and integration tests for each feature (if applicable). ~
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog or Grafana (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

[va-vsp-bot]

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file:

[va-vsp-bot]

Error: A file (or its parent directories) was deleted but its reference still exists in CODEOWNERS. Please update the .github/CODEOWNERS file and delete the entry for the Offending file:

[micahaspyr]

👍

[RachalCassity]

@tfink419 Can you confirm the CODEOWNERS file is correct for all of these files? Should the @department-of-veterans-affairs/benefits-dependents-management be added to these other files in CODEOWNERS?

[RachalCassity]

@tfink419 Is this PR still relevant?

[tfink419]

@tfink419 Is this PR still relevant?

@RachalCassity
Yes this PR needs to be approved and merged

[RachalCassity]

I noticed that the in_progress_form shows has_many :form_submissions, dependent: :nullify. Would it be appropriate to add add_foreign_key :form_submissions, :in_progress_forms, column: :in_progress_form_id, on_delete: :nullify.

I also noticed this in SavedClaim and UserAccount. These should be in the schema as well.

[tfink419]

I noticed that the in_progress_form shows has_many :form_submissions, dependent: :nullify. Would it be appropriate to add add_foreign_key :form_submissions, :in_progress_forms, column: :in_progress_form_id, on_delete: :nullify.

I also noticed this in SavedClaim and UserAccount. These should be in the schema as well.

@RachalCassity I do see the foreign key constraints in the schema already. I can add them to the model files

[tfink419]

I noticed that the in_progress_form shows has_many :form_submissions, dependent: :nullify. Would it be appropriate to add add_foreign_key :form_submissions, :in_progress_forms, column: :in_progress_form_id, on_delete: :nullify.
I also noticed this in SavedClaim and UserAccount. These should be in the schema as well.

@RachalCassity I do see the foreign key constraints in the schema already. I can add them to the model files

@RachalCassity I'm not sure anything should be done, form submission has those 3 other models as optional which aligns with nullifying the id.

[RachalCassity]

@tfink419 Thank you for your patience. In the past, I have seen errors with InProgressForms which is why I'm digging through the models and controllers. Here's one sentry issue, here. I noticed during the update, a InProgressForm is initialized here:

    def update
      form = InProgressForm.form_for_user(form_id, @current_user) ||
             InProgressForm.new(form_id:, user_uuid: @current_user.uuid)

Would it make sense to use find_or_initialize_by?

    def update
      form = InProgressForm.form_for_user(form_id, @current_user) ||
             InProgressForm.find_or_intialize_by(form_id:, user_uuid: @current_user.uuid)

I also noticed in the model, the user.account is being used.

  def self.form_for_user(form_id, user)
    user_uuid_form = InProgressForm.find_by(form_id:, user_uuid: user.uuid)
    user_account_form = InProgressForm.find_by(form_id:, user_account: user.user_account) if user.user_account
    user_uuid_form || user_account_form
  end

I'm wondering if this is the issue:
InProgressForm being updated contains the user.uuid but in the form_for_user method, the user_account is taking precedence. Since the InProgressForm does not contain the account info, a new form is initialized in the controller.

[tfink419]

@RachalCassity I'll look into it for a new ticket but for now this PR is overdue to be merged so I will proceed with that

[rileyanderson]

Looks good from an identity perspective. One small unrelated suggestion

[rileyanderson]

Can this just be removed?

[tfink419]

I'll remove this in a new ticket along with the suggestions from Rachal. I wanted to leave that comment in initially to point out the change in the logic