Merge pull request #239 from opensafely-core/directories

Add support for directories of files without ACRO metdata

.gitignore

@@ -2,11 +2,14 @@
 .wenv/
 .ipynb*
 .pytest-cache/
+.ruff_cache
+.pytest_cache
 .coverage
 __pycache__
 .*.sw*
 /build
 sacro-app/dist
+sacro-app/SACRO
 node_modules/
 assets/dist
 sacro/staticfiles/

README.md

@@ -31,14 +31,10 @@ When the installation completes, it will run the application.
 
 ## Usage
 
-To view some ACRO outputs, you need to open the ACRO generated JSON file for
-those outputs.
+To view outputs, you need to open a directory containing the output files. The Viewer will detect if there is ACRO-generated metadata, and use that to display the files. If there is no ACRO data, it will generate some, adding each file in the directory as a `custom` ACRO outputs.
 
-If you don't have any, you can use the test outputs we have included in the
-downloaded zip to get started with.
-
-Navigate to and select the `.json` file in the ACRO `outputs` directory (`results.json` by
-default). If  you are using the test outputs, it is `outputs\results.json`.
+If you don't have any outputs to hand, you can use the test outputs we have included in the
+downloaded zip to get started with: navigate to and select directory `outputs`  in the zip file.
 
 In the appliction, you should now see the list of outputs on the left, and can
 select each one to view it.

sacro-app/src/create-window.js

@@ -11,6 +11,14 @@ const { waitThenLoad } = require("./utils");
 
 let TEMPDIR = null;
 
+function rtrim(x, characters) {
+  let end = x.length - 1;
+  while (characters.indexOf(x[end]) >= 0) {
+    end -= 1;
+  }
+  return x.substr(0, end + 1);
+}
+
 const createWindow = async () => {
   let serverUrl = process.env.SACRO_URL;
   let serverProcess = null;
@@ -21,6 +29,8 @@ const createWindow = async () => {
     serverProcess = server;
   }
 
+  serverUrl = rtrim(serverUrl, "/");
+
   console.log(`Using ${serverUrl} as backend`);
 
   // handle downloads
@@ -73,20 +83,16 @@ const createWindow = async () => {
   });
 
   const result = await dialog.showOpenDialog({
-    title: "Choose ACRO outputs json file",
-    properties: ["openFile"],
+    title: "Choose directory containing outputs you wish to review",
+    properties: ["openDirectory"],
     defaultPath: os.homedir(),
-    filters: [
-      { name: "ACRO Outputs", extensions: ["json", "acro"] },
-      { name: "All files", extensions: ["*"] },
-    ],
   });
 
   if (result.canceled) {
     win.loadFile("no-file.html");
   } else {
-    const qs = querystring.stringify({ path: result.filePaths[0] });
-    const url = `${serverUrl}?${qs}`;
+    const qs = querystring.stringify({ dirpath: result.filePaths[0] });
+    const url = `${serverUrl}/load?${qs}`;
 
     const timeout = serverProcess === null ? 0 : 4000;
     waitThenLoad(url, timeout)

sacro-app/src/main-menu.js

@@ -12,25 +12,21 @@ const mainMenu = (serverUrl) => {
       label: "File",
       submenu: [
         {
-          label: "Open File",
+          label: "Open Directory",
           accelerator: "CommandOrControl+O",
           click: async () => {
             dialog
               .showOpenDialog({
-                title: "Choose ACRO outputs json file",
-                properties: ["openFile"],
+                title: "Choose directory containing outputs you wish to review",
+                properties: ["openDirectory"],
                 defaultPath: os.homedir(),
-                filters: [
-                  { name: "ACRO Outputs", extensions: ["json", "acro"] },
-                  { name: "All files", extensions: ["*"] },
-                ],
               })
               .then((result) => {
                 if (!result.canceled) {
                   const qs = querystring.stringify({
-                    path: result.filePaths[0],
+                    dirpath: result.filePaths[0],
                   });
-                  const url = `${serverUrl}?${qs}`;
+                  const url = `${serverUrl}/load?${qs}`;
                   BrowserWindow.getFocusedWindow().loadURL(url);
                 }
               })

sacro/models.py

@@ -2,11 +2,89 @@
 import json
 from collections import defaultdict
 from dataclasses import dataclass, field
+from datetime import datetime
 from pathlib import Path
 
 from sacro import utils, versioning
 
 
+class MultipleACROFiles(Exception):
+    pass
+
+
+def find_acro_metadata(dirpath):
+    json_files = list(dirpath.glob("*.json"))
+    default = dirpath / "outputs.json"
+    path = None
+
+    if default.exists():
+        path = default
+    elif len(json_files) == 1:
+        path = dirpath / json_files[0]
+    else:
+        valid_jsons = []
+        for jf in json_files:
+            try:
+                ACROOutputs(dirpath / jf)
+            except ACROOutputs.InvalidFile:
+                pass
+            else:
+                valid_jsons.append(dirpath / jf)
+        if len(valid_jsons) == 1:
+            path = valid_jsons[0]
+        elif len(valid_jsons) > 1:
+            files = ", ".join(str(s.name) for s in valid_jsons)
+            raise MultipleACROFiles(
+                f"SACRO Viewer does not support multiple ACRO json files in the same directory\n"
+                f"Found {len(valid_jsons)}: {files}"
+            )
+
+    if path is None:
+        path = default
+        scaffold_acro_metadata(default)
+
+    return path
+
+
+def scaffold_acro_metadata(path):
+    dirpath = path.parent
+    checksums_dir = dirpath / "checksums"
+    metadata = {
+        "version": "0.4.0",
+        "results": {},
+    }
+
+    for output in dirpath.glob("*"):
+        if output.is_dir():
+            continue
+        uid = output.name
+        if uid.startswith("."):
+            continue
+        metadata["results"][uid] = {
+            "uid": uid,
+            "files": [{"name": output.name}],
+            "status": "review",
+            "type": "custom",
+            "properties": {},
+            "outcome": {},
+            "command": "custom",
+            "summary": "review",
+            "exception": None,
+            "timestamp": datetime.fromtimestamp(output.stat().st_mtime).isoformat(),
+            "comments": [
+                "This non-ACRO output metadata was auto generated the SACRO Viewer application",
+            ],
+        }
+
+        # Write the checksums at the time of first looking at the directory
+        # This is a bit of a hack. Ideally, we'd find a way to disable checksums in such cases
+        checksums_dir.mkdir(exist_ok=True)
+        checksum_path = checksums_dir / (output.name + ".txt")
+        checksum_path.write_text(hashlib.sha256(output.read_bytes()).hexdigest())
+
+    path.write_text(json.dumps(metadata, indent=2))
+
+
 def load_from_path(path):
     """Use outputs path from request and load it"""
     outputs = ACROOutputs(path)

sacro/templates/error.html

@@ -23,7 +23,7 @@ <h1 class="text-3xl font-bold leading-tight tracking-tight">
       <div class="flex flex-col gap-y-4 text-lg">
         <p class="break-words">
           {% if message %}
-            {{ message }}
+            {{ message|linebreaks }}
           {% else %}
             Please restart the application and try again.<br />
             If this problem continues please contact support.

sacro/urls.py

@@ -8,6 +8,7 @@
 
 urlpatterns = [
     path("", views.index, name="index"),
+    path("load/", views.load, name="load"),
     path("contents/", views.contents, name="contents"),
     path("error/", errors.error, name="error"),
     path("review/", views.review_create, name="review-create"),

sacro/views.py

@@ -12,7 +12,7 @@
 from django.urls import reverse
 from django.views.decorators.http import require_GET, require_POST
 
-from sacro import models, utils
+from sacro import errors, models, utils
 from sacro.adapters import local_audit, zipfile
 
 
@@ -21,9 +21,8 @@
 REVIEWS = {}
 
 
-def get_outputs_from_request(data):
-    """Use outputs path from request and load it"""
-    param_path = data.get("path")
+def get_filepath_from_request(data, name):
+    param_path = data.get(name)
     if param_path is None:
         raise Http404
 
@@ -32,9 +31,25 @@ def get_outputs_from_request(data):
     if not path.exists():  # pragma: no cover
         raise Http404
 
+    return path
+
+
+def get_outputs_from_request(data):
+    """Use outputs path from request and load it"""
+    path = get_filepath_from_request(data, "path")
     return models.load_from_path(path)
 
 
+@require_GET
+def load(request):
+    dirpath = get_filepath_from_request(request.GET, "dirpath")
+    try:
+        path = models.find_acro_metadata(dirpath)
+    except models.MultipleACROFiles as exc:
+        return errors.error(request, status=500, message=str(exc))
+    return redirect(utils.reverse_with_params({"path": str(path)}, "index"))
+
+
 @require_GET
 def index(request):
     """Render the template with all details"""

tests/test_models.py

@@ -54,3 +54,43 @@ def test_validation(data, tmp_path):
     bad_json.write_text(json.dumps(data))
     with pytest.raises(models.ACROOutputs.InvalidFile):
         models.ACROOutputs(bad_json)
+
+
+def test_find_acro_metadata(test_outputs):
+    dirpath = test_outputs.path.parent
+    # remove other json files to have blank slate
+    (dirpath / "config.json").unlink()
+    (dirpath / "XandY.json").unlink()
+
+    default = test_outputs.path.rename(dirpath / "outputs.json")
+
+    # test default name
+    assert models.find_acro_metadata(dirpath) == default
+
+    # test alt name
+    expected = default.rename(dirpath / "results.json")
+    assert models.find_acro_metadata(dirpath) == expected
+
+    # test additional bad file
+    (dirpath / "notit.json").write_text("{}")
+    assert models.find_acro_metadata(dirpath) == expected
+
+    # test additional valid file error
+    (dirpath / "valid.json").write_text(json.dumps(test_outputs.raw_metadata))
+    with pytest.raises(models.MultipleACROFiles):
+        models.find_acro_metadata(dirpath)
+
+
+def test_find_acro_metadata_no_file(test_outputs):
+    test_outputs.path.unlink()
+
+    dirpath = test_outputs.path.parent
+    path = models.find_acro_metadata(dirpath)
+    # check the generated metadata loads correctly
+    outputs = models.ACROOutputs(path)
+    for name, result in outputs.items():
+        assert result["files"][0]["name"] == name
+        assert result["status"] == "review"
+        assert result["type"] == "custom"
+        assert result["command"] == "custom"
+        assert result["summary"] == "review"

tests/test_views.py

@@ -12,6 +12,26 @@
 from sacro import models, views
 
 
+def test_load(test_outputs):
+    request = RequestFactory().get(
+        path="/load", data={"dirpath": str(test_outputs.path.parent)}
+    )
+    response = views.load(request)
+    assert response.status_code == 302
+    assert response.headers["Location"] == f"/?{urlencode({'path': test_outputs.path})}"
+
+
+def test_load_multiple(test_outputs):
+    (test_outputs.path.parent / "valid.json").write_text(
+        json.dumps(test_outputs.raw_metadata)
+    )
+    request = RequestFactory().get(
+        path="/load", data={"dirpath": str(test_outputs.path.parent)}
+    )
+    response = views.load(request)
+    assert response.status_code == 500
+
+
 def test_index(test_outputs):
     request = RequestFactory().get(path="/", data={"path": str(test_outputs.path)})