order_insertion_by filters should be calculated after pre_save() methods #175

Closed
craigds opened this Issue Mar 7, 2012 · 6 comments

Projects

None yet

3 participants

@craigds
Collaborator
craigds commented Mar 7, 2012

At present we figure out where to insert new nodes in get_ordered_insertion_target(), which gets called from save() before the superclass's save() gets called.

This is kind of bad because it means field.pre_save() hasn't yet been called for the relevant fields.

As a result, specifying a datetime field with auto_add_now=True in insertion_order_by will not currently work, because the value is not set until field.pre_save() is called. Reported in this SO post: http://stackoverflow.com/questions/9596115/how-to-order-django-mptt-tree-by-datetimefield

@craigds craigds was assigned Mar 7, 2012
@movielady

I can confirm that this causes problems not just when using an auto_add_now in order_insertion_by, but also when you have any auto_now fields in the model. None of the date fields I have in my model are being used as order_insertion_by fields, and I get the same error as indicated on the SO post. The workaround doesn't work for me.

@dokterbob

Have confirmed this problem as well with an automatically filled Integer ordering field.

@craigds
Collaborator
craigds commented May 11, 2013

@dokterbob how is the integer field auto-filled? Can you post your model definition?

@craigds craigds added a commit that closed this issue May 11, 2013
@craigds craigds fix usage of date fields with auto_now_add in order_insertion_by. @do…
…kterbob / @movielady please confirm this fixes the problem (closes #175)
c64ef1d
@craigds craigds closed this in c64ef1d May 11, 2013
@dokterbob

Will look at the patch on monday maybe, rest of the week will be Djangocon. :)

@craigds
Collaborator
craigds commented May 12, 2013

Maybe I'll see you there

@dokterbob

The patch didn't solve the issue for me.

Setting the ordering attribute from the clean() method as recommended by the Django docs versions did, however. Perhaps we should have a word about this in the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment