Fix next_occurrence function

jemrobinson commented 1 month ago

:white_check_mark: Checklist

[x] You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
[x] You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
[x] Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

:vertical_traffic_light: Depends on

n/a

:arrow_heading_up: Summary

Ensure that next_occurrence only adds one day when necessary, to avoid it choosing a datetime more than 1 day in the future

:closed_umbrella: Related issues

Closes #1895

:microscope: Tests

Check CI

github-actions[bot] commented 1 month ago

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
data_safe_haven/functions
strings.py
Project Total

This report was generated by python-coverage-comment-action

jemrobinson commented 1 month ago

next_occurrence is supposed to give the next time that a desired local time happens in UTC. For some Azure applications the time that you pass cannot be in the past.

Previously, we took local-time-in-UTC + 1 day which is always in the future (over the lifetime of a deploy operation). However, this can break the test which assumes that the output of next_occurrence will always be within one day of current-time-in-UTC.

Possible solutions are:

relax the test so that next_occurrence must be within 2 days of now
update the function so that it only adds 1 day if the suggested time is in the past
- checking for in-the-past uses now-plus-1-minute to ensure that we don't end up with a time that is currently valid, but ends up being invalid by the time it gets given to an Azure function
- possibly this should be longer (e.g. 5 minutes) and have a better explanation as to why

I'm happy to switch to solution (1) if you prefer.

JimMadge commented 1 month ago

Hmm, some thoughts that might help,

Why does the test assume next_occurrence will be within one day? Does it have to be? Will Azure reject times not within one day of now? Could next_occurrence simply being in the future be a better test?
The fudge sounds more reasonable given how the Azure API works (and operations are not instantaneous). Adding a comment to explain it would be good.
Should this really be >1 function? next_occurrence and ensure_within_1d?

jemrobinson commented 1 month ago

Why does the test assume next_occurrence will be within one day?

Just because that's the intention of the function (to generate the UTC timestamp corresponding to the next time it is e.g. 3am in Perth)

Does it have to be? Will Azure reject times not within one day of now?

No, it doesn't have to be, but some of these schedulers won't start until the time occurs, so it would bad bad if this was e.g. 10 days in the future.

Could next_occurrence simply being in the future be a better test?

Potentially, but I think there should be a maximum window on how far in the future it could be (see above)

The fudge sounds more reasonable given how the Azure API works (and operations are not instantaneous). Adding a comment to explain it would be good.

Happy to remove this fudge (esp. given your comments above)

Should this really be >1 function? next_occurrence and ensure_within_1d?

I think it only needs to be one function - the important thing is to come up with a UTC value that represents when your thing will happen in the future.

jemrobinson commented 1 month ago

Looks like you can't mock builtin methods. I'll have a go with freezegun.

alan-turing-institute / data-safe-haven