Honor the end date (#1862)

Fix #1859

lib/ask/ecto_types/schedule.ex

@@ -9,7 +9,7 @@ defmodule Ask.Schedule do
   @behaviour Ecto.Type
 
   alias __MODULE__
-  alias Ask.DayOfWeek
+  alias Ask.{DayOfWeek, ScheduleError, SystemTime}
 
   defstruct [:day_of_week, :start_time, :end_time, :blocked_days, :timezone, :start_date, :end_date]
 
@@ -143,39 +143,78 @@ defmodule Ask.Schedule do
     "Etc/UTC"
   end
 
-  def next_available_date_time(%Schedule{} = schedule, %DateTime{} = date_time \\ DateTime.utc_now) do
+  # Find the beggining of the first active window, going forward
+  def next_available_date_time(%Schedule{end_date: end_date, start_date: start_date} = schedule, %DateTime{} = from_date_time \\ SystemTime.time.now) do
+    from_date_time = select_from_date_time(from_date_time, start_date)
+    backward = false
+    limit = end_date
+    reversible_next_available_date_time(schedule, from_date_time, backward, limit)
+  end
+
+  defp select_from_date_time(from_date_time, nil = _start_date), do: from_date_time
+
+  defp select_from_date_time(from_date_time, start_date) do
+    # 1 -- the from_date_time comes after the start_date
+    if Timex.compare(from_date_time, start_date) > 0 do
+      from_date_time
+    else
+      start_date
+    end
+  end
+
+  # Find the ending of the last active window, going backward from the last minute of the end_date
+  def last_window_ends_at(%{end_date: nil} = _schedule), do: nil
+  def last_window_ends_at(%{end_date: end_date, start_date: start_date} = schedule) do
+    backward = true
+    limit = start_date
+    from_date_time = Date.add(end_date, 1)
+    reversible_next_available_date_time(schedule, from_date_time, backward, limit)
+  end
+
+  # Why do we need this reversible function? Because we need to calculate:
+  # 1. The start_time of the first active window (backward = false)
+  # 2. The end_time of the last active window (backward = true)
+  # And the logic for doing that is pretty much the same.
+  defp reversible_next_available_date_time(schedule, date_time, backward, limit) do
+    date_time = Timex.to_datetime(date_time, schedule.timezone)
+
     # TODO: Remove the necessity of converting this to erl dates
-    date_time = date_time
-    |> Timex.to_datetime(schedule.timezone)
-
-    {erl_date, erl_time} = date_time |> Timex.to_erl
-
-    {:ok, time} = erl_time
-    |> Time.from_erl
-
-    # If this day is enabled in the schedule
-    date_time = if day_of_week_available?(schedule, erl_date) do
-      # Check if the time is inside the schedule time range
-      case compare_time(schedule, time) do
-        :before ->
-          # If it's before the time range, move it to the beginning of the range
-          at_start_time(schedule, erl_date)
-        :inside ->
-          # If it's inside there's nothing to do
-          date_time
-        :after ->
-          # If it's after the time range, find the next day
-          next_available_date_time_internal(schedule, erl_date)
-      end
+    {erl_date, erl_time} = Timex.to_erl(date_time)
+    {:ok, time} = Time.from_erl(erl_time)
+
+    selected_datetime = if day_of_week_available?(schedule, erl_date) && compare_time(schedule, time) == :inside do
+      date_time
     else
-      # If the day is not enabled, find the next day
-      next_available_date_time_internal(schedule, erl_date)
+      selected_date = select_available_date(schedule, erl_date, time, backward, limit)
+      select_time_for_date(schedule, selected_date, backward)
     end
 
-    date_time
+    selected_datetime
     |> Timex.Timezone.convert("Etc/UTC")
   end
 
+  def select_time_for_date(schedule, erl_date, backward) do
+    if backward, do: at_end_time_erl(schedule, erl_date), else: at_start_time(schedule, erl_date)
+  end
+
+  def select_available_date(schedule, erl_date, time, backward, limit) do
+    date_to_return = if day_of_week_available?(schedule, erl_date) do
+      pick_date_based_on_time_and_direction(compare_time(schedule, time), backward)
+    else
+      :next_date
+    end
+
+    case date_to_return do
+      :given_date -> erl_date
+      :next_date -> next_available_date(schedule, erl_date, backward, limit)
+    end
+  end
+
+  defp pick_date_based_on_time_and_direction(:before = _time_is, true = _backward), do: :next_date
+  defp pick_date_based_on_time_and_direction(:before = _time_is, false = _backward), do: :given_date
+  defp pick_date_based_on_time_and_direction(:after = _time_is, true = _backward), do: :given_date
+  defp pick_date_based_on_time_and_direction(:after = _time_is, false = _backward), do: :next_date
+
   def at_end_time(%Schedule{end_time: end_time, timezone: timezone}, %DateTime{} = date_time) do
     {erlang_date, _} = date_time |> Timex.Timezone.convert(timezone) |> Timex.to_erl
     erlang_time = end_time |> Time.to_erl
@@ -190,23 +229,6 @@ defmodule Ask.Schedule do
     Map.put(schedule, :end_date, nil)
   end
 
-  def end_date_passed?(schedule, date_time \\ DateTime.utc_now())
-
-  def end_date_passed?(%{end_date: nil} = _schedule, _date_time) do
-    false
-  end
-
-  def end_date_passed?(
-        %{end_date: end_date, timezone: timezone} = _schedule,
-        date_time
-      ) do
-
-    date_time
-    |> Timex.to_datetime(timezone)
-    |> DateTime.to_date()
-    |> Date.compare(end_date) != :lt
-  end
-
   defp compare_time(%Schedule{start_time: start_time, end_time: end_time}, time) do
     case Time.compare(time, start_time) do
       :lt -> :before
@@ -225,21 +247,47 @@ defmodule Ask.Schedule do
     Timex.Timezone.resolve(schedule.timezone, {erl_date, erl_time})
   end
 
-  defp next_available_date_time_internal(schedule, erl_date) do
-    erl_date = next_available_date(schedule, erl_date)
-    at_start_time(schedule, erl_date)
+  defp at_end_time_erl(schedule, erl_date) do
+    erl_time = schedule.end_time |> Time.to_erl
+    Timex.Timezone.resolve(schedule.timezone, {erl_date, erl_time})
   end
 
-  defp next_available_date(schedule, erl_date) do
-    erl_date = Timex.shift(erl_date, days: 1)
-    if day_of_week_available?(schedule, erl_date) do
-      erl_date
+  defp next_available_date(schedule, erl_date, backward, limit) do
+    raise_if_date_exceeds_limit(erl_date, backward, limit)
+
+    shift_days = if backward, do: -1, else: 1
+    next_date = Timex.shift(erl_date, days: shift_days)
+    if day_of_week_available?(schedule, next_date) do
+      next_date
     else
-      next_available_date(schedule, erl_date)
+      next_available_date(schedule, next_date, backward, limit)
     end
   end
 
-  defp day_of_week_available?(%Schedule{day_of_week: day_of_week, blocked_days: blocked_days, start_date: start_date}, erl_date) do
+  defp raise_if_date_exceeds_limit(date_time, backward, limit), do:
+    if date_exceeds_limit?(date_time, backward, limit), do:
+      raise_date_exceeds_limit(backward)
+
+  defp raise_date_exceeds_limit(true = _backward), do:
+    raise ScheduleError, "last active window not found"
+
+  defp raise_date_exceeds_limit(false = _backward), do:
+    raise ScheduleError, "next active window not found"
+
+  defp date_exceeds_limit?(_date_time, _backward, nil = _limit), do: false
+
+  defp date_exceeds_limit?(date_time, backward, limit) do
+    comparison = Timex.compare(date_time, limit)
+    if backward do
+      # -1 -- date_time comes before the limit
+      comparison < 0
+    else
+      # 1 -- date_time comes after the limit
+      comparison > 0
+    end
+  end
+
+  defp day_of_week_available?(%Schedule{day_of_week: day_of_week, blocked_days: blocked_days}, erl_date) do
     date = Date.from_erl!(erl_date)
     if day_of_week == DayOfWeek.never do
       # Just in case a schedule remains empty (can happen in a test)
@@ -254,7 +302,6 @@ defmodule Ask.Schedule do
         6 -> day_of_week.sat
         7 -> day_of_week.sun
       end && !Enum.member?(blocked_days, date)
-      && (!start_date || Date.compare(start_date, date) != :gt)
     end
   end
 

lib/ask/ecto_types/schedule_error.ex

@@ -0,0 +1,3 @@
+defmodule Ask.ScheduleError do
+  defexception message: "schedule error"
+end

lib/ask/runtime/broker.ex

@@ -74,29 +74,37 @@ defmodule Ask.Runtime.Broker do
     |> RespondentDispositionHistory.create(respondent.disposition, primary_mode)
   end
 
-  defp poll_active_surveys(now) do
-    all_running_surveys = Repo.all(from s in Survey,
-                                   where: s.state == "running",
-                                   preload: [respondent_groups: [respondent_group_channels: :channel]])
-    all_running_surveys
-    |> Enum.filter(&Schedule.intersect?(&1.schedule, now))
+  def poll_active_surveys(now) do
+    all_running_surveys =
+      Repo.all(
+        from(s in Survey,
+          where: s.state == "running",
+          preload: [respondent_groups: [respondent_group_channels: :channel]]
+        )
+      )
+
+    expired_surveys = Enum.filter(all_running_surveys, fn survey -> Survey.expired?(survey) end)
+    expired_surveys_ids = Enum.map(expired_surveys, fn %{id: id} -> id end)
+
+    Enum.filter(all_running_surveys, fn survey ->
+      survey.id not in expired_surveys_ids and Schedule.intersect?(survey.schedule, now)
+    end)
     |> Enum.each(fn survey -> poll_survey(survey, now) end)
+
+    stop_surveys(expired_surveys)
+  end
+
+  defp stop_surveys(surveys) do
+    Enum.map(surveys, fn survey -> stop_survey(survey) end)
   end
 
   def poll_survey(survey, now) do
-    if Schedule.end_date_passed?(survey.schedule, now) do
-      # Between the 00:00 of the end_date and this survey poll (the poll_interval is 1 minute)
-      # the survey will be active during a short but unexpected time window.
-      # We explicitly decided to ignore this corner case to gain solidity and simplicity
-      stop_survey(survey)
-    else
-      channels = survey |> Survey.survey_channels
-      channel_is_down? = channels |> Enum.any?(fn c ->
-        status = c.id |> ChannelStatusServer.get_channel_status
-        (status != :up && status != :unknown)
-      end)
-      poll_survey(survey, now, channel_is_down?)
-    end
+    channels = survey |> Survey.survey_channels
+    channel_is_down? = channels |> Enum.any?(fn c ->
+      status = c.id |> ChannelStatusServer.get_channel_status
+      (status != :up && status != :unknown)
+    end)
+    poll_survey(survey, now, channel_is_down?)
   end
 
   defp poll_survey(survey, _now, true = _channel_is_down) do

lib/ask/runtime/survey_action.ex

@@ -1,5 +1,5 @@
 defmodule Ask.Runtime.SurveyAction do
-  alias Ask.{Survey, Logger, Repo, Questionnaire, ActivityLog, SurveyCanceller, Project, SystemTime}
+  alias Ask.{Survey, Logger, Repo, Questionnaire, ActivityLog, SurveyCanceller, Project, SystemTime, Schedule}
   alias Ask.Runtime.PanelSurvey
   alias Ecto.Multi
 
@@ -105,7 +105,11 @@ defmodule Ask.Runtime.SurveyAction do
   end
 
   defp perform_start(survey, repetition?) do
-    changeset = Survey.changeset(survey, %{state: "running", started_at: SystemTime.time.now})
+    changeset = Survey.changeset(survey, %{
+      state: "running",
+      started_at: SystemTime.time.now,
+      last_window_ends_at: Schedule.last_window_ends_at(survey.schedule)
+    })
 
     case Repo.update(changeset) do
       {:ok, survey} ->

locales/template/translation.json

@@ -544,7 +544,7 @@
   "You'll be able to see surveys, questionnaires, content and collaborators": "",
   "activities": "",
   "activity": "",
-  "and will be canceled on {{endDate}}": "",
+  "and will be canceled on {{lastWindowEndsAt}}": "",
   "channel": "",
   "channels": "",
   "disposition history": "",

priv/repo/migrations/20210326160009_add_last_window_ends_at_to_surveys.exs

@@ -0,0 +1,9 @@
+defmodule Ask.Repo.Migrations.AddLastWindowEndsAtToSurveys do
+  use Ecto.Migration
+
+  def change do
+    alter table(:surveys) do
+      add :last_window_ends_at, :naive_datetime
+    end
+  end
+end

priv/repo/migrations/20210326160205_fill_last_window_ends_at_in_surveys.exs

@@ -0,0 +1,11 @@
+defmodule Ask.Repo.Migrations.FillLastWindowEndsAtInSurveys do
+  use Ecto.Migration
+
+  def up do
+    Ask.Repo.query("UPDATE surveys SET last_window_ends_at = ended_at WHERE ended_at IS NOT NULL")
+  end
+
+  def down do
+    Ask.Repo.query("UPDATE surveys SET last_window_ends_at = null WHERE last_window_ends_at IS NOT NULL")
+  end
+end

test/controllers/short_link_controller_test.exs

@@ -22,7 +22,7 @@ defmodule Ask.ShortLinkControllerTest do
     conn = get conn, short_link_path(conn, :access, link.hash)
 
     assert json_response(conn, 200)["data"] == [
-      %{"cutoff" => survey.cutoff, "id" => survey.id, "mode" => survey.mode, "name" => survey.name, "description" => nil, "project_id" => project.id, "state" => "not_ready", "locked" => false, "exit_code" => nil, "exit_message" => nil, "schedule" => %{"blocked_days" => [], "day_of_week" => %{"fri" => true, "mon" => true, "sat" => true, "sun" => true, "thu" => true, "tue" => true, "wed" => true}, "end_time" => "23:59:59", "start_time" => "00:00:00", "start_date" => nil, "end_date" => nil, "timezone" => "Etc/UTC"}, "next_schedule_time" => nil, "down_channels" => [], "started_at" => nil, "ended_at" => nil, "updated_at" => DateTime.to_iso8601(survey.updated_at), "folder_id" => nil, "first_window_started_at" => nil, "is_panel_survey" => false, "is_repeatable" => false, "panel_survey_of" => nil}
+      %{"cutoff" => survey.cutoff, "id" => survey.id, "mode" => survey.mode, "name" => survey.name, "description" => nil, "project_id" => project.id, "state" => "not_ready", "locked" => false, "exit_code" => nil, "exit_message" => nil, "schedule" => %{"blocked_days" => [], "day_of_week" => %{"fri" => true, "mon" => true, "sat" => true, "sun" => true, "thu" => true, "tue" => true, "wed" => true}, "end_time" => "23:59:59", "start_time" => "00:00:00", "start_date" => nil, "end_date" => nil, "timezone" => "Etc/UTC"}, "next_schedule_time" => nil, "down_channels" => [], "started_at" => nil, "ended_at" => nil, "updated_at" => DateTime.to_iso8601(survey.updated_at), "folder_id" => nil, "first_window_started_at" => nil, "is_panel_survey" => false, "is_repeatable" => false, "panel_survey_of" => nil, "last_window_ends_at" => nil}
     ]
   end
 end