Skip to content
15 changes: 15 additions & 0 deletions src/backend/core/admin.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Admin config for find's core app"""

from django import forms
from django.contrib import admin
from django.shortcuts import render
from django.urls import path
Expand All @@ -9,10 +10,24 @@
from .selftests import registry


class ServiceAdminForm(forms.ModelForm):
"""Admin form keeping the slug immutable after creation."""

class Meta:
model = Service
fields = ("name", "slug", "is_active", "client_id", "services")

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if self.instance and self.instance.pk:
self.fields["slug"].disabled = True


@admin.register(Service)
class ServiceAdmin(admin.ModelAdmin):
"""Register the serivce model for the admin site"""

form = ServiceAdminForm
list_display = ("name", "created_at", "is_active")
search_fields = ("name",)
list_filter = ("is_active", "created_at")
Expand Down
3 changes: 2 additions & 1 deletion src/backend/core/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class ServiceFactory(factory.django.DjangoModelFactory):
A factory for generating service instances for testing and development purposes.
"""

name = factory.Sequence(lambda n: f"test-index-{n!s}")
slug = factory.Sequence(lambda n: f"testidx{n!s}")
name = factory.LazyAttribute(lambda o: f"Test Service {o.slug}")
Comment thread
StephanMeijer marked this conversation as resolved.
created_at = factory.Faker("date_time_this_year", tzinfo=None)
is_active = True
client_id = "some_client_id"
Expand Down
47 changes: 40 additions & 7 deletions src/backend/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@
import string

from django.contrib.auth.models import AbstractUser
from django.core.exceptions import ValidationError
from django.core.validators import RegexValidator
from django.db import models
from django.db.models.functions import Length
from django.utils.text import slugify
from django.utils.translation import gettext_lazy as _

models.CharField.register_lookup(Length)
TOKEN_LENGTH = 50
SLUG_REGEX = r"^[a-z0-9]+$"
SLUG_VALIDATOR = RegexValidator(
regex=SLUG_REGEX,
message=_("Slug must contain only lowercase letters and digits."),
)


class User(AbstractUser):
Expand All @@ -20,15 +26,22 @@ class User(AbstractUser):
class Service(models.Model):
"""Service registered to index its documents to our find"""

name = models.SlugField(max_length=20, unique=True)
name = models.CharField(max_length=255)
slug = models.SlugField(
max_length=20,
unique=True,
validators=[SLUG_VALIDATOR],
Comment thread
StephanMeijer marked this conversation as resolved.
help_text=_(
"Stable identifier used in the OpenSearch index name. "
"Lowercase alphanumeric only. Set on creation, immutable thereafter."
),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you may want to modify the admin model and prepopulate this field using dedicated utility (see Django docs).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the admin flow. I think we should make slug editable only at creation, but not prepopulate it from name: slug is the stable OpenSearch index identifier, while name is an editable display label. Prepopulating from name could suggest the slug should track the display name, which is exactly what this PR is decoupling.

So I propose to expose slug on the admin add form for an explicit, intentional value, make it read-only on existing services via ServiceAdmin.get_readonly_fields(), and keep the model-level immutability check as a backstop outside the admin.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

token = models.CharField(max_length=TOKEN_LENGTH)
created_at = models.DateTimeField(auto_now_add=True)
is_active = models.BooleanField(default=True)
client_id = models.CharField(blank=True, null=True)
services = models.ManyToManyField(
"self",
verbose_name=_("Allowed services for search"),
blank=True,
"self", blank=True, verbose_name=_("Allowed services for search")
)

class Meta:
Expand All @@ -41,14 +54,34 @@ class Meta:
condition=models.Q(token__length=TOKEN_LENGTH),
name="token_length_exact_50",
),
models.CheckConstraint(
condition=models.Q(slug__regex=SLUG_REGEX),
name="slug_alphanumeric_only",
),
]

def __str__(self):
return self.name

def save(self, *args, **kwargs):
"""Automatically slugify the service name and generate a token on creation"""
self.name = slugify(self.name)
"""Generate token and enforce slug immutability.

- ``slug`` must be provided explicitly on creation and is immutable
thereafter. The DB check constraint enforces the allowed character
set; no auto-derivation is performed.
- ``name`` is a free-form display field and can be edited.
- ``token`` is generated once on creation if missing.
"""
if not self._state.adding:
stored_slug = (
Service.objects.filter(pk=self.pk)
.values_list("slug", flat=True)
.first()
)
if self.slug != stored_slug:
raise ValidationError(
{"slug": _("Service.slug is immutable after creation.")}
)
if not self.token:
self.token = self.generate_secure_token()
super().save(*args, **kwargs)
Expand Down
18 changes: 18 additions & 0 deletions src/backend/core/tests/test_admin_selftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import pytest

from core.admin import ServiceAdminForm
from core.models import Service
from core.selftests import SelfTestResult

pytestmark = pytest.mark.django_db
Expand All @@ -27,6 +29,22 @@ def _override_storage_settings(settings):
}


def test_service_admin_form_enables_slug_on_creation():
"""The service slug must be explicit when adding a service in admin."""
form = ServiceAdminForm()

assert not form.fields["slug"].disabled


@patch("django.forms.models.model_to_dict", return_value={})
def test_service_admin_form_disables_slug_after_creation(_mock_model_to_dict):
"""The service slug must be read-only when editing an existing service in admin."""
service = Service(pk=1, name="Test Service", slug="testidx")
form = ServiceAdminForm(instance=service)

assert form.fields["slug"].disabled


def test_selftest_requires_authentication(client):
"""Test that the selftest page requires authentication."""
selftest_url = reverse("admin:selftest")
Expand Down
23 changes: 16 additions & 7 deletions src/backend/core/tests/test_models_services.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests Service model for find's core app."""

from django.core.exceptions import ValidationError
from django.db import DataError, IntegrityError

import pytest
Expand All @@ -9,18 +10,18 @@
pytestmark = pytest.mark.django_db


def test_models_services_name_unique():
"""The name field should be unique across services."""
def test_models_services_slug_unique():
"""The slug field must be unique across services."""
service = factories.ServiceFactory()

with pytest.raises(IntegrityError):
factories.ServiceFactory(name=service.name)
factories.ServiceFactory(slug=service.slug)


def test_models_services_name_slugified():
"""The name field should be slugified."""
service = factories.ServiceFactory(name="My service name")
assert service.name == "my-service-name"
def test_models_services_slug_rejects_non_alphanumeric():
"""Explicit non-alphanumeric slugs are rejected by the DB check constraint."""
with pytest.raises(IntegrityError):
factories.ServiceFactory(slug="has-hyphen")


def test_models_services_token_50_characters_exact():
Expand All @@ -39,3 +40,11 @@ def test_models_services_token_50_characters_more():
"""The token field should be 50 characters long."""
with pytest.raises(DataError):
factories.ServiceFactory(token="a" * 51)


def test_service_slug_immutable_after_creation():
"""The slug field must be immutable after creation."""
service = factories.ServiceFactory(slug="originalname")
service.slug = "differentname"
with pytest.raises(ValidationError):
service.save()
9 changes: 6 additions & 3 deletions src/backend/demo/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@

DEV_SERVICES = (
{
"name": "docs",
"slug": "docs",
"name": "Docs",
"client_id": "impress",
"token": "find-api-key-for-docs-with-exactly-50-chars-length",
},
{
"name": "drive",
"slug": "drive",
"name": "Drive",
"client_id": "drive",
"token": "find-api-key-for-driv-with-exactly-50-chars-length",
},
{
"name": "conversations",
"slug": "conversations",
"name": "Conversations",
"client_id": "conversations",
"token": "find-api-key-for-conv-with-exactly-50-chars-length",
},
Expand Down
6 changes: 3 additions & 3 deletions src/backend/demo/tests/test_commands_create_demo.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ def test_commands_create_demo(settings):
"""The create_demo management command should create objects as expected."""
call_command("create_demo")

assert models.Service.objects.exclude(name="docs").count() == 4
assert models.Service.objects.exclude(slug="docs").count() == 4
assert opensearch_client().count(index=settings.OPENSEARCH_INDEX)["count"] == 4

docs = models.Service.objects.get(name="docs")
docs = models.Service.objects.get(slug="docs")
assert docs.client_id == "impress"

drive = models.Service.objects.get(name="drive")
drive = models.Service.objects.get(slug="drive")
assert drive.client_id == "drive"
Loading