Fix 500 during proofing verify step when expected information is not present

[mitchellhenke]

We had a few errors in New Relic

I suspect there may be other paths to get into this state, but I was able to reproduce with:

1. Start proofing process, and proceed up to the resolution/verify step
2. Open the page in another tab
3. Click "Start over" in one of the tabs
4. In the other tab, click "Continue", and the server will 500

It's likely an uncommon path towards a 500, but the result is:

[aduth]

I kinda wonder if we should be preventing this before it gets to #call. For example, should the before_action here also apply to update? 🤔

identity-idp/app/services/flow/flow_state_machine.rb

Line 7 in 9a9fc8a

before_action :ensure_correct_step, only: :show

[aduth]

I drafted a comment here but it looks like it was updated / fixed in the meantime. Still, depending if we want a one-liner, could be relevant:

---

Curious about the || here: Looking at the implementation of mark_step_incomplete, I might assume it returns a truthy value, assuming the corresponding step exists, and never hit the return.

identity-idp/app/services/flow/base_step.rb

Line 26 in 9a9fc8a

flow_session.delete(klass.to_s)

Wonder if we could make an explicit (nil?) return value from mark_step_incomplete, and write this as:

return mark_step_incomplete(:ssn) if pii_from_doc.nil?

... with added advantage that it's clearer from the start of the line that this is an early return path.

[mitchellhenke]

Yeah, that's a great idea, will give that a go

[mitchellhenke]

I kinda wonder if we should be preventing this before it gets to #call. For example, should the before_action here also apply to update? 🤔

identity-idp/app/services/flow/flow_state_machine.rb

Line 7 in 9a9fc8a

before_action :ensure_correct_step, only: :show

This was the other primary option I considered, and could be a better/more comprehensive way to solve, since I discovered it also affects the prior SsnStep. The tricky part is we don't want it applied to all steps, so it is kind of step-specific.

[aduth]

Maybe want to ensure that the #call result is empty, to protect in case a future maintainer decides the nil return in mark_step_incomplete is redundant?

[mitchellhenke]

Good call, thank you, added

[aduth]

Should we have one of these specs for ssn_step as well?

[mitchellhenke]

Yep, added new spec file for it