changes from code reviews

Author: jason-raitz

CHANGELOG.md

@@ -8,12 +8,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased (Targeting 0.2.5)]
 
 ### Added
-- Optional parameter to `fetch_file()` with a modified time of the remote file pulled from the TIND API
-- `fetch_file()` uses this to avoid unnecessary downloads if a file already exists at the target
-  location and has a modified time that is newer than the requested file
+- Optional parameter to `fetch_file()` with a modified time of the remote file pulled from the TIND API.
+  `fetch_file()` uses this to avoid unnecessary downloads if a file already exists at the target
+  location and has a modified time that is newer than the requested file.
+- Added tests for this new behavior.
 
 ### Changed
-- slight change to raise a file not downloaded error if `tind_download()` fails to return a written file path
+- slight change to raise a file not downloaded error if `tind_download()` fails to return a written file path.
 
 
 ## [0.2.4]

tests/test_fetch.py

@@ -5,7 +5,9 @@
 import json
 import xml.etree.ElementTree as E
 
+from datetime import datetime, timezone
 from pathlib import Path
+from unittest.mock import MagicMock, patch
 
 import pytest
 import requests_mock as req_mock  # noqa: F401 — activates the requests_mock fixture
@@ -87,6 +89,59 @@ def test_fetch_file_not_found(
         client.fetch_file(url, output_dir=str(tmp_path))
 
 
+def test_fetch_file_skips_download_when_local_file_is_newer(
+    tmp_path: Path,
+    client: TINDClient,
+) -> None:
+    """fetch_file returns the cached path without downloading when local mtime >= meta_mtime."""
+
+    url = f"{BASE_URL}/api/v1/record/12345/files/some-image.jpg/download/?version=1"
+    cached = tmp_path / "some-image.jpg"
+    cached.write_bytes(b"cached content")
+
+    meta_mtime = datetime(2026, 1, 1, 0, 0, 0, tzinfo=timezone.utc)
+    local_mtime = datetime(2026, 1, 3, 0, 0, 0, tzinfo=timezone.utc)  # newer
+
+    mock_stat = MagicMock()
+    mock_stat.st_mtime = local_mtime.timestamp()
+
+    with patch.object(Path, "stat", return_value=mock_stat):
+        result = client.fetch_file(url, output_dir=str(tmp_path), meta_mtime=meta_mtime)
+
+    assert result == str(cached)
+
+
+def test_fetch_file_redownloads_when_local_file_is_older(
+    requests_mock: req_mock.Mocker,
+    tmp_path: Path,
+    client: TINDClient,
+) -> None:
+    """fetch_file re-downloads when local mtime < meta_mtime."""
+
+    url = f"{BASE_URL}/api/v1/record/12345/files/some-other-image.jpg/download/?version=1"
+    cached = tmp_path / "some-other-image.jpg"
+    cached.write_bytes(b"stale content")
+
+    meta_mtime = datetime(2026, 1, 3, 0, 0, 0, tzinfo=timezone.utc)
+    local_mtime = datetime(2026, 1, 1, 0, 0, 0, tzinfo=timezone.utc)  # older
+
+    mock_stat = MagicMock()
+    mock_stat.st_mtime = local_mtime.timestamp()
+
+    requests_mock.get(
+        url,
+        content=b"fresh content",
+        status_code=200,
+        headers={"Content-Disposition": 'attachment; filename="some-other-image.jpg"'},
+    )
+
+    with patch.object(Path, "stat", return_value=mock_stat):
+        result = client.fetch_file(url, output_dir=str(tmp_path), meta_mtime=meta_mtime)
+
+    assert result.endswith("some-other-image.jpg")
+    assert Path(result).read_bytes() == b"fresh content"
+
+
 # ---------------------------------------------------------------------------
 # fetch_file_metadata
 # ---------------------------------------------------------------------------

tind_client/client.py

@@ -72,17 +72,19 @@ def fetch_metadata(self, record: str) -> Record:
 
         return records[0]
 
-    def fetch_file(self, file_url: str, output_dir: str = "", modified: str = "") -> str:
+    def fetch_file(
+        self, file_url: str, output_dir: str = "", meta_mtime: datetime | None = None
+    ) -> str:
         """Download a file from TIND and save it locally.
 
         If the file already exists in the output directory and was modified at or after a supplied
-        ``modified`` timestamp, the file will not be re-downloaded.
+        ``meta_mtime`` timestamp, the file will not be re-downloaded.
 
         :param str file_url: The TIND file download URL.
         :param str output_dir: Directory in which to save the file.
                                Falls back to ``default_storage_dir`` when empty.
-        :param str modified: Optional modified timestamp from the file metadata returned by TIND.
-                             If not specified, the file will always be downloaded.
+        :param datetime meta_mtime: Optional modified timestamp from the file metadata returned by
+                                    TIND. If not specified, the file will always be downloaded.
         :raises AuthorizationError: When the TIND API key is invalid or the file is restricted.
         :raises ValueError: When ``file_url`` is not a valid TIND file download URL.
         :raises RecordNotFoundError: When the file is invalid or not found.
@@ -96,8 +98,7 @@ def fetch_file(self, file_url: str, output_dir: str = "", modified: str = "") ->
         expected_filename = file_url.rstrip("/").split("/")[-2]
         expected_path = Path(output_target) / expected_filename
 
-        if modified and expected_path.exists():
-            meta_mtime = datetime.fromisoformat(modified).replace(tzinfo=timezone.utc)
+        if meta_mtime and expected_path.exists():
             local_mtime = datetime.fromtimestamp(expected_path.stat().st_mtime, tz=timezone.utc)
             if local_mtime >= meta_mtime:
                 logger.debug("Cached file at (%s) is newer; skipping download.", expected_path)