Testing a model form not raising validation error

Hi all

I am slowly chipping away at a task manager app, and I’ve come across a testing issue that I cannot explain.

By way of background, I have a Task model that stores tasks, including the creator (a FK to a custom User model) of each task. By default, tasks are visible to all users and the model also has a boolean field named ‘private’ that if true means that task private to the creator. (The actual logic to check that is part of a custom QuerySet manager on the model and called from the generic CBV views.) I have a TaskForm (a model form relying on the Task model, used for creating and updating tasks). At the moment, any user can edit a task that is not private, including making a task private - even tasks that were created by another user.

I’ve made some changes to stop a user from making other creator’s tasks private.

In views.py, I’ve over-ridden get_form_kwargs() to pass the logged-in user id from the session, so it is accessible in the form. (All pages require the user to be logged in.)

class TaskUpdateView(UpdateView):

    model = Task
    form_class = TaskForm
    template_name = "tasks/task_detail.html"
    success_url = reverse_lazy("tasks:list")

    def get_form_kwargs(self):
        kwargs = super(TaskUpdateView, self).get_form_kwargs()
        kwargs['user_id'] = self.request.session.get("_auth_user_id")
        return kwargs

In forms.py, I’ve overridden __init__() to bring the logged-in user id from the view and overridden clean_private() with the logic to check whether the form instance matches the logged-in user from the session. I have opted to override the cleaning of only the private field, rather than the whole clean() method, since this logic only applies when that field is changing in the form.

class TaskForm(forms.ModelForm):

    def __init__(self, *args, **kwargs):
        self.user = kwargs.pop("user_id", None)
        super(TaskForm, self).__init__(*args, **kwargs)

    def clean_private(self):
        private = self.cleaned_data["private"]
        user_id = int(self.user)
        creator_id = self.instance.creator.id
        if user_id != creator_id:
             raise ValidationError(
                  "As you did not create this task, you cannot set it private."
             )
        return private

If the creator and current user do match, the form is processed and the update proceeds. If they don’t match, a ValidationError is raised and the form re-rendered with the error message in the template.

This all works, as expected, and if I test it in the browser, an error is rendered as a form.field error.

Unfortunately, when I try to test this with Django’s testing system, I cannot get the Validation Error to trigger the test.

def setUp(self):
       ... [sets up two users] ...
       ... [logs in as test_user_1] ...
       ... [instantiates a task created by test_user_2] ...

       self.task_form_data = {
            "creator": self.test_user_2,
            "text": "Basic test task",
            "private": "True",
        }

       def test_user_tries_to_change_another_creators_task_to_private_post_request(self):
            with self.assertRaises(ValidationError):
                   response = self.client.post(
                       path=reverse("tasks:edit", args=[self.task.pk]), data=self.task_form_data, follow=True
                   )

I’ve checked the content of the response, which includes the form correctly re-rendered and a status of 200. That response includes the error message in the right place in the template, which suggests the Validation Error was triggered but not picked up in the test.

I’ve tried making follow=False and I’ve tried replacing the Validation Error in the form with add_error() (and I also have the template set up to catch non-field errors), but the test still fails.

So, two questions:

  1. Any suggestions on why the test isn’t passing?
  2. Are there better ways to approach this problem?

I don’t know about the test itself but looking at the view and form code… it seems odd to me to create a form kwarg based on the session data, when you have self.request.user in the view already.

I’d replace your get_form_kwargs() method with something like this, to pass the user to the form:

    def get_form_kwargs(self):
        kwargs = super().get_form_kwargs()
        kwargs["user"] = self.request.user
        return kwargs

Then in the form:

class TaskForm(forms.ModelForm):
    def __init__(self, *args, **kwargs):
        self.user = kwargs.pop("user", None)
        super().__init__(*args, **kwargs)

    def clean_private(self):
        private = self.cleaned_data["private"]
        if self.user != self.instance.creator:
             raise ValidationError(
                  "As you did not create this task, you cannot set it private."
             )
        return private

It feels more robust, and slightly simpler, than using the _auth_user_id value from the session.

Thanks @philgyford - that is a far more elegant solution to my problem! I’ve made those changes and manual testing suggests the application is working as hoped.

Alas, the application renders the page with the error message with status code 200. It’s almost as if the Validation Error is buried and handled by the view without stopping because of the error, so, my automated test still fails.

I realised in my copy and paste for my post, I omitted part of my view - overriding the get_context_data() method. Could this be playing a part?

The full view:

class TaskUpdateView(UpdateView):
    model = Task
    form_class = TaskForm
    template_name = "tasks/task_detail.html"
    success_url = reverse_lazy("tasks:list")

    def get_form_kwargs(self):
        kwargs = super().get_form_kwargs()
        kwargs['user'] = self.request.user
        return kwargs

    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)
        context["page_title"] = "Edit Task"
        return context

Cheers
David

I’m guessing, but where you prepare the data for the form in the test:

  1. I imagine the value submitted from the actual form is a User ID, but you’re passing self.test_user_2 which I assume is a User object. Try self.test_user_2.id.
  2. Is “True” the value you’d expect to be returned for “private” by submitting the form? I don’t know what kind of form field it is but I’d check what is actually submitted by the form when setting a task to private.
  3. But also, now I look again, your clean_private() method isn’t checking what the value of private is. Shouldn’t you only raise an error if self.user != self.instance.creator and private is being set to True (or whatever the submitted “true” value is)?

Three further helpful suggestions, thanks again!

  1. Ah, that’s right it is an id on the User model, and I’ve corrected, but no luck, alas. I’ve also tried taking that out of the form input entirely, on the basis that the creator value is not supplied by the form and is not actually required by the logic (because the check is done on the model instance through the pk and the user in the request).
  2. Private is a boolean field and the form input field is a checkbox (and neither null nor blank are acceptable values; the default is None), so I think the expected values should be either True or None. That said, when the form is instantiated in other tests I have been passing it True or False (not strings) and if creating form data manually I have been passing those values in as strings (eg. “True”, in this case). Other tests which post this form have been passing using this approach …
  3. Good spot, and yes! I also picked this up (with the help of another test!) and added the if clause you suggested within clean_private().

fwiw, it looks like a Django checkbox widget by default has a value of the string “on”, but I imagine any submitted value would do – if it’s unchecked the field is not submitted at all.

On reflection I think the issue is that here:

       def test_user_tries_to_change_another_creators_task_to_private_post_request(self):
            with self.assertRaises(ValidationError):
                   response = self.client.post(
                       path=reverse("tasks:edit", args=[self.task.pk]), data=self.task_form_data, follow=True
                   )

you’re testing to see if posting to the view raises a validation error. But that’s never going to happen - the form validation raises a validation error, which is translated into an error message for display on the page.

So you should either:

Test something about the actual response. If you’re expecting an error message, you should get a 200 status code (instead of a redirect) and the expected error message should be in the page content:

self.assertIn("As you did not create this task, you cannot set it private.", response.content.decode())

Or test the form instead of the view:

form = TaskForm(data=data, instance=task_obj)
self.assertFalse(form.is_valid())
self.assertEqual(form.errors["private"], ["As you did not create this task, you cannot set it private."])

(I haven’t tested that code, but something like that.)

The latter will be quicker, because no request/response is made. But (as the link a above suggests) doing some view testing as well is good.

That is a logical explanation for my hunch about the AssertionError not ‘reaching’ the test. I’ll work through the suggested approach in that link. I have some integration tests for the views and some for the form, but that link will help form up some more useful unit tests too. Thanks again!

That was some journey to a working test:

    def test_user_tries_to_change_another_creators_task_to_private_post_request(self):
        self.form = TaskForm(data={
            "creator": self.test_user_2.id,
            "text": "Test task text for form",
            "private": "True",
        }, instance=self.task)
        self.assertFalse(self.form.is_valid())
        self.assertEqual(self.form.errors["private"], ["As you did not create this task, you cannot set it to be private."])

There was more of a wrestle than I hoped in creating a form instance manually, but eventually it worked (and is now a separate form unit test of its own!). The main challenge was working out how to bring the instance of the creator object into the form without it being provided by the view, so the validation could do the comparison. (Though now I look at @philgyford’s last post, he did provide that hint.)

Thanks again!

Well done!

One little thing - there’s no need to use self.form in your test. form will do, and is shorter, because that form instance is only used within the test method.