-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
[fix] Fixed fallback fields having default values #534
Conversation
2b6885b
to
a0fab95
Compare
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.
I have the impression that this is not enough.
Premise: I have destroyed the OrganizationRadiusSettings
instance in my env and created it from scratch. I verified by querying with the sqlite3
command line tool that the values in the DB are NULL, that worked.
Redundancy
Why do we have to define always:
null=True,
blank=True,
default=None,
On all fields?
Wouldn't be better to set these attributes as default in the base classes to reduce redundancy and ensure that if in the future we need to add any new field, we won't make the mistake of forgetting to set these?
Inconsistency
Why doesn't sms_message
have default=None
? Is that on purpose?
Is it necessary? Can we make all the fields consistent?
Unexpected value
In [6]: org.radius_settings.sms_verification is None
Out[6]: True
Shouldn't the above return either True
or False
?
The docs for that field say:
The field will use the fallback value whenever the field is set to None.
This field is particularly useful when you want to present a choice between enabled and disabled options, with an additional "Default" option that reflects the fallback value.
"The field will use the fallback value": did we mean to say "return" instead of "use" here? Or what does "use" mean?
I would expect this field to return the fallback value if the value in the db is None
, but that is not happening.
The same happens for needs_identity_verification
and any other FallbackCharChoiceField
. The fallback choice fields seem to be broken.
I see that we have a method called get_setting
in AbstractOrganizationRadiusSettings
which returns the fallback value if the field returns None
, but isn't that redundant with the logic defined in the fields and doesn't that defeat the work we've done to make these fields reusable?
I see 2 issues:
- the fields are not really reusable without additional code
- the documentation of the fields is misleading / wrong
I think the best solution is to fix this in the base fields and remove any redundant code here.
Data migration
We need to add a data migration that removes any value in the database which is the same as the fallback value and sets NULL
.
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.
Looks good, please rebase on the latest master (the docs have changed), you may also need to run openwisp-qa-format again.
openwisp_radius/base/models.py
Outdated
@@ -1238,7 +1239,7 @@ class AbstractOrganizationRadiusSettings(UUIDModel): | |||
blank=True, | |||
max_length=200, | |||
help_text=_PASSWORD_RESET_URL_HELP_TEXT, | |||
default=DEFAULT_PASSWORD_RESET_URL, | |||
default=None, |
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.
we should be able to remove null
, blank
and default
now from every fallback field.
Fallback fields should not have default field set to the fallback value as it would require users to manually update those fields when the fallback value is changed.
bcd2428
to
a4e3370
Compare
Fallback fields should not have default field set to the fallback value as it would require users to manually update those fields when the fallback value is changed.