Default manager should never return unapproved objects

[adityar7]

Since there is no explicit field in ModeratedObject that stores whether the object has ever been approved, use the moderation_date field which is always set when an object is approved.

[dominno]

Hi @adityar7

Tests are not passing. https://travis-ci.org/dominno/django-moderation/jobs/13803312

All pull requests must include tests. Please follow the https://github.com/dominno/django-moderation/blob/master/CONTRIBUTING.rst

[coveralls]

Coverage decreased (-0.14%) when pulling ed85a3e60bc71b2bab37397230ba1eccc9952409 on adityar7:master into b357fec29011c7c973a9f75722caef219e5d817f on dominno:master.

[adityar7]

Oops, sorry! I don't know how I forgot to run tests the last time.

Updated the tests to reflect the change. I don't think the change requires any new test cases.

[coveralls]

Coverage remained the same when pulling 15ec40d909a3a87e8f94778e670aac1e594183fe on adityar7:master into b357fec29011c7c973a9f75722caef219e5d817f on dominno:master.

[adityar7]

Found another subtle bug: he current code (without my pull request) uses obj_changed to essentially determine whether the object is in Pending status but has been approved before, in which case it should not be excluded. However, if a field is in fields_exclude then changing that field makes obj_changed false and the object gets excluded from the manager even if has been approved.

Removed the obj_changed condition completely. Using moderation_date is sufficient and covers all cases.

[dominno]

There is field that tells status of object, moderation_status. Your approach will only work for the first time when object is new. When object exist and was approved before and when its changed again then changes will not be detected and your solution will not work.

In future visibility_column option on GenericModerator will be used by default for detecting which objects should be excluded from query.

[adityar7]

I believe my approach works even for objects that have been edited after being approved, but I suppose that depends on what the "correct" or expected behavior of the manager is.

What I can tell from the docs and from usage scenarios is that the manager should return objects that are either in moderated (approved/rejected) status, or those that have been edited after being moderated (if the object has been moderated in the past and then changed, the actual object returned to the caller will be the moderated object and not the changed object). In other words, the manager should only exclude objects that are in pending state and have never been moderated in the past. Is that correct? If not, what is the expected behavior?

By the way, currently the code is also excluding objects that are in rejected status and have not changed, but is including rejected objects that have changed. Why? I don't even know if an object can still be in rejected status after it is changed, since the post-save signal handler will change it to pending status.

If the behavior I described above is correct, we should just need:

if mobject.moderation_status == MODERATION_STATUS_PENDING and not mobject.moderation_date:
    exclude_pks.append(obj.pk)

[dominno]

@adityar7 I will check this during the weekend

[coveralls]

Coverage increased (+0.12%) when pulling 1be8adce55df296460d08f187d38fd7d3832e438 on adityar7:master into b357fec29011c7c973a9f75722caef219e5d817f on dominno:master.

[dominno]

Hi @adityar7

I have tested this and you are right this will be working. Merging. Thank you. Good job!