From 69ac7a30b05fdc53e3c2b2514c6f1105bf55c86d Mon Sep 17 00:00:00 2001 From: Quentin BEY Date: Mon, 2 Dec 2024 11:14:19 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=83=EF=B8=8F(contacts)=20rename=20`bas?= =?UTF-8?q?e`=20to=20`override`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To improve code readability, I propose to rename the contact field `override`. This comes along with the fact a contact should not not always override another (it's the case were I only want to create some personal contacts). --- src/backend/core/admin.py | 2 +- src/backend/core/api/client/serializers.py | 6 +-- src/backend/core/api/client/viewsets.py | 2 +- src/backend/core/factories.py | 12 ++++- .../0007_rename_contact_base_to_override.py | 47 +++++++++++++++++ src/backend/core/models.py | 20 +++---- src/backend/core/tests/test_api_contacts.py | 52 +++++++++---------- .../core/tests/test_models_contacts.py | 26 +++++----- src/backend/core/tests/test_models_users.py | 2 +- 9 files changed, 113 insertions(+), 56 deletions(-) create mode 100644 src/backend/core/migrations/0007_rename_contact_base_to_override.py diff --git a/src/backend/core/admin.py b/src/backend/core/admin.py index b2a59c364..7df08d0eb 100644 --- a/src/backend/core/admin.py +++ b/src/backend/core/admin.py @@ -194,7 +194,7 @@ class ContactAdmin(admin.ModelAdmin): list_display = ( "full_name", "owner", - "base", + "override", ) diff --git a/src/backend/core/api/client/serializers.py b/src/backend/core/api/client/serializers.py index d0b707d1b..36b9cf102 100644 --- a/src/backend/core/api/client/serializers.py +++ b/src/backend/core/api/client/serializers.py @@ -14,7 +14,7 @@ class Meta: model = models.Contact fields = [ "id", - "base", + "override", "data", "full_name", "notes", @@ -23,12 +23,12 @@ class Meta: ] read_only_fields = ["id", "owner"] extra_kwargs = { - "base": {"required": False}, + "override": {"required": False}, } def update(self, instance, validated_data): """Make "base" field readonly but only for update/patch.""" - validated_data.pop("base", None) + validated_data.pop("override", None) return super().update(instance, validated_data) diff --git a/src/backend/core/api/client/viewsets.py b/src/backend/core/api/client/viewsets.py index 2849f18a4..81e2741c3 100644 --- a/src/backend/core/api/client/viewsets.py +++ b/src/backend/core/api/client/viewsets.py @@ -152,7 +152,7 @@ def list(self, request, *args, **kwargs): # - are profile contacts for a user user__isnull=True, # - are overriden base contacts - overriding_contacts__isnull=True, + overridden_by__isnull=True, ) # Search by case-insensitive and accent-insensitive similarity diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index 96e211a9e..ce978d120 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -115,7 +115,17 @@ class ContactFactory(BaseContactFactory): class Meta: model = models.Contact - base = factory.SubFactory("core.factories.ContactFactory", base=None, owner=None) + # base = factory.SubFactory("core.factories.ContactFactory", base=None, owner=None) + owner = factory.SubFactory("core.factories.UserFactory", profile_contact=None) + + +class OverrideContactFactory(BaseContactFactory): + """A factory to create contacts for a user""" + + class Meta: + model = models.Contact + + override = factory.SubFactory("core.factories.ContactFactory", owner=None) owner = factory.SubFactory("core.factories.UserFactory", profile_contact=None) diff --git a/src/backend/core/migrations/0007_rename_contact_base_to_override.py b/src/backend/core/migrations/0007_rename_contact_base_to_override.py new file mode 100644 index 000000000..757123183 --- /dev/null +++ b/src/backend/core/migrations/0007_rename_contact_base_to_override.py @@ -0,0 +1,47 @@ +# Generated by Django 5.1.3 on 2024-12-02 10:04 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0006_contact_notes_alter_contact_full_name'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='contact', + name='base_owner_constraint', + ), + migrations.RemoveConstraint( + model_name='contact', + name='base_not_self', + ), + migrations.AlterUniqueTogether( + name='contact', + unique_together=set(), + ), + migrations.AddField( + model_name='contact', + name='override', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='overridden_by', to='core.contact'), + ), + migrations.AddConstraint( + model_name='contact', + constraint=models.CheckConstraint(condition=models.Q(('override__isnull', False), ('owner__isnull', True), _negated=True), name='override_owner_constraint', violation_error_message='A contact overriding a base contact must be owned.'), + ), + migrations.AddConstraint( + model_name='contact', + constraint=models.CheckConstraint(condition=models.Q(('override', models.F('id')), _negated=True), name='override_not_self', violation_error_message='A contact cannot override itself.'), + ), + migrations.RemoveField( + model_name='contact', + name='base', + ), + migrations.AlterUniqueTogether( + name='contact', + unique_together={('owner', 'override')}, + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 585ac520e..e17cf3c1f 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -99,10 +99,10 @@ def save(self, *args, **kwargs): class Contact(BaseModel): """User contacts""" - base = models.ForeignKey( + override = models.ForeignKey( "self", on_delete=models.SET_NULL, - related_name="overriding_contacts", + related_name="overridden_by", null=True, blank=True, ) @@ -136,17 +136,17 @@ class Meta: ordering = ("full_name", "short_name") verbose_name = _("contact") verbose_name_plural = _("contacts") - unique_together = ("owner", "base") + unique_together = ("owner", "override") constraints = [ models.CheckConstraint( - condition=~models.Q(base__isnull=False, owner__isnull=True), - name="base_owner_constraint", + condition=~models.Q(override__isnull=False, owner__isnull=True), + name="override_owner_constraint", violation_error_message="A contact overriding a base contact must be owned.", ), models.CheckConstraint( - condition=~models.Q(base=models.F("id")), - name="base_not_self", - violation_error_message="A contact cannot be based on itself.", + condition=~models.Q(override=models.F("id")), + name="override_not_self", + violation_error_message="A contact cannot override itself.", ), ] @@ -158,9 +158,9 @@ def clean(self): super().clean() # Check if the contact points to a base contact that itself points to another base contact - if self.base_id and self.base.base_id: + if self.override_id and self.override.override_id: raise exceptions.ValidationError( - "A contact cannot point to a base contact that itself points to a base contact." + "A contact cannot point to a base contact that itself overrides a contact." ) # Validate the content of the "data" field against our jsonschema definition diff --git a/src/backend/core/tests/test_api_contacts.py b/src/backend/core/tests/test_api_contacts.py index 9cd26b807..66d87b92f 100644 --- a/src/backend/core/tests/test_api_contacts.py +++ b/src/backend/core/tests/test_api_contacts.py @@ -78,10 +78,10 @@ def test_api_contacts_list_authenticated_no_query(): assert user.profile_contact is not None # Excluded because profile contact base_contact = factories.BaseContactFactory() # Excluded because overridden factories.ContactFactory( - base=base_contact + override=base_contact ) # Excluded because belongs to other user contact2 = factories.ContactFactory( - base=base_contact, owner=user, full_name="Bernard" + override=base_contact, owner=user, full_name="Bernard" ) # Included client = APIClient() @@ -93,7 +93,7 @@ def test_api_contacts_list_authenticated_no_query(): assert response.json() == [ { "id": str(contact2.id), - "base": str(base_contact.id), + "override": str(base_contact.id), "owner": str(contact2.owner.id), "data": contact2.data, "full_name": contact2.full_name, @@ -268,7 +268,7 @@ def test_api_contacts_retrieve_authenticated_owned(): assert response.status_code == 200 assert response.json() == { "id": str(contact.id), - "base": str(contact.base.id), + "override": None, "owner": str(contact.owner.id), "data": contact.data, "full_name": contact.full_name, @@ -291,7 +291,7 @@ def test_api_contacts_retrieve_authenticated_public(): assert response.status_code == 200 assert response.json() == { "id": str(contact.id), - "base": None, + "override": None, "owner": None, "data": contact.data, "full_name": contact.full_name, @@ -351,7 +351,7 @@ def test_api_contacts_create_authenticated_missing_base(): new_contact = models.Contact.objects.get(owner=user) assert response.json() == { - "base": None, + "override": None, "data": {}, "full_name": "David Bowman", "id": str(new_contact.pk), @@ -370,12 +370,12 @@ def test_api_contacts_create_authenticated_successful(): client.force_login(user) # Existing override for another user should not interfere - factories.ContactFactory(base=base_contact) + factories.ContactFactory(override=base_contact) response = client.post( "/api/v1.0/contacts/", { - "base": str(base_contact.id), + "override": str(base_contact.id), "full_name": "David Bowman", "short_name": "Dave", "data": CONTACT_DATA, @@ -389,7 +389,7 @@ def test_api_contacts_create_authenticated_successful(): contact = models.Contact.objects.get(owner=user) assert response.json() == { "id": str(contact.id), - "base": str(base_contact.id), + "override": str(base_contact.id), "data": CONTACT_DATA, "full_name": "David Bowman", "notes": "", @@ -400,7 +400,7 @@ def test_api_contacts_create_authenticated_successful(): assert contact.full_name == "David Bowman" assert contact.short_name == "Dave" assert contact.data == CONTACT_DATA - assert contact.base == base_contact + assert contact.override == base_contact assert contact.owner == user @@ -412,7 +412,7 @@ def test_api_contacts_create_authenticated_existing_override(): """ user = factories.UserFactory(profile_contact=None) base_contact = factories.BaseContactFactory() - factories.ContactFactory(base=base_contact, owner=user) + factories.ContactFactory(override=base_contact, owner=user) client = APIClient() client.force_login(user) @@ -420,7 +420,7 @@ def test_api_contacts_create_authenticated_existing_override(): response = client.post( "/api/v1.0/contacts/", { - "base": str(base_contact.id), + "override": str(base_contact.id), "full_name": "David Bowman", "notes": "", "short_name": "Dave", @@ -433,7 +433,7 @@ def test_api_contacts_create_authenticated_existing_override(): assert models.Contact.objects.count() == 2 assert response.json() == { - "__all__": ["Contact with this Owner and Base already exists."] + "__all__": ["Contact with this Owner and Override already exists."] } @@ -461,7 +461,7 @@ def test_api_contacts_create_authenticated_successful_with_notes(): contact = models.Contact.objects.get(owner=user) assert response.json() == { "id": str(contact.id), - "base": None, + "override": None, "data": CONTACT_DATA, "full_name": "David Bowman", "notes": "This is a note", @@ -484,7 +484,7 @@ def test_api_contacts_update_anonymous(): new_contact_values = serializers.ContactSerializer( instance=factories.ContactFactory() ).data - new_contact_values["base"] = str(factories.ContactFactory().id) + new_contact_values["override"] = str(factories.ContactFactory().id) response = APIClient().put( f"/api/v1.0/contacts/{contact.id!s}/", new_contact_values, @@ -515,7 +515,7 @@ def test_api_contacts_update_authenticated_owned(): new_contact_values = serializers.ContactSerializer( instance=factories.ContactFactory() ).data - new_contact_values["base"] = str(factories.ContactFactory().id) + new_contact_values["override"] = str(factories.ContactFactory().id) response = client.put( f"/api/v1.0/contacts/{contact.id!s}/", @@ -528,7 +528,7 @@ def test_api_contacts_update_authenticated_owned(): contact.refresh_from_db() contact_values = serializers.ContactSerializer(instance=contact).data for key, value in contact_values.items(): - if key in ["base", "owner", "id"]: + if key in ["override", "owner", "id"]: assert value == old_contact_values[key] else: assert value == new_contact_values[key] @@ -551,7 +551,7 @@ def test_api_contacts_update_authenticated_profile(): new_contact_values = serializers.ContactSerializer( instance=factories.ContactFactory() ).data - new_contact_values["base"] = str(factories.ContactFactory().id) + new_contact_values["override"] = str(factories.ContactFactory().id) response = client.put( f"/api/v1.0/contacts/{contact.id!s}/", @@ -563,7 +563,7 @@ def test_api_contacts_update_authenticated_profile(): contact.refresh_from_db() contact_values = serializers.ContactSerializer(instance=contact).data for key, value in contact_values.items(): - if key in ["base", "owner", "id"]: + if key in ["override", "owner", "id"]: assert value == old_contact_values[key] else: assert value == new_contact_values[key] @@ -584,7 +584,7 @@ def test_api_contacts_update_authenticated_other(): new_contact_values = serializers.ContactSerializer( instance=factories.ContactFactory() ).data - new_contact_values["base"] = str(factories.ContactFactory().id) + new_contact_values["override"] = str(factories.ContactFactory().id) response = client.put( f"/api/v1.0/contacts/{contact.id!s}/", @@ -606,7 +606,7 @@ def test_api_contacts_delete_list_anonymous(): response = APIClient().delete("/api/v1.0/contacts/") assert response.status_code == 401 - assert models.Contact.objects.count() == 4 + assert models.Contact.objects.count() == 2 def test_api_contacts_delete_list_authenticated(): @@ -621,7 +621,7 @@ def test_api_contacts_delete_list_authenticated(): response = client.delete("/api/v1.0/contacts/") assert response.status_code == 405 - assert models.Contact.objects.count() == 4 + assert models.Contact.objects.count() == 2 def test_api_contacts_delete_anonymous(): @@ -632,7 +632,7 @@ def test_api_contacts_delete_anonymous(): response = client.delete(f"/api/v1.0/contacts/{contact.id!s}/") assert response.status_code == 401 - assert models.Contact.objects.count() == 2 + assert models.Contact.objects.count() == 1 def test_api_contacts_delete_authenticated_public(): @@ -669,7 +669,7 @@ def test_api_contacts_delete_authenticated_owner(): ) assert response.status_code == 204 - assert models.Contact.objects.count() == 1 + assert models.Contact.objects.count() == 0 assert models.Contact.objects.filter(id=contact.id).exists() is False @@ -678,7 +678,7 @@ def test_api_contacts_delete_authenticated_profile(): Authenticated users should be allowed to delete their profile contact. """ user = factories.UserFactory() - contact = factories.ContactFactory(owner=user, base=None) + contact = factories.ContactFactory(owner=user, override=None) user.profile_contact = contact user.save() @@ -708,4 +708,4 @@ def test_api_contacts_delete_authenticated_other(): ) assert response.status_code == 403 - assert models.Contact.objects.count() == 2 + assert models.Contact.objects.count() == 1 diff --git a/src/backend/core/tests/test_models_contacts.py b/src/backend/core/tests/test_models_contacts.py index 7434737bd..2ee3f0403 100644 --- a/src/backend/core/tests/test_models_contacts.py +++ b/src/backend/core/tests/test_models_contacts.py @@ -20,49 +20,49 @@ def test_models_contacts_str_full_name(): def test_models_contacts_base_self(): """A contact should not point to itself as a base contact.""" contact = factories.ContactFactory() - contact.base = contact + contact.override = contact with pytest.raises(ValidationError) as excinfo: contact.save() error_message = ( - "{'__all__': ['A contact cannot point to a base contact that itself points to a " - "base contact.', 'A contact cannot be based on itself.']}" + "{'__all__': ['A contact cannot point to a base contact that itself overrides " + "a contact.', 'A contact cannot override itself.']}" ) assert str(excinfo.value) == error_message def test_models_contacts_base_to_base(): """A contact should not point to a base contact that is itself derived from a base contact.""" - contact = factories.ContactFactory() + contact = factories.OverrideContactFactory() with pytest.raises(ValidationError) as excinfo: - factories.ContactFactory(base=contact) + factories.OverrideContactFactory(override=contact) error_message = ( - "{'__all__': ['A contact cannot point to a base contact that itself points to a " - "base contact.']}" + "{'__all__': ['A contact cannot point to a base contact that itself overrides " + "a contact.']}" ) assert str(excinfo.value) == error_message def test_models_contacts_owner_base_unique(): """There should be only one contact deriving from a given base contact for a given owner.""" - contact = factories.ContactFactory() + contact = factories.OverrideContactFactory() with pytest.raises(ValidationError) as excinfo: - factories.ContactFactory(base=contact.base, owner=contact.owner) + factories.OverrideContactFactory(override=contact.override, owner=contact.owner) assert ( str(excinfo.value) - == "{'__all__': ['Contact with this Owner and Base already exists.']}" + == "{'__all__': ['Contact with this Owner and Override already exists.']}" ) def test_models_contacts_base_not_owned(): - """A contact cannot have a base and not be owned.""" + """A contact cannot override a contact without being owned.""" with pytest.raises(ValidationError) as excinfo: - factories.ContactFactory(owner=None) + factories.OverrideContactFactory(owner=None) assert ( str(excinfo.value) @@ -72,7 +72,7 @@ def test_models_contacts_base_not_owned(): def test_models_contacts_profile_not_owned(): """A contact cannot be defined as profile for a user if is not owned.""" - base_contact = factories.ContactFactory(owner=None, base=None) + base_contact = factories.ContactFactory(owner=None) with pytest.raises(ValidationError) as excinfo: factories.UserFactory(profile_contact=base_contact) diff --git a/src/backend/core/tests/test_models_users.py b/src/backend/core/tests/test_models_users.py index bb306c6c5..d2a86da0d 100644 --- a/src/backend/core/tests/test_models_users.py +++ b/src/backend/core/tests/test_models_users.py @@ -133,7 +133,7 @@ def test_models_users_email_not_unique(): def test_models_users_profile_not_owned(): """A user cannot declare as profile a contact that not is owned.""" user = factories.UserFactory() - contact = factories.ContactFactory(base=None, owner=None) + contact = factories.ContactFactory(override=None, owner=None) user.profile_contact = contact with pytest.raises(ValidationError) as excinfo: