I removed the "bug" label, because even though we don't respect scheduled hours for SMS messages, we didn't have the mean to fix the the problem before. We now have the ability to finally implement the feature.

Related #2288

(summoning @matiasgarciaisaia who's the most up to date with the project)

Hey, I'm now working on this and I'm a bit confused by this statement:

Ask.Runtime.Channel can compute and pass the not_before and not_after attributes for the SMS modes, as it does for IVR mode, and pass them to Ask.Runtime.ChannelBroker.ask, allowing the channel-broker to respect the scheduled time: keep messages in queue until not_before and skip messages when not_after is reached.

Is it possible that you meant Ask.Runtime.Session? That's the actor that seems to be computing and forwarding the not_before and not_after params to ChannelBroker:

def contact_respondent(%{current_mode: %SMSMode{}} = session) do
    token = Ecto.UUID.generate()

    respondent = session.respondent
    {:ok, _flow, reply} = Flow.retry(session.flow, TextVisitor.new("sms"), respondent.disposition)
    channel = session.current_mode.channel
    log_prompts(reply, channel, session.flow.mode, respondent)

    ChannelBroker.ask(channel.id, channel.type, session.respondent, token, reply)

    respondent = Respondent.update_stats(respondent.id, reply)
    %{session | token: token, respondent: respondent}
  end

  def contact_respondent(%{schedule: schedule, current_mode: %IVRMode{}} = session) do
    token = Ecto.UUID.generate()

    next_available_date_time =
      schedule
      |> Schedule.next_available_date_time()

    today_end_time =
      schedule
      |> Schedule.at_end_time(next_available_date_time)

    channel = session.current_mode.channel

    ChannelBroker.setup(
      channel.id,
      channel.type,
      session.respondent,
      token,
      next_available_date_time,
      today_end_time
    )

    %{session | token: token}
  end

Follow your guts. My 🧠 was likely wrong when I write that; what you're reading is most likely right 👍

(Using the issue as a log, please don't feel like you need to provide feedback, but it's always welcome)

It looks like a combination of the following would resolve this issue:

1. Ask.Runtime.Session currently computes not_before and not_after for IVR contacts, then activates the ChannelBroker with that data. AFAICT, it could do the same for SMS.
2. Then, ChannelBroker.ask simply queues the respondent to be contacted, including not_before and not_after info.
3. Then we could modify ChannelBrokerState.activate_next_in_queue so that it filters out any queued contacts outside of the given range.

def activate_next_in_queue(state) do
    # add leeway to activate contacts to be scheduled soon:
    not_before = SystemTime.time().now |> DateTime.add(60, :second)

    contact =
      Repo.one!(
        from q in Queue,
          where:
            q.channel_id == ^state.channel_id and is_nil(q.last_contact) and
              (q.not_before <= ^not_before or is_nil(q.not_before)),
          order_by: [q.priority, q.queued_at],
          limit: 1
      )

    contact
    |> Queue.changeset(%{
      contacts: contact.size,
      last_contact: SystemTime.time().now
    })
    |> Repo.update()

    {state, to_item(state.channel_type, contact)}
  end

Note that there's currently only a filter by not_before, which currently only matters for IVR. SMS contacts come with not_before=nil and not_after=nil, so all of them are selected by this query.

I'm not sure the purpose of filtering by not_before in the current version, since the setting is forwarded to Verboice which is smart enough to handle it. I guess it helps in not flooding Verboice unnecessarily?

Anyway, I think if we add a filter for not_after here we should be guaranteeing that the respondent won't be contacted.

Now, if we do that, we'll end up with garbage in the queue. I saw there's a garbage collection process, but AFAICT it doesn't take into account not_after (which again, makes sense because Surveda just relies on Verboice for it).

Now, if we do that, we'll end up with garbage in the queue. I saw there's a garbage collection process, but AFAICT it doesn't take into account not_after (which again, makes sense because Surveda just relies on Verboice for it).

So maybe instead of filtering "expired" elements at unenqueue, we can make it a responsibility of the ChannelBroker.

ChannelBroker                                                     ChannelBrokerState
          |         --Give me next contact-->                            |
          |         <-- respondent-1, sms, not_after past --    |
          |         too late for respondent-1, discarding                    *
          |         --Give me next contact-->                            |
          |         <-- respondent-2, sms, not_after good --  |
          |        cool, sending to Nuntium!

Question for tomorrow's Martin: then, when ChannelBroker discards the contact, is there any active process that will pick things up in the next valid window? (Eg SurveyBroker). Maybe we'll need to change the respondent's state?

Hint for Q: c5fa0c9

I guess it helps in not flooding Verboice unnecessarily?

It's actually one of the main feature of the ChannelBroker: it sets a limit to the number of messages sent to an external channel, whatever the survey.

If we sent messages queued for tomorrow to the external channel, it could fill the output quota for that channel and limit or block another survey from sending messages on the same channel (oops).

If we sent messages queued for tomorrow to the external channel, it could fill the output quota for that channel and limit or block another survey from sending messages on the same channel (oops).

Surveda was already smart enough not to do this (due to the contact window, it wouldn't ever queue contacts for tomorrow), but what used to happen was that at the start of the day, it'd queue about 8x the daily capacity of the channel (ie, queue 80k calls for a 11k calls/day channel), and then Verboice would spend the whole night expiring the 70k calls that it couldn't manage to make during the day. We even saw that expiring process finish just about 5 minutes before the next day's active window started again - so a lot of overhead for no real reason.