Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🗑️ [#3283] Make FormDefinitionSerializer.name read only #4829

Merged
merged 1 commit into from
Dec 6, 2024
Merged

🗑️ [#3283] Make FormDefinitionSerializer.name read only #4829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/installation/upgrade-300.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,15 @@ The legacy format (from before Open Forms 2.3) for registration backend will no
converted to the current standard. When importing a form with this configuration,
the form will be created without registration backends.

``FormDefinition.name`` field is now read only
----------------------------------------------

The ``name`` field of a ``FormDefinition`` export is no longer written to the matching
active locale field (``name_nl`` or ``name_en``) during imports. Instead, the
``translations`` key is used. This affects forms that were exported before the
``translations`` key existed. We recommend re-creating the exports on a newer version
of Open Forms.

Removal of /api/v2/location/get-street-name-and-city endpoint
=============================================================

Expand Down
6 changes: 3 additions & 3 deletions src/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7825,7 +7825,7 @@ components:
format: uuid
name:
type: string
maxLength: 50
readOnly: true
internalName:
type: string
description: internal name for management purposes
Expand Down Expand Up @@ -7876,7 +7876,7 @@ components:
format: uuid
name:
type: string
maxLength: 50
readOnly: true
internalName:
type: string
description: internal name for management purposes
Expand Down Expand Up @@ -9383,7 +9383,7 @@ components:
format: uuid
name:
type: string
maxLength: 50
readOnly: true
internalName:
type: string
description: internal name for management purposes
Expand Down
3 changes: 1 addition & 2 deletions src/openforms/forms/api/serializers/form_definition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ class Meta:
"view_name": "api:formdefinition-detail",
"lookup_field": "uuid",
},
# TODO: enable this in v3, deprecate writing this field
# "name": {"read_only": True}, # writing is done via the `translations` field
"name": {"read_only": True}, # writing is done via the `translations` field
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sergei-maertens do I understand it correctly that this field is currently no longer used for writing? I was wonder whether the import code needs to add the name in translations if it doesn't exist. Or can we assume that imports have these translations already?

Copy link
Member

Choose a reason for hiding this comment

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

indeed - when we added translations we forgot that the 'main' field could also still be written to and that results in writes happening with the currently active locale (so if you have the UI in english, it would write to name_en).

Things didn't break so far because the translations are defined after the name field, so those overwrite the name_en and name_nl fields again, but that was not deliberately and works completely by accident.

You can indeed assume the imports have translations already, and if they don't, well, it's one of the things to mention in the 3.0 release notes and a reason why this was postponed until a major version :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a changelog entry for it

Copy link
Member

Choose a reason for hiding this comment

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

Probably better to mention it in the dedicated docs page: https://github.com/open-formulieren/open-forms/blob/master/docs/installation/upgrade-300.rst

"slug": {
"required": False,
},
Expand Down
4 changes: 4 additions & 0 deletions src/openforms/forms/tests/admin/test_form.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ def test_form_admin_import_feedback_messages(self):
}
]
},
"translations": {
"en": {"name": "testform"},
"nl": {"name": "testformulier"},
},
}
]
).encode("utf-8")
Expand Down
6 changes: 5 additions & 1 deletion src/openforms/forms/tests/exports/formDefinitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
"internal_name": "Test Definition Internal 1",
"slug": "test-definition-1",
"url": "http://testserver/api/v2/form-definitions/f0dad93b-333b-49af-868b-a6bcb94fa1b8",
"uuid": "f0dad93b-333b-49af-868b-a6bcb94fa1b8"
"uuid": "f0dad93b-333b-49af-868b-a6bcb94fa1b8",
"translations": {
"en": {"name": "Test Definition 1"},
"nl": {"name": "Test Definitie 1"}
}
}
]
5 changes: 4 additions & 1 deletion src/openforms/forms/tests/test_api_form_versions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime

from django.urls import reverse
from django.utils import translation

from freezegun import freeze_time
from rest_framework import status
Expand Down Expand Up @@ -191,7 +192,9 @@ def test_restore_version(self):
form_step = form_steps.get()
restored_form_definition = form_step.form_definition

self.assertEqual("Test Definition 1", restored_form_definition.name)
with translation.override("en"):
self.assertEqual("Test Definition 1", restored_form_definition.name)
self.assertEqual("Test Definitie 1", restored_form_definition.name_nl)
self.assertEqual(
"Test Definition Internal 1", restored_form_definition.internal_name
)
Expand Down
14 changes: 12 additions & 2 deletions src/openforms/forms/tests/test_api_formdefinition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,19 @@ def test_update(self):
],
},
"login_required": True,
"translations": {
"en": {"name": "Updated name"},
"nl": {"name": "Bijgewerkte naam"},
},
},
)

self.assertEqual(response.status_code, status.HTTP_200_OK)

definition.refresh_from_db()

self.assertEqual("Updated name", definition.name)
self.assertEqual("Bijgewerkte naam", definition.name)
self.assertEqual("Updated name", definition.name_en)
self.assertEqual("updated-slug", definition.slug)
self.assertEqual(True, definition.login_required)
self.assertIn(
Expand Down Expand Up @@ -217,14 +222,19 @@ def test_create(self):
}
],
},
"translations": {
"en": {"name": "Name"},
"nl": {"name": "Naam"},
},
},
)

self.assertEqual(status.HTTP_201_CREATED, response.status_code)

definition = FormDefinition.objects.get()

self.assertEqual("Name", definition.name)
self.assertEqual("Naam", definition.name)
self.assertEqual("Name", definition.name_en)
self.assertEqual("a-slug", definition.slug)
self.assertEqual(
[
Expand Down
5 changes: 4 additions & 1 deletion src/openforms/forms/tests/test_restore_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import json

from django.test import TestCase
from django.utils import translation
from django.utils.translation import gettext as _

from freezegun import freeze_time
Expand Down Expand Up @@ -63,7 +64,9 @@ def test_restoring_version(self):

restored_form_definition = form_steps.get().form_definition

self.assertEqual("Test Definition 1", restored_form_definition.name)
with translation.override("en"):
self.assertEqual("Test Definition 1", restored_form_definition.name)
self.assertEqual("Test Definitie 1", restored_form_definition.name_nl)
self.assertEqual(
"Test Definition Internal 1", restored_form_definition.internal_name
)
Expand Down
Loading