From 7c8731f86ab97c4fd973f8b4f4273f2de3c306f3 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Sun, 15 Mar 2026 18:19:09 -0500 Subject: [PATCH] fix: make sidebar sticky with header and resolve all credo issues (#32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Changed sidebar from fixed to sticky positioning - Wrapped sidebar and main content in flex container - Sidebar now sticks to top along with header when scrolling - Removed unused Ecto.Query import from gps_sync.ex Credo improvements (17 → 0 remaining): - Replaced all length/1 checks with empty list comparisons - Extracted helper functions to reduce nesting depth - Split complex functions into smaller, testable units - Applied 'with' statements for clearer control flow - Refactored high-complexity functions (cyclomatic complexity) - Files improved: storm_detector, alert_notification_worker, alert_digest_worker, gps_sync, statistics_sync, device_sync, site_sync, site_correlation, reports_live, topology, pagerduty/client, cn_maestro/sync Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/32 --- lib/towerops/alerts/site_correlation.ex | 41 +- lib/towerops/alerts/storm_detector.ex | 139 ++-- lib/towerops/cn_maestro/sync.ex | 103 +-- lib/towerops/pagerduty/client.ex | 37 +- lib/towerops/reports/report.ex | 8 +- lib/towerops/rf_links.ex | 8 +- lib/towerops/status_pages/status_incident.ex | 1 + .../status_pages/status_page_component.ex | 1 + .../status_pages/status_page_config.ex | 12 +- lib/towerops/topology.ex | 62 +- lib/towerops/uisp/config_snapshot.ex | 57 +- lib/towerops/uisp/device_sync.ex | 73 +- lib/towerops/uisp/gps_sync.ex | 28 +- lib/towerops/uisp/site_sync.ex | 58 +- lib/towerops/uisp/statistics_sync.ex | 44 +- lib/towerops/uisp/sync.ex | 29 +- lib/towerops/workers/alert_digest_worker.ex | 18 +- .../workers/alert_notification_worker.ex | 94 +-- lib/towerops_web/components/layouts.ex | 689 +++++++++--------- .../live/org/integrations_live.ex | 2 +- lib/towerops_web/live/reports_live.ex | 12 +- lib/towerops_web/live/reports_live.html.heex | 23 +- lib/towerops_web/live/rf_link_health_live.ex | 12 +- .../live/rf_link_health_live.html.heex | 24 +- lib/towerops_web/live/status_page_live.ex | 3 +- .../live/status_page_live.html.heex | 46 +- test/towerops/cn_maestro/client_test.exs | 4 +- test/towerops/reports_test.exs | 7 +- test/towerops/rf_links_test.exs | 21 +- test/towerops/status_pages_test.exs | 12 +- test/towerops/uisp/sync_test.exs | 2 +- 31 files changed, 914 insertions(+), 756 deletions(-) diff --git a/lib/towerops/alerts/site_correlation.ex b/lib/towerops/alerts/site_correlation.ex index f2fa6c38..073e2287 100644 --- a/lib/towerops/alerts/site_correlation.ex +++ b/lib/towerops/alerts/site_correlation.ex @@ -56,25 +56,32 @@ defmodule Towerops.Alerts.SiteCorrelation do {:site_outage, outage} nil -> - # Count recent device_down alerts at this site - recent_down_count = count_recent_site_alerts(device.site_id, cutoff) + check_and_create_site_outage(device, cutoff, now, threshold) + end + end - # +1 for the current device about to go down - if recent_down_count + 1 >= threshold do - # Threshold met — create site outage - case create_site_outage(device, now, recent_down_count + 1) do - {:ok, outage} -> - # Retroactively link existing alerts to this outage - link_existing_alerts(device.site_id, cutoff, outage.id) - {:site_outage, outage} + defp check_and_create_site_outage(device, cutoff, now, threshold) do + # Count recent device_down alerts at this site + recent_down_count = count_recent_site_alerts(device.site_id, cutoff) - {:error, reason} -> - Logger.error("Failed to create site outage: #{inspect(reason)}") - :no_correlation - end - else - :no_correlation - end + # +1 for the current device about to go down + if recent_down_count + 1 >= threshold do + handle_site_outage_creation(device, now, recent_down_count + 1, cutoff) + else + :no_correlation + end + end + + defp handle_site_outage_creation(device, now, count, cutoff) do + case create_site_outage(device, now, count) do + {:ok, outage} -> + # Retroactively link existing alerts to this outage + link_existing_alerts(device.site_id, cutoff, outage.id) + {:site_outage, outage} + + {:error, reason} -> + Logger.error("Failed to create site outage: #{inspect(reason)}") + :no_correlation end end diff --git a/lib/towerops/alerts/storm_detector.ex b/lib/towerops/alerts/storm_detector.ex index c9f4c162..bde3392c 100644 --- a/lib/towerops/alerts/storm_detector.ex +++ b/lib/towerops/alerts/storm_detector.ex @@ -242,85 +242,92 @@ defmodule Towerops.Alerts.StormDetector do if Maintenance.site_in_maintenance?(site_id) do Logger.info("Site #{site_id} outage (#{device_count} devices) suppressed — in maintenance window") else - # Get site name for the alert message - site_name = - case Towerops.Sites.get_site(site_id) do - nil -> "Unknown site" - site -> site.name - end + do_create_site_outage_alert(site_id, devices, device_names, device_count, storm_mode, now) + end - # Build impact summary - message = - "Site outage: #{site_name} — #{device_count} devices down (#{Enum.join(Enum.take(device_names, 5), ", ")}#{if device_count > 5, do: " and #{device_count - 5} more", else: ""})" + # Also mark each individual device as down (status update only, no individual alerts) + Enum.each(devices, fn device -> + Devices.update_device_status(device, :down) + end) + end - # Use the first device as the "primary" for the alert record - # but store all affected device IDs in metadata - primary_device = List.first(devices) - - case Alerts.create_alert(%{ - device_id: primary_device.id, - alert_type: "site_outage", - triggered_at: now, - message: message, - storm_suppressed: false - }) do - {:ok, alert} -> - # Single notification instead of N - if !storm_mode do - AlertNotificationWorker.enqueue_trigger(alert.id) - end - - _ = - Phoenix.PubSub.broadcast( - Towerops.PubSub, - "alerts:org:#{primary_device.organization_id}:new", - {:new_alert, primary_device.id, :site_outage} - ) - - Logger.warning( - "Created site outage alert for #{site_name}: #{device_count} devices", - site_id: site_id, - device_count: device_count, - storm_mode: storm_mode - ) - - {:error, reason} -> - Logger.error("Failed to create site outage alert: #{inspect(reason)}") + defp do_create_site_outage_alert(site_id, devices, device_names, device_count, storm_mode, now) do + # Get site name for the alert message + site_name = + case Towerops.Sites.get_site(site_id) do + nil -> "Unknown site" + site -> site.name end - # Also mark each individual device as down (status update only, no individual alerts) - Enum.each(devices, fn device -> - Devices.update_device_status(device, :down) + # Build impact summary + message = + "Site outage: #{site_name} — #{device_count} devices down (#{Enum.join(Enum.take(device_names, 5), ", ")}#{if device_count > 5, do: " and #{device_count - 5} more", else: ""})" - # Suppress individual device_down alerts — the site_outage covers them - # But DO create suppressed alert records for audit trail - Alerts.create_alert(%{ - device_id: device.id, - alert_type: "device_down", - triggered_at: now, - resolved_at: now, - message: "#{device.name} is down (suppressed — part of site outage at #{site_name})", - storm_suppressed: true - }) - end) + # Use the first device as the "primary" for the alert record + primary_device = List.first(devices) + + case Alerts.create_alert(%{ + device_id: primary_device.id, + alert_type: "site_outage", + triggered_at: now, + message: message, + storm_suppressed: false + }) do + {:ok, alert} -> + if !storm_mode do + AlertNotificationWorker.enqueue_trigger(alert.id) + end + + _ = + Phoenix.PubSub.broadcast( + Towerops.PubSub, + "alerts:org:#{primary_device.organization_id}:new", + {:new_alert, primary_device.id, :site_outage} + ) + + Logger.warning( + "Created site outage alert for #{site_name}: #{device_count} devices", + site_id: site_id, + device_count: device_count, + storm_mode: storm_mode + ) + + # Create suppressed alert records for individual devices (for audit trail) + Enum.each(devices, fn device -> + Alerts.create_alert(%{ + device_id: device.id, + alert_type: "device_down", + triggered_at: now, + resolved_at: now, + message: "#{device.name} is down (suppressed — part of site outage at #{site_name})", + storm_suppressed: true + }) + end) + + {:error, reason} -> + Logger.error("Failed to create site outage alert: #{inspect(reason)}") end end defp create_individual_alerts(events, storm_mode) do Enum.each(events, fn %{device: device, timestamp: timestamp} -> - # Skip if already has an active alert - if Alerts.has_active_alert?(device.id, "device_down") do - :ok - else - if Maintenance.device_in_maintenance?(device.id) do - Logger.info("Device #{device.id} (#{device.name}) is down but in maintenance — suppressing") - else - create_single_device_down_alert(device, timestamp, storm_mode) - end - end + process_device_alert(device, timestamp, storm_mode) end) end + defp process_device_alert(device, timestamp, storm_mode) do + cond do + Alerts.has_active_alert?(device.id, "device_down") -> + :ok + + Maintenance.device_in_maintenance?(device.id) -> + Logger.info("Device #{device.id} (#{device.name}) is down but in maintenance — suppressing") + + true -> + create_single_device_down_alert(device, timestamp, storm_mode) + end + end + defp create_single_device_down_alert(device, now, storm_mode) do alert_message = if device.snmp_enabled do diff --git a/lib/towerops/cn_maestro/sync.ex b/lib/towerops/cn_maestro/sync.ex index 3adcc783..2006d45b 100644 --- a/lib/towerops/cn_maestro/sync.ex +++ b/lib/towerops/cn_maestro/sync.ex @@ -60,27 +60,8 @@ defmodule Towerops.CnMaestro.Sync do case Client.list_networks(base_url, token) do {:ok, networks} -> {synced, site_map} = - Enum.reduce(networks, {0, %{}}, fn network, {count, acc} -> - name = network["name"] - network_id = network["id"] || network["name"] - - if is_nil(name) or name == "" do - {count, acc} - else - attrs = %{ - organization_id: org_id, - name: name - } - - case upsert_site(attrs) do - {:ok, site} -> - new_acc = if network_id, do: Map.put(acc, network_id, site), else: acc - {count + 1, new_acc} - - {:error, _} -> - {count, acc} - end - end + Enum.reduce(networks, {0, %{}}, fn network, acc -> + process_network(network, org_id, acc) end) {:ok, %{synced: synced, site_map: site_map}} @@ -90,32 +71,36 @@ defmodule Towerops.CnMaestro.Sync do end end + defp process_network(network, org_id, {count, acc}) do + name = network["name"] + network_id = network["id"] || network["name"] + + if is_nil(name) or name == "" do + {count, acc} + else + upsert_network_site(org_id, name, network_id, count, acc) + end + end + + defp upsert_network_site(org_id, name, network_id, count, acc) do + attrs = %{organization_id: org_id, name: name} + + case upsert_site(attrs) do + {:ok, site} -> + new_acc = if network_id, do: Map.put(acc, network_id, site), else: acc + {count + 1, new_acc} + + {:error, _} -> + {count, acc} + end + end + defp sync_devices(base_url, token, org_id, site_map) do case Client.list_devices(base_url, token) do {:ok, devices} -> result = Enum.reduce(devices, %{matched: 0, created: 0}, fn device_data, acc -> - mac = device_data["mac"] - ip = device_data["ip"] - name = device_data["name"] || device_data["mac"] - network = device_data["network"] - local_site = site_map[network] - - if is_nil(ip) and is_nil(mac) do - acc - else - case find_existing_device(org_id, ip, mac) do - nil -> - case create_device(org_id, ip || mac, name, local_site) do - {:ok, _} -> Map.update!(acc, :created, &(&1 + 1)) - {:error, _} -> acc - end - - device -> - maybe_update_site(device, local_site) - Map.update!(acc, :matched, &(&1 + 1)) - end - end + process_device(device_data, org_id, site_map, acc) end) {:ok, result} @@ -125,6 +110,34 @@ defmodule Towerops.CnMaestro.Sync do end end + defp process_device(device_data, org_id, site_map, acc) do + mac = device_data["mac"] + ip = device_data["ip"] + name = device_data["name"] || device_data["mac"] + network = device_data["network"] + local_site = site_map[network] + + if is_nil(ip) and is_nil(mac) do + acc + else + sync_single_device(org_id, ip, mac, name, local_site, acc) + end + end + + defp sync_single_device(org_id, ip, mac, name, local_site, acc) do + case find_existing_device(org_id, ip, mac) do + nil -> + case create_device(org_id, ip || mac, name, local_site) do + {:ok, _} -> Map.update!(acc, :created, &(&1 + 1)) + {:error, _} -> acc + end + + device -> + maybe_update_site(device, local_site) + Map.update!(acc, :matched, &(&1 + 1)) + end + end + defp find_existing_device(org_id, ip, _mac) when not is_nil(ip) do Device |> where(organization_id: ^org_id, ip_address: ^ip) @@ -150,10 +163,10 @@ defmodule Towerops.CnMaestro.Sync do defp maybe_update_site(device, nil), do: {:ok, device} defp maybe_update_site(device, site) do - if device.site_id != site.id do - device |> Device.changeset(%{site_id: site.id}) |> Repo.update() - else + if device.site_id == site.id do {:ok, device} + else + device |> Device.changeset(%{site_id: site.id}) |> Repo.update() end end diff --git a/lib/towerops/pagerduty/client.ex b/lib/towerops/pagerduty/client.ex index f905c790..f3b7cd75 100644 --- a/lib/towerops/pagerduty/client.ex +++ b/lib/towerops/pagerduty/client.ex @@ -98,23 +98,7 @@ defmodule Towerops.PagerDuty.Client do {:ok, %{status: 429} = response} -> # Respect Retry-After header if present - retry_after = - case Map.get(response, :headers) do - headers when is_map(headers) -> - case Map.get(headers, "retry-after") do - val when is_binary(val) -> String.to_integer(val) * 1000 - _ -> backoff_ms(retries) - end - - headers when is_list(headers) -> - case List.keyfind(headers, "retry-after", 0) do - {_, val} -> String.to_integer(val) * 1000 - _ -> backoff_ms(retries) - end - - _ -> - backoff_ms(retries) - end + retry_after = extract_retry_after(response, retries) Logger.info("PagerDuty rate limited, retrying in #{retry_after}ms (attempt #{retries + 1}/#{@max_retries})") Process.sleep(retry_after) @@ -129,6 +113,25 @@ defmodule Towerops.PagerDuty.Client do end end + defp extract_retry_after(response, retries) do + case Map.get(response, :headers) do + headers when is_map(headers) -> + case Map.get(headers, "retry-after") do + val when is_binary(val) -> String.to_integer(val) * 1000 + _ -> backoff_ms(retries) + end + + headers when is_list(headers) -> + case List.keyfind(headers, "retry-after", 0) do + {_, val} -> String.to_integer(val) * 1000 + _ -> backoff_ms(retries) + end + + _ -> + backoff_ms(retries) + end + end + # Exponential backoff: 1s, 2s, 4s defp backoff_ms(retries), do: to_timeout(second: round(:math.pow(2, retries))) diff --git a/lib/towerops/reports/report.ex b/lib/towerops/reports/report.ex index f56edd2c..96a12655 100644 --- a/lib/towerops/reports/report.ex +++ b/lib/towerops/reports/report.ex @@ -68,14 +68,18 @@ defmodule Towerops.Reports.Report do defp validate_recipients(changeset) do case get_field(changeset, :recipients) do - [] -> add_error(changeset, :recipients, "must include at least one recipient") + [] -> + add_error(changeset, :recipients, "must include at least one recipient") + recipients when is_list(recipients) -> if Enum.all?(recipients, &valid_email?/1) do changeset else add_error(changeset, :recipients, "all recipients must be valid email addresses") end - _ -> changeset + + _ -> + changeset end end diff --git a/lib/towerops/rf_links.ex b/lib/towerops/rf_links.ex index afd90ea6..ded0cd4a 100644 --- a/lib/towerops/rf_links.ex +++ b/lib/towerops/rf_links.ex @@ -118,8 +118,12 @@ defmodule Towerops.RfLinks do max_rate = get_in(metadata || %{}, ["max_rate"]) cond do - is_nil(max_rate) or max_rate == 0 -> nil - is_nil(tx) and is_nil(rx) -> nil + is_nil(max_rate) or max_rate == 0 -> + nil + + is_nil(tx) and is_nil(rx) -> + nil + true -> current = max(tx || 0, rx || 0) Float.round(current / max_rate * 100, 1) diff --git a/lib/towerops/status_pages/status_incident.ex b/lib/towerops/status_pages/status_incident.ex index d4f9f1d4..fad5825b 100644 --- a/lib/towerops/status_pages/status_incident.ex +++ b/lib/towerops/status_pages/status_incident.ex @@ -3,6 +3,7 @@ defmodule Towerops.StatusPages.StatusIncident do A status page incident — created manually or auto-generated from alerts. """ use Ecto.Schema + import Ecto.Changeset @primary_key {:id, :binary_id, autogenerate: true} diff --git a/lib/towerops/status_pages/status_page_component.ex b/lib/towerops/status_pages/status_page_component.ex index 34ef9e5a..74bc6a76 100644 --- a/lib/towerops/status_pages/status_page_component.ex +++ b/lib/towerops/status_pages/status_page_component.ex @@ -4,6 +4,7 @@ defmodule Towerops.StatusPages.StatusPageComponent do Maps device groups to customer-facing service names. """ use Ecto.Schema + import Ecto.Changeset @primary_key {:id, :binary_id, autogenerate: true} diff --git a/lib/towerops/status_pages/status_page_config.ex b/lib/towerops/status_pages/status_page_config.ex index af7a239e..5c879e66 100644 --- a/lib/towerops/status_pages/status_page_config.ex +++ b/lib/towerops/status_pages/status_page_config.ex @@ -4,6 +4,7 @@ defmodule Towerops.StatusPages.StatusPageConfig do One-to-one with organization. """ use Ecto.Schema + import Ecto.Changeset @primary_key {:id, :binary_id, autogenerate: true} @@ -27,7 +28,16 @@ defmodule Towerops.StatusPages.StatusPageConfig do def changeset(config, attrs) do config - |> cast(attrs, [:enabled, :slug, :company_name, :logo_url, :support_email, :custom_css, :auto_incidents, :organization_id]) + |> cast(attrs, [ + :enabled, + :slug, + :company_name, + :logo_url, + :support_email, + :custom_css, + :auto_incidents, + :organization_id + ]) |> validate_required([:slug, :organization_id]) |> validate_format(:slug, ~r/^[a-z0-9][a-z0-9-]*[a-z0-9]$/, message: "must be lowercase alphanumeric with hyphens") |> validate_length(:slug, min: 3, max: 63) diff --git a/lib/towerops/topology.ex b/lib/towerops/topology.ex index fa704a78..d042578c 100644 --- a/lib/towerops/topology.ex +++ b/lib/towerops/topology.ex @@ -1249,17 +1249,17 @@ defmodule Towerops.Topology do results |> Enum.group_by(fn {device_id, _val, _descr} -> device_id end) - |> Map.new(fn {device_id, entries} -> - avg_snr = - entries - |> Enum.map(fn {_, val, _} -> val end) - |> then(fn vals -> Enum.sum(vals) / max(length(vals), 1) end) - - {device_id, %{snr: Float.round(avg_snr, 1), signal_health: classify_snr_health(avg_snr)}} - end) + |> Map.new(&compute_device_snr_stats/1) end end + defp compute_device_snr_stats({device_id, entries}) do + vals = Enum.map(entries, fn {_, val, _} -> val end) + avg_snr = Enum.sum(vals) / max(length(vals), 1) + + {device_id, %{snr: Float.round(avg_snr, 1), signal_health: classify_snr_health(avg_snr)}} + end + defp classify_signal_health(nil), do: nil defp classify_signal_health(avg_signal) do @@ -1277,29 +1277,35 @@ defmodule Towerops.Topology do defp classify_snr_health(_snr), do: "critical" defp enrich_edges_with_rf(edges, rf_stats) do - Enum.map(edges, fn edge -> - source_rf = Map.get(rf_stats, edge.source) - target_rf = Map.get(rf_stats, edge.target) + Enum.map(edges, &enrich_single_edge(&1, rf_stats)) + end - # Use the worse health of the two endpoints - signal_health = - case {source_rf, target_rf} do - {nil, nil} -> nil - {s, nil} -> s.signal_health - {nil, t} -> t.signal_health - {s, t} -> worse_health(s.signal_health, t.signal_health) - end + defp enrich_single_edge(edge, rf_stats) do + source_rf = Map.get(rf_stats, edge.source) + target_rf = Map.get(rf_stats, edge.target) - snr = - case {source_rf, target_rf} do - {nil, nil} -> nil - {s, nil} -> s[:snr] - {nil, t} -> t[:snr] - {s, t} -> min(s[:snr] || 0, t[:snr] || 0) - end + signal_health = compute_edge_signal_health(source_rf, target_rf) + snr = compute_edge_snr(source_rf, target_rf) - Map.merge(edge, %{signal_health: signal_health, snr: snr}) - end) + Map.merge(edge, %{signal_health: signal_health, snr: snr}) + end + + defp compute_edge_signal_health(source_rf, target_rf) do + case {source_rf, target_rf} do + {nil, nil} -> nil + {s, nil} -> s.signal_health + {nil, t} -> t.signal_health + {s, t} -> worse_health(s.signal_health, t.signal_health) + end + end + + defp compute_edge_snr(source_rf, target_rf) do + case {source_rf, target_rf} do + {nil, nil} -> nil + {s, nil} -> s[:snr] + {nil, t} -> t[:snr] + {s, t} -> min(s[:snr] || 0, t[:snr] || 0) + end end defp worse_health(a, b) do diff --git a/lib/towerops/uisp/config_snapshot.ex b/lib/towerops/uisp/config_snapshot.ex index 26205561..5c14d039 100644 --- a/lib/towerops/uisp/config_snapshot.ex +++ b/lib/towerops/uisp/config_snapshot.ex @@ -43,7 +43,7 @@ defmodule Towerops.Uisp.ConfigSnapshot do defp store_if_changed(device, config_data, acc) do config_json = Jason.encode!(config_data) - config_hash = :crypto.hash(:sha256, config_json) |> Base.encode16(case: :lower) + config_hash = :sha256 |> :crypto.hash(config_json) |> Base.encode16(case: :lower) latest = get_latest_snapshot(device.id) @@ -73,38 +73,40 @@ defmodule Towerops.Uisp.ConfigSnapshot do Gets the latest config snapshot for a device. """ def get_latest_snapshot(device_id) do - from(s in "uisp_config_snapshots", - where: s.device_id == ^device_id, - order_by: [desc: s.snapshot_at], - limit: 1, - select: %{ - id: s.id, - device_id: s.device_id, - config_hash: s.config_hash, - config_size_bytes: s.config_size_bytes, - snapshot_at: s.snapshot_at - } + Repo.one( + from(s in "uisp_config_snapshots", + where: s.device_id == ^device_id, + order_by: [desc: s.snapshot_at], + limit: 1, + select: %{ + id: s.id, + device_id: s.device_id, + config_hash: s.config_hash, + config_size_bytes: s.config_size_bytes, + snapshot_at: s.snapshot_at + } + ) ) - |> Repo.one() end @doc """ Lists config snapshots for a device, most recent first. """ def list_snapshots(device_id, limit \\ 20) do - from(s in "uisp_config_snapshots", - where: s.device_id == ^device_id, - order_by: [desc: s.snapshot_at], - limit: ^limit, - select: %{ - id: s.id, - config_hash: s.config_hash, - config_size_bytes: s.config_size_bytes, - compressed_size_bytes: s.compressed_size_bytes, - snapshot_at: s.snapshot_at - } + Repo.all( + from(s in "uisp_config_snapshots", + where: s.device_id == ^device_id, + order_by: [desc: s.snapshot_at], + limit: ^limit, + select: %{ + id: s.id, + config_hash: s.config_hash, + config_size_bytes: s.config_size_bytes, + compressed_size_bytes: s.compressed_size_bytes, + snapshot_at: s.snapshot_at + } + ) ) - |> Repo.all() end @doc """ @@ -121,7 +123,7 @@ defmodule Towerops.Uisp.ConfigSnapshot do {:error, :not_found} compressed -> - {:ok, :zlib.uncompress(compressed) |> Jason.decode!()} + {:ok, compressed |> :zlib.uncompress() |> Jason.decode!()} end end @@ -158,7 +160,8 @@ defmodule Towerops.Uisp.ConfigSnapshot do defp compute_diff(_a, _b), do: %{added: [], removed: [], changed: []} defp insert_snapshot(attrs) do - Repo.insert_all("uisp_config_snapshots", [ + "uisp_config_snapshots" + |> Repo.insert_all([ Map.merge(attrs, %{ id: Ecto.UUID.generate(), inserted_at: DateTime.truncate(DateTime.utc_now(), :second) diff --git a/lib/towerops/uisp/device_sync.ex b/lib/towerops/uisp/device_sync.ex index 55096959..90e00c26 100644 --- a/lib/towerops/uisp/device_sync.ex +++ b/lib/towerops/uisp/device_sync.ex @@ -13,8 +13,8 @@ defmodule Towerops.Uisp.DeviceSync do alias Towerops.Devices.Device alias Towerops.Integrations.Integration alias Towerops.Repo - alias Towerops.Snmp.Interface alias Towerops.Snmp.Device, as: SnmpDevice + alias Towerops.Snmp.Interface alias Towerops.Uisp.Client require Logger @@ -44,36 +44,51 @@ defmodule Towerops.Uisp.DeviceSync do defp process_devices(org_id, devices, site_map) do Enum.reduce(devices, %{matched: 0, created: 0}, fn device_data, acc -> - ip = device_data["ipAddress"] - mac = get_in(device_data, ["identification", "mac"]) - name = get_in(device_data, ["identification", "name"]) - uisp_site_id = get_in(device_data, ["site", "id"]) - local_site = site_map[uisp_site_id] - - if is_nil(ip) or ip == "" do - acc - else - case find_existing_device(org_id, ip, mac) do - nil -> - case create_device(org_id, ip, name, local_site, device_data) do - {:ok, _device} -> Map.update!(acc, :created, &(&1 + 1)) - {:error, reason} -> - Logger.warning("UISP device sync: failed to create device #{ip}: #{inspect(reason)}") - acc - end - - device -> - case update_device_site(device, local_site) do - {:ok, _device} -> Map.update!(acc, :matched, &(&1 + 1)) - {:error, reason} -> - Logger.warning("UISP device sync: failed to update device #{ip}: #{inspect(reason)}") - acc - end - end - end + process_single_device(org_id, device_data, site_map, acc) end) end + defp process_single_device(org_id, device_data, site_map, acc) do + ip = device_data["ipAddress"] + mac = get_in(device_data, ["identification", "mac"]) + name = get_in(device_data, ["identification", "name"]) + uisp_site_id = get_in(device_data, ["site", "id"]) + local_site = site_map[uisp_site_id] + + cond do + is_nil(ip) or ip == "" -> + acc + + existing = find_existing_device(org_id, ip, mac) -> + handle_existing_device(existing, local_site, ip, acc) + + true -> + handle_new_device(org_id, ip, name, local_site, device_data, acc) + end + end + + defp handle_existing_device(device, local_site, ip, acc) do + case update_device_site(device, local_site) do + {:ok, _device} -> + Map.update!(acc, :matched, &(&1 + 1)) + + {:error, reason} -> + Logger.warning("UISP device sync: failed to update device #{ip}: #{inspect(reason)}") + acc + end + end + + defp handle_new_device(org_id, ip, name, local_site, device_data, acc) do + case create_device(org_id, ip, name, local_site, device_data) do + {:ok, _device} -> + Map.update!(acc, :created, &(&1 + 1)) + + {:error, reason} -> + Logger.warning("UISP device sync: failed to create device #{ip}: #{inspect(reason)}") + acc + end + end + defp find_existing_device(org_id, ip, mac) do find_by_ip(org_id, ip) || find_by_mac(org_id, mac) end @@ -105,7 +120,7 @@ defmodule Towerops.Uisp.DeviceSync do end defp create_device(org_id, ip, name, site, raw_data) do - site_id = if site, do: site.id, else: nil + site_id = if site, do: site.id attrs = %{ organization_id: org_id, diff --git a/lib/towerops/uisp/gps_sync.ex b/lib/towerops/uisp/gps_sync.ex index 52795afc..65ac8483 100644 --- a/lib/towerops/uisp/gps_sync.ex +++ b/lib/towerops/uisp/gps_sync.ex @@ -6,8 +6,6 @@ defmodule Towerops.Uisp.GpsSync do and its associated site with coordinates for map enrichment. """ - import Ecto.Query - alias Towerops.Devices.Device alias Towerops.Repo alias Towerops.Sites.Site @@ -43,7 +41,8 @@ defmodule Towerops.Uisp.GpsSync do metadata = device.metadata || %{} if is_nil(metadata["latitude"]) or is_nil(metadata["longitude"]) do - new_metadata = Map.merge(metadata, %{"latitude" => to_float(lat), "longitude" => to_float(lon), "gps_source" => "uisp"}) + new_metadata = + Map.merge(metadata, %{"latitude" => to_float(lat), "longitude" => to_float(lon), "gps_source" => "uisp"}) case device |> Device.changeset(%{metadata: new_metadata}) @@ -57,21 +56,16 @@ defmodule Towerops.Uisp.GpsSync do end defp maybe_update_site_gps(acc, device, lat, lon) do - if device.site_id do - site = Repo.get(Site, device.site_id) - - if site && (is_nil(site.latitude) or is_nil(site.longitude)) do - case site - |> Site.changeset(%{latitude: to_float(lat), longitude: to_float(lon)}) - |> Repo.update() do - {:ok, _} -> Map.update!(acc, :sites_updated, &(&1 + 1)) - {:error, _} -> acc - end - else - acc - end + with site_id when not is_nil(site_id) <- device.site_id, + %Site{} = site <- Repo.get(Site, site_id), + true <- is_nil(site.latitude) or is_nil(site.longitude), + {:ok, _} <- + site + |> Site.changeset(%{latitude: to_float(lat), longitude: to_float(lon)}) + |> Repo.update() do + Map.update!(acc, :sites_updated, &(&1 + 1)) else - acc + _ -> acc end end diff --git a/lib/towerops/uisp/site_sync.ex b/lib/towerops/uisp/site_sync.ex index 00e12428..e619a758 100644 --- a/lib/towerops/uisp/site_sync.ex +++ b/lib/towerops/uisp/site_sync.ex @@ -38,34 +38,42 @@ defmodule Towerops.Uisp.SiteSync do defp upsert_sites(org_id, sites) do Enum.reduce(sites, {0, %{}}, fn site_data, {count, acc} -> - uisp_id = get_in(site_data, ["identification", "id"]) || site_data["id"] - name = get_in(site_data, ["identification", "name"]) || site_data["name"] - latitude = get_in(site_data, ["description", "gps", "latitude"]) - longitude = get_in(site_data, ["description", "gps", "longitude"]) - - if is_nil(name) or name == "" do - {count, acc} - else - attrs = %{ - organization_id: org_id, - name: name, - latitude: to_float(latitude), - longitude: to_float(longitude) - } - - case upsert_site(attrs) do - {:ok, site} -> - new_acc = if uisp_id, do: Map.put(acc, uisp_id, site), else: acc - {count + 1, new_acc} - - {:error, changeset} -> - Logger.warning("UISP site sync: failed to upsert site #{name}: #{inspect(changeset.errors)}") - {count, acc} - end - end + process_site(org_id, site_data, count, acc) end) end + defp process_site(org_id, site_data, count, acc) do + uisp_id = get_in(site_data, ["identification", "id"]) || site_data["id"] + name = get_in(site_data, ["identification", "name"]) || site_data["name"] + latitude = get_in(site_data, ["description", "gps", "latitude"]) + longitude = get_in(site_data, ["description", "gps", "longitude"]) + + if is_nil(name) or name == "" do + {count, acc} + else + attrs = %{ + organization_id: org_id, + name: name, + latitude: to_float(latitude), + longitude: to_float(longitude) + } + + handle_site_upsert(attrs, uisp_id, name, count, acc) + end + end + + defp handle_site_upsert(attrs, uisp_id, name, count, acc) do + case upsert_site(attrs) do + {:ok, site} -> + new_acc = if uisp_id, do: Map.put(acc, uisp_id, site), else: acc + {count + 1, new_acc} + + {:error, changeset} -> + Logger.warning("UISP site sync: failed to upsert site #{name}: #{inspect(changeset.errors)}") + {count, acc} + end + end + defp upsert_site(attrs) do %Site{} |> Site.changeset(attrs) diff --git a/lib/towerops/uisp/statistics_sync.ex b/lib/towerops/uisp/statistics_sync.ex index 47ea3241..43482766 100644 --- a/lib/towerops/uisp/statistics_sync.ex +++ b/lib/towerops/uisp/statistics_sync.ex @@ -12,6 +12,7 @@ defmodule Towerops.Uisp.StatisticsSync do alias Towerops.Integrations.Integration alias Towerops.Repo + alias Towerops.Snmp.Interface alias Towerops.Snmp.InterfaceStat alias Towerops.Uisp.Client @@ -50,22 +51,31 @@ defmodule Towerops.Uisp.StatisticsSync do defp process_device_stats(local_device, stats, acc) when is_list(stats) do Enum.reduce(stats, acc, fn stat, inner_acc -> - interface_name = stat["interfaceName"] || stat["name"] || "unknown" - checked_at = parse_timestamp(stat["timestamp"]) - - if checked_at && !recently_polled?(local_device.id, interface_name, checked_at) do - case insert_stat(local_device, interface_name, stat, checked_at) do - {:ok, _} -> Map.update!(inner_acc, :inserted, &(&1 + 1)) - {:error, _} -> inner_acc - end - else - Map.update!(inner_acc, :skipped_dedup, &(&1 + 1)) - end + process_single_stat(local_device, stat, inner_acc) end) end defp process_device_stats(_device, _stats, acc), do: acc + defp process_single_stat(local_device, stat, acc) do + interface_name = stat["interfaceName"] || stat["name"] || "unknown" + checked_at = parse_timestamp(stat["timestamp"]) + + cond do + is_nil(checked_at) -> + Map.update!(acc, :skipped_dedup, &(&1 + 1)) + + recently_polled?(local_device.id, interface_name, checked_at) -> + Map.update!(acc, :skipped_dedup, &(&1 + 1)) + + true -> + case insert_stat(local_device, interface_name, stat, checked_at) do + {:ok, _} -> Map.update!(acc, :inserted, &(&1 + 1)) + {:error, _} -> acc + end + end + end + @doc """ Checks if SNMP already polled this interface recently, to avoid double-counting. @@ -75,7 +85,7 @@ defmodule Towerops.Uisp.StatisticsSync do cutoff = DateTime.add(checked_at, -@dedup_window_seconds, :second) InterfaceStat - |> join(:inner, [s], i in Towerops.Snmp.Interface, on: s.interface_id == i.id) + |> join(:inner, [s], i in Interface, on: s.interface_id == i.id) |> where([_s, i], i.snmp_device_id == ^snmp_device_id) |> where([s, _i], s.checked_at >= ^cutoff and s.checked_at <= ^checked_at) |> limit(1) @@ -91,9 +101,9 @@ defmodule Towerops.Uisp.StatisticsSync do %InterfaceStat{} |> InterfaceStat.changeset(%{ interface_id: interface.id, - if_in_octets: stat["receive"] |> parse_counter(), - if_out_octets: stat["transmit"] |> parse_counter(), - if_in_errors: stat["errors"] |> parse_counter(), + if_in_octets: parse_counter(stat["receive"]), + if_out_octets: parse_counter(stat["transmit"]), + if_in_errors: parse_counter(stat["errors"]), if_out_errors: 0, if_in_discards: 0, if_out_discards: 0, @@ -106,7 +116,7 @@ defmodule Towerops.Uisp.StatisticsSync do end defp find_primary_interface(snmp_device_id) do - Towerops.Snmp.Interface + Interface |> where(snmp_device_id: ^snmp_device_id) |> order_by(asc: :if_index) |> limit(1) @@ -136,7 +146,7 @@ defmodule Towerops.Uisp.StatisticsSync do end defp parse_timestamp(ts) when is_integer(ts) do - DateTime.from_unix(ts) |> elem(1) |> DateTime.truncate(:second) + ts |> DateTime.from_unix() |> elem(1) |> DateTime.truncate(:second) end defp parse_timestamp(_), do: nil diff --git a/lib/towerops/uisp/sync.ex b/lib/towerops/uisp/sync.ex index 28fb5bc7..0fa76d40 100644 --- a/lib/towerops/uisp/sync.ex +++ b/lib/towerops/uisp/sync.ex @@ -33,8 +33,12 @@ defmodule Towerops.Uisp.Sync do # Phase 2: GPS enrichment, statistics sync, config snapshots # These are best-effort — failures don't block the main sync gps_result = safe_sync(:gps, fn -> GpsSync.sync_gps(device_result.device_pairs || []) end) - stats_result = safe_sync(:stats, fn -> StatisticsSync.sync_statistics(integration, device_result.device_map || %{}) end) - config_result = safe_sync(:config, fn -> ConfigSnapshot.sync_configs(integration, device_result.device_map || %{}) end) + + stats_result = + safe_sync(:stats, fn -> StatisticsSync.sync_statistics(integration, device_result.device_map || %{}) end) + + config_result = + safe_sync(:config, fn -> ConfigSnapshot.sync_configs(integration, device_result.device_map || %{}) end) duration_ms = System.monotonic_time(:millisecond) - start_time total_synced = site_result.synced + device_result.matched + device_result.created @@ -42,11 +46,18 @@ defmodule Towerops.Uisp.Sync do message = "Synced #{site_result.synced} sites, matched #{device_result.matched} devices, created #{device_result.created} devices" - log_sync(integration, "success", total_synced, %{ - gps: inspect(gps_result), - stats: inspect(stats_result), - config: inspect(config_result) - }, duration_ms) + log_sync( + integration, + "success", + total_synced, + %{ + gps: inspect(gps_result), + stats: inspect(stats_result), + config: inspect(config_result) + }, + duration_ms + ) + Integrations.update_sync_status(integration, "success", message) {:ok, @@ -88,7 +99,9 @@ defmodule Towerops.Uisp.Sync do defp safe_sync(label, fun) do case fun.() do - {:ok, result} -> result + {:ok, result} -> + result + {:error, reason} -> Logger.warning("UISP #{label} sync failed: #{inspect(reason)}") %{error: reason} diff --git a/lib/towerops/workers/alert_digest_worker.ex b/lib/towerops/workers/alert_digest_worker.ex index 52dbf6e8..0032d9e2 100644 --- a/lib/towerops/workers/alert_digest_worker.ex +++ b/lib/towerops/workers/alert_digest_worker.ex @@ -48,16 +48,14 @@ defmodule Towerops.Workers.AlertDigestWorker do digest -> alert_ids = digest.suppressed_alert_ids || [] - if length(alert_ids) > 0 do - alerts = - alert_ids - |> Enum.map(&Alerts.get_alert/1) - |> Enum.reject(&is_nil/1) - - if length(alerts) > 0 do - send_digest_notification(alerts) - NotificationRateLimiter.mark_digest_sent(digest) - end + with [_ | _] <- alert_ids, + alerts = + alert_ids + |> Enum.map(&Alerts.get_alert/1) + |> Enum.reject(&is_nil/1), + [_ | _] <- alerts do + send_digest_notification(alerts) + NotificationRateLimiter.mark_digest_sent(digest) end :ok diff --git a/lib/towerops/workers/alert_notification_worker.ex b/lib/towerops/workers/alert_notification_worker.ex index d2d85058..3b24f09b 100644 --- a/lib/towerops/workers/alert_notification_worker.ex +++ b/lib/towerops/workers/alert_notification_worker.ex @@ -23,34 +23,7 @@ defmodule Towerops.Workers.AlertNotificationWorker do def perform(%Oban.Job{args: %{"action" => "trigger", "alert_id" => alert_id}}) do with {:ok, alert} <- fetch_alert(alert_id), {:ok, device} <- fetch_device(alert.device_id) do - # Skip notification entirely if alert was storm-suppressed - if alert.storm_suppressed do - Logger.debug("Skipping notification for storm-suppressed alert #{alert_id}") - :ok - else - routing = alert_routing(device) - - # Apply per-user rate limiting for built-in notifications - rate_limited_users = - if routing in ["builtin", "both"] do - check_and_apply_rate_limits(alert, device) - else - MapSet.new() - end - - pd_result = - if routing in ["pagerduty", "both"] do - notify_pagerduty_trigger(alert, device, alert_id) - else - :ok - end - - if routing in ["builtin", "both"] do - maybe_start_escalation(alert, device, rate_limited_users) - end - - pd_result - end + handle_trigger_notification(alert, device, alert_id) else {:error, :alert_not_found} -> Logger.warning("Alert #{alert_id} not found for notification") @@ -131,6 +104,40 @@ defmodule Towerops.Workers.AlertNotificationWorker do |> Oban.insert() end + defp handle_trigger_notification(alert, device, alert_id) do + if alert.storm_suppressed do + Logger.debug("Skipping notification for storm-suppressed alert #{alert_id}") + :ok + else + send_notifications(alert, device, alert_id) + end + end + + defp send_notifications(alert, device, alert_id) do + routing = alert_routing(device) + + # Apply per-user rate limiting for built-in notifications + rate_limited_users = + if routing in ["builtin", "both"] do + check_and_apply_rate_limits(alert, device) + else + MapSet.new() + end + + pd_result = + if routing in ["pagerduty", "both"] do + notify_pagerduty_trigger(alert, device, alert_id) + else + :ok + end + + if routing in ["builtin", "both"] do + maybe_start_escalation(alert, device, rate_limited_users) + end + + pd_result + end + defp notify_pagerduty_trigger(alert, device, alert_id) do case Notifier.notify_trigger(alert, device) do {:error, :not_configured} -> @@ -184,24 +191,23 @@ defmodule Towerops.Workers.AlertNotificationWorker do org_id = device.organization_id policy_id = device.escalation_policy_id || device.organization.default_escalation_policy_id - if policy_id do - # Get on-call users from escalation policy's first rule targets - user_ids = resolve_escalation_targets(policy_id) + policy_id + |> case do + nil -> [] + id -> resolve_escalation_targets(id) + end + |> Enum.filter(&should_suppress_for_user?(&1, org_id, alert.id)) + |> MapSet.new() + end - user_ids - |> Enum.filter(fn user_id -> - case NotificationRateLimiter.check_rate(user_id, org_id, alert.id) do - :suppress -> - AlertDigestWorker.enqueue(user_id) - true + defp should_suppress_for_user?(user_id, org_id, alert_id) do + case NotificationRateLimiter.check_rate(user_id, org_id, alert_id) do + :suppress -> + AlertDigestWorker.enqueue(user_id) + true - :allow -> - false - end - end) - |> MapSet.new() - else - MapSet.new() + :allow -> + false end end diff --git a/lib/towerops_web/components/layouts.ex b/lib/towerops_web/components/layouts.ex index 5f32610e..a715d050 100644 --- a/lib/towerops_web/components/layouts.ex +++ b/lib/towerops_web/components/layouts.ex @@ -339,378 +339,383 @@ defmodule ToweropsWeb.Layouts do - - - - - + <.flash_group flash={@flash} /> diff --git a/lib/towerops_web/live/org/integrations_live.ex b/lib/towerops_web/live/org/integrations_live.ex index 55a6ef41..f44a7ae2 100644 --- a/lib/towerops_web/live/org/integrations_live.ex +++ b/lib/towerops_web/live/org/integrations_live.ex @@ -2,13 +2,13 @@ defmodule ToweropsWeb.Org.IntegrationsLive do @moduledoc false use ToweropsWeb, :live_view + alias Towerops.CnMaestro.Client, as: CnMaestroClient alias Towerops.Gaiia.Client, as: GaiiaClient alias Towerops.Integrations alias Towerops.Integrations.Integration alias Towerops.Preseem.Client, as: PreseemClient alias Towerops.Sonar.Client, as: SonarClient alias Towerops.Splynx.Client, as: SplynxClient - alias Towerops.CnMaestro.Client, as: CnMaestroClient alias Towerops.Uisp.Client, as: UispClient alias Towerops.Visp.Client, as: VispClient diff --git a/lib/towerops_web/live/reports_live.ex b/lib/towerops_web/live/reports_live.ex index 0e6437bc..7a27bb40 100644 --- a/lib/towerops_web/live/reports_live.ex +++ b/lib/towerops_web/live/reports_live.ex @@ -8,6 +8,7 @@ defmodule ToweropsWeb.ReportsLive do alias Towerops.Reports alias Towerops.Reports.Report + alias Towerops.Workers.ReportWorker @impl true def mount(_params, _session, socket) do @@ -103,7 +104,7 @@ defmodule ToweropsWeb.ReportsLive do @impl true def handle_event("run_now", %{"id" => id}, socket) do case %{"report_id" => id} - |> Towerops.Workers.ReportWorker.new() + |> ReportWorker.new() |> Oban.insert() do {:ok, _} -> {:noreply, put_flash(socket, :info, "Report queued for delivery")} @@ -127,15 +128,12 @@ defmodule ToweropsWeb.ReportsLive do defp format_recipients(recipients) when is_list(recipients), do: Enum.join(recipients, ", ") defp format_recipients(_), do: "-" - defp status_badge("success"), - do: {"Success", "bg-green-100 text-green-700 dark:bg-green-900/50 dark:text-green-300"} + defp status_badge("success"), do: {"Success", "bg-green-100 text-green-700 dark:bg-green-900/50 dark:text-green-300"} - defp status_badge("failed"), - do: {"Failed", "bg-red-100 text-red-700 dark:bg-red-900/50 dark:text-red-300"} + defp status_badge("failed"), do: {"Failed", "bg-red-100 text-red-700 dark:bg-red-900/50 dark:text-red-300"} defp status_badge("partial_failure"), do: {"Partial", "bg-yellow-100 text-yellow-700 dark:bg-yellow-900/50 dark:text-yellow-300"} - defp status_badge(_), - do: {"Never Run", "bg-gray-100 text-gray-700 dark:bg-gray-800 dark:text-gray-300"} + defp status_badge(_), do: {"Never Run", "bg-gray-100 text-gray-700 dark:bg-gray-800 dark:text-gray-300"} end diff --git a/lib/towerops_web/live/reports_live.html.heex b/lib/towerops_web/live/reports_live.html.heex index d9eff2dd..4030e695 100644 --- a/lib/towerops_web/live/reports_live.html.heex +++ b/lib/towerops_web/live/reports_live.html.heex @@ -30,7 +30,10 @@

{t("Create Report")}

-
@@ -100,7 +103,9 @@