refactor: FCM.ResultParser

Moved FCM result parsing into it's own module for
readability. Also cleaned up various linting warnings.

lib/pigeon/fcm/result_parser.ex

@@ -0,0 +1,65 @@
+defmodule Pigeon.FCM.ResultParser do
+  @moduledoc false
+
+  alias Pigeon.FCM.NotificationResponse
+
+  def parse([], [], on_response, result) do
+    on_response.({:ok, result})
+  end
+
+  def parse(regid, results, on_response, result) when is_binary(regid) do
+    parse([regid], results, on_response, result)
+  end
+
+  # Handle RegID updates
+  def parse([regid | reg_res],
+            [%{"message_id" => id, "registration_id" => new_regid} | rest_results],
+            on_response,
+            %NotificationResponse{ update: update} =  resp) do
+
+    new_updates = [{regid, new_regid} | update]
+    parse(reg_res, rest_results, on_response, %{resp | message_id: id, update: new_updates})
+  end
+
+  # Handle successful RegIDs, also parse `message_id`
+  def parse([regid | reg_res],
+            [%{"message_id" => id} | rest_results],
+            on_response,
+            %NotificationResponse{ok: ok} = resp) do
+
+    parse(reg_res, rest_results, on_response, %{resp | message_id: id, ok: [regid | ok]})
+  end
+
+  # Retry `Unavailable` RegIDs
+  def parse([regid | reg_res],
+            [%{"error" => "Unavailable"} | rest_results],
+            on_response,
+            %NotificationResponse{retry: retry} = resp) do
+
+    parse(reg_res, rest_results, on_response, %{resp | retry: [regid | retry]})
+  end
+
+  # Remove `NotRegistered` or `InvalidRegistration` RegIDs
+  def parse([regid | reg_res],
+            [%{"error" => invalid} | rest_results],
+            on_response,
+            %NotificationResponse{remove: remove} = resp) when invalid == "NotRegistered"
+                                                            or invalid == "InvalidRegistration" do
+
+    parse(reg_res, rest_results, on_response, %{resp | remove: [regid | remove]})
+  end
+
+  # Handle all other error keys
+  def parse([regid | reg_res] = regs,
+            [%{"error" => error} | rest_results] = results,
+            on_response,
+            %NotificationResponse{error: regs_in_error} = resp) do
+
+    if Map.has_key?(regs_in_error, error) do
+      parse(reg_res, rest_results, on_response, %{resp | error: %{regs_in_error | error => regid}})
+    else
+      # create map key if required.
+      parse(regs, results, on_response, %{resp | error: Map.merge(%{error => []}, regs_in_error)})
+    end
+  end
+end

lib/pigeon/fcm/worker.ex

@@ -5,7 +5,7 @@ defmodule Pigeon.FCM.Worker do
   use GenServer
   require Logger
 
-  alias Pigeon.FCM.NotificationResponse
+  alias Pigeon.FCM.{NotificationResponse, ResultParser}
 
   @ping_period 600_000 # 10 minutes
 
@@ -169,7 +169,7 @@ defmodule Pigeon.FCM.Worker do
     {registration_ids, on_response} = queue["#{stream_id}"]
     case get_status(headers) do
       "200" ->
-        result =  Poison.decode! body
+        result = Poison.decode!(body)
         parse_result(registration_ids, result, on_response)
         new_queue = Map.delete(queue, "#{stream_id}")
         {:noreply, %{state | queue: new_queue}}
@@ -194,77 +194,13 @@ defmodule Pigeon.FCM.Worker do
     end
   end
 
-  #def handle_info({:ok, _from}, state), do: {:noreply, state}
-
   # no on_response callback, ignore
   def parse_result(_, _, nil), do: :ok
 
   def parse_result(ids, %{"results" => results}, on_response) do
-    parse_result1(ids, results, on_response, %NotificationResponse{})
-  end
-
-  def parse_result1([], [], on_response, result) do
-    on_response.({:ok, result})
-  end
-
-  def parse_result1(regid, results, on_response, result) when is_binary(regid) do
-    parse_result1([regid], results, on_response, result)
-  end
-
-  def parse_result1([regid | reg_res],
-                    [%{"message_id" => id, "registration_id" => new_regid} | rest_results],
-                    on_response,
-                    %NotificationResponse{ update: update} =  resp) do
-
-    new_updates = [{regid, new_regid} | update]
-    parse_result1(reg_res, rest_results, on_response, %{resp | message_id: id, update: new_updates})
-  end
-
-  def parse_result1([regid | reg_res],
-                    [%{"message_id" => id} | rest_results],
-                    on_response,
-                    %NotificationResponse{ok: ok} = resp) do
-
-    parse_result1(reg_res, rest_results, on_response, %{resp | message_id: id, ok: [regid | ok]})
+    ResultParser.parse(ids, results, on_response, %NotificationResponse{})
   end
 
-  def parse_result1([regid | reg_res],
-                    [%{"error" => "Unavailable"} | rest_results],
-                    on_response,
-                    %NotificationResponse{retry: retry} = resp) do
-
-    parse_result1(reg_res, rest_results, on_response, %{resp | retry: [regid | retry]})
-  end
-
-  def parse_result1([regid | reg_res],
-                    [%{"error" => invalid } | rest_results],
-                    on_response,
-                    %NotificationResponse{remove: remove} = resp) when invalid == "NotRegistered"
-                                                                    or invalid == "InvalidRegistration" do
-
-    parse_result1(reg_res, rest_results, on_response, %{resp | remove: [regid | remove]})
-  end
-
-  def parse_result1([regid | reg_res] = regs,
-                    [%{"error" => error} | rest_results] = results,
-                    on_response,
-                    %NotificationResponse{error: regs_in_error} = resp) do
-
-    case Map.has_key?(regs_in_error, error) do
-      true ->
-        parse_result1(reg_res, rest_results, on_response,
-          %{resp | error: %{regs_in_error | error => regid}})
-      false -> # create map key if required.
-         parse_result1(regs, results, on_response,
-          %{resp | error: Map.merge(%{error => []}, regs_in_error)})
-    end
-  end
-
-  # def parse_result1(regs, [%{"error" => _error} | _r] = _results, on_response,
-  #     %NotificationResponse{error: _errors} = resp) do
-
-  # end
-
   defp get_status(headers) do
     case Enum.find(headers, fn({key, _val}) -> key == ":status" end) do
       {":status", status} -> status

test/fcm/result_parser_test.exs

@@ -0,0 +1,49 @@
+defmodule Pigeon.FCM.ResultParserTest do
+  use ExUnit.Case
+
+  alias Pigeon.FCM.{NotificationResponse, ResultParser}
+
+  test "parse_result with success" do
+    {:ok, response} =
+      ResultParser.parse(
+        ["regid"],
+        [%{ "message_id" => "1:0408" }],
+        &(&1), %NotificationResponse{}
+      )
+    assert  response.ok == ["regid"]
+  end
+
+  test "parse_result with success and new registration_id" do
+    {:ok, response} =
+      ResultParser.parse(
+        ["regid"],
+        [%{ "message_id" => "1:2342", "registration_id" => "32" }],
+        &(&1), %NotificationResponse{}
+      )
+
+    assert response.update == [{"regid", "32"}]
+    assert response.message_id == "1:2342"
+  end
+
+  test "parse_result with error unavailable" do
+    {:ok, response} =
+      ResultParser.parse(
+        ["regid"],
+        [%{ "error" => "Unavailable" }],
+        &(&1),
+        %NotificationResponse{}
+      )
+    assert response.retry == ["regid"]
+  end
+
+  test "parse_result with custom error" do
+    {:ok, response} =
+      ResultParser.parse(
+        ["regid"],
+        [%{ "error" => "CustomError" }],
+        &(&1),
+        %NotificationResponse{}
+      )
+    assert response.error == %{"CustomError" => "regid"}
+  end
+end

test/fcm/worker_test.exs

@@ -1,65 +1,20 @@
 defmodule Pigeon.FCM.WorkerTest do
   use ExUnit.Case
-  alias Pigeon.FCM
-  alias Pigeon.FCM.{NotificationResponse}
-
-  @data %{"message" => "Test push"}
-  @payload %{"data" => @data}
 
-  defp valid_fcm_reg_id, do: Application.get_env(:pigeon, :test)[:valid_fcm_reg_id]
-
-  test "parse_result with success" do
-    {:ok, response} =
-      FCM.Worker.parse_result1(
-        ["regid"],
-        [%{ "message_id" => "1:0408" }],
-        &(&1), %NotificationResponse{}
-      )
-    assert  response.ok == ["regid"]
-  end
-
-  test "parse_result with success and new registration_id" do
-    {:ok, response} =
-      FCM.Worker.parse_result1(
-        ["regid"],
-        [%{ "message_id" => "1:2342", "registration_id" => "32" }],
-        &(&1), %NotificationResponse{}
-      )
-
-    assert response.update == [{"regid", "32"}]
-    assert response.message_id == "1:2342"
-  end
-
-  test "parse_result with error unavailable" do
-    {:ok, response} =
-      FCM.Worker.parse_result1(
-        ["regid"],
-        [%{ "error" => "Unavailable" }],
-        &(&1),
-        %NotificationResponse{}
-      )
-    assert response.retry == ["regid"]
-  end
-
-  test "parse_result with custom error" do
-    {:ok, response} =
-      FCM.Worker.parse_result1(
-        ["regid"],
-        [%{ "error" => "CustomError" }],
-        &(&1),
-        %NotificationResponse{}
-      )
-    assert response.error == %{"CustomError" => "regid"}
-  end
+  alias Pigeon.FCM
 
   test "send malformed JSON" do
-    #{:ok, pid} = FCM.Worker.start_link(:gonecrashing, key: Application.get_env(:pigeon, :fcm)[:key])
-    opts = [name: :gonecrashing, key: Application.get_env(:pigeon, :test)[:fcm_key]]
+    opts = [
+      name: :gonecrashing,
+      key: Application.get_env(:pigeon, :test)[:fcm_key]
+    ]
     {:ok, pid} = FCM.start_connection(opts)
 
     me = self()
-    :gen_server.cast(pid, {:push, :fcm, {"toto", "this is not json"}, &(send me, &1), %{}})
+    bad = {"toto", "this is not json"}
+    :gen_server.cast(pid, {:push, :fcm, bad, &(send me, &1), %{}})
     assert_receive {:error, :malformed_json}, 5000
+
     :gen_server.cast(pid, :stop)
   end
 end

test/fcm_test.exs

@@ -6,9 +6,10 @@ defmodule Pigeon.FCMTest do
   require Logger
 
   @data %{"message" => "Test push"}
-  @payload %{"data" => @data}
 
-  defp valid_fcm_reg_id, do: Application.get_env(:pigeon, :test)[:valid_fcm_reg_id]
+  defp valid_fcm_reg_id do
+    Application.get_env(:pigeon, :test)[:valid_fcm_reg_id]
+  end
 
   describe "start_connection/1" do
     test "starts conneciton with opts keyword list" do
@@ -72,7 +73,7 @@ defmodule Pigeon.FCMTest do
     nr2 = %NotificationResponse{
       ok: ["43"],
       update: ["12"],
-      error: %{"error" => ["2"], "error2"=> ["1"]}
+      error: %{"error" => ["2"], "error2" => ["1"]}
     }
     assert Pigeon.FCM.merge(nr1, nr2) ==
       %NotificationResponse{
@@ -86,14 +87,14 @@ defmodule Pigeon.FCMTest do
   test "Message for less than 1000 recipients should not be chunked" do
     regs = Enum.to_list(1..999)
     notification = Notification.new(regs, %{}, @data)
-    assert [{^regs, encoded}] = res = Pigeon.FCM.encode_requests(notification)
+    assert [{^regs, _encoded}] = Pigeon.FCM.encode_requests(notification)
   end
 
   test "Message for over 1000 recipients should be chunked" do
     regs = Enum.to_list(1..2534)
     notification = Notification.new(regs, %{}, @data)
     res = Pigeon.FCM.encode_requests(notification)
-    assert [{r1, e1}, {r2, e2}, {r3, e3}]  = res
+    assert [{r1, _e1}, {r2, _e2}, {r3, _e3}]  = res
     assert length(r1) == 1000
     assert length(r2) == 1000
     assert length(r3) == 534
@@ -119,12 +120,12 @@ defmodule Pigeon.FCMTest do
   end
 
   test "returns an error on pushing with a bad registration_id" do
-    reg_id = "bad_registration_id"
+    reg_id = "bad_reg_id"
     n = Notification.new(reg_id, %{}, @data)
     pid = self()
     Pigeon.FCM.send_push(n, fn(x) -> send pid, x end, %{})
 
-    assert_receive {:ok, %Pigeon.FCM.NotificationResponse{remove: ["bad_registration_id"]}}, 5000
+    assert_receive {:ok, %NotificationResponse{remove: [^reg_id]}}, 5000
     assert n.registration_id == reg_id
     assert n.payload == %{"data" => @data}
   end
@@ -137,9 +138,9 @@ defmodule Pigeon.FCMTest do
   end
 
   test "encode_requests with multiple registration_ids" do
-    registration_id = ["aaaaaa", "bbbbbb", "cccccc"]
-    payload = Notification.new(registration_id, %{},@data)
-    expected = ~S({"registration_ids":["aaaaaa","bbbbbb","cccccc"],"priority":"normal","data":{"message":"Test push"}})
+    registration_id = ["a", "b", "c"]
+    payload = Notification.new(registration_id, %{}, %{"k" => "v"})
+    expected = ~S({"registration_ids":["a","b","c"],"priority":"normal","data":{"k":"v"}})
     assert Pigeon.FCM.encode_requests(payload) == [{registration_id, expected}]
   end
 end