-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat increase frequency of schedulednotify credentials #36180
feat increase frequency of schedulednotify credentials #36180
Conversation
I'm letting autoformat hit this file to make it match our current standards before I actually make any code changes.
`notify_credentials` has 2 ways of running. 1. The manual, one-off method which uses `--args_from_database` to specify what should be sent. 2. [The automated method](https://github.com/openedx/edx-platform/blob/7316111b35c8db0b93665e00aa4071685d772ab3/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py#L157-L159), which runs on a regular schedule, to catch anything which fell through the cracks. The automated method does a certain amount of time/date math in order to calculate the entry point of the window based on the current runtime. This is, I assume, why it has some hardcoded logic; it's not at all simple to have a `cron`-run management command running on a regular cadence that can do the same time logic. ```py if options['auto']: options['end_date'] = datetime.now().replace(minute=0, second=0, microsecond=0) options['start_date'] = options['end_date'] - timedelta(hours=4) ``` However, it is not ideal that the actual time window of 4 hours is hardcoded directly into `edx-platform`. This fix * creates an overridable `NOTIFY_CREDENTIALS_FREQUENCY` that defaults to 14400 seconds (4 hours). * pulls that frequency into the quoted code Using seconds allows maximum flexibility. FIXES: APER-3383
…chedulednotify_credentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
removing too many blank lines
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
`notify_credentials` has 2 ways of running. 1. The manual, one-off method which uses `--args_from_database` to specify what should be sent. 2. [The automated method](https://github.com/openedx/edx-platform/blob/7316111b35c8db0b93665e00aa4071685d772ab3/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py#L157-L159), which runs on a regular schedule, to catch anything which fell through the cracks. The automated method does a certain amount of time/date math in order to calculate the entry point of the window based on the current runtime. This is, I assume, why it has some hardcoded logic; it's not at all simple to have a `cron`-run management command running on a regular cadence that can do the same time logic. ```py if options['auto']: options['end_date'] = datetime.now().replace(minute=0, second=0, microsecond=0) options['start_date'] = options['end_date'] - timedelta(hours=4) ``` However, it is not ideal that the actual time window of 4 hours is hardcoded directly into `edx-platform`. This fix * creates an overridable `NOTIFY_CREDENTIALS_FREQUENCY` that defaults to 14400 seconds (4 hours). * pulls that frequency into the quoted code Using seconds allows maximum flexibility. FIXES: APER-3383
note
This relies on #36179; until that one is merged this will look a little busy.
Problem being fixed
notify_credentials
has 2 ways of running.--args_from_database
to specify what should be sent.The automated method does a certain amount of time/date math in order to calculate the entry point of the window based on the current runtime. This is, I assume, why it has some hardcoded logic; it's not at all simple to have a
cron
-run management command running on a regular cadence that can do the same time logic.However, it is not ideal that the actual time window of 4 hours is hardcoded directly into
edx-platform
.Description of fix
This fix
NOTIFY_CREDENTIALS_FREQUENCY
that defaults to 14400 seconds (4 hours).Using seconds allows maximum flexibility.