Excluded fields should be saveable to the object without moderation

[adityar7]

We have a fields that store meta-data about the object. We've added these fields to fields_exclude so that the moderation state doesn't change to Pending when these fields are modified by the system.

Now let's say we update one of these excluded fields and some non-excluded fields and save and approve the object. The non-excluded field gets updated but the excluded one doesn't. Same thing if you update only the excluded field and save and approve (although this scenario is less usual in practice but certainly possible).

Here's an example in ./manage.py shell using the included example_project. fields_exclude = ['url'] has been added to UserProfileModerator.

from example_app.models import *
u = CustomUser.objects.create(first_name='john')
e = ExampleUserProfile.objects.create(user=u, description = 'hello')
e.moderated_object.approve()
e = ExampleUserProfile.objects.get(id=e.id)
e.url # Empty

# Update both excluded and non-excluded field.
e.description = 'goodbye'
e.save()
e.url = 'http://www.google.com'
e.save()
e.moderated_object.approve()
e = ExampleUserProfile.objects.get(id=e.id)
e.description # goodbye
e.url # Still empty

# Now update only the excluded field.
e.url = 'http://www.github.com'
e.save()
e.moderated_object.approve()
e = ExampleUserProfile.objects.get(id=e.id)
e.url # Still empty

# Works fine without moderating.
e.url = 'http://www.bitbucket.org'
e.save()
e = ExampleUserProfile.objects.get(id=e.id)
e.url # http://www.bitbucket.org

Surely the excluded fields should be saveable to the database without affecting moderation, since such fields are typically going to be fields that store meta-data about the objects, such as modified time or number of views.

In fact, this issue basically makes excluded fields useless since they don't update properly.

[dominno]

Hi @adityar7

Right now i dont have time to check this. Can you check if solution in this PR would help with your problem https://github.com/dominno/django-moderation/pull/83

I will check this during the weekend. Changing this part is very tricky could broke other things, need more time to check this.

Dominik.

[adityar7]

Dominik,

Updated the issue description with more precise information.

The solution in #83 doesn't help with this (that guy and I are part of the same team). We've made some progress and finally managed to fix the case where both excluded and non-excluded fields are updated, but not the case where only the excluded field is updated.

Our solution modifies the pre_save_handler to add any excluded fields to the unchanged_obj. At least this allows us to move forward with our project since we don't use the other case, but we'll still try to fix it since it leaves room for lots of errors in the future.

[adityar7]

The pull request fixes both cases including the one where only the excluded field is updated.

I think there are several ways to fix this issue. Some of them will require changing the code significantly and we've avoided them for now given how easy it is to introduce errors into such a complex flow.

[dominno]

Hi @adityar7 Please fix PEP8 rules, you can use autopep8 tool https://pypi.python.org/pypi/autopep8/

$ pep8 --exclude=migrations --ignore=W291 moderation tests moderation/register.py:146:56: E502 the backslash is redundant between brackets moderation/register.py:147:52: E502 the backslash is redundant between brackets moderation/register.py:195:80: E501 line too long (84 > 79 characters) moderation/register.py:199:80: E501 line too long (80 > 79 characters)

As for the solution, i need more time to review this. For some reason its working in admin without your change, but i reproduced it in shell. Need more time to review.

[adityar7]

Fixed pep8.

Oh, I should point out that in the "Update both excluded and non-excluded field" case the bug only happens if you save the included and excluded separately:

# Bug
e.description = 'goodbye'
e.save()
e.url = 'http://www.google.com'
e.save()

# No bug
e.description = 'goodbye'
e.url = 'http://www.google.com'
e.save()

When you use the admin, you will be doing the second way by default, which is probably why you can't reproduce it.

[coveralls]

Coverage increased (+0.21%) when pulling 1f4f6c2fb88a416bcc2858bb7a67411de249c328 on adityar7:excluded into d523f89dc4f5deaa308b5f165184560bfe664535 on dominno:master.

[dominno]

@blag Can you rebase also this one. Thank you

[blag]

The one commit in this PR was already merged as the second commit in #148 - see the second commit on this page. The test from this PR is also included there and is passing Travis tests.

So this PR can simply be closed. 👍