How to implement pre-save operations for inlines in the admin interface?

[Carsten Fuchs]

Hi all,

using Django 1.3 (soon 1.4), we use models like these:


class Staff(models.Model):
name = models.CharField(max_length=80)


class Period(models.Model):
staff = models.ForeignKey(Staff)

begin = models.DateField()
end = models.DateField(editable=False) # see below

# The actual data that describes this period,
# e.g. department, salary, ...
department = models.CharField(max_length=60)

class Meta:
ordering = ['begin']


There are normally several Period instances for each staff member, and
the set of periods for one staff member form a chronological sequence:

A period begins at the day given in 'begin'.
It ends at the day before the *next* period begins.

Thus, in our original design, there was no field 'end' in the Period
model, because logically, it was not needed. (The last period is
"open-ended".)

However, we found that many queries are very difficult to build without
the 'end' (such as "Find all staff members in department X on November
1st"), and thus are planning to add it as shown above.

'end' should always be computed automatically as the day before the
begin of the next period, but I don't know where this is best
implemented in the admin interface:


class PeriodInline(admin.TabularInline):
model = Period

class StaffAdmin(admin.ModelAdmin):
inlines = [ PeriodInline ]


(Note that it is not enough to consider the Period that changed --
rather, it is the 'end' in *another* period (the one "before" it) that
must be modified and saved, too.)

What is the proper hook or where the best place to do this?

Many thanks in advance, and best regards,
Carsten



--
Cafu - the open-source Game and Graphics Engine
for multiplayer, cross-platform, real-time 3D Action
Learn more at http://www.cafu.de

[Arnold Krille]

Why do you want to do this only in the admin interface?
Its a generic thing: every time you save/change a period you should set the
end-date of the previous one. So I would do this with a post-save hook (aka
signal) directly in the models.py. Or maybe even subclass your models save-
operation.

Have fun,

Arnold

[Carsten Fuchs]

Hi Arnold,

Am 15.11.2012 17:52, schrieb Arnold Krille:
> On Thursday 15 November 2012 17:12:09 Carsten Fuchs wrote:

>> [...]

>> (Note that it is not enough to consider the Period that changed --
>> rather, it is the 'end' in *another* period (the one "before" it) that
>> must be modified and saved, too.)
>>
>> What is the proper hook or where the best place to do this?
>
> Why do you want to do this only in the admin interface?
> Its a generic thing: every time you save/change a period you should set the
> end-date of the previous one. So I would do this with a post-save hook (aka
> signal) directly in the models.py. Or maybe even subclass your models save-
> operation.

Well, I realize that this would be useful also generically, outside of
the admin interface.

Alas, if e.g. I overrode the save() method of the Period class, is it
safe to access *another* Period instance from there?

I don't really fully understand how the admin interface works when a
model with inlines is saved, but I suspect that the inlines are saved in
a loop. But if in an early iteration of the loop I modified another
instance that is routinely saved *again* in a later iteration of the
loop, it will be a bug.

Thus my original question. ;-)

Best regards,

[Arnold Krille]

Hi,

On Thu, 15 Nov 2012 18:15:37 +0100 Carsten Fuchs
<carste...@cafu.de> wrote:
> Am 15.11.2012 17:52, schrieb Arnold Krille:
> > On Thursday 15 November 2012 17:12:09 Carsten Fuchs wrote:
> >> [...]
> >> (Note that it is not enough to consider the Period that changed --
> >> rather, it is the 'end' in *another* period (the one "before" it)
> >> that must be modified and saved, too.)
> >> What is the proper hook or where the best place to do this?
> > Why do you want to do this only in the admin interface?
> > Its a generic thing: every time you save/change a period you should
> > set the end-date of the previous one. So I would do this with a
> > post-save hook (aka signal) directly in the models.py. Or maybe
> > even subclass your models save- operation.
> Well, I realize that this would be useful also generically, outside
> of the admin interface.

Even if you do not need it outside the admin-interface (in this
project), from the logic this belongs to the model, not into some view.

> Alas, if e.g. I overrode the save() method of the Period class, is it
> safe to access *another* Period instance from there?

Yep, thats save.

If you use a db that has transactions, both the change to the other
object should be rolled back when saving of the current objects fails
as it should all end up in one transaction. :-)

> I don't really fully understand how the admin interface works when a
> model with inlines is saved, but I suspect that the inlines are saved
> in a loop. But if in an early iteration of the loop I modified
> another instance that is routinely saved *again* in a later iteration
> of the loop, it will be a bug.

I can't really comment on the admin interfaces working as we use that
only for the stuff only a superuser has rights. Everything else is
edited in our frontend-code.
But saving inlines in the admin interface should be the same as saving
forms with formsets underneath.

Have fun,

Arnold

[Mike Dewhirst]

On 16/11/2012 3:52am, Arnold Krille wrote:
> Why do you want to do this only in the admin interface?
> Its a generic thing: every time you save/change a period you should set the
> end-date of the previous one. So I would do this with a post-save hook (aka
> signal) directly in the models.py. Or maybe even subclass your models save-
> operation.

This is a question not a competing opinion.

Why would you use a post-save signal? Why not just override save() and
use the model manager directly to find the previous period and if it
doesn't have and end date pop one in?

Thanks

Mike

[Arnold Krille]

I don't have the ultimate answer.

We have a case here where we apply one function to several models, on
one model on pre-save-signal and on several models on post-save-signal.
We could have done the same by subclassing all these models from one
abstract base which would have had one field and the save-function. The
signals seemed easier to us with less clutter in the model-graph.

For the case presented by Carsten, I am even more open to do it with
overwriting the save-function.
With the save-function you have the functionality directly where it
belongs.
With a post-save-signal you have maybe a bit cleaner kind of
code: the save-function of the object is only the save-function of the
object. The modification of a different object is in a different
function that has its execution-definition written directly above.

But its a matter of personal taste I think.

Have fun,

Arnold

[Mike Dewhirst]

Thanks Arnold - makes sense.

Cheers

Mike

[Carsten Fuchs]

Hi Arnold,

Am 15.11.2012 21:29, schrieb Arnold Krille:
> Even if you do not need it outside the admin-interface (in this
> project), from the logic this belongs to the model, not into some view.

Thank you very much for your replies, and in this regard (that the logic
would much better be kept in the model rather than elsewhere), I'm fully
with you.

However, I don't think that it works!

Please note that changing the 'begin' of a single period can imply
updates to several of them: the one before it in its old place (changed
'end' date), and the one before it in its new place (also changed 'end'
date).

So an implementation that updates all Periods of one staff member like
this seems to be reasonable:

end_date = date(2999, 12, 31) # or e.g. None

# Iterate the periods in *reverse* order:
for p in StaffMember.period_set.all().order_by('-begin'):
if p.end != end_date:
p.end = end_date
p.save()

end_date = p.begin

However, if this was moved into a save() override of Period, it would
introduce the problem of infinite recursion (this could be solved), but
it would *also* conflict with the save-loop in the admin interface,
please see the next paragraph:

>> Alas, if e.g. I overrode the save() method of the Period class, is it
>> safe to access *another* Period instance from there?
>
> Yep, thats save.
>
> If you use a db that has transactions, both the change to the other
> object should be rolled back when saving of the current objects fails
> as it should all end up in one transaction. :-)

I guess that when the admin interface saves a set of inlines, it has a
Queryset of them over which it loops and calls save(), e.g.

for p in InlinePeriodsQueryset:
p.save()

Now imagine that in the first iteration of this loop, p.save() calls or
has code like the update code above, where the *other* Periods of the
Staff member are updated, then enters the second iteration of this loop.

If my understanding is correct, this will overwrite a Period with
stale/outdated information, causing conflicts, won't it?

Thus my original question where to hook into the admin interface.

Despite your and my preference to have this generically work on the
level of model code, this seemingly simple updating of Period instances
seems best to be done *after* the admin interface has completed saving them.
So in fact, keeping these steps (admin saves all inlines, then update
them all as shown above) separate seems like a good idea to me, but I
still wonder where I should hook into the admin in order to call the
update code above?

[Martin J. Laubach]

  Don't overthink it.

  If believe you are likely to run into cascading recursive updates, then those cascades will happen no matter where you put the update. You will need to deal with it some way or another (locking, careful ordering, better data structures, whatever), but just moving things around will not automatically clean up the mess :-)

[Carsten Fuchs]

Uh, why? I don't try to move things around, but just to separate them to
remove any complexity:

I want to have the admin interface save the inline models, and when that
is done, post-process / update them as required.

The only key piece that I'm missing is where to hook into the admin for
that purpose?

The documentation mentions several methods that look promising, e.g.

ModelAdmin.save_model()
ModelAdmin.save_formset()
ModelAdmin.save_related()

but unfortunately says nothing about their purpose, or the context or
the order that they're called in.

[Carsten Fuchs]

Just for info, to complete this:

I read in the admin interface source code (contrib/admin/options.py) and
found a solution myself. It's not as clear and straightforward as one
would wish, but simple and robust:

We now simply override the `ModelAdmin.save_formset()` function like this:


def save_formset(self, request, form, formset, change):
if formset.model == Period:
# Save normally.
instances = formset.save()

if instances:
# At least *one* period has changed and was saved,
# so check/update *all* periods of that staff member.
instances[0].staff.UpdateAllPeriods()
else:
# The default implementation of save_formset():
formset.save()


(The ugly trick is the `instances[0].staff` to obtain the parent object.)

Function `UpdateAllPeriods()` simply loops over the `period_set` of the
Staff instance and updates the periods as required.

:-)