fix(dev-admin): a version check that did not happen says so

tests/test_dev_admin.py

@@ -402,6 +402,26 @@ def test_version_check_handler_registered(self):
         assert method == "GET"
 
 
+def _fake_pypi(payload):
+    """Stand in for ``urllib.request.urlopen`` with a canned PyPI body."""
+    import json as _json
+
+    class _Resp:
+        def read(self):
+            return _json.dumps(payload).encode()
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *_):
+            return False
+
+    def _open(*_args, **_kwargs):
+        return _Resp()
+
+    return _open
+
+
 class TestAPIHandlers:
     """Test API handler functions with mock request/response."""
 
@@ -738,13 +758,85 @@ async def test_connections_save_preserves_tina4_prefix(
         assert "OTHER_VAR=keep-me" in written
 
     @pytest.mark.asyncio
-    async def test_version_check_handler(self, mock_req, mock_resp):
+    async def test_version_check_handler(self, mock_req, mock_resp, monkeypatch):
+        """A successful check reports what PyPI published.
+
+        Stubbed rather than live: the old version of this test called PyPI for
+        real and asserted ``latest`` was always a string, which is exactly the
+        bug — offline, the handler invented one.
+        """
+        import urllib.request
         from tina4_python.dev_admin import _api_version_check
+
+        monkeypatch.setattr(
+            urllib.request, "urlopen", _fake_pypi({"info": {"version": "9.9.9"}})
+        )
         result = await _api_version_check(mock_req, mock_resp)
-        assert "current" in result
-        assert "latest" in result
         assert isinstance(result["current"], str)
-        assert isinstance(result["latest"], str)
+        assert result["latest"] == "9.9.9"
+        assert "error" not in result
+
+    @pytest.mark.asyncio
+    async def test_version_check_reports_a_failure_instead_of_claiming_up_to_date(
+        self, mock_req, mock_resp, monkeypatch
+    ):
+        """The toolbar renders ``latest == current`` as "You are up to date!".
+
+        So a check that never reached PyPI must not answer with it. A developer
+        several releases behind, on a machine with no route out, was being told
+        the opposite of the truth.
+        """
+        import urllib.error
+        import urllib.request
+        from tina4_python.dev_admin import _api_version_check
+
+        def explode(*_args, **_kwargs):
+            raise urllib.error.URLError("no route to host")
+
+        monkeypatch.setattr(urllib.request, "urlopen", explode)
+        result = await _api_version_check(mock_req, mock_resp)
+
+        assert result["latest"] is None, (
+            "a check that did not happen must not answer with a version"
+        )
+        assert result["latest"] != result["current"]
+        assert result.get("error"), "the reason has to reach the client"
+
+    @pytest.mark.asyncio
+    async def test_version_check_does_not_invent_a_version_from_an_unreadable_answer(
+        self, mock_req, mock_resp, monkeypatch
+    ):
+        """Reaching PyPI is not the same as learning the version.
+
+        An answer with no version field used to fall back to ``current`` down
+        the same path as being offline, which is the same lie by another route.
+        """
+        import urllib.request
+        from tina4_python.dev_admin import _api_version_check
+
+        monkeypatch.setattr(urllib.request, "urlopen", _fake_pypi({"info": {}}))
+        result = await _api_version_check(mock_req, mock_resp)
+
+        assert result["latest"] is None
+        assert result.get("error")
+
+    def test_toolbar_reads_a_missing_latest_as_a_failed_check(self):
+        """The client half of the same defect.
+
+        The toolbar already had a "Could not check for updates (offline?)"
+        branch, on ``fetch``'s ``.catch``, which could never fire because the
+        server answered 200. It has to act on the null the server now sends.
+        """
+        from tina4_python.dev_admin import toolbar_js
+
+        js = toolbar_js()
+        assert "couldNotCheck" in js, "no branch for a check that did not happen"
+        assert "if (!latest) { couldNotCheck" in js, (
+            "a missing latest must be handled before the up-to-date comparison"
+        )
+        assert js.index("if (!latest)") < js.index("if (latest === current)"), (
+            "the up-to-date branch must not run first — null would fall into it"
+        )
 
     @pytest.mark.asyncio
     async def test_query_multi_statement_rollback(self, mock_req, mock_resp, tmp_path, monkeypatch):

tina4_python/dev_admin/__init__.py

@@ -1933,20 +1933,45 @@ def _route_count() -> int:
 
 
 async def _api_version_check(request, response):
-    """Proxy version check to PyPI to avoid browser CORS errors."""
+    """Proxy the version check to PyPI to avoid browser CORS errors.
+
+    A check that did not happen says so. This used to fall back to
+    ``latest = current`` on any failure, which the toolbar renders as a green
+    "You are up to date!" — so a developer several releases behind, on a
+    machine with no route out, was told the opposite of the truth. The toolbar
+    already had the right message for this case and could never reach it,
+    because the failure arrived as a success.
+
+    ``latest`` is ``None`` when the check could not be made, and ``error``
+    carries the short reason.
+    """
     import urllib.request
     current = __version__
-    latest = current
     try:
         req = urllib.request.Request(
             "https://pypi.org/pypi/tina4-python/json",
             headers={"User-Agent": "tina4-python/" + current},
         )
         with urllib.request.urlopen(req, timeout=5) as resp:
             data = json.loads(resp.read().decode())
-            latest = data.get("info", {}).get("version", current)
-    except Exception:
-        pass  # Offline or timeout — return current as latest
+        latest = data.get("info", {}).get("version")
+        if not latest:
+            # Reached PyPI, got something we cannot read a version out of.
+            return response(
+                {
+                    "current": current,
+                    "latest": None,
+                    "error": "PyPI did not report a version",
+                }
+            )
+    except Exception as exc:
+        return response(
+            {
+                "current": current,
+                "latest": None,
+                "error": "{}: {}".format(type(exc).__name__, exc),
+            }
+        )
     return response({"current": current, "latest": latest})
 
 
@@ -2139,6 +2164,13 @@ def toolbar_js() -> str:
         el.className = 't4-ok';
         el.innerHTML = 'Latest: <strong class="t4-ok">v' + latest + '</strong> &mdash; You are up to date!';
     }
+    // A check that did not happen is not a clean bill of health. The server
+    // sends latest: null when it could not reach PyPI, and saying so is the
+    // whole point -- "up to date" here would be a guess dressed as a fact.
+    function couldNotCheck(el, why) {
+        el.className = 't4-err';
+        el.textContent = 'Could not check for updates' + (why ? ' (' + why + ')' : '');
+    }
     function checkVersion() {
         if (modal.style.display === 'block') { modal.style.display = 'none'; return; }
         modal.style.display = 'block';
@@ -2147,6 +2179,7 @@ def toolbar_js() -> str:
         el.textContent = 'Checking for updates...';
         fetch('/__dev/api/version-check').then(function (r) { return r.json(); }).then(function (d) {
             var latest = d.latest, current = d.current;
+            if (!latest) { couldNotCheck(el, d.error); return; }
             if (latest === current) { upToDate(el, latest); return; }
             var cP = current.split('.').map(Number), lP = latest.split('.').map(Number);
             var isNewer = false, i, c, l;