From 0d4dc31b2f7971cfc86cf9339f16571bab920f46 Mon Sep 17 00:00:00 2001
From: Jonathan Cooper <j.p.cooper@ucl.ac.uk>
Date: Wed, 15 Jan 2020 16:22:28 +0000
Subject: [PATCH 01/13] Add fitting result related models and migrations

Some models in repocache are now referenced using names to avoid
circular imports
---
 weblab/experiments/models.py                  |  19 ++-
 .../migrations/0002_auto_20200821_1339.py     |  55 ++++++++
 weblab/fitting/models.py                      | 126 +++++++++++++++++-
 weblab/repocache/models.py                    |   8 +-
 4 files changed, 195 insertions(+), 13 deletions(-)
 create mode 100644 weblab/fitting/migrations/0002_auto_20200821_1339.py

diff --git a/weblab/experiments/models.py b/weblab/experiments/models.py
index fcd12f954..20ef68324 100644
--- a/weblab/experiments/models.py
+++ b/weblab/experiments/models.py
@@ -140,33 +140,38 @@ class Runnable(UserCreatedModelMixin, FileCollectionMixin, models.Model):
     )
     return_text = models.TextField(blank=True)
 
-    def __str__(self):
-        return '%s at %s: (%s)' % (self.experiment, self.created_at, self.status)
-
     class Meta:
         indexes = [
             models.Index(fields=['created_at'])
         ]
 
+    def __str__(self):
+        return '%s at %s: (%s)' % (self.parent, self.created_at, self.status)
+
+    @property
+    def parent(self):
+        """E.g. the Experiment this is a version of. Must be defined by subclasses."""
+        raise NotImplementedError
+
     @property
     def name(self):
         return '{:%Y-%m-%d %H:%M:%S}'.format(self.created_at)
 
     @property
     def run_number(self):
-        return self.experiment.versions.filter(created_at__lte=self.created_at).count()
+        return self.parent.versions.filter(created_at__lte=self.created_at).count()
 
     @property
     def is_latest(self):
-        return not self.experiment.versions.filter(created_at__gt=self.created_at).exists()
+        return not self.parent.versions.filter(created_at__gt=self.created_at).exists()
 
     @property
     def visibility(self):
-        return self.experiment.visibility
+        return self.parent.visibility
 
     @property
     def viewers(self):
-        return self.experiment.viewers
+        return self.parent.viewers
 
     @property
     def abs_path(self):
diff --git a/weblab/fitting/migrations/0002_auto_20200821_1339.py b/weblab/fitting/migrations/0002_auto_20200821_1339.py
new file mode 100644
index 000000000..bc850310e
--- /dev/null
+++ b/weblab/fitting/migrations/0002_auto_20200821_1339.py
@@ -0,0 +1,55 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2020-08-21 13:39
+from __future__ import unicode_literals
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('repocache', '0021_auto_20200116_0913'),
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('entities', '0015_auto_20191128_1601'),
+        ('datasets', '0005_auto_20190628_1253'),
+        ('experiments', '0032_auto_20200317_0927'),
+        ('fitting', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='FittingResult',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('created_at', models.DateTimeField(auto_now_add=True)),
+                ('author', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
+                ('dataset', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='fitting_results', to='datasets.Dataset')),
+                ('fittingspec', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='fitting_results', to='fitting.FittingSpec')),
+                ('fittingspec_version', models.ForeignKey(default=None, on_delete=django.db.models.deletion.CASCADE, related_name='fit_ver_fitres', to='repocache.CachedFittingSpecVersion')),
+                ('model', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='model_fitting_results', to='entities.ModelEntity')),
+                ('model_version', models.ForeignKey(default=None, on_delete=django.db.models.deletion.CASCADE, related_name='model_ver_fitres', to='repocache.CachedModelVersion')),
+                ('protocol', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='protocol_fitting_results', to='entities.ProtocolEntity')),
+                ('protocol_version', models.ForeignKey(default=None, on_delete=django.db.models.deletion.CASCADE, related_name='pro_ver_fitres', to='repocache.CachedProtocolVersion')),
+            ],
+            options={
+                'permissions': (('run_fits', 'Can run parameter fitting experiments'),),
+            },
+        ),
+        migrations.CreateModel(
+            name='FittingResultVersion',
+            fields=[
+                ('runnable_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='experiments.Runnable')),
+                ('fittingresult', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='fitting.FittingResult')),
+            ],
+            options={
+                'abstract': False,
+            },
+            bases=('experiments.runnable',),
+        ),
+        migrations.AlterUniqueTogether(
+            name='fittingresult',
+            unique_together=set([('fittingspec', 'dataset', 'model', 'protocol', 'fittingspec_version', 'model_version', 'protocol_version')]),
+        ),
+    ]
diff --git a/weblab/fitting/models.py b/weblab/fitting/models.py
index 71d57ae29..2ba6b16fe 100644
--- a/weblab/fitting/models.py
+++ b/weblab/fitting/models.py
@@ -1,6 +1,15 @@
 from django.db import models
 
-from entities.models import Entity, EntityManager, ProtocolEntity
+from core.models import UserCreatedModelMixin
+from core.visibility import Visibility, get_joint_visibility, visibility_check
+from datasets.models import Dataset
+from entities.models import (
+    Entity,
+    EntityManager,
+    ModelEntity,
+    ProtocolEntity,
+)
+from experiments.models import Runnable
 
 
 class FittingSpec(Entity):
@@ -47,3 +56,118 @@ def remove_collaborator(self, user):
     @property
     def collaborators(self):
         return self.entity_ptr.collaborators
+
+
+class FittingResult(UserCreatedModelMixin, models.Model):
+    """Represents the result of running a parameter fitting experiment.
+
+    This class essentially just stores the links to (particular versions of) a fitting spec,
+    model, protocol, and dataset. The actual results are contained within FittingResultVersion
+    instances, available as .versions, that represent specific runs of the fitting experiment.
+
+    There will only ever be one FittingResult for a given combination of model, protocol,
+    dataset and fitting spec versions.
+
+    TODO: Consider creating a mixin for fields/methods shared with Experiment.
+    """
+    fittingspec = models.ForeignKey(FittingSpec, related_name='fitting_results')
+    dataset = models.ForeignKey(Dataset, related_name='fitting_results')
+    model = models.ForeignKey(ModelEntity, related_name='model_fitting_results')
+    protocol = models.ForeignKey(ProtocolEntity, related_name='protocol_fitting_results')
+
+    # Note that we can't use a ForeignKey here, because versions of models, protocols & fitting
+    # specs are not stored in the DB - they are just commits in the associated git repo.
+    # We could link to the repocache tables, but those aren't guaranteed to exist or be permanent.
+    # So instead we store the full git SHA as a string.
+    fittingspec_version = models.CharField(max_length=50)
+    model_version = models.CharField(max_length=50)
+    protocol_version = models.CharField(max_length=50)
+
+    class Meta:
+        unique_together = ('fittingspec', 'dataset', 'model', 'protocol',
+                           'fittingspec_version', 'model_version', 'protocol_version')
+
+        permissions = (
+            ('run_fits', 'Can run parameter fitting experiments'),
+        )
+
+    def __str__(self):
+        return self.name
+
+    @property
+    def name(self):
+        """There isn't an obvious easy naming for fitting results..."""
+        return 'Fit {} to {} using {}'.format(self.model.name, self.dataset.name, self.fittingspec.name)
+
+    @property
+    def visibility(self):
+        return get_joint_visibility(
+            self.fittingspec.get_version_visibility(
+                self.fittingspec_version,
+                default=self.fittingspec.DEFAULT_VISIBILITY),
+            self.dataset.visibility,
+            self.model.get_version_visibility(
+                self.model_version,
+                default=self.model.DEFAULT_VISIBILITY),
+            self.protocol.get_version_visibility(
+                self.protocol_version,
+                default=self.protocol.DEFAULT_VISIBILITY),
+        )
+
+    @property
+    def viewers(self):
+        """
+        Get users which have special permissions to view this experiment.
+
+        We take the intersection of users with special permissions to view each object
+        (model, fitting spec, etc) involved, if that object is private. If it's public,
+        we can ignore it because everyone can see it.
+
+        :return: `set` of `User` objects
+        """
+        relevant_viewers = []
+        for obj in (self.fittingspec, self.dataset, self.model, self.protocol):
+            if obj.visibility == Visibility.PRIVATE:
+                relevant_viewers.append(obj.viewers)
+        return set.intersection(*relevant_viewers)
+
+    def is_visible_to_user(self, user):
+        """
+        Can the user view the experiment?
+
+        :param user: user to test against
+
+        :returns: True if the user is allowed to view the experiment, False otherwise
+        """
+        return visibility_check(self.visibility, self.viewers, user)
+
+    @property
+    def latest_version(self):
+        return self.versions.latest('created_at')
+
+    @property
+    def nice_model_version(self):
+        """Use tags to give a nicer representation of the commit id"""
+        return self.model.nice_version(self.model_version)
+
+    @property
+    def nice_protocol_version(self):
+        """Use tags to give a nicer representation of the commit id"""
+        return self.protocol.nice_version(self.protocol_version)
+
+    @property
+    def latest_result(self):
+        try:
+            return self.latest_version.status
+        except FittingResultVersion.DoesNotExist:
+            return ''
+
+
+class FittingResultVersion(Runnable):
+    """The results of a single parameter fitting run."""
+    fittingresult = models.ForeignKey(FittingResult, related_name='versions')
+
+    @property
+    def parent(self):
+        """The FittingResult this is a version of."""
+        return self.fittingresult
diff --git a/weblab/repocache/models.py b/weblab/repocache/models.py
index 6549b0a75..6d2ba638b 100644
--- a/weblab/repocache/models.py
+++ b/weblab/repocache/models.py
@@ -3,8 +3,6 @@
 
 from core.models import VisibilityModelMixin
 from core.visibility import Visibility
-from entities.models import ModelEntity, ProtocolEntity
-from fitting.models import FittingSpec
 
 from .exceptions import RepoCacheMiss
 
@@ -211,7 +209,7 @@ def _set_class_links(entity_cache_type, version_cache_type, tag_cache_type):
 
 class CachedModel(CachedEntity):
     """Cache for a CellML model's repository."""
-    entity = models.OneToOneField(ModelEntity, on_delete=models.CASCADE, related_name='cachedmodel')
+    entity = models.OneToOneField('entities.ModelEntity', on_delete=models.CASCADE, related_name='cachedmodel')
 
 
 class CachedModelVersion(CachedEntityVersion):
@@ -230,7 +228,7 @@ class CachedModelTag(CachedEntityTag):
 
 class CachedProtocol(CachedEntity):
     """Cache for a protocol's repository."""
-    entity = models.OneToOneField(ProtocolEntity, on_delete=models.CASCADE, related_name='cachedprotocol')
+    entity = models.OneToOneField('entities.ProtocolEntity', on_delete=models.CASCADE, related_name='cachedprotocol')
 
 
 class CachedProtocolVersion(CachedEntityVersion):
@@ -249,7 +247,7 @@ class CachedProtocolTag(CachedEntityTag):
 
 class CachedFittingSpec(CachedEntity):
     """Cache for a fitting specifications's repository."""
-    entity = models.OneToOneField(FittingSpec, on_delete=models.CASCADE, related_name='cachedfittingspec')
+    entity = models.OneToOneField('fitting.FittingSpec', on_delete=models.CASCADE, related_name='cachedfittingspec')
 
 
 class CachedFittingSpecVersion(CachedEntityVersion):

From 795784d44eec09fc366a0dbf3fd3d24f23599341 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Tue, 25 Aug 2020 11:54:57 +0000
Subject: [PATCH 02/13] Add visibility and naming tests for FittingResult

---
 weblab/conftest.py                  |  34 ++++++
 weblab/core/recipes.py              |  17 +++
 weblab/fitting/models.py            |  32 +++---
 weblab/fitting/tests/test_models.py | 162 ++++++++++++++++++++++++++++
 4 files changed, 226 insertions(+), 19 deletions(-)

diff --git a/weblab/conftest.py b/weblab/conftest.py
index 77197e12e..31ac94748 100644
--- a/weblab/conftest.py
+++ b/weblab/conftest.py
@@ -42,6 +42,13 @@ def add_version(entity,
             populate_entity_cache(entity)
         return commit
 
+    @staticmethod
+    def cached_version(entity, **kwargs):
+        """Add a single commit/version to an entity and return the relevant repocache entry"""
+        assert kwargs.get('cache', True), "Cache must be true for cached version"
+        version = Helpers.add_version(entity, **kwargs)
+        return entity.repocache.get_version(version.sha)
+
     @staticmethod
     def add_fake_version(entity, visibility='private', date=None, message='cache-only commit'):
         """Add a new commit/version only in the cache, not in git."""
@@ -154,6 +161,19 @@ def public_protocol(helpers):
     return protocol
 
 
+@pytest.fixture
+def public_fittingspec(helpers):
+    fittingspec = recipes.fittingspec.make()
+    helpers.add_version(fittingspec, visibility='public')
+    return fittingspec
+
+
+@pytest.fixture
+def public_dataset(helpers):
+    dataset = recipes.dataset.make(visibility='public')
+    return dataset
+
+
 @pytest.fixture
 def moderated_model(helpers):
     model = recipes.model.make()
@@ -331,3 +351,17 @@ def my_dataset_with_file(logged_in_user, helpers, public_protocol, client):
     )
     yield dataset
     dataset.delete()
+
+
+@pytest.fixture
+def fittingresult_version(public_model, public_protocol, public_fittingspec, public_dataset):
+    return recipes.fittingresult_version.make(
+        status='SUCCESS',
+        fittingresult__model=public_model,
+        fittingresult__model_version=public_model.repocache.latest_version,
+        fittingresult__protocol=public_protocol,
+        fittingresult__protocol_version=public_protocol.repocache.latest_version,
+        fittingresult__fittingspec=public_fittingspec,
+        fittingresult__fittingspec_version=public_fittingspec.repocache.latest_version,
+        fittingresult__dataset=public_dataset,
+    )
diff --git a/weblab/core/recipes.py b/weblab/core/recipes.py
index 65d75ca91..1b289b485 100644
--- a/weblab/core/recipes.py
+++ b/weblab/core/recipes.py
@@ -30,6 +30,10 @@
 cached_protocol_version = Recipe('CachedProtocolVersion')
 cached_protocol_tag = Recipe('CachedProtocolTag')
 
+cached_fittingspec = Recipe('CachedFittingSpec')
+cached_fittingspec_version = Recipe('CachedFittingSpecVersion')
+cached_fittingspec_tag = Recipe('CachedFittingSpecTag')
+
 experiment = Recipe(
     'Experiment',
     model=foreign_key(model),
@@ -51,3 +55,16 @@
 
 dataset_file = Recipe('DatasetFile',
                       dataset=foreign_key(dataset))
+
+fittingresult = Recipe(
+    'FittingResult',
+    model=foreign_key(model),
+    model_version=foreign_key(cached_model_version),
+    protocol=foreign_key(protocol),
+    protocol_version=foreign_key(cached_protocol_version),
+    fittingspec=foreign_key(fittingspec),
+    fittingspec_version=foreign_key(cached_fittingspec_version),
+    dataset=foreign_key(dataset),
+)
+
+fittingresult_version = Recipe('FittingResultVersion', fittingresult=foreign_key(fittingresult))
diff --git a/weblab/fitting/models.py b/weblab/fitting/models.py
index 2ba6b16fe..b186ed339 100644
--- a/weblab/fitting/models.py
+++ b/weblab/fitting/models.py
@@ -10,6 +10,7 @@
     ProtocolEntity,
 )
 from experiments.models import Runnable
+from repocache.models import CachedFittingSpecVersion, CachedModelVersion, CachedProtocolVersion
 
 
 class FittingSpec(Entity):
@@ -75,13 +76,12 @@ class FittingResult(UserCreatedModelMixin, models.Model):
     model = models.ForeignKey(ModelEntity, related_name='model_fitting_results')
     protocol = models.ForeignKey(ProtocolEntity, related_name='protocol_fitting_results')
 
-    # Note that we can't use a ForeignKey here, because versions of models, protocols & fitting
-    # specs are not stored in the DB - they are just commits in the associated git repo.
-    # We could link to the repocache tables, but those aren't guaranteed to exist or be permanent.
-    # So instead we store the full git SHA as a string.
-    fittingspec_version = models.CharField(max_length=50)
-    model_version = models.CharField(max_length=50)
-    protocol_version = models.CharField(max_length=50)
+    model_version = models.ForeignKey(CachedModelVersion, default=None, null=False, related_name='model_ver_fitres')
+    protocol_version = models.ForeignKey(CachedProtocolVersion, default=None, null=False, related_name='pro_ver_fitres')
+    fittingspec_version = models.ForeignKey(
+        CachedFittingSpecVersion,
+        default=None, null=False, related_name='fit_ver_fitres',
+    )
 
     class Meta:
         unique_together = ('fittingspec', 'dataset', 'model', 'protocol',
@@ -102,16 +102,10 @@ def name(self):
     @property
     def visibility(self):
         return get_joint_visibility(
-            self.fittingspec.get_version_visibility(
-                self.fittingspec_version,
-                default=self.fittingspec.DEFAULT_VISIBILITY),
+            self.fittingspec_version.visibility,
             self.dataset.visibility,
-            self.model.get_version_visibility(
-                self.model_version,
-                default=self.model.DEFAULT_VISIBILITY),
-            self.protocol.get_version_visibility(
-                self.protocol_version,
-                default=self.protocol.DEFAULT_VISIBILITY),
+            self.model_version.visibility,
+            self.protocol_version.visibility,
         )
 
     @property
@@ -148,18 +142,18 @@ def latest_version(self):
     @property
     def nice_model_version(self):
         """Use tags to give a nicer representation of the commit id"""
-        return self.model.nice_version(self.model_version)
+        return self.model_version.nice_version()
 
     @property
     def nice_protocol_version(self):
         """Use tags to give a nicer representation of the commit id"""
-        return self.protocol.nice_version(self.protocol_version)
+        return self.protocol_version.nice_version()
 
     @property
     def latest_result(self):
         try:
             return self.latest_version.status
-        except FittingResultVersion.DoesNotExist:
+        except Runnable.DoesNotExist:
             return ''
 
 
diff --git a/weblab/fitting/tests/test_models.py b/weblab/fitting/tests/test_models.py
index c43fa884f..fe5f29b50 100644
--- a/weblab/fitting/tests/test_models.py
+++ b/weblab/fitting/tests/test_models.py
@@ -1,10 +1,14 @@
+from datetime import date
+
 import pytest
 from django.db.utils import IntegrityError
 from django.shortcuts import get_object_or_404
 from guardian.shortcuts import assign_perm
 
 from core import recipes
+from datasets.models import Dataset
 from repocache.models import CachedFittingSpec
+from repocache.populate import populate_entity_cache
 
 
 @pytest.mark.django_db
@@ -86,3 +90,161 @@ def test_get_repocache(self):
         assert CachedFittingSpec.objects.count() == 1
         spec.delete()
         assert CachedFittingSpec.objects.count() == 0
+
+
+@pytest.mark.django_db
+class TestFittingResult:
+    def test_name(self, helpers):
+        model = recipes.model.make(name='my model')
+        protocol = recipes.protocol.make(name='my protocol')
+        dataset = recipes.dataset.make(name='my dataset')
+        fittingspec = recipes.fittingspec.make(name='my fitting spec')
+
+        model_version = helpers.add_version(model, tag_name='v1')
+        protocol_version = helpers.add_version(protocol, tag_name='v2')
+        fittingspec_version = helpers.add_version(fittingspec, tag_name='v3')
+
+        fitres = recipes.fittingresult.make(
+            model=model,
+            model_version=model.repocache.get_version(model_version.sha),
+            protocol=protocol,
+            protocol_version=protocol.repocache.get_version(protocol_version.sha),
+            fittingspec=fittingspec,
+            fittingspec_version=fittingspec.repocache.get_version(fittingspec_version.sha),
+            dataset=dataset,
+        )
+
+        assert str(fitres) == fitres.name == 'Fit my model to my dataset using my fitting spec'
+
+    def test_latest_version(self):
+        v1 = recipes.fittingresult_version.make(created_at=date(2017, 1, 2))
+        v2 = recipes.fittingresult_version.make(fittingresult=v1.fittingresult, created_at=date(2017, 1, 3))
+
+        assert v1.fittingresult.latest_version == v2
+        assert not v1.is_latest
+        assert v2.is_latest
+
+    def test_latest_result(self):
+        ver = recipes.fittingresult_version.make(created_at=date(2017, 1, 2), status='FAILED')
+
+        assert ver.fittingresult.latest_result == 'FAILED'
+
+    def test_latest_result_empty_if_no_versions(self):
+        fitres = recipes.fittingresult.make()
+
+        assert fitres.latest_result == ''
+
+    def test_nice_versions(self, fittingresult_version):
+        fitres = fittingresult_version.fittingresult
+
+        assert fitres.nice_model_version == fitres.model.repocache.latest_version.sha[:8] + '...'
+        assert fitres.nice_protocol_version == fitres.protocol.repocache.latest_version.sha[:8] + '...'
+
+        fitres.model.repo.tag('v1')
+        populate_entity_cache(fitres.model)
+        assert fitres.nice_model_version == 'v1'
+
+        fitres.protocol.repo.tag('v2')
+        populate_entity_cache(fitres.protocol)
+
+        assert fitres.nice_protocol_version == 'v2'
+
+    def test_visibility(self, helpers):
+        model = recipes.model.make()
+        protocol = recipes.protocol.make()
+        ds1 = recipes.dataset.make(visibility='private')
+        ds2 = recipes.dataset.make(visibility='public')
+        fittingspec = recipes.fittingspec.make()
+
+        mv1 = helpers.cached_version(model, visibility='private')
+        mv2 = helpers.cached_version(model, visibility='public')
+        pv1 = helpers.cached_version(protocol, visibility='private')
+        pv2 = helpers.cached_version(protocol, visibility='public')
+        fv1 = helpers.cached_version(fittingspec, visibility='private')
+        fv2 = helpers.cached_version(fittingspec, visibility='public')
+
+        # all public
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv2,
+            protocol=protocol, protocol_version=pv2,
+            fittingspec=fittingspec, fittingspec_version=fv2,
+            dataset=ds2,
+        ).visibility == 'public'
+
+        # all private
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv1,
+            protocol=protocol, protocol_version=pv1,
+            fittingspec=fittingspec, fittingspec_version=fv1,
+            dataset=ds1,
+        ).visibility == 'private'
+
+        # model private version => private
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv1,
+            protocol=protocol, protocol_version=pv2,
+            fittingspec=fittingspec, fittingspec_version=fv2,
+            dataset=ds2,
+        ).visibility == 'private'
+
+        # protocol private version => private
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv2,
+            protocol=protocol, protocol_version=pv1,
+            fittingspec=fittingspec, fittingspec_version=fv2,
+            dataset=ds2,
+        ).visibility == 'private'
+
+        # fitting spec private version => private
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv2,
+            protocol=protocol, protocol_version=pv2,
+            fittingspec=fittingspec, fittingspec_version=fv1,
+            dataset=ds2,
+        ).visibility == 'private'
+
+        # dataset private version => private
+        assert recipes.fittingresult.make(
+            model=model, model_version=mv2,
+            protocol=protocol, protocol_version=pv2,
+            fittingspec=fittingspec, fittingspec_version=fv2,
+            dataset=ds1,
+        ).visibility == 'private'
+
+    def test_viewers(self, helpers, user):
+        helpers.add_permission(user, 'create_model')
+        helpers.add_permission(user, 'create_protocol')
+        helpers.add_permission(user, 'create_fittingspec')
+        helpers.add_permission(user, 'create_dataset', model=Dataset)
+
+        model = recipes.model.make()
+        protocol = recipes.protocol.make()
+        fittingspec = recipes.fittingspec.make()
+        # Datasets do not currently support collaborators
+        # (https://github.com/ModellingWebLab/WebLab/issues/247)
+        # so test with a public dataset for now
+        dataset = recipes.dataset.make(visibility='public')
+        mv = helpers.cached_version(model, visibility='private')
+        pv = helpers.cached_version(protocol, visibility='private')
+        fv = helpers.cached_version(fittingspec, visibility='private')
+
+        fr = recipes.fittingresult.make(
+            model=model, model_version=mv,
+            protocol=protocol, protocol_version=pv,
+            fittingspec=fittingspec, fittingspec_version=fv,
+            dataset=dataset,
+        )
+        assert user not in fr.viewers
+        assert not fr.is_visible_to_user(user)
+
+        fr.model.add_collaborator(user)
+        assert user not in fr.viewers
+        assert not fr.is_visible_to_user(user)
+
+        fr.protocol.add_collaborator(user)
+        assert user not in fr.viewers
+        assert not fr.is_visible_to_user(user)
+
+        fr.fittingspec.add_collaborator(user)
+        assert user in fr.viewers
+        assert fr.is_visible_to_user(user)

From d3838bc0591c9666682d245fd7910db7e9202f09 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 10:08:23 +0000
Subject: [PATCH 03/13] Add fitting result version list view

---
 weblab/fitting/models.py                      | 11 +++--
 weblab/fitting/tests/test_models.py           | 17 +++++++
 weblab/fitting/tests/test_views.py            | 12 +++++
 weblab/fitting/urls.py                        |  6 +++
 weblab/fitting/views.py                       | 10 ++++
 .../fitting/fittingresult_versions.html       | 48 +++++++++++++++++++
 .../includes/fittingresult_header.html        | 21 ++++++++
 7 files changed, 120 insertions(+), 5 deletions(-)
 create mode 100644 weblab/fitting/tests/test_views.py
 create mode 100644 weblab/templates/fitting/fittingresult_versions.html
 create mode 100644 weblab/templates/fitting/includes/fittingresult_header.html

diff --git a/weblab/fitting/models.py b/weblab/fitting/models.py
index b186ed339..2bb892d58 100644
--- a/weblab/fitting/models.py
+++ b/weblab/fitting/models.py
@@ -119,11 +119,12 @@ def viewers(self):
 
         :return: `set` of `User` objects
         """
-        relevant_viewers = []
-        for obj in (self.fittingspec, self.dataset, self.model, self.protocol):
-            if obj.visibility == Visibility.PRIVATE:
-                relevant_viewers.append(obj.viewers)
-        return set.intersection(*relevant_viewers)
+        viewers = [
+            obj.viewers
+            for obj in (self.fittingspec, self.dataset, self.model, self.protocol)
+            if obj.visibility == Visibility.PRIVATE
+        ]
+        return set.intersection(*viewers) if viewers else {}
 
     def is_visible_to_user(self, user):
         """
diff --git a/weblab/fitting/tests/test_models.py b/weblab/fitting/tests/test_models.py
index fe5f29b50..3835b3914 100644
--- a/weblab/fitting/tests/test_models.py
+++ b/weblab/fitting/tests/test_models.py
@@ -248,3 +248,20 @@ def test_viewers(self, helpers, user):
         fr.fittingspec.add_collaborator(user)
         assert user in fr.viewers
         assert fr.is_visible_to_user(user)
+
+    def test_viewers_of_public_fittingresult(self, helpers, user):
+        model = recipes.model.make()
+        protocol = recipes.protocol.make()
+        fittingspec = recipes.fittingspec.make()
+        dataset = recipes.dataset.make(visibility='public')
+        mv = helpers.cached_version(model, visibility='public')
+        pv = helpers.cached_version(protocol, visibility='public')
+        fv = helpers.cached_version(fittingspec, visibility='public')
+
+        fr = recipes.fittingresult.make(
+            model=model, model_version=mv,
+            protocol=protocol, protocol_version=pv,
+            fittingspec=fittingspec, fittingspec_version=fv,
+            dataset=dataset,
+        )
+        assert fr.viewers == {}
diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
new file mode 100644
index 000000000..514ef8776
--- /dev/null
+++ b/weblab/fitting/tests/test_views.py
@@ -0,0 +1,12 @@
+import pytest
+
+
+@pytest.mark.django_db
+class TestFittingResultVersionsView:
+    def test_view_fittingresult_versions(self, client, fittingresult_version):
+        response = client.get(
+            ('/fitting/result/%d/versions/' % fittingresult_version.fittingresult.pk)
+        )
+
+        assert response.status_code == 200
+        assert response.context['fittingresult'] == fittingresult_version.fittingresult
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index 2ceb658bc..a7c85f1af 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -132,4 +132,10 @@
         entity_views.EntityDiffView.as_view(),
         name='diff',
     ),
+
+    url(
+        r'^result/(?P<pk>\d+)/versions/$',
+        views.FittingResultVersionListView.as_view(),
+        name='versions',
+    ),
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index 5ce650eac..a4540e50c 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -13,11 +13,14 @@
 from braces.views import UserFormKwargsMixin
 from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin
 from django.urls import reverse
+from django.views.generic.detail import DetailView
 from django.views.generic.edit import CreateView
 
+from core.visibility import VisibilityMixin
 from entities.views import EntityNewVersionView, EntityTypeMixin
 
 from .forms import FittingSpecForm, FittingSpecVersionForm
+from .models import FittingResult
 
 
 class FittingSpecCreateView(
@@ -40,3 +43,10 @@ class FittingSpecNewVersionView(EntityNewVersionView):
     This is almost identical to other entities, except that we can't re-run experiments.
     """
     form_class = FittingSpecVersionForm
+
+
+class FittingResultVersionListView(VisibilityMixin, DetailView):
+    """Show all versions of a fitting result"""
+    model = FittingResult
+    context_object_name = 'fittingresult'
+    template_name = 'fitting/fittingresult_versions.html'
diff --git a/weblab/templates/fitting/fittingresult_versions.html b/weblab/templates/fitting/fittingresult_versions.html
new file mode 100644
index 000000000..807fd3aaf
--- /dev/null
+++ b/weblab/templates/fitting/fittingresult_versions.html
@@ -0,0 +1,48 @@
+{% extends "base.html" %}
+{% load staticfiles %}
+
+{% block title %}Fitting result versions - {% endblock title %}
+
+{% block content %}
+  {% include "./includes/fittingresult_header.html" %}
+
+  <h2>Versions</h2>
+
+  <div id="entityversionlist_content">
+    <ul>
+    {% for version in fittingresult.versions.all|dictsortreversed:"created_at" %}
+    <li>
+      <p class="experiment-{{ version.status }}">
+        <strong>
+          <a class="entityversionlink" href="{#% url 'experiments:version' experiment.id version.id %#}">
+            {{ version.name }}
+          </a>
+        </strong>
+        by <em>{{ version.author.full_name }}</em>
+        <!-- TODO: Add delete version link -->
+        <br />
+        <span class="suppl">
+          <small>created </small> <time>{{ version.created_at }}</time>
+          {% with version.files|length as numfiles %}
+          <small>containing</small> {{ numfiles }} file{{ numfiles|pluralize }}
+          {% endwith %}
+        </span><br />
+        <span class="suppl">
+          {{ version.return_text|safe|linebreaksbr }}
+        </span>
+      </p>
+    </li>
+    {% endfor %}
+    </ul>
+
+    <p id='expversioncolorlegend'>
+      Status Legend:
+      <span class="experiment-QUEUED">queued</span>
+      <span class="experiment-RUNNING">running</span>
+      <span class="experiment-INAPPLICABLE">inapplicable</span>
+      <span class="experiment-FAILED">failed</span>
+      <span class="experiment-PARTIAL">partial failure</span>
+      <span class="experiment-SUCCESS">success</span>
+    </p>
+  </div>
+{% endblock %}
diff --git a/weblab/templates/fitting/includes/fittingresult_header.html b/weblab/templates/fitting/includes/fittingresult_header.html
new file mode 100644
index 000000000..b680d18e8
--- /dev/null
+++ b/weblab/templates/fitting/includes/fittingresult_header.html
@@ -0,0 +1,21 @@
+
+{% load staticfiles %}
+{% load experiments %}
+
+<h1 id="entityname">
+  <small>Fitting result: </small><span>{{ fittingresult.name }}</span>
+</h1>
+
+<div class="suppl">
+  <small>Created <time>{{fittingresult.created_at}}</time> by <em>{{fittingresult.author.full_name}}</em>.
+    {% comment %}
+    {% can_delete_entity fittingresult as can_delete %}
+    {% if can_delete %}
+      Delete fitting result:
+      <a title="delete all versions of this fitting result" href="{% url 'fittingresult:delete' experiment.id %}">
+        <img src="{% static 'img/delete.png' %}" alt="delete all versions of this experiment"
+            title="delete all versions of this experiment"/></a>
+    {% endif %}
+    {% endcomment %}
+  </small>
+</div>

From 7c2e92d697551a05c02ff3665a2eb0ea430f0e25 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 10:08:41 +0000
Subject: [PATCH 04/13] Update gitignore

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 6d1dee062..a2e2b37c7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -53,6 +53,7 @@ nosetests.xml
 coverage.xml
 *.cover
 .hypothesis/
+.pytest_cache/
 
 # Translations
 *.mo

From c73d35c31d00e4b794676d944cd6e003955f23cb Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 10:32:47 +0000
Subject: [PATCH 05/13] Add fitting result version view

---
 weblab/fitting/tests/test_views.py            |  14 +++
 weblab/fitting/urls.py                        |   9 +-
 weblab/fitting/views.py                       |   7 +-
 .../fitting/fittingresult_versions.html       |   2 +-
 .../fitting/fittingresultversion_detail.html  | 115 ++++++++++++++++++
 5 files changed, 144 insertions(+), 3 deletions(-)
 create mode 100644 weblab/templates/fitting/fittingresultversion_detail.html

diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index 514ef8776..a297a754e 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -1,4 +1,5 @@
 import pytest
+from pytest_django.asserts import assertContains, assertTemplateUsed
 
 
 @pytest.mark.django_db
@@ -10,3 +11,16 @@ def test_view_fittingresult_versions(self, client, fittingresult_version):
 
         assert response.status_code == 200
         assert response.context['fittingresult'] == fittingresult_version.fittingresult
+
+
+@pytest.mark.django_db
+class TestFittingResultVersionView:
+    def test_view_fittingresult_version(self, client, fittingresult_version):
+        response = client.get(
+            ('/fitting/result/%d/versions/%d' % (fittingresult_version.fittingresult.pk,
+                                                 fittingresult_version.pk))
+        )
+
+        assert response.context['version'] == fittingresult_version
+        assertTemplateUsed(response, 'fitting/fittingresultversion_detail.html')
+        assertContains(response, 'Download archive of all files')
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index a7c85f1af..7220f4a32 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -136,6 +136,13 @@
     url(
         r'^result/(?P<pk>\d+)/versions/$',
         views.FittingResultVersionListView.as_view(),
-        name='versions',
+        name='result_versions',
     ),
+
+    url(
+        r'^result/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)(?:/%s)?$' % _FILEVIEW,
+        views.FittingResultVersionView.as_view(),
+        name='result_version',
+    ),
+
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index a4540e50c..cd05b6b7f 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -20,7 +20,7 @@
 from entities.views import EntityNewVersionView, EntityTypeMixin
 
 from .forms import FittingSpecForm, FittingSpecVersionForm
-from .models import FittingResult
+from .models import FittingResult, FittingResultVersion
 
 
 class FittingSpecCreateView(
@@ -50,3 +50,8 @@ class FittingResultVersionListView(VisibilityMixin, DetailView):
     model = FittingResult
     context_object_name = 'fittingresult'
     template_name = 'fitting/fittingresult_versions.html'
+
+
+class FittingResultVersionView(VisibilityMixin, DetailView):
+    model = FittingResultVersion
+    context_object_name = 'version'
diff --git a/weblab/templates/fitting/fittingresult_versions.html b/weblab/templates/fitting/fittingresult_versions.html
index 807fd3aaf..258d2d8fa 100644
--- a/weblab/templates/fitting/fittingresult_versions.html
+++ b/weblab/templates/fitting/fittingresult_versions.html
@@ -14,7 +14,7 @@ <h2>Versions</h2>
     <li>
       <p class="experiment-{{ version.status }}">
         <strong>
-          <a class="entityversionlink" href="{#% url 'experiments:version' experiment.id version.id %#}">
+          <a class="entityversionlink" href="{% url 'fitting:result_version' fittingresult.id version.id %}">
             {{ version.name }}
           </a>
         </strong>
diff --git a/weblab/templates/fitting/fittingresultversion_detail.html b/weblab/templates/fitting/fittingresultversion_detail.html
new file mode 100644
index 000000000..dafc6bd8d
--- /dev/null
+++ b/weblab/templates/fitting/fittingresultversion_detail.html
@@ -0,0 +1,115 @@
+{% extends "base.html" %}
+{% load staticfiles %}
+{% load experiments %}
+
+{% block title %}Fitting result details - {% endblock title %}
+
+{% block body_id %}experiment-version{% endblock %}
+
+{% block content %}
+  {% with version.fittingresult as fittingresult %}
+  {% include "./includes/fittingresult_header.html" %}
+
+  <div id="entitydetails"></div>
+
+  <div id="entityversion"
+       data-version-href="{% url 'fitting:result_version' fittingresult.id version.id %}"
+       data-version-json-href="{#% url 'experiments:version_json' experiment.id version.id %#}">
+    <div class="closebtn">
+      <small>
+        <a href="{% url 'fitting:result_versions' fittingresult.id %}">list all versions</a>
+      </small>
+    </div>
+    <h2 id="entityversionname">
+      <small>Version:</small> {{ version.name }}
+    </h2>
+    <div class="suppl">
+      <small>
+        Created <time id="entityversiontime">{{ version.created_at }}</time>
+        by <em id="entityversionauthor">{{ version.author.full_name }}</em>.
+        {% if version.finished_at %}Took {{ version.created_at|timesince:version.finished_at }}.{% endif %}
+        Visibility: <span title="This is derived from the corresponding model version and protocol version visibilities">{{ version.visibility }}</span>
+        <img src="{% static 'img/info.png' %}" alt="help" title="{{ VISIBILITY_HELP }}" />.
+        <span id="exptStatus"></span>
+
+        {% comment %}
+        {% if perms.create_experiment %}
+          <a id="rerunExperiment"><img src="{% static 'img/refresh.png' %}" alt="rerun experiment" title="rerun experiment"/></a> <span id="rerunExperimentAction"></span>
+        {% endif %}
+
+        {% can_delete_entity version as can_delete %}
+        {% if can_delete %}
+          Delete experiment version:
+          <a title="delete this version of this experiment" href="{% url 'experiments:delete_version' experiment.id version.id %}">
+            <img src="{% static 'img/delete.png' %}" alt="delete this version of this experiment"
+                title="delete this version of this experiment"/></a>
+        {% endif %}
+        {% endcomment %}
+
+        <br/>Corresponding model:
+        <a href="{% url 'entities:version' 'model' fittingresult.model.pk fittingresult.model_version.sha %}">{{ fittingresult.model.name }} @ {{ fittingresult.nice_model_version }}</a>
+          &amp; protocol:
+          <a href="{% url 'entities:version' 'protocol' fittingresult.protocol.pk fittingresult.protocol_version.sha %}">{{ fittingresult.protocol.name }} @ {{ fittingresult.nice_protocol_version }}</a>
+
+      </small>
+    </div>
+
+    {% if not version.is_latest %}
+      <p class="info">
+        Note: there is a <a href="{% url 'fitting:result_version' fittingresult.id fittingresult.latest_version.id %}">later version of this fitting</a>.
+      </p>
+    {% endif %}
+
+    <p id="return-text" class="experiment-{{ version.status }}" style="display:none;">
+      {{ version.return_text|safe|linebreaksbr }}
+    </p>
+
+    <div id="running-experiment-note" style="display:none;">
+      <p>
+        This fitting has not yet finished running, and so no result files are available.
+        You can refresh the page to see if it has completed.
+      </p>
+    </div>
+
+    {% comment %}
+    {% if experiment.protocol.protocol_experimental_datasets.exists %}
+    <div id="dataset-link-div">
+      <span>Overlay a dataset linked to this protocol:</span>
+      <select id="dataset-link">
+        <option selected value="">None</option>
+        {% dataset_options experiment %}
+      </select>
+    </div>
+    {% endif %}
+    {% endcomment %}
+
+    <div id="entityversiondetails">
+      <div id="entityversionfiledetails">
+          <div class="closebtn"><small>
+              <a id="exportPlot" style="display: none;">export plot data |</a>
+          <a id="zoomFile">zoom |</a>
+              <a id="entityversionfileclose">&otimes; close</a>
+          </small></div>
+          <h3 id="entityversionfilename"></h3>
+          <div class="suppl"><small>Created <time id="entityversionfiletime"></time> by
+              <em id="entityversionfileauthor"></em>.</small></div>
+          <div id="entityversionfiledisplay"></div>
+      </div>
+
+        <div id="entityversionfiles">
+          <h3>Files attached to this version</h3>
+          <p id="parse_status"></p>
+          <table id="entityversionfilestable">
+          </table>
+          <a id='downloadArchive' title="Download a 'combine' format archive of all the files">
+            <img src="{% static 'img/download.png' %}" alt="Download"
+                title="Download a 'combine' format archive of all the files"/>
+                Download archive of all files
+          </a>
+          <div id="entityversionfilesreadme"></div>
+      </div>
+    </div>
+  </div>
+
+{% endwith %}
+{% endblock %}

From 5fa7fb2bdfb3e5e6ae3a131557d1a7f623f0c49e Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 14:20:50 +0000
Subject: [PATCH 06/13] Add fitting result archive file view

---
 weblab/fitting/tests/test.omex     | Bin 0 -> 953 bytes
 weblab/fitting/tests/test_views.py |  42 +++++++++++++++++++++++++++++
 weblab/fitting/urls.py             |   6 +++++
 weblab/fitting/views.py            |  12 +++++++++
 4 files changed, 60 insertions(+)
 create mode 100644 weblab/fitting/tests/test.omex

diff --git a/weblab/fitting/tests/test.omex b/weblab/fitting/tests/test.omex
new file mode 100644
index 0000000000000000000000000000000000000000..53699e151c051599dbd3792def254cf655cc2615
GIT binary patch
literal 953
zcmWIWW@Zs#U|`^25Y5`|_k~|%(MKT9m5G6Yhe3uRH!&|WEw#8ruOc@mG=!6ZS*!n9
zj1CZ&R&X;gvixFXU;yjhI_aR-5d(p?_qAOD6XwlYQ!#Pr+ee9eUuM=e+$@P=_Eb~Z
zyma5*jq8Fws8!98|My<HEt9{yJn7gJ-PU#cLU%_c7?|8JDE)f+<;I=s`D?FEI{uWi
zN!D=liWZHv6RdxplKCEWo00kUMX{bmCYjFva+p`iT%2-)LA>gk{Dfoinw#`zTnl(`
zNmN;t{||4^^e17LK7U+d%P00jH|d(d;@yu*A|1Kh=Lf6r-;s6JHs#orlY%jBnqTr$
zdbnS|IMB9M{j-C3*q#sdQ3k47$NxlkpZt8yvD>|EuY0|pz=vn=64j0yEX;7}O9)uo
zI{#N#{{4mP?=c3zgRRS_*T>g;u_+%g*aU!>3m9yvMMe2V#d;+bCE!3~&bbhUCD6Qe
zwNB{=89eb~z3djlfH1KaXyTpQ?ia*>rb$ChEG|jOFD-$a$cf#=(<je(ho0~cKB2Fx
zsiFJu)LDJ6){S7daska@VBiNv@drVkIu;-sgjIoV&9BH!ElIS>P0dXzO3X`DHFhk^
z&nZbvPgT%WC@x4$PSr~;E(3c*78vhxm~n5YW5^ZY&B!Fjj4K^Ufb3=frWJ-IjUX0k
zs$zwtDzp@ZY$9e5Bb#UrG#EL+f#!h%9FKXJ0gP;3C(u0P;0Brp3T~|C;R;q{^Bx1u
eBQlVIR)GQ;!zxxbP_VH8VJy%vH(=&uU;qF`{2EvQ

literal 0
HcmV?d00001

diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index a297a754e..4beb31bf8 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -1,7 +1,17 @@
+import shutil
+import zipfile
+from io import BytesIO
+from pathlib import Path
+
 import pytest
 from pytest_django.asserts import assertContains, assertTemplateUsed
 
 
+@pytest.fixture
+def archive_file_path():
+    return str(Path(__file__).absolute().parent.joinpath('./test.omex'))
+
+
 @pytest.mark.django_db
 class TestFittingResultVersionsView:
     def test_view_fittingresult_versions(self, client, fittingresult_version):
@@ -24,3 +34,35 @@ def test_view_fittingresult_version(self, client, fittingresult_version):
         assert response.context['version'] == fittingresult_version
         assertTemplateUsed(response, 'fitting/fittingresultversion_detail.html')
         assertContains(response, 'Download archive of all files')
+
+
+@pytest.mark.django_db
+class TestFittingResultArchiveView:
+    def test_download_archive(self, client, fittingresult_version, archive_file_path):
+        fittingresult_version.mkdir()
+        fittingresult_version.fittingresult.model.name = 'my_model'
+        fittingresult_version.fittingresult.model.save()
+        fittingresult_version.fittingresult.fittingspec.name = 'my_spec'
+        fittingresult_version.fittingresult.fittingspec.save()
+        fittingresult_version.fittingresult.dataset.name = 'my_dataset'
+        fittingresult_version.fittingresult.dataset.save()
+        shutil.copyfile(archive_file_path, str(fittingresult_version.archive_path))
+
+        response = client.get(
+            '/fitting/result/%d/versions/%d/archive' %
+            (fittingresult_version.fittingresult.pk, fittingresult_version.pk)
+        )
+        assert response.status_code == 200
+        archive = zipfile.ZipFile(BytesIO(response.content))
+        assert set(archive.namelist()) == {
+            'stdout.txt', 'errors.txt', 'manifest.xml', 'oxmeta:membrane%3Avoltage - space.csv'}
+        assert response['Content-Disposition'] == (
+            'attachment; filename=Fit_my_model_to_my_dataset_using_my_spec.zip'
+        )
+
+    def test_returns_404_if_no_archive_exists(self, client, fittingresult_version):
+        response = client.get(
+            '/fitting/result/%d/versions/%d/archive' %
+            (fittingresult_version.fittingresult.pk, fittingresult_version.pk)
+        )
+        assert response.status_code == 404
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index 7220f4a32..fb3c20807 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -145,4 +145,10 @@
         name='result_version',
     ),
 
+
+    url(
+        r'^result/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/archive$',
+        views.FittingResultVersionArchiveView.as_view(),
+        name='archive',
+    ),
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index cd05b6b7f..6989c6357 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -13,10 +13,12 @@
 from braces.views import UserFormKwargsMixin
 from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin
 from django.urls import reverse
+from django.utils.text import get_valid_filename
 from django.views.generic.detail import DetailView
 from django.views.generic.edit import CreateView
 
 from core.visibility import VisibilityMixin
+from datasets import views as dataset_views
 from entities.views import EntityNewVersionView, EntityTypeMixin
 
 from .forms import FittingSpecForm, FittingSpecVersionForm
@@ -55,3 +57,13 @@ class FittingResultVersionListView(VisibilityMixin, DetailView):
 class FittingResultVersionView(VisibilityMixin, DetailView):
     model = FittingResultVersion
     context_object_name = 'version'
+
+
+class FittingResultVersionArchiveView(dataset_views.DatasetArchiveView):
+    """
+    Download a combine archive of an experiment version
+    """
+    model = FittingResultVersion
+
+    def get_archive_name(self, version):
+        return get_valid_filename('%s.zip' % version.fittingresult.name)

From 900157e2ae899c106d0471bacf8d6ef1da681152 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 14:45:53 +0000
Subject: [PATCH 07/13] Add fitting result file download view

---
 weblab/fitting/tests/test_views.py | 58 +++++++++++++++++++++++++++---
 weblab/fitting/urls.py             | 13 ++++---
 weblab/fitting/views.py            |  9 ++++-
 3 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index 4beb31bf8..7a1ae7b7e 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -3,6 +3,8 @@
 from io import BytesIO
 from pathlib import Path
 
+from django.core.urlresolvers import reverse
+
 import pytest
 from pytest_django.asserts import assertContains, assertTemplateUsed
 
@@ -16,7 +18,7 @@ def archive_file_path():
 class TestFittingResultVersionsView:
     def test_view_fittingresult_versions(self, client, fittingresult_version):
         response = client.get(
-            ('/fitting/result/%d/versions/' % fittingresult_version.fittingresult.pk)
+            ('/fitting/results/%d/versions/' % fittingresult_version.fittingresult.pk)
         )
 
         assert response.status_code == 200
@@ -27,8 +29,8 @@ def test_view_fittingresult_versions(self, client, fittingresult_version):
 class TestFittingResultVersionView:
     def test_view_fittingresult_version(self, client, fittingresult_version):
         response = client.get(
-            ('/fitting/result/%d/versions/%d' % (fittingresult_version.fittingresult.pk,
-                                                 fittingresult_version.pk))
+            ('/fitting/results/%d/versions/%d' %
+             (fittingresult_version.fittingresult.pk, fittingresult_version.pk))
         )
 
         assert response.context['version'] == fittingresult_version
@@ -49,7 +51,7 @@ def test_download_archive(self, client, fittingresult_version, archive_file_path
         shutil.copyfile(archive_file_path, str(fittingresult_version.archive_path))
 
         response = client.get(
-            '/fitting/result/%d/versions/%d/archive' %
+            '/fitting/results/%d/versions/%d/archive' %
             (fittingresult_version.fittingresult.pk, fittingresult_version.pk)
         )
         assert response.status_code == 200
@@ -62,7 +64,53 @@ def test_download_archive(self, client, fittingresult_version, archive_file_path
 
     def test_returns_404_if_no_archive_exists(self, client, fittingresult_version):
         response = client.get(
-            '/fitting/result/%d/versions/%d/archive' %
+            '/fitting/results/%d/versions/%d/archive' %
             (fittingresult_version.fittingresult.pk, fittingresult_version.pk)
         )
         assert response.status_code == 404
+
+
+@pytest.mark.django_db
+class TestFittingResultFileDownloadView:
+    def test_download_file(self, client, archive_file_path, fittingresult_version):
+        fittingresult_version.mkdir()
+        shutil.copyfile(archive_file_path, str(fittingresult_version.archive_path))
+
+        response = client.get(
+            '/fitting/results/%d/versions/%d/download/stdout.txt' %
+            (fittingresult_version.fittingresult.pk, fittingresult_version.pk)
+        )
+        assert response.status_code == 200
+        assert response.content == b'line of output\nmore output\n'
+        assert response['Content-Disposition'] == (
+            'attachment; filename=stdout.txt'
+        )
+        assert response['Content-Type'] == 'text/plain'
+
+    def test_handles_odd_characters(self, client, archive_file_path, fittingresult_version):
+        fittingresult_version.mkdir()
+        shutil.copyfile(archive_file_path, str(fittingresult_version.archive_path))
+        filename = 'oxmeta:membrane%3Avoltage - space.csv'
+
+        response = client.get(
+            reverse('fitting:file_download',
+                    args=[fittingresult_version.fittingresult.pk, fittingresult_version.pk, filename])
+        )
+
+        assert response.status_code == 200
+        assert response.content == b'1,1\n'
+        assert response['Content-Disposition'] == (
+            'attachment; filename=' + filename
+        )
+        assert response['Content-Type'] == 'text/csv'
+
+    def test_disallows_non_local_files(self, client, archive_file_path, fittingresult_version):
+        fittingresult_version.mkdir()
+        shutil.copyfile(archive_file_path, str(fittingresult_version.archive_path))
+
+        for filename in ['/etc/passwd', '../../../pytest.ini']:
+            response = client.get(
+                '/fitting/results/%d/versions/%d/download/%s' % (
+                    fittingresult_version.fittingresult.pk, fittingresult_version.pk, filename)
+            )
+            assert response.status_code == 404
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index fb3c20807..80660c4c8 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -134,21 +134,26 @@
     ),
 
     url(
-        r'^result/(?P<pk>\d+)/versions/$',
+        r'^results/(?P<pk>\d+)/versions/$',
         views.FittingResultVersionListView.as_view(),
         name='result_versions',
     ),
 
     url(
-        r'^result/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)(?:/%s)?$' % _FILEVIEW,
+        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)(?:/%s)?$' % _FILEVIEW,
         views.FittingResultVersionView.as_view(),
         name='result_version',
     ),
 
-
     url(
-        r'^result/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/archive$',
+        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/archive$',
         views.FittingResultVersionArchiveView.as_view(),
         name='archive',
     ),
+
+    url(
+        r'^results/(?P<experiment_pk>\d+)/versions/(?P<pk>\d+)/download/%s$' % _FILENAME,
+        views.FittingResultFileDownloadView.as_view(),
+        name='file_download',
+    ),
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index 6989c6357..fdff6e504 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -61,9 +61,16 @@ class FittingResultVersionView(VisibilityMixin, DetailView):
 
 class FittingResultVersionArchiveView(dataset_views.DatasetArchiveView):
     """
-    Download a combine archive of an experiment version
+    Download a combine archive of a fitting result version
     """
     model = FittingResultVersion
 
     def get_archive_name(self, version):
         return get_valid_filename('%s.zip' % version.fittingresult.name)
+
+
+class FittingResultFileDownloadView(dataset_views.DatasetFileDownloadView):
+    """
+    Download an individual file from a fitting result
+    """
+    model = FittingResultVersion

From 67f61a0b05028bf296500525b6cbdf84e2e1f532 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Wed, 26 Aug 2020 15:34:32 +0000
Subject: [PATCH 08/13] Add FittingResult version json view

---
 weblab/fitting/tests/test_views.py            | 62 ++++++++++++++++++-
 weblab/fitting/urls.py                        |  6 ++
 weblab/fitting/views.py                       | 25 +++++++-
 .../fitting/fittingresultversion_detail.html  |  2 +-
 4 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index 7a1ae7b7e..2f637bf9d 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -1,11 +1,12 @@
+import json
 import shutil
 import zipfile
 from io import BytesIO
 from pathlib import Path
 
-from django.core.urlresolvers import reverse
-
 import pytest
+from django.core.urlresolvers import reverse
+from django.utils.dateparse import parse_datetime
 from pytest_django.asserts import assertContains, assertTemplateUsed
 
 
@@ -114,3 +115,60 @@ def test_disallows_non_local_files(self, client, archive_file_path, fittingresul
                     fittingresult_version.fittingresult.pk, fittingresult_version.pk, filename)
             )
             assert response.status_code == 404
+
+
+@pytest.mark.django_db
+class TestFittingResultVersionJsonView:
+    def test_fittingresult_json(self, client, logged_in_user, fittingresult_version):
+        version = fittingresult_version
+
+        version.author.full_name = 'test user'
+        version.author.save()
+        version.status = 'SUCCESS'
+
+        response = client.get(
+            ('/fitting/results/%d/versions/%d/files.json' % (version.fittingresult.pk, version.pk))
+        )
+
+        assert response.status_code == 200
+        data = json.loads(response.content.decode())
+        ver = data['version']
+        assert ver['id'] == version.pk
+        assert ver['author'] == 'test user'
+        assert ver['status'] == 'SUCCESS'
+        assert ver['visibility'] == 'public'
+        assert (
+            parse_datetime(ver['created']).replace(microsecond=0) ==
+            version.created_at.replace(microsecond=0)
+        )
+        assert ver['name'] == '{:%Y-%m-%d %H:%M:%S}'.format(version.created_at)
+        assert ver['fittingResultId'] == version.fittingresult.id
+        assert ver['version'] == version.id
+        assert ver['files'] == []
+        assert ver['numFiles'] == 0
+        assert ver['download_url'] == (
+            '/fitting/results/%d/versions/%d/archive' % (version.fittingresult.pk, version.pk)
+        )
+
+    def test_file_json(self, client, archive_file_path, fittingresult_version):
+        version = fittingresult_version
+        version.author.full_name = 'test user'
+        version.author.save()
+        version.mkdir()
+        shutil.copyfile(archive_file_path, str(version.archive_path))
+
+        response = client.get(
+            ('/fitting/results/%d/versions/%d/files.json' % (version.fittingresult.pk, version.pk))
+        )
+
+        assert response.status_code == 200
+        data = json.loads(response.content.decode())
+        file1 = data['version']['files'][0]
+        assert file1['author'] == 'test user'
+        assert file1['name'] == 'stdout.txt'
+        assert file1['filetype'] == 'http://purl.org/NET/mediatypes/text/plain'
+        assert not file1['masterFile']
+        assert file1['size'] == 27
+        assert file1['url'] == (
+            '/fitting/results/%d/versions/%d/download/stdout.txt' % (version.fittingresult.pk, version.pk)
+        )
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index 80660c4c8..3e826a7fe 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -156,4 +156,10 @@
         views.FittingResultFileDownloadView.as_view(),
         name='file_download',
     ),
+
+    url(
+        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/files.json$',
+        views.FittingResultVersionJsonView.as_view(),
+        name='result_version_json',
+    ),
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index fdff6e504..cf79d47d5 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -12,9 +12,11 @@
 
 from braces.views import UserFormKwargsMixin
 from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin
+from django.http import JsonResponse
 from django.urls import reverse
 from django.utils.text import get_valid_filename
-from django.views.generic.detail import DetailView
+from django.views import View
+from django.views.generic.detail import DetailView, SingleObjectMixin
 from django.views.generic.edit import CreateView
 
 from core.visibility import VisibilityMixin
@@ -74,3 +76,24 @@ class FittingResultFileDownloadView(dataset_views.DatasetFileDownloadView):
     Download an individual file from a fitting result
     """
     model = FittingResultVersion
+
+
+class FittingResultVersionJsonView(VisibilityMixin, SingleObjectMixin, View):
+    """
+    Serve up json view of a fitting result verson
+    """
+    model = FittingResultVersion
+
+    def get(self, request, *args, **kwargs):
+        version = self.get_object()
+        ns = self.request.resolver_match.namespace
+        url_args = [version.fittingresult.id, version.id]
+        details = version.get_json(ns, url_args)
+        details.update({
+            'status': version.status,
+            'version': version.id,
+            'fittingResultId': version.fittingresult.id,
+        })
+        return JsonResponse({
+            'version': details,
+        })
diff --git a/weblab/templates/fitting/fittingresultversion_detail.html b/weblab/templates/fitting/fittingresultversion_detail.html
index dafc6bd8d..022f5bbf6 100644
--- a/weblab/templates/fitting/fittingresultversion_detail.html
+++ b/weblab/templates/fitting/fittingresultversion_detail.html
@@ -14,7 +14,7 @@
 
   <div id="entityversion"
        data-version-href="{% url 'fitting:result_version' fittingresult.id version.id %}"
-       data-version-json-href="{#% url 'experiments:version_json' experiment.id version.id %#}">
+       data-version-json-href="{% url 'fitting:result_version_json' fittingresult.id version.id %}">
     <div class="closebtn">
       <small>
         <a href="{% url 'fitting:result_versions' fittingresult.id %}">list all versions</a>

From 1c236c394ec383dfea938a6cebd701616065fc7f Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Thu, 27 Aug 2020 09:32:59 +0000
Subject: [PATCH 09/13] Add fitting result deletion views

---
 weblab/conftest.py                 | 15 +++++++
 weblab/fitting/tests/test_views.py | 70 ++++++++++++++++++++++++++++++
 weblab/fitting/urls.py             | 15 ++++++-
 weblab/fitting/views.py            | 20 +++++++++
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/weblab/conftest.py b/weblab/conftest.py
index 31ac94748..f4500bb7b 100644
--- a/weblab/conftest.py
+++ b/weblab/conftest.py
@@ -365,3 +365,18 @@ def fittingresult_version(public_model, public_protocol, public_fittingspec, pub
         fittingresult__fittingspec_version=public_fittingspec.repocache.latest_version,
         fittingresult__dataset=public_dataset,
     )
+
+
+@pytest.fixture
+def fittingresult_with_result(model_with_version, protocol_with_version):
+    version = recipes.fittingresult_version.make(
+        status='SUCCESS',
+        fittingrseult__model=model_with_version,
+        fittingrseult__model_version=model_with_version.repocache.latest_version,
+        fittingrseult__protocol=protocol_with_version,
+        fittingrseult__protocol_version=protocol_with_version.repocache.latest_version,
+    )
+    version.mkdir()
+    with (version.abs_path / 'result.txt').open('w') as f:
+        f.write('fitting results')
+    return version
diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index 2f637bf9d..6c0903dba 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -9,6 +9,8 @@
 from django.utils.dateparse import parse_datetime
 from pytest_django.asserts import assertContains, assertTemplateUsed
 
+from fitting.models import FittingResult, FittingResultVersion
+
 
 @pytest.fixture
 def archive_file_path():
@@ -172,3 +174,71 @@ def test_file_json(self, client, archive_file_path, fittingresult_version):
         assert file1['url'] == (
             '/fitting/results/%d/versions/%d/download/stdout.txt' % (version.fittingresult.pk, version.pk)
         )
+
+
+@pytest.mark.django_db
+class TestFittingResultDeletion:
+    def test_owner_can_delete_fittingresult(
+        self, logged_in_user, client, fittingresult_with_result
+    ):
+        fittingresult = fittingresult_with_result.fittingresult
+        fittingresult.author = logged_in_user
+        fittingresult.save()
+        exp_ver_path = fittingresult_with_result.abs_path
+        assert FittingResult.objects.filter(pk=fittingresult.pk).exists()
+
+        response = client.post('/fitting/results/%d/delete' % fittingresult.pk)
+
+        assert response.status_code == 302
+        assert response.url == '/experiments/?show_fits=true'
+
+        assert not FittingResult.objects.filter(pk=fittingresult.pk).exists()
+        assert not exp_ver_path.exists()
+
+    @pytest.mark.usefixtures('logged_in_user')
+    def test_non_owner_cannot_delete_fittingresult(
+        self, other_user, client, fittingresult_with_result
+    ):
+        fittingresult = fittingresult_with_result.fittingresult
+        fittingresult.author = other_user
+        fittingresult.save()
+        exp_ver_path = fittingresult_with_result.abs_path
+
+        response = client.post('/fitting/results/%d/delete' % fittingresult.pk)
+
+        assert response.status_code == 403
+        assert FittingResult.objects.filter(pk=fittingresult.pk).exists()
+        assert exp_ver_path.exists()
+
+    def test_owner_can_delete_fittingresult_version(
+        self, logged_in_user, client, fittingresult_with_result
+    ):
+        fittingresult = fittingresult_with_result.fittingresult
+        fittingresult_with_result.author = logged_in_user
+        fittingresult_with_result.save()
+        exp_ver_path = fittingresult_with_result.abs_path
+
+        response = client.post('/fitting/results/%d/versions/%d/delete' % (fittingresult.pk, fittingresult_with_result.pk))
+
+        assert response.status_code == 302
+        assert response.url == '/fitting/results/%d/versions/' % fittingresult.pk
+
+        assert not FittingResultVersion.objects.filter(pk=fittingresult_with_result.pk).exists()
+        assert not exp_ver_path.exists()
+        assert FittingResult.objects.filter(pk=fittingresult.pk).exists()
+
+    @pytest.mark.usefixtures('logged_in_user')
+    def test_non_owner_cannot_delete_fittingresult_version(
+        self, other_user, client, fittingresult_with_result
+    ):
+        fittingresult = fittingresult_with_result.fittingresult
+        fittingresult_with_result.author = other_user
+        fittingresult_with_result.save()
+        exp_ver_path = fittingresult_with_result.abs_path
+
+        response = client.post('/fitting/results/%d/versions/%d/delete' % (fittingresult.pk, fittingresult_with_result.pk))
+
+        assert response.status_code == 403
+        assert FittingResultVersion.objects.filter(pk=fittingresult_with_result.pk).exists()
+        assert FittingResult.objects.filter(pk=fittingresult.pk).exists()
+        assert exp_ver_path.exists()
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index 3e826a7fe..705925b43 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -152,7 +152,7 @@
     ),
 
     url(
-        r'^results/(?P<experiment_pk>\d+)/versions/(?P<pk>\d+)/download/%s$' % _FILENAME,
+        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/download/%s$' % _FILENAME,
         views.FittingResultFileDownloadView.as_view(),
         name='file_download',
     ),
@@ -162,4 +162,17 @@
         views.FittingResultVersionJsonView.as_view(),
         name='result_version_json',
     ),
+
+    url(
+        r'^results/(?P<pk>\d+)/delete$',
+        views.FittingResultDeleteView.as_view(),
+        name='result_delete',
+    ),
+
+    url(
+        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/delete$',
+        views.FittingResultVersionDeleteView.as_view(),
+        name='result_delete_version',
+    ),
+
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index cf79d47d5..f25d13ae8 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -97,3 +97,23 @@ def get(self, request, *args, **kwargs):
         return JsonResponse({
             'version': details,
         })
+
+
+class FittingResultDeleteView(dataset_views.DatasetDeleteView):
+    """
+    Delete all versions of a fitting result
+    """
+    model = FittingResult
+
+    def get_success_url(self, *args, **kwargs):
+        return reverse('experiments:list') + '?show_fits=true'
+
+
+class FittingResultVersionDeleteView(dataset_views.DatasetDeleteView):
+    """
+    Delete a single version of a fitting result
+    """
+    model = FittingResultVersion
+
+    def get_success_url(self, *args, **kwargs):
+        return reverse('fitting:result_versions', args=[self.get_object().fittingresult.id])

From a9163f3d249e25a1b593424d6cc3e0cf2557ae52 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Thu, 27 Aug 2020 11:22:28 +0000
Subject: [PATCH 10/13] Refactor URLs for fitting results

Fitting result URLs get their own subnamespace within the fitting app
---
 weblab/fitting/tests/test_views.py            | 10 ++-
 weblab/fitting/urls.py                        | 88 ++++++++++---------
 weblab/fitting/views.py                       |  2 +-
 .../fitting/fittingresult_versions.html       |  2 +-
 .../fitting/fittingresultversion_detail.html  |  8 +-
 5 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/weblab/fitting/tests/test_views.py b/weblab/fitting/tests/test_views.py
index 6c0903dba..5c232b5c7 100644
--- a/weblab/fitting/tests/test_views.py
+++ b/weblab/fitting/tests/test_views.py
@@ -96,7 +96,7 @@ def test_handles_odd_characters(self, client, archive_file_path, fittingresult_v
         filename = 'oxmeta:membrane%3Avoltage - space.csv'
 
         response = client.get(
-            reverse('fitting:file_download',
+            reverse('fitting:result:file_download',
                     args=[fittingresult_version.fittingresult.pk, fittingresult_version.pk, filename])
         )
 
@@ -218,7 +218,9 @@ def test_owner_can_delete_fittingresult_version(
         fittingresult_with_result.save()
         exp_ver_path = fittingresult_with_result.abs_path
 
-        response = client.post('/fitting/results/%d/versions/%d/delete' % (fittingresult.pk, fittingresult_with_result.pk))
+        response = client.post(
+            '/fitting/results/%d/versions/%d/delete' %
+            (fittingresult.pk, fittingresult_with_result.pk))
 
         assert response.status_code == 302
         assert response.url == '/fitting/results/%d/versions/' % fittingresult.pk
@@ -236,7 +238,9 @@ def test_non_owner_cannot_delete_fittingresult_version(
         fittingresult_with_result.save()
         exp_ver_path = fittingresult_with_result.abs_path
 
-        response = client.post('/fitting/results/%d/versions/%d/delete' % (fittingresult.pk, fittingresult_with_result.pk))
+        response = client.post(
+            '/fitting/results/%d/versions/%d/delete' %
+            (fittingresult.pk, fittingresult_with_result.pk))
 
         assert response.status_code == 403
         assert FittingResultVersion.objects.filter(pk=fittingresult_with_result.pk).exists()
diff --git a/weblab/fitting/urls.py b/weblab/fitting/urls.py
index 705925b43..1e1c22eb3 100644
--- a/weblab/fitting/urls.py
+++ b/weblab/fitting/urls.py
@@ -1,4 +1,4 @@
-from django.conf.urls import url
+from django.conf.urls import include, url
 
 from entities import views as entity_views
 
@@ -11,6 +11,51 @@
 _FILEVIEW = r'%s/(?P<viz>\w+)' % _FILENAME
 _ENTITY_TYPE = '(?P<entity_type>%s)s' % FittingSpec.url_type
 
+
+result_patterns = [
+    url(
+        r'^(?P<pk>\d+)/versions/$',
+        views.FittingResultVersionListView.as_view(),
+        name='versions',
+    ),
+
+    url(
+        r'^(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)(?:/%s)?$' % _FILEVIEW,
+        views.FittingResultVersionView.as_view(),
+        name='version',
+    ),
+
+    url(
+        r'^(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/archive$',
+        views.FittingResultVersionArchiveView.as_view(),
+        name='archive',
+    ),
+
+    url(
+        r'^(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/download/%s$' % _FILENAME,
+        views.FittingResultFileDownloadView.as_view(),
+        name='file_download',
+    ),
+
+    url(
+        r'^(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/files.json$',
+        views.FittingResultVersionJsonView.as_view(),
+        name='version_json',
+    ),
+
+    url(
+        r'^(?P<pk>\d+)/delete$',
+        views.FittingResultDeleteView.as_view(),
+        name='delete',
+    ),
+
+    url(
+        r'^(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/delete$',
+        views.FittingResultVersionDeleteView.as_view(),
+        name='delete_version',
+    ),
+]
+
 urlpatterns = [
     url(
         r'^%s/$' % _ENTITY_TYPE,
@@ -134,45 +179,6 @@
     ),
 
     url(
-        r'^results/(?P<pk>\d+)/versions/$',
-        views.FittingResultVersionListView.as_view(),
-        name='result_versions',
-    ),
-
-    url(
-        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)(?:/%s)?$' % _FILEVIEW,
-        views.FittingResultVersionView.as_view(),
-        name='result_version',
-    ),
-
-    url(
-        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/archive$',
-        views.FittingResultVersionArchiveView.as_view(),
-        name='archive',
+        r'^results/', include(result_patterns, namespace='result')
     ),
-
-    url(
-        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/download/%s$' % _FILENAME,
-        views.FittingResultFileDownloadView.as_view(),
-        name='file_download',
-    ),
-
-    url(
-        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/files.json$',
-        views.FittingResultVersionJsonView.as_view(),
-        name='result_version_json',
-    ),
-
-    url(
-        r'^results/(?P<pk>\d+)/delete$',
-        views.FittingResultDeleteView.as_view(),
-        name='result_delete',
-    ),
-
-    url(
-        r'^results/(?P<fittingresult_pk>\d+)/versions/(?P<pk>\d+)/delete$',
-        views.FittingResultVersionDeleteView.as_view(),
-        name='result_delete_version',
-    ),
-
 ]
diff --git a/weblab/fitting/views.py b/weblab/fitting/views.py
index f25d13ae8..42c8c42a6 100644
--- a/weblab/fitting/views.py
+++ b/weblab/fitting/views.py
@@ -116,4 +116,4 @@ class FittingResultVersionDeleteView(dataset_views.DatasetDeleteView):
     model = FittingResultVersion
 
     def get_success_url(self, *args, **kwargs):
-        return reverse('fitting:result_versions', args=[self.get_object().fittingresult.id])
+        return reverse('fitting:result:versions', args=[self.get_object().fittingresult.id])
diff --git a/weblab/templates/fitting/fittingresult_versions.html b/weblab/templates/fitting/fittingresult_versions.html
index 258d2d8fa..83830fd99 100644
--- a/weblab/templates/fitting/fittingresult_versions.html
+++ b/weblab/templates/fitting/fittingresult_versions.html
@@ -14,7 +14,7 @@ <h2>Versions</h2>
     <li>
       <p class="experiment-{{ version.status }}">
         <strong>
-          <a class="entityversionlink" href="{% url 'fitting:result_version' fittingresult.id version.id %}">
+          <a class="entityversionlink" href="{% url 'fitting:result:version' fittingresult.id version.id %}">
             {{ version.name }}
           </a>
         </strong>
diff --git a/weblab/templates/fitting/fittingresultversion_detail.html b/weblab/templates/fitting/fittingresultversion_detail.html
index 022f5bbf6..9f5944df4 100644
--- a/weblab/templates/fitting/fittingresultversion_detail.html
+++ b/weblab/templates/fitting/fittingresultversion_detail.html
@@ -13,11 +13,11 @@
   <div id="entitydetails"></div>
 
   <div id="entityversion"
-       data-version-href="{% url 'fitting:result_version' fittingresult.id version.id %}"
-       data-version-json-href="{% url 'fitting:result_version_json' fittingresult.id version.id %}">
+       data-version-href="{% url 'fitting:result:version' fittingresult.id version.id %}"
+       data-version-json-href="{% url 'fitting:result:version_json' fittingresult.id version.id %}">
     <div class="closebtn">
       <small>
-        <a href="{% url 'fitting:result_versions' fittingresult.id %}">list all versions</a>
+        <a href="{% url 'fitting:result:versions' fittingresult.id %}">list all versions</a>
       </small>
     </div>
     <h2 id="entityversionname">
@@ -56,7 +56,7 @@ <h2 id="entityversionname">
 
     {% if not version.is_latest %}
       <p class="info">
-        Note: there is a <a href="{% url 'fitting:result_version' fittingresult.id fittingresult.latest_version.id %}">later version of this fitting</a>.
+        Note: there is a <a href="{% url 'fitting:result:version' fittingresult.id fittingresult.latest_version.id %}">later version of this fitting</a>.
       </p>
     {% endif %}
 

From eae6895d5cea77d428729a846c819f6e6d8542bd Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Thu, 27 Aug 2020 11:59:45 +0000
Subject: [PATCH 11/13] Uncomment deletion links

---
 .../fitting/fittingresultversion_detail.html       | 14 +++++---------
 .../fitting/includes/fittingresult_header.html     |  8 +++-----
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/weblab/templates/fitting/fittingresultversion_detail.html b/weblab/templates/fitting/fittingresultversion_detail.html
index 9f5944df4..f29359c70 100644
--- a/weblab/templates/fitting/fittingresultversion_detail.html
+++ b/weblab/templates/fitting/fittingresultversion_detail.html
@@ -32,19 +32,17 @@ <h2 id="entityversionname">
         <img src="{% static 'img/info.png' %}" alt="help" title="{{ VISIBILITY_HELP }}" />.
         <span id="exptStatus"></span>
 
-        {% comment %}
         {% if perms.create_experiment %}
           <a id="rerunExperiment"><img src="{% static 'img/refresh.png' %}" alt="rerun experiment" title="rerun experiment"/></a> <span id="rerunExperimentAction"></span>
         {% endif %}
 
         {% can_delete_entity version as can_delete %}
         {% if can_delete %}
-          Delete experiment version:
-          <a title="delete this version of this experiment" href="{% url 'experiments:delete_version' experiment.id version.id %}">
-            <img src="{% static 'img/delete.png' %}" alt="delete this version of this experiment"
-                title="delete this version of this experiment"/></a>
+          Delete fitting result version:
+          <a title="delete this version of this fitting result" href="{% url 'fitting:result:delete_version' fittingresult.id version.id %}">
+            <img src="{% static 'img/delete.png' %}" alt="delete this version of this fitting result"
+                title="delete this version of this fitting result"/></a>
         {% endif %}
-        {% endcomment %}
 
         <br/>Corresponding model:
         <a href="{% url 'entities:version' 'model' fittingresult.model.pk fittingresult.model_version.sha %}">{{ fittingresult.model.name }} @ {{ fittingresult.nice_model_version }}</a>
@@ -71,8 +69,7 @@ <h2 id="entityversionname">
       </p>
     </div>
 
-    {% comment %}
-    {% if experiment.protocol.protocol_experimental_datasets.exists %}
+    {% if fittingresult.protocol.protocol_experimental_datasets.exists %}
     <div id="dataset-link-div">
       <span>Overlay a dataset linked to this protocol:</span>
       <select id="dataset-link">
@@ -81,7 +78,6 @@ <h2 id="entityversionname">
       </select>
     </div>
     {% endif %}
-    {% endcomment %}
 
     <div id="entityversiondetails">
       <div id="entityversionfiledetails">
diff --git a/weblab/templates/fitting/includes/fittingresult_header.html b/weblab/templates/fitting/includes/fittingresult_header.html
index b680d18e8..0a3aff632 100644
--- a/weblab/templates/fitting/includes/fittingresult_header.html
+++ b/weblab/templates/fitting/includes/fittingresult_header.html
@@ -8,14 +8,12 @@ <h1 id="entityname">
 
 <div class="suppl">
   <small>Created <time>{{fittingresult.created_at}}</time> by <em>{{fittingresult.author.full_name}}</em>.
-    {% comment %}
     {% can_delete_entity fittingresult as can_delete %}
     {% if can_delete %}
       Delete fitting result:
-      <a title="delete all versions of this fitting result" href="{% url 'fittingresult:delete' experiment.id %}">
-        <img src="{% static 'img/delete.png' %}" alt="delete all versions of this experiment"
-            title="delete all versions of this experiment"/></a>
+      <a title="delete all versions of this fitting result" href="{% url 'fitting:result:delete' fittingresult.id %}">
+        <img src="{% static 'img/delete.png' %}" alt="delete all versions of this fitting result"
+            title="delete all versions of this fitting result"/></a>
     {% endif %}
-    {% endcomment %}
   </small>
 </div>

From 288c29b0675318e3b79ab7add40a44fdd95d073e Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Thu, 27 Aug 2020 15:50:06 +0000
Subject: [PATCH 12/13] Fitting result submission to backend

---
 weblab/conftest.py                      |  15 +-
 weblab/fitting/processing.py            | 155 +++++++++++++++
 weblab/fitting/tests/test_processing.py | 241 ++++++++++++++++++++++++
 3 files changed, 407 insertions(+), 4 deletions(-)
 create mode 100644 weblab/fitting/processing.py
 create mode 100644 weblab/fitting/tests/test_processing.py

diff --git a/weblab/conftest.py b/weblab/conftest.py
index f4500bb7b..0cff5a104 100644
--- a/weblab/conftest.py
+++ b/weblab/conftest.py
@@ -147,6 +147,13 @@ def protocol_with_version():
     return protocol
 
 
+@pytest.fixture
+def fittingspec_with_version():
+    fittingspec = recipes.fittingspec.make()
+    Helpers.add_version(fittingspec, visibility='private')
+    return fittingspec
+
+
 @pytest.fixture
 def public_model(helpers):
     model = recipes.model.make()
@@ -371,10 +378,10 @@ def fittingresult_version(public_model, public_protocol, public_fittingspec, pub
 def fittingresult_with_result(model_with_version, protocol_with_version):
     version = recipes.fittingresult_version.make(
         status='SUCCESS',
-        fittingrseult__model=model_with_version,
-        fittingrseult__model_version=model_with_version.repocache.latest_version,
-        fittingrseult__protocol=protocol_with_version,
-        fittingrseult__protocol_version=protocol_with_version.repocache.latest_version,
+        fittingresult__model=model_with_version,
+        fittingresult__model_version=model_with_version.repocache.latest_version,
+        fittingresult__protocol=protocol_with_version,
+        fittingresult__protocol_version=protocol_with_version.repocache.latest_version,
     )
     version.mkdir()
     with (version.abs_path / 'result.txt').open('w') as f:
diff --git a/weblab/fitting/processing.py b/weblab/fitting/processing.py
new file mode 100644
index 000000000..5193e06ca
--- /dev/null
+++ b/weblab/fitting/processing.py
@@ -0,0 +1,155 @@
+import logging
+from urllib.parse import urljoin
+
+import requests
+from django.conf import settings
+from django.core.exceptions import MultipleObjectsReturned
+from django.core.urlresolvers import reverse
+
+from experiments.models import Runnable, RunningExperiment
+
+from .models import FittingResult, FittingResultVersion
+
+
+logger = logging.getLogger(__name__)
+
+
+class ChasteProcessingStatus:
+    RUNNING = "running"
+    SUCCESS = "success"
+    PARTIAL = "partial"
+    FAILED = "failed"
+    INAPPLICABLE = "inapplicable"
+
+    MODEL_STATUSES = {
+        SUCCESS: Runnable.STATUS_SUCCESS,
+        RUNNING: Runnable.STATUS_RUNNING,
+        PARTIAL: Runnable.STATUS_PARTIAL,
+        FAILED: Runnable.STATUS_FAILED,
+        INAPPLICABLE: Runnable.STATUS_INAPPLICABLE,
+    }
+
+    @classmethod
+    def get_model_status(cls, status):
+        return cls.MODEL_STATUSES.get(status)
+
+
+class ProcessingException(Exception):
+    pass
+
+
+def submit_fitting(
+    model, model_version,
+    protocol, protocol_version,
+    fittingspec, fittingspec_version,
+    dataset, user, rerun_ok,
+):
+    """Submit a Celery task to run a fitting
+
+    @param rerun_ok  if False and a FittingResultVersion already exists, will just return that.
+        Otherwise will create a new version of the fitting result.
+    @return the FittingResultVersion for the run
+    """
+    fittingresult, _ = FittingResult.objects.get_or_create(
+        model=model,
+        protocol=protocol,
+        fittingspec=fittingspec,
+        dataset=dataset,
+        model_version=model.repocache.get_version(model_version),
+        protocol_version=protocol.repocache.get_version(protocol_version),
+        fittingspec_version=fittingspec.repocache.get_version(fittingspec_version),
+        defaults={
+            'author': user,
+        }
+    )
+
+    # Check there isn't an existing version if we're not allowed to re-run
+    if not rerun_ok:
+        try:
+            version, created = FittingResultVersion.objects.get_or_create(
+                fittingresult=fittingresult,
+                defaults={
+                    'author': user,
+                }
+            )
+        except MultipleObjectsReturned:
+            return FittingResultVersion.objects.filter(fittingresult=fittingresult).latest('created_at'), False
+        if not created:
+            return version, False
+    else:
+        version = FittingResultVersion.objects.create(
+            fittingresult=fittingresult,
+            author=user,
+        )
+
+    run = RunningExperiment.objects.create(runnable=version)
+    signature = version.signature
+
+    model_url = reverse(
+        'entities:entity_archive',
+        args=['model', model.pk, model_version]
+    )
+    protocol_url = reverse(
+        'entities:entity_archive',
+        args=['protocol', protocol.pk, protocol_version]
+    )
+    fittingspec_url = reverse(
+        'fitting:entity_archive',
+        args=['spec', fittingspec.pk, fittingspec_version]
+    )
+    dataset_url = reverse(
+        'datasets:archive',
+        args=[dataset.pk]
+    )
+    body = {
+        'model': urljoin(settings.CALLBACK_BASE_URL, model_url),
+        'protocol': urljoin(settings.CALLBACK_BASE_URL, protocol_url),
+        'fittingSpec': urljoin(settings.CALLBACK_BASE_URL, fittingspec_url),
+        'dataset': urljoin(settings.CALLBACK_BASE_URL, dataset_url),
+        'signature': signature,
+        'callBack': urljoin(settings.CALLBACK_BASE_URL, reverse('experiments:callback')),
+        'user': user.full_name,
+        'password': settings.CHASTE_PASSWORD,
+        'isAdmin': user.is_staff,
+    }
+    if protocol.is_fitting_spec:
+        body['dataset'] = body['fittingSpec'] = body['protocol']
+
+    try:
+        response = requests.post(settings.CHASTE_URL, body)
+    except requests.exceptions.ConnectionError:
+        run.delete()
+        version.status = Runnable.STATUS_FAILED
+        version.return_text = 'Unable to connect to experiment runner service'
+        version.save()
+        logger.exception(version.return_text)
+        return version, True
+
+    res = response.content.decode().strip()
+    logger.debug('Response from chaste backend: %s' % res)
+
+    if not res.startswith(signature):
+        run.delete()
+        version.status = Runnable.STATUS_FAILED
+        version.return_text = 'Chaste backend answered with something unexpected: %s' % res
+        version.save()
+        logger.error(version.return_text)
+        raise ProcessingException(res)
+
+    status = res[len(signature):].strip()
+
+    if status.startswith('succ'):
+        run.task_id = status[4:].strip()
+        run.save()
+    elif status == 'inapplicable':
+        run.delete()
+        version.status = Runnable.STATUS_INAPPLICABLE
+    else:
+        run.delete()
+        logger.error('Chaste backend answered with error: %s' % status)
+        version.status = Runnable.STATUS_FAILED
+        version.return_text = status
+
+    version.save()
+
+    return version, True
diff --git a/weblab/fitting/tests/test_processing.py b/weblab/fitting/tests/test_processing.py
new file mode 100644
index 000000000..fd97a55aa
--- /dev/null
+++ b/weblab/fitting/tests/test_processing.py
@@ -0,0 +1,241 @@
+from pathlib import Path
+from unittest.mock import Mock, patch
+
+import pytest
+from django.conf import settings
+from django.core.files.uploadedfile import SimpleUploadedFile
+
+from core import recipes
+from experiments.models import RunningExperiment
+from fitting.models import FittingResult, FittingResultVersion
+from fitting.processing import ProcessingException, submit_fitting
+
+
+def generate_response(template='%s succ celery-task-id', field='signature'):
+    def mock_submit(url, body):
+        return Mock(content=(template % body[field]).encode())
+    return mock_submit
+
+
+@pytest.fixture
+def archive_file_path():
+    return str(Path(__file__).absolute().parent.joinpath('./test.omex'))
+
+
+@pytest.fixture
+def archive_upload(archive_file_path):
+    with open(archive_file_path, 'rb') as fp:
+        return SimpleUploadedFile('test.omex', fp.read())
+
+
+@patch('requests.post', side_effect=generate_response())
+@pytest.mark.django_db
+class TestSubmitFitting:
+    def test_creates_new_fittingresult_and_side_effects(
+            self, mock_post,
+            user, model_with_version, protocol_with_version,
+            fittingspec_with_version, public_dataset
+    ):
+        model = model_with_version
+        protocol = protocol_with_version
+        fittingspec = fittingspec_with_version
+        dataset = public_dataset
+        model_version = model.repo.latest_commit.sha
+        protocol_version = protocol.repo.latest_commit.sha
+        fittingspec_version = fittingspec.repo.latest_commit.sha
+
+        assert FittingResult.objects.count() == 0
+        assert RunningExperiment.objects.count() == 0
+
+        version, is_new = submit_fitting(
+            model,
+            model_version,
+            protocol,
+            protocol_version,
+            fittingspec,
+            fittingspec_version,
+            dataset,
+            user,
+            False
+        )
+        assert is_new
+
+        # Check properties of the new fitting result & version
+        assert FittingResult.objects.count() == 1
+        assert version.fittingresult.model == model
+        assert version.fittingresult.protocol == protocol
+        assert version.fittingresult.fittingspec == fittingspec
+        assert version.fittingresult.dataset == dataset
+        assert version.author == user
+        assert version.fittingresult.model_version.sha == model_version
+        assert version.fittingresult.protocol_version.sha == protocol_version
+        assert version.fittingresult.fittingspec_version.sha == fittingspec_version
+        assert version.fittingresult.author == user
+        assert version.status == FittingResultVersion.STATUS_QUEUED
+
+        # Check it did submit to the webservice
+        model_url = '/entities/models/%d/versions/%s/archive' % (model.pk, model_version)
+        protocol_url = (
+            '/entities/protocols/%d/versions/%s/archive' %
+            (protocol.pk, protocol_version))
+        fittingspec_url = (
+            '/fitting/specs/%d/versions/%s/archive' %
+            (fittingspec.pk, fittingspec_version))
+        dataset_url = '/datasets/%d/archive' % (dataset.pk)
+
+        assert mock_post.call_count == 1
+        assert mock_post.call_args[0][0] == settings.CHASTE_URL
+        assert mock_post.call_args[0][1] == {
+            'model': settings.CALLBACK_BASE_URL + model_url,
+            'protocol': settings.CALLBACK_BASE_URL + protocol_url,
+            'fittingSpec': settings.CALLBACK_BASE_URL + fittingspec_url,
+            'dataset': settings.CALLBACK_BASE_URL + dataset_url,
+            'signature': str(version.running.first().id),
+            'callBack': settings.CALLBACK_BASE_URL + '/experiments/callback',
+            'user': 'Test User',
+            'isAdmin': False,
+            'password': settings.CHASTE_PASSWORD,
+        }
+
+        # Check running fitting record
+        assert RunningExperiment.objects.count() == 1
+        assert version.running.count() == 1
+        assert version.running.first().task_id == 'celery-task-id'
+
+        # Check the run is cancelled when we delete the fitting result version
+        # We check indirect deletion - this should cascade to everything
+        mock_post.side_effect = generate_response(field='cancelTask')
+        model.delete()
+        assert FittingResult.objects.count() == 0
+        assert FittingResultVersion.objects.count() == 0
+        assert RunningExperiment.objects.count() == 0
+        assert mock_post.call_count == 2
+        assert mock_post.call_args[0][0] == settings.CHASTE_URL
+        assert mock_post.call_args[0][1] == {
+            'cancelTask': 'celery-task-id',
+            'password': settings.CHASTE_PASSWORD,
+        }
+
+    def test_uses_existing_fittingresult(
+        self, mock_post, user, model_with_version,
+        protocol_with_version, fittingspec_with_version, public_dataset,
+    ):
+        model = model_with_version
+        protocol = protocol_with_version
+        fittingspec = fittingspec_with_version
+        dataset = public_dataset
+        model_version = model.repocache.latest_version
+        protocol_version = protocol.repocache.latest_version
+        fittingspec_version = fittingspec.repocache.latest_version
+
+        fittingresult = recipes.fittingresult.make(
+            model=model, model_version=model_version,
+            protocol=protocol, protocol_version=protocol_version,
+            fittingspec=fittingspec, fittingspec_version=fittingspec_version,
+            dataset=dataset,
+        )
+
+        version, is_new = submit_fitting(
+            fittingresult.model,
+            fittingresult.model_version.sha,
+            fittingresult.protocol,
+            fittingresult.protocol_version.sha,
+            fittingresult.fittingspec,
+            fittingresult.fittingspec_version.sha,
+            fittingresult.dataset,
+            user,
+            False,
+        )
+
+        assert is_new
+        assert version.fittingresult == fittingresult
+
+    def test_raises_exception_on_webservice_error(
+        self, mock_post, user, model_with_version, protocol_with_version,
+        fittingspec_with_version, public_dataset
+    ):
+        model = model_with_version
+        protocol = protocol_with_version
+        fittingspec = fittingspec_with_version
+        dataset = public_dataset
+        model_version = model.repocache.latest_version
+        protocol_version = protocol.repocache.latest_version
+        fittingspec_version = fittingspec.repocache.latest_version
+
+        mock_post.side_effect = generate_response('something %s')
+        with pytest.raises(ProcessingException):
+            submit_fitting(
+                model, model_version.sha,
+                protocol, protocol_version.sha,
+                fittingspec, fittingspec_version.sha,
+                dataset,
+                user,
+                False
+            )
+
+        # There should be no running fitting
+        assert RunningExperiment.objects.count() == 0
+
+        # It should still record a failed fittingresult version however
+        assert FittingResultVersion.objects.count() == 1
+        version = FittingResultVersion.objects.first()
+        assert version.running.count() == 0
+        assert version.fittingresult.model == model
+        assert version.fittingresult.protocol == protocol
+        assert version.status == FittingResultVersion.STATUS_FAILED
+        assert version.return_text.startswith('Chaste backend answered with something unexpected:')
+
+    def test_records_submission_error(
+        self, mock_post, user, model_with_version, protocol_with_version,
+        fittingspec_with_version, public_dataset
+    ):
+        model = model_with_version
+        protocol = protocol_with_version
+        fittingspec = fittingspec_with_version
+        dataset = public_dataset
+        model_version = model.repocache.latest_version
+        protocol_version = protocol.repocache.latest_version
+        fittingspec_version = fittingspec.repocache.latest_version
+
+        mock_post.side_effect = generate_response('%s an error occurred')
+
+        version, is_new = submit_fitting(
+            model, model_version.sha,
+            protocol, protocol_version.sha,
+            fittingspec, fittingspec_version.sha,
+            dataset,
+            user,
+            False
+        )
+
+        assert is_new
+        assert version.status == FittingResultVersion.STATUS_FAILED
+        assert version.return_text == 'an error occurred'
+        assert RunningExperiment.objects.count() == 0
+
+    def test_records_inapplicable_result(
+        self, mock_post, user, model_with_version, protocol_with_version,
+        fittingspec_with_version, public_dataset
+    ):
+        model = model_with_version
+        protocol = protocol_with_version
+        fittingspec = fittingspec_with_version
+        dataset = public_dataset
+        model_version = model.repocache.latest_version
+        protocol_version = protocol.repocache.latest_version
+        fittingspec_version = fittingspec.repocache.latest_version
+
+        mock_post.side_effect = generate_response('%s inapplicable')
+
+        version, is_new = submit_fitting(
+            model, model_version.sha,
+            protocol, protocol_version.sha,
+            fittingspec, fittingspec_version.sha,
+            dataset,
+            user,
+            False
+        )
+
+        assert is_new
+        assert version.status == FittingResultVersion.STATUS_INAPPLICABLE
+        assert RunningExperiment.objects.count() == 0

From 5df21fe2bd6753639086af976132ded60540f833 Mon Sep 17 00:00:00 2001
From: Helen Sherwood-Taylor <helen@rrdlabs.co.uk>
Date: Sun, 30 Aug 2020 09:19:40 +0000
Subject: [PATCH 13/13] Refactor experiment/fitting submission common code

---
 weblab/experiments/processing.py        | 101 +++++++++++++-----------
 weblab/fitting/processing.py            |  76 +-----------------
 weblab/fitting/tests/test_processing.py |   3 +-
 3 files changed, 57 insertions(+), 123 deletions(-)

diff --git a/weblab/experiments/processing.py b/weblab/experiments/processing.py
index 40c8c62f9..2f27367a4 100644
--- a/weblab/experiments/processing.py
+++ b/weblab/experiments/processing.py
@@ -44,6 +44,58 @@ class ProcessingException(Exception):
     pass
 
 
+def submit_runnable(runnable, body, user):
+    run = RunningExperiment.objects.create(runnable=runnable)
+    signature = runnable.signature
+
+    body.update({
+        'signature': runnable.signature,
+        'callBack': urljoin(settings.CALLBACK_BASE_URL, reverse('experiments:callback')),
+        'user': user.full_name,
+        'password': settings.CHASTE_PASSWORD,
+        'isAdmin': user.is_staff,
+    })
+
+    try:
+        response = requests.post(settings.CHASTE_URL, body)
+    except requests.exceptions.ConnectionError:
+        run.delete()
+        runnable.status = Runnable.STATUS_FAILED
+        runnable.return_text = 'Unable to connect to experiment runner service'
+        runnable.save()
+        logger.exception(runnable.return_text)
+        return runnable, True
+
+    res = response.content.decode().strip()
+    logger.debug('Response from chaste backend: %s' % res)
+
+    if not res.startswith(signature):
+        run.delete()
+        runnable.status = Runnable.STATUS_FAILED
+        runnable.return_text = 'Chaste backend answered with something unexpected: %s' % res
+        runnable.save()
+        logger.error(runnable.return_text)
+        raise ProcessingException(res)
+
+    status = res[len(signature):].strip()
+
+    if status.startswith('succ'):
+        run.task_id = status[4:].strip()
+        run.save()
+    elif status == 'inapplicable':
+        run.delete()
+        runnable.status = Runnable.STATUS_INAPPLICABLE
+    else:
+        run.delete()
+        logger.error('Chaste backend answered with error: %s' % status)
+        runnable.status = Runnable.STATUS_FAILED
+        runnable.return_text = status
+
+    runnable.save()
+
+    return runnable, True
+
+
 def submit_experiment(model, model_version, protocol, protocol_version, user, rerun_ok):
     """Submit a Celery task to run an experiment.
 
@@ -80,9 +132,6 @@ def submit_experiment(model, model_version, protocol, protocol_version, user, re
             author=user,
         )
 
-    run = RunningExperiment.objects.create(runnable=version)
-    signature = version.signature
-
     model_url = reverse(
         'entities:entity_archive',
         args=['model', model.pk, model_version]
@@ -94,53 +143,9 @@ def submit_experiment(model, model_version, protocol, protocol_version, user, re
     body = {
         'model': urljoin(settings.CALLBACK_BASE_URL, model_url),
         'protocol': urljoin(settings.CALLBACK_BASE_URL, protocol_url),
-        'signature': signature,
-        'callBack': urljoin(settings.CALLBACK_BASE_URL, reverse('experiments:callback')),
-        'user': user.full_name,
-        'password': settings.CHASTE_PASSWORD,
-        'isAdmin': user.is_staff,
     }
-    if protocol.is_fitting_spec:
-        body['dataset'] = body['fittingSpec'] = body['protocol']
-
-    try:
-        response = requests.post(settings.CHASTE_URL, body)
-    except requests.exceptions.ConnectionError:
-        run.delete()
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = 'Unable to connect to experiment runner service'
-        version.save()
-        logger.exception(version.return_text)
-        return version, True
-
-    res = response.content.decode().strip()
-    logger.debug('Response from chaste backend: %s' % res)
-
-    if not res.startswith(signature):
-        run.delete()
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = 'Chaste backend answered with something unexpected: %s' % res
-        version.save()
-        logger.error(version.return_text)
-        raise ProcessingException(res)
-
-    status = res[len(signature):].strip()
-
-    if status.startswith('succ'):
-        run.task_id = status[4:].strip()
-        run.save()
-    elif status == 'inapplicable':
-        run.delete()
-        version.status = Runnable.STATUS_INAPPLICABLE
-    else:
-        run.delete()
-        logger.error('Chaste backend answered with error: %s' % status)
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = status
-
-    version.save()
 
-    return version, True
+    return submit_runnable(version, body, user)
 
 
 def cancel_experiment(task_id):
diff --git a/weblab/fitting/processing.py b/weblab/fitting/processing.py
index 5193e06ca..56574ccf9 100644
--- a/weblab/fitting/processing.py
+++ b/weblab/fitting/processing.py
@@ -1,12 +1,11 @@
 import logging
 from urllib.parse import urljoin
 
-import requests
 from django.conf import settings
 from django.core.exceptions import MultipleObjectsReturned
 from django.core.urlresolvers import reverse
 
-from experiments.models import Runnable, RunningExperiment
+from experiments.processing import submit_runnable
 
 from .models import FittingResult, FittingResultVersion
 
@@ -14,30 +13,6 @@
 logger = logging.getLogger(__name__)
 
 
-class ChasteProcessingStatus:
-    RUNNING = "running"
-    SUCCESS = "success"
-    PARTIAL = "partial"
-    FAILED = "failed"
-    INAPPLICABLE = "inapplicable"
-
-    MODEL_STATUSES = {
-        SUCCESS: Runnable.STATUS_SUCCESS,
-        RUNNING: Runnable.STATUS_RUNNING,
-        PARTIAL: Runnable.STATUS_PARTIAL,
-        FAILED: Runnable.STATUS_FAILED,
-        INAPPLICABLE: Runnable.STATUS_INAPPLICABLE,
-    }
-
-    @classmethod
-    def get_model_status(cls, status):
-        return cls.MODEL_STATUSES.get(status)
-
-
-class ProcessingException(Exception):
-    pass
-
-
 def submit_fitting(
     model, model_version,
     protocol, protocol_version,
@@ -82,9 +57,6 @@ def submit_fitting(
             author=user,
         )
 
-    run = RunningExperiment.objects.create(runnable=version)
-    signature = version.signature
-
     model_url = reverse(
         'entities:entity_archive',
         args=['model', model.pk, model_version]
@@ -106,50 +78,6 @@ def submit_fitting(
         'protocol': urljoin(settings.CALLBACK_BASE_URL, protocol_url),
         'fittingSpec': urljoin(settings.CALLBACK_BASE_URL, fittingspec_url),
         'dataset': urljoin(settings.CALLBACK_BASE_URL, dataset_url),
-        'signature': signature,
-        'callBack': urljoin(settings.CALLBACK_BASE_URL, reverse('experiments:callback')),
-        'user': user.full_name,
-        'password': settings.CHASTE_PASSWORD,
-        'isAdmin': user.is_staff,
     }
-    if protocol.is_fitting_spec:
-        body['dataset'] = body['fittingSpec'] = body['protocol']
-
-    try:
-        response = requests.post(settings.CHASTE_URL, body)
-    except requests.exceptions.ConnectionError:
-        run.delete()
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = 'Unable to connect to experiment runner service'
-        version.save()
-        logger.exception(version.return_text)
-        return version, True
-
-    res = response.content.decode().strip()
-    logger.debug('Response from chaste backend: %s' % res)
-
-    if not res.startswith(signature):
-        run.delete()
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = 'Chaste backend answered with something unexpected: %s' % res
-        version.save()
-        logger.error(version.return_text)
-        raise ProcessingException(res)
-
-    status = res[len(signature):].strip()
-
-    if status.startswith('succ'):
-        run.task_id = status[4:].strip()
-        run.save()
-    elif status == 'inapplicable':
-        run.delete()
-        version.status = Runnable.STATUS_INAPPLICABLE
-    else:
-        run.delete()
-        logger.error('Chaste backend answered with error: %s' % status)
-        version.status = Runnable.STATUS_FAILED
-        version.return_text = status
-
-    version.save()
 
-    return version, True
+    return submit_runnable(version, body, user)
diff --git a/weblab/fitting/tests/test_processing.py b/weblab/fitting/tests/test_processing.py
index fd97a55aa..b4b6f7928 100644
--- a/weblab/fitting/tests/test_processing.py
+++ b/weblab/fitting/tests/test_processing.py
@@ -7,8 +7,9 @@
 
 from core import recipes
 from experiments.models import RunningExperiment
+from experiments.processing import ProcessingException
 from fitting.models import FittingResult, FittingResultVersion
-from fitting.processing import ProcessingException, submit_fitting
+from fitting.processing import submit_fitting
 
 
 def generate_response(template='%s succ celery-task-id', field='signature'):