Skip to content

Fix the way Coldfront handles settings #708

Open
@Eric-Butcher

Description

@Eric-Butcher

Django currently handles settings by using a function coldfront.core.utils.common.import_from_settings to get settings. This is usually done at the top of a module.

An excerpt from coldfront.core.allocation.models.py:

ALLOCATION_ATTRIBUTE_VIEW_LIST = import_from_settings("ALLOCATION_ATTRIBUTE_VIEW_LIST", [])
ALLOCATION_FUNCS_ON_EXPIRE = import_from_settings("ALLOCATION_FUNCS_ON_EXPIRE", [])
ALLOCATION_RESOURCE_ORDERING = import_from_settings("ALLOCATION_RESOURCE_ORDERING", ["-is_allocatable", "name"])

The problem is that when importing settings in this way, the individual variables being assigned to are not the actual settings, they are only copies of the value the settings had when the module was first instantiated. This causes problems when trying to test functions, as the django provided utilities such as the @override_settings decorator cannot operate on a module level variable. See discussion in #699 . Instead, tests are forced to using patching. This is problematic as patching tightly couples test cases to the implementations being tested. It also means the programmer testing any functionality has to patch a variable in every single module where the call stack of that tested functionality can ever reach. This open the rooms for more errors in testing and makes testing significantly more difficult.

According to Django's documentation settings should be accessed by importing from django.conf import settings and by accessing through settings.<setting-name>.

Additionally, Django's documentation on testing specifically says not to have settings as module level constant variables as coldfront is currently doing:

... Finally, avoid aliasing your settings as module-level constants as override_settings() won’t work on such values since they are only evaluated the first time the module is imported.

If settings are accessed through django.conf then the @override_settings decorator will work properly for test cases.

There are two ways that this problem could be sensibly approached:

  1. Putting in place a rule for all new contributions to access settings through the idiomatic method. Preexisting code will be updated to use the idiomatic method the next time it is updated or tested.
  2. A complete refactor for all of coldfront is performed replacing each SETTING_VAR with settings.SETTING_VAR in one PR.

Coldfront maintainers should pick one of these strategies or come up with another one to address this issue. This well help coldfront to be more stable and testable.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions