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 loading of local settings. #461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olmari
Copy link
Collaborator

@olmari olmari commented Dec 22, 2023

In drxf/settings.py move importing of local settings last thing that is done, this way any/all custom settings gets priority as they're loaded last.

Fixes: #460

Signed-off-by: Sami Olmari [email protected]

In drxf/settings.py move importing of local settings last thing that is done, this way any/all custom settings gets priority as they're loaded last.

Signed-off-by: Sami Olmari <[email protected]>
@olmari olmari requested a review from tswfi December 22, 2023 22:53
@tswfi
Copy link
Member

tswfi commented Dec 22, 2023

In my testing I don't see any difference on where the settings_local is loaded.

  1. uncomment "ACCOUNT_IBAN": (ACCOUNT_IBAN, "IBAN of the association's bank account"), in CONSTANCE_CONFIG in settings.py
  2. add ACCOUNT_IBAN = "ABC123" in settings_local.py (the default value in settings.py is "FI12 3456 789"
  3. run ./manage.py shell_plus
  4. in the shell run
from constance import config
print(config.ACCOUNT_IBAN)

and it prints out ABC123

But what I do see is that Constance has added an object for the setting even if I have never saved anything into it...

Constance.objects.get(key='ACCOUNT_IBAN').value

Looks like constance initializes its objects when the key is queried for the first time. It will take the default value configured at that point in time and then keeps it in the db.

@olmari
Copy link
Collaborator Author

olmari commented Dec 22, 2023

That would conceptually match why local settings loading as last thing would make it also go...

Does generally things know local settings has priority? Or how else they know other than loading local settings in settings.py as last thing "overriding" anything set prior? Tho goes too deep into code for my knowledge anyways, kinda, sorta.

Copy link
Collaborator

@braaar braaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in order for this to function as I originally intended it we need to load settings from settings_local.py both before setting up constance and afterwards.

If you don't define a constance config in your local settings, you probably want your local settings to be loaded into the defaults that are defined in the constance config section here in settings.py.

Feels a bit clunky, but that's kinda how it has to be when the constance config reads from settings/settings_local.

I'm not very well versed in python or django, so the ordering may be moot.

Comment on lines -348 to -353
# Load non-default settings from settings_local.py if it exists
try:
from .settings_local import * # noqa
except ImportError:
pass

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Un-delete these lines and the PR will work as intended. Perhaps leave a comment like so:

settings_local is loaded so that the constance config will receive defaults from settings_local.

Comment on lines +384 to +389

# Load non-default settings from settings_local.py if it exists
try:
from .settings_local import * # noqa
except ImportError:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this work?

Suggested change
# Load non-default settings from settings_local.py if it exists
try:
from .settings_local import * # noqa
except ImportError:
pass
# Load constance config from settings_local.py if it exists
try:
from .settings_local import CONSTANCE_BACKEND, CONSTANCE_CONFIG # noqa
except ImportError:
pass

@braaar
Copy link
Collaborator

braaar commented Feb 15, 2024

Just to clarify: Here is the sequence of operations we need:

  1. Load regular settings from settings.py
  2. Load regular settings from settings_local.py
  3. Consume the settings from settings.py and settings_local.py in the constance config.
  4. (if constance config is set in settings_local.py) load the settings_local constance config, which should consume values from settings and setting_local just like the original constance config

Local settings should always override, therefore we need to override the constance config in the end, if applicable

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.

Constance settings are not read from settings_local.py
3 participants