[admin] Move AlwaysHasChangedMixin to openwisp-utils #32

strang1ato · 2018-12-07T20:32:52Z

coveralls · 2018-12-08T10:47:37Z

Coverage decreased (-1.5%) to 96.277% when pulling 28e8968 on OltarzewskiK:move_class into d1de1e3 on openwisp:master.

strang1ato · 2018-12-08T11:39:07Z

Getting this error when trying to reach this site: admin/test_project/id/add/ with commented this line https://github.com/openwisp/openwisp-utils/pull/32/files#diff-51ded2606895802985b9435e4fc4bd26R29 code works properly

nemesifier

mm I have not enough information from that screenshot, let's discuss it over the general chat

nemesifier · 2018-12-08T17:24:06Z

tests/test_project/tests/test_admin.py

+        options = dict(name='test')
+        params = self._get_defaults(options)
+        path = reverse('admin:test_project_id_add')
+        self.client.post(path, params)


here you should test that the post you just sent did create an object which has those default params assigned

@nemesisdesign If there is need to do it I will have to create object not by self.client.post, but by function like _create_project or something like that, because self.client.post is giving me only httpresponse.

nemesifier · 2018-12-08T17:25:34Z

tests/test_project/tests/test_admin.py

@@ -30,3 +30,9 @@ def test_radiusaccounting_changelist(self):
        url = reverse('admin:test_project_radiusaccounting_changelist')
        response = self.client.get(url)
        self.assertNotContains(response, 'Add accounting')
+
+    def test_AlwaysHasChangedMixin_add(self):
+        options = dict(name='test')


please add a paranoid test to ensure no items are present in the database for this type of object, that way we can be sure there's not another test which creates it before the test is executed (we are sure of that now, but we can't be sure in the future somebody will add it)

nemesifier

See my comments and address all of them please

nemesifier · 2018-12-08T19:41:00Z

tests/test_project/admin.py

@@ -1,12 +1,35 @@
 from django.contrib import admin
-from openwisp_utils.admin import ReadOnlyAdmin
+from django.contrib.admin import ModelAdmin
+from django.contrib.admin.options import InlineModelAdmin


this is wrong, you shouldn't use this class, where did you read this? You have to use model.StackedInline, I will explain further in the next comments

nemesifier · 2018-12-08T19:41:38Z

tests/test_project/admin.py

+from .models import Id, Operator, RadiusAccounting
+
+
+class OperatorAdmin(ModelAdmin):


change to class OperatorAdmin(admin.ModelAdmin):

nemesifier · 2018-12-08T19:41:56Z

tests/test_project/admin.py

+    form = OperatorForm
+
+
+class IdAdmin(ModelAdmin):


change to class IdAdmin(admin.ModelAdmin):

nemesifier · 2018-12-08T19:42:21Z

tests/test_project/admin.py

+    pass
+
+
+class OperatorInline(InlineModelAdmin):


change to class OperatorInline(admin.StackedInline):
this is what is causing the exception

nemesifier · 2018-12-08T19:42:44Z

tests/test_project/admin.py

+
+class OperatorInline(InlineModelAdmin):
+    model = Operator
+    form = OperatorForm


add extra = 0

nemesifier · 2018-12-08T19:43:31Z

tests/test_project/models.py

@@ -43,3 +43,22 @@ class RadiusAccounting(models.Model):
                                db_index=True,
                                null=True,
                                blank=True)
+
+
+class Id(models.Model):


please rename this model to something a bit more meaningful, for example Project

nemesifier · 2018-12-08T19:44:42Z

tests/test_project/models.py

+class Operator(models.Model):
+    first_name = models.CharField(max_length=30)
+    last_name = models.CharField(max_length=30)
+    operator = models.ForeignKey(Id, on_delete=models.CASCADE, blank=True)


this field should be named as the model used in the foreign key, so if you rename that model to Project, this field should be called project.

This is more for clean up and to improve readability of the test models (which is still important in the grand scheme of things, as complexity increases, these things matter too).

nemesifier · 2018-12-08T19:45:12Z

tests/test_project/tests/test_admin.py

@@ -30,3 +30,9 @@ def test_radiusaccounting_changelist(self):
        url = reverse('admin:test_project_radiusaccounting_changelist')
        response = self.client.get(url)
        self.assertNotContains(response, 'Add accounting')
+
+    def test_AlwaysHasChangedMixin_add(self):


please rename this method to test_alwayshaschangedmixin

nemesifier

almost there, see my comments

nemesifier · 2018-12-09T15:56:19Z

tests/test_project/models.py

+
+
+class Project(models.Model):
+    id = models.BigAutoField(primary_key=True)


remove this line

nemesifier · 2018-12-09T15:56:47Z

tests/test_project/tests/test_admin.py

+        options = dict(name='test')
+        params = self._get_defaults(options)
+        path = reverse('admin:test_project_project_add')
+        obj = self.client.post(path, params)


you need to test an object was created correctly here

@nemesisdesign Do you know method how to show all parameters of formset (I was trying to use OperatorInline.formset.management_form but it was returning <django.utils.functional.cached_property object at 0x7f920c04be10>) like operator-TOTAL_FORMS, operator-INITIAL_FORMS because I am getting error django.core.exceptions.ValidationError: ['ManagementForm data is missing or has been tampered with']

this is enough:

self.assertEqual(self.project_model.objects.count(), 1) project = self.project_model.objects.first() self.assertEqual(project.name, params['name'])

nemesifier · 2018-12-09T19:06:24Z

tests/test_project/tests/test_admin.py

@@ -30,3 +31,10 @@ def test_radiusaccounting_changelist(self):
        url = reverse('admin:test_project_radiusaccounting_changelist')
        response = self.client.get(url)
        self.assertNotContains(response, 'Add accounting')
+
+    def test_alwayshaschangedmixin(self):
+        self.assertEqual(self.project_model.objects.all().count(), 0)


this can be shortened to self.project_model.objects.count()

nemesifier · 2018-12-09T19:07:38Z

tests/test_project/tests/test_admin.py

+        options = dict(name='test')
+        params = self._get_defaults(options)
+        path = reverse('admin:test_project_project_add')
+        obj = self.client.post(path, params)


this is enough:

self.assertEqual(self.project_model.objects.count(), 1) project = self.project_model.objects.first() self.assertEqual(project.name, params['name'])

strang1ato changed the title ~~[admin] Move AlwaysHasChangedMixin to openwisp-utils~~ [WIP] [admin] Move AlwaysHasChangedMixin to openwisp-utils Dec 7, 2018

nemesifier reviewed Dec 8, 2018

View reviewed changes

nemesifier requested changes Dec 8, 2018

View reviewed changes

nemesifier requested changes Dec 9, 2018

View reviewed changes

nemesifier reviewed Dec 9, 2018

View reviewed changes

[admin] Move AlwaysHasChangedMixin to openwisp-utils

28e8968

nemesifier mentioned this pull request Dec 9, 2018

Added test for AlwaysHasChangedMixin #36

Merged

nemesifier closed this in #36 Dec 9, 2018

strang1ato changed the title ~~[WIP] [admin] Move AlwaysHasChangedMixin to openwisp-utils~~ [admin] Move AlwaysHasChangedMixin to openwisp-utils Dec 9, 2018

strang1ato deleted the move_class branch December 9, 2018 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[admin] Move AlwaysHasChangedMixin to openwisp-utils #32

[admin] Move AlwaysHasChangedMixin to openwisp-utils #32

strang1ato commented Dec 7, 2018

coveralls commented Dec 8, 2018 •

edited

Loading

strang1ato commented Dec 8, 2018

nemesifier left a comment

nemesifier Dec 8, 2018

strang1ato Dec 9, 2018 •

edited

Loading

nemesifier Dec 8, 2018

strang1ato Dec 9, 2018

nemesifier left a comment

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier Dec 8, 2018

nemesifier left a comment

nemesifier Dec 9, 2018

nemesifier Dec 9, 2018

strang1ato Dec 9, 2018 •

edited

Loading

nemesifier Dec 9, 2018

nemesifier Dec 9, 2018

nemesifier Dec 9, 2018

		from .models import Id, Operator, RadiusAccounting


		class OperatorAdmin(ModelAdmin):



		class Project(models.Model):
		id = models.BigAutoField(primary_key=True)

[admin] Move AlwaysHasChangedMixin to openwisp-utils #32

[admin] Move AlwaysHasChangedMixin to openwisp-utils #32

Conversation

strang1ato commented Dec 7, 2018

coveralls commented Dec 8, 2018 • edited Loading

strang1ato commented Dec 8, 2018

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strang1ato Dec 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strang1ato Dec 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 8, 2018 •

edited

Loading

strang1ato Dec 9, 2018 •

edited

Loading

strang1ato Dec 9, 2018 •

edited

Loading