[PATCH] REST: Don't iterate through models at class level

[Daniel Axtens]

Hi Stephen,

> This causes two issues. Firstly, on fresh installs you see the following
> error message:
>
>   "Table 'patchwork.patchwork_state' doesn't exist"

Weird, I do a database reset quite often and haven't seen
this. (Although I'm not sure I have done that since the REST stuff went
in...)

This also seems very very similar to a bug I fixed in
231966452f22 ("Fix failure to start with uninitalised database"):
should we fix both in the same way? If we could avoid the ugly hack in
my patch that would be pretty great.

> Secondly, any new states created when the process is running will not be
> reflected in the API until the server process is restarted.
>
> Resolve this issue by moving the step into a method, thus ensuring it's
> continuously refreshed. It doesn't seem possible to add tests to prevent
> this regressing but some similarly useful tests are included to at least
> validate the behavior of that field.

What's the expected db overhead of doing this? It's one extra call
(possibly cached) per instance of StateField?

> Signed-off-by: Stephen Finucane <stephen at that.guru>
> Fixes: a2993505 ("REST: Make 'Patch.state' editable")
> Closes-bug: #67
> Closes-bug: #80
I thought GitHub parsed on "Closes:", but I could be wrong...

> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index cc1fcef..ace84cf 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -398,6 +398,20 @@ class TestPatchAPI(APITestCase):
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(Patch.objects.get(id=patch.id).state, state)
>  
> +    def test_update_invalid(self):
> +        """Ensure StateField is working as expected."""
Hmm. So your name suggests this tests for invalid updates, yet the doc
string is more broad...

> +        project = create_project()
> +        patch = create_patch(project=project)
> +        user = create_maintainer(project)
> +
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.get(self.api_url(patch.id))
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
This is a very generic API test - is it duplicating any other test? Does
it belong in a more generic method?
> +
> +        # invalid state
> +        resp = self.client.patch(self.api_url(patch.id), {'state': 'foobar'})
> +        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
This bit I approve of :)

Regards,
Daniel