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

[Denis Laxalde]

Hi Stephen,

Thank you for looking into this issue!

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

Unfortunately, it does not solve this point because you still query the
DB for a table that does not exist in StateField.__init__() which is
called to define the "state" class attribute of PatchListSerializer,
which happens at module import.
Maybe the states choices could be initialized in a lazy manner?

> Secondly, any new states created when the process is running will not be
> reflected in the API until the server process is restarted.

This point is solved as far as I can tell.

> 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.
>
> Signed-off-by: Stephen Finucane <stephen at that.guru>
> Fixes: a2993505 ("REST: Make 'Patch.state' editable")
> Closes-bug: #67
> Closes-bug: #80
> Cc: denis at laxalde.org
> Cc: steve at asksteved.com
> ---
>  patchwork/api/base.py            |  5 -----
>  patchwork/api/patch.py           |  7 ++++---
>  patchwork/tests/test_rest_api.py | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 8 deletions(-)
>

[snip]

> 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."""
> +        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)
> +
> +        # invalid state
> +        resp = self.client.patch(self.api_url(patch.id), {'state': 'foobar'})
> +        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
> +

I wonder if we shouldn't check the validation error message here, to
make sure it includes actual states of the DB, and not only those
present at server startup. Something like:

self.assertEqual(['Invalid state. Expected one of: state_0, state_1 '],
                  resp.json()['state'])

with a `create_state()` at the beginning of test method.