feat(channels): add CLI for managing publication channels#77
Conversation
| self, | ||
| name: str, | ||
| print_issn: str | None = None, | ||
| online_issn: str | None = None, |
There was a problem hiding this comment.
Not necessary to rewrite this now, but: Would it be easier to have one or two dataclasses as the parameter in these methods? Then each method only needs one parameter and we can do the validation in the dataclass constructor.
| params = {"query": query, "offset": offset, "size": size} | ||
| if year is not None: | ||
| params["year"] = year | ||
| response = requests.get(self._channel_url(kind), params=params) |
There was a problem hiding this comment.
The various request calls in this file should probably have a timeout set, like in search_api.py:56.
| return func(*args, **kwargs) | ||
| except ChannelNotFoundError as exc: | ||
| raise click.ClickException(str(exc)) | ||
| except requests.HTTPError as exc: |
There was a problem hiding this comment.
Not sure we should be trying to handle network errors in the CLI layer (and dragging in requests to do so). I'm also not sure HTTPError covers everything that can go wrong (ConnectionError/Timeout/JSONDecodeError?). Might want to catch requests.RequestException instead.
Could move this to the service layer and map to a new ChannelApiError, same as the ChannelNotFoundError.
| if response.text: | ||
| return response.json() |
There was a problem hiding this comment.
This isn't covered by tests. If we need it, maybe add a test case?
| 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) |
There was a problem hiding this comment.
This isn't covered by tests. If we need it, maybe add a test case?
| body = json.loads(put_calls[0].request.body) | ||
| assert body == {"type": "UpdateSerialPublicationRequest", "name": "New name"} | ||
|
|
||
|
|
There was a problem hiding this comment.
Might want to add another test case here to verify that ISSN gets set properly on update. Something like this for example:
@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",
}
2 participants
Summary
Adds a new
channelscommand group for managing entries in the NVA Publication Channel Registry. Designed for 3rd-line support.search/get/create/update/deletesubcommandsget,update,deleteauto-detect publisher vs serial-publicationsearchmerges journal/series/publisher hits into one Rich tabledeleteusesDELETE /publication-channels-v2/channel/{id}with confirmation prompt (--yesto skip)search,get) call API without bearer token since they are public; write endpoints use the existing backend client-credentials secretUsage examples
https://sikt.atlassian.net/browse/NP-51227