From baed88afee54f8fc9bd8208b7e67ed698ae424b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Fri, 29 May 2026 15:23:55 +0200 Subject: [PATCH 1/6] feat(channels): add CLI for managing publication channels #NP-51227 --- cli.py | 2 + commands/channels.py | 332 +++++++++++++++++ commands/services/channels_api.py | 245 +++++++++++++ commands/services/tests/test_channels.py | 437 +++++++++++++++++++++++ 4 files changed, 1016 insertions(+) create mode 100644 commands/channels.py create mode 100644 commands/services/channels_api.py create mode 100644 commands/services/tests/test_channels.py diff --git a/cli.py b/cli.py index db8c4e1..cf5930b 100755 --- a/cli.py +++ b/cli.py @@ -2,6 +2,7 @@ import click import logging +from commands.channels import channels from commands.cognito import cognito from commands.dlq import dlq from commands.dynamodb import dynamodb @@ -51,6 +52,7 @@ def cli(ctx: click.Context, log_level: int, profile: str | None): ) +cli.add_command(channels) cli.add_command(cognito) cli.add_command(dlq) cli.add_command(dynamodb) diff --git a/commands/channels.py b/commands/channels.py new file mode 100644 index 0000000..c22c140 --- /dev/null +++ b/commands/channels.py @@ -0,0 +1,332 @@ +import functools +import logging +from typing import Optional + +import click +import requests +from rich.console import Console +from rich.table import Table + +from commands.services.channels_api import ( + KIND_JOURNAL, + KIND_PUBLISHER, + KIND_SERIAL, + KIND_SERIES, + ChannelNotFoundError, + ChannelsApiService, +) +from commands.utils import AppContext + +logger = logging.getLogger(__name__) + +KIND_CHOICES = ["serial", "journal", "series", "publisher"] + + +def _handle_api_errors(func): + @functools.wraps(func) + def wrapper(*args, **kwargs): + try: + return func(*args, **kwargs) + except ChannelNotFoundError as exc: + raise click.ClickException(str(exc)) + except requests.HTTPError as exc: + raise click.ClickException(_format_http_error(exc)) + + return wrapper + + +@click.group() +@click.pass_obj +def channels(ctx: AppContext): + """Manage publication channels (journals, series, publishers). + + Type is auto-detected where possible so you rarely need --kind. + """ + pass + + +@channels.command() +@click.argument("query") +@click.option( + "--kind", + type=click.Choice(KIND_CHOICES), + default=None, + help="Restrict to a single channel kind. Default: search all kinds.", +) +@click.option("--year", type=int, default=None, help="Limit results to a given year") +@click.option("--offset", type=int, default=0) +@click.option("--size", type=int, default=10) +@click.pass_obj +@_handle_api_errors +def search( + ctx: AppContext, + query: str, + kind: Optional[str], + year: Optional[int], + offset: int, + size: int, +) -> None: + """Search channels across journals/series/publishers.""" + service = ChannelsApiService(ctx.profile) + rows = _collect_search_rows(service, query, kind, year, offset, size) + _render_table(rows, query) + + +@channels.command() +@click.argument("identifier") +@click.option("--year", type=int, default=None, help="Fetch data for a given year") +@click.option( + "--kind", + type=click.Choice(KIND_CHOICES), + default=None, + help="Force lookup of a specific kind. Default: auto-detect.", +) +@click.pass_obj +@_handle_api_errors +def get( + ctx: AppContext, identifier: str, year: Optional[int], kind: Optional[str] +) -> None: + """Fetch a single channel by identifier (auto-detects type).""" + service = ChannelsApiService(ctx.profile) + if kind is None: + channel = service.fetch_auto(identifier, year) + else: + channel = service.fetch(_resolve_kind(kind), identifier, year) + channel.setdefault("_resolvedKind", _resolve_kind(kind)) + _print_channel(channel) + + +@channels.command() +@click.option("--name", required=True, help="Channel name") +@click.option( + "--kind", + type=click.Choice(["publisher", "journal", "series", "serial"]), + default=None, + help="Explicit kind. Default: inferred from other flags.", +) +@click.option("--isbn-prefix", default=None, help="Publisher only") +@click.option("--print-issn", default=None, help="Journal/series only") +@click.option("--online-issn", default=None, help="Journal/series only") +@click.option("--homepage", default=None, help="Channel homepage URL") +@click.pass_obj +@_handle_api_errors +def create( + ctx: AppContext, + name: str, + kind: Optional[str], + isbn_prefix: Optional[str], + print_issn: Optional[str], + online_issn: Optional[str], + homepage: Optional[str], +) -> None: + """Create a new channel. Picks publisher vs serial-publication from flags.""" + resolved_kind = _infer_create_kind(kind, isbn_prefix, print_issn, online_issn) + service = ChannelsApiService(ctx.profile) + + if resolved_kind == KIND_PUBLISHER: + if print_issn or online_issn: + raise click.UsageError( + "ISSN flags are not valid for publisher; remove or set --kind" + ) + result = service.create_publisher(name, isbn_prefix, homepage) + elif resolved_kind == KIND_JOURNAL: + _reject_isbn(isbn_prefix) + result = service.create_journal(name, print_issn, online_issn, homepage) + elif resolved_kind == KIND_SERIES: + _reject_isbn(isbn_prefix) + result = service.create_series(name, print_issn, online_issn, homepage) + else: + _reject_isbn(isbn_prefix) + result = service.create_serial_publication( + name, "Journal", print_issn, online_issn, homepage + ) + + click.echo(f"CREATED {resolved_kind}: {name}") + _print_channel(result) + + +@channels.command() +@click.argument("identifier") +@click.option("--name", default=None, help="New name") +@click.option("--isbn", default=None, help="Publisher only") +@click.option("--print-issn", default=None, help="Serial publication only") +@click.option("--online-issn", default=None, help="Serial publication only") +@click.pass_obj +@_handle_api_errors +def update( + ctx: AppContext, + identifier: str, + name: Optional[str], + isbn: Optional[str], + print_issn: Optional[str], + online_issn: Optional[str], +) -> None: + """Update an existing channel. Type is detected from the channel itself.""" + if name is None and isbn is None and print_issn is None and online_issn is None: + raise click.UsageError("Specify at least one field to update.") + + service = ChannelsApiService(ctx.profile) + existing = service.fetch_auto(identifier) + resolved_kind = existing.get("_resolvedKind") + + if resolved_kind == KIND_PUBLISHER: + if print_issn or online_issn: + raise click.UsageError("ISSN flags are not valid for a publisher channel") + service.update_publisher(identifier, name=name, isbn=isbn) + else: + if isbn: + raise click.UsageError("--isbn is only valid for publisher channels") + service.update_serial_publication( + identifier, name=name, print_issn=print_issn, online_issn=online_issn + ) + click.echo(f"UPDATED {resolved_kind} {identifier}") + + +@channels.command() +@click.argument("identifier") +@click.option("--yes", is_flag=True, default=False, help="Skip confirmation prompt") +@click.pass_obj +@_handle_api_errors +def delete(ctx: AppContext, identifier: str, yes: bool) -> None: + """Delete a channel by identifier.""" + service = ChannelsApiService(ctx.profile) + existing = service.fetch_auto(identifier) + + name = existing.get("name", "?") + resolved_kind = existing.get("_resolvedKind", "?") + if not yes: + click.confirm(f"Delete {resolved_kind} '{name}' ({identifier})?", abort=True) + + service.delete_channel(identifier) + click.echo(f"DELETED {resolved_kind} {identifier}") + + +def _resolve_kind(kind: str) -> str: + return KIND_SERIAL if kind == "serial" else kind + + +def _format_http_error(exc: requests.HTTPError) -> str: + response = exc.response + if response is None: + return f"API error: {exc}" + snippet = response.text[:500].strip() if response.text else "" + base = f"API error {response.status_code} from {response.url}" + return f"{base}\n {snippet}" if snippet else base + + +def _infer_create_kind( + kind: Optional[str], + isbn_prefix: Optional[str], + print_issn: Optional[str], + online_issn: Optional[str], +) -> str: + if kind: + return _resolve_kind(kind) + if isbn_prefix: + return KIND_PUBLISHER + if print_issn or online_issn: + return KIND_SERIAL + raise click.UsageError( + "Cannot infer channel kind. Pass --kind or one of " + "--isbn-prefix / --print-issn / --online-issn." + ) + + +def _reject_isbn(isbn_prefix: Optional[str]) -> None: + if isbn_prefix: + raise click.UsageError("--isbn-prefix is only valid for publisher channels") + + +def _collect_search_rows( + service: ChannelsApiService, + query: str, + kind: Optional[str], + year: Optional[int], + offset: int, + size: int, +) -> list: + rows: list = [] + if kind in (None, "serial", "journal", "series"): + serial_kind = _resolve_kind(kind) if kind else KIND_SERIAL + rows.extend( + _rows_from_hits(service.search(serial_kind, query, year, offset, size)) + ) + if kind in (None, "publisher"): + rows.extend( + _rows_from_hits(service.search(KIND_PUBLISHER, query, year, offset, size)) + ) + return rows + + +def _rows_from_hits(payload: dict) -> list: + return [_row_from_hit(hit) for hit in payload.get("hits", [])] + + +def _row_from_hit(hit: dict) -> dict: + return { + "type": hit.get("type", "?"), + "identifier": _identifier_from_id(hit.get("id", "")), + "name": hit.get("name", ""), + "issn_or_isbn": ( + hit.get("printIssn") or hit.get("onlineIssn") or hit.get("isbnPrefix") or "" + ), + "year": hit.get("year") or "", + } + + +def _identifier_from_id(channel_id: str) -> str: + if not channel_id: + return "" + stripped = channel_id.rstrip("/") + parts = stripped.split("/") + if len(parts) >= 2 and parts[-1].isdigit() and len(parts[-1]) == 4: + return parts[-2] + return parts[-1] + + +def _render_table(rows: list, query: str) -> None: + console = Console() + if not rows: + console.print(f"[yellow]No hits for {query!r}[/yellow]") + return + table = Table( + show_header=True, + header_style="bold cyan", + title=f"[bold magenta]Channels matching {query!r} ({len(rows)} hits)[/bold magenta]", + ) + table.add_column("Type") + table.add_column("Identifier") + table.add_column("Name") + table.add_column("ISSN/ISBN") + table.add_column("Year") + for row in rows: + table.add_row( + row["type"], + row["identifier"], + row["name"], + row["issn_or_isbn"], + str(row["year"]), + ) + console.print(table) + + +def _print_channel(channel: dict) -> None: + console = Console() + resolved = channel.get("_resolvedKind") + if resolved: + console.print(f"[bold]Resolved kind:[/bold] {resolved}") + keys = [ + "id", + "type", + "name", + "printIssn", + "onlineIssn", + "isbnPrefix", + "homepage", + "scientificValue", + "year", + "sameAs", + ] + for key in keys: + if key in channel and channel[key] not in (None, ""): + console.print(f" {key}: {channel[key]}") diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py new file mode 100644 index 0000000..ab3d63b --- /dev/null +++ b/commands/services/channels_api.py @@ -0,0 +1,245 @@ +import json +from datetime import datetime, timedelta +from typing import Optional + +import boto3 +import requests + +CHANNEL_BASE_PATH = "publication-channels-v2" +DELETE_PATH_SEGMENT = "channel" + +KIND_PUBLISHER = "publisher" +KIND_SERIAL = "serial-publication" +KIND_JOURNAL = "journal" +KIND_SERIES = "series" + +VALID_KINDS = {KIND_PUBLISHER, KIND_SERIAL, KIND_JOURNAL, KIND_SERIES} + + +class ChannelNotFoundError(Exception): + pass + + +class ChannelsApiService: + def __init__(self, profile: Optional[str]): + self.session = boto3.Session(profile_name=profile) + self.ssm = self.session.client("ssm") + self.secretsmanager = self.session.client("secretsmanager") + self.api_domain = self._get_system_parameter("/NVA/ApiDomain") + self.cognito_uri = self._get_system_parameter("/NVA/CognitoUri") + self.client_credentials = self._get_secret("BackendCognitoClientCredentials") + self.token: Optional[str] = None + self.token_expiry_time = datetime.now() + + def search( + self, + kind: str, + query: str, + year: Optional[int] = None, + offset: int = 0, + size: int = 10, + ) -> dict: + params = {"query": query, "offset": offset, "size": size} + if year is not None: + params["year"] = year + response = requests.get(self._channel_url(kind), params=params) + response.raise_for_status() + return response.json() + + def fetch(self, kind: str, identifier: str, year: Optional[int] = None) -> dict: + url = f"{self._channel_url(kind)}/{identifier}" + if year is not None: + url = f"{url}/{year}" + response = requests.get(url) + if response.status_code == 404: + raise ChannelNotFoundError(f"{kind}/{identifier} not found") + response.raise_for_status() + return response.json() + + def fetch_auto(self, identifier: str, year: Optional[int] = None) -> dict: + last_http_error: Optional[requests.HTTPError] = None + for kind in (KIND_SERIAL, KIND_PUBLISHER): + try: + channel = self.fetch(kind, identifier, year) + channel.setdefault("_resolvedKind", kind) + return channel + except ChannelNotFoundError: + continue + except requests.HTTPError as exc: + status = exc.response.status_code if exc.response is not None else 0 + if 500 <= status < 600: + last_http_error = exc + continue + raise + if last_http_error is not None: + raise last_http_error + raise ChannelNotFoundError( + f"No channel with identifier {identifier} found in serial-publication or publisher" + ) + + def create_publisher( + self, + name: str, + isbn_prefix: Optional[str] = None, + homepage: Optional[str] = None, + ) -> dict: + body = _drop_none( + {"name": name, "isbnPrefix": isbn_prefix, "homepage": homepage} + ) + return self._post(KIND_PUBLISHER, body) + + def create_serial_publication( + self, + name: str, + serial_type: str, + print_issn: Optional[str] = None, + online_issn: Optional[str] = None, + homepage: Optional[str] = None, + ) -> dict: + if serial_type not in ("Series", "Journal"): + raise ValueError("serial_type must be 'Series' or 'Journal'") + body = _drop_none( + { + "name": name, + "type": serial_type, + "printIssn": print_issn, + "onlineIssn": online_issn, + "homepage": homepage, + } + ) + return self._post(KIND_SERIAL, body) + + def create_journal( + self, + name: str, + print_issn: Optional[str] = None, + online_issn: Optional[str] = None, + homepage: Optional[str] = None, + ) -> dict: + body = _drop_none( + { + "name": name, + "printIssn": print_issn, + "onlineIssn": online_issn, + "homepage": homepage, + } + ) + return self._post(KIND_JOURNAL, body) + + def create_series( + self, + name: str, + print_issn: Optional[str] = None, + online_issn: Optional[str] = None, + homepage: Optional[str] = None, + ) -> dict: + body = _drop_none( + { + "name": name, + "printIssn": print_issn, + "onlineIssn": online_issn, + "homepage": homepage, + } + ) + return self._post(KIND_SERIES, body) + + def update_publisher( + self, + identifier: str, + name: Optional[str] = None, + isbn: Optional[str] = None, + ) -> dict: + body = _drop_none( + {"type": "UpdatePublisherRequest", "name": name, "isbn": isbn} + ) + return self._put(KIND_PUBLISHER, identifier, body) + + def update_serial_publication( + self, + identifier: str, + name: Optional[str] = None, + print_issn: Optional[str] = None, + online_issn: Optional[str] = None, + ) -> dict: + body = _drop_none( + { + "type": "UpdateSerialPublicationRequest", + "name": name, + "printIssn": print_issn, + "onlineIssn": online_issn, + } + ) + return self._put(KIND_SERIAL, identifier, body) + + def delete_channel(self, identifier: str) -> None: + url = ( + f"https://{self.api_domain}/{CHANNEL_BASE_PATH}/" + f"{DELETE_PATH_SEGMENT}/{identifier}" + ) + response = requests.delete(url, headers=self._auth_headers()) + if response.status_code == 404: + raise ChannelNotFoundError(f"channel {identifier} not found") + response.raise_for_status() + + def _channel_url(self, kind: str) -> str: + if kind not in VALID_KINDS: + raise ValueError(f"Unknown channel kind: {kind}") + return f"https://{self.api_domain}/{CHANNEL_BASE_PATH}/{kind}" + + def _auth_headers(self) -> dict: + return {"Authorization": f"Bearer {self._get_token()}"} + + def _post(self, kind: str, body: dict) -> dict: + headers = self._auth_headers() | {"Content-Type": "application/ld+json"} + response = requests.post(self._channel_url(kind), headers=headers, json=body) + response.raise_for_status() + if response.text: + return response.json() + return {"location": response.headers.get("Location")} + + def _put(self, kind: str, identifier: str, body: dict) -> dict: + url = f"{self._channel_url(kind)}/{identifier}" + headers = self._auth_headers() | {"Content-Type": "application/json"} + response = requests.put(url, headers=headers, json=body) + response.raise_for_status() + if response.status_code == 202: + return {"status": "accepted"} + if response.text: + return response.json() + return {} + + def _get_system_parameter(self, name: str) -> str: + response = self.ssm.get_parameter(Name=name) + return response["Parameter"]["Value"] + + def _get_secret(self, name: str) -> dict: + response = self.secretsmanager.get_secret_value(SecretId=name) + return json.loads(response["SecretString"]) + + def _get_cognito_token(self) -> str: + url = f"{self.cognito_uri}/oauth2/token" + headers = {"Content-Type": "application/x-www-form-urlencoded"} + data = { + "grant_type": "client_credentials", + "client_id": self.client_credentials["backendClientId"], + "client_secret": self.client_credentials["backendClientSecret"], + } + response = requests.post(url, headers=headers, data=data) + response.raise_for_status() + response_json = response.json() + self.token_expiry_time = datetime.now() + timedelta( + seconds=response_json["expires_in"] + ) + return response_json["access_token"] + + def _is_token_expired(self) -> bool: + return datetime.now() > self.token_expiry_time - timedelta(seconds=30) + + def _get_token(self) -> str: + if self.token is None or self._is_token_expired(): + self.token = self._get_cognito_token() + return self.token + + +def _drop_none(body: dict) -> dict: + return {key: value for key, value in body.items() if value is not None} diff --git a/commands/services/tests/test_channels.py b/commands/services/tests/test_channels.py new file mode 100644 index 0000000..54cbe3a --- /dev/null +++ b/commands/services/tests/test_channels.py @@ -0,0 +1,437 @@ +import json + +import boto3 +import pytest +import responses +from click.testing import CliRunner +from moto import mock_aws + +from commands.channels import channels +from commands.services.channels_api import ( + ChannelNotFoundError, + ChannelsApiService, +) +from commands.utils import AppContext + +API_DOMAIN = "api.example.org" +COGNITO_URL = "https://cognito.example.org/oauth2/token" +SERIAL_URL = f"https://{API_DOMAIN}/publication-channels-v2/serial-publication" +PUBLISHER_URL = f"https://{API_DOMAIN}/publication-channels-v2/publisher" +JOURNAL_URL = f"https://{API_DOMAIN}/publication-channels-v2/journal" +SERIES_URL = f"https://{API_DOMAIN}/publication-channels-v2/series" +CHANNEL_URL = f"https://{API_DOMAIN}/publication-channels-v2/channel" + +AN_IDENTIFIER = "151f411d-68cd-4c7a-9cbb-daf00e0326ce" + + +def _ctx() -> AppContext: + return AppContext( + log_level=0, profile=None, session=boto3.Session(region_name="eu-west-1") + ) + + +def _seed_aws() -> None: + ssm = boto3.client("ssm", region_name="eu-west-1") + ssm.put_parameter(Name="/NVA/ApiDomain", Value=API_DOMAIN, Type="String") + ssm.put_parameter( + Name="/NVA/CognitoUri", Value="https://cognito.example.org", Type="String" + ) + boto3.client("secretsmanager", region_name="eu-west-1").create_secret( + Name="BackendCognitoClientCredentials", + SecretString=json.dumps( + {"backendClientId": "id", "backendClientSecret": "secret"} + ), + ) + + +def _add_cognito() -> None: + responses.add( + responses.POST, COGNITO_URL, json={"access_token": "token", "expires_in": 3600} + ) + + +def _a_serial_hit() -> dict: + return { + "id": f"https://{API_DOMAIN}/publication-channels-v2/serial-publication/{AN_IDENTIFIER}/2024", + "type": "Journal", + "name": "Nature", + "printIssn": "0028-0836", + "onlineIssn": "1476-4687", + "year": "2024", + } + + +def _a_publisher_hit() -> dict: + return { + "id": f"https://{API_DOMAIN}/publication-channels-v2/publisher/{AN_IDENTIFIER}/2024", + "type": "Publisher", + "name": "Nature Publishing Group", + "isbnPrefix": "978-0", + "year": "2024", + } + + +@mock_aws +@responses.activate +def test_search_merges_serial_and_publisher_hits(): + _seed_aws() + _add_cognito() + responses.add( + responses.GET, SERIAL_URL, json={"hits": [_a_serial_hit()], "totalHits": 1} + ) + responses.add( + responses.GET, + PUBLISHER_URL, + json={"hits": [_a_publisher_hit()], "totalHits": 1}, + ) + + runner = CliRunner() + result = runner.invoke(channels, ["search", "Nature"], obj=_ctx()) + + assert result.exit_code == 0, result.output + assert "Nature" in result.output + assert "Publishing" in result.output + assert "Journal" in result.output + assert "Publisher" in result.output + + +@mock_aws +@responses.activate +def test_search_with_kind_publisher_only_calls_publisher_endpoint(): + _seed_aws() + _add_cognito() + responses.add( + responses.GET, + PUBLISHER_URL, + json={"hits": [_a_publisher_hit()], "totalHits": 1}, + ) + + runner = CliRunner() + result = runner.invoke( + channels, ["search", "Nature", "--kind", "publisher"], obj=_ctx() + ) + + assert result.exit_code == 0, result.output + get_calls = [c for c in responses.calls if c.request.method == "GET"] + assert len(get_calls) == 1 + assert "publisher" in get_calls[0].request.url + + +@mock_aws +@responses.activate +def test_get_auto_detects_serial_first(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + + runner = CliRunner() + result = runner.invoke(channels, ["get", AN_IDENTIFIER], obj=_ctx()) + + assert result.exit_code == 0, result.output + assert "Resolved kind: serial-publication" in result.output + assert "Nature" in result.output + + +@mock_aws +@responses.activate +def test_get_falls_back_to_publisher_on_500_from_serial(): + _seed_aws() + _add_cognito() + responses.add( + responses.GET, + f"{SERIAL_URL}/{AN_IDENTIFIER}", + json={"message": "Internal server error"}, + status=500, + ) + responses.add( + responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", json=_a_publisher_hit() + ) + + runner = CliRunner() + result = runner.invoke(channels, ["get", AN_IDENTIFIER], obj=_ctx()) + + assert result.exit_code == 0, result.output + assert "Resolved kind: publisher" in result.output + + +@mock_aws +@responses.activate +def test_get_falls_back_to_publisher_on_404(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) + responses.add( + responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", json=_a_publisher_hit() + ) + + runner = CliRunner() + result = runner.invoke(channels, ["get", AN_IDENTIFIER], obj=_ctx()) + + assert result.exit_code == 0, result.output + assert "Resolved kind: publisher" in result.output + + +@mock_aws +@responses.activate +def test_get_raises_when_neither_kind_has_channel(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) + responses.add(responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", status=404) + + runner = CliRunner() + result = runner.invoke(channels, ["get", AN_IDENTIFIER], obj=_ctx()) + + assert result.exit_code != 0 + assert "no channel" in result.output.lower() + + +@mock_aws +@responses.activate +def test_create_with_isbn_prefix_creates_publisher(): + _seed_aws() + _add_cognito() + responses.add( + responses.POST, + PUBLISHER_URL, + json={"id": "x", "type": "Publisher", "name": "Acme"}, + status=201, + ) + + runner = CliRunner() + result = runner.invoke( + channels, + ["create", "--name", "Acme", "--isbn-prefix", "978-82-12"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + post_calls = [c for c in responses.calls if c.request.method == "POST"] + publisher_posts = [c for c in post_calls if "publisher" in c.request.url] + assert len(publisher_posts) == 1 + body = json.loads(publisher_posts[0].request.body) + assert body == {"name": "Acme", "isbnPrefix": "978-82-12"} + + +@mock_aws +@responses.activate +def test_create_with_print_issn_creates_serial_journal_by_default(): + _seed_aws() + _add_cognito() + responses.add( + responses.POST, + SERIAL_URL, + json={"id": "x", "type": "Journal", "name": "Foo"}, + status=201, + ) + + runner = CliRunner() + result = runner.invoke( + channels, + ["create", "--name", "Foo", "--print-issn", "1234-5678"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + serial_posts = [ + c + for c in responses.calls + if c.request.method == "POST" and "serial" in c.request.url + ] + assert len(serial_posts) == 1 + body = json.loads(serial_posts[0].request.body) + assert body["type"] == "Journal" + + +@mock_aws +@responses.activate +def test_create_with_kind_series_uses_series_endpoint(): + _seed_aws() + _add_cognito() + responses.add( + responses.POST, + SERIES_URL, + json={"id": "x", "type": "Series", "name": "Bar"}, + status=201, + ) + + runner = CliRunner() + result = runner.invoke( + channels, + ["create", "--name", "Bar", "--kind", "series", "--print-issn", "1234-5678"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + series_posts = [ + c + for c in responses.calls + if c.request.method == "POST" and c.request.url.endswith("/series") + ] + assert len(series_posts) == 1 + + +def test_create_without_inferrable_kind_fails(): + runner = CliRunner() + result = runner.invoke(channels, ["create", "--name", "Foo"], obj=_ctx()) + assert result.exit_code != 0 + assert "infer" in result.output.lower() or "kind" in result.output.lower() + + +@mock_aws +@responses.activate +def test_create_rejects_issn_for_publisher_kind(): + _seed_aws() + _add_cognito() + + runner = CliRunner() + result = runner.invoke( + channels, + [ + "create", + "--name", + "Acme", + "--kind", + "publisher", + "--print-issn", + "1234-5678", + ], + obj=_ctx(), + ) + assert result.exit_code != 0 + assert "issn" in result.output.lower() + + +@mock_aws +@responses.activate +def test_update_auto_detects_kind_and_calls_serial_put(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + responses.add(responses.PUT, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=202) + + runner = CliRunner() + result = runner.invoke( + channels, + ["update", AN_IDENTIFIER, "--name", "New name"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + put_calls = [c for c in responses.calls if c.request.method == "PUT"] + assert len(put_calls) == 1 + body = json.loads(put_calls[0].request.body) + assert body == {"type": "UpdateSerialPublicationRequest", "name": "New name"} + + +@mock_aws +@responses.activate +def test_update_publisher_uses_publisher_put_when_serial_returns_404(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) + responses.add( + responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", json=_a_publisher_hit() + ) + responses.add(responses.PUT, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", status=202) + + runner = CliRunner() + result = runner.invoke( + channels, + ["update", AN_IDENTIFIER, "--name", "Acme", "--isbn", "978-1"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + put_calls = [c for c in responses.calls if c.request.method == "PUT"] + assert len(put_calls) == 1 + body = json.loads(put_calls[0].request.body) + assert body == {"type": "UpdatePublisherRequest", "name": "Acme", "isbn": "978-1"} + + +def test_update_requires_at_least_one_field(): + runner = CliRunner() + result = runner.invoke(channels, ["update", AN_IDENTIFIER], obj=_ctx()) + assert result.exit_code != 0 + assert "at least one" in result.output.lower() + + +@mock_aws +@responses.activate +def test_fetch_auto_raises_when_not_found(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) + responses.add(responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", status=404) + + service = ChannelsApiService(None) + with pytest.raises(ChannelNotFoundError): + service.fetch_auto(AN_IDENTIFIER) + + +@mock_aws +@responses.activate +def test_search_kind_journal_calls_journal_endpoint_only(): + _seed_aws() + _add_cognito() + responses.add( + responses.GET, JOURNAL_URL, json={"hits": [_a_serial_hit()], "totalHits": 1} + ) + + runner = CliRunner() + result = runner.invoke( + channels, ["search", "Nature", "--kind", "journal"], obj=_ctx() + ) + + assert result.exit_code == 0, result.output + get_calls = [c for c in responses.calls if c.request.method == "GET"] + assert len(get_calls) == 1 + assert get_calls[0].request.url.startswith(JOURNAL_URL) + + +@mock_aws +@responses.activate +def test_delete_calls_channel_endpoint_after_confirmation(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + responses.add(responses.DELETE, f"{CHANNEL_URL}/{AN_IDENTIFIER}", status=204) + + runner = CliRunner() + result = runner.invoke(channels, ["delete", AN_IDENTIFIER, "--yes"], obj=_ctx()) + + assert result.exit_code == 0, result.output + delete_calls = [c for c in responses.calls if c.request.method == "DELETE"] + assert len(delete_calls) == 1 + assert delete_calls[0].request.url == f"{CHANNEL_URL}/{AN_IDENTIFIER}" + assert "DELETED" in result.output + + +@mock_aws +@responses.activate +def test_delete_prompts_for_confirmation_when_yes_not_set(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + + runner = CliRunner() + result = runner.invoke(channels, ["delete", AN_IDENTIFIER], input="n\n", obj=_ctx()) + + assert result.exit_code != 0 + delete_calls = [c for c in responses.calls if c.request.method == "DELETE"] + assert len(delete_calls) == 0 + + +@mock_aws +@responses.activate +def test_delete_raises_when_channel_not_found(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) + responses.add(responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", status=404) + + runner = CliRunner() + result = runner.invoke(channels, ["delete", AN_IDENTIFIER, "--yes"], obj=_ctx()) + + assert result.exit_code != 0 + assert "no channel" in result.output.lower() From 5bb56f4b4a32f438365643cdd39d3a2dab327fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Mon, 1 Jun 2026 09:37:55 +0200 Subject: [PATCH 2/6] refactor(channels): apply claude feedback #NP-51227 --- commands/channels.py | 101 +++++++++++++---------- commands/services/channels_api.py | 75 ++++++++++------- commands/services/tests/test_channels.py | 20 +++++ 3 files changed, 120 insertions(+), 76 deletions(-) diff --git a/commands/channels.py b/commands/channels.py index c22c140..93abb45 100644 --- a/commands/channels.py +++ b/commands/channels.py @@ -1,6 +1,4 @@ import functools -import logging -from typing import Optional import click import requests @@ -12,14 +10,15 @@ KIND_PUBLISHER, KIND_SERIAL, KIND_SERIES, + SERIAL_TYPE_JOURNAL, ChannelNotFoundError, ChannelsApiService, ) from commands.utils import AppContext -logger = logging.getLogger(__name__) - -KIND_CHOICES = ["serial", "journal", "series", "publisher"] +KIND_ALIAS_SERIAL = "serial" +KIND_CHOICES = [KIND_ALIAS_SERIAL, KIND_JOURNAL, KIND_SERIES, KIND_PUBLISHER] +SERIAL_LIKE_CHOICES = (None, KIND_ALIAS_SERIAL, KIND_JOURNAL, KIND_SERIES) def _handle_api_errors(func): @@ -61,15 +60,15 @@ def channels(ctx: AppContext): def search( ctx: AppContext, query: str, - kind: Optional[str], - year: Optional[int], + kind: str | None, + year: int | None, offset: int, size: int, ) -> None: """Search channels across journals/series/publishers.""" service = ChannelsApiService(ctx.profile) - rows = _collect_search_rows(service, query, kind, year, offset, size) - _render_table(rows, query) + rows, total_hits = _collect_search_rows(service, query, kind, year, offset, size) + _render_table(rows, query, total_hits) @channels.command() @@ -83,16 +82,15 @@ def search( ) @click.pass_obj @_handle_api_errors -def get( - ctx: AppContext, identifier: str, year: Optional[int], kind: Optional[str] -) -> None: +def get(ctx: AppContext, identifier: str, year: int | None, kind: str | None) -> None: """Fetch a single channel by identifier (auto-detects type).""" service = ChannelsApiService(ctx.profile) if kind is None: channel = service.fetch_auto(identifier, year) else: - channel = service.fetch(_resolve_kind(kind), identifier, year) - channel.setdefault("_resolvedKind", _resolve_kind(kind)) + resolved_kind = _resolve_kind(kind) + channel = service.fetch(resolved_kind, identifier, year) + channel.setdefault("_resolvedKind", resolved_kind) _print_channel(channel) @@ -100,7 +98,7 @@ def get( @click.option("--name", required=True, help="Channel name") @click.option( "--kind", - type=click.Choice(["publisher", "journal", "series", "serial"]), + type=click.Choice(KIND_CHOICES), default=None, help="Explicit kind. Default: inferred from other flags.", ) @@ -113,11 +111,11 @@ def get( def create( ctx: AppContext, name: str, - kind: Optional[str], - isbn_prefix: Optional[str], - print_issn: Optional[str], - online_issn: Optional[str], - homepage: Optional[str], + kind: str | None, + isbn_prefix: str | None, + print_issn: str | None, + online_issn: str | None, + homepage: str | None, ) -> None: """Create a new channel. Picks publisher vs serial-publication from flags.""" resolved_kind = _infer_create_kind(kind, isbn_prefix, print_issn, online_issn) @@ -138,7 +136,7 @@ def create( else: _reject_isbn(isbn_prefix) result = service.create_serial_publication( - name, "Journal", print_issn, online_issn, homepage + name, SERIAL_TYPE_JOURNAL, print_issn, online_issn, homepage ) click.echo(f"CREATED {resolved_kind}: {name}") @@ -156,10 +154,10 @@ def create( def update( ctx: AppContext, identifier: str, - name: Optional[str], - isbn: Optional[str], - print_issn: Optional[str], - online_issn: Optional[str], + name: str | None, + isbn: str | None, + print_issn: str | None, + online_issn: str | None, ) -> None: """Update an existing channel. Type is detected from the channel itself.""" if name is None and isbn is None and print_issn is None and online_issn is None: @@ -202,7 +200,7 @@ def delete(ctx: AppContext, identifier: str, yes: bool) -> None: def _resolve_kind(kind: str) -> str: - return KIND_SERIAL if kind == "serial" else kind + return KIND_SERIAL if kind == KIND_ALIAS_SERIAL else kind def _format_http_error(exc: requests.HTTPError) -> str: @@ -215,10 +213,10 @@ def _format_http_error(exc: requests.HTTPError) -> str: def _infer_create_kind( - kind: Optional[str], - isbn_prefix: Optional[str], - print_issn: Optional[str], - online_issn: Optional[str], + kind: str | None, + isbn_prefix: str | None, + print_issn: str | None, + online_issn: str | None, ) -> str: if kind: return _resolve_kind(kind) @@ -232,7 +230,7 @@ def _infer_create_kind( ) -def _reject_isbn(isbn_prefix: Optional[str]) -> None: +def _reject_isbn(isbn_prefix: str | None) -> None: if isbn_prefix: raise click.UsageError("--isbn-prefix is only valid for publisher channels") @@ -240,22 +238,30 @@ def _reject_isbn(isbn_prefix: Optional[str]) -> None: def _collect_search_rows( service: ChannelsApiService, query: str, - kind: Optional[str], - year: Optional[int], + kind: str | None, + year: int | None, offset: int, size: int, -) -> list: +) -> tuple[list, int]: rows: list = [] - if kind in (None, "serial", "journal", "series"): + total_hits = 0 + if kind in SERIAL_LIKE_CHOICES: serial_kind = _resolve_kind(kind) if kind else KIND_SERIAL - rows.extend( - _rows_from_hits(service.search(serial_kind, query, year, offset, size)) - ) - if kind in (None, "publisher"): - rows.extend( - _rows_from_hits(service.search(KIND_PUBLISHER, query, year, offset, size)) - ) - return rows + payload = service.search(serial_kind, query, year, offset, size) + rows.extend(_rows_from_hits(payload)) + total_hits += _total_hits(payload) + if kind in (None, KIND_PUBLISHER): + payload = service.search(KIND_PUBLISHER, query, year, offset, size) + rows.extend(_rows_from_hits(payload)) + total_hits += _total_hits(payload) + return rows, total_hits + + +def _total_hits(payload: dict) -> int: + value = payload.get("totalHits") + if isinstance(value, int): + return value + return len(payload.get("hits", [])) def _rows_from_hits(payload: dict) -> list: @@ -284,15 +290,19 @@ def _identifier_from_id(channel_id: str) -> str: return parts[-1] -def _render_table(rows: list, query: str) -> None: +def _render_table(rows: list, query: str, total_hits: int) -> None: console = Console() if not rows: console.print(f"[yellow]No hits for {query!r}[/yellow]") return + title = ( + f"[bold magenta]Channels matching {query!r} " + f"(showing {len(rows)} of {total_hits})[/bold magenta]" + ) table = Table( show_header=True, header_style="bold cyan", - title=f"[bold magenta]Channels matching {query!r} ({len(rows)} hits)[/bold magenta]", + title=title, ) table.add_column("Type") table.add_column("Identifier") @@ -317,6 +327,7 @@ def _print_channel(channel: dict) -> None: console.print(f"[bold]Resolved kind:[/bold] {resolved}") keys = [ "id", + "location", "type", "name", "printIssn", diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py index ab3d63b..33d6538 100644 --- a/commands/services/channels_api.py +++ b/commands/services/channels_api.py @@ -1,6 +1,5 @@ import json from datetime import datetime, timedelta -from typing import Optional import boto3 import requests @@ -15,27 +14,39 @@ VALID_KINDS = {KIND_PUBLISHER, KIND_SERIAL, KIND_JOURNAL, KIND_SERIES} +SERIAL_TYPE_JOURNAL = "Journal" +SERIAL_TYPE_SERIES = "Series" +VALID_SERIAL_TYPES = {SERIAL_TYPE_JOURNAL, SERIAL_TYPE_SERIES} + +UPDATE_PUBLISHER_REQUEST_TYPE = "UpdatePublisherRequest" +UPDATE_SERIAL_PUBLICATION_REQUEST_TYPE = "UpdateSerialPublicationRequest" + +CONTENT_TYPE_LD_JSON = "application/ld+json" +CONTENT_TYPE_JSON = "application/json" + +TOKEN_REFRESH_MARGIN_SECONDS = 30 + class ChannelNotFoundError(Exception): pass class ChannelsApiService: - def __init__(self, profile: Optional[str]): + def __init__(self, profile: str | None): self.session = boto3.Session(profile_name=profile) self.ssm = self.session.client("ssm") self.secretsmanager = self.session.client("secretsmanager") self.api_domain = self._get_system_parameter("/NVA/ApiDomain") self.cognito_uri = self._get_system_parameter("/NVA/CognitoUri") self.client_credentials = self._get_secret("BackendCognitoClientCredentials") - self.token: Optional[str] = None - self.token_expiry_time = datetime.now() + self.token: str | None = None + self.token_expiry_time = datetime.min def search( self, kind: str, query: str, - year: Optional[int] = None, + year: int | None = None, offset: int = 0, size: int = 10, ) -> dict: @@ -46,7 +57,7 @@ def search( response.raise_for_status() return response.json() - def fetch(self, kind: str, identifier: str, year: Optional[int] = None) -> dict: + def fetch(self, kind: str, identifier: str, year: int | None = None) -> dict: url = f"{self._channel_url(kind)}/{identifier}" if year is not None: url = f"{url}/{year}" @@ -56,8 +67,8 @@ def fetch(self, kind: str, identifier: str, year: Optional[int] = None) -> dict: response.raise_for_status() return response.json() - def fetch_auto(self, identifier: str, year: Optional[int] = None) -> dict: - last_http_error: Optional[requests.HTTPError] = None + def fetch_auto(self, identifier: str, year: int | None = None) -> dict: + last_http_error: requests.HTTPError | None = None for kind in (KIND_SERIAL, KIND_PUBLISHER): try: channel = self.fetch(kind, identifier, year) @@ -80,8 +91,8 @@ def fetch_auto(self, identifier: str, year: Optional[int] = None) -> dict: def create_publisher( self, name: str, - isbn_prefix: Optional[str] = None, - homepage: Optional[str] = None, + isbn_prefix: str | None = None, + homepage: str | None = None, ) -> dict: body = _drop_none( {"name": name, "isbnPrefix": isbn_prefix, "homepage": homepage} @@ -92,12 +103,12 @@ def create_serial_publication( self, name: str, serial_type: str, - print_issn: Optional[str] = None, - online_issn: Optional[str] = None, - homepage: Optional[str] = None, + print_issn: str | None = None, + online_issn: str | None = None, + homepage: str | None = None, ) -> dict: - if serial_type not in ("Series", "Journal"): - raise ValueError("serial_type must be 'Series' or 'Journal'") + if serial_type not in VALID_SERIAL_TYPES: + raise ValueError(f"serial_type must be one of {sorted(VALID_SERIAL_TYPES)}") body = _drop_none( { "name": name, @@ -112,9 +123,9 @@ def create_serial_publication( def create_journal( self, name: str, - print_issn: Optional[str] = None, - online_issn: Optional[str] = None, - homepage: Optional[str] = None, + print_issn: str | None = None, + online_issn: str | None = None, + homepage: str | None = None, ) -> dict: body = _drop_none( { @@ -129,9 +140,9 @@ def create_journal( def create_series( self, name: str, - print_issn: Optional[str] = None, - online_issn: Optional[str] = None, - homepage: Optional[str] = None, + print_issn: str | None = None, + online_issn: str | None = None, + homepage: str | None = None, ) -> dict: body = _drop_none( { @@ -146,24 +157,24 @@ def create_series( def update_publisher( self, identifier: str, - name: Optional[str] = None, - isbn: Optional[str] = None, + name: str | None = None, + isbn: str | None = None, ) -> dict: body = _drop_none( - {"type": "UpdatePublisherRequest", "name": name, "isbn": isbn} + {"type": UPDATE_PUBLISHER_REQUEST_TYPE, "name": name, "isbn": isbn} ) return self._put(KIND_PUBLISHER, identifier, body) def update_serial_publication( self, identifier: str, - name: Optional[str] = None, - print_issn: Optional[str] = None, - online_issn: Optional[str] = None, + name: str | None = None, + print_issn: str | None = None, + online_issn: str | None = None, ) -> dict: body = _drop_none( { - "type": "UpdateSerialPublicationRequest", + "type": UPDATE_SERIAL_PUBLICATION_REQUEST_TYPE, "name": name, "printIssn": print_issn, "onlineIssn": online_issn, @@ -190,7 +201,7 @@ def _auth_headers(self) -> dict: return {"Authorization": f"Bearer {self._get_token()}"} def _post(self, kind: str, body: dict) -> dict: - headers = self._auth_headers() | {"Content-Type": "application/ld+json"} + headers = self._auth_headers() | {"Content-Type": CONTENT_TYPE_LD_JSON} response = requests.post(self._channel_url(kind), headers=headers, json=body) response.raise_for_status() if response.text: @@ -199,7 +210,7 @@ def _post(self, kind: str, body: dict) -> dict: def _put(self, kind: str, identifier: str, body: dict) -> dict: url = f"{self._channel_url(kind)}/{identifier}" - headers = self._auth_headers() | {"Content-Type": "application/json"} + headers = self._auth_headers() | {"Content-Type": CONTENT_TYPE_JSON} response = requests.put(url, headers=headers, json=body) response.raise_for_status() if response.status_code == 202: @@ -233,7 +244,9 @@ def _get_cognito_token(self) -> str: return response_json["access_token"] def _is_token_expired(self) -> bool: - return datetime.now() > self.token_expiry_time - timedelta(seconds=30) + return datetime.now() > self.token_expiry_time - timedelta( + seconds=TOKEN_REFRESH_MARGIN_SECONDS + ) def _get_token(self) -> str: if self.token is None or self._is_token_expired(): diff --git a/commands/services/tests/test_channels.py b/commands/services/tests/test_channels.py index 54cbe3a..b1fe9b7 100644 --- a/commands/services/tests/test_channels.py +++ b/commands/services/tests/test_channels.py @@ -356,6 +356,26 @@ def test_update_requires_at_least_one_field(): assert "at least one" in result.output.lower() +@mock_aws +@responses.activate +def test_update_rejects_isbn_for_serial_channel(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + + runner = CliRunner() + result = runner.invoke( + channels, + ["update", AN_IDENTIFIER, "--isbn", "978-1"], + obj=_ctx(), + ) + + assert result.exit_code != 0 + assert "isbn" in result.output.lower() + put_calls = [c for c in responses.calls if c.request.method == "PUT"] + assert len(put_calls) == 0 + + @mock_aws @responses.activate def test_fetch_auto_raises_when_not_found(): From dd54974164d2ad9031fbaacfdeb225abf06c0cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Mon, 1 Jun 2026 09:59:31 +0200 Subject: [PATCH 3/6] refactor(channels): apply claude feedback #NP-51227 --- commands/channels.py | 63 ++++++++++----------- commands/services/channels_api.py | 20 ++----- commands/services/tests/test_channels.py | 72 +++++++++++++++++++++++- 3 files changed, 103 insertions(+), 52 deletions(-) diff --git a/commands/channels.py b/commands/channels.py index 93abb45..5a08c4d 100644 --- a/commands/channels.py +++ b/commands/channels.py @@ -11,6 +11,7 @@ KIND_SERIAL, KIND_SERIES, SERIAL_TYPE_JOURNAL, + VALID_KINDS, ChannelNotFoundError, ChannelsApiService, ) @@ -86,12 +87,11 @@ def get(ctx: AppContext, identifier: str, year: int | None, kind: str | None) -> """Fetch a single channel by identifier (auto-detects type).""" service = ChannelsApiService(ctx.profile) if kind is None: - channel = service.fetch_auto(identifier, year) + channel, resolved_kind = service.fetch_auto(identifier, year) else: resolved_kind = _resolve_kind(kind) channel = service.fetch(resolved_kind, identifier, year) - channel.setdefault("_resolvedKind", resolved_kind) - _print_channel(channel) + _print_channel(channel, resolved_kind) @channels.command() @@ -102,23 +102,21 @@ def get(ctx: AppContext, identifier: str, year: int | None, kind: str | None) -> default=None, help="Explicit kind. Default: inferred from other flags.", ) -@click.option("--isbn-prefix", default=None, help="Publisher only") +@click.option("--isbn", default=None, help="Publisher only (ISBN prefix)") @click.option("--print-issn", default=None, help="Journal/series only") @click.option("--online-issn", default=None, help="Journal/series only") -@click.option("--homepage", default=None, help="Channel homepage URL") @click.pass_obj @_handle_api_errors def create( ctx: AppContext, name: str, kind: str | None, - isbn_prefix: str | None, + isbn: str | None, print_issn: str | None, online_issn: str | None, - homepage: str | None, ) -> None: """Create a new channel. Picks publisher vs serial-publication from flags.""" - resolved_kind = _infer_create_kind(kind, isbn_prefix, print_issn, online_issn) + resolved_kind = _infer_create_kind(kind, isbn, print_issn, online_issn) service = ChannelsApiService(ctx.profile) if resolved_kind == KIND_PUBLISHER: @@ -126,17 +124,17 @@ def create( raise click.UsageError( "ISSN flags are not valid for publisher; remove or set --kind" ) - result = service.create_publisher(name, isbn_prefix, homepage) + result = service.create_publisher(name, isbn) elif resolved_kind == KIND_JOURNAL: - _reject_isbn(isbn_prefix) - result = service.create_journal(name, print_issn, online_issn, homepage) + _reject_isbn(isbn) + result = service.create_journal(name, print_issn, online_issn) elif resolved_kind == KIND_SERIES: - _reject_isbn(isbn_prefix) - result = service.create_series(name, print_issn, online_issn, homepage) + _reject_isbn(isbn) + result = service.create_series(name, print_issn, online_issn) else: - _reject_isbn(isbn_prefix) + _reject_isbn(isbn) result = service.create_serial_publication( - name, SERIAL_TYPE_JOURNAL, print_issn, online_issn, homepage + name, SERIAL_TYPE_JOURNAL, print_issn, online_issn ) click.echo(f"CREATED {resolved_kind}: {name}") @@ -164,8 +162,7 @@ def update( raise click.UsageError("Specify at least one field to update.") service = ChannelsApiService(ctx.profile) - existing = service.fetch_auto(identifier) - resolved_kind = existing.get("_resolvedKind") + _, resolved_kind = service.fetch_auto(identifier) if resolved_kind == KIND_PUBLISHER: if print_issn or online_issn: @@ -188,10 +185,9 @@ def update( def delete(ctx: AppContext, identifier: str, yes: bool) -> None: """Delete a channel by identifier.""" service = ChannelsApiService(ctx.profile) - existing = service.fetch_auto(identifier) + existing, resolved_kind = service.fetch_auto(identifier) name = existing.get("name", "?") - resolved_kind = existing.get("_resolvedKind", "?") if not yes: click.confirm(f"Delete {resolved_kind} '{name}' ({identifier})?", abort=True) @@ -214,25 +210,25 @@ def _format_http_error(exc: requests.HTTPError) -> str: def _infer_create_kind( kind: str | None, - isbn_prefix: str | None, + isbn: str | None, print_issn: str | None, online_issn: str | None, ) -> str: if kind: return _resolve_kind(kind) - if isbn_prefix: + if isbn: return KIND_PUBLISHER if print_issn or online_issn: return KIND_SERIAL raise click.UsageError( "Cannot infer channel kind. Pass --kind or one of " - "--isbn-prefix / --print-issn / --online-issn." + "--isbn / --print-issn / --online-issn." ) -def _reject_isbn(isbn_prefix: str | None) -> None: - if isbn_prefix: - raise click.UsageError("--isbn-prefix is only valid for publisher channels") +def _reject_isbn(isbn: str | None) -> None: + if isbn: + raise click.UsageError("--isbn is only valid for publisher channels") def _collect_search_rows( @@ -283,11 +279,11 @@ def _row_from_hit(hit: dict) -> dict: def _identifier_from_id(channel_id: str) -> str: if not channel_id: return "" - stripped = channel_id.rstrip("/") - parts = stripped.split("/") - if len(parts) >= 2 and parts[-1].isdigit() and len(parts[-1]) == 4: - return parts[-2] - return parts[-1] + parts = [segment for segment in channel_id.split("/") if segment] + for index, segment in enumerate(parts): + if segment in VALID_KINDS and index + 1 < len(parts): + return parts[index + 1] + return parts[-1] if parts else "" def _render_table(rows: list, query: str, total_hits: int) -> None: @@ -320,11 +316,10 @@ def _render_table(rows: list, query: str, total_hits: int) -> None: console.print(table) -def _print_channel(channel: dict) -> None: +def _print_channel(channel: dict, resolved_kind: str | None = None) -> None: console = Console() - resolved = channel.get("_resolvedKind") - if resolved: - console.print(f"[bold]Resolved kind:[/bold] {resolved}") + if resolved_kind: + console.print(f"[bold]Resolved kind:[/bold] {resolved_kind}") keys = [ "id", "location", diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py index 33d6538..de09595 100644 --- a/commands/services/channels_api.py +++ b/commands/services/channels_api.py @@ -40,7 +40,7 @@ def __init__(self, profile: str | None): self.cognito_uri = self._get_system_parameter("/NVA/CognitoUri") self.client_credentials = self._get_secret("BackendCognitoClientCredentials") self.token: str | None = None - self.token_expiry_time = datetime.min + self.token_expiry_time = datetime.fromtimestamp(0) def search( self, @@ -67,13 +67,12 @@ def fetch(self, kind: str, identifier: str, year: int | None = None) -> dict: response.raise_for_status() return response.json() - def fetch_auto(self, identifier: str, year: int | None = None) -> dict: + def fetch_auto(self, identifier: str, year: int | None = None) -> tuple[dict, str]: last_http_error: requests.HTTPError | None = None for kind in (KIND_SERIAL, KIND_PUBLISHER): try: channel = self.fetch(kind, identifier, year) - channel.setdefault("_resolvedKind", kind) - return channel + return channel, kind except ChannelNotFoundError: continue except requests.HTTPError as exc: @@ -92,11 +91,8 @@ def create_publisher( self, name: str, isbn_prefix: str | None = None, - homepage: str | None = None, ) -> dict: - body = _drop_none( - {"name": name, "isbnPrefix": isbn_prefix, "homepage": homepage} - ) + body = _drop_none({"name": name, "isbnPrefix": isbn_prefix}) return self._post(KIND_PUBLISHER, body) def create_serial_publication( @@ -105,7 +101,6 @@ def create_serial_publication( serial_type: str, print_issn: str | None = None, online_issn: str | None = None, - homepage: str | None = None, ) -> dict: if serial_type not in VALID_SERIAL_TYPES: raise ValueError(f"serial_type must be one of {sorted(VALID_SERIAL_TYPES)}") @@ -115,7 +110,6 @@ def create_serial_publication( "type": serial_type, "printIssn": print_issn, "onlineIssn": online_issn, - "homepage": homepage, } ) return self._post(KIND_SERIAL, body) @@ -125,14 +119,12 @@ def create_journal( name: str, print_issn: str | None = None, online_issn: str | None = None, - homepage: str | None = None, ) -> dict: body = _drop_none( { "name": name, "printIssn": print_issn, "onlineIssn": online_issn, - "homepage": homepage, } ) return self._post(KIND_JOURNAL, body) @@ -142,14 +134,12 @@ def create_series( name: str, print_issn: str | None = None, online_issn: str | None = None, - homepage: str | None = None, ) -> dict: body = _drop_none( { "name": name, "printIssn": print_issn, "onlineIssn": online_issn, - "homepage": homepage, } ) return self._post(KIND_SERIES, body) @@ -249,7 +239,7 @@ def _is_token_expired(self) -> bool: ) def _get_token(self) -> str: - if self.token is None or self._is_token_expired(): + if self._is_token_expired(): self.token = self._get_cognito_token() return self.token diff --git a/commands/services/tests/test_channels.py b/commands/services/tests/test_channels.py index b1fe9b7..a765326 100644 --- a/commands/services/tests/test_channels.py +++ b/commands/services/tests/test_channels.py @@ -6,7 +6,7 @@ from click.testing import CliRunner from moto import mock_aws -from commands.channels import channels +from commands.channels import _identifier_from_id, channels from commands.services.channels_api import ( ChannelNotFoundError, ChannelsApiService, @@ -188,7 +188,7 @@ def test_get_raises_when_neither_kind_has_channel(): @mock_aws @responses.activate -def test_create_with_isbn_prefix_creates_publisher(): +def test_create_with_isbn_creates_publisher(): _seed_aws() _add_cognito() responses.add( @@ -201,7 +201,7 @@ def test_create_with_isbn_prefix_creates_publisher(): runner = CliRunner() result = runner.invoke( channels, - ["create", "--name", "Acme", "--isbn-prefix", "978-82-12"], + ["create", "--name", "Acme", "--isbn", "978-82-12"], obj=_ctx(), ) @@ -455,3 +455,69 @@ def test_delete_raises_when_channel_not_found(): assert result.exit_code != 0 assert "no channel" in result.output.lower() + + +@mock_aws +@responses.activate +def test_search_passes_year_query_param(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, SERIAL_URL, json={"hits": [], "totalHits": 0}) + responses.add(responses.GET, PUBLISHER_URL, json={"hits": [], "totalHits": 0}) + + runner = CliRunner() + result = runner.invoke(channels, ["search", "Nature", "--year", "2024"], obj=_ctx()) + + assert result.exit_code == 0, result.output + get_calls = [c for c in responses.calls if c.request.method == "GET"] + assert len(get_calls) == 2 + for call in get_calls: + assert "year=2024" in call.request.url + + +@mock_aws +@responses.activate +def test_get_appends_year_path_segment(): + _seed_aws() + _add_cognito() + responses.add( + responses.GET, + f"{SERIAL_URL}/{AN_IDENTIFIER}/2024", + json=_a_serial_hit(), + ) + + runner = CliRunner() + result = runner.invoke( + channels, ["get", AN_IDENTIFIER, "--year", "2024"], obj=_ctx() + ) + + assert result.exit_code == 0, result.output + get_calls = [c for c in responses.calls if c.request.method == "GET"] + assert len(get_calls) == 1 + assert get_calls[0].request.url.endswith(f"/{AN_IDENTIFIER}/2024") + + +def test_identifier_from_id_handles_year_suffix(): + channel_id = ( + f"https://{API_DOMAIN}/publication-channels-v2/" + f"serial-publication/{AN_IDENTIFIER}/2024" + ) + assert _identifier_from_id(channel_id) == AN_IDENTIFIER + + +def test_identifier_from_id_handles_no_year_suffix(): + channel_id = ( + f"https://{API_DOMAIN}/publication-channels-v2/publisher/{AN_IDENTIFIER}" + ) + assert _identifier_from_id(channel_id) == AN_IDENTIFIER + + +def test_identifier_from_id_handles_journal_kind(): + channel_id = ( + f"https://{API_DOMAIN}/publication-channels-v2/journal/{AN_IDENTIFIER}/2024" + ) + assert _identifier_from_id(channel_id) == AN_IDENTIFIER + + +def test_identifier_from_id_empty_string(): + assert _identifier_from_id("") == "" From aaea9fb00827b50560fa00685a3b4f628cef6412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Mon, 1 Jun 2026 10:12:37 +0200 Subject: [PATCH 4/6] refactor(channels): apply claude feedback #NP-51227 --- commands/channels.py | 11 ++-- commands/services/channels_api.py | 75 +++--------------------- commands/services/tests/test_channels.py | 18 +++--- 3 files changed, 26 insertions(+), 78 deletions(-) diff --git a/commands/channels.py b/commands/channels.py index 5a08c4d..38de504 100644 --- a/commands/channels.py +++ b/commands/channels.py @@ -5,6 +5,7 @@ from rich.console import Console from rich.table import Table +from commands.services.api_client import ApiClient from commands.services.channels_api import ( KIND_JOURNAL, KIND_PUBLISHER, @@ -67,7 +68,7 @@ def search( size: int, ) -> None: """Search channels across journals/series/publishers.""" - service = ChannelsApiService(ctx.profile) + service = ChannelsApiService(ApiClient(session=ctx.session)) rows, total_hits = _collect_search_rows(service, query, kind, year, offset, size) _render_table(rows, query, total_hits) @@ -85,7 +86,7 @@ def search( @_handle_api_errors def get(ctx: AppContext, identifier: str, year: int | None, kind: str | None) -> None: """Fetch a single channel by identifier (auto-detects type).""" - service = ChannelsApiService(ctx.profile) + service = ChannelsApiService(ApiClient(session=ctx.session)) if kind is None: channel, resolved_kind = service.fetch_auto(identifier, year) else: @@ -117,7 +118,7 @@ def create( ) -> None: """Create a new channel. Picks publisher vs serial-publication from flags.""" resolved_kind = _infer_create_kind(kind, isbn, print_issn, online_issn) - service = ChannelsApiService(ctx.profile) + service = ChannelsApiService(ApiClient(session=ctx.session)) if resolved_kind == KIND_PUBLISHER: if print_issn or online_issn: @@ -161,7 +162,7 @@ def update( if name is None and isbn is None and print_issn is None and online_issn is None: raise click.UsageError("Specify at least one field to update.") - service = ChannelsApiService(ctx.profile) + service = ChannelsApiService(ApiClient(session=ctx.session)) _, resolved_kind = service.fetch_auto(identifier) if resolved_kind == KIND_PUBLISHER: @@ -184,7 +185,7 @@ def update( @_handle_api_errors def delete(ctx: AppContext, identifier: str, yes: bool) -> None: """Delete a channel by identifier.""" - service = ChannelsApiService(ctx.profile) + service = ChannelsApiService(ApiClient(session=ctx.session)) existing, resolved_kind = service.fetch_auto(identifier) name = existing.get("name", "?") diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py index de09595..ef33c50 100644 --- a/commands/services/channels_api.py +++ b/commands/services/channels_api.py @@ -1,9 +1,7 @@ -import json -from datetime import datetime, timedelta - -import boto3 import requests +from commands.services.api_client import ApiClient + CHANNEL_BASE_PATH = "publication-channels-v2" DELETE_PATH_SEGMENT = "channel" @@ -24,23 +22,14 @@ CONTENT_TYPE_LD_JSON = "application/ld+json" CONTENT_TYPE_JSON = "application/json" -TOKEN_REFRESH_MARGIN_SECONDS = 30 - class ChannelNotFoundError(Exception): pass class ChannelsApiService: - def __init__(self, profile: str | None): - self.session = boto3.Session(profile_name=profile) - self.ssm = self.session.client("ssm") - self.secretsmanager = self.session.client("secretsmanager") - self.api_domain = self._get_system_parameter("/NVA/ApiDomain") - self.cognito_uri = self._get_system_parameter("/NVA/CognitoUri") - self.client_credentials = self._get_secret("BackendCognitoClientCredentials") - self.token: str | None = None - self.token_expiry_time = datetime.fromtimestamp(0) + def __init__(self, client: ApiClient): + self.client = client def search( self, @@ -68,21 +57,12 @@ def fetch(self, kind: str, identifier: str, year: int | None = None) -> dict: return response.json() def fetch_auto(self, identifier: str, year: int | None = None) -> tuple[dict, str]: - last_http_error: requests.HTTPError | None = None for kind in (KIND_SERIAL, KIND_PUBLISHER): try: channel = self.fetch(kind, identifier, year) return channel, kind except ChannelNotFoundError: continue - except requests.HTTPError as exc: - status = exc.response.status_code if exc.response is not None else 0 - if 500 <= status < 600: - last_http_error = exc - continue - raise - if last_http_error is not None: - raise last_http_error raise ChannelNotFoundError( f"No channel with identifier {identifier} found in serial-publication or publisher" ) @@ -174,10 +154,10 @@ def update_serial_publication( def delete_channel(self, identifier: str) -> None: url = ( - f"https://{self.api_domain}/{CHANNEL_BASE_PATH}/" + f"https://{self.client.api_domain}/{CHANNEL_BASE_PATH}/" f"{DELETE_PATH_SEGMENT}/{identifier}" ) - response = requests.delete(url, headers=self._auth_headers()) + response = requests.delete(url, headers=self.client.auth_header()) if response.status_code == 404: raise ChannelNotFoundError(f"channel {identifier} not found") response.raise_for_status() @@ -185,13 +165,10 @@ def delete_channel(self, identifier: str) -> None: def _channel_url(self, kind: str) -> str: if kind not in VALID_KINDS: raise ValueError(f"Unknown channel kind: {kind}") - return f"https://{self.api_domain}/{CHANNEL_BASE_PATH}/{kind}" - - def _auth_headers(self) -> dict: - return {"Authorization": f"Bearer {self._get_token()}"} + return f"https://{self.client.api_domain}/{CHANNEL_BASE_PATH}/{kind}" def _post(self, kind: str, body: dict) -> dict: - headers = self._auth_headers() | {"Content-Type": CONTENT_TYPE_LD_JSON} + headers = self.client.auth_header() | {"Content-Type": CONTENT_TYPE_LD_JSON} response = requests.post(self._channel_url(kind), headers=headers, json=body) response.raise_for_status() if response.text: @@ -200,7 +177,7 @@ def _post(self, kind: str, body: dict) -> dict: def _put(self, kind: str, identifier: str, body: dict) -> dict: url = f"{self._channel_url(kind)}/{identifier}" - headers = self._auth_headers() | {"Content-Type": CONTENT_TYPE_JSON} + headers = self.client.auth_header() | {"Content-Type": CONTENT_TYPE_JSON} response = requests.put(url, headers=headers, json=body) response.raise_for_status() if response.status_code == 202: @@ -209,40 +186,6 @@ def _put(self, kind: str, identifier: str, body: dict) -> dict: return response.json() return {} - def _get_system_parameter(self, name: str) -> str: - response = self.ssm.get_parameter(Name=name) - return response["Parameter"]["Value"] - - def _get_secret(self, name: str) -> dict: - response = self.secretsmanager.get_secret_value(SecretId=name) - return json.loads(response["SecretString"]) - - def _get_cognito_token(self) -> str: - url = f"{self.cognito_uri}/oauth2/token" - headers = {"Content-Type": "application/x-www-form-urlencoded"} - data = { - "grant_type": "client_credentials", - "client_id": self.client_credentials["backendClientId"], - "client_secret": self.client_credentials["backendClientSecret"], - } - response = requests.post(url, headers=headers, data=data) - response.raise_for_status() - response_json = response.json() - self.token_expiry_time = datetime.now() + timedelta( - seconds=response_json["expires_in"] - ) - return response_json["access_token"] - - def _is_token_expired(self) -> bool: - return datetime.now() > self.token_expiry_time - timedelta( - seconds=TOKEN_REFRESH_MARGIN_SECONDS - ) - - def _get_token(self) -> str: - if self._is_token_expired(): - self.token = self._get_cognito_token() - return self.token - def _drop_none(body: dict) -> dict: return {key: value for key, value in body.items() if value is not None} diff --git a/commands/services/tests/test_channels.py b/commands/services/tests/test_channels.py index a765326..4a15a11 100644 --- a/commands/services/tests/test_channels.py +++ b/commands/services/tests/test_channels.py @@ -7,6 +7,7 @@ from moto import mock_aws from commands.channels import _identifier_from_id, channels +from commands.services.api_client import ApiClient from commands.services.channels_api import ( ChannelNotFoundError, ChannelsApiService, @@ -134,7 +135,7 @@ def test_get_auto_detects_serial_first(): @mock_aws @responses.activate -def test_get_falls_back_to_publisher_on_500_from_serial(): +def test_get_propagates_500_from_serial_without_falling_back(): _seed_aws() _add_cognito() responses.add( @@ -143,15 +144,16 @@ def test_get_falls_back_to_publisher_on_500_from_serial(): json={"message": "Internal server error"}, status=500, ) - responses.add( - responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", json=_a_publisher_hit() - ) runner = CliRunner() result = runner.invoke(channels, ["get", AN_IDENTIFIER], obj=_ctx()) - assert result.exit_code == 0, result.output - assert "Resolved kind: publisher" in result.output + assert result.exit_code != 0 + assert "500" in result.output + publisher_calls = [ + c for c in responses.calls if c.request.url.startswith(f"{PUBLISHER_URL}/") + ] + assert publisher_calls == [] @mock_aws @@ -384,7 +386,9 @@ def test_fetch_auto_raises_when_not_found(): responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=404) responses.add(responses.GET, f"{PUBLISHER_URL}/{AN_IDENTIFIER}", status=404) - service = ChannelsApiService(None) + service = ChannelsApiService( + ApiClient(session=boto3.Session(region_name="eu-west-1")) + ) with pytest.raises(ChannelNotFoundError): service.fetch_auto(AN_IDENTIFIER) From d52dda394d36b38196adcf03c736912c60a1ea81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Mon, 1 Jun 2026 10:20:08 +0200 Subject: [PATCH 5/6] documentation --- commands/channels.py | 11 +++++++++-- commands/services/channels_api.py | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/commands/channels.py b/commands/channels.py index 38de504..0b93995 100644 --- a/commands/channels.py +++ b/commands/channels.py @@ -55,8 +55,15 @@ def channels(ctx: AppContext): help="Restrict to a single channel kind. Default: search all kinds.", ) @click.option("--year", type=int, default=None, help="Limit results to a given year") -@click.option("--offset", type=int, default=0) -@click.option("--size", type=int, default=10) +@click.option( + "--offset", type=int, default=0, help="Applied per kind when --kind is omitted" +) +@click.option( + "--size", + type=int, + default=10, + help="Max hits per kind (when --kind is omitted, both serial and publisher are queried, so up to 2x this many rows are returned)", +) @click.pass_obj @_handle_api_errors def search( diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py index ef33c50..8c69c7c 100644 --- a/commands/services/channels_api.py +++ b/commands/services/channels_api.py @@ -130,6 +130,7 @@ def update_publisher( name: str | None = None, isbn: str | None = None, ) -> dict: + # Backend uses 'isbn' on update but 'isbnPrefix' on create — see UpdatePublisherRequest.java body = _drop_none( {"type": UPDATE_PUBLISHER_REQUEST_TYPE, "name": name, "isbn": isbn} ) From e1726c6e19565266635307046b53dc4101aebbaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Kv=C3=A5le?= Date: Tue, 2 Jun 2026 10:22:51 +0200 Subject: [PATCH 6/6] review --- commands/channels.py | 42 ++-- commands/services/channels_api.py | 236 +++++++++++++---------- commands/services/tests/test_channels.py | 63 ++++++ 3 files changed, 220 insertions(+), 121 deletions(-) diff --git a/commands/channels.py b/commands/channels.py index 0b93995..46b757f 100644 --- a/commands/channels.py +++ b/commands/channels.py @@ -1,7 +1,6 @@ import functools import click -import requests from rich.console import Console from rich.table import Table @@ -13,8 +12,12 @@ KIND_SERIES, SERIAL_TYPE_JOURNAL, VALID_KINDS, - ChannelNotFoundError, + ChannelApiError, ChannelsApiService, + PublisherCreate, + PublisherUpdate, + SerialCreate, + SerialUpdate, ) from commands.utils import AppContext @@ -28,10 +31,8 @@ def _handle_api_errors(func): def wrapper(*args, **kwargs): try: return func(*args, **kwargs) - except ChannelNotFoundError as exc: + except ChannelApiError as exc: raise click.ClickException(str(exc)) - except requests.HTTPError as exc: - raise click.ClickException(_format_http_error(exc)) return wrapper @@ -132,17 +133,26 @@ def create( raise click.UsageError( "ISSN flags are not valid for publisher; remove or set --kind" ) - result = service.create_publisher(name, isbn) + result = service.create_publisher(PublisherCreate(name=name, isbn_prefix=isbn)) elif resolved_kind == KIND_JOURNAL: _reject_isbn(isbn) - result = service.create_journal(name, print_issn, online_issn) + result = service.create_journal( + SerialCreate(name=name, print_issn=print_issn, online_issn=online_issn) + ) elif resolved_kind == KIND_SERIES: _reject_isbn(isbn) - result = service.create_series(name, print_issn, online_issn) + result = service.create_series( + SerialCreate(name=name, print_issn=print_issn, online_issn=online_issn) + ) else: _reject_isbn(isbn) result = service.create_serial_publication( - name, SERIAL_TYPE_JOURNAL, print_issn, online_issn + SerialCreate( + name=name, + serial_type=SERIAL_TYPE_JOURNAL, + print_issn=print_issn, + online_issn=online_issn, + ) ) click.echo(f"CREATED {resolved_kind}: {name}") @@ -175,12 +185,13 @@ def update( if resolved_kind == KIND_PUBLISHER: if print_issn or online_issn: raise click.UsageError("ISSN flags are not valid for a publisher channel") - service.update_publisher(identifier, name=name, isbn=isbn) + service.update_publisher(identifier, PublisherUpdate(name=name, isbn=isbn)) else: if isbn: raise click.UsageError("--isbn is only valid for publisher channels") service.update_serial_publication( - identifier, name=name, print_issn=print_issn, online_issn=online_issn + identifier, + SerialUpdate(name=name, print_issn=print_issn, online_issn=online_issn), ) click.echo(f"UPDATED {resolved_kind} {identifier}") @@ -207,15 +218,6 @@ def _resolve_kind(kind: str) -> str: return KIND_SERIAL if kind == KIND_ALIAS_SERIAL else kind -def _format_http_error(exc: requests.HTTPError) -> str: - response = exc.response - if response is None: - return f"API error: {exc}" - snippet = response.text[:500].strip() if response.text else "" - base = f"API error {response.status_code} from {response.url}" - return f"{base}\n {snippet}" if snippet else base - - def _infer_create_kind( kind: str | None, isbn: str | None, diff --git a/commands/services/channels_api.py b/commands/services/channels_api.py index 8c69c7c..93d26a3 100644 --- a/commands/services/channels_api.py +++ b/commands/services/channels_api.py @@ -1,9 +1,13 @@ +from dataclasses import dataclass + import requests from commands.services.api_client import ApiClient CHANNEL_BASE_PATH = "publication-channels-v2" DELETE_PATH_SEGMENT = "channel" +REQUEST_TIMEOUT_SECONDS = 30 +HTTP_ERROR_BODY_SNIPPET_LENGTH = 500 KIND_PUBLISHER = "publisher" KIND_SERIAL = "serial-publication" @@ -22,11 +26,81 @@ CONTENT_TYPE_LD_JSON = "application/ld+json" CONTENT_TYPE_JSON = "application/json" +HTTP_NOT_FOUND = 404 + + +class ChannelApiError(Exception): + pass + -class ChannelNotFoundError(Exception): +class ChannelNotFoundError(ChannelApiError): pass +@dataclass(kw_only=True) +class PublisherCreate: + name: str + isbn_prefix: str | None = None + + def to_body(self) -> dict: + return _drop_none({"name": self.name, "isbnPrefix": self.isbn_prefix}) + + +@dataclass(kw_only=True) +class SerialCreate: + name: str + serial_type: str | None = None + print_issn: str | None = None + online_issn: str | None = None + + def __post_init__(self) -> None: + if self.serial_type is not None and self.serial_type not in VALID_SERIAL_TYPES: + raise ValueError(f"serial_type must be one of {sorted(VALID_SERIAL_TYPES)}") + + def to_body(self) -> dict: + return _drop_none( + { + "name": self.name, + "type": self.serial_type, + "printIssn": self.print_issn, + "onlineIssn": self.online_issn, + } + ) + + +@dataclass(kw_only=True) +class PublisherUpdate: + name: str | None = None + isbn: str | None = None + + def to_body(self) -> dict: + # Backend uses 'isbn' on update but 'isbnPrefix' on create — see UpdatePublisherRequest.java + return _drop_none( + { + "type": UPDATE_PUBLISHER_REQUEST_TYPE, + "name": self.name, + "isbn": self.isbn, + } + ) + + +@dataclass(kw_only=True) +class SerialUpdate: + name: str | None = None + print_issn: str | None = None + online_issn: str | None = None + + def to_body(self) -> dict: + return _drop_none( + { + "type": UPDATE_SERIAL_PUBLICATION_REQUEST_TYPE, + "name": self.name, + "printIssn": self.print_issn, + "onlineIssn": self.online_issn, + } + ) + + class ChannelsApiService: def __init__(self, client: ApiClient): self.client = client @@ -42,18 +116,16 @@ def search( params = {"query": query, "offset": offset, "size": size} if year is not None: params["year"] = year - response = requests.get(self._channel_url(kind), params=params) - response.raise_for_status() + response = self._request("GET", self._channel_url(kind), params=params) return response.json() def fetch(self, kind: str, identifier: str, year: int | None = None) -> dict: url = f"{self._channel_url(kind)}/{identifier}" if year is not None: url = f"{url}/{year}" - response = requests.get(url) - if response.status_code == 404: - raise ChannelNotFoundError(f"{kind}/{identifier} not found") - response.raise_for_status() + response = self._request( + "GET", url, not_found_message=f"{kind}/{identifier} not found" + ) return response.json() def fetch_auto(self, identifier: str, year: int | None = None) -> tuple[dict, str]: @@ -67,101 +139,37 @@ def fetch_auto(self, identifier: str, year: int | None = None) -> tuple[dict, st f"No channel with identifier {identifier} found in serial-publication or publisher" ) - def create_publisher( - self, - name: str, - isbn_prefix: str | None = None, - ) -> dict: - body = _drop_none({"name": name, "isbnPrefix": isbn_prefix}) - return self._post(KIND_PUBLISHER, body) + def create_publisher(self, request: PublisherCreate) -> dict: + return self._post(KIND_PUBLISHER, request.to_body()) - def create_serial_publication( - self, - name: str, - serial_type: str, - print_issn: str | None = None, - online_issn: str | None = None, - ) -> dict: - if serial_type not in VALID_SERIAL_TYPES: - raise ValueError(f"serial_type must be one of {sorted(VALID_SERIAL_TYPES)}") - body = _drop_none( - { - "name": name, - "type": serial_type, - "printIssn": print_issn, - "onlineIssn": online_issn, - } - ) - return self._post(KIND_SERIAL, body) + def create_serial_publication(self, request: SerialCreate) -> dict: + if request.serial_type is None: + raise ValueError("serial_type is required for /serial-publication") + return self._post(KIND_SERIAL, request.to_body()) - def create_journal( - self, - name: str, - print_issn: str | None = None, - online_issn: str | None = None, - ) -> dict: - body = _drop_none( - { - "name": name, - "printIssn": print_issn, - "onlineIssn": online_issn, - } - ) - return self._post(KIND_JOURNAL, body) + def create_journal(self, request: SerialCreate) -> dict: + return self._post(KIND_JOURNAL, request.to_body()) - def create_series( - self, - name: str, - print_issn: str | None = None, - online_issn: str | None = None, - ) -> dict: - body = _drop_none( - { - "name": name, - "printIssn": print_issn, - "onlineIssn": online_issn, - } - ) - return self._post(KIND_SERIES, body) + def create_series(self, request: SerialCreate) -> dict: + return self._post(KIND_SERIES, request.to_body()) - def update_publisher( - self, - identifier: str, - name: str | None = None, - isbn: str | None = None, - ) -> dict: - # Backend uses 'isbn' on update but 'isbnPrefix' on create — see UpdatePublisherRequest.java - body = _drop_none( - {"type": UPDATE_PUBLISHER_REQUEST_TYPE, "name": name, "isbn": isbn} - ) - return self._put(KIND_PUBLISHER, identifier, body) + def update_publisher(self, identifier: str, request: PublisherUpdate) -> None: + self._put(KIND_PUBLISHER, identifier, request.to_body()) - def update_serial_publication( - self, - identifier: str, - name: str | None = None, - print_issn: str | None = None, - online_issn: str | None = None, - ) -> dict: - body = _drop_none( - { - "type": UPDATE_SERIAL_PUBLICATION_REQUEST_TYPE, - "name": name, - "printIssn": print_issn, - "onlineIssn": online_issn, - } - ) - return self._put(KIND_SERIAL, identifier, body) + def update_serial_publication(self, identifier: str, request: SerialUpdate) -> None: + self._put(KIND_SERIAL, identifier, request.to_body()) def delete_channel(self, identifier: str) -> None: url = ( f"https://{self.client.api_domain}/{CHANNEL_BASE_PATH}/" f"{DELETE_PATH_SEGMENT}/{identifier}" ) - response = requests.delete(url, headers=self.client.auth_header()) - if response.status_code == 404: - raise ChannelNotFoundError(f"channel {identifier} not found") - response.raise_for_status() + self._request( + "DELETE", + url, + headers=self.client.auth_header(), + not_found_message=f"channel {identifier} not found", + ) def _channel_url(self, kind: str) -> str: if kind not in VALID_KINDS: @@ -170,22 +178,48 @@ def _channel_url(self, kind: str) -> str: def _post(self, kind: str, body: dict) -> dict: headers = self.client.auth_header() | {"Content-Type": CONTENT_TYPE_LD_JSON} - response = requests.post(self._channel_url(kind), headers=headers, json=body) - response.raise_for_status() + response = self._request( + "POST", self._channel_url(kind), headers=headers, json=body + ) if response.text: return response.json() return {"location": response.headers.get("Location")} - def _put(self, kind: str, identifier: str, body: dict) -> dict: + def _put(self, kind: str, identifier: str, body: dict) -> None: url = f"{self._channel_url(kind)}/{identifier}" headers = self.client.auth_header() | {"Content-Type": CONTENT_TYPE_JSON} - response = requests.put(url, headers=headers, json=body) - response.raise_for_status() - if response.status_code == 202: - return {"status": "accepted"} - if response.text: - return response.json() - return {} + self._request("PUT", url, headers=headers, json=body) + + def _request( + self, + method: str, + url: str, + *, + not_found_message: str | None = None, + **kwargs, + ) -> requests.Response: + try: + response = requests.request( + method, url, timeout=REQUEST_TIMEOUT_SECONDS, **kwargs + ) + except requests.RequestException as exc: + raise ChannelApiError( + f"Network error contacting channel API: {exc}" + ) from exc + + if response.status_code == HTTP_NOT_FOUND and not_found_message is not None: + raise ChannelNotFoundError(not_found_message) + if not response.ok: + raise ChannelApiError(_format_http_error_message(response)) + return response + + +def _format_http_error_message(response: requests.Response) -> str: + snippet = ( + response.text[:HTTP_ERROR_BODY_SNIPPET_LENGTH].strip() if response.text else "" + ) + base = f"API error {response.status_code} from {response.url}" + return f"{base}\n {snippet}" if snippet else base def _drop_none(body: dict) -> dict: diff --git a/commands/services/tests/test_channels.py b/commands/services/tests/test_channels.py index 4a15a11..ea5fa05 100644 --- a/commands/services/tests/test_channels.py +++ b/commands/services/tests/test_channels.py @@ -273,6 +273,36 @@ def test_create_with_kind_series_uses_series_endpoint(): assert len(series_posts) == 1 +@mock_aws +@responses.activate +def test_create_with_kind_journal_uses_journal_endpoint(): + _seed_aws() + _add_cognito() + responses.add( + responses.POST, + JOURNAL_URL, + json={"id": "x", "type": "Journal", "name": "Baz"}, + status=201, + ) + + runner = CliRunner() + result = runner.invoke( + channels, + ["create", "--name", "Baz", "--kind", "journal", "--print-issn", "1234-5678"], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + journal_posts = [ + c + for c in responses.calls + if c.request.method == "POST" and c.request.url.endswith("/journal") + ] + assert len(journal_posts) == 1 + body = json.loads(journal_posts[0].request.body) + assert body == {"name": "Baz", "printIssn": "1234-5678"} + + def test_create_without_inferrable_kind_fails(): runner = CliRunner() result = runner.invoke(channels, ["create", "--name", "Foo"], obj=_ctx()) @@ -326,6 +356,39 @@ def test_update_auto_detects_kind_and_calls_serial_put(): assert body == {"type": "UpdateSerialPublicationRequest", "name": "New name"} +@mock_aws +@responses.activate +def test_update_serial_sends_issn_fields_in_body(): + _seed_aws() + _add_cognito() + responses.add(responses.GET, f"{SERIAL_URL}/{AN_IDENTIFIER}", json=_a_serial_hit()) + responses.add(responses.PUT, f"{SERIAL_URL}/{AN_IDENTIFIER}", status=202) + + runner = CliRunner() + result = runner.invoke( + channels, + [ + "update", + AN_IDENTIFIER, + "--print-issn", + "1234-5678", + "--online-issn", + "8765-4321", + ], + obj=_ctx(), + ) + + assert result.exit_code == 0, result.output + put_calls = [c for c in responses.calls if c.request.method == "PUT"] + assert len(put_calls) == 1 + body = json.loads(put_calls[0].request.body) + assert body == { + "type": "UpdateSerialPublicationRequest", + "printIssn": "1234-5678", + "onlineIssn": "8765-4321", + } + + @mock_aws @responses.activate def test_update_publisher_uses_publisher_put_when_serial_returns_404():