From 520ea0df064074cf286cf5f71088b41afdffd097 Mon Sep 17 00:00:00 2001 From: Franciscello Date: Fri, 10 Dec 2021 07:44:13 -0300 Subject: [PATCH] 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 | 67 +++++----------------- lib/ask/runtime/respondent_group_action.ex | 59 ++++++++++++++++++- test/lib/runtime/panel_survey_test.exs | 11 ++-- 3 files changed, 76 insertions(+), 61 deletions(-) diff --git a/lib/ask/runtime/panel_survey.ex b/lib/ask/runtime/panel_survey.ex index 64d794e09..d99edf103 100644 --- a/lib/ask/runtime/panel_survey.ex +++ b/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]) diff --git a/lib/ask/runtime/respondent_group_action.ex b/lib/ask/runtime/respondent_group_action.ex index f9da7b555..337cdaa48 100644 --- a/lib/ask/runtime/respondent_group_action.ex +++ b/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 diff --git a/test/lib/runtime/panel_survey_test.exs b/test/lib/runtime/panel_survey_test.exs index b0fbda45b..312c7ae66 100644 --- a/test/lib/runtime/panel_survey_test.exs +++ b/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