Respect survey schedules before sending SMS #2380
Conversation
| @@ -36,8 +36,8 @@ defmodule Ask.Runtime.ChannelBroker do | |||
| call(channel_id, {:has_delivery_confirmation?}) | |||
| end | |||
|
|
|||
| def ask(channel_id, channel_type, respondent, token, reply) do | |||
| cast(channel_id, {:ask, channel_type, respondent, token, reply}) | |||
| def ask(channel_id, channel_type, respondent, token, reply, not_before \\ nil, not_after \\ nil) do | |||
There was a problem hiding this comment.
Tempted to refactor so that nil's aren't accepted, but I wonder if there are other parts of Surveda where a schedule is really optional
There was a problem hiding this comment.
Heh, was about to notice the same thing, but didn't want to nitpick too much.
The only Surveda use case without a schedule may be the questionnaire simulator, but even then I think we're setting up a whole survey with a fake/"always on" schedule.
There was a problem hiding this comment.
Yeah, the simulator came to mind. I can do it then. Let's pray that test coverage is enough :D
There was a problem hiding this comment.
Mmm, I just noticed ask is also used for mobile web mode, and in that one we don't care about schedules
| @@ -36,8 +36,8 @@ defmodule Ask.Runtime.ChannelBroker do | |||
| call(channel_id, {:has_delivery_confirmation?}) | |||
| end | |||
|
|
|||
| def ask(channel_id, channel_type, respondent, token, reply) do | |||
| cast(channel_id, {:ask, channel_type, respondent, token, reply}) | |||
| def ask(channel_id, channel_type, respondent, token, reply, not_before \\ nil, not_after \\ nil) do | |||
There was a problem hiding this comment.
Heh, was about to notice the same thing, but didn't want to nitpick too much.
The only Surveda use case without a schedule may be the questionnaire simulator, but even then I think we're setting up a whole survey with a fake/"always on" schedule.
| @@ -69,9 +69,9 @@ defmodule Ask.Runtime.ChannelBrokerStateTest do | |||
| mock_time(now) | |||
|
|
|||
| State.new(0, "sms", %{}) | |||
| |> State.queue_contact({%{id: 2, disposition: :queued}, "secret", []}, 5) | |||
| |> State.queue_contact({%{id: 2, disposition: :queued}, "secret", [], nil, nil}, 5) | |||
There was a problem hiding this comment.
Shall we set non-nil times here, so we don't add coverage for a case we don't want to support?
| Enum.at(respondents, 4) | ||
| ] | ||
|
|
||
| # All respondents should be in broker queue, but only the first one contacted |
There was a problem hiding this comment.
Where's Surveda guaranteeing this? We activate respondents with a not_before in the following minute, too, so I think there's a good chance of contacting both respondents 3 and 4 here when the broker runs.
There was a problem hiding this comment.
It's an outdated comment from a previous iteration on this test, it's actually not true even in the context of this test. Will fix
Fixes #2283
The fix involves adding
not_beforeandnot_afterfields toChannelBroker.ask(aka,sms, btw why don't we rename this??), and honoring them (it was only sent withChannelBroker.setup(aka, 'ivr', ditto).The way to honor those fields involves:
nilfor SMS)ChannelBrokerdequeuesPending work: