Merge pull request #224 from ImperialCollegeLondon/mandatory-names

Mandatory names

battDB/forms.py

@@ -330,11 +330,12 @@ class Meta:
             "binary_file",
         ]
         help_texts = {
+            "name": mark_safe("If left blank, the file name will be used."),
             "machine": mark_safe(
                 "The machine this data file was collected on. "
                 '<a href="/battDB/new_equipment/" target="_blank"> '
                 "new machine &#10697;</a>"
-            )
+            ),
         }
 
     def __init__(self, *args, **kwargs):

battDB/migrations/0032_alter_devicespecification_name_alter_equipment_name_and_more.py

@@ -0,0 +1,33 @@
+# Generated by Django 4.1.3 on 2022-11-29 15:52
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('battDB', '0031_experimentdatafile_binary_file_and_more'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='devicespecification',
+            name='name',
+            field=models.CharField(default='', max_length=128, unique=True),
+        ),
+        migrations.AlterField(
+            model_name='equipment',
+            name='name',
+            field=models.CharField(default='', max_length=128, unique=True),
+        ),
+        migrations.AlterField(
+            model_name='experiment',
+            name='name',
+            field=models.CharField(default='', max_length=128),
+        ),
+        migrations.AlterField(
+            model_name='parser',
+            name='name',
+            field=models.CharField(default='', max_length=128, unique=True),
+        ),
+    ]

battDB/models.py

@@ -19,7 +19,7 @@
 from parsing_engines import available_parsing_engines, parse_data_file
 
 
-class DeviceSpecification(cm.BaseModel, cm.HasMPTT):
+class DeviceSpecification(cm.BaseModelMandatoryName, cm.HasMPTT):
     """A template for creating a device or batch of devices in the system.
 
     <br> Specifications are structured as a tree, so that each device can be composed of
@@ -345,7 +345,7 @@ def get_number_parameters(self):
         return self.columns.count()
 
 
-class Equipment(cm.BaseModel):
+class Equipment(cm.BaseModelMandatoryName):
     """Definitions of equipment such as cycler machines."""
 
     institution = models.ForeignKey(
@@ -398,6 +398,13 @@ class Experiment(cm.BaseModel):
         ("other", "Other"),
     )
 
+    name = models.CharField(
+        max_length=128,
+        blank=False,
+        default="",
+        null=False,
+    )
+
     date = models.DateField(default=datetime.now)
 
     config = models.ForeignKey(
@@ -474,6 +481,24 @@ def __str__(self):
     def get_absolute_url(self):
         return reverse("battDB:Experiment", kwargs={"pk": self.pk})
 
+    def clean(self):
+        """Validate that the name for an experiment is unique per institution.
+
+        Args:
+            name: The name attempting to be saved.
+
+        Raises:
+            ValidationError: _description_
+        """
+        instances = Experiment.objects.filter(
+            name=self.name,
+            user_owner__institution=self.user_owner.institution,
+        )
+        if len(instances) > 0 and instances[0] != self:
+            raise ValidationError(
+                f"Name '{self.name}' is already used in your institution"
+            )
+
 
 class ExperimentDataFile(cm.BaseModel):
     """EDF is the class tying together data files, parsed data tables and experiments.

common/models.py

@@ -181,7 +181,9 @@ class Meta:
 class BaseModelMandatoryName(BaseModel):
     """Changed BaseModel.name to blank=False."""
 
-    name = models.CharField(max_length=128, blank=False, default="", null=False)
+    name = models.CharField(
+        max_length=128, blank=False, default="", null=False, unique=True
+    )
 
     class Meta:
         abstract = True

common/views.py

@@ -19,10 +19,10 @@ def get(self, request, *args, **kwargs):
 
     def post(self, request, *args, **kwargs):
         form = self.form_class(request.POST, request.FILES)
+        form.instance.user_owner = request.user
         if form.is_valid():
             obj = form.save(commit=False)
             # Do other stuff before saving here
-            obj.user_owner = request.user
             if form.is_public():
                 obj.status = "public"
             else:
@@ -70,12 +70,13 @@ def get(self, request, *args, **kwargs):
 
     def post(self, request, *args, **kwargs):
         form = self.form_class(request.POST, request.FILES)
+        form.instance.user_owner = request.user
         context = self.get_context_data()
+        context["form"] = form  # update form in context for form errors in render
         if form.is_valid():
             # Save instance incluing setting user owner and status
             with transaction.atomic():
                 obj = form.save(commit=False)
-                obj.user_owner = request.user
                 if form.is_public():
                     obj.status = "public"
                 else:

dfndb/migrations/0016_alter_compound_unique_together_alter_component_name_and_more.py

@@ -0,0 +1,34 @@
+# Generated by Django 4.1.3 on 2022-11-29 11:28
+
+from django.db import migrations, models
+
+import dfndb.validators
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('dfndb', '0015_remove_compound_mass_alter_compound_formula'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='compound',
+            unique_together=set(),
+        ),
+        migrations.AlterField(
+            model_name='component',
+            name='name',
+            field=models.CharField(default='', max_length=128, unique=True),
+        ),
+        migrations.AlterField(
+            model_name='compound',
+            name='formula',
+            field=models.CharField(help_text='Chemical formula', max_length=20, unique=True, validators=[dfndb.validators.validate_formula]),
+        ),
+        migrations.AlterField(
+            model_name='compound',
+            name='name',
+            field=models.CharField(help_text='Full name for the element or compound.', max_length=100, unique=True),
+        ),
+    ]

dfndb/models.py

@@ -17,9 +17,13 @@ class Compound(models.Model):
     name = models.CharField(
         max_length=100,
         help_text="Full name for the element or compound.",
+        unique=True,
     )
     formula = models.CharField(
-        max_length=20, help_text="Chemical formula", validators=[validate_formula]
+        max_length=20,
+        help_text="Chemical formula",
+        unique=True,
+        validators=[validate_formula],
     )
 
     @property
@@ -29,14 +33,8 @@ def mass(self):
     def __str__(self):
         return "%s (%s)" % (self.name, self.formula)
 
-    class Meta:
-        unique_together = (
-            "name",
-            "formula",
-        )
-
 
-class Component(cm.BaseModel):
+class Component(cm.BaseModelMandatoryName):
     """A component used as part of an electrochemical cell specification, e.g. NMC622.
 
     Make use of the 'notes' field for additional explanation

tests/battDB/baker_recipes.py

@@ -1,4 +1,4 @@
-from model_bakery.recipe import Recipe, foreign_key
+from model_bakery.recipe import Recipe, foreign_key, seq
 
 from battDB.models import (
     Batch,
@@ -22,7 +22,11 @@
 from tests.dfndb.baker_recipes import component, parameter
 from tests.management.baker_recipes import user
 
-device_specification = Recipe(DeviceSpecification, user_owner=foreign_key(user))
+device_specification = Recipe(
+    DeviceSpecification,
+    user_owner=foreign_key(user),
+    name=seq("Device Specification"),  # Ensure uniqueness
+)
 
 device_parameter = Recipe(
     DeviceParameter,
@@ -57,11 +61,17 @@
 parser = Recipe(Parser, user_owner=foreign_key(user))
 
 equipment = Recipe(
-    Equipment, institution=foreign_key(org), user_owner=foreign_key(user)
+    Equipment,
+    institution=foreign_key(org),
+    user_owner=foreign_key(user),
+    name=seq("Equipment"),  # Ensure uniqueness
 )
 
 experiment = Recipe(
-    Experiment, config=foreign_key(device_config), user_owner=foreign_key(user)
+    Experiment,
+    config=foreign_key(device_config),
+    user_owner=foreign_key(user),
+    name=seq("Experiment"),  # Ensure uniqueness
 )
 
 edf = Recipe(ExperimentDataFile, user_owner=foreign_key(user))

tests/battDB/test_models.py

@@ -158,6 +158,8 @@ def test_definition(self):
 class TestExperiment(TestCase):
     def setUp(self):
         self.model = baker.make_recipe("tests.battDB.experiment")
+        self.model.user_owner.institution = baker.make_recipe("tests.common.org")
+        self.model.user_owner.save()
 
     def test_definition(self):
         self.assertTrue(hasattr(self.model, "date"))
@@ -179,6 +181,17 @@ def test_get_absolute_url(self):
         url = self.model.get_absolute_url()
         self.assertIn("battDB/exps", url)
 
+    def test_unique_name(self):
+        with self.assertRaises(ValidationError):
+            new = baker.make_recipe(
+                "tests.battDB.experiment",
+                name=self.model.name,
+                user_owner=self.model.user_owner,
+            )
+            new.clean()
+        another = baker.make_recipe("tests.battDB.experiment", name=self.model.name)
+        another.clean()
+
 
 class TestExperimentDataFile(TestCase):
     def setUp(self):

tests/battDB/test_views.py

@@ -1,4 +1,5 @@
 import tempfile
+from datetime import datetime
 
 from django.contrib.auth.models import Group
 from django.test import TestCase, override_settings
@@ -302,6 +303,84 @@ def test_detail_view(self):
         self.assertContains(response, "<h4> test experiment </h4>")
 
 
+class CreateExperimentTest(TestCase):
+    def setUp(self):
+        self.user = baker.make_recipe(
+            "tests.management.user",
+            username="test_contributor",
+        )
+        self.user.is_active = True
+        self.user.set_password("contribpass")
+        self.user.institution = baker.make_recipe("tests.common.org")
+        self.user.save()
+        group = Group.objects.get(name="Contributor")
+        group.user_set.add(self.user)
+
+    def test_create_update_delete_experiment(self):
+        login_response = self.client.post(
+            "/accounts/login/",
+            {"username": "test_contributor", "password": "contribpass"},
+        )
+        self.assertEqual(login_response.status_code, 302)
+        self.assertEqual(login_response.url, "/")
+
+        form_fields = {
+            "name": "Experiment 1",
+            "date": datetime.now().date(),
+            "exp_type": "constant",
+            "thermal": "none",
+            "summary": "Not long enough",
+        }
+
+        # Invalid Form (short summary)
+        response = self.client.post(reverse("battDB:New Experiment"), form_fields)
+        with self.assertRaises(bdb.Experiment.DoesNotExist):
+            bdb.Experiment.objects.get()
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Ensure this value has at least 20 characters")
+
+        # Create new experiment
+        form_fields["summary"] = "At least 20 characters"
+        response = self.client.post(reverse("battDB:New Experiment"), form_fields)
+        experiment = bdb.Experiment.objects.get(name="Experiment 1")
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, f"/battDB/exps/{experiment.id}/")
+        self.assertEqual(experiment.user_owner.username, "test_contributor")
+        self.assertEqual(experiment.user_owner.institution, self.user.institution)
+        self.assertEqual(experiment.status, "private")
+        for key, val in form_fields.items():
+            self.assertEqual(getattr(experiment, key), val)
+
+        # Create new experiment with same name - should fail
+        response = self.client.post(reverse("battDB:New Experiment"), form_fields)
+        self.assertEqual(len(bdb.Experiment.objects.all()), 1)
+        self.assertEqual(bdb.Experiment.objects.get(name="Experiment 1"), experiment)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, " is already used in your institution")
+        self.assertContains(response, form_fields["name"])
+
+        # Update experiment
+        form_fields["summary"] = "A longer and more detailed summary"
+        update_response = self.client.post(
+            reverse("battDB:Update Experiment", kwargs={"pk": experiment.id}),
+            form_fields,
+        )
+        self.assertEqual(update_response.status_code, 302)
+        self.assertEqual(update_response.url, f"/battDB/exps/{experiment.id}/")
+
+        experiment.refresh_from_db()
+        self.assertEqual(experiment.summary, "A longer and more detailed summary")
+
+        # Delete experiment
+        delete_response = self.client.post(
+            reverse("battDB:Delete Experiment", kwargs={"pk": experiment.id})
+        )
+        self.assertEqual(delete_response.status_code, 302)
+        self.assertEqual(delete_response.url, "/battDB/exps/")
+        experiment.refresh_from_db()
+        self.assertEqual(experiment.status, "deleted")
+
+
 class DeviceSpecificationViewTest(TestCase):
     def setUp(self):
         self.user = baker.make_recipe(

tests/dfndb/test_models.py

@@ -21,12 +21,19 @@ def test_str(self):
             f"{self.expected['name']} ({self.expected['formula']})",
         )
 
-    def test_unique_together(self):
-        baker.make_recipe("tests.dfndb.compound", name="Carbon Dioxide", formula="CO3")
+    def test_unique(self):
+        from django.db import transaction
+
         with self.assertRaises(IntegrityError):
-            baker.make_recipe(
-                "tests.dfndb.compound", name="Carbon Dioxide", formula="CO2"
-            )
+            with transaction.atomic():
+                baker.make_recipe(
+                    "tests.dfndb.compound", name="Carbon Dioxide", formula="CO3"
+                )
+        with self.assertRaises(IntegrityError):
+            with transaction.atomic():
+                baker.make_recipe(
+                    "tests.dfndb.compound", name="Carbon Dioxide 2", formula="CO2"
+                )
 
     def test_invalid_formula(self):
         with self.assertRaises(ValidationError):