Refactor github path construction and add tests

docs/_static/openapi.json

@@ -1 +1 @@
-{"openapi": "3.1.0", "info": {"title": "Ghostwriter", "description": "URL shortener/personalizer for Phalanx\n\n[Return to Ghostwriter documentation](.)", "version": "0.1.2.dev6+ge335638.d20241017"}, "paths": {"/ghostwriter/": {"get": {"summary": "Application metadata", "description": "Document the top-level API here. By default it only returns metadata about the application.", "operationId": "get_index_ghostwriter__get", "responses": {"200": {"description": "Successful Response", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/Index"}}}}}}}, "/ghostwriter/rewrite/{full_path}": {"get": {"summary": "Rewrite", "operationId": "rewrite_ghostwriter_rewrite__full_path__get", "parameters": [{"name": "full_path", "in": "path", "required": true, "schema": {"type": "string", "title": "The URL path to rewrite"}}], "responses": {"307": {"description": "Successful Response"}, "422": {"description": "Validation Error", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/HTTPValidationError"}}}}}}}}, "components": {"schemas": {"HTTPValidationError": {"properties": {"detail": {"items": {"$ref": "#/components/schemas/ValidationError"}, "type": "array", "title": "Detail"}}, "type": "object", "title": "HTTPValidationError"}, "Index": {"properties": {"metadata": {"$ref": "#/components/schemas/Metadata", "title": "Package metadata"}}, "type": "object", "required": ["metadata"], "title": "Index", "description": "Metadata returned by the external root URL of the application.\n\nNotes\n-----\nAs written, this is not very useful. Add additional metadata that will be\nhelpful for a user exploring the application, or replace this model with\nsome other model that makes more sense to return from the application API\nroot."}, "Metadata": {"properties": {"name": {"type": "string", "title": "Application name", "examples": ["myapp"]}, "version": {"type": "string", "title": "Version", "examples": ["1.0.0"]}, "description": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Description", "examples": ["Some package description"]}, "repository_url": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Repository URL", "examples": ["https://example.com/"]}, "documentation_url": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Documentation URL", "examples": ["https://example.com/"]}}, "type": "object", "required": ["name", "version"], "title": "Metadata", "description": "Metadata about a package."}, "ValidationError": {"properties": {"loc": {"items": {"anyOf": [{"type": "string"}, {"type": "integer"}]}, "type": "array", "title": "Location"}, "msg": {"type": "string", "title": "Message"}, "type": {"type": "string", "title": "Error Type"}}, "type": "object", "required": ["loc", "msg", "type"], "title": "ValidationError"}}}}
+{"openapi": "3.1.0", "info": {"title": "Ghostwriter", "description": "URL shortener/personalizer for Phalanx\n\n[Return to Ghostwriter documentation](.)", "version": "0.1.2.dev6+ge335638.d20241015"}, "paths": {"/ghostwriter/": {"get": {"summary": "Application metadata", "description": "Document the top-level API here. By default it only returns metadata about the application.", "operationId": "get_index_ghostwriter__get", "responses": {"200": {"description": "Successful Response", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/Index"}}}}}}}, "/ghostwriter/rewrite/{full_path}": {"get": {"summary": "Rewrite", "operationId": "rewrite_ghostwriter_rewrite__full_path__get", "parameters": [{"name": "full_path", "in": "path", "required": true, "schema": {"type": "string", "title": "The URL path to rewrite"}}], "responses": {"307": {"description": "Successful Response"}, "422": {"description": "Validation Error", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/HTTPValidationError"}}}}}}}}, "components": {"schemas": {"HTTPValidationError": {"properties": {"detail": {"items": {"$ref": "#/components/schemas/ValidationError"}, "type": "array", "title": "Detail"}}, "type": "object", "title": "HTTPValidationError"}, "Index": {"properties": {"metadata": {"$ref": "#/components/schemas/Metadata", "title": "Package metadata"}}, "type": "object", "required": ["metadata"], "title": "Index", "description": "Metadata returned by the external root URL of the application.\n\nNotes\n-----\nAs written, this is not very useful. Add additional metadata that will be\nhelpful for a user exploring the application, or replace this model with\nsome other model that makes more sense to return from the application API\nroot."}, "Metadata": {"properties": {"name": {"type": "string", "title": "Application name", "examples": ["myapp"]}, "version": {"type": "string", "title": "Version", "examples": ["1.0.0"]}, "description": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Description", "examples": ["Some package description"]}, "repository_url": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Repository URL", "examples": ["https://example.com/"]}, "documentation_url": {"anyOf": [{"type": "string"}, {"type": "null"}], "title": "Documentation URL", "examples": ["https://example.com/"]}}, "type": "object", "required": ["name", "version"], "title": "Metadata", "description": "Metadata about a package."}, "ValidationError": {"properties": {"loc": {"items": {"anyOf": [{"type": "string"}, {"type": "integer"}]}, "type": "array", "title": "Location"}, "msg": {"type": "string", "title": "Message"}, "type": {"type": "string", "title": "Error Type"}}, "type": "object", "required": ["loc", "msg", "type"], "title": "ValidationError"}}}}

docs/hooks/index.rst

@@ -9,7 +9,7 @@ There is a list of hooks (possibly empty) for each route specified in the config
 Each hook is run in sequence. If any hook raises an exception, the user will receive an error rather than a redirect.
 
 Because (at least at present) hooks are, by definition, located inside the ``ghostwriter.hooks`` Python module namespace, any additions or modifications to hooks are code and repository changes.
-Therefore, any proposed hooks or modififactions to existing ones will need to go through the SQuaRE PR process.
+Therefore, any proposed hooks or modifications to existing ones will need to go through the SQuaRE PR process.
 
 Anatomy of a Hook
 =================

src/ghostwriter/hooks/github_notebook.py

@@ -30,27 +30,38 @@ 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
         unique_id = serial
         path += f"-{unique_id}"
     path += ".ipynb"  # And add the extension
     target += path
+
     new_param = Parameters(
         user=params.user,
         base_url=params.base_url,

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