django-cms / djangocms-history

Provides undo/redo functionality for django CMS operations
https://www.django-cms.org
Other
28 stars 13 forks source link

Error when undoing changes on a JSONField (escaped quotes in result) #20

Open AnnaDamm opened 6 years ago

AnnaDamm commented 6 years ago

Summary

  1. Create a plugin that uses a JSONField (https://github.com/dmkoch/django-jsonfield)
  2. Add the plugin anywhere
  3. Fill the field with any json, save
  4. change the contents of the json, save
  5. 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

AnnaDamm commented 6 years ago

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

haricot commented 6 years ago

@Arany, does this PR haricot/djangocms-history#1 solve your problem or in part?

AnnaDamm commented 6 years ago

@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.

haricot commented 6 years ago

@Arany, and this PR haricot/djangocms-history#2 ? I have not yet run the history tests of django-cms.

bplociennik commented 5 years ago

Thanks, @haricot for your PRs I checked them and:

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.

haricot commented 5 years ago

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.

bplociennik commented 5 years ago

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?

haricot commented 5 years ago

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.