AdminServer is now the only one to send_webhook

[farskipper]

First stab at the refactor. All the tests are passing and seems to be working fine with the toolbox.

Any event with a topic acapy::webhook::(.*) will get forwarded as a webhook. However, I'm thinking now might be a good time to improve the naming convention of these events, while still maintaining backwards compatibility with webhook topics.

i.e. a mapping from event name to webhook topic might be:

{
  "acapy::basicmessage_received": "basicmessages",
  "acapy::ping": "ping",
  "acapy::ping_response": "ping",
  "acapy::driver_get_active_menu": "get-active-menu",
  "acapy::driver_perform_menu_action": "perform-menu-action",
  "acapy::problem_report": "problem_report",
}

[dbluhm]

However, I'm thinking now might be a good time to improve the naming convention of these events, while still maintaining backwards compatibility with webhook topics.

i.e. a mapping from event name to webhook topic might be:

{
  "acapy::basicmessage_received": "basicmessages",
  "acapy::ping": "ping",
  "acapy::ping_response": "ping",
  "acapy::driver_get_active_menu": "get-active-menu",
  "acapy::driver_perform_menu_action": "perform-menu-action",
  "acapy::problem_report": "problem_report",
}

I agree with this approach with the one suggestion that adding more "namespaces" could be nice. So acapy::basicmessage_received might be acapy::basicmessage::received.

[dbluhm]

Looking pretty good so far. Left some comments and thoughts.

[farskipper]

Made some changes:

• Brought back responder.send_webhook with a deprecation warning
• Added comment to explain why if TYPE_CHECKING is used
• Added EVENT_WEBHOOK_MAPPING

Question: Is it ok to remove these?

• BaseAdminServer.add_webhook_target - superseded by profile.settings.get("admin.webhook_urls")
• BaseAdminServer.remove_webhook_target - superseded by profile.settings.get("admin.webhook_urls")
• BaseAdminServer.send_webhook - Now that we have events, this seems like an implementation detail, not necessarily something all admin servers need to support.

[dbluhm]

BaseAdminServer.send_webhook - Now that we have events, this seems like an implementation detail, not necessarily something all admin servers need to support.

Sounds good to me

• BaseAdminServer.add_webhook_target - superseded by profile.settings.get("admin.webhook_urls")
• BaseAdminServer.remove_webhook_target - superseded by profile.settings.get("admin.webhook_urls")

I see, I didn't pick up on the self.webhook_urls not even being used in the send_webhook method; I defer to your findings.

[dbluhm]

Apologies, I've lost track of all the different conversations -- are there further changes needed after our discussion or is this ready for final review?

[farskipper]

@dbluhm I think it's ready for final review if we are ok removing BaseAdminServer.[add/remove]_webhook_target All the tests are passing and seems to work fine with the toolbox.

Are there other implementations of BaseAdminServer running around out there? If so we'll just have to note that it's a breaking change to rely on add_webhook_target and you should be using profile.settings.get("admin.webhook_urls") instead. Which I think is fine b/c that's all it's really using right now anyways.

[dbluhm]

Nice work! Looking really good!

Looking through, I think we should generalize the send_webhook method on BaseRecord. At the moment, record updates (which as it turns out is one of the most useful and frequent types of webhook event sent out) are emitting an event specifically scoped to the webhook.

I recommend:

1. Rename BaseRecord.send_webhook to something event oriented, perhaps emit_event or similar.
2. Use acapy::RECORD_TOPIC::STATE as the topic.
   • This will require adding mappings as we have for the basic messages and pings.

This one might get a little messy but I think I would also recommend modifying the BaseRecord.save and BaseRecord.post_save (and any other relevant) methods to take a bool event instead of webhook or else keep webhook around for backwards compatibility and add the event but make sure they are the same and emit a deprecation warning on webhook usage. I'm not bent on this one but I think abstracting away the webhook as much as possible will make the system cleaner overall. If this ends up being a huge and/or brittle change, we'll likely need to regroup and come up with a different solution.

[farskipper]

@dbluhm Ok, I made a PR to make those changes: #59 I did a PR so it'd be easy to diff and back out if we decide against it.