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
Extend Apprise JSON notification functionality with programmatic data #1355
Extend Apprise JSON notification functionality with programmatic data #1355
Conversation
I guess this code was written before type checking was implemented. I'm going to leave it alone and add |
Hey! Just wanted to give you heads up, I'm probably not going to be able to dig into this PR (or your other ones) until this weekend. Cursory look pretty good though, thanks for all your work lately! |
No problem! I had some free time this week, and you guys made spinning up a dev environment insanely easy with v1 |
Hmm. I didn't realize the limitation of the payload for the Apprise integration. Maybe that's not the best approach for this? My concern is that the head isn't a super expected place to find data, and it seems add some complexity to the overall usage. I wonder if it would make more sense to just use the requests package to make a post request with the What do you think? |
I was just about to post a comment with a couple other alternatives, but then I stumbled upon this thread: Which led me here: Looks like you can add custom keys to the JSON payload, it's just not documented yet. I'm going to toy around with this |
This commit replaces the {
"version": "1.0",
"title": "Recipe Updated",
"message": "Pasta Fagioli has been updated, http://localhost:8080/recipe/pasta-fagioli",
"attachments": [],
"type": "info",
"event_type": "update",
"item_type": "recipe",
"item_id": "b13a63a1-66a1-46f6-941d-544bcd87f950",
"slug": "pasta-fagioli"
} |
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.
Overall, looks pretty good. Theres a few minor things that I think need to get fixed.
I do think we should to utilize a class of some-sort to define what the event_source type looks like. I think having a simple class that allows for extension would be sufficient (see example below). That would give us a little type safety when calling and ensure that typo errors are minimal.
class EventSource:
event_type: str
item_type: str
item_id: UUID4 | int
kwargs: dict
def __init__(self, event_type: str, item_type: str, item_id: UUID4 | int, **kwargs) -> None:
self.event_type = event_type
self.item_type = item_type
self.item_id = item_id
self.kwargs = kwargs
def dict(self) -> dict:
return {
"event_type": self.event_type,
"item_type": self.item_type,
"item_id": str(self.item_id),
**self.kwargs,
}
Let me know if you need any support on this one. Happy to take on some of the rework as needed.
if self.event_source: | ||
urls = [ | ||
# We use query params to add custom key: value pairs to the Apprise payload by prepending the key with ":". | ||
# As of 2022-11-06 this is undocumented, but discussed in this pull request: | ||
# https://github.com/caronc/apprise/pull/547 | ||
EventBusService.merge_query_parameters(url, {f":{k}": v for k, v in self.event_source.items()}) | ||
for url in urls | ||
if EventBusService.is_json_url(url) | ||
] | ||
|
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.
Won't this filter out any non json_urls? Was that intended behavior here?
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.
Yeah, custom key value pairs are only supported by the custom form, custom XML, and custom JSON endpoints. The original implementation used custom headers which was only supported by JSON, but I can add XML and form endpoints and leave a note explaining why.
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.
I noticed that limitation too, kind of a bummer. I think the problem here is that if the is_json_url, function returns false we discard the url all together. Wouldn't that break regular Apprise integrations like discord where we're implementing them as just a message service? If we specify an Apprise URL for discord AND the event_source is defined, we'll never make the call.
Or am I reading this wrong?
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.
Oh wow I totally missed that, you're right. I was only testing the JSON ones. I can an else statement so we keep all URLs
|
||
@staticmethod | ||
def merge_query_parameters(url: str, params: dict): | ||
scheme, netloc, path, query_string, fragment = urlsplit(url) | ||
|
||
# merge query params | ||
query_params = parse_qs(query_string) | ||
query_params.update(params) | ||
new_query_string = urlencode(query_params, doseq=True) | ||
|
||
return urlunsplit((scheme, netloc, path, new_query_string, fragment)) | ||
|
||
@staticmethod | ||
def is_json_url(url: str): | ||
return url.split(":", 1)[0].lower() in ["json", "jsons"] |
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.
Can you add some basic unit-tests for these two methods?
|
||
if event_source is None: | ||
event_source = {} | ||
|
||
self.event_source = event_source |
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.
I don't quite understand the need for this. Could you just use event_source
as a function level variable and remove it from the class?
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.
I think I did that because of something to do with it getting passed to the dispatch
function and then added to the async _dispatch
function, but looking at it now I have no idea why I chose to make it a class attr as opposed to a function var ¯\(ツ)/¯ I'll change it
fixed bug that excluded non-custom notification types
I tweaked the shopping list item event trigger slightly to make it more consistent with the new |
LGTM, I think we can do a separate PR for the documentation so we don't run into anymore merge conflicts with some other stuff that's going on. I'm thinking a "Webhooks" Page in the documentation that outlines the limitations of the Apprise integration and some of the extensions. |
Overview
While the existing Apprise notification implementation works great for general use, it's not as useful for programmatic use. This PR adds programmatic data to the Apprise custom JSON notification implementation. This was done to solve for feature request #1350.
Functional changes
When a user creates a notification using either the
json://
orjsons://
endpoint, the message contains a custommealie-event-source
header. As an example, here is themealie-event-source
header for when a new recipe is created:This is implemented for all existing CRUD events (recipies, cookbooks, categories, tags, and shopping lists).
The event listener for shopping lists has been extended to include CRUD operations for shopping list items. The event source header is a bit more customized to accommodate this:
In order to avoid reworking the API for adding and removing recipe ingredients to a shopping list, the shopping list id and all shopping list item ids are added to the header, not just the ones that were modified. A different event type is used to convey this:
Implementation
To achieve this, a new
event_source
arg was added to theevent_bus.dispatch
function. The dispatch function is updated to add the event source JSON data to the headers as a URL param (per the Apprise docs):What's Next?
The shopping list items are a bit of a hack; ideally the user is able to subscribe to just shopping list events, just shopping list item events, or both. However, this would require a database change and some frontend changes.
When a full recipe is added or removed to/from a shopping list, the API doesn't return the details of which items were impacted, it just returns the shopping list in its new state (including all items). A more proper solution would be to either send a mixed header (items a/b/c were created and items d/e/f were updated) or send two events: one for each CRUD operation. This requires a re-write of the relevant bits of the shopping list API and its dependencies.