views:

239

answers:

2

I have a method on my user registration form that looks like this:

def save(self):
    user = User(
        username = self.cleaned_data['username'],
        email = self.cleaned_data['email1'],
        first_name = self.cleaned_data['first_name'],
        last_name = self.cleaned_data['last_name'],
    )
    user.set_password(self.cleaned_data['password1'])
    user.profile = Profile(
        primary_phone = self.cleaned_data['phone'],
    )
    user.profile.address = Address(
        country = self.cleaned_data['country'],
        province = self.cleaned_data['province'],
        city = self.cleaned_data['city'],
        postal_code = self.cleaned_data['postal_code'],
        street1 = self.cleaned_data['street1'],
        street2 = self.cleaned_data['street2'],
        street3 = self.cleaned_data['street3'],
    )
    user.save()
    return user

Problem is when I call form.save() it creates the user object as expected, but doesn't save his profile or address. Why doesn't it cascade and save all the sub-models? I suspect I could call user.profile.save() and user.profile.address.save() manually, but I want the whole thing to succeed or fail together. What's the best way to do this?


Current solution:

def save(self):
    address = Address(
        country = self.cleaned_data['country'],
        province = self.cleaned_data['province'],
        city = self.cleaned_data['city'],
        postal_code = self.cleaned_data['postal_code'],
        street1 = self.cleaned_data['street1'],
        street2 = self.cleaned_data['street2'],
        street3 = self.cleaned_data['street3'],
    )
    address.save()

    user = User(
        username = self.cleaned_data['username'],
        email = self.cleaned_data['email1'],
        first_name = self.cleaned_data['first_name'],
        last_name = self.cleaned_data['last_name'],
    )
    user.set_password(self.cleaned_data['password1'])
    user.save()

    profile = Profile(
        primary_phone = self.cleaned_data['phone'],
    )
    profile.address = address
    profile.user = user
    profile.save()

I had to make profile the "central" object. Needed to set profile.user = user rather than user.profile = profile to make it work (I guess because the key is on the profile model, not on the user model).


Newer solution:

I took a hint from this article suggested in this answer.

Now I have separated my model forms and moved the logic into the view:

def register(request):
    if request.POST:
        account_type_form = forms.AccountTypeForm(request.POST)
        user_form = forms.UserForm(request.POST)
        profile_form = forms.ProfileForm(request.POST)
        address_form = forms.AddressForm(request.POST)

        if user_form.is_valid() and profile_form.is_valid() and address_form.is_valid():
            user = user_form.save()
            address = address_form.save()
            profile = profile_form.save(commit=False)
            profile.user = user
            profile.address = address
            profile.save()
            return HttpResponseRedirect('/thanks/')
    else:
        account_type_form = forms.AccountTypeForm()
        user_form = forms.UserForm()
        profile_form = forms.ProfileForm()
        address_form = forms.AddressForm()

    return render_to_response(
        'register.html',
        {'account_type_form': account_type_form, 'user_form': user_form, 'address_form': address_form, 'profile_form': profile_form},
        context_instance=RequestContext(request)
    )

I'm not too fond of shifting the burden to the view, but I guess I get a bit more flexibility this way?

+1  A: 

It doesn't cascade-save because it doesn't actually know whether or not the other objects need to be saved.

To do it in one go, first start a transaction:

@transaction.commit_on_success
def save(self):
  ....

Then save the subobjects in order:

  user.profile.address.save()
  user.profile.save()
  user.save()
Ignacio Vazquez-Abrams
Why can't it figure out if they need to be saved? They don't even have `id`s yet... it's an easy check. Is it really necessary to use transactions for something so simple? I've had all sorts of problems with transactions masking other errors.
Mark
Having a set PK isn't enough to determine that an object shouldn't be saved. Either the PK may be forced or the object may need updating, both of which require a call to `save()`.
Ignacio Vazquez-Abrams
Oh... and the bigger problem is that `profile.user_id` cannot be null. `profile.user_id` is never set, even though `profile` is an attribute of `user`...
Mark
That would indeed pose a problem.
Ignacio Vazquez-Abrams
A big problem. I'm strongly opposed to having to set both `user.profile = profile` and `profile.user = user`.
Mark
You shouldn't have to. One is a field, and the other is a reverse relation. Only the actual field should have to be bound to a model that has a PK.
Ignacio Vazquez-Abrams
You're right :) Too bad you can't set either-or.
Mark
+1  A: 

The problem is that you're trying to create or update fields in a User object that doesn't even exist yet. So the other fields aren't really updated because they aren't associated to any primary keys of the child fields.

Every time you're instantiating a new model field, you have to make sure you're saving so that a child model field has an id (primary key) to associate with.

You need something more like this:

def save(self):
    user = User(
        username = self.cleaned_data['username'],
        email = self.cleaned_data['email1'],
        first_name = self.cleaned_data['first_name'],
        last_name = self.cleaned_data['last_name'],
    )
    ## save user so we get an id
    user.save()

    ## make sure we have a user.id
    if user.id:
        ## this doesn't save the password, just updates the working instance
        user.set_password(self.cleaned_data['password1'])
        user.profile = Profile(
            primary_phone = self.cleaned_data['phone'],
        )
        ## save the profile so we get an id
        user.profile.save()

    ## make sure we have a profile.id
    if user.profile.id:
        user.profile.address = Address(
            country = self.cleaned_data['country'],
            province = self.cleaned_data['province'],
            city = self.cleaned_data['city'],
            postal_code = self.cleaned_data['postal_code'],
            street1 = self.cleaned_data['street1'],
            street2 = self.cleaned_data['street2'],
            street3 = self.cleaned_data['street3'],
        )
        ## save the profile address
        user.profile.address.save()

    ## final save to commit password and profile changes
    user.save()
    return user

This cascading save() thing you have going on here just doesn't feel right. You're prone to way too many errors there, if any of the fields doesn't save you'll end up with a partially complete user instance and posisbly end up with duplicates if the user has to go back and try again. Not fun!

Edit: Removed the 2nd half of this because it was not accurate.

jathanism
It looks better because you didn't post the internals of the UserFormSet, you posted the view method, which looks more-or-less identical to what I have now. I'll have to look into formsets deeper to see if they're the right choice. I always thought of them as being used for creating many objects at once, not many forms for creating one object. My cascading idea was aimed to prevent duplicates by having it all save or fail together, hence having only one call to `save()`.
Mark
Yeah, you're right about that. I don't know what I was thinking! I will remove that latter half from the answer.
jathanism