diff --git a/python/exporter/cli.py b/python/exporter/cli.py index 41e41319..683c9980 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -49,7 +49,7 @@ suggested_name, ) from exporter.icloud import Candidate, ExportSourceError -from exporter.version import EXPORT_VIA_CLI, VERSION +from exporter.version import EXPORT_VIA_CLI, GITHUB_ISSUES_LINK, VERSION from opentagviewer_export import ( ExportError, KeyFileError, @@ -939,6 +939,12 @@ def _run_and_return(arguments: argparse.Namespace) -> int: print("it prints about you, and it is worth reading before pasting it anywhere.", file=sys.stderr) + # **And where.** Asking somebody to report something without saying where to put it leaves + # them holding output and no destination - which is not a hint they should have to take. + # The window has said this for as long as it has had an error dialog; this one asked for a + # report and named nowhere. + print(f"\nReport it at: {GITHUB_ISSUES_LINK}", file=sys.stderr) + return 1 except KeyboardInterrupt: print("\nStopped. Nothing was written.", file=sys.stderr) diff --git a/python/exporter/version.py b/python/exporter/version.py index da5855ed..7a5364da 100644 --- a/python/exporter/version.py +++ b/python/exporter/version.py @@ -42,3 +42,12 @@ A different producer, because it is one: same version, same format, different program - and a bug report that says which is worth more than one that says "the exporter". """ + +GITHUB_ISSUES_LINK = "https://github.com/parawanderer/OpenTagViewer/issues/new" +""" +Where to report something neither the user nor this program can fix. + +Here rather than in `wizard.py`, which is where it used to live alone, so the CLI can say it too +without importing tkinter - the same reason `VERSION` is here. Two copies of a URL is exactly the +sort of thing that goes stale in one place and is never noticed, because nothing tests a link. +""" diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index e4c45a0d..9d4d7ec5 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -57,7 +57,7 @@ from findmy.keychain.recovery import RecoveryError from exporter.icloud import Candidate, ExportSourceError -from exporter.version import APP_TITLE, EXPORT_VIA_WIZARD, VERSION +from exporter.version import APP_TITLE, EXPORT_VIA_WIZARD, GITHUB_ISSUES_LINK, VERSION from opentagviewer_export import ( ExportError, KeyFileError, @@ -69,7 +69,6 @@ logger = logging.getLogger(__name__) -GITHUB_ISSUES_LINK = "https://github.com/parawanderer/OpenTagViewer/issues/new" # The "Need Help?" link. It points at the page covering both routes: the older `…-From-Mac` page # keeps its name because binaries already released open it, but a copy shipping now should send # people to the one that describes what it actually does. diff --git a/python/test/test_cli.py b/python/test/test_cli.py index 66195b8f..219a45cd 100644 --- a/python/test/test_cli.py +++ b/python/test/test_cli.py @@ -16,11 +16,12 @@ from pathlib import Path import pytest -from findmy import InvalidCredentialsError, MobileMeDelegateError +from findmy import InvalidCredentialsError, MobileMeDelegateError, UnhandledProtocolError +from findmy.keychain.session import KeychainSessionError from exporter import cli, prompts from exporter.icloud import ExportSourceError -from exporter.version import EXPORT_VIA_CLI +from exporter.version import EXPORT_VIA_CLI, GITHUB_ISSUES_LINK from opentagviewer_export import ExportBundle, generate_passcode KEY = bytes(range(28)) @@ -507,3 +508,59 @@ async def _no(*_args, **_kwargs): monkeypatch.setattr(prompts, "confirm", _no) assert asyncio.run(cli._confirm_devices([mac], or_nothing=True)) == [] + + +class TestWhereToReportSomethingUnfixable: + """ + `UnhandledProtocolError` means Apple said something this library does not model. + + There is nothing for the user to correct, so the only useful next step is a bug report - and + for years this asked for one without saying where to put it. The window has always named the + URL in its error dialog; the headless half said "if you report this" and stopped. + + Issue #140 is the worked example: a `KeychainSessionError`, which is an + `UnhandledProtocolError` by inheritance, arriving as "no keychain keys are held" - which reads + as a broken account and is not one. + """ + + def _failing_run(self, monkeypatch, error): + # `run`, not `_run_and_return` - the handlers are inside the latter, so patching it + # would test nothing and let the exception straight out. + async def _boom(_arguments): + raise error + + monkeypatch.setattr(cli, "run", _boom) + + def test_it_names_where_to_report(self, monkeypatch, capsys): + self._failing_run(monkeypatch, UnhandledProtocolError("no keychain keys are held")) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK in capsys.readouterr().err + + def test_it_still_says_what_apple_did(self, monkeypatch, capsys): + # The link is an addition, not a replacement: the message is the only description of the + # actual problem anybody has. + self._failing_run(monkeypatch, UnhandledProtocolError("no keychain keys are held")) + + cli.main([]) + + assert "no keychain keys are held" in capsys.readouterr().err + + def test_a_keychain_failure_reaches_the_same_place(self, monkeypatch, capsys): + # By inheritance rather than by being listed, which is the only reason #140 got a usable + # message at all. A separate handler that forgot it would be silent here. + self._failing_run( + monkeypatch, + KeychainSessionError("No keychain keys are held, so nothing can be decrypted."), + ) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK in capsys.readouterr().err + + def test_a_mistake_the_user_can_fix_does_not_ask_for_a_bug_report(self, monkeypatch, capsys): + # The distinction that makes the link worth anything. Being sent to file an issue about + # your own typo is wrong, and it teaches people to ignore the link when it matters. + self._failing_run(monkeypatch, ExportSourceError("Signing in was stopped.")) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK not in capsys.readouterr().err