Repository: openwisp/openwisp-radius (language: python)

Issue:
[feature] Added more user fields to registration api #219
Closes #219
[feature] Added more user fields to registration api #219
Closes #219


Discussion / hints:

[![Coverage Status](https://coveralls.io/builds/36372790/badge)](https://coveralls.io/builds/36372790)

Coverage increased (+0.02%) to 99.337% when pulling **c1bb1c73b0979e17bf5eec0af0843788c7af9568 on issue-219-add-more-user-fields-to-registration-api** into **f48713356f4fe5c62e843878c46d37e092e40d3d on master**.


[![Coverage Status](https://coveralls.io/builds/36372790/badge)](https://coveralls.io/builds/36372790)

Coverage increased (+0.02%) to 99.337% when pulling **c1bb1c73b0979e17bf5eec0af0843788c7af9568 on issue-219-add-more-user-fields-to-registration-api** into **f48713356f4fe5c62e843878c46d37e092e40d3d on master**.


Fix patch:
diff --git a/docs/source/user/api.rst b/docs/source/user/api.rst
index 0967af39..f03182ba 100644
--- a/docs/source/user/api.rst
+++ b/docs/source/user/api.rst
@@ -350,22 +350,22 @@ Parameters:
 Param              Description
 ===============    ===============================
 username           string
-first_name         string (*)
-last_name          string (*)
-birth_date         string (*)
-location           string (*)
+phone_number       string (\*)
 email              string
 password1          string
 password2          string
-phone_number       string (*)
+first_name         string (\*\*)
+last_name          string (\*\*)
+birth_date         string (\*\*)
+location           string (\*\*)
 ===============    ===============================
 
-(*) ``phone_number`` is required only when the organization has enabled
+(\*) ``phone_number`` is required only when the organization has enabled
 SMS verification in its "Organization RADIUS Settings".
 
-(*) ``first_name``, ``last_name``, ``birth_date`` and ``location`` are optional fields 
-and can be `configured <./settings.html#openwisp-radius-optional-registration-fields>`_ 
-to be optionally allowed, ignored or required.
+(\*\*) ``first_name``, ``last_name``, ``birth_date`` and ``location``
+are optional fields which are disabled by default to make the registration
+simple, but can be `enabled through configuration <./settings.html#openwisp-radius-optional-registration-fields>`_.
 
 Reset password
 --------------
diff --git a/docs/source/user/settings.rst b/docs/source/user/settings.rst
index e5f325b4..fd4bb64c 100644
--- a/docs/source/user/settings.rst
+++ b/docs/source/user/settings.rst
@@ -273,6 +273,63 @@ or Cameroon (``+237``).
 
 **Note:** this setting is applicable only for organizations which have enabled SMS verification.
 
+``OPENWISP_RADIUS_OPTIONAL_REGISTRATION_FIELDS``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+**Default**:
+
+.. code-block:: python
+
+    {
+        'first_name': 'disabled',
+        'last_name': 'disabled',
+        'birth_date': 'disabled',
+        'location': 'disabled',
+    }
+
+This global setting is used to specify if the optional user fields
+(``first_name``, ``last_name``, ``location`` and ``birth_date``)
+shall be disabled (hence ignored), allowed or required in the
+`User Registration API <api.html#user-registration>`_.
+
+The allowed values are:
+
+- ``disabled``: (**default**) the field is disabled.
+- ``allowed``: the field is allowed but not mandatory.
+- ``mandatory``: the field is mandatory.
+
+For example:
+
+.. code-block:: python
+
+    OPENWISP_RADIUS_OPTIONAL_REGISTRATION_FIELDS = {
+        'first_name': 'disabled',
+        'last_name': 'disabled',
+        'birth_date': 'mandatory',
+        'location': 'allowed',
+    }
+
+Means:
+
+- ``first_name`` and ``last_name`` fields are not required and their values
+  if provided are ignored.
+- ``location`` field is not required but its value will
+  be saved to the database if provided.
+- ``birth_date`` field is required and a ``ValidationError``
+  exception is raised if its value is not provided.
+
+The setting for each field can also be overridden at organization level
+if needed, by going to
+``Home › Users and Organizations › Organizations > Edit organization`` and
+then scrolling down to ``ORGANIZATION RADIUS SETTINGS``.
+
+.. image:: /images/optional_fields.png
+    :alt: optional field setting
+
+By default the fields at organization level hold a ``NULL`` value,
+which means that the global setting specified in ``settings.py`` will
+be used.
+
 ``OPENWISP_RADIUS_PASSWORD_RESET_URLS``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -314,51 +371,6 @@ the configuration should be simply changed to:
         'default': 'https://login.wifiservice.com/{organization}/password/reset/confirm/{uid}/{token}'
     }
 
-``OPENWISP_RADIUS_OPTIONAL_REGISTRATION_FIELDS``
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-**Default**:
-
-.. code-block:: python
-
-    {
-        'first_name': 'disabled',
-        'last_name': 'disabled',
-        'birth_date': 'disabled',
-        'location': 'disabled',
-    }
-
-This setting is used to specify if the user's optional fields (``first_name``, ``last_name``, ``location`` and ``birth_date``) 
-should either be optionally allowed, ignored or required in the `User Registration API <api.html#user-registration>`_.
-It can take any of the following values:
-
-- ``disabled``: this is the **default** setting. Value of user's optional fields are not required and will not be saved to the database if supplied.
-- ``allowed``: value of user's optional fields will be save to the database but they are not required.
-- ``mandatory``: value of user's optional fields are required and will be saved to the database.
-
-For example:
-
-.. code-block:: python
-
-    OPENWISP_RADIUS_OPTIONAL_REGISTRATION_FIELDS = {
-        'first_name': 'disabled',
-        'last_name': 'disabled',
-        'birth_date': 'mandatory',
-        'location': 'allowed', 
-    }
-
-In the global setting above;
-
-- ``first_name`` and ``last_name`` fields are not required and their values if provided are ignored.
-- ``location`` field is not required but its value will be save to the database if provided.
-- ``birth_date`` field is required and a ``ValidationError`` exception is raised if its value is not provided.
-
-These fields can also be configured to behave in a similar manner at the organization level by setting them in the admin 
-as shown in the screenshot below:
-
-.. image:: /images/optional_fields.png
-    :alt: optional field setting
-
 Email related settings
 ======================
 
diff --git a/openwisp_radius/admin.py b/openwisp_radius/admin.py
index 862c48a8..85e92cc5 100644
--- a/openwisp_radius/admin.py
+++ b/openwisp_radius/admin.py
@@ -499,78 +499,61 @@ def has_delete_permission(self, request, obj=None):
 UserAdminInline.inlines += [RadiusUserGroupInline, PhoneTokenInline]
 
 
-class AllowedHostsField(forms.CharField):
-    def prepare_value(self, value):
-        if not value:
-            value = ','.join(app_settings.FREERADIUS_ALLOWED_HOSTS)
-        return super().prepare_value(value)
-
-
-class AllowedMobilePrefixesField(forms.CharField):
-    def prepare_value(self, value):
-        if not value:
-            value = ','.join(app_settings.ALLOWED_MOBILE_PREFIXES)
-        return super().prepare_value(value)
-
-
-class FirstNameField(forms.ChoiceField):
-    def prepare_value(self, value):
-        if not value:
-            value = OPTIONAL_SETTINGS.get('first_name')
-        return super().prepare_value(value)
+class FallbackFieldMixin(object):
+    def __init__(self, fallback, *args, **kwargs):
+        self.fallback = fallback
+        super().__init__(*args, **kwargs)
 
-
-class LastNameField(forms.ChoiceField):
     def prepare_value(self, value):
         if not value:
-            value = OPTIONAL_SETTINGS.get('last_name')
+            value = self.fallback
         return super().prepare_value(value)
 
 
-class LocationField(forms.ChoiceField):
-    def prepare_value(self, value):
-        if not value:
-            value = OPTIONAL_SETTINGS.get('location')
-        return super().prepare_value(value)
+class FallbackCharField(FallbackFieldMixin, forms.CharField):
+    pass
 
 
-class BirthDateField(forms.ChoiceField):
-    def prepare_value(self, value):
-        if not value:
-            value = OPTIONAL_SETTINGS.get('birth_date')
-        return super().prepare_value(value)
+class FallbackChoiceField(FallbackFieldMixin, forms.ChoiceField):
+    pass
 
 
 class AlwaysHasChangedForm(AlwaysHasChangedMixin, forms.ModelForm):
-    freeradius_allowed_hosts = AllowedHostsField(
+    freeradius_allowed_hosts = FallbackCharField(
         required=False,
         widget=forms.Textarea(attrs={'rows': 2, 'cols': 34}),
         help_text=_GET_IP_LIST_HELP_TEXT,
+        fallback=','.join(app_settings.FREERADIUS_ALLOWED_HOSTS),
     )
-    allowed_mobile_prefixes = AllowedMobilePrefixesField(
+    allowed_mobile_prefixes = FallbackCharField(
         required=False,
         widget=forms.Textarea(attrs={'rows': 2, 'cols': 34}),
         help_text=_GET_MOBILE_PREFIX_HELP_TEXT,
+        fallback=','.join(app_settings.ALLOWED_MOBILE_PREFIXES),
     )
-    first_name = FirstNameField(
+    first_name = FallbackChoiceField(
         required=False,
         help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
         choices=OPTIONAL_FIELD_CHOICES,
+        fallback=OPTIONAL_SETTINGS.get('first_name', 'disabled'),
     )
-    last_name = LastNameField(
+    last_name = FallbackChoiceField(
         required=False,
         help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
         choices=OPTIONAL_FIELD_CHOICES,
+        fallback=OPTIONAL_SETTINGS.get('last_name', 'disabled'),
     )
-    location = LocationField(
+    location = FallbackChoiceField(
         required=False,
         help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
         choices=OPTIONAL_FIELD_CHOICES,
+        fallback=OPTIONAL_SETTINGS.get('location', 'disabled'),
     )
-    birth_date = BirthDateField(
+    birth_date = FallbackChoiceField(
         required=False,
         help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
         choices=OPTIONAL_FIELD_CHOICES,
+        fallback=OPTIONAL_SETTINGS.get('birth_date', 'disabled'),
     )
 
 
diff --git a/openwisp_radius/base/models.py b/openwisp_radius/base/models.py
index 0d9c278f..9e7755d8 100644
--- a/openwisp_radius/base/models.py
+++ b/openwisp_radius/base/models.py
@@ -149,8 +149,8 @@
     'allowed to register via the user registration API.'
 )
 _GET_OPTIONAL_FIELDS_HELP_TEXT = _(
-    'Chose if this user field should be disabled, allowed or mandatory '
-    'during user registration.'
+    'Whether this field should be disabled, allowed or mandatory '
+    'in the user registration API.'
 )
 
 
diff --git a/openwisp_radius/migrations/0020_added_optional_registration_fields.py b/openwisp_radius/migrations/0020_added_optional_registration_fields.py
index eb1d2271..62df15c6 100644
--- a/openwisp_radius/migrations/0020_added_optional_registration_fields.py
+++ b/openwisp_radius/migrations/0020_added_optional_registration_fields.py
@@ -2,6 +2,8 @@
 
 from django.db import migrations, models
 
+from ..base.models import _GET_OPTIONAL_FIELDS_HELP_TEXT
+
 
 class Migration(migrations.Migration):
 
@@ -20,10 +22,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='birth date',
@@ -39,10 +38,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='first name',
@@ -58,10 +54,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='last name',
@@ -77,10 +70,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='location',


Test patch:
diff --git a/openwisp_radius/tests/test_api.py b/openwisp_radius/tests/test_api.py
index e834c1b0..9bdb671b 100644
--- a/openwisp_radius/tests/test_api.py
+++ b/openwisp_radius/tests/test_api.py
@@ -50,12 +50,6 @@
 
 class TestApi(ApiTokenMixin, FileMixin, BaseTestCase):
     _test_email = 'test@openwisp.org'
-    optional_settings_params = {
-        'first_name': 'disabled',
-        'last_name': 'allowed',
-        'birth_date': 'disabled',
-        'location': 'mandatory',
-    }
 
     def setUp(self):
         cache.clear()
@@ -1113,7 +1107,14 @@ def test_register_201(self):
         self.assertTrue(user.is_active)
 
     @mock.patch.object(
-        app_settings, 'OPTIONAL_REGISTRATION_FIELDS', optional_settings_params
+        app_settings,
+        'OPTIONAL_REGISTRATION_FIELDS',
+        {
+            'first_name': 'disabled',
+            'last_name': 'allowed',
+            'birth_date': 'disabled',
+            'location': 'mandatory',
+        },
     )
     def test_optional_fields_registration(self):
         self._superuser_login()
diff --git a/tests/openwisp2/sample_radius/migrations/0020_added_optional_registration_fields.py b/tests/openwisp2/sample_radius/migrations/0020_added_optional_registration_fields.py
index 9e02dcaa..545dc2f5 100644
--- a/tests/openwisp2/sample_radius/migrations/0020_added_optional_registration_fields.py
+++ b/tests/openwisp2/sample_radius/migrations/0020_added_optional_registration_fields.py
@@ -2,6 +2,8 @@
 
 from django.db import migrations, models
 
+from openwisp_radius.base.models import _GET_OPTIONAL_FIELDS_HELP_TEXT
+
 
 class Migration(migrations.Migration):
 
@@ -20,10 +22,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='birth date',
@@ -39,10 +38,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='first name',
@@ -58,10 +54,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='last name',
@@ -77,10 +70,7 @@ class Migration(migrations.Migration):
                     ('allowed', 'Allowed'),
                     ('mandatory', 'Mandatory'),
                 ],
-                help_text=(
-                    'Chose if this user field should be disabled, '
-                    'allowed or mandatory during user registration.'
-                ),
+                help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
                 max_length=12,
                 null=True,
                 verbose_name='location',