Skip to content
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

Base record replace webhooks with events #59

Merged

Conversation

farskipper
Copy link

I removed all webhook stuff from BaseRecord so it now just uses events.

I namespaced the events like this: acapy::record::{RECORD_TOPIC}::{state} This way we can easily catch all the record events and forward them to webhooks.

Copy link
Member

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great! One minor comment that I'll leave to your discretion whether it should be changed; otherwise I think this is ready to merge.

"""
if not payload:

if not self.RECORD_TOPIC or not self.state or not payload:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling out the logic I'm seeing to validate intent: so if the BaseRecord subclass does not define a RECORD_TOPIC or the state has not been correctly set on the record, or if no payload is given, no event will be emitted. Looking at the diff, this is what was happening before as well, just simplified.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, same intent as before, we don't want to send events if there is not topic+state+payload. Before, there was no state and you could pass in a webhook topic to override.

Comment on lines 712 to 716

if not webhook_topic:
match = EVENT_PATTERN_RECORD.search(event.topic)
webhook_topic = match.group(1) if match else None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it perhaps simplify the event handling here to separate this into two subscribers, one for the webhook pattern and one for the record pattern?

@farskipper
Copy link
Author

farskipper commented Apr 5, 2021

@dbluhm I split up the admin event subscriber, it's still a little tedious b/c the topics need to be extracted from the regex, but it is nice to have them separated.

See commit: 4218259

@dbluhm dbluhm merged commit 99bcd4d into event-bus-webhook-refactor Apr 5, 2021
@farskipper farskipper deleted the event-bus-webhook-refactor-base_record branch April 5, 2021 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants