invert logic in assign relay

Author: Matthew Peck

lib/cog/command/pipeline/executor.ex

@@ -228,13 +228,9 @@ defmodule Cog.Command.Pipeline.Executor do
     #   fail_pipeline_with_error({:disabled_bundle, bundle}, state)
     #
     case assign_relay(relays, bundle_name, version) do
-      {nil, _} ->
-        fail_pipeline_with_error({:no_relays, bundle_name}, state)
-      {:no_relay_group, _} ->
-        fail_pipeline_with_error({:no_relay_group, bundle_name}, state)
-      {:no_enabled_relays, _} ->
-        fail_pipeline_with_error({:no_enabled_relays, bundle_name}, state)
-      {relay, relays} ->
+      {:error, reason} ->
+        fail_pipeline_with_error({reason, bundle_name}, state)
+      {:ok, {relay, relays}} ->
         topic = "/bot/commands/#{relay}/#{bundle_name}/#{command_name}"
         reply_to_topic = "#{state.topic}/reply"
         req = request_for_plan(current_plan, request, user, reply_to_topic, state.service_token)
@@ -671,35 +667,31 @@ defmodule Cog.Command.Pipeline.Executor do
   end
 
   defp assign_relay(relays, bundle_name, bundle_version) do
-    # If the bundle is assigned to a relay group carry on, otherwise bail
-    if Cog.Repository.Bundles.assigned_to_group?(bundle_name) do
-      case Map.get(relays, bundle_name) do
-        # No assignment so let's pick one
-        nil ->
-          case Relays.pick_one(bundle_name, bundle_version) do
-            # No relays available
-            {:error, :no_relays} ->
-              {nil, relays}
-            # relays exist, but are disabled
-            {:error, :no_enabled_relays} ->
-              {:no_enabled_relays, relays}
-            # Store the selected relay in the relay cache
-            {:ok, relay} ->
-              {relay, Map.put(relays, bundle_name, relay)}
-          end
-        # Relay was previously assigned
-        relay ->
-          # Is the bundle still available on the relay? If not, remove the current assignment from the cache
-          # and select a new relay
-          if Relays.relay_available?(relay, bundle_name, bundle_version) do
-            {relay, relays}
-          else
-            relays = Map.delete(relays, bundle_name)
-            assign_relay(relays, bundle_name, bundle_version)
-          end
-      end
-    else
-      {:no_relay_group, relays}
+    case Map.get(relays, bundle_name) do
+      # No assignment so let's pick one
+      nil ->
+        case Relays.pick_one(bundle_name, bundle_version) do
+          # Store the selected relay in the relay cache
+          {:ok, relay} ->
+            {:ok, {relay, Map.put(relays, bundle_name, relay)}}
+          error ->
+            # Query DB to clarify error before reporting to the user
+            if Cog.Repository.Bundles.assigned_to_group?(bundle_name) do
+              error
+            else
+              {:error, :no_relay_group}
+            end
+        end
+      # Relay was previously assigned
+      relay ->
+        # Is the bundle still available on the relay? If not, remove the current assignment from the cache
+        # and select a new relay
+        if Relays.relay_available?(relay, bundle_name, bundle_version) do
+          {:ok, {relay, relays}}
+        else
+          relays = Map.delete(relays, bundle_name)
+          assign_relay(relays, bundle_name, bundle_version)
+        end
     end
   end
 

lib/cog/relay/tracker.ex

@@ -120,10 +120,10 @@ defmodule Cog.Relay.Tracker do
   end
 
   @doc """
-  Return a tuple with a list of ALL relays serving the bundle and a list of the
-  ENABLED relays serving the bundle.
+  Return a tuple with the list of relays serving the bundle or an error and
+  a reason.
   """
-  @spec relays(t, bundle_name, version) :: {[String.t], [String.t]}
+  @spec relays(t, bundle_name, version) :: {:ok, [String.t]} | {:error, atom()}
   def relays(tracker, bundle_name, bundle_version) when is_binary(bundle_name) do
     relays = Map.get(tracker.map, {bundle_name, bundle_version}, MapSet.new)
     enabled_relays = MapSet.difference(relays, tracker.disabled)