From fe92036b54934c45d6f9c78c532ac508a1d762b9 Mon Sep 17 00:00:00 2001 From: Simon Davy Date: Tue, 17 Oct 2023 13:55:42 +0100 Subject: [PATCH 1/5] Add support for directories of files without ACRO metdata This means changing to use a directory picker, rather than a file picker. This means a bit of guess work when there are acro metadata files in the directory. The upshot of this is: a) we now launch the app in a special /load url to do this guess work b) we no longer support a directory with multiple acro metadata files in --- sacro-app/src/create-window.js | 12 ++---- sacro/models.py | 70 ++++++++++++++++++++++++++++++++++ sacro/templates/error.html | 2 +- sacro/urls.py | 1 + sacro/views.py | 23 +++++++++-- tests/test_models.py | 40 +++++++++++++++++++ tests/test_views.py | 20 ++++++++++ 7 files changed, 155 insertions(+), 13 deletions(-) diff --git a/sacro-app/src/create-window.js b/sacro-app/src/create-window.js index c085fe90..d316189e 100644 --- a/sacro-app/src/create-window.js +++ b/sacro-app/src/create-window.js @@ -73,20 +73,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) diff --git a/sacro/models.py b/sacro/models.py index 6384ae06..76000988 100644 --- a/sacro/models.py +++ b/sacro/models.py @@ -2,11 +2,81 @@ 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 + 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 ACRO output was auto generated the SACRO Viewer application", + ], + } + path.write_text(json.dumps(metadata, indent=2)) + + def load_from_path(path): """Use outputs path from request and load it""" outputs = ACROOutputs(path) diff --git a/sacro/templates/error.html b/sacro/templates/error.html index 5197afd7..8ab3c825 100644 --- a/sacro/templates/error.html +++ b/sacro/templates/error.html @@ -23,7 +23,7 @@

{% if message %} - {{ message }} + {{ message|linebreaks }} {% else %} Please restart the application and try again.
If this problem continues please contact support. diff --git a/sacro/urls.py b/sacro/urls.py index 90a1e670..c3599a01 100644 --- a/sacro/urls.py +++ b/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"), diff --git a/sacro/views.py b/sacro/views.py index 68b169f9..510babd0 100644 --- a/sacro/views.py +++ b/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""" diff --git a/tests/test_models.py b/tests/test_models.py index 1135fbd2..1bc4c8e5 100644 --- a/tests/test_models.py +++ b/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" diff --git a/tests/test_views.py b/tests/test_views.py index 556749bb..bfe36987 100644 --- a/tests/test_views.py +++ b/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)}) From a85fa609c22df8c4f71fae569b736d1559f3d37c Mon Sep 17 00:00:00 2001 From: Simon Davy Date: Tue, 17 Oct 2023 16:10:53 +0100 Subject: [PATCH 2/5] Strip trailing any / from serverUrl --- sacro-app/src/create-window.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sacro-app/src/create-window.js b/sacro-app/src/create-window.js index d316189e..e5c7945c 100644 --- a/sacro-app/src/create-window.js +++ b/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 From a8ea31124dea04191e237cba17937cf61c91733a Mon Sep 17 00:00:00 2001 From: Simon Davy Date: Tue, 17 Oct 2023 16:22:08 +0100 Subject: [PATCH 3/5] Fix File dialog to match --- sacro-app/src/main-menu.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/sacro-app/src/main-menu.js b/sacro-app/src/main-menu.js index 595b3eff..92f4a626 100644 --- a/sacro-app/src/main-menu.js +++ b/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); } }) From 331cbd075667d52858f9bed1fe749f0645eca601 Mon Sep 17 00:00:00 2001 From: Simon Davy Date: Tue, 17 Oct 2023 17:47:29 +0100 Subject: [PATCH 4/5] Fix UI warts - better comment text - calculate checksums to silence notice It'd be better to switch off checkshums in the non-ACRO case. But that trickier, so this hack should work --- sacro/models.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/sacro/models.py b/sacro/models.py index 76000988..7a791883 100644 --- a/sacro/models.py +++ b/sacro/models.py @@ -48,6 +48,7 @@ def find_acro_metadata(dirpath): def scaffold_acro_metadata(path): dirpath = path.parent + checksums_dir = dirpath / "checksums" metadata = { "version": "0.4.0", "results": {}, @@ -71,9 +72,16 @@ def scaffold_acro_metadata(path): "exception": None, "timestamp": datetime.fromtimestamp(output.stat().st_mtime).isoformat(), "comments": [ - "This ACRO output was auto generated the SACRO Viewer application", + "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)) From 8088cb1e7f8b6657bbf2b6253bd2ef5ef1813973 Mon Sep 17 00:00:00 2001 From: Simon Davy Date: Wed, 18 Oct 2023 12:35:45 +0100 Subject: [PATCH 5/5] tweak docs and README --- .gitignore | 3 +++ README.md | 10 +++------- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 78a3abbb..34da45fe 100644 --- a/.gitignore +++ b/.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/ diff --git a/README.md b/README.md index b50149e8..17943960 100644 --- a/README.md +++ b/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.