From 160b52863186612c5183f0e9c4bf44475018ce9b Mon Sep 17 00:00:00 2001
From: Adam Thornton <athornton@lsst.org>
Date: Tue, 22 Oct 2024 13:39:21 -0700
Subject: [PATCH] Refactor github path construction and add tests

---
 src/ghostwriter/hooks/github_notebook.py | 15 +++++-
 tests/hooks/serial_test.py               | 69 ++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 tests/hooks/serial_test.py

diff --git a/src/ghostwriter/hooks/github_notebook.py b/src/ghostwriter/hooks/github_notebook.py
index bf72c39..e83867c 100644
--- a/src/ghostwriter/hooks/github_notebook.py
+++ b/src/ghostwriter/hooks/github_notebook.py
@@ -30,20 +30,30 @@ async def github_notebook(params: Parameters) -> Parameters:
     # Honestly it's easier to just unconditionally rewrite the target
     # than to figure out whether it needs rewriting.
 
+    return _get_new_params(serial, params)
+
+
+def _get_new_params(serial: str, params: Parameters) -> Parameters:
     # Start with the common fragment
     target = (
         f"{params.base_url}nb/user/{params.user}/lab/tree/notebooks"
         "/on-demand/github.com/"
     )
+
     # Remove branch information, if any (the checkout will have handled
     # it in the code we ran to get the serial).
     path = params.path.split("@")[0]
-    if path.startswith("notebooks/github.com/"):
+
+    # Canonicalize path.
+    prefix = "notebooks/github.com/"
+    if path.startswith(prefix):
         # Needs stripping
-        path = path[(len("notebooks/github.com/")) :]
+        path = path[(len(prefix)) :]
     if path.endswith(".ipynb"):
         # Also needs stripping
         path = path[: -(len(".ipynb"))]
+
+    # Add discriminator if needed
     unique_id: str | None = None
     if serial and serial != "0":
         # Glue in serial if it is nonzero
@@ -51,6 +61,7 @@ async def github_notebook(params: Parameters) -> Parameters:
         path += f"-{unique_id}"
     path += ".ipynb"  # And add the extension
     target += path
+
     new_param = Parameters(
         user=params.user,
         base_url=params.base_url,
diff --git a/tests/hooks/serial_test.py b/tests/hooks/serial_test.py
new file mode 100644
index 0000000..63a02e5
--- /dev/null
+++ b/tests/hooks/serial_test.py
@@ -0,0 +1,69 @@
+"""Test the filename construction from serial bit of the github hook."""
+
+import json
+from pathlib import Path
+
+import pytest
+import pytest_asyncio
+from httpx import AsyncClient
+from rubin.nublado.client import NubladoClient
+from rubin.nublado.client.models import User
+
+from ghostwriter.config import Configuration
+from ghostwriter.hooks.github_notebook import _get_new_params
+from ghostwriter.models.substitution import Parameters
+
+COMMON_PREFIX = (
+    "https://data.example.org/nb/user/rachel/lab/tree/notebooks"
+    "/on-demand/github.com/lsst-sqre/system-test/Firefly"
+)
+
+
+@pytest_asyncio.fixture
+def std_params(config: Configuration, client: AsyncClient) -> Parameters:
+    user_objs = json.loads(
+        (Path(__file__).parent.parent / "support" / "users.json").read_text()
+    )
+    token = next(iter(user_objs.keys()))
+    username = user_objs[token]["username"]
+    user = User(username=username, token=token)
+    nc = NubladoClient(user=user, base_url=str(config.environment_url))
+    return Parameters(
+        user=user.username,
+        base_url=str(config.environment_url),
+        token=token,
+        client=nc,
+        path="notebooks/github.com/lsst-sqre/system-test/Firefly",
+        target="inconsequential",
+        unique_id=None,
+    )
+
+
+@pytest.mark.asyncio
+async def test_basic_rewrite(std_params: Parameters) -> None:
+    new_p = _get_new_params("0", std_params)
+    expected = f"{COMMON_PREFIX}.ipynb"
+    assert new_p.target == expected
+
+
+@pytest.mark.asyncio
+async def test_serial_rewrite(std_params: Parameters) -> None:
+    new_p = _get_new_params("3", std_params)
+    expected = f"{COMMON_PREFIX}-3.ipynb"
+    assert new_p.target == expected
+
+
+@pytest.mark.asyncio
+async def test_strip_branch(std_params: Parameters) -> None:
+    inp = Parameters(
+        user=std_params.user,
+        base_url=std_params.base_url,
+        token=std_params.token,
+        client=std_params.client,
+        path=f"{std_params.path}@prod",
+        target=std_params.target,
+        unique_id=std_params.unique_id,
+    )
+    new_p = _get_new_params("0", inp)
+    expected = f"{COMMON_PREFIX}.ipynb"
+    assert new_p.target == expected