Opened 3 years ago

Closed 3 years ago

#19553 closed Cleanup/optimization (wontfix)

Please make save_instance and construct_instance methods of ModelForm

Reported by: gcc Owned by: nobody
Component: Forms Version: 1.4
Severity: Normal Keywords:
Cc: charette.s@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I want to use ModelForm and Generic Class-based Views to create records for an ordering/payment system (so security is important). I want to set the user field of the created record to the current logged-in user, without any way for them to override it.

I asked the best way to do this on Stack Overflow, and I didn't get any satisfactory (non-ugly) answers of how to do this. The essential problem is that:

  • CBVs have access to the request, but not the instance;
  • ModelForm has no access to the request (but can be given it) and only has access to the model after it's been instantiated;
  • Model has no access to the request, and its attributes are magical.

The closest to a "nice" solution was the only answer I got so far: construct the ModelForm passing in the request, and modify the object before saving it. However ModelForm has no access to the object until it's saved, unless it does a save(commit=False) and then saves the object and its m2m relations, which is also ugly.

The save_instance function is not a method of ModelForm, but it does poke a new method into the ModelForm which called it, and that's pretty ugly too:

    else:
        # We're not committing. Add a method to the form to allow deferred
        # saving of m2m data.
        form.save_m2m = save_m2m

I propose making save_instance and construct_instance methods of ModelForm, instead of stand-alone functions that do black magic on a ModelForm. This way, I could override construct_instance to modify the instance before it's saved, setting its user member to the one in the request that I saved earlier, and that's the cleanest approach that I've seen so far (minimal code, minimal magic).

Change History (3)

comment:1 Changed 3 years ago by nkryptic@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

What about if BaseCreateView was a subclass of BaseUpdateView and overrode the get_object() method to return None:

class BaseCreateView(BaseUpdateView):
    """
    Base view for creating an new object instance.

    Using this base class requires subclassing to provide a response mixin.
    """
    def get_object(self):
        return None

Then, it would be easy, in a subclass, to have get_object() return an new and unsaved instance of your model - populated as you see fit - before it is passed to the form via the instance kwarg:

class OrderView(CreateView):
    def get_object(self):
        return Order(user=self.request.user)

I think this is an easier approach to hooking into the process and I think would be a welcome enhancement of CreateView.

comment:2 Changed 3 years ago by charettes

  • Cc charette.s@… added

You can achieve this by overriding form_valid:

class OrderCreateViewMixin(CreateView):
    def form_valid(self, form):
        form.instance.user = request.user
        return super(OrderCreateViewMixin, self).form_valid(form)

comment:3 Changed 3 years ago by gcc

  • Resolution set to wontfix
  • Status changed from new to closed

Thanks charettes, that's an excellent idea.

Now that you suggested it, I found the documentation that proposes almost exactly this use case.

Note: See TracTickets for help on using tickets.
Back to Top