djangocms-history icon indicating copy to clipboard operation
djangocms-history copied to clipboard

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

Open AnnaDamm opened this issue 6 years ago • 8 comments

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

  • Python version: 3.5
  • Django version: 1.11.11
  • django CMS version: 3.4.5
  • django CMS History version: 0.5.3

AnnaDamm avatar Mar 08 '18 16:03 AnnaDamm

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

AnnaDamm avatar Mar 08 '18 16:03 AnnaDamm

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

haricot avatar May 07 '18 23:05 haricot

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

AnnaDamm avatar May 08 '18 07:05 AnnaDamm

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

haricot avatar May 08 '18 14:05 haricot

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.

bplociennik avatar Mar 05 '19 10:03 bplociennik

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.

haricot avatar Mar 05 '19 12:03 haricot

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?

bplociennik avatar Mar 06 '19 13:03 bplociennik

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.

haricot avatar Mar 07 '19 14:03 haricot