[01/11] REST: Check.user is not read-only

Message ID 20180624195557.19909-1-stephen@that.guru
State Accepted
Headers show
Series
  • [01/11] REST: Check.user is not read-only
Related show

Commit Message

Stephen Finucane June 24, 2018, 7:55 p.m.
We only support 'Check' creation - not check updating. As a result,
there's no real reason that the 'Check.user' field should be read-only
and this is causing an issue with Django REST Framework 3.7. Simply
remove the attribute and extend the tests to validate things are working
as expected.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
 patchwork/api/check.py            | 2 +-
 patchwork/tests/api/test_check.py | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Axtens July 12, 2018, 12:27 a.m. | #1
Stephen Finucane <stephen@that.guru> writes:

> We only support 'Check' creation - not check updating. As a result,
> there's no real reason that the 'Check.user' field should be read-only
> and this is causing an issue with Django REST Framework 3.7. Simply
> remove the attribute and extend the tests to validate things are working
> as expected.

Weird, but sure, no harm in this approach.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
>  patchwork/api/check.py            | 2 +-
>  patchwork/tests/api/test_check.py | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index 8753c7de..5e461de6 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -44,7 +44,7 @@ class CheckSerializer(HyperlinkedModelSerializer):
>  
>      url = CheckHyperlinkedIdentityField('api-check-detail')
>      patch = HiddenField(default=CurrentPatchDefault())
> -    user = UserSerializer(read_only=True, default=CurrentUserDefault())
> +    user = UserSerializer(default=CurrentUserDefault())
>  
>      def run_validation(self, data):
>          for val, label in Check.STATE_CHOICES:
> diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
> index 43181af3..0e7e0cfc 100644
> --- a/patchwork/tests/api/test_check.py
> +++ b/patchwork/tests/api/test_check.py
> @@ -67,6 +67,7 @@ class TestCheckAPI(APITestCase):
>          self.assertEqual(check_obj.target_url, check_json['target_url'])
>          self.assertEqual(check_obj.context, check_json['context'])
>          self.assertEqual(check_obj.description, check_json['description'])
> +        self.assertEqual(check_obj.user.id, check_json['user']['id'])
>  
>      def test_list(self):
>          """Validate we can list checks on a patch."""
> -- 
> 2.17.1
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Daniel Axtens Aug. 26, 2018, 7:10 a.m. | #2
Hi Stephen,

Super keen to have Django 2.0 support! I dropped patch 8 because I
couldn't get it to work, and applied the small changes I suggested.

I have applied and pushed the remainder.

Regards,
Daniel

> We only support 'Check' creation - not check updating. As a result,
> there's no real reason that the 'Check.user' field should be read-only
> and this is causing an issue with Django REST Framework 3.7. Simply
> remove the attribute and extend the tests to validate things are working
> as expected.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
>  patchwork/api/check.py            | 2 +-
>  patchwork/tests/api/test_check.py | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index 8753c7de..5e461de6 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -44,7 +44,7 @@ class CheckSerializer(HyperlinkedModelSerializer):
>  
>      url = CheckHyperlinkedIdentityField('api-check-detail')
>      patch = HiddenField(default=CurrentPatchDefault())
> -    user = UserSerializer(read_only=True, default=CurrentUserDefault())
> +    user = UserSerializer(default=CurrentUserDefault())
>  
>      def run_validation(self, data):
>          for val, label in Check.STATE_CHOICES:
> diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
> index 43181af3..0e7e0cfc 100644
> --- a/patchwork/tests/api/test_check.py
> +++ b/patchwork/tests/api/test_check.py
> @@ -67,6 +67,7 @@ class TestCheckAPI(APITestCase):
>          self.assertEqual(check_obj.target_url, check_json['target_url'])
>          self.assertEqual(check_obj.context, check_json['context'])
>          self.assertEqual(check_obj.description, check_json['description'])
> +        self.assertEqual(check_obj.user.id, check_json['user']['id'])
>  
>      def test_list(self):
>          """Validate we can list checks on a patch."""
> -- 
> 2.17.1
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Stephen Finucane Aug. 29, 2018, 10:11 a.m. | #3
On Sun, 2018-08-26 at 17:10 +1000, Daniel Axtens wrote:
> Hi Stephen,
> 
> Super keen to have Django 2.0 support! I dropped patch 8 because I
> couldn't get it to work, and applied the small changes I suggested.
> 
> I have applied and pushed the remainder.

Cheers :) Now for Django 2.1. This train never stops, apparently.

Stephen

> Regards,
> Daniel
> 
> > We only support 'Check' creation - not check updating. As a result,
> > there's no real reason that the 'Check.user' field should be read-only
> > and this is causing an issue with Django REST Framework 3.7. Simply
> > remove the attribute and extend the tests to validate things are working
> > as expected.
> > 
> > Signed-off-by: Stephen Finucane <stephen@that.guru>
> > ---
> >  patchwork/api/check.py            | 2 +-
> >  patchwork/tests/api/test_check.py | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> > index 8753c7de..5e461de6 100644
> > --- a/patchwork/api/check.py
> > +++ b/patchwork/api/check.py
> > @@ -44,7 +44,7 @@ class CheckSerializer(HyperlinkedModelSerializer):
> >  
> >      url = CheckHyperlinkedIdentityField('api-check-detail')
> >      patch = HiddenField(default=CurrentPatchDefault())
> > -    user = UserSerializer(read_only=True, default=CurrentUserDefault())
> > +    user = UserSerializer(default=CurrentUserDefault())
> >  
> >      def run_validation(self, data):
> >          for val, label in Check.STATE_CHOICES:
> > diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
> > index 43181af3..0e7e0cfc 100644
> > --- a/patchwork/tests/api/test_check.py
> > +++ b/patchwork/tests/api/test_check.py
> > @@ -67,6 +67,7 @@ class TestCheckAPI(APITestCase):
> >          self.assertEqual(check_obj.target_url, check_json['target_url'])
> >          self.assertEqual(check_obj.context, check_json['context'])
> >          self.assertEqual(check_obj.description, check_json['description'])
> > +        self.assertEqual(check_obj.user.id, check_json['user']['id'])
> >  
> >      def test_list(self):
> >          """Validate we can list checks on a patch."""
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork

Patch

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index 8753c7de..5e461de6 100644
--- a/patchwork/api/check.py
+++ b/patchwork/api/check.py
@@ -44,7 +44,7 @@  class CheckSerializer(HyperlinkedModelSerializer):
 
     url = CheckHyperlinkedIdentityField('api-check-detail')
     patch = HiddenField(default=CurrentPatchDefault())
-    user = UserSerializer(read_only=True, default=CurrentUserDefault())
+    user = UserSerializer(default=CurrentUserDefault())
 
     def run_validation(self, data):
         for val, label in Check.STATE_CHOICES:
diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
index 43181af3..0e7e0cfc 100644
--- a/patchwork/tests/api/test_check.py
+++ b/patchwork/tests/api/test_check.py
@@ -67,6 +67,7 @@  class TestCheckAPI(APITestCase):
         self.assertEqual(check_obj.target_url, check_json['target_url'])
         self.assertEqual(check_obj.context, check_json['context'])
         self.assertEqual(check_obj.description, check_json['description'])
+        self.assertEqual(check_obj.user.id, check_json['user']['id'])
 
     def test_list(self):
         """Validate we can list checks on a patch."""