-
-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[admin] Move AlwaysHasChangedMixin to openwisp-utils #32
Conversation
Getting this error when trying to reach this site: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm I have not enough information from that screenshot, let's discuss it over the general chat
options = dict(name='test') | ||
params = self._get_defaults(options) | ||
path = reverse('admin:test_project_id_add') | ||
self.client.post(path, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you should test that the post you just sent did create an object which has those default params assigned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments and address all of them please
tests/test_project/admin.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
tests/test_project/admin.py
Outdated
from .models import Id, Operator, RadiusAccounting | ||
|
||
|
||
class OperatorAdmin(ModelAdmin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to class OperatorAdmin(admin.ModelAdmin):
tests/test_project/admin.py
Outdated
form = OperatorForm | ||
|
||
|
||
class IdAdmin(ModelAdmin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to class IdAdmin(admin.ModelAdmin):
tests/test_project/admin.py
Outdated
pass | ||
|
||
|
||
class OperatorInline(InlineModelAdmin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to class OperatorInline(admin.StackedInline):
this is what is causing the exception
|
||
class OperatorInline(InlineModelAdmin): | ||
model = Operator | ||
form = OperatorForm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add extra = 0
tests/test_project/models.py
Outdated
@@ -43,3 +43,22 @@ class RadiusAccounting(models.Model): | |||
db_index=True, | |||
null=True, | |||
blank=True) | |||
|
|||
|
|||
class Id(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename this model to something a bit more meaningful, for example Project
tests/test_project/models.py
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename this method to test_alwayshaschangedmixin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there, see my comments
tests/test_project/models.py
Outdated
|
||
|
||
class Project(models.Model): | ||
id = models.BigAutoField(primary_key=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line
options = dict(name='test') | ||
params = self._get_defaults(options) | ||
path = reverse('admin:test_project_project_add') | ||
obj = self.client.post(path, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to test an object was created correctly here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is enough:
self.assertEqual(self.project_model.objects.count(), 1)
project = self.project_model.objects.first()
self.assertEqual(project.name, params['name'])
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be shortened to self.project_model.objects.count()
options = dict(name='test') | ||
params = self._get_defaults(options) | ||
path = reverse('admin:test_project_project_add') | ||
obj = self.client.post(path, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is enough:
self.assertEqual(self.project_model.objects.count(), 1)
project = self.project_model.objects.first()
self.assertEqual(project.name, params['name'])
Fixes openwisp/openwisp-controller#44