-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
Conversation
d621be4
to
35c4c20
Compare
@@ -114,8 +114,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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4829 +/- ##
=======================================
Coverage 96.60% 96.60%
=======================================
Files 752 752
Lines 25809 25809
Branches 3411 3411
=======================================
Hits 24932 24932
Misses 613 613
Partials 264 264 ☔ View full report in Codecov by Sentry. |
35c4c20
to
cae00db
Compare
cae00db
to
2bd739a
Compare
the name is set via translations
2bd739a
to
6b4fbed
Compare
Related to #3283
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh
./bin/compilemessages_js.sh
Commit hygiene