fix nil firmware comparison

lib/towerops/devices/firmware.ex

@@ -47,6 +47,8 @@ defmodule Towerops.Devices.Firmware do
   @doc """
   Gets the latest firmware release for a vendor and product line.
 
+  Returns nil if either vendor or product_line is nil.
+
   ## Examples
 
       iex> get_latest_firmware_release("mikrotik", "routeros")
@@ -54,7 +56,13 @@ defmodule Towerops.Devices.Firmware do
 
       iex> get_latest_firmware_release("unknown", "unknown")
       nil
+
+      iex> get_latest_firmware_release("mikrotik", nil)
+      nil
   """
+  def get_latest_firmware_release(nil, _product_line), do: nil
+  def get_latest_firmware_release(_vendor, nil), do: nil
+
   def get_latest_firmware_release(vendor, product_line) do
     Repo.one(from(f in FirmwareRelease, where: f.vendor == ^vendor and f.product_line == ^product_line))
   end

lib/towerops_web/live/device_live/show.ex

@@ -293,7 +293,9 @@ defmodule ToweropsWeb.DeviceLive.Show do
     vendor = determine_vendor(snmp_device.manufacturer)
     product_line = determine_product_line(snmp_device.manufacturer)
 
-    if vendor do
+    # Only check firmware if both vendor and product_line are known
+    # Currently only MikroTik/RouterOS is supported
+    if vendor && product_line do
       Firmware.get_latest_firmware_release(vendor, product_line)
     end
   end

test/support/snmp_kit_mock.ex

@@ -0,0 +1,18 @@
+defmodule Towerops.Mocks.SnmpKitMock do
+  @moduledoc """
+  Mock for SnmpKit to be used in tests.
+  """
+
+  def get(_target, _oid, _opts) do
+    {:ok, {:octet_string, "Mock Value"}}
+  end
+
+  def walk(_target, _oid, _opts) do
+    # Return empty list to simulate no results
+    {:ok, []}
+  end
+
+  def get_bulk(_target, _oid, _opts) do
+    {:ok, []}
+  end
+end

test/towerops/devices/firmware_test.exs

@@ -79,6 +79,18 @@ defmodule Towerops.Devices.FirmwareTest do
     test "returns nil when firmware release does not exist" do
       assert is_nil(Firmware.get_latest_firmware_release("unknown", "unknown"))
     end
+
+    test "returns nil when vendor is nil" do
+      assert is_nil(Firmware.get_latest_firmware_release(nil, "routeros"))
+    end
+
+    test "returns nil when product_line is nil" do
+      assert is_nil(Firmware.get_latest_firmware_release("mikrotik", nil))
+    end
+
+    test "returns nil when both vendor and product_line are nil" do
+      assert is_nil(Firmware.get_latest_firmware_release(nil, nil))
+    end
   end
 
   describe "list_firmware_releases/0" do

test/towerops/workers/device_poller_worker_test.exs

@@ -1,15 +1,47 @@
 defmodule Towerops.Workers.DevicePollerWorkerTest do
   use Towerops.DataCase, async: false
 
+  import Ecto.Query
+  import Mox
   import Towerops.AccountsFixtures
 
   alias Towerops.Devices
   alias Towerops.Organizations
+  alias Towerops.Repo
   alias Towerops.Sites
+  alias Towerops.Snmp.Device
+  alias Towerops.Snmp.Interface
+  alias Towerops.Snmp.Sensor
+  alias Towerops.Snmp.SnmpMock
+  alias Towerops.Snmp.StateSensor
   alias Towerops.Workers.DevicePollerWorker
   alias Towerops.Workers.PollingOffset
 
+  setup :verify_on_exit!
+
   setup do
+    # Configure Mock SNMP Adapter
+    old_adapter = Application.get_env(:towerops, :snmp_adapter)
+    Application.put_env(:towerops, :snmp_adapter, SnmpMock)
+
+    # Disable Phoenix SNMP check for tests
+    old_disable = Application.get_env(:towerops, :disable_phoenix_snmp)
+    Application.put_env(:towerops, :disable_phoenix_snmp, false)
+
+    on_exit(fn ->
+      if old_adapter do
+        Application.put_env(:towerops, :snmp_adapter, old_adapter)
+      else
+        Application.delete_env(:towerops, :snmp_adapter)
+      end
+
+      if old_disable == nil do
+        Application.delete_env(:towerops, :disable_phoenix_snmp)
+      else
+        Application.put_env(:towerops, :disable_phoenix_snmp, old_disable)
+      end
+    end)
+
     user = user_fixture()
     {:ok, organization} = Organizations.create_organization(%{name: "Test Org"}, user.id)
 
@@ -22,6 +54,246 @@ defmodule Towerops.Workers.DevicePollerWorkerTest do
     %{organization: organization, site: site, user: user}
   end
 
+  describe "perform/1" do
+    test "returns :ok when device does not exist" do
+      assert :ok = DevicePollerWorker.perform(%Oban.Job{args: %{"device_id" => Ecto.UUID.generate()}})
+    end
+
+    test "skips polling when snmp_enabled is false", %{site: site} do
+      {:ok, device} =
+        Devices.create_device(%{
+          name: "Router No SNMP",
+          ip_address: "192.168.1.10",
+          site_id: site.id,
+          organization_id: site.organization_id,
+          snmp_enabled: false
+        })
+
+      # Ensure no jobs exist initially (created by create_device)
+      Repo.delete_all(Oban.Job)
+
+      assert :ok = DevicePollerWorker.perform(%Oban.Job{args: %{"device_id" => device.id}})
+
+      # Should NOT schedule next poll (because it returns :ok early in maybe_poll_device,
+      # but wait, does it schedule next poll if skipped?
+      # perform/1 calls maybe_poll_device then schedule_next_poll_with_error_handling.
+      # Let's check the code of perform/1 again.
+      # It calls maybe_poll_device(device) -> returns :ok
+      # Then calls schedule_next_poll_with_error_handling(device_id, device)
+
+      # So it SHOULD schedule the next poll even if skipped?
+      # Logic:
+      # device ->
+      #   maybe_poll_device(device)
+      #   schedule_next_poll_with_error_handling(device_id, device)
+      #   :ok
+
+      # Yes, it schedules next poll.
+      assert Repo.aggregate(Oban.Job, :count) == 1
+    end
+
+    test "polls device and schedules next run", %{site: site} do
+      {:ok, device} =
+        Devices.create_device(%{
+          name: "Router Poll",
+          ip_address: "192.168.1.11",
+          site_id: site.id,
+          organization_id: site.organization_id,
+          snmp_enabled: true,
+          check_interval_seconds: 60
+        })
+
+      # Clear existing jobs
+      Repo.delete_all(Oban.Job)
+
+      assert :ok = DevicePollerWorker.perform(%Oban.Job{args: %{"device_id" => device.id}})
+
+      # Verify next job scheduled
+      assert Repo.aggregate(Oban.Job, :count) == 1
+    end
+  end
+
+  describe "polling logic" do
+    test "polls sensors and updates values", %{site: site} do
+      # 1. Create Device
+      {:ok, device} =
+        Devices.create_device(%{
+          name: "Sensor Device",
+          ip_address: "192.168.1.50",
+          site_id: site.id,
+          organization_id: site.organization_id,
+          snmp_enabled: true
+        })
+
+      # 2. Create SnmpDevice association
+      {:ok, snmp_device} =
+        Repo.insert(%Device{
+          device_id: device.id,
+          sys_object_id: "1.3.6.1.4.1.9.1",
+          sys_descr: "Cisco Router",
+          sys_name: "router01"
+        })
+
+      # 3. Create Sensor
+      {:ok, sensor} =
+        Repo.insert(%Sensor{
+          snmp_device_id: snmp_device.id,
+          sensor_type: "temperature",
+          sensor_index: "1",
+          sensor_oid: "1.3.6.1.2.1.99.1.1.1.4.1",
+          sensor_descr: "Chassis Temp",
+          sensor_unit: "C",
+          sensor_divisor: 1,
+          monitored: true
+        })
+
+      # 4. Create Interface
+      {:ok, interface} =
+        Repo.insert(%Interface{
+          snmp_device_id: snmp_device.id,
+          if_index: 2,
+          if_name: "eth0",
+          if_descr: "Ethernet 0",
+          if_type: 6,
+          if_speed: 100_000_000,
+          if_phys_address: "00:11:22:33:44:55",
+          if_admin_status: "up",
+          if_oper_status: "up",
+          monitored: true
+        })
+
+      # 4b. Create State Sensor
+      {:ok, state_sensor} =
+        Repo.insert(%StateSensor{
+          snmp_device_id: snmp_device.id,
+          sensor_descr: "Power Supply 1",
+          sensor_oid: "1.3.6.1.4.1.9.9.13.1.5.1.3.1",
+          sensor_index: "1",
+          metadata: %{"states" => %{"1" => "unknown", "2" => "enabled", "3" => "disabled"}}
+          # status defaults to unknown
+        })
+
+      # 4c. Create Processor
+      {:ok, processor} =
+        Repo.insert(%Towerops.Snmp.Processor{
+          snmp_device_id: snmp_device.id,
+          processor_index: 1,
+          processor_type: "hr_processor",
+          processor_descr: "CPU 1"
+        })
+
+      # 5. Mock SNMP response
+      # The worker runs tasks in parallel, so we need to be careful with expectations.
+      # It calls poll_device_sensors, poll_device_state_sensors, etc.
+      # We only have one sensor, so poll_device_sensors will try to fetch it.
+
+      SnmpMock
+      |> stub(:get, fn _target, oid, _opts ->
+        case oid do
+          # Sensor
+          "1.3.6.1.2.1.99.1.1.1.4.1" -> {:ok, 45}
+          # State Sensor
+          # enabled/ok
+          "1.3.6.1.4.1.9.9.13.1.5.1.3.1" -> {:ok, 2}
+          # Processor Load
+          # 25% load
+          "1.3.6.1.2.1.25.3.3.1.2.1" -> {:ok, 25}
+          # Interface HC In Octets
+          "1.3.6.1.2.1.31.1.1.1.6.2" -> {:ok, 1000}
+          # Interface HC Out Octets
+          "1.3.6.1.2.1.31.1.1.1.10.2" -> {:ok, 2000}
+          # Interface Errors
+          "1.3.6.1.2.1.2.2.1.14.2" -> {:ok, 0}
+          "1.3.6.1.2.1.2.2.1.20.2" -> {:ok, 0}
+          "1.3.6.1.2.1.2.2.1.13.2" -> {:ok, 0}
+          "1.3.6.1.2.1.2.2.1.19.2" -> {:ok, 0}
+          _ -> {:error, :no_such_object}
+        end
+      end)
+      |> stub(:get_multiple, fn _target, oids, _opts ->
+        # 1.3.6.1.2.1.2.2.1.5.2 (speed)
+        # 1.3.6.1.2.1.2.2.1.6.2 (phys)
+        # 1.3.6.1.2.1.2.2.1.7.2 (admin)
+        # 1.3.6.1.2.1.2.2.1.8.2 (oper)
+        if Enum.any?(oids, &String.contains?(&1, ".2")) do
+          # Change admin status to down (2)
+          # 1=up
+          {:ok, [100_000_000, <<0, 17, 34, 51, 68, 85>>, 2, 1]}
+        else
+          {:error, :timeout}
+        end
+      end)
+      # Allow other calls (like interfaces, neighbors etc) to return empty/ok
+      |> stub(:walk, fn _target, _oid, _opts -> {:ok, []} end)
+
+      # 6. Run Perform
+      Repo.delete_all(Oban.Job)
+      assert :ok = DevicePollerWorker.perform(%Oban.Job{args: %{"device_id" => device.id}})
+
+      # 7. Verify Sensor Updated
+      updated_sensor = Repo.get(Sensor, sensor.id)
+      assert updated_sensor.last_value == 45.0
+      assert updated_sensor.last_checked_at
+
+      # 8. Verify Interface Stats
+      stats = Repo.all(Towerops.Snmp.InterfaceStat)
+      assert length(stats) >= 1
+      stat = List.last(stats)
+      assert stat.interface_id == interface.id
+      assert stat.if_in_octets == 1000
+      assert stat.if_out_octets == 2000
+
+      # 9. Verify Interface Changed
+      updated_interface = Repo.get(Interface, interface.id)
+      assert updated_interface.if_admin_status == "down"
+
+      # 10. Verify State Sensor
+      updated_state = Repo.get(StateSensor, state_sensor.id)
+      assert updated_state.state_value == 2
+      assert updated_state.status == "ok"
+      assert updated_state.state_descr == "enabled"
+
+      # 11. Verify Processor
+      readings = Repo.all(Towerops.Snmp.ProcessorReading)
+      assert length(readings) >= 1
+      reading = List.last(readings)
+      assert reading.processor_id == processor.id
+      assert reading.usage_percent == 25
+    end
+
+    test "handles SNMP errors gracefully", %{site: site} do
+      {:ok, device} =
+        Devices.create_device(%{
+          name: "Error Device",
+          ip_address: "192.168.1.51",
+          site_id: site.id,
+          organization_id: site.organization_id,
+          snmp_enabled: true
+        })
+
+      {:ok, snmp_device} = Repo.insert(%Device{device_id: device.id})
+
+      {:ok, _sensor} =
+        Repo.insert(%Sensor{
+          snmp_device_id: snmp_device.id,
+          sensor_type: "temperature",
+          sensor_index: "1",
+          sensor_oid: "1.3.6.1.2.1.99.1.1.1.4.1",
+          sensor_descr: "Chassis Temp"
+        })
+
+      # Mock timeout
+      SnmpMock
+      |> expect(:get, fn _target, "1.3.6.1.2.1.99.1.1.1.4.1", _opts ->
+        {:error, :timeout}
+      end)
+      |> stub(:walk, fn _, _, _ -> {:ok, []} end)
+      |> stub(:get, fn _, _, _ -> {:error, :no_such_object} end)
+
+      assert :ok = DevicePollerWorker.perform(%Oban.Job{args: %{"device_id" => device.id}})
+    end
+  end
+
   describe "start_polling/1" do
     test "schedules initial job with offset", %{site: site} do
       {:ok, device} =

test/towerops/workers/job_health_check_worker_test.exs

@@ -0,0 +1,93 @@
+defmodule Towerops.Workers.JobHealthCheckWorkerTest do
+  use Towerops.DataCase
+
+  import Ecto.Query
+
+  alias Towerops.DevicesFixtures
+  alias Towerops.Repo
+  alias Towerops.Workers.JobHealthCheckWorker
+
+  describe "perform/1" do
+    test "recovers missing monitor jobs" do
+      device = DevicesFixtures.device_fixture(%{monitoring_enabled: true})
+
+      # Ensure no jobs exist initially
+      Repo.delete_all(Oban.Job)
+
+      assert :ok = JobHealthCheckWorker.perform(%Oban.Job{})
+
+      # Verify job was created
+      assert Repo.one(
+               from j in Oban.Job,
+                 where:
+                   j.worker == "Towerops.Workers.DeviceMonitorWorker" and fragment("args->>'device_id' = ?", ^device.id)
+             )
+    end
+
+    test "recovers missing poller jobs" do
+      device = DevicesFixtures.device_fixture(%{snmp_enabled: true})
+
+      # Ensure no jobs exist initially
+      Repo.delete_all(Oban.Job)
+
+      assert :ok = JobHealthCheckWorker.perform(%Oban.Job{})
+
+      # Verify job was created
+      assert Repo.one(
+               from j in Oban.Job,
+                 where:
+                   j.worker == "Towerops.Workers.DevicePollerWorker" and fragment("args->>'device_id' = ?", ^device.id)
+             )
+    end
+
+    test "does not create duplicate jobs if they already exist" do
+      _device = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: true})
+
+      # Jobs are already created by device_fixture
+
+      initial_job_count = Repo.aggregate(Oban.Job, :count)
+      assert initial_job_count == 2
+
+      assert :ok = JobHealthCheckWorker.perform(%Oban.Job{})
+
+      final_job_count = Repo.aggregate(Oban.Job, :count)
+      assert final_job_count == initial_job_count
+    end
+
+    test "handles mixed scenario correctly" do
+      # Device 1: Monitoring enabled (should have 1 job)
+      device1 = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: false})
+
+      # Device 2: SNMP enabled (should have 1 job)
+      device2 = DevicesFixtures.device_fixture(%{monitoring_enabled: false, snmp_enabled: true})
+
+      # Device 3: Both enabled (should have 2 jobs)
+      _device3 = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: true})
+
+      # Simulate missing jobs for device 1 and 2 by deleting them
+      Repo.delete_all(from j in Oban.Job, where: fragment("args->>'device_id' = ?", ^device1.id))
+      Repo.delete_all(from j in Oban.Job, where: fragment("args->>'device_id' = ?", ^device2.id))
+
+      # Ensure we only have jobs for device 3 left (2 jobs)
+      assert Repo.aggregate(Oban.Job, :count) == 2
+
+      assert :ok = JobHealthCheckWorker.perform(%Oban.Job{})
+
+      # Verify jobs created for device 1 and 2
+      assert Repo.one(
+               from j in Oban.Job,
+                 where:
+                   j.worker == "Towerops.Workers.DeviceMonitorWorker" and fragment("args->>'device_id' = ?", ^device1.id)
+             )
+
+      assert Repo.one(
+               from j in Oban.Job,
+                 where:
+                   j.worker == "Towerops.Workers.DevicePollerWorker" and fragment("args->>'device_id' = ?", ^device2.id)
+             )
+
+      # Total jobs should be 4 (2 existing + 2 new)
+      assert Repo.aggregate(Oban.Job, :count) == 4
+    end
+  end
+end