Feature/allocation model tests

[Eric-Butcher]

This is the start of adding to the Allocation app test suite. I have wrote several tests covering the clean method, the save method, and the get_information property.

Many of the tests for the clean method use datetimes. I am not sure how well coldfront currently handles dates and times so these may need to be revised as currently they are quite naive.

Because the save() method uses an external setting to control whether or not it calls arbitrary functions it was quite hard to test and I had to get quite creative. Some of the code for that is admittedly ugly but it seems that it was the only way I could test it without making huge changes to the current way the code is written. I did need to make a small change to the save() method so that it refreshes the setting ALLOCATION_FUNCS_ON_EXPIRE when the method is called instead of relying on the value from when the module was instantiated by python. Without doing this there would be no way to test that behavior during test time. Some of the code that assists with the TestCase for save() had to live outside of the actual class because of issues with the way python initializes things and the meta-programming the I had to use to be able to test that an external function actually was being called by save().

It seems that it might be a bigger issue throughout the codebase of settings being loaded in at the top of a python file but then having their values used inside of functions defined in that file. This would mean that if someone ever went in a changed the value for an environment variable it might not take effect until they restarted the entire application. Let me know if you would like me to make an issue / PR to go through the codebase and fix everywhere that happens.

[Eric-Butcher]

I did more research on how Django handles datetimes and inspected how coldfront currently handles timezones.

It seems that coldfront is appropriately using the configuration environment variables to set the locality. However, in most instances in this codebase it seems that the project uses datetime.datetime.now() to create and reference datetimes and dates which are timezone naive.

For now I am using all dates and times as utc. The django.utils.timezone.now() function returns a timezone aware datetime object for the current time represented in UTC. It seems like it would be best practice to replace all instances of datetime.datetime.now() with django.utils.timezone.now() throughout the project. Similarly, all instances of datetime.date.today() should be replaced by timezone.now().date() to ensure that all dates we are storing and comparing are in UTC. Another option would be to change all DateFields where appropriate to DateTimeFields.

If having timezone aware tests are important it might be worth considering bringing a global time mocking library in such as freezegun or time machine.

https://docs.djangoproject.com/en/5.2/topics/i18n/timezones/

[Eric-Butcher]

So I went ahead and replaced all occurrences of replacing environment variables using django.test.override_settings with monkey patches using unittest.mock.patch.

The problem with testing any functions that relied on the value of environment variables was that the code does not actually use the values of the environment variables inside of its functions.

For example, at the top of the file coldfront.core.allocation.models there are lines that create variables with the values of the environment variables:

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"])

Crucially, these are now new variables that are set to the values of the environment variables in the settings, but are not actually the environment variables in the settings. The code in the module then uses these newly created variables.

This meant that when trying to change the values of the environment variables in test cases using django.test.override_settings, the actual variable that the functions were using did not get changed and would be stuck to whatever the default value was in the source code.

The first approach I used to fix this was to just put a new line at the top of each function that used an env value to get the current value of the environment. However, this could be problematic because I do not know how python would handle it if an admin ever came in and manually changed the values in settings file at some point in time in the middle of a coldfront deployment. When the function would then be called and the env value fetched, I am not sure if it would use the new or old value.

The second approach is to use monkey patching instead, which hot-changes the value specifically of the variable inside the particular module when the tests are run using unittest.mock.patch. This is a very important distinction because this could cause confusion down the road. This is not actually changing the value of the environment variable while testing. It will only change the specific variable in the specific module being tested. You could now get a scenario where you are testing some module_a.function_a() that calls inside of it some module_b.function_b(). If both of these functions at same point use environment variables, then you would need to perform two separate patches, one for each module; even if they are referencing the same value in the env ultimately. This allows to test without changing the way the code is currently written.

[Eric-Butcher]

Anton requested that this get split up into different PRs to make it more reviewable.

Now split into #706 #705 #704 and #703

I will close this PR once those are looked at. Will probably take the information here and move it into an issue.

[Eric-Butcher]

Anton requested that this get split up into different PRs to make it more reviewable.

Now split into #707 #705 #704 and #703