[bug] Fallback fields generate DB migrations when default settings are changed

[nemesifier]

Describe the bug
Fallback fields generate migrations when default settings are changed.

openwisp-radius/openwisp_radius/base/models.py

Lines 1207 to 1328 in 2646a5c

	sms_verification = FallbackBooleanChoiceField(
	help_text=_SMS_VERIFICATION_HELP_TEXT,
	fallback=app_settings.SMS_VERIFICATION_ENABLED,
	verbose_name=_("SMS verification"),
	)
	needs_identity_verification = FallbackBooleanChoiceField(
	help_text=_IDENTITY_VERIFICATION_ENABLED_HELP_TEXT,
	fallback=app_settings.NEEDS_IDENTITY_VERIFICATION,
	)
	sms_sender = models.CharField(
	_("Sender"),
	max_length=128,
	blank=True,
	null=True,
	help_text=_(
	"alpha numeric identifier used as sender for SMS sent by this organization"
	),
	)
	sms_message = FallbackTextField(
	_("SMS Message"),
	max_length=160,
	help_text=_(
	"SMS message template used for sending verification code."
	' Must contain "{code}" placeholder for OTP value.'
	),
	fallback=app_settings.SMS_MESSAGE_TEMPLATE,
	)
	sms_cooldown = FallbackPositiveIntegerField(
	_("SMS Cooldown"),
	help_text=_(
	"Time period a user will have to wait before requesting"
	" another SMS token (in seconds)."
	),
	fallback=app_settings.SMS_COOLDOWN,
	)
	sms_meta_data = JSONField(
	null=True,
	blank=True,
	help_text=_(
	"Additional configuration for SMS backend in JSON format"
	" (optional, leave blank if unsure)"
	),
	verbose_name=_("SMS meta data"),
	)
	freeradius_allowed_hosts = FallbackTextField(
	help_text=_GET_IP_LIST_HELP_TEXT,
	fallback=",".join(app_settings.FREERADIUS_ALLOWED_HOSTS),
	)
	coa_enabled = FallbackBooleanChoiceField(
	help_text=_COA_ENABLED_HELP_TEXT,
	fallback=app_settings.COA_ENABLED,
	verbose_name=_("CoA Enabled"),
	)
	allowed_mobile_prefixes = FallbackTextField(
	help_text=_GET_MOBILE_PREFIX_HELP_TEXT,
	fallback=",".join(app_settings.ALLOWED_MOBILE_PREFIXES),
	)
	first_name = FallbackCharChoiceField(
	verbose_name=_("first name"),
	help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
	max_length=12,
	choices=OPTIONAL_FIELD_CHOICES,
	fallback=OPTIONAL_SETTINGS.get("first_name", None),
	)
	last_name = FallbackCharChoiceField(
	verbose_name=_("last name"),
	help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
	max_length=12,
	choices=OPTIONAL_FIELD_CHOICES,
	fallback=OPTIONAL_SETTINGS.get("last_name", None),
	)
	location = FallbackCharChoiceField(
	verbose_name=_("location"),
	help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
	max_length=12,
	choices=OPTIONAL_FIELD_CHOICES,
	fallback=OPTIONAL_SETTINGS.get("location", None),
	)
	birth_date = FallbackCharChoiceField(
	verbose_name=_("birth date"),
	help_text=_GET_OPTIONAL_FIELDS_HELP_TEXT,
	max_length=12,
	choices=OPTIONAL_FIELD_CHOICES,
	fallback=OPTIONAL_SETTINGS.get("birth_date", None),
	)
	registration_enabled = FallbackBooleanChoiceField(
	help_text=_REGISTRATION_ENABLED_HELP_TEXT,
	fallback=app_settings.REGISTRATION_API_ENABLED,
	)
	saml_registration_enabled = FallbackBooleanChoiceField(
	help_text=_SAML_REGISTRATION_ENABLED_HELP_TEXT,
	verbose_name=_("SAML registration enabled"),
	fallback=app_settings.SAML_REGISTRATION_ENABLED,
	)
	mac_addr_roaming_enabled = FallbackBooleanChoiceField(
	help_text=_MAC_ADDR_ROAMING_ENABLED_HELP_TEXT,
	verbose_name=_("MAC address roaming enabled"),
	fallback=app_settings.MAC_ADDR_ROAMING_ENABLED,
	)
	social_registration_enabled = FallbackBooleanChoiceField(
	help_text=_SOCIAL_REGISTRATION_ENABLED_HELP_TEXT,
	fallback=app_settings.SOCIAL_REGISTRATION_ENABLED,
	)
	login_url = models.URLField(
	verbose_name=_("Login URL"),
	null=True,
	blank=True,
	help_text=_LOGIN_URL_HELP_TEXT,
	)
	status_url = models.URLField(
	verbose_name=_("Status URL"),
	null=True,
	blank=True,
	help_text=_STATUS_URL_HELP_TEXT,
	)
	password_reset_url = FallbackCharField(
	verbose_name=_("Password reset URL"),
	max_length=200,
	help_text=_PASSWORD_RESET_URL_HELP_TEXT,
	fallback=DEFAULT_PASSWORD_RESET_URL,
	validators=[password_reset_url_validator],
	)

Steps To Reproduce
Steps to reproduce the behavior:

1. Change the value of any of those settings referenced by the fallback fields
2. run ./tests/manage.py makemigrations
3. observe DB migrations being created by the django migration framework

Expected behavior
No DB migrations should be created.

Potential solutions

We need to define constants holding the default values, which never change.

Help in updating the documentation of openwisp-utils to highlight this better practice is welcome.

[SAMurai-16]

Hey @nemesifier , I tried reproducing the fallback field migration issue by changing OPENWISP_RADIUS_SMS_VERIFICATION_ENABLED and OPENWISP_RADIUS_SMS_MESSAGE_TEMPLATE in settings, but makemigrations keeps saying "No changes detected."

Is there a specific way to trigger this bug? Maybe certain package versions or a fresh install scenario?

[nemesifier]

@SAMurai-16 I found out this bug in OpenWISP Controller when testing the WHOIS Lookup and Estimated Location feature so I opened openwisp/openwisp-controller#1231 and then I assumed the same was happening everywhere else. I am not sure why it is happening there but not here, can you confirm it does happen in OpenWISP Controller?

[SAMurai-16]

@nemesifier i checked and the bug do exist in both the repos but is not being triggered by the way i am trying to reproduce it as both models and migrations use the same dynamic app_settings reference. If the migration system were properly isolated from settings, changes to OPENWISP_RADIUS_SMS_VERIFICATION_ENABLED and OPENWISP_RADIUS_SMS_MESSAGE_TEMPLATE would generate migrations. The solution would be indeed to use static constants for model field choices. Would you like me to implement it?

[pandafy]

@nemesifier i checked and the bug do exist in both the repos but is not being triggered by the way i am trying to reproduce it as both models and migrations use the same dynamic app_settings reference.

@SAMurai-16 you said that the bug exists in both repos, how did you confirm this if it didn't trigger in your local environment?

[SAMurai-16]

@pandafy So the bug only happens when migrations get "out of sync" and store hardcoded values instead of the dynamic "app_settings" references. I reproduced the bug by adding static values to a new migration while the models were using dynamic values.