From 6abf32a3fd4eefbd04bca02551df4dd2220d7c33 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Wed, 23 Nov 2022 17:47:18 +0000 Subject: [PATCH 01/12] Make BaseModelMandatoryName name also unique --- common/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/models.py b/common/models.py index 33fc36f2..698cbd32 100644 --- a/common/models.py +++ b/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 From b56fc36b8ea63861a45c5ce72b9b25f9f0dd7105 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Wed, 23 Nov 2022 17:48:44 +0000 Subject: [PATCH 02/12] Make device spec, equipment, experiment, component and compound names manditory and unique --- battDB/models.py | 12 +++++++++--- dfndb/models.py | 14 ++++++-------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/battDB/models.py b/battDB/models.py index 3f92dade..884ff0dd 100644 --- a/battDB/models.py +++ b/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.
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( @@ -372,7 +372,7 @@ class Meta: verbose_name_plural = "Equipment" -class Experiment(cm.BaseModel): +class Experiment(cm.BaseModelMandatoryName): """Main "Experiment" aka DataSet class.
Has many: data files (from cyclers)
Has many: devices (actual physical @@ -474,6 +474,12 @@ def __str__(self): def get_absolute_url(self): return reverse("battDB:Experiment", kwargs={"pk": self.pk}) + class Meta: + unique_together = ( + "name", + "user_owner", + ) + class ExperimentDataFile(cm.BaseModel): """EDF is the class tying together data files, parsed data tables and experiments. diff --git a/dfndb/models.py b/dfndb/models.py index 929774ee..04b88338 100644 --- a/dfndb/models.py +++ b/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 From 119ee8da41e43ae6b25ade57e1b93866d67a1e2e Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Wed, 23 Nov 2022 17:56:51 +0000 Subject: [PATCH 03/12] Include note about data file name in form --- battDB/forms.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/battDB/forms.py b/battDB/forms.py index 8f2bf2da..e9713c15 100644 --- a/battDB/forms.py +++ b/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. " ' ' "new machine ⧉" - ) + ), } def __init__(self, *args, **kwargs): From 3632c9d64cb85406a7159be73d5ae47edad806f9 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Tue, 29 Nov 2022 16:51:23 +0000 Subject: [PATCH 04/12] Make Experiment name unique together with user owner institution --- battDB/models.py | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/battDB/models.py b/battDB/models.py index 884ff0dd..76b6ce70 100644 --- a/battDB/models.py +++ b/battDB/models.py @@ -372,7 +372,7 @@ class Meta: verbose_name_plural = "Equipment" -class Experiment(cm.BaseModelMandatoryName): +class Experiment(cm.BaseModel): """Main "Experiment" aka DataSet class.
Has many: data files (from cyclers)
Has many: devices (actual physical @@ -398,6 +398,13 @@ class Experiment(cm.BaseModelMandatoryName): ("other", "Other"), ) + name = models.CharField( + max_length=128, + blank=False, + default="", + null=False, + ) + date = models.DateField(default=datetime.now) config = models.ForeignKey( @@ -474,11 +481,23 @@ def __str__(self): def get_absolute_url(self): return reverse("battDB:Experiment", kwargs={"pk": self.pk}) - class Meta: - unique_together = ( - "name", - "user_owner", + 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: + raise ValidationError( + f"Name '{self.name}' is already used in your institution" + ) class ExperimentDataFile(cm.BaseModel): From a02dd9a7851e166b8b50f577f25fca6f03a39fd5 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Tue, 29 Nov 2022 16:52:48 +0000 Subject: [PATCH 05/12] Migrate mandatory/unique name changes --- ...tion_name_alter_equipment_name_and_more.py | 33 ++++++++++++++++++ ..._together_alter_component_name_and_more.py | 34 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 battDB/migrations/0032_alter_devicespecification_name_alter_equipment_name_and_more.py create mode 100644 dfndb/migrations/0016_alter_compound_unique_together_alter_component_name_and_more.py diff --git a/battDB/migrations/0032_alter_devicespecification_name_alter_equipment_name_and_more.py b/battDB/migrations/0032_alter_devicespecification_name_alter_equipment_name_and_more.py new file mode 100644 index 00000000..647d2509 --- /dev/null +++ b/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), + ), + ] diff --git a/dfndb/migrations/0016_alter_compound_unique_together_alter_component_name_and_more.py b/dfndb/migrations/0016_alter_compound_unique_together_alter_component_name_and_more.py new file mode 100644 index 00000000..c7fa9b01 --- /dev/null +++ b/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), + ), + ] From a73bf7cc84a515e3a68ca6bba344bcedfcd9de70 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Tue, 29 Nov 2022 16:53:01 +0000 Subject: [PATCH 06/12] Adjust battDB tests and bakery for unique constraints --- tests/battDB/baker_recipes.py | 18 ++++++++++++++---- tests/battDB/test_models.py | 10 ++++++++++ tests/dfndb/test_models.py | 17 ++++++++++++----- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/tests/battDB/baker_recipes.py b/tests/battDB/baker_recipes.py index 3acae8bd..bcd09a86 100644 --- a/tests/battDB/baker_recipes.py +++ b/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"), +) 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"), ) 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"), ) edf = Recipe(ExperimentDataFile, user_owner=foreign_key(user)) diff --git a/tests/battDB/test_models.py b/tests/battDB/test_models.py index 5d5a4a1e..d2fc03d3 100644 --- a/tests/battDB/test_models.py +++ b/tests/battDB/test_models.py @@ -179,6 +179,16 @@ 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() + baker.make_recipe("tests.battDB.experiment", name=self.model.name) + class TestExperimentDataFile(TestCase): def setUp(self): diff --git a/tests/dfndb/test_models.py b/tests/dfndb/test_models.py index ca49bdc5..28b06d0c 100644 --- a/tests/dfndb/test_models.py +++ b/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): From ce1887ef58701dad4487e1b774bbe45f81e883f7 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Fri, 2 Dec 2022 16:09:38 +0000 Subject: [PATCH 07/12] Allow to update experiments without changing its name --- battDB/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/battDB/models.py b/battDB/models.py index 76b6ce70..7fde6b35 100644 --- a/battDB/models.py +++ b/battDB/models.py @@ -494,7 +494,7 @@ def clean(self): name=self.name, user_owner__institution=self.user_owner.institution, ) - if len(instances) > 0: + if len(instances) > 0 and instances[0] != self: raise ValidationError( f"Name '{self.name}' is already used in your institution" ) From ac117c53b89de1f2dd0a1033f299719b0fd3baa2 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Fri, 2 Dec 2022 16:10:48 +0000 Subject: [PATCH 08/12] Adding user_owner to form before checking validity of form --- common/views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/views.py b/common/views.py index 789a57dd..150d3de8 100644 --- a/common/views.py +++ b/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 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: From 437a88d7e2e1c06899247732ea5be6cc4ab5aa1c Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Fri, 2 Dec 2022 16:11:39 +0000 Subject: [PATCH 09/12] Tests for create/update experiment model and view --- tests/battDB/test_models.py | 5 ++- tests/battDB/test_views.py | 85 +++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/tests/battDB/test_models.py b/tests/battDB/test_models.py index d2fc03d3..2fbb7b36 100644 --- a/tests/battDB/test_models.py +++ b/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")) @@ -187,7 +189,8 @@ def test_unique_name(self): user_owner=self.model.user_owner, ) new.clean() - baker.make_recipe("tests.battDB.experiment", name=self.model.name) + another = baker.make_recipe("tests.battDB.experiment", name=self.model.name) + another.clean() class TestExperimentDataFile(TestCase): diff --git a/tests/battDB/test_views.py b/tests/battDB/test_views.py index 6d6502c0..6955291f 100644 --- a/tests/battDB/test_views.py +++ b/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,90 @@ def test_detail_view(self): self.assertContains(response, "

test experiment

") +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 + 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) + + # 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 + 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) + + # 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( From 234611890fa29481471bb7be9606a096431b5585 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Mon, 5 Dec 2022 15:10:39 +0000 Subject: [PATCH 10/12] Update tests/battDB/test_views.py Co-authored-by: Dan Davies <5871253+dandavies99@users.noreply.github.com> --- tests/battDB/test_views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/battDB/test_views.py b/tests/battDB/test_views.py index 6955291f..5115f2e6 100644 --- a/tests/battDB/test_views.py +++ b/tests/battDB/test_views.py @@ -340,6 +340,7 @@ def test_create_update_delete_experiment(self): 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" From 2abb2d0b6bb896c6bd88ec0499acb5c985d45605 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Mon, 5 Dec 2022 15:41:45 +0000 Subject: [PATCH 11/12] Test error strings in response --- tests/battDB/test_views.py | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/tests/battDB/test_views.py b/tests/battDB/test_views.py index 5115f2e6..2a0cb1eb 100644 --- a/tests/battDB/test_views.py +++ b/tests/battDB/test_views.py @@ -332,11 +332,8 @@ def test_create_update_delete_experiment(self): "summary": "Not long enough", } - # Invalid Form - response = self.client.post( - reverse("battDB:New Experiment"), - form_fields, - ) + # 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) @@ -344,10 +341,7 @@ def test_create_update_delete_experiment(self): # Create new experiment form_fields["summary"] = "At least 20 characters" - response = self.client.post( - reverse("battDB:New Experiment"), - form_fields, - ) + 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}/") @@ -357,14 +351,13 @@ def test_create_update_delete_experiment(self): for key, val in form_fields.items(): self.assertEqual(getattr(experiment, key), val) - # Create new experiment with same name - response = self.client.post( - reverse("battDB:New Experiment"), - form_fields, - ) + # 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" From 069a2f9a203611897252dcd1b36e76799906ac95 Mon Sep 17 00:00:00 2001 From: Adrian D'Alessandro Date: Mon, 5 Dec 2022 15:58:20 +0000 Subject: [PATCH 12/12] Some comments for clarifty --- common/views.py | 2 +- tests/battDB/baker_recipes.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/views.py b/common/views.py index 150d3de8..8708ff88 100644 --- a/common/views.py +++ b/common/views.py @@ -72,7 +72,7 @@ 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 + 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(): diff --git a/tests/battDB/baker_recipes.py b/tests/battDB/baker_recipes.py index bcd09a86..e85293a2 100644 --- a/tests/battDB/baker_recipes.py +++ b/tests/battDB/baker_recipes.py @@ -25,7 +25,7 @@ device_specification = Recipe( DeviceSpecification, user_owner=foreign_key(user), - name=seq("Device Specification"), + name=seq("Device Specification"), # Ensure uniqueness ) device_parameter = Recipe( @@ -64,14 +64,14 @@ Equipment, institution=foreign_key(org), user_owner=foreign_key(user), - name=seq("Equipment"), + name=seq("Equipment"), # Ensure uniqueness ) experiment = Recipe( Experiment, config=foreign_key(device_config), user_owner=foreign_key(user), - name=seq("Experiment"), + name=seq("Experiment"), # Ensure uniqueness ) edf = Recipe(ExperimentDataFile, user_owner=foreign_key(user))