#2058 Twilio status override, Celery scheduler #2088
Conversation
| .where(Notification.notification_type == 'sms') | ||
| .where(Notification.sent_by == 'twilio') | ||
| .where(~Notification.status.in_(NOTIFICATION_STATUS_TYPES_COMPLETED)) | ||
| .where(Notification.created_at < one_hour_ago) |
There was a problem hiding this comment.
should this be greater than one_hour_ago?
There was a problem hiding this comment.
No, the variable one_hour_ago is one hour in the past, so we're looking for things less than that. Greater than would be newer.
There was a problem hiding this comment.
I don't insist that you make a change for this, but I think you can put all the clauses in one "where" rather than have a separate "where" for each clause.
| ) | ||
| def test__get_notifications_datefilter(sample_notification, minute_offset, expected): | ||
| """Test that _get_notifications() returns either a list with the test notification, or an empty list, depending | ||
| on the parametrized minute_offset. If the notification was created more than one hour ago, it is not returned.""" |
There was a problem hiding this comment.
I thought we wanted to only fetch notification created earlier than one hour ago?
There was a problem hiding this comment.
Oops, that comment is backwards.
| (180, True), | ||
| ], | ||
| ) | ||
| def test__get_notifications_datefilter(sample_notification, minute_offset, expected): |
There was a problem hiding this comment.
there is an extra underscore here, please remove
| .where(Notification.notification_type == 'sms') | ||
| .where(Notification.sent_by == 'twilio') | ||
| .where(~Notification.status.in_(NOTIFICATION_STATUS_TYPES_COMPLETED)) | ||
| .where(Notification.created_at < one_hour_ago) |
There was a problem hiding this comment.
I don't insist that you make a change for this, but I think you can put all the clauses in one "where" rather than have a separate "where" for each clause.
app/celery/twilio_tasks.py
Outdated
| .order_by(Notification.created_at) | ||
| .limit(current_app.config['TWILIO_STATUS_PAGE_SIZE']) | ||
| ) | ||
| return db.session.execute(query).scalars().all() |
There was a problem hiding this comment.
I think this works:
| return db.session.execute(query).scalars().all() | |
| return db.session.scalars(stmt).all() |
app/celery/twilio_tasks.py
Outdated
|
|
||
| current_app.logger.info('Getting notifications to update status') | ||
| one_hour_ago = datetime.now(timezone.utc) - timedelta(hours=1) | ||
| query = ( |
There was a problem hiding this comment.
We decided earlier that we would name all queries "stmt" (statement) for consistency.
| query = ( | |
| stmt = ( |
app/celery/twilio_tasks.py
Outdated
| current_app.logger.info('Getting notifications to update status') | ||
| one_hour_ago = datetime.now(timezone.utc) - timedelta(hours=1) | ||
| query = ( | ||
| select([Notification]) |
There was a problem hiding this comment.
| select([Notification]) | |
| select(Notification) |
There was a problem hiding this comment.
I have these changes implemented and deployed
Description
This PR adds a celery beat task to update Twilio messages that are older than hour, and not in a final state. The celery task is executed every 5 minutes, and pulls back at most 500 notifications. If a 429 is encountered, the task stops. Any other exceptions are ignored.
issue #2058
How Has This Been Tested?
Checklist