State enter events are fired twice

[justinvdm]

State switches (and thus state creations) happen in two cases: (a) a state is created so we can reply the user with its content (b) a state is created to process the user's input

Since each state is created twice, we have checks to see whether the creation was because of (a) or (b), and fire an enter event if it was (a).

Problem is, the checks are broken. At the moment, state switching goes something like:

if im already on target_state:
  return

create target_state
emit exit event on current_state

if im not already on target_state: # <-- problem is here
  emit enter event

change user data to put them on target_state
current_state = target_state

I'm surprised that that problem wasn't revealed in tests, but I think I actually meant to check if the user is on the target state already, not the interaction machine (since we already check that higher up). Still, checking if the user is on the target state already is not enough, that check will always true, since states tell the interaction machine which target state to go to next by modifying the user's current state (which happens before switch_state is reached).

One fix would be to change states to tell the interaction machine what state to go to next instead of changing the users state (and leaving that up to the interaction machine). That will allow the interaction machine to figure out if (a) or (b) happened.

A better, longer term fix would be to structure the toolkit to differentiate between (a) and (b) as follows:

• create the state, then perform only the state things that should happen for (a)
• serialize the state and shut down the sandbox
• deserialize and recreate the state, then perform only the things that should happen for (b)

That would solve this issue, along with a number of other issues that we often encounter because of the fact that states are created twice. If we have that approach in place, whether (a) or (b) happened would be more explicit, instead of us needing all kinds of checks to try and infer which one happened in different places all over the interaction machine and each app. Obviously, that approach will be more work.

[hodgestar]

There are also cases where the same state interacts with the phone user multiple times (e.g. when input is rejected and the state stays the same or for multipage state like the booklet).

[justinvdm]

Plan is to try the first approach. We may end up using it for the longer term approach.

[justinvdm]

Ready for review. Summary of the changes:

• We now are more explicit about how we change between states. Here's the pseudocode:

when we get an inbound message:
  if the user is on a state:
    # see the new im.resume_state(), which also fires a states:resume event
    resume the state
  else:
    enter the start state

  when this is a resume:
    # see `im.next_state`
    the resumed state will process the user's input and tell the interaction machine which state to go to next

  when replying:
    # see the changed `im.switch_state()`
    the interaction machine will switch from its current state to the state it should go to next

A separate note: I first had switch_state take in both the source and destination state (I think that was a detail of the discussed plan), but it looked really weird giving the interaction machine its current state that it already knows, then getting and setting other things on the interaction machine. If we had a pure function-data split, I think having it take in both might make sense, but because we are using an oo approach, it looked pretty weird.

[justinvdm]

Ah, just realised I still need to add a test to make sure this bug is actually fixed, doing so now.

[justinvdm]

Ready for review.

[justinvdm]

@smn @imsickofmaps this PR documents how state lifecycle events happen a bit more clearly (I hope). This test might clarify things better than an explanation will.

state:resume is an event type added in this PR.

[hodgestar]

A couple of high-level questions:

• What happens when a user dials into a state which no longer exists? This would go via the resume path, but end up in a new state and fire the resume event for the new state. Maybe .resume_state should fire an enter state event if the new state has a different name to the one it attempted to enter?
• im.user.state is now gone right? Or at least a lot of tests that used it change? Bit worried that this is something existing code might access. Perhaps a quick check of the few more complex / active users would be a good idea?

Other than that, really liking the new enter/resume/exit cycle -- makes everything much clearer.

[justinvdm]

Maybe .resume_state should fire an enter state event if the new state has a different name to the one it attempted to enter

+1, thanks for catching that case

im.user.state is now gone right?

It still exists. We use it when we get an inbound message to determine which state to resume (or to enter the start state if it doesn't exist). We also set it in in set_state, which would happen when a resume, enter or (after fixing) exit happens.

What is different is how the next state is changed. Previously, states would change im.user.state to set the next state, but now we use im.next_state for this. Specific state implementation don't change im.user.state directly though, they do this via State.set_next_state, so luckily this won't break the actual implementations. It will however break the state implementations' tests if the tests are checking if the state changed im.user.state. This is only the case for the 'old style' tests (the tests we wrote before we had AppTester). I think it is very unlikely that there are state implementation tests out there that don't use AppTester, but will do a quick check if you think it is worth it.

[hodgestar]

I think we can skip the checking. Now that I understand the change a bit better, it doesn't look as compatibility breaking.

[justinvdm]

Ready for re-review, I think.

[hodgestar]

:+1: