Code refactor PanelSurvey (#2007)

Code refactor PanelSurvey's `copy_respondents`, following ToDo comment.

New `RespondentGroupAction` functions:

- `clone_respondents_groups_into` (protocol message)
- `phone_numbers_by_group_id` (private)

Code refactor's goal was:

- more clarity while reading the code
- extract Repo's query from `Enum.map` (replace n queries with 1 unique query)

Code refactor impact on dependencies:

- Remove `Ecto.Query` from `Ask.Runtime.PanelSurvey`

lib/ask/runtime/panel_survey.ex

@@ -1,6 +1,5 @@
 defmodule Ask.Runtime.PanelSurvey do
-  import Ecto.Query
-  alias Ask.{Survey, Repo, Respondent, RespondentGroupChannel, Schedule, PanelSurvey}
+  alias Ask.{Survey, Repo, Schedule, PanelSurvey}
   alias Ask.Runtime.RespondentGroupAction
 
   defp new_wave_changeset(survey) do
@@ -13,8 +12,7 @@ defmodule Ask.Runtime.PanelSurvey do
       |> Schedule.remove_start_date()
       |> Schedule.remove_end_date()
 
-    new_wave = %{
-      # basic settings
+    basic_settings = %{
       project_id: survey.project_id,
       folder_id: survey.folder_id,
       name: PanelSurvey.new_wave_name(),
@@ -23,7 +21,9 @@ defmodule Ask.Runtime.PanelSurvey do
       state: "ready",
       started_at: Timex.now(),
       panel_survey_id: survey.panel_survey_id,
-      # advanced settings
+    }
+
+    advanced_settings = %{
       cutoff: survey.cutoff,
       count_partial_results: survey.count_partial_results,
       schedule: schedule,
@@ -36,42 +36,24 @@ defmodule Ask.Runtime.PanelSurvey do
       incentives_enabled: survey.incentives_enabled
     }
 
+    new_wave = Map.merge(basic_settings, advanced_settings)
+
     survey.project
     |> Ecto.build_assoc(:surveys)
     |> Survey.changeset(new_wave)
   end
 
-  def copy_respondents(current_wave, new_wave) do
-    current_wave =
-      current_wave
-      |> Repo.preload([:respondent_groups])
+  defp copy_respondents(current_wave, new_wave) do
+    current_wave = current_wave |> Repo.preload([:respondent_groups])
 
-    # TODO: Improve the following respondent group creation logic.
-    # For each existing group a new respondent group with the same name is created. Each
-    # respondent group has a copy of every respondent (except refused) and channel association
-    respondent_group_ids =
-      Enum.map(current_wave.respondent_groups, fn respondent_group ->
-        phone_numbers =
-          from(r in Respondent,
-            where:
-              r.respondent_group_id == ^respondent_group.id and r.disposition != :refused and
-                r.disposition != :ineligible,
-            select: r.phone_number
-          )
-          |> Repo.all()
-          |> RespondentGroupAction.loaded_phone_numbers()
-
-        new_respondent_group =
-          RespondentGroupAction.create(respondent_group.name, phone_numbers, new_wave)
-
-        copy_respondent_group_channels(respondent_group, new_respondent_group)
-        new_respondent_group.id
-      end)
+    new_respondent_groups_ids =
+      RespondentGroupAction.clone_respondents_groups_into(current_wave.respondent_groups, new_wave)
+      |> Enum.map(& &1.id)
 
     new_wave
     |> Repo.preload(:respondent_groups)
     |> Survey.changeset()
-    |> Survey.update_respondent_groups(respondent_group_ids)
+    |> Survey.update_respondent_groups(new_respondent_groups_ids)
     |> Repo.update!()
   end
 
@@ -128,29 +110,6 @@ defmodule Ask.Runtime.PanelSurvey do
     {:ok, Repo.get!(PanelSurvey, panel_survey.id)}
   end
 
-  defp copy_respondent_group_channels(respondent_group, new_respondent_group) do
-    respondent_group =
-      respondent_group
-      |> Repo.preload(:respondent_group_channels)
-
-    Repo.transaction(fn ->
-      Enum.each(
-        respondent_group.respondent_group_channels,
-        fn respondent_group_channel ->
-          RespondentGroupChannel.changeset(
-            %RespondentGroupChannel{},
-            %{
-              respondent_group_id: new_respondent_group.id,
-              channel_id: respondent_group_channel.channel_id,
-              mode: respondent_group_channel.mode
-            }
-          )
-          |> Repo.insert!()
-        end
-      )
-    end)
-  end
-
   def new_wave(panel_survey) do
     latest_wave = PanelSurvey.latest_wave(panel_survey)
       |> Repo.preload([:project])

lib/ask/runtime/respondent_group_action.ex

@@ -139,8 +139,9 @@ defmodule Ask.Runtime.RespondentGroupAction do
     Enum.map(loaded_entries, fn %{phone_number: phone_number} -> phone_number end)
   end
 
-  def loaded_phone_numbers(phone_numbers),
-    do: Enum.map(phone_numbers, fn phone_number -> %{phone_number: phone_number} end)
+  def loaded_phone_numbers(phone_numbers) do
+    Enum.map(phone_numbers, fn phone_number -> %{phone_number: phone_number} end)
+  end
 
   def merge_sample(old_sample, loaded_entries) do
     new_sample = old_sample ++ entries_for_sample(loaded_entries)
@@ -323,4 +324,58 @@ defmodule Ask.Runtime.RespondentGroupAction do
       }
     end)
   end
+
+  # For each existing group a new respondent group with the same name is created.
+  # Each respondent group has a copy of every respondent (except refused and
+  # ineligible) and channel association.
+  def clone_respondents_groups_into(respondent_groups, survey) do
+    groups_phone_numbers = phone_numbers_by_group_id(respondent_groups)
+
+    Enum.map(respondent_groups, fn group ->
+      loaded_phone_numbers = groups_phone_numbers
+        |> Map.get(group.id)
+        |> loaded_phone_numbers()
+
+      new_group = create(group.name, loaded_phone_numbers, survey)
+      copy_respondent_group_channels(group, new_group)
+
+      new_group
+    end)
+  end
+
+  defp phone_numbers_by_group_id(respondent_groups) do
+    groups_ids = respondent_groups |> Enum.map(& &1.id)
+
+    (from r in Respondent,
+      where:
+        r.respondent_group_id in ^groups_ids and
+        r.disposition != :refused and
+        r.disposition != :ineligible,
+      select: [r.respondent_group_id, r.phone_number])
+      |> Repo.all()
+      |> Enum.group_by(&List.first/1, &List.last/1) # group phone_numbers by respondent_group_id
+  end
+
+  defp copy_respondent_group_channels(respondent_group, new_respondent_group) do
+    respondent_group =
+      respondent_group
+      |> Repo.preload(:respondent_group_channels)
+
+    Repo.transaction(fn ->
+      Enum.each(
+        respondent_group.respondent_group_channels,
+        fn respondent_group_channel ->
+          RespondentGroupChannel.changeset(
+            %RespondentGroupChannel{},
+            %{
+              respondent_group_id: new_respondent_group.id,
+              channel_id: respondent_group_channel.channel_id,
+              mode: respondent_group_channel.mode
+            }
+          )
+          |> Repo.insert!()
+        end
+      )
+    end)
+  end
 end

test/lib/runtime/panel_survey_test.exs

@@ -254,17 +254,18 @@ defmodule Ask.Runtime.PanelSurveyTest do
       survey
       |> Repo.preload(respondents: [respondent_group: [respondent_group_channels: :channel]])
 
-    Enum.map(survey.respondents, fn %{
-                                      hashed_number: hashed_number,
-                                      respondent_group: respondent_group
-                                    } ->
+    respondent_channels = Enum.map(survey.respondents, fn %{ hashed_number: hashed_number, respondent_group: respondent_group } ->
       respondent_group_channels =
         Enum.map(respondent_group.respondent_group_channels, fn %{channel: channel, mode: mode} ->
           %{channel_id: channel.id, mode: mode}
         end)
 
-      %{hashed_number: hashed_number, respondent_group_channels: respondent_group_channels}
+      { hashed_number, respondent_group_channels }
     end)
+
+    # We sort the response by hashed_number so that the function
+    # is deterministic enough to be used to compare lists using ==
+    List.keysort(respondent_channels, 0)
   end
 
   defp assert_incentives_enabled(survey) do