Skip to content
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

Merged
merged 6 commits into from
Aug 9, 2024

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Jul 17, 2024

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.

@pandafy pandafy force-pushed the fallback-fields branch 2 times, most recently from 2b6885b to a0fab95 Compare July 18, 2024 09:12
@coveralls
Copy link

coveralls commented Jul 18, 2024

Coverage Status

coverage: 98.655% (-0.002%) from 98.657%
when pulling 3b35c79 on fallback-fields
into 7dd725e on master.

Copy link
Member

@nemesifier nemesifier left a 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.

Copy link
Member

@nemesifier nemesifier left a 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.

@@ -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,
Copy link
Member

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.

pandafy added 4 commits August 7, 2024 21:54
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.
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@nemesifier nemesifier merged commit 0663ac3 into master Aug 9, 2024
15 checks passed
@nemesifier nemesifier deleted the fallback-fields branch August 9, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants