From 31f9a8106225f538f790b14ef6ef731ac117b244 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Tue, 14 Jan 2020 15:32:50 +0000
Subject: [PATCH 01/10] Remove unused `Dataset.is_visible_to_user`

---
 weblab/datasets/models.py            | 11 -----------
 weblab/datasets/tests/test_models.py |  5 -----
 2 files changed, 16 deletions(-)

diff --git a/weblab/datasets/models.py b/weblab/datasets/models.py
index 4447edf67..88950b19e 100644
--- a/weblab/datasets/models.py
+++ b/weblab/datasets/models.py
@@ -4,7 +4,6 @@
 
 from core.combine import ArchiveReader
 from core.models import UserCreatedModelMixin, VisibilityModelMixin
-from core.visibility import visibility_check
 from entities.models import ProtocolEntity
 
 
@@ -51,16 +50,6 @@ def files(self):
     def open_file(self, name):
         return ArchiveReader(str(self.archive_path)).open_file(name)
 
-    def is_visible_to_user(self, user):
-        """
-        Can the user view the dataset?
-
-        :param user: user to test against
-
-        :returns: True if the user is allowed to view the dataset, False otherwise
-        """
-        return visibility_check(self.visibility, self.viewers, user)
-
 
 class DatasetFile(models.Model):
     dataset = models.ForeignKey(Dataset, related_name='file_uploads')
diff --git a/weblab/datasets/tests/test_models.py b/weblab/datasets/tests/test_models.py
index afa3b08df..fd60908fa 100644
--- a/weblab/datasets/tests/test_models.py
+++ b/weblab/datasets/tests/test_models.py
@@ -11,11 +11,6 @@ def test_str(self):
         dataset = recipes.dataset.make(name='test dataset')
         assert str(dataset) == 'test dataset'
 
-    def test_is_visible_to_user(self, user, other_user):
-        dataset = recipes.dataset.make(author=user, name='mydataset', visibility="private")
-        assert dataset.is_visible_to_user(user)
-        assert not dataset.is_visible_to_user(other_user)
-
     def test_related_protocol(self, user):
         protocol = recipes.protocol.make(author=user)
         dataset = recipes.dataset.make(author=user, name='mydataset', protocol=protocol)

From 0c53a2cb1b57d1c0c2c78dfc0d26c39de4492f33 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Tue, 14 Jan 2020 15:57:03 +0000
Subject: [PATCH 02/10] Add `FileCollectionMixin` and use it

---
 weblab/core/models.py        | 45 ++++++++++++++++++++++++++++++++++++
 weblab/datasets/models.py    | 19 ++-------------
 weblab/experiments/models.py | 23 ++++--------------
 3 files changed, 51 insertions(+), 36 deletions(-)

diff --git a/weblab/core/models.py b/weblab/core/models.py
index 916521269..2313ca98d 100644
--- a/weblab/core/models.py
+++ b/weblab/core/models.py
@@ -3,6 +3,7 @@
 from guardian.shortcuts import assign_perm, get_users_with_perms, remove_perm
 
 from . import visibility
+from .combine import ArchiveReader
 
 
 class VisibilityModelMixin(models.Model):
@@ -95,3 +96,47 @@ def is_managed_by(self, user):
 
     class Meta:
         abstract = True
+
+
+class FileCollectionMixin:
+    """Mixin for DB models that represent collections of files backed by a COMBINE Archive.
+
+    This doesn't provide any DB fields, but does define common properties and methods for
+    such collections, ensuring they present a consistent API.
+    """
+    @property
+    def abs_path(self):
+        """The folder where the backing archive is stored on disk.
+
+        Must be defined by subclasses.
+        """
+        raise NotImplementedError
+
+    @property
+    def archive_name(self):
+        """The name of the backing archive.
+
+        Must be defined by subclasses.
+        """
+        raise NotImplementedError
+
+    @property
+    def archive_path(self):
+        """The full path to the backing archive. A ``pathlib.Path`` instance."""
+        return self.abs_path / self.archive_name
+
+    @property
+    def files(self):
+        """The list of files (``core.combine.ArchiveFile`` instances) contained in this archive."""
+        if self.archive_path.exists():
+            return ArchiveReader(str(self.archive_path)).files
+        else:
+            return []
+
+    def mkdir(self):
+        """Create the folder for the backing archive (and parents if needed)."""
+        self.abs_path.mkdir(exist_ok=True, parents=True)
+
+    def open_file(self, name):
+        """Open the given file in the archive for reading."""
+        return ArchiveReader(str(self.archive_path)).open_file(name)
diff --git a/weblab/datasets/models.py b/weblab/datasets/models.py
index 88950b19e..b309e407e 100644
--- a/weblab/datasets/models.py
+++ b/weblab/datasets/models.py
@@ -2,12 +2,11 @@
 from django.db import models
 from django.utils.text import get_valid_filename
 
-from core.combine import ArchiveReader
-from core.models import UserCreatedModelMixin, VisibilityModelMixin
+from core.models import FileCollectionMixin, UserCreatedModelMixin, VisibilityModelMixin
 from entities.models import ProtocolEntity
 
 
-class Dataset(UserCreatedModelMixin, VisibilityModelMixin, models.Model):
+class Dataset(UserCreatedModelMixin, VisibilityModelMixin, FileCollectionMixin, models.Model):
     """Prototyping class for experimental datasets
     """
     name = models.CharField(validators=[MinLengthValidator(2)], max_length=255)
@@ -36,20 +35,6 @@ def abs_path(self):
     def archive_name(self):
         return get_valid_filename(self.name + '.zip')
 
-    @property
-    def archive_path(self):
-        return self.abs_path / self.archive_name
-
-    @property
-    def files(self):
-        if self.archive_path.exists():
-            return ArchiveReader(str(self.archive_path)).files
-        else:
-            return []
-
-    def open_file(self, name):
-        return ArchiveReader(str(self.archive_path)).open_file(name)
-
 
 class DatasetFile(models.Model):
     dataset = models.ForeignKey(Dataset, related_name='file_uploads')
diff --git a/weblab/experiments/models.py b/weblab/experiments/models.py
index 3a44afa60..409d532c0 100644
--- a/weblab/experiments/models.py
+++ b/weblab/experiments/models.py
@@ -4,8 +4,7 @@
 from django.conf import settings
 from django.db import models
 
-from core.combine import ArchiveReader
-from core.models import UserCreatedModelMixin
+from core.models import FileCollectionMixin, UserCreatedModelMixin
 from core.visibility import Visibility, get_joint_visibility, visibility_check
 from entities.models import ModelEntity, ProtocolEntity
 
@@ -119,7 +118,7 @@ def latest_result(self):
             return ''
 
 
-class ExperimentVersion(UserCreatedModelMixin, models.Model):
+class ExperimentVersion(UserCreatedModelMixin, FileCollectionMixin, models.Model):
     STATUS_QUEUED = "QUEUED"
     STATUS_RUNNING = "RUNNING"
     STATUS_SUCCESS = "SUCCESS"
@@ -177,13 +176,9 @@ def viewers(self):
     def abs_path(self):
         return Path(settings.EXPERIMENT_BASE, str(self.id))
 
-    def mkdir(self):
-        """Create the folder for this version's results (and parents if needed)."""
-        self.abs_path.mkdir(exist_ok=True, parents=True)
-
     @property
-    def archive_path(self):
-        return self.abs_path / 'results.omex'
+    def archive_name(self):
+        return 'results.omex'
 
     @property
     def signature(self):
@@ -209,16 +204,6 @@ def update(self, status, txt):
         self.return_text = txt
         self.save()
 
-    @property
-    def files(self):
-        if self.archive_path.exists():
-            return ArchiveReader(str(self.archive_path)).files
-        else:
-            return []
-
-    def open_file(self, name):
-        return ArchiveReader(str(self.archive_path)).open_file(name)
-
 
 class RunningExperiment(models.Model):
     """

From 4ee8bbe8cd6cd257beb368f728363226f5316d4c Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Tue, 14 Jan 2020 16:01:16 +0000
Subject: [PATCH 03/10] Reorder dataset views so creation-related views are
 together

---
 weblab/datasets/views.py | 42 ++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/weblab/datasets/views.py b/weblab/datasets/views.py
index f9d905eb5..9649abc50 100644
--- a/weblab/datasets/views.py
+++ b/weblab/datasets/views.py
@@ -46,27 +46,6 @@ def get_success_url(self):
         return reverse('datasets:addfiles', args=[self.object.pk])
 
 
-class DatasetListView(LoginRequiredMixin, ListView):
-    """
-    List all user's datasets
-    """
-    model = Dataset
-    template_name = 'datasets/dataset_list.html'
-
-    def get_queryset(self):
-        return Dataset.objects.filter(author=self.request.user)
-
-
-class DatasetView(VisibilityMixin, DetailView):
-    """
-    View a Dataset
-
-    """
-    model = Dataset
-    context_object_name = 'dataset'
-    template_name = 'datasets/dataset_detail.html'
-
-
 class DatasetAddFilesView(
     LoginRequiredMixin, FormMixin, DetailView
 ):
@@ -151,6 +130,27 @@ def post(self, request, *args, **kwargs):
             return HttpResponseBadRequest(form.errors)
 
 
+class DatasetListView(LoginRequiredMixin, ListView):
+    """
+    List all user's datasets
+    """
+    model = Dataset
+    template_name = 'datasets/dataset_list.html'
+
+    def get_queryset(self):
+        return self.model.objects.filter(author=self.request.user)
+
+
+class DatasetView(VisibilityMixin, DetailView):
+    """
+    View a Dataset
+
+    """
+    model = Dataset
+    context_object_name = 'dataset'
+    template_name = 'datasets/dataset_detail.html'
+
+
 class DatasetJsonView(VisibilityMixin, SingleObjectMixin, View):
     """
     Serve up json view of files in a dataset

From 450ae8051548b8a4d1098edc9ac26cf3b22e5398 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Tue, 14 Jan 2020 16:05:49 +0000
Subject: [PATCH 04/10] Make dataset views use dynamic namespace if they could
 be inherited

---
 weblab/datasets/views.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/weblab/datasets/views.py b/weblab/datasets/views.py
index 9649abc50..4293b22de 100644
--- a/weblab/datasets/views.py
+++ b/weblab/datasets/views.py
@@ -158,6 +158,7 @@ class DatasetJsonView(VisibilityMixin, SingleObjectMixin, View):
     model = Dataset
 
     def _file_json(self, archive_file):
+        ns = self.request.resolver_match.namespace
         dataset = self.object
         return {
             'id': archive_file.name,
@@ -168,12 +169,13 @@ def _file_json(self, archive_file):
             'masterFile': archive_file.is_master,
             'size': archive_file.size,
             'url': reverse(
-                'datasets:file_download',
+                ns + ':file_download',
                 args=[dataset.id, urllib.parse.quote(archive_file.name)]
             )
         }
 
     def get(self, request, *args, **kwargs):
+        ns = self.request.resolver_match.namespace
         dataset = self.object = self.get_object()
         files = [
             self._file_json(f)
@@ -192,7 +194,7 @@ def get(self, request, *args, **kwargs):
                 'files': files,
                 'numFiles': len(files),
                 'download_url': reverse(
-                    'datasets:archive', args=[dataset.id]
+                    ns + ':archive', args=[dataset.id]
                 ),
             }
         })
@@ -261,4 +263,5 @@ def test_func(self):
         return self.get_object().is_deletable_by(self.request.user)
 
     def get_success_url(self, *args, **kwargs):
-        return reverse('datasets:list')
+        ns = self.request.resolver_match.namespace
+        return reverse(ns + ':list')

From 34f04c928658710e9d82e8df5b95124a659faac5 Mon Sep 17 00:00:00 2001
From: "steve@roderick.com" <steve@roderick.com>
Date: Tue, 14 Jan 2020 12:33:00 +0000
Subject: [PATCH 05/10] Changes required to support running tests in windows

---
 weblab/accounts/models.py               | 12 ------------
 weblab/conftest.py                      |  3 ---
 weblab/core/combine.py                  |  3 +++
 weblab/entities/repository.py           | 13 ++++++++++++-
 weblab/entities/tests/test_models.py    |  4 +++-
 weblab/experiments/tests/test_models.py |  5 +++--
 6 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/weblab/accounts/models.py b/weblab/accounts/models.py
index b5321b4ae..01f289574 100644
--- a/weblab/accounts/models.py
+++ b/weblab/accounts/models.py
@@ -60,15 +60,3 @@ def get_storage_dir(self, kind):
         :return: `Path` object
         """
         return self.STORAGE_DIRS[kind] / str(self.id)
-
-    def clean_up_storage(self):
-        """Remove all on-disk storage associated with this user.
-
-        Intended for use by tests, not production code. It works around the problem
-        that DB transaction rollback doesn't call pre_delete signals.
-        """
-        from shutil import rmtree
-        for kind in self.STORAGE_DIRS.keys():
-            storage_dir = self.get_storage_dir(kind)
-            if storage_dir.exists():
-                rmtree(str(storage_dir))
diff --git a/weblab/conftest.py b/weblab/conftest.py
index 8c305c220..1836685a2 100644
--- a/weblab/conftest.py
+++ b/weblab/conftest.py
@@ -239,7 +239,6 @@ def admin_user():
         password='password',
     )
     yield user
-    user.clean_up_storage()
 
 
 @pytest.fixture
@@ -251,7 +250,6 @@ def user():
         password='password',
     )
     yield user
-    user.clean_up_storage()
 
 
 @pytest.fixture
@@ -263,7 +261,6 @@ def other_user():
         password='password',
     )
     yield user
-    user.clean_up_storage()
 
 
 @pytest.fixture
diff --git a/weblab/core/combine.py b/weblab/core/combine.py
index 47110d79d..9c94f859f 100644
--- a/weblab/core/combine.py
+++ b/weblab/core/combine.py
@@ -46,6 +46,9 @@ def identify_mime_type(path):
         :path: Path of target file
         :return: namespaced mime type if identified, empty string otherwise
         """
+        # make csv mapping explicit (in windows tests, defaults to excel)
+        mimetypes.add_type('text/csv', '.csv')
+
         fmt, _ = mimetypes.guess_type(path)
         return MIME_NS + fmt if fmt else ''
 
diff --git a/weblab/entities/repository.py b/weblab/entities/repository.py
index d6582f5e0..1e8fed3a5 100644
--- a/weblab/entities/repository.py
+++ b/weblab/entities/repository.py
@@ -1,5 +1,7 @@
 import binascii
+import errno
 import os
+import stat
 from datetime import datetime
 from itertools import chain
 from pathlib import Path
@@ -44,7 +46,16 @@ def delete(self):
         """
         Delete the repository
         """
-        rmtree(self._root)
+        rmtree(self._root, ignore_errors=False, onerror=self.handle_remove_readonly)
+
+    @staticmethod
+    def handle_remove_readonly(func, path, exc):
+        """
+        In Windows.os repo files are marked as read only
+        """
+        if exc[1].errno == errno.EACCES:
+            os.chmod(path, stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)
+            func(path)
 
     @cached_property
     def _repo(self):
diff --git a/weblab/entities/tests/test_models.py b/weblab/entities/tests/test_models.py
index e5f519764..ebfe1c9ad 100644
--- a/weblab/entities/tests/test_models.py
+++ b/weblab/entities/tests/test_models.py
@@ -1,3 +1,5 @@
+import os
+
 import pytest
 from django.db.utils import IntegrityError
 
@@ -85,7 +87,7 @@ def test_str(self):
 
     def test_repo_abs_path(self, fake_repo_path):
         model = recipes.model.make()
-        path = '%s/%d/models/%d' % (fake_repo_path, model.author.pk, model.pk)
+        path = os.path.join(str(fake_repo_path), str(model.author.pk), 'models', str(model.pk))
 
         assert model.repo._root == path
         assert str(model.repo_abs_path) == path
diff --git a/weblab/experiments/tests/test_models.py b/weblab/experiments/tests/test_models.py
index 38fb720da..1964e122a 100644
--- a/weblab/experiments/tests/test_models.py
+++ b/weblab/experiments/tests/test_models.py
@@ -1,3 +1,4 @@
+import os
 import shutil
 from datetime import date
 from pathlib import Path
@@ -132,11 +133,11 @@ def test_experiment_is_deleted_when_model_is_deleted(self, experiment_with_resul
 class TestExperimentVersion:
     def test_abs_path(self, fake_experiment_path):
         version = recipes.experiment_version.make(id=2)
-        assert str(version.abs_path) == '%s/2' % fake_experiment_path
+        assert str(version.abs_path) == os.path.join(str(fake_experiment_path), '2')
 
     def test_archive_path(self, fake_experiment_path):
         version = recipes.experiment_version.make(id=2)
-        assert str(version.archive_path) == '%s/2/results.omex' % fake_experiment_path
+        assert str(version.archive_path) == os.path.join(str(fake_experiment_path), '2', 'results.omex')
 
     def test_signature(self):
         running = recipes.running_experiment.make()

From a6ca490a893feee262f103e1552619a9e023b967 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Tue, 14 Jan 2020 21:13:39 +0000
Subject: [PATCH 06/10] Make experiments reuse dataset views where applicable

---
 weblab/datasets/views.py                    |  8 +--
 weblab/experiments/models.py                | 13 ++--
 weblab/experiments/tests/test_models.py     |  2 +-
 weblab/experiments/tests/test_processing.py |  6 +-
 weblab/experiments/tests/test_views.py      |  2 +-
 weblab/experiments/views.py                 | 75 ++++-----------------
 6 files changed, 29 insertions(+), 77 deletions(-)

diff --git a/weblab/datasets/views.py b/weblab/datasets/views.py
index 4293b22de..bde7568e2 100644
--- a/weblab/datasets/views.py
+++ b/weblab/datasets/views.py
@@ -17,7 +17,6 @@
     HttpResponseRedirect,
     JsonResponse,
 )
-from django.utils.text import get_valid_filename
 from django.views import View
 from django.views.generic.detail import DetailView, SingleObjectMixin
 from django.views.generic.edit import CreateView, DeleteView, FormMixin
@@ -231,6 +230,9 @@ class DatasetArchiveView(VisibilityMixin, SingleObjectMixin, View):
     """
     model = Dataset
 
+    def get_archive_name(self, dataset):
+        return dataset.archive_name
+
     def get(self, request, *args, **kwargs):
         dataset = self.get_object()
         path = dataset.archive_path
@@ -238,9 +240,7 @@ def get(self, request, *args, **kwargs):
         if not path.exists():
             raise Http404
 
-        zipfile_name = os.path.join(
-            get_valid_filename('%s.zip' % dataset.name)
-        )
+        zipfile_name = self.get_archive_name(dataset)
 
         with path.open('rb') as archive:
             response = HttpResponse(content_type='application/zip')
diff --git a/weblab/experiments/models.py b/weblab/experiments/models.py
index 409d532c0..04ed53a3a 100644
--- a/weblab/experiments/models.py
+++ b/weblab/experiments/models.py
@@ -50,12 +50,13 @@ def get_name(self, model_version=False, proto_version=False):
         :param model_version: Whether to include model version
         :param proto_version: Whether to include protocol version
         """
-        return '{0} / {1}'.format(
-            ('{0}@{1}' if model_version else '{0}').format(
-                self.model.name, self.nice_model_version),
-            ('{0}@{1}' if proto_version else '{0}').format(
-                self.protocol.name, self.nice_protocol_version),
-        )
+        model_part = self.model.name
+        if model_version:
+            model_part += '@' + self.nice_model_version
+        proto_part = self.protocol.name
+        if proto_version:
+            proto_part += '@' + self.nice_protocol_version
+        return '{0} / {1}'.format(model_part, proto_part)
 
     @property
     def visibility(self):
diff --git a/weblab/experiments/tests/test_models.py b/weblab/experiments/tests/test_models.py
index 1964e122a..11209f417 100644
--- a/weblab/experiments/tests/test_models.py
+++ b/weblab/experiments/tests/test_models.py
@@ -137,7 +137,7 @@ def test_abs_path(self, fake_experiment_path):
 
     def test_archive_path(self, fake_experiment_path):
         version = recipes.experiment_version.make(id=2)
-        assert str(version.archive_path) == os.path.join(str(fake_experiment_path), '2', 'results.omex')
+        assert str(version.archive_path) == os.path.join(str(fake_experiment_path), '2', version.archive_name)
 
     def test_signature(self):
         running = recipes.running_experiment.make()
diff --git a/weblab/experiments/tests/test_processing.py b/weblab/experiments/tests/test_processing.py
index d0e2ad14d..1dba8da28 100644
--- a/weblab/experiments/tests/test_processing.py
+++ b/weblab/experiments/tests/test_processing.py
@@ -306,7 +306,7 @@ def test_stores_archive(self, queued_experiment, archive_upload):
 
         assert result == {'experiment': 'ok'}
 
-        assert (queued_experiment.abs_path / 'results.omex').exists()
+        assert queued_experiment.archive_path.exists()
 
     def test_finished_experiment_must_have_attachment(self, queued_experiment):
         result = process_callback({
@@ -371,7 +371,7 @@ def test_overwrites_previous_results(self, queued_experiment, archive_upload):
         assert result == {}
         queued_experiment.refresh_from_db()
         assert queued_experiment.status == 'RUNNING'
-        assert not (queued_experiment.abs_path / 'results.omex').exists()
+        assert not queued_experiment.archive_path.exists()
 
         result = process_callback({
             'signature': queued_experiment.signature,
@@ -384,4 +384,4 @@ def test_overwrites_previous_results(self, queued_experiment, archive_upload):
 
         queued_experiment.refresh_from_db()
         assert queued_experiment.status == 'SUCCESS'
-        assert (queued_experiment.abs_path / 'results.omex').exists()
+        assert queued_experiment.archive_path.exists()
diff --git a/weblab/experiments/tests/test_views.py b/weblab/experiments/tests/test_views.py
index d81d0c63c..04da9d536 100644
--- a/weblab/experiments/tests/test_views.py
+++ b/weblab/experiments/tests/test_views.py
@@ -1323,11 +1323,11 @@ def test_file_json(self, client, archive_file_path, experiment_version):
 class TestExperimentArchiveView:
     def test_download_archive(self, client, experiment_version, archive_file_path):
         experiment_version.mkdir()
-        shutil.copyfile(archive_file_path, str(experiment_version.archive_path))
         experiment_version.experiment.model.name = 'my_model'
         experiment_version.experiment.model.save()
         experiment_version.experiment.protocol.name = 'my_protocol'
         experiment_version.experiment.protocol.save()
+        shutil.copyfile(archive_file_path, str(experiment_version.archive_path))
 
         response = client.get(
             '/experiments/%d/versions/%d/archive' %
diff --git a/weblab/experiments/views.py b/weblab/experiments/views.py
index 76efcca60..ad69076eb 100644
--- a/weblab/experiments/views.py
+++ b/weblab/experiments/views.py
@@ -1,15 +1,9 @@
 import logging
-import mimetypes
-import os.path
 import urllib.parse
 
 from django.contrib import messages
 from django.contrib.admin.views.decorators import staff_member_required
-from django.contrib.auth.mixins import (
-    LoginRequiredMixin,
-    PermissionRequiredMixin,
-    UserPassesTestMixin,
-)
+from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin
 from django.core.urlresolvers import reverse
 from django.db.models import (
     F,
@@ -18,7 +12,7 @@
     Subquery,
 )
 from django.db.models.functions import Coalesce
-from django.http import Http404, HttpResponse, JsonResponse
+from django.http import Http404, JsonResponse
 from django.shortcuts import get_object_or_404, redirect
 from django.utils.decorators import method_decorator
 from django.utils.text import get_valid_filename
@@ -26,10 +20,11 @@
 from django.views.decorators.csrf import csrf_exempt
 from django.views.generic import ListView, TemplateView
 from django.views.generic.detail import DetailView, SingleObjectMixin
-from django.views.generic.edit import DeleteView, FormMixin
+from django.views.generic.edit import FormMixin
 from guardian.shortcuts import get_objects_for_user
 
 from core.visibility import VisibilityMixin
+from datasets import views as dataset_views
 from entities.models import ModelEntity, ProtocolEntity
 from repocache.models import CACHED_VERSION_TYPE_MAP, CachedModelVersion, CachedProtocolVersion
 
@@ -393,33 +388,18 @@ class ExperimentVersionListView(VisibilityMixin, DetailView):
     template_name = 'experiments/experiment_versions.html'
 
 
-class ExperimentDeleteView(UserPassesTestMixin, DeleteView):
+class ExperimentDeleteView(dataset_views.DatasetDeleteView):
     """
     Delete all versions of an experiment
     """
     model = Experiment
-    # Raise a 403 error rather than redirecting to login,
-    # if the user doesn't have delete permissions.
-    raise_exception = True
-
-    def test_func(self):
-        return self.get_object().is_deletable_by(self.request.user)
-
-    def get_success_url(self, *args, **kwargs):
-        return reverse('experiments:list')
 
 
-class ExperimentVersionDeleteView(UserPassesTestMixin, DeleteView):
+class ExperimentVersionDeleteView(dataset_views.DatasetDeleteView):
     """
     Delete a single version of an experiment
     """
     model = ExperimentVersion
-    # Raise a 403 error rather than redirecting to login,
-    # if the user doesn't have delete permissions.
-    raise_exception = True
-
-    def test_func(self):
-        return self.get_object().is_deletable_by(self.request.user)
 
     def get_success_url(self, *args, **kwargs):
         return reverse('experiments:versions', args=[self.get_object().experiment.id])
@@ -584,7 +564,7 @@ class ExperimentVersionJsonView(VisibilityMixin, SingleObjectMixin, View):
     model = ExperimentVersion
 
     def _file_json(self, archive_file):
-        version = self.get_object()
+        version = self.object
         return {
             'id': archive_file.name,
             'author': version.author.full_name,
@@ -600,7 +580,7 @@ def _file_json(self, archive_file):
         }
 
     def get(self, request, *args, **kwargs):
-        version = self.get_object()
+        version = self.object = self.get_object()
         files = [
             self._file_json(f)
             for f in version.files
@@ -627,48 +607,19 @@ def get(self, request, *args, **kwargs):
         })
 
 
-class ExperimentFileDownloadView(VisibilityMixin, SingleObjectMixin, View):
+class ExperimentFileDownloadView(dataset_views.DatasetFileDownloadView):
     """
     Download an individual file from an experiment
     """
     model = ExperimentVersion
 
-    def get(self, request, *args, **kwargs):
-        filename = self.kwargs['filename']
-        version = self.get_object()
-
-        content_type, _ = mimetypes.guess_type(filename)
-        if content_type is None:
-            content_type = 'application/octet-stream'
-
-        with version.open_file(filename) as file_:
-            response = HttpResponse(content_type=content_type)
-            response['Content-Disposition'] = 'attachment; filename=%s' % filename
-            response.write(file_.read())
 
-        return response
-
-
-class ExperimentVersionArchiveView(VisibilityMixin, SingleObjectMixin, View):
+class ExperimentVersionArchiveView(dataset_views.DatasetArchiveView):
     """
     Download a combine archive of an experiment version
     """
     model = ExperimentVersion
 
-    def get(self, request, *args, **kwargs):
-        version = self.get_object()
-        path = version.archive_path
-
-        if not path.exists():
-            raise Http404
-
-        zipfile_name = os.path.join(
-            get_valid_filename('%s.zip' % version.experiment.name)
-        )
-
-        with path.open('rb') as archive:
-            response = HttpResponse(content_type='application/zip')
-            response['Content-Disposition'] = 'attachment; filename=%s' % zipfile_name
-            response.write(archive.read())
-
-        return response
+    def get_archive_name(self, version):
+        """For historical reasons this is different from the archive_name."""
+        return get_valid_filename('%s.zip' % version.experiment.name)

From 8fff5a9eba2498408dd1491f7e5cd205ad7f6a54 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Wed, 15 Jan 2020 13:27:21 +0000
Subject: [PATCH 07/10] Add `FileCollectionMixin.get_json` and use in views

---
 weblab/core/models.py       |  49 +++++++++++++++++
 weblab/datasets/views.py    |  41 ++------------
 weblab/experiments/views.py | 104 +++++++-----------------------------
 3 files changed, 71 insertions(+), 123 deletions(-)

diff --git a/weblab/core/models.py b/weblab/core/models.py
index 2313ca98d..7de04e982 100644
--- a/weblab/core/models.py
+++ b/weblab/core/models.py
@@ -1,4 +1,7 @@
+import urllib.parse
+
 from django.conf import settings
+from django.core.urlresolvers import reverse
 from django.db import models
 from guardian.shortcuts import assign_perm, get_users_with_perms, remove_perm
 
@@ -140,3 +143,49 @@ def mkdir(self):
     def open_file(self, name):
         """Open the given file in the archive for reading."""
         return ArchiveReader(str(self.archive_path)).open_file(name)
+
+    def get_file_json(self, file_, ns, url_args):
+        """Get information about a file in JSON format for use by Javascript code.
+
+        :param core.combine.ArchiveFile file_: the file to provide info about
+        :param str ns: the app namespace to use for reversing download URLs
+        :param list url_args: initial argument(s) for reverse to identify the collection the file is in
+        :return: a dictionary of file metadata
+        """
+        return {
+            'id': file_.name,
+            'author': self.author.full_name,
+            'created': self.created_at,
+            'name': file_.name,
+            'filetype': file_.fmt,
+            'masterFile': file_.is_master,
+            'size': file_.size,
+            'url': reverse(
+                ns + ':file_download',
+                args=url_args + [urllib.parse.quote(file_.name)]
+            )
+        }
+
+    def get_json(self, ns, url_args):
+        """Get information about this collection in JSON format for use by Javascript code.
+
+        :param str ns: the app namespace to use for reversing download URLs
+        :param list url_args: initial argument(s) for reverse to identify this collection
+        :return: a dictionary of collection metadata, including info about each file
+        """
+        files = [
+            self.get_file_json(f, ns, url_args)
+            for f in self.files
+            if f.name not in ['manifest.xml', 'metadata.rdf']
+        ]
+        return {
+            'id': self.id,
+            'author': self.author.full_name,
+            'parsedOk': False,
+            'visibility': self.visibility,
+            'created': self.created_at,
+            'name': self.name,
+            'files': files,
+            'numFiles': len(files),
+            'download_url': reverse(ns + ':archive', args=url_args),
+        }
diff --git a/weblab/datasets/views.py b/weblab/datasets/views.py
index bde7568e2..1be6ee147 100644
--- a/weblab/datasets/views.py
+++ b/weblab/datasets/views.py
@@ -1,6 +1,5 @@
 import mimetypes
 import os.path
-import urllib
 from zipfile import ZipFile
 
 from braces.views import UserFormKwargsMixin
@@ -156,46 +155,12 @@ class DatasetJsonView(VisibilityMixin, SingleObjectMixin, View):
     """
     model = Dataset
 
-    def _file_json(self, archive_file):
-        ns = self.request.resolver_match.namespace
-        dataset = self.object
-        return {
-            'id': archive_file.name,
-            'author': dataset.author.full_name,
-            'created': dataset.created_at,
-            'name': archive_file.name,
-            'filetype': archive_file.fmt,
-            'masterFile': archive_file.is_master,
-            'size': archive_file.size,
-            'url': reverse(
-                ns + ':file_download',
-                args=[dataset.id, urllib.parse.quote(archive_file.name)]
-            )
-        }
-
     def get(self, request, *args, **kwargs):
         ns = self.request.resolver_match.namespace
-        dataset = self.object = self.get_object()
-        files = [
-            self._file_json(f)
-            for f in dataset.files
-            if f.name not in ['manifest.xml', 'metadata.rdf']
-        ]
-
+        dataset = self.get_object()
+        url_args = [dataset.id]
         return JsonResponse({
-            'version': {
-                'id': dataset.id,
-                'author': dataset.author.full_name,
-                'parsedOk': False,
-                'visibility': dataset.visibility,
-                'created': dataset.created_at,
-                'name': dataset.name,
-                'files': files,
-                'numFiles': len(files),
-                'download_url': reverse(
-                    ns + ':archive', args=[dataset.id]
-                ),
-            }
+            'version': dataset.get_json(ns, url_args)
         })
 
 
diff --git a/weblab/experiments/views.py b/weblab/experiments/views.py
index ad69076eb..cb024adb0 100644
--- a/weblab/experiments/views.py
+++ b/weblab/experiments/views.py
@@ -1,5 +1,4 @@
 import logging
-import urllib.parse
 
 from django.contrib import messages
 from django.contrib.admin.views.decorators import staff_member_required
@@ -433,27 +432,6 @@ class ExperimentComparisonJsonView(View):
     """
     Serve up JSON view of multiple experiment versions for comparison
     """
-    def _file_json(self, version, archive_file):
-        """
-        JSON for a single file in the experiment archive
-
-        :param version: ExperimentVersion object
-        :param archive_file: ArchiveFile object
-        """
-        return {
-            'id': archive_file.name,
-            'author': version.author.full_name,
-            'created': version.created_at,
-            'name': archive_file.name,
-            'filetype': archive_file.fmt,
-            'masterFile': archive_file.is_master,
-            'size': archive_file.size,
-            'url': reverse(
-                'experiments:file_download',
-                args=[version.experiment.id, version.id, urllib.parse.quote(archive_file.name)]
-            )
-        }
-
     def _version_json(self, version, model_version_in_name, protocol_version_in_name):
         """
         JSON for a single experiment version
@@ -462,36 +440,21 @@ def _version_json(self, version, model_version_in_name, protocol_version_in_name
         :param model_version_in_name: Whether to include model version specifier in name field
         :param protocol_version_in_name: Whether to include protocol version specifier in name field
         """
-        files = [
-            self._file_json(version, f)
-            for f in version.files
-            if f.name not in ['manifest.xml', 'metadata.rdf']
-        ]
         exp = version.experiment
-        return {
-            'id': version.id,
-            'author': version.author.full_name,
-            'status': version.status,
-            'parsedOk': False,
-            'visibility': version.visibility,
-            'created': version.created_at,
-            'name': version.experiment.get_name(model_version_in_name, protocol_version_in_name),
-            'experimentId': version.experiment.id,
+        ns = self.request.resolver_match.namespace
+        url_args = [exp.id, version.id]
+        details = version.get_json(ns, url_args)
+        details.update({
+            'name': exp.get_name(model_version_in_name, protocol_version_in_name),
+            'url': reverse(ns + ':version', args=url_args),
             'versionId': version.id,
-            'files': files,
-            'numFiles': len(files),
-            'url': reverse(
-                'experiments:version', args=[exp.id, version.id]
-            ),
-            'download_url': reverse(
-                'experiments:archive', args=[exp.id, version.id]
-            ),
             'modelName': exp.model.name,
             'protoName': exp.protocol.name,
-            'modelVersion': exp.model.repo.get_name_for_commit(exp.model_version),
+            'modelVersion': exp.model.repo.get_name_for_commit(exp.model_version),  # TODO #191: Use repocache instead
             'protoVersion': exp.protocol.repo.get_name_for_commit(exp.protocol_version),
             'runNumber': version.run_number,
-        }
+        })
+        return details
 
     def get(self, request, *args, **kwargs):
         pks = {int(pk) for pk in self.kwargs['version_pks'][1:].split('/') if pk}
@@ -563,47 +526,18 @@ class ExperimentVersionJsonView(VisibilityMixin, SingleObjectMixin, View):
     """
     model = ExperimentVersion
 
-    def _file_json(self, archive_file):
-        version = self.object
-        return {
-            'id': archive_file.name,
-            'author': version.author.full_name,
-            'created': version.created_at,
-            'name': archive_file.name,
-            'filetype': archive_file.fmt,
-            'masterFile': archive_file.is_master,
-            'size': archive_file.size,
-            'url': reverse(
-                'experiments:file_download',
-                args=[version.experiment.id, version.id, urllib.parse.quote(archive_file.name)]
-            )
-        }
-
     def get(self, request, *args, **kwargs):
-        version = self.object = self.get_object()
-        files = [
-            self._file_json(f)
-            for f in version.files
-            if f.name not in ['manifest.xml', 'metadata.rdf']
-        ]
-
+        version = self.get_object()
+        ns = self.request.resolver_match.namespace
+        url_args = [version.experiment.id, version.id]
+        details = version.get_json(ns, url_args)
+        details.update({
+            'status': version.status,
+            'version': version.id,
+            'experimentId': version.experiment.id,
+        })
         return JsonResponse({
-            'version': {
-                'id': version.id,
-                'author': version.author.full_name,
-                'status': version.status,
-                'parsedOk': False,
-                'visibility': version.visibility,
-                'created': version.created_at,
-                'name': version.name,
-                'experimentId': version.experiment.id,
-                'version': version.id,
-                'files': files,
-                'numFiles': len(files),
-                'download_url': reverse(
-                    'experiments:archive', args=[version.experiment.id, version.id]
-                ),
-            }
+            'version': details,
         })
 
 

From 92e6146f5fe52fece4f952e4590460580ec0f3b9 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Wed, 15 Jan 2020 14:04:59 +0000
Subject: [PATCH 08/10] Extract JSON producing methods to `Entity`

---
 weblab/entities/models.py           |  49 ++++++++++++
 weblab/entities/tests/test_views.py |   4 +-
 weblab/entities/views.py            | 117 +++++-----------------------
 3 files changed, 71 insertions(+), 99 deletions(-)

diff --git a/weblab/entities/models.py b/weblab/entities/models.py
index 5f8133dc2..43b650182 100644
--- a/weblab/entities/models.py
+++ b/weblab/entities/models.py
@@ -3,10 +3,12 @@
 from pathlib import Path
 
 from django.core.exceptions import ObjectDoesNotExist
+from django.core.urlresolvers import reverse
 from django.core.validators import MinLengthValidator
 from django.db import models
 from guardian.shortcuts import get_objects_for_user
 
+from core.filetypes import get_file_type
 from core.models import UserCreatedModelMixin
 from core.visibility import HELP_TEXT as VIS_HELP_TEXT
 from core.visibility import Visibility, visibility_check
@@ -274,6 +276,53 @@ def is_parsed_ok(self, version):
             ok = self.repocache.get_version(version).parsed_ok
         return ok
 
+    def get_version_json(self, commit, ns):
+        """Get metadata for a particular version of this entity suitable for sending as JSON.
+
+        :param commit: a `Commit` instance (TODO #191 a `CachedEntityVersion` instance)
+        :param str ns: the app namespace to use for reversing download URLs
+        :return: a dictionary of version metadata, including file info
+        """
+        files = [
+            self.get_file_json(commit, f, ns)
+            for f in commit.files
+            if f.name not in ['manifest.xml', 'metadata.rdf']
+        ]
+        return {
+            'id': commit.sha,
+            'entityId': self.id,
+            'author': commit.author.name,
+            'parsedOk': self.is_parsed_ok(commit.sha),
+            'visibility': self.get_version_visibility(commit.sha, default=self.DEFAULT_VISIBILITY),
+            'created': commit.timestamp,
+            'name': self.name,
+            'version': self.repo.get_name_for_commit(commit.sha),  # TODO #191 use repocache instead
+            'files': files,
+            'commitMessage': commit.message,
+            'numFiles': len(files),
+        }
+
+    def get_file_json(self, commit, file_, ns):
+        """Get metadata for a single file within a commit suitable for sending as JSON.
+
+        :param commit: a `Commit` instance (TODO #191 a `CachedEntityVersion` instance)
+        :param git.Blob file_: the file to get metadata for
+        :param str ns: the app namespace to use for reversing download URLs
+        :return: a dictionary of file metadata
+        """
+        return {
+            'id': file_.name,
+            'name': file_.name,
+            'author': commit.author.name,
+            'created': commit.timestamp,
+            'filetype': get_file_type(file_.name),
+            'size': file_.size,
+            'url': reverse(
+                ns + ':file_download',
+                args=[self.url_type, self.id, commit.sha, file_.name]
+            ),
+        }
+
 
 class EntityManager(models.Manager):
     def get_queryset(self):
diff --git a/weblab/entities/tests/test_views.py b/weblab/entities/tests/test_views.py
index 38e855f3a..eb70b79fa 100644
--- a/weblab/entities/tests/test_views.py
+++ b/weblab/entities/tests/test_views.py
@@ -615,7 +615,7 @@ def test_compare_entities(self, client, helpers):
         versions = data['getEntityInfos']['entities']
         assert versions[0]['id'] == v1.sha
         assert versions[1]['id'] == v2.sha
-        assert versions[0]['author'] == model.author.full_name
+        assert versions[0]['author'] == v1.author.name
         assert versions[0]['visibility'] == 'public'
         assert versions[0]['name'] == model.name
         assert versions[0]['version'] == v1.sha
@@ -676,7 +676,7 @@ def test_file_json(self, client, helpers):
         file_ = versions[0]['files'][0]
         assert file_['id'] == filename
         assert file_['name'] == filename
-        assert file_['author'] == model.author.full_name
+        assert file_['author'] == v1.author.name
         assert file_['filetype'] == 'TXTPROTOCOL'
         assert file_['size'] == 15
         assert file_['url'] == (
diff --git a/weblab/entities/views.py b/weblab/entities/views.py
index 12b49a493..52e2a88d6 100644
--- a/weblab/entities/views.py
+++ b/weblab/entities/views.py
@@ -37,7 +37,6 @@
 from git import BadName, GitCommandError
 from guardian.shortcuts import get_objects_for_user
 
-from core.filetypes import get_file_type
 from core.visibility import Visibility, VisibilityMixin
 from experiments.models import Experiment, ExperimentVersion, PlannedExperiment
 from fitting.models import FittingSpec
@@ -197,23 +196,6 @@ def get_context_data(self, **kwargs):
 
 
 class EntityVersionJsonView(EntityTypeMixin, EntityVersionMixin, SingleObjectMixin, View):
-    def _file_json(self, blob):
-        obj = self._get_object()
-        commit = self.get_commit()
-        ns = self.request.resolver_match.namespace
-
-        return {
-            'id': blob.name,
-            'name': blob.name,
-            'filetype': get_file_type(blob.name),
-            'size': blob.size,
-            'created': commit.timestamp,
-            'url': reverse(
-                ns + ':file_download',
-                args=[obj.url_type, obj.id, commit.sha, blob.name]
-            ),
-        }
-
     def _planned_experiments(self):
         obj = self._get_object()
         commit = self.get_commit()
@@ -229,41 +211,29 @@ def get(self, request, *args, **kwargs):
         commit = self.get_commit()
         ns = self.request.resolver_match.namespace
 
-        files = [
-            self._file_json(f)
-            for f in commit.files
-            if f.name not in ['manifest.xml', 'metadata.rdf']
-        ]
         if request.user.has_perm('experiments.create_experiment') and obj.entity_type in ('model', 'protocol'):
             planned_experiments = self._planned_experiments()
         else:
             planned_experiments = []
+
+        details = obj.get_version_json(commit, ns)
+        details.update({
+            'planned_experiments': planned_experiments,
+            'url': reverse(
+                ns + ':version',
+                args=[obj.url_type, obj.id, commit.sha]
+            ),
+            'download_url': reverse(
+                ns + ':entity_archive',
+                args=[obj.url_type, obj.id, commit.sha]
+            ),
+            'change_url': reverse(
+                ns + ':change_visibility',
+                args=[obj.url_type, obj.id, commit.sha]
+            ),
+        })
         return JsonResponse({
-            'version': {
-                'id': commit.sha,
-                'author': commit.author.name,
-                'entityId': obj.id,
-                'visibility': obj.get_version_visibility(commit.sha),
-                'created': commit.timestamp,
-                'name': obj.name,
-                'version': obj.repo.get_name_for_commit(commit.sha),
-                'parsedOk': obj.is_parsed_ok(commit.sha),
-                'files': files,
-                'numFiles': len(files),
-                'planned_experiments': planned_experiments,
-                'url': reverse(
-                    ns + ':version',
-                    args=[obj.url_type, obj.id, commit.sha]
-                ),
-                'download_url': reverse(
-                    ns + ':entity_archive',
-                    args=[obj.url_type, obj.id, commit.sha]
-                ),
-                'change_url': reverse(
-                    ns + ':change_visibility',
-                    args=[obj.url_type, obj.id, commit.sha]
-                ),
-            }
+            'version': details,
         })
 
 
@@ -325,55 +295,8 @@ class EntityComparisonJsonView(EntityTypeMixin, View):
     """
     Serve up JSON view of multiple entity versions for comparison
     """
-    def _file_json(self, entity, commit, blob):
-        """
-        JSON for a single file in a version of the entity
-
-        :param entity: Entity object
-        :param commit: `Commit` object
-        :param blob: `git.Blob` object
-        """
-        ns = self.request.resolver_match.namespace
-        return {
-            'id': blob.name,
-            'name': blob.name,
-            'author': entity.author.full_name,
-            'created': commit.timestamp,
-            'filetype': get_file_type(blob.name),
-            'size': blob.size,
-            'url': reverse(
-                ns + ':file_download',
-                args=[entity.url_type, entity.id, commit.sha, blob.name]
-            ),
-        }
-
-    def _version_json(self, entity, commit):
-        """
-        JSON for a single entity version
-
-        :param entity: Entity object
-        :param commit: `Commit` object
-        """
-        files = [
-            self._file_json(entity, commit, f)
-            for f in commit.files
-            if f.name not in ['manifest.xml', 'metadata.rdf']
-        ]
-        return {
-            'id': commit.sha,
-            'entityId': entity.id,
-            'author': entity.author.full_name,
-            'parsedOk': False,
-            'visibility': entity.get_version_visibility(commit.sha, default=entity.DEFAULT_VISIBILITY),
-            'created': commit.timestamp,
-            'name': entity.name,
-            'version': entity.repo.get_name_for_commit(commit.sha),
-            'files': files,
-            'commitMessage': commit.message,
-            'numFiles': len(files),
-        }
-
     def get(self, request, *args, **kwargs):
+        ns = self.request.resolver_match.namespace
         json_entities = []
         for version in self.kwargs['versions'].strip('/').split('/'):
             id, sha = version.split(':')
@@ -381,7 +304,7 @@ def get(self, request, *args, **kwargs):
                 entity = self.model.objects.get(pk=id)
                 if entity.is_version_visible_to_user(sha, request.user):
                     json_entities.append(
-                        self._version_json(entity, entity.repo.get_commit(sha))
+                        entity.get_version_json(entity.repo.get_commit(sha), ns)
                     )
             except (RepoCacheMiss, Entity.DoesNotExist):
                 pass

From d0bb6b8a41f822936cb1b5b88104f35b21585060 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Wed, 15 Jan 2020 14:34:34 +0000
Subject: [PATCH 09/10] Add another note re #191

---
 weblab/entities/models.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/weblab/entities/models.py b/weblab/entities/models.py
index 43b650182..94c3beda9 100644
--- a/weblab/entities/models.py
+++ b/weblab/entities/models.py
@@ -305,7 +305,10 @@ def get_version_json(self, commit, ns):
     def get_file_json(self, commit, file_, ns):
         """Get metadata for a single file within a commit suitable for sending as JSON.
 
-        :param commit: a `Commit` instance (TODO #191 a `CachedEntityVersion` instance)
+        TODO #191 consider how to replace Commit with CachedEntityVersion here. We'd need
+        to cache the list of file names and sizes.
+
+        :param commit: a `Commit` instance
         :param git.Blob file_: the file to get metadata for
         :param str ns: the app namespace to use for reversing download URLs
         :return: a dictionary of file metadata

From 123001dfc67a3417136f78c38e77a8cfa9d8a01b Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Fri, 17 Jan 2020 16:43:27 +0000
Subject: [PATCH 10/10] Fix entity comparison drop-down select

---
 weblab/entities/models.py           | 4 ++++
 weblab/entities/tests/test_views.py | 1 +
 2 files changed, 5 insertions(+)

diff --git a/weblab/entities/models.py b/weblab/entities/models.py
index 94c3beda9..e375b3c9c 100644
--- a/weblab/entities/models.py
+++ b/weblab/entities/models.py
@@ -300,6 +300,10 @@ def get_version_json(self, commit, ns):
             'files': files,
             'commitMessage': commit.message,
             'numFiles': len(files),
+            'url': reverse(
+                ns + ':version',
+                args=[self.url_type, self.id, commit.sha]
+            ),
         }
 
     def get_file_json(self, commit, file_, ns):
diff --git a/weblab/entities/tests/test_views.py b/weblab/entities/tests/test_views.py
index eb70b79fa..e07649659 100644
--- a/weblab/entities/tests/test_views.py
+++ b/weblab/entities/tests/test_views.py
@@ -621,6 +621,7 @@ def test_compare_entities(self, client, helpers):
         assert versions[0]['version'] == v1.sha
         assert versions[0]['numFiles'] == 1
         assert versions[0]['commitMessage'] == v1.message
+        assert versions[0]['url'] == '/entities/models/%d/versions/%s' % (model.pk, v1.sha)
 
     def test_cannot_compare_entities_with_no_access(self, client, helpers):
         model = recipes.model.make()