djangocms-history
djangocms-history copied to clipboard
Error when undoing changes on a JSONField (escaped quotes in result)
Summary
- Create a plugin that uses a JSONField (https://github.com/dmkoch/django-jsonfield)
- Add the plugin anywhere
- Fill the field with any json, save
- change the contents of the json, save
- undo via django history
Expected behaviour
The contents of the database field are the same as before
Actual behaviour
There is escaping added to special characters in the database field.
For example:
{"foo": "bar"}
becomes:
"{\"foo\": \"bar\"}"
which is valid json, but now a string instead of an object.
I found out that it already adds the action wrong to the placeholderaction table. It adds it already as a string into that table.
Environment
- Python version: 3.5
- Django version: 1.11.11
- django CMS version: 3.4.5
- django CMS History version: 0.5.3
I suppose it might as well be a bug in the JSONField or the django serializer (after digging into the code), so I created a bug ticket there: dmkoch/django-jsonfield/issues/216
@Arany, does this PR haricot/djangocms-history#1 solve your problem or in part?
@haricot: I have not tested it yet. But it seems to me from the code that this only works with djangocms-cascade. It will not work with other plugins with a JSONField.
@Arany, and this PR haricot/djangocms-history#2 ? I have not yet run the history tests of django-cms.
Thanks, @haricot for your PRs I checked them and:
- haricot#1 is not working,
- haricot#2 is working but not for other cases,
- https://github.com/divio/djangocms-history/pull/22 - is working me with https://github.com/dmkoch/django-jsonfield as a field in the plugin.
but when I dig deeper I found some issues:
local variable 'jsonfield_to_dict' referenced before assignment
so you should check if is in the locals or declare at the beginning:
return value if is_protected_type(value) or jsonfield_to_dict else field.value_to_string(obj)
details:
I checked this with ckeditor and is the problem when I'm trying to undo changes for my text plugin. It doesn't handle it in a proper way because https://github.com/divio/djangocms-history/blob/master/djangocms_history/action_handlers.py#L112 doesn't have plugin.data value
:
ipdb> plugin.data
{'body': None}
ipdb> plugin.model.objects.filter(pk=plugin.pk).update(**plugin.data)
*** django.db.utils.IntegrityError: NOT NULL constraint failed: djangocms_text_ckeditor_text.body
Except this one is working correctly with JsonField.
I didn't check with djangocms-cascade but based on code I believe is working good.
Thanks @vThaian The local variable error referenced before the assignment you found at, I think, has been fixed in: 4ab688e I can do you the same pull request for djangocms_transfer/helpers.py#L47 which also requires this modification.
However, your proposal doesn't work with TextPlugin properly. Could you resolve the problem with the missing plugin.data
value mentioned above or should I take care of this?
These new commits solves the problem ? #22
https://github.com/divio/djangocms-history/blob/c5d1d9daec74167a5cd74cf9cefa611428758fa2/djangocms_history/helpers.py#L74-L84
if the filed "filed" is not there, custom_data = _plugin_data
I did not have a problem with the Text Plugin but by testing this with djangocms-installer and bootstrap4 list for example, there was a similar problem.