credo cleanup

TODOS.md

@@ -0,0 +1,5 @@
+* change "revoke" agent to delete
+* add ability to rename an agent in a new edit page
+* When adding a new device, allow the community string to be empty so it will inherit from the parent
+* add and track ICMP pings for each device
+* when adding a new device, make it a tab selector to show the default SNMP & ICMP or ICMP only

config/config.exs

@@ -20,7 +20,17 @@ config :esbuild,
 # Configure Elixir's Logger
 config :logger, :default_formatter,
   format: "$time $metadata[$level] $message\n",
-  metadata: [:request_id]
+  metadata: [
+    :request_id,
+    :status,
+    :duration_ms,
+    :kind,
+    :reason,
+    :stacktrace,
+    :agent_token_id,
+    :device_id,
+    :error
+  ]
 
 # Filter out noisy errors from port scanners and bots
 config :logger, :default_handler,

lib/towerops/agents/stats.ex

@@ -321,8 +321,8 @@ defmodule Towerops.Agents.Stats do
 
   defp get_batch_device_counts(agent_token_ids) do
     # For now, use the existing function per agent since list_agent_polling_targets
-    # handles complex hierarchical logic in application code
-    # TODO: Optimize this with a pure SQL solution if it becomes a bottleneck
+    # handles complex hierarchical logic in application code.
+    # NOTE: This could be optimized with a pure SQL solution if performance becomes a concern.
     Map.new(agent_token_ids, fn id -> {id, get_agent_device_count(id)} end)
   end
 

lib/towerops/devices.ex

@@ -160,53 +160,51 @@ defmodule Towerops.Devices do
   end
 
   def get_snmp_config(%DeviceSchema{} = device) do
-    # Ensure associations are loaded
-    device =
-      if Ecto.assoc_loaded?(device.site) do
-        if Ecto.assoc_loaded?(device.site.organization) do
-          device
-        else
-          Repo.preload(device, site: :organization)
-        end
-      else
-        Repo.preload(device, site: :organization)
-      end
+    device = ensure_snmp_config_loaded(device)
+    resolve_snmp_config(device)
+  end
 
-    cond do
-      # Device-level override
-      device.snmp_community != nil ->
-        %{
-          version: device.snmp_version || "2c",
-          community: device.snmp_community,
-          source: :device
-        }
+  defp ensure_snmp_config_loaded(device) do
+    # Ensure site and organization associations are loaded
+    needs_preload =
+      !Ecto.assoc_loaded?(device.site) ||
+        !Ecto.assoc_loaded?(device.site.organization)
 
-      # Site-level override
-      device.site.snmp_community != nil ->
-        %{
-          version: device.site.snmp_version || "2c",
-          community: device.site.snmp_community,
-          source: :site
-        }
-
-      # Organization-level default
-      device.site.organization.snmp_community != nil ->
-        %{
-          version: device.site.organization.snmp_version || "2c",
-          community: device.site.organization.snmp_community,
-          source: :organization
-        }
-
-      # No SNMP config set at any level
-      true ->
-        %{
-          version: "2c",
-          community: nil,
-          source: :default
-        }
+    if needs_preload do
+      Repo.preload(device, site: :organization)
+    else
+      device
     end
   end
 
+  defp resolve_snmp_config(device) do
+    cond do
+      device.snmp_community != nil ->
+        build_snmp_config(device.snmp_version, device.snmp_community, :device)
+
+      device.site.snmp_community != nil ->
+        build_snmp_config(device.site.snmp_version, device.site.snmp_community, :site)
+
+      device.site.organization.snmp_community != nil ->
+        build_snmp_config(
+          device.site.organization.snmp_version,
+          device.site.organization.snmp_community,
+          :organization
+        )
+
+      true ->
+        %{version: "2c", community: nil, source: :default}
+    end
+  end
+
+  defp build_snmp_config(version, community, source) do
+    %{
+      version: version || "2c",
+      community: community,
+      source: source
+    }
+  end
+
   @doc """
   Creates device.
   """

lib/towerops/mobile_sessions.ex

@@ -173,30 +173,29 @@ defmodule Towerops.MobileSessions do
   """
   def complete_qr_login(token, device_attrs) do
     Repo.transaction(fn ->
-      case get_qr_login_token(token) do
-        nil ->
-          Repo.rollback(:invalid_token)
+      with {:ok, qr_token} <- fetch_qr_token(token),
+           session_attrs = Map.put(device_attrs, :user_id, qr_token.user_id),
+           {:ok, session} <- create_mobile_session(session_attrs) do
+        # Mark QR token as completed
+        qr_token
+        |> QRLoginToken.complete_changeset(session.id)
+        |> Repo.update!()
 
-        qr_token ->
-          # Create mobile session
-          session_attrs = Map.put(device_attrs, :user_id, qr_token.user_id)
-
-          case create_mobile_session(session_attrs) do
-            {:ok, session} ->
-              # Mark QR token as completed
-              qr_token
-              |> QRLoginToken.complete_changeset(session.id)
-              |> Repo.update!()
-
-              session
-
-            {:error, changeset} ->
-              Repo.rollback(changeset)
-          end
+        session
+      else
+        {:error, :invalid_token} -> Repo.rollback(:invalid_token)
+        {:error, changeset} -> Repo.rollback(changeset)
       end
     end)
   end
 
+  defp fetch_qr_token(token) do
+    case get_qr_login_token(token) do
+      nil -> {:error, :invalid_token}
+      qr_token -> {:ok, qr_token}
+    end
+  end
+
   @doc """
   Deletes expired QR login tokens.
 

lib/towerops/repo.ex

@@ -1,4 +1,6 @@
 defmodule Towerops.Repo do
+  @moduledoc false
+
   use Ecto.Repo,
     otp_app: :towerops,
     adapter: Ecto.Adapters.Postgres

lib/towerops/snmp.ex

@@ -376,36 +376,37 @@ defmodule Towerops.Snmp do
 
   # device in the organization
   defp find_matching_equipment(neighbor, organization_id) do
-    # Try matching by chassis ID (MAC address) first
-    equipment_by_chassis =
-      if neighbor.remote_chassis_id && is_mac_address?(neighbor.remote_chassis_id) do
-        find_device_by_mac(neighbor.remote_chassis_id, organization_id)
-      end
+    # Try matching strategies in order: chassis ID, port ID, system name
+    try_match_by_chassis(neighbor, organization_id) ||
+      try_match_by_port(neighbor, organization_id) ||
+      try_match_by_name(neighbor, organization_id)
+  end
 
-    # Try matching by port ID (often contains MAC address in LLDP)
-    equipment_by_port =
-      if is_nil(equipment_by_chassis) && neighbor.remote_port_id &&
-           is_mac_address?(neighbor.remote_port_id) do
-        find_device_by_mac(neighbor.remote_port_id, organization_id)
-      end
+  defp try_match_by_chassis(neighbor, organization_id) do
+    if neighbor.remote_chassis_id && mac_address?(neighbor.remote_chassis_id) do
+      find_device_by_mac(neighbor.remote_chassis_id, organization_id)
+    end
+  end
 
-    # Try matching by system name if MAC matches didn't work
-    equipment_by_name =
-      if is_nil(equipment_by_chassis) && is_nil(equipment_by_port) &&
-           neighbor.remote_system_name do
-        find_device_by_name(neighbor.remote_system_name, organization_id)
-      end
+  defp try_match_by_port(neighbor, organization_id) do
+    if neighbor.remote_port_id && mac_address?(neighbor.remote_port_id) do
+      find_device_by_mac(neighbor.remote_port_id, organization_id)
+    end
+  end
 
-    equipment_by_chassis || equipment_by_port || equipment_by_name
+  defp try_match_by_name(neighbor, organization_id) do
+    if neighbor.remote_system_name do
+      find_device_by_name(neighbor.remote_system_name, organization_id)
+    end
   end
 
   # Check if a string looks like a MAC address
-  defp is_mac_address?(str) when is_binary(str) do
+  defp mac_address?(str) when is_binary(str) do
     # Match patterns like "AA:BB:CC:DD:EE:FF" or "aa-bb-cc-dd-ee-ff"
     String.match?(str, ~r/^([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}$/)
   end
 
-  defp is_mac_address?(_), do: false
+  defp mac_address?(_), do: false
 
   defp find_device_by_mac(mac_address, organization_id) do
     # MAC address might be in format "aa:bb:cc:dd:ee:ff", "aa-bb-cc-dd-ee-ff", or "aabbccddeeff"

lib/towerops/snmp/profiles/net_snmp.ex

@@ -204,25 +204,8 @@ defmodule Towerops.Snmp.Profiles.NetSnmp do
       {:ok, [total, available]} ->
         total_kb = parse_integer(total)
         available_kb = parse_integer(available)
-        used_kb = if total_kb && available_kb, do: total_kb - available_kb
-
-        used_percent =
-          if total_kb && total_kb > 0 && used_kb do
-            percent = used_kb / total_kb * 100
-
-            # Cap percentage at 100% and log warning if exceeded
-            if percent > 100 do
-              Logger.warning(
-                "Memory usage exceeded 100%: #{Float.round(percent, 2)}% " <>
-                  "(used=#{used_kb} KB, total=#{total_kb} KB). " <>
-                  "Capping at 100%. This may indicate a device firmware bug."
-              )
-
-              100.0
-            else
-              percent
-            end
-          end
+        used_kb = calculate_used_memory(total_kb, available_kb)
+        used_percent = calculate_memory_percentage(total_kb, used_kb)
 
         [
           %{
@@ -242,6 +225,29 @@ defmodule Towerops.Snmp.Profiles.NetSnmp do
     end
   end
 
+  defp calculate_used_memory(nil, _available_kb), do: nil
+  defp calculate_used_memory(_total_kb, nil), do: nil
+  defp calculate_used_memory(total_kb, available_kb), do: total_kb - available_kb
+
+  defp calculate_memory_percentage(total_kb, used_kb) when is_number(total_kb) and total_kb > 0 and is_number(used_kb) do
+    percent = used_kb / total_kb * 100
+    cap_memory_percentage(percent, used_kb, total_kb)
+  end
+
+  defp calculate_memory_percentage(_total_kb, _used_kb), do: nil
+
+  defp cap_memory_percentage(percent, _used_kb, _total_kb) when percent <= 100, do: percent
+
+  defp cap_memory_percentage(percent, used_kb, total_kb) do
+    Logger.warning(
+      "Memory usage exceeded 100%: #{Float.round(percent, 2)}% " <>
+        "(used=#{used_kb} KB, total=#{total_kb} KB). " <>
+        "Capping at 100%. This may indicate a device firmware bug."
+    )
+
+    100.0
+  end
+
   defp extract_linux_info(sys_descr) do
     cond do
       String.contains?(sys_descr, "Ubuntu") ->

lib/towerops_web/channels/agent_channel.ex

@@ -272,23 +272,41 @@ defmodule ToweropsWeb.AgentChannel do
   end
 
   defp process_snmp_result(organization_id, result) do
-    case Devices.get_device_with_details(result.device_id) do
-      nil ->
+    with {:ok, device} <- fetch_device(result.device_id),
+         :ok <- verify_device_organization(device, organization_id) do
+      process_job_result(device, result)
+    else
+      {:error, :device_not_found} ->
         Logger.error("Device not found: #{result.device_id}")
 
-      device ->
-        # Verify device belongs to agent's organization
-        if device.site.organization_id == organization_id do
-          case result.job_type do
-            :DISCOVER -> process_discovery_result(device, result)
-            :POLL -> process_polling_result(device, result)
-          end
-        else
-          Logger.error("Device #{result.device_id} not in agent's organization")
-        end
+      {:error, :wrong_organization} ->
+        Logger.error("Device #{result.device_id} not in agent's organization")
     end
   end
 
+  defp fetch_device(device_id) do
+    case Devices.get_device_with_details(device_id) do
+      nil -> {:error, :device_not_found}
+      device -> {:ok, device}
+    end
+  end
+
+  defp verify_device_organization(device, organization_id) do
+    if device.site.organization_id == organization_id do
+      :ok
+    else
+      {:error, :wrong_organization}
+    end
+  end
+
+  defp process_job_result(device, %{job_type: :DISCOVER} = result) do
+    process_discovery_result(device, result)
+  end
+
+  defp process_job_result(device, %{job_type: :POLL} = result) do
+    process_polling_result(device, result)
+  end
+
   defp process_discovery_result(device, _result) do
     # Agent confirmed device is reachable, trigger full discovery from server
     # This hybrid approach simplifies the initial implementation:

lib/towerops_web/controllers/health_controller.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.HealthController do
+  @moduledoc false
+
   use ToweropsWeb, :controller
 
   alias Ecto.Adapters.SQL

lib/towerops_web/controllers/page_controller.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.PageController do
+  @moduledoc false
+
   use ToweropsWeb, :controller
 
   def home(conn, _params) do

lib/towerops_web/controllers/user_credential_controller.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserCredentialController do
+  @moduledoc false
+
   use ToweropsWeb, :controller
 
   alias Towerops.Accounts

lib/towerops_web/controllers/user_registration_controller.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserRegistrationController do
+  @moduledoc false
+
   use ToweropsWeb, :controller
 
   alias Towerops.Accounts

lib/towerops_web/controllers/user_registration_html.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserRegistrationHTML do
+  @moduledoc false
+
   use ToweropsWeb, :html
 
   embed_templates "user_registration_html/*"

lib/towerops_web/controllers/user_session_controller.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserSessionController do
+  @moduledoc false
+
   use ToweropsWeb, :controller
 
   alias Towerops.Accounts

lib/towerops_web/controllers/user_session_html.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserSessionHTML do
+  @moduledoc false
+
   use ToweropsWeb, :html
 
   embed_templates "user_session_html/*"

lib/towerops_web/controllers/user_settings_html.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.UserSettingsHTML do
+  @moduledoc false
+
   use ToweropsWeb, :html
 
   embed_templates "user_settings_html/*"

lib/towerops_web/live/device_live/show.ex

@@ -6,6 +6,32 @@ defmodule ToweropsWeb.DeviceLive.Show do
   alias Towerops.Monitoring
   alias Towerops.Snmp
 
+  # SNMP interface type to category mappings
+  @interface_type_categories %{
+    6 => "Ethernet",
+    24 => "Loopback",
+    23 => "PPP",
+    108 => "PPPoE",
+    131 => "Tunnel",
+    135 => "VLAN",
+    136 => "VLAN",
+    161 => "IEEE 802.11",
+    244 => "WWP"
+  }
+
+  # Display order for interface categories
+  @interface_category_order %{
+    "Ethernet" => 1,
+    "VLAN" => 2,
+    "IEEE 802.11" => 3,
+    "PPPoE" => 4,
+    "PPP" => 5,
+    "Tunnel" => 6,
+    "Loopback" => 7,
+    "WWP" => 8,
+    "Other" => 99
+  }
+
   @impl true
   def mount(_params, _session, socket) do
     {:ok, socket}
@@ -390,34 +416,13 @@ defmodule ToweropsWeb.DeviceLive.Show do
 
   # Map SNMP interface types to human-readable categories
   defp get_interface_type_category(if_type) when is_integer(if_type) do
-    case if_type do
-      6 -> "Ethernet"
-      24 -> "Loopback"
-      23 -> "PPP"
-      108 -> "PPPoE"
-      131 -> "Tunnel"
-      135 -> "VLAN"
-      136 -> "VLAN"
-      161 -> "IEEE 802.11"
-      244 -> "WWP"
-      _ -> "Other"
-    end
+    Map.get(@interface_type_categories, if_type, "Other")
   end
 
   defp get_interface_type_category(_), do: "Other"
 
   # Define display order for interface categories
   defp interface_type_order(category) do
-    case category do
-      "Ethernet" -> 1
-      "VLAN" -> 2
-      "IEEE 802.11" -> 3
-      "PPPoE" -> 4
-      "PPP" -> 5
-      "Tunnel" -> 6
-      "Loopback" -> 7
-      "WWP" -> 8
-      "Other" -> 99
-    end
+    Map.get(@interface_category_order, category, 99)
   end
 end

lib/towerops_web/router.ex

@@ -1,4 +1,6 @@
 defmodule ToweropsWeb.Router do
+  @moduledoc false
+
   use ToweropsWeb, :router
 
   import Phoenix.LiveDashboard.Router

test/towerops/snmp/discovery_test.exs

@@ -207,7 +207,8 @@ defmodule Towerops.Snmp.DiscoveryTest do
       end)
 
       # Mock interface discovery - no interfaces for simplicity
-      # Walk called 5 times: interfaces (1), cisco sensors (1), base sensors fallback (1), LLDP neighbors (1), CDP neighbors (1)
+      # Walk called 5 times: interfaces (1), cisco sensors (1), base sensors fallback (1),
+      # LLDP neighbors (1), CDP neighbors (1)
       expect(SnmpMock, :walk, 5, fn _, _, _ ->
         {:ok, []}
       end)