[change] Update trigger_vpn_server_endpoint to send generic_message #… #1104
Conversation
| # of the django success message container | ||
| # with the ow-notification container | ||
| # https://github.com/openwisp/openwisp-notifications/issues/264 | ||
| sleep(2) |
There was a problem hiding this comment.
This should be optional. Eg:
if snooze:
sleep(snooze)| @@ -206,3 +210,60 @@ def get_default_templates_queryset( | |||
| def get_config_error_notification_target_url(obj, field, absolute_url=True): | |||
| url = _get_object_link(obj._related_object(field), absolute_url) | |||
| return f"{url}#config-group" | |||
|
|
|||
|
|
|||
| def send_api_task_notification(type, **kwargs): | |||
There was a problem hiding this comment.
| def send_api_task_notification(type, **kwargs): | |
| def send_api_task_notification(type, snooze=None, **kwargs): |
| if task_result == "error": | ||
| self._send_api_task_notification("recovery", **kwargs) | ||
| cache.set(task_key, "success", None) | ||
| handle_recovery_notification(task_key, **kwargs) |
| self._send_api_task_notification( | ||
| "error", status_code=response.status_code, **kwargs | ||
| ) | ||
| handle_error_notification(task_key, response, **kwargs) |
| ) | ||
|
|
||
|
|
||
| def handle_recovery_notification(task_key, **kwargs): |
There was a problem hiding this comment.
Why are we extracting this logic here if we aren't reusing it for trigger_vpn_server_endpoint? What's the point of this work? We don't do changes for the sake of it. There's no advantage in exctracting this heare if we don't reuse it as discussed initially.
There was a problem hiding this comment.
I thought this is something that can be used and would be needed to be exposed in future and looks cleaner. Just as we thought _send_api_task_notification wasn't meant to be exposed separately initially but as we see it had to be.
I can undo those 2 functions back where they were and it won't affect the current implementation but I think they also follow the utility.
openwisp_controller/config/tasks.py
Outdated
| @@ -121,6 +123,12 @@ def trigger_vpn_server_endpoint(endpoint, auth_token, vpn_id): | |||
| f"Response status code: {response.status_code}, " | |||
| f"VPN Server UUID: {vpn_id}", | |||
| ) | |||
| send_api_task_notification( | |||
There was a problem hiding this comment.
I was expecting handle_error_notification and handle_recovery_notification to be used in this function.
We'll need to update existing tests to assert these are being executed as expected.
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
Extracted
send_api_task_notification,handle_error_notificationandhandle_recovery_notificationhelpers from existing code to send error/recovery messages via generic notifications. Updatedtrigger_vpn_server_endpointto use this mechanism for better observability of failures.
Closes #1049
Checklist
Reference to Existing Issue
Closes #1049.