Fix script and device pagination (#2400)

Turns out the device controller was broken as well

Fixes #2392

Author: kybishop

lib/nerves_hub_web/controllers/api/device_controller.ex

@@ -11,6 +11,7 @@ defmodule NervesHubWeb.API.DeviceController do
   alias NervesHub.Products
   alias NervesHub.Repo
 
+  alias NervesHubWeb.API.PaginationHelpers
   alias NervesHubWeb.Endpoint
   alias NervesHubWeb.Helpers.RoleValidateHelpers
 
@@ -34,18 +35,16 @@ defmodule NervesHubWeb.API.DeviceController do
 
   def index(%{assigns: %{org: org, product: product}} = conn, params) do
     opts = %{
-      pagination: Map.get(params, "pagination", %{}),
+      pagination: PaginationHelpers.atomize_pagination_params(Map.get(params, "pagination", %{})),
       filters: Map.get(params, "filters", %{})
     }
 
     {devices, page} =
       Devices.get_devices_by_org_id_and_product_id_with_pager(org.id, product.id, opts)
 
-    pagination = Map.take(page, [:page_number, :page_size, :total_entries, :total_pages])
-
     conn
     |> assign(:devices, devices)
-    |> assign(:pagination, pagination)
+    |> assign(:pagination, PaginationHelpers.format_pagination_meta(page))
     |> render(:index)
   end
 

lib/nerves_hub_web/controllers/api/pagination_helpers.ex

@@ -0,0 +1,72 @@
+defmodule NervesHubWeb.API.PaginationHelpers do
+  @moduledoc """
+  Shared helper functions for handling pagination in API controllers.
+  """
+
+  @doc """
+  Converts pagination parameters from string keys and values (as they come from URL params)
+  to atom keys with integer values (as expected by Flop and internal APIs).
+
+  ## Examples
+
+      iex> atomize_pagination_params(%{"page" => "1", "page_size" => "25"})
+      %{page: 1, page_size: 25}
+
+      iex> atomize_pagination_params(%{"page" => 2, "page_size" => 10})
+      %{page: 2, page_size: 10}
+
+      iex> atomize_pagination_params(%{})
+      %{}
+
+      iex> atomize_pagination_params(nil)
+      %{}
+  """
+  @spec atomize_pagination_params(map() | nil) :: map()
+  def atomize_pagination_params(pagination) when is_map(pagination) do
+    pagination
+    |> Enum.reduce(%{}, fn
+      {"page", value}, acc when is_binary(value) ->
+        Map.put(acc, :page, String.to_integer(value))
+
+      {"page", value}, acc when is_integer(value) ->
+        Map.put(acc, :page, value)
+
+      {"page_size", value}, acc when is_binary(value) ->
+        Map.put(acc, :page_size, String.to_integer(value))
+
+      {"page_size", value}, acc when is_integer(value) ->
+        Map.put(acc, :page_size, value)
+
+      _other, acc ->
+        acc
+    end)
+  end
+
+  def atomize_pagination_params(_), do: %{}
+
+  @doc """
+  Converts Flop.Meta struct fields to the standard pagination response format
+  used by the API.
+
+  Maps:
+  - current_page -> page_number
+  - total_count -> total_entries
+  - page_size -> page_size (unchanged)
+  - total_pages -> total_pages (unchanged)
+
+  ## Examples
+
+      iex> meta = %Flop.Meta{current_page: 2, page_size: 10, total_count: 45, total_pages: 5}
+      iex> format_pagination_meta(meta)
+      %{page_number: 2, page_size: 10, total_entries: 45, total_pages: 5}
+  """
+  @spec format_pagination_meta(Flop.Meta.t()) :: map()
+  def format_pagination_meta(%Flop.Meta{} = meta) do
+    %{
+      page_number: meta.current_page,
+      page_size: meta.page_size,
+      total_entries: meta.total_count,
+      total_pages: meta.total_pages
+    }
+  end
+end

lib/nerves_hub_web/controllers/api/script_controller.ex

@@ -3,6 +3,7 @@ defmodule NervesHubWeb.API.ScriptController do
   use OpenApiSpex.ControllerSpecs
 
   alias NervesHub.Scripts
+  alias NervesHubWeb.API.PaginationHelpers
 
   security([%{}, %{"bearer_auth" => []}])
   tags(["Support Scripts"])
@@ -55,18 +56,15 @@ defmodule NervesHubWeb.API.ScriptController do
           into: %{},
           do: {String.to_existing_atom(key), val}
 
-    opts = %{
-      pagination: Map.get(params, "pagination", %{}),
-      filters: filters
-    }
-
-    {scripts, page} = Scripts.filter(product, opts)
-
-    pagination = Map.take(page, [:page_number, :page_size, :total_entries, :total_pages])
+    {scripts, page} =
+      Scripts.filter(product, %{
+        pagination: PaginationHelpers.atomize_pagination_params(Map.get(params, "pagination", %{})),
+        filters: filters
+      })
 
     conn
     |> assign(:scripts, scripts)
-    |> assign(:pagination, pagination)
+    |> assign(:pagination, PaginationHelpers.format_pagination_meta(page))
     |> render(:index)
   end
 end

lib/nerves_hub_web/controllers/api/script_json.ex

@@ -1,9 +1,10 @@
 defmodule NervesHubWeb.API.ScriptJSON do
   @moduledoc false
 
-  def index(%{scripts: scripts}) do
+  def index(%{scripts: scripts, pagination: pagination}) do
     %{
-      data: for(script <- scripts, do: script(script))
+      data: for(script <- scripts, do: script(script)),
+      pagination: pagination
     }
   end
 

test/nerves_hub_web/controllers/api/device_controller_test.exs

@@ -45,7 +45,45 @@ defmodule NervesHubWeb.API.DeviceControllerTest do
 
       conn = get(conn, Routes.api_device_path(conn, :index, org.name, product.name))
 
-      assert json_response(conn, 200)["data"]
+      assert json_response(conn, 200) == %{
+               "data" => [
+                 %{
+                   "connection_status" => "not_seen",
+                   "deleted" => false,
+                   "deployment_group" => nil,
+                   "description" => nil,
+                   "firmware_metadata" => %{
+                     "architecture" => "x86_64",
+                     "author" => "me",
+                     "description" => "D ",
+                     "fwup_version" => "1.0.0",
+                     "id" => device.firmware_metadata.id,
+                     "misc" => nil,
+                     "platform" => "platform",
+                     "product" => "valid product",
+                     "uuid" => firmware.uuid,
+                     "vcs_identifier" => nil,
+                     "version" => "1.0.0"
+                   },
+                   "identifier" => device.identifier,
+                   "last_communication" => "never",
+                   "online" => "not_seen",
+                   "org_name" => "Test-Org",
+                   "priority_updates" => false,
+                   "product_name" => "valid product",
+                   "tags" => ["beta", "beta-edge"],
+                   "updates_blocked_until" => nil,
+                   "updates_enabled" => true,
+                   "version" => "1.0.0"
+                 }
+               ],
+               "pagination" => %{
+                 "page_number" => 1,
+                 "page_size" => nil,
+                 "total_entries" => 1,
+                 "total_pages" => 1
+               }
+             }
 
       assert Enum.find(conn.assigns.devices, fn %{identifier: identifier} ->
                device.identifier == identifier

test/nerves_hub_web/controllers/api/script_controller_test.exs

@@ -44,6 +44,104 @@ defmodule NervesHubWeb.API.ScriptControllerTest do
       assert Enum.count(data) == 1
       assert script_response["id"] == script_with_tags.id
     end
+
+    test "returns pagination metadata", %{conn: conn, product: product, device: device, user: user} do
+      _script = Fixtures.support_script_fixture(product, user)
+
+      conn = get(conn, Routes.api_script_path(conn, :index, device))
+      response = json_response(conn, 200)
+
+      assert Map.has_key?(response, "data")
+      assert Map.has_key?(response, "pagination")
+
+      pagination = response["pagination"]
+      assert pagination["page_number"] == 1
+      assert pagination["page_size"] == 25
+      assert pagination["total_entries"] == 1
+      assert pagination["total_pages"] == 1
+    end
+
+    test "respects pagination parameters with device scope", %{conn: conn, product: product, device: device, user: user} do
+      # Create 5 scripts
+      _scripts = for i <- 1..5, do: Fixtures.support_script_fixture(product, user, %{name: "script#{i}"})
+
+      # Get first page with page_size=2
+      conn =
+        get(conn, Routes.api_script_path(conn, :index, device), %{
+          pagination: %{page: 1, page_size: 2}
+        })
+
+      response = json_response(conn, 200)
+
+      assert length(response["data"]) == 2
+      assert response["pagination"]["page_number"] == 1
+      assert response["pagination"]["page_size"] == 2
+      assert response["pagination"]["total_entries"] == 5
+      assert response["pagination"]["total_pages"] == 3
+
+      # Get second page with page_size=2
+      conn =
+        get(conn, Routes.api_script_path(conn, :index, device), %{
+          pagination: %{page: 2, page_size: 2}
+        })
+
+      response = json_response(conn, 200)
+
+      assert length(response["data"]) == 2
+      assert response["pagination"]["page_number"] == 2
+      assert response["pagination"]["page_size"] == 2
+      assert response["pagination"]["total_entries"] == 5
+      assert response["pagination"]["total_pages"] == 3
+
+      # Get third page with page_size=2 (should have 1 item)
+      conn =
+        get(conn, Routes.api_script_path(conn, :index, device), %{
+          pagination: %{page: 3, page_size: 2}
+        })
+
+      response = json_response(conn, 200)
+
+      assert length(response["data"]) == 1
+      assert response["pagination"]["page_number"] == 3
+      assert response["pagination"]["total_entries"] == 5
+    end
+
+    test "respects pagination parameters with product scope", %{conn: conn, org: org, product: product, user: user} do
+      # Create 3 scripts
+      _scripts = for i <- 1..3, do: Fixtures.support_script_fixture(product, user, %{name: "script#{i}"})
+
+      # Get page with custom page_size
+      conn =
+        get(conn, Routes.api_script_path(conn, :index, org.name, product.name), %{
+          pagination: %{page: 1, page_size: 2}
+        })
+
+      response = json_response(conn, 200)
+
+      assert length(response["data"]) == 2
+      assert response["pagination"]["page_number"] == 1
+      assert response["pagination"]["page_size"] == 2
+      assert response["pagination"]["total_entries"] == 3
+      assert response["pagination"]["total_pages"] == 2
+    end
+
+    test "handles string pagination parameters", %{conn: conn, product: product, device: device, user: user} do
+      # Create 3 scripts
+      _scripts = for i <- 1..3, do: Fixtures.support_script_fixture(product, user, %{name: "script#{i}"})
+
+      # Send pagination params as strings (as they come from URL query params)
+      conn =
+        get(conn, Routes.api_script_path(conn, :index, device), %{
+          pagination: %{"page" => "2", "page_size" => "1"}
+        })
+
+      response = json_response(conn, 200)
+
+      assert length(response["data"]) == 1
+      assert response["pagination"]["page_number"] == 2
+      assert response["pagination"]["page_size"] == 1
+      assert response["pagination"]["total_entries"] == 3
+    end
   end
 
   describe "send" do