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

[general] added abstract models #13

Merged
merged 31 commits into from Jun 25, 2017

Conversation

Projects
None yet
4 participants
@lillopaco
Member

lillopaco commented Jun 16, 2017

added abstract models and cleanup some db_columns redundant.

Related to #6
fix #11

  • define abstract models
  • implement swappable models
  • create test application + tests
model = RadiusPostAuthentication
admin.site.register(RadiusPostAuthentication, RadiusPostAuthenticationAdmin)

This comment has been minimized.

@yakky

yakky Jun 16, 2017

Member

I prefer the decorator version @admin.register of registration

# An abstract base class model that provides self-updating
# modified fields.
id = models.UUIDField(primary_key=True, editable=False)

This comment has been minimized.

@yakky

yakky Jun 16, 2017

Member

As discussed, probably UUIDField is not useful for freeradius-native models. And it's definitely too "heavy" (sizewise) for highly populated tables like AbstractRadiusAccounting
I would keep this base model without the id and I'd just use standard numeric primary keys
I would also add a created = AutoCreatedField(_('created'))
Check the class name too (it should be TimeStampedEditableModel)

lillopaco added some commits Jun 16, 2017

@yakky

yakky approved these changes Jun 17, 2017

@yakky

yakky requested changes Jun 17, 2017 edited

Updated migrations needed
Also django-model-utils must be added to https://github.com/openwisp/django-freeradius/blob/master/requirements.txt

ModelAdmin for TimeStampedEditableModel
"""
def __init__(self, *args, **kwargs):
self.readonly_fields += ('created', 'modified',)

This comment has been minimized.

@yakky
@coveralls

This comment has been minimized.

coveralls commented Jun 17, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9929df9 on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 17, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 62b5e4d on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 17, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c5acf30 on freeradius_abstract into fb84df3 on master.

@yakky

yakky approved these changes Jun 17, 2017

@lillopaco lillopaco requested a review from nemesisdesign Jun 17, 2017

@yakky yakky changed the title from [general] added abstract models to WiP: [general] added abstract models Jun 17, 2017

@nemesisdesign

👍 you can add tests to this PR or a new one if you like

@yakky yakky changed the title from WiP: [general] added abstract models to [general] added abstract models Jun 21, 2017

lillopaco added some commits Jun 24, 2017

@@ -13,8 +24,9 @@
ALLOWED_HOSTS = []
DATABASES = {
'default': env.db(default='sqlite:///django-freeradius.db'),
}

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Remove this whitespace

'django_freeradius'
'django_freeradius',

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Remove these whitespaces

@@ -2,6 +2,17 @@
import environ
DJANGO_FREERADIUS_RADIUSREPLY_MODEL = "sample_radius.RadiusReply"

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

It's better to move this setting inside the if at the end of the file

@skipIf(True, "env = DATABASE_URL='mysql://mysql@127.0.0.1/sample_test' SAMPLE_APP=1")
class UserTest(TestCase):
def setUp(self):

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

always call the super inside setUp

@skipIf(True, "env = DATABASE_URL='mysql://mysql@127.0.0.1/sample_test' SAMPLE_APP=1")

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

To skip the entire test class, add the following code to setUp method below:

@skipIf(os.environ.get('SAMPLE_APP', False), 'Running tests on SAMPLE_APP')
class UserTest(TestCase):
    ...
@skipIf(True, "env = DATABASE_URL='mysql://mysql@127.0.0.1/sample_test' SAMPLE_APP=1")

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Same as in UserTest

@skipIf(True, "env = DATABASE_URL='postgres://postgres@127.0.0.1/freeradius_test'")
@skipIf(True, "env = DATABASE_URL='mysql://root@127.0.0.1/freeradius_test'")
class NasModelTest(TestCase):

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

@skipUnless(os.environ.get('SAMPLE_APP', False), 'Running tests on standard django_freeradius models')
class NasModelTest(TestCase):
...

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-1.1%) to 98.851% when pulling 685ec3c on freeradius_abstract into fb84df3 on master.

2 similar comments
@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-1.1%) to 98.851% when pulling 685ec3c on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-1.1%) to 98.851% when pulling 685ec3c on freeradius_abstract into fb84df3 on master.

@@ -86,3 +70,16 @@
from local_settings import *
except ImportError:
pass
if os.environ.get('SAMPLE_APP', True):

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Fix indentation (move left one space)

@skipIf(True, "env = DATABASE_URL='postgres://postgres@127.0.0.1/freeradius_test'")
@skipIf(True, "env = DATABASE_URL='mysql://root@127.0.0.1/freeradius_test'")
@skipUnless(os.environ.get('SAMPLE_APP', False), 'Running tests on standard django_freeradius models')

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Decorate all the classes

from ..models import (Nas, RadiusAccounting, RadiusCheck, RadiusGroup, RadiusGroupCheck, RadiusGroupReply,
RadiusGroupUsers, RadiusPostAuthentication, RadiusReply, RadiusUserGroup,)
@skipIf(True, "env = DATABASE_URL='mysql://mysql@127.0.0.1/sample_test' SAMPLE_APP=1")
@skipIf(os.environ.get('SAMPLE_APP', True), 'Running tests on SAMPLE_APP')

This comment has been minimized.

@yakky

yakky Jun 25, 2017

Member

Decorate all the classes

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-1.1%) to 98.851% when pulling c04348d on freeradius_abstract into fb84df3 on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-1.1%) to 98.851% when pulling c04348d on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 02290da on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e36c42c on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3103a95 on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1539552 on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1539552 on freeradius_abstract into fb84df3 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1e40453 on freeradius_abstract into fb84df3 on master.

@yakky

yakky approved these changes Jun 25, 2017

@yakky yakky merged commit 3aa4fa4 into master Jun 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

This was referenced Jun 25, 2017

@lillopaco lillopaco deleted the freeradius_abstract branch Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment