Event refactor #55

rowanseymour · 2017-07-17T14:57:38Z

Requires nyaruka/rapidpro#1354 to use with rapidpro unit tests

rowanseymour · 2017-07-17T14:58:16Z

cmd/flowrunner/testdata/flows/brochure_test.json

-          "type": "msg_received",
-          "urn": "tel:+12065551212"
-        },
-        {


We no longer repeat caller events back to the caller

rowanseymour · 2017-07-17T14:58:37Z

cmd/flowrunner/testdata/flows/brochure_test.json

@@ -129,7 +119,6 @@
              "channel_uuid": "57f1078f-88aa-46f4-a59a-948a5739c03d",
              "contact_uuid": "ba96bf7f-bc2a-4873-a7c7-254d1927c4e3",
              "created_on": "2000-01-01T00:00:00.000000000-00:00",
-              "id": 0,


We don't use this anywhere so I think we can drop it for now

rowanseymour · 2017-07-17T15:02:59Z

flows/events/flow_entered.go

 }

 // Type returns the type of this event
 func (e *FlowEnteredEvent) Type() string { return TypeFlowEntered }

-// Resolve resolves the passed in key for this event
-func (e *FlowEnteredEvent) Resolve(key string) interface{} {


These kind of events are no longer become input

rowanseymour · 2017-07-17T16:02:45Z

cmd/flowrunner/testdata/flows/brochure_test.json

-          "contact_uuid": "ba96bf7f-bc2a-4873-a7c7-254d1927c4e3",
-          "created_on": "2000-01-01T00:00:00.000000000-00:00",
-          "id": 0,
-          "step_uuid": "",


At some point I think we can stop repeating "step_uuid" here as events are also nested under the steps in the path... but whilst we're generating flow step objects in rapidpro, it's a bit awkward to figure out which event occured where without these

nicpottier · 2017-07-17T15:51:58Z

cmd/flowrunner/testdata/flows/brochure_test.json

@@ -129,7 +119,6 @@
              "channel_uuid": "57f1078f-88aa-46f4-a59a-948a5739c03d",
              "contact_uuid": "ba96bf7f-bc2a-4873-a7c7-254d1927c4e3",
              "created_on": "2000-01-01T00:00:00.000000000-00:00",
-              "id": 0,


nicpottier · 2017-07-17T16:02:27Z

flows/engine/engine.go

+	// we can now apply the caller events
+	for _, event := range callerEvents {
+		run.ApplyEvent(step, event)
+	}


I don't disagree, but mostly interested in maybe documenting here why we do the wait and apply in this order.. perhaps having a longer comment here would help. I imagine there is a good reason around flow exits and such..

The wait is the gate keeper than decides if flow execution can proceed - so I think it makes sense that caller events are applied after that. I'll document that somewhere..

nicpottier · 2017-07-17T16:05:51Z

flows/interfaces.go

+	FromCaller() bool
+	SetFromCaller(bool)
+
+	Apply(FlowRun)


I'm surprised this doesn't return an error.. we don't have any case where something can go wrong when applying?

Currently the actions catch the error cases and turn them into error events, but yeah seems ok for events to generate error events - certainly if they come from the caller.

I guess we can always add this if/when we see a need, so no need to change it now.

nicpottier · 2017-07-17T16:06:38Z

flows/interfaces.go

 	SetCreatedOn(time.Time)

 	Step() StepUUID
 	SetStep(StepUUID)

+	FromCaller() bool
+	SetFromCaller(bool)


I know this is because we are trying not to echo this back out, but man, smells a bit having it on our interface.

You'd prefer setting the flag directly in ReadEvents?

Wonder if this, step and create_on are other signs that maybe we need something wrapping an Event that is really the StepEvent or the like.. would that make sense?

So while we currently have stuff like:

{ "events": [ { "created_on": "2000-01-01T00:00:00.000000000-00:00", "groups": [ { "name": "Survey Audience", "uuid": "2aad21f6-30b7-42c5-bd7f-1b720c154817" } ], "step_uuid": "", "type": "add_to_group" }, { "created_on": "2000-01-01T00:00:00.000000000-00:00", "field_name": "Activation Token", "field_uuid": "f06c5b73-eb0d-4417-b7e0-4f650ed30dc8", "step_uuid": "", "type": "save_contact_field", "value": "XXX-YYY-ZZZ" } ] }

Maybe it is more right to have something like:

{ "events": [ { "created_on": "2000-01-01T00:00:00.000000000-00:00", "step_uuid": "", "event": { "type": "add_to_group", "groups": [ { "name": "Survey Audience", "uuid": "2aad21f6-30b7-42c5-bd7f-1b720c154817" } ], } }, { "created_on": "2000-01-01T00:00:00.000000000-00:00", "step_uuid": "", "event": { "type": "save_contact_field", "field_name": "Activation Token", "field_uuid": "f06c5b73-eb0d-4417-b7e0-4f650ed30dc8", "value": "XXX-YYY-ZZZ" } } ] }

It definitely is less than ideal that we have step_uuid on event for example, when this is really only an attribute that is there because of the passed back events list, not really an attribute of event at all. Basically maybe we should be putting events in containers that contain timestamps / step uuids instead of making them all deal with that. (and then having the confusion as to whether that is a required attribute by the caller)

Any ideas there?

Well now that I've embraced the "events as real history" view I'm cool with created_on being the real world time. Regarding step_uuid I think we could ditch it and infer it from the nested events in the path.. but for what we're trying to do currently creating flowsteps, it's kinda awkward to figure out which events in the session are which events in the path.

I was wondering about changing events to something like:

"new_events": [ { "step_uuid": "1234..." "events": [ ... ], } ]

That makes it easy to get stuff like step time, node uuid etc for each of your new events that you need to apply in the caller.

I like that.. at least not having step_uuid on event would be a move in the right direction. Should we have an applied_on?

K so then I'll update it as above - do like having caller events have all the same fields as server events

Also, from a parallel discussion, we also need to know the action uuid that generated an event, so can easily see this as a sibling of step_uuid above.

We don't currently know/need that but is that something you think we should log?

Eric kind of has a use case for that actually in the broadcast batching.. being able to map which action created a send_msg event would actually be useful there I think.

Ok am merging this as is for now because I need to take care of some other stuff, and have created #56

nicpottier · 2017-07-17T16:08:59Z

flows/engine/engine.go

@@ -143,24 +139,30 @@ func resumeNode(run flows.FlowRun, node flows.Node, step flows.Step, event flows
 		return noDestination, step, nil
 	}

-	err := wait.End(run, step, event)
+	// try to resume our wait with the first caller event
+	err := wait.End(run, step, callerEvents[0])


Maybe waits should take a list of events? This seems like a very specific and subtle contract with our callers.

If order is important then the wait is waiting for the next event, i.e. the first in the list. This feels right IMO

Ya, I can't currently really think of a case when we would resume with more than one event so I'm fine with it as is for now.

rowanseymour added 6 commits July 17, 2017 14:22

Rework events and actions so that events make the context changes

9f44393

msg_received event is responsible for setting input on the run

6e431bb

Rework initial event processing and waits

3db3357

Start and resume requests should both take an array of initial events

08fefed

Don't repeat caller events in the session events

6bb4398

Remove msg id from msg_received events

5861255

rowanseymour self-assigned this Jul 17, 2017

rowanseymour commented Jul 17, 2017

View reviewed changes

Rename resume_events -> caller_events in tests

db997d2

rowanseymour requested review from nicpottier and ericnewcomer July 17, 2017 15:49

rowanseymour commented Jul 17, 2017

View reviewed changes

nicpottier requested changes Jul 17, 2017

View reviewed changes

Tweaks from review

c5ae8e4

nicpottier approved these changes Jul 17, 2017

View reviewed changes

ericnewcomer approved these changes Jul 17, 2017

View reviewed changes

rowanseymour mentioned this pull request Jul 18, 2017

Inputs as separate thing #53

Closed

rowanseymour merged commit d9a947e into master Jul 18, 2017

rowanseymour deleted the events_2 branch July 18, 2017 07:54

rowanseymour mentioned this pull request Jul 18, 2017

Add a container class for engine generated events to hold step UUID etc #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event refactor #55

Event refactor #55

rowanseymour commented Jul 17, 2017 •

edited

Loading

rowanseymour Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017 •

edited

Loading

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

ericnewcomer Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 18, 2017

nicpottier Jul 17, 2017

rowanseymour Jul 17, 2017

nicpottier Jul 17, 2017

Event refactor #55

Event refactor #55

Conversation

rowanseymour commented Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicpottier Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanseymour commented Jul 17, 2017 •

edited

Loading

nicpottier Jul 17, 2017 •

edited

Loading