Skip to content

Commit

Permalink
🗃️(contacts) rename base to override
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
qbey committed Dec 2, 2024
1 parent 2a4dd17 commit 69ac7a3
Show file tree
Hide file tree
Showing 9 changed files with 113 additions and 56 deletions.
2 changes: 1 addition & 1 deletion src/backend/core/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class ContactAdmin(admin.ModelAdmin):
list_display = (
"full_name",
"owner",
"base",
"override",
)


Expand Down
6 changes: 3 additions & 3 deletions src/backend/core/api/client/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class Meta:
model = models.Contact
fields = [
"id",
"base",
"override",
"data",
"full_name",
"notes",
Expand All @@ -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)


Expand Down
2 changes: 1 addition & 1 deletion src/backend/core/api/client/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 11 additions & 1 deletion src/backend/core/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)


Expand Down
Original file line number Diff line number Diff line change
@@ -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')},
),
]
20 changes: 10 additions & 10 deletions src/backend/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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.",
),
]

Expand All @@ -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
Expand Down
52 changes: 26 additions & 26 deletions src/backend/core/tests/test_api_contacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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,
Expand All @@ -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": "",
Expand All @@ -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


Expand All @@ -412,15 +412,15 @@ 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)

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",
Expand All @@ -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."]
}


Expand Down Expand Up @@ -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",
Expand All @@ -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,
Expand Down Expand Up @@ -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}/",
Expand All @@ -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]
Expand All @@ -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}/",
Expand All @@ -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]
Expand All @@ -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}/",
Expand All @@ -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():
Expand All @@ -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():
Expand All @@ -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():
Expand Down Expand Up @@ -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


Expand All @@ -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()

Expand Down Expand Up @@ -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
Loading

0 comments on commit 69ac7a3

Please sign in to comment.