Async proofing calls (LG-3330)

[mitchellhenke]

PR currently includes base work for making specific flow steps asynchronous and the implementation of making the IDV verify step async.

My thoughts/notes/documentation as I worked through this:

The FlowStateMachine mixin is used by controllers and implements the show/update controller actions for each step. The controller defines a flow in the controller's FSM_SETTINGS constant, and the class under the :flow key defines the steps for that flow. A lot of the state management for flows is done in the cookie session.

Parts of the base flow functionality are less flexible towards asynchronous functionality, especially related to marking parts of a flow as complete. My current solution has the step class and flow classes go back and forth with one another, which is less than ideal.

The normal synchronous flow is show form -> submit form to update action -> validate/process -> go to next step -> repeat

The asynchronous flow is more of a finite state machine, and the current implementation has the controller show action having the responsibility of showing the form like the synchronous flow, but it also checks the job status.

A job can have a status of:

1. :none
2. :in_progress
3. :timed_out
4. :done

If a job is :none or :timed_out, the show action will display the form and allow the user to submit it. If the job is :in_progress, it will eventually show a waiting screen that refreshes the page on some interval. If the job is :done, the step is marked as complete and the user is redirected to the next step if the result is successful, otherwise an error is rendered.

When the form is submitted, a DocumentCaptureSession is created, and the uuid is stored in the session to be able to check the status. The pii data for the job is stored in Redis via that as well, and a job is enqueued using that DocumentCaptureSession uuid. The user is redirected back to the show action, which will then check job status.

The job eventually runs and updates the DocumentCaptureSession result stored in Redis, and the show page will follow the steps for a job that is :done.

A job is timed out if the Redis storage for the job is empty (currently the TTL is 60 seconds).

It is not possible at the moment for a job to be :in_progress since the jobs are run inline right now.

[zachmargolis]

This is nearly a full copy of DocumentCaptureSessionResult --- now that we have 2 of these, would it be worth having something that's like a RedisJsonStruct that we can make and then separate the value objects from the storage? I'm thinking like:

ProofingDocumentCaptureSessionResult = RedisJsonStruct.new(
  attrs: [:id, :pii, :result],
  redis_key_prefix: 'dcs-proofing:result'
)
# alternative, possibly easier to read
ProofingDocumentCaptureSessionResult = Struct.new(:id, :pii, :result, keyword_init: true) do
  def self.redis_key_prefix
    'dcs-proofing:result'
  end
end

Then we could have a separate like RedisStructStorage that does something like:

result = ProofingDocumentCaptureSessionResult.new(id: SecureRandom.hex, pii: pii, result: result)
RedisStructStorage.new.store(result)

deserialized_result = RedisStructStorage.new.load(result.id, type: ProofingDocumentCaptureSessionResult)

[mitchellhenke]

The abstraction of the similar functionality was something I briefly discussed with @jmhooper so he may have thoughts here too.

My gut is to invoke the rule of threes since there may be more use cases along these lines. I don't know if I should be thinking of the common functionality more in terms of "private temporary document storage" or "Redis storage" right now.

I do think it would be good to align the methods and structure for the two classes a bit more though to move in that direction.

[zachmargolis]

I feel somewhat strongly about this, so I'm going to make a PR on to this branch with refactoring ideas

[zachmargolis]

PR: #4217

[zachmargolis]

😂
As Andy would say, "time is a flat circle"
https://github.com/18F/identity-idp/pull/1524/files#diff-2df0b9c9e83aa13bc56217ce31c8040c

[zachmargolis]

one idea, I know I mentioned it in chat, but just posting for posterity:

In case anybody in the future expects the old behavior of any key in the hash getting turned into an instance variable, but that not happening because it's not listed here, one idea would be to delete from a copy as we go, and error if there are any left:

fsm_settings = klass::FSM_SETTINGS.dup
flow = fsm_settings.delete(:flow)
@step_url = fsm_settings.delete(:step_url)
# ...
@view = fsm_settings.delete(:view)
raise "unknown FSM_SETTINGS keys: #{fsm_settings.keys.join(', ')}" unless fsm_settings.empty?

[stevegsa]

@mitchellhenke this is some really great work! I originally planned on supporting optional "show" service classes (ie VerifyStepShow) and the framework would automatically call them if they existed. I got around it twice by adding methods to controllers: once for polling in DocAuthController and the other for token processing in CaptureDocController. Some of my original goals were to keep the controllers as lean as possible and utilize service classes (single purpose w/one public method like those found in lib) for steps: #2451 .

As I see it we have three ways we could go here:

1. use your new async code as is and allow for multiple public methods in our service classes (ie after_call, async, async_state, etc.)
2. create optional "show" service class support
3. add methods to the appropriate controller

My personal preference would be to add support for optional "show" service classes and encapsulate any logic for showing a step there. Once that is created I can reel in those two one-offs. What do you think?

[mitchellhenke]

@stevegsa could you share a short example of #2 and #3?

I think part of my struggle with it is whether adding more exceptions to the flow behavior is preferable to making asynchronous an optional behavior for a step implemented at the same level as the normal flow. I think what I have implemented is a bit complex and not necessarily intuitive, so I'm very open to seeing what #2/#3 would look like.

[stevegsa]

Here would be a short example of option 2:

class DocAuthFlow < Flow::BaseFlow
  OPTIONAL_SHOW_STEPS = {
    verify: Idv::Steps::VerifyStepShow
  }

module Flow
  module FlowStateMachine
    def render_step(step, flow_session)
	  ...
	  optional_show_step = OPTIONAL_SHOW_STEPS[step]
	  call_optional_show_step(step, flow_session) if optional_show_step
	  ...

[stevegsa]

btw @mitchellhenke i am thoroughly impressed with your solution here. i can't believe you wrapped your brain around this code so quickly...you are a total ACE!

[stevegsa]

An example of option 3:

module Idv
  class DocAuthController < ApplicationController
    before_action :call_optional_show_steps

[stevegsa]

I added example 2 to this PR: #4226. It has an example of making the old resolution update call pass instead to an interstitial page which has an optional show step that makes the resolution call and marks the step complete (that interstitial page will poll in the show step, etc.). It's not complete yet because I need your async stuff but the state machine should be close to done. It ignores any return values in the show step but follows redirects, allows you to mark steps complete, etc.

[stevegsa]

I added polling to that PR along with a stub for the async check.