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

Improve Apprise integration, with a focus on Ntfy #619

Merged
merged 5 commits into from
Feb 13, 2023
Merged

Conversation

amotl
Copy link
Member

@amotl amotl commented Feb 13, 2023

Hi again,

based on what @zoic21 reported at #607 (comment), this patch improves the mqttwarn <-> Apprise integration by propagating the mqttwarn data dictionary into the Apprise plugin template arguments. It is accompanied by a dedicated test case for the Ntfy notification service.

We may discuss some details on this patch before merging. Thanks for your feedback already!

With kind regards,
Andreas.

References

],
title="⚽ Message title ⚽",
message="⚽ Notification message ⚽",
data={"priority": "high", "tags": "foo,bar", "click": "https://httpbin.org/headers"},
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, tags are propagated 1:1 from a string to a string, without any transformation in between.

The Apprise Ntfy parameter breakdown documentation says:

Variable Required Description
tags No The ntfy tags to associate with the ntfy post. Use a comma and/or space to specify more then one.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, @zoic21 outlined at #607 (comment), that they would like to submit tags as a list? Please raise your voice if this would be important to you, then we can try to add a corresponding translator.

Copy link

Choose a reason for hiding this comment

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

Hello,
No string to string is better, I used array because it's mandatory by ntfy when we used json but it's no necessary by apprise

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent. Then let's just keep it as is, for the sake of simplicity.

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023

Codecov Report

Merging #619 (58770b6) into main (ae97072) will increase coverage by 2.40%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
+ Coverage   36.68%   39.08%   +2.40%     
==========================================
  Files          80       83       +3     
  Lines        3604     3746     +142     
==========================================
+ Hits         1322     1464     +142     
  Misses       2282     2282              
Flag Coverage Δ
unittests 39.08% <100.00%> (+2.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mqttwarn/services/apprise.py 100.00% <100.00%> (ø)
mqttwarn/services/apprise_multi.py 95.45% <100.00%> (ø)
mqttwarn/services/apprise_single.py 95.34% <100.00%> (ø)
mqttwarn/services/apprise_util.py 100.00% <100.00%> (ø)
mqttwarn/util.py 100.00% <0.00%> (ø)
mqttwarn/__init__.py 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zoic21
Copy link

zoic21 commented Feb 13, 2023

Hello,

I make some test with your change but I have some issue. My configuration :

[config:ntfy]
; Dispatch message to ntfy.
; https://github.com/caronc/apprise/wiki/URLBasics
; https://github.com/caronc/apprise/wiki/Notify_ntfy
module   = 'apprise_single'
baseuri  = 'ntfys://xxxx:xxxx@ntfy.xxxxx.net:443/loic'

[ntfy2]
targets = ntfy

I send this : {"topic":"loic","message":"coucou","priority":1,"tags":"grinning,snail","title":"plop"}. It's ok for priority and tags but not for message, topic and title. I received all json on message and no title (I also test with multiple topic and I received message on all topic).

@amotl
Copy link
Member Author

amotl commented Feb 13, 2023

Hi Loïc,

thank you for trying this.

It's ok for priority and tags but not for message, topic and title.

I think the message and title are not part of the "Apprise template arguments" list, and are routed a bit differently. In this manner, I think the classic way of configuring them via mqttwarn machinery, using, for example, format and title attributes within the corresponding dispatcher rule configuration section, would apply better, similar to your original post at #607 (comment).

https://github.com/jpmens/mqttwarn/blob/ae970727b2ab86cf4f28d8202da7f243d644a924/HANDBOOK.md?plain=1#L606-L617

With kind regards,
Andreas.

@zoic21
Copy link

zoic21 commented Feb 13, 2023

Ok with this configuration :

[config:ntfy]
; Dispatch message to ntfy.
; https://github.com/caronc/apprise/wiki/URLBasics
; https://github.com/caronc/apprise/wiki/Notify_ntfy
module   = 'apprise_single'
baseuri  = 'ntfys://xxxx:xxxx@ntfy.xxxxx.net:443/loic/jeedom'

[ntfy2]
targets = ntfy
format   = {message}
title    = {title} 

It's ok for title and message but not for topic, it'send message on all topic, I try with topic = {topic} on configuration but no luck.

@amotl
Copy link
Member Author

amotl commented Feb 13, 2023

Hi again,

Status quo

I think topic is a parameter of its own kind for Apprise » Ntfy. It is neither part of the mqttwarn data dictionary, nor it is an URL parameter of the Apprise configuration setting. Instead, the is obtained as URL path, and there can be multiple topics given, see also the upstream documentation at 1:

You can specify more than one topic such as:

ntfy://{user}:{password}/{hostname}/{topic1}/{topic2}

I think there is currently no way to propagate the topic to Apprise using an URL parameter. This example outlines it:

echo hello | apprise --debug "ntfy://user:password@ntfy.example.org/?topic=foo"
WARNING - A specified ntfy topic (ntfy.example.org) is invalid and will be ignored
WARNING - There are no ntfy topics to notify

The topic MUST be obtained as URL path instead:

echo hello | apprise --debug "ntfy://user:password@ntfy.example.org/foo"
DEBUG - ntfy Payload: {'topic': 'foo', 'message': 'hello'}

Question

So, with respect to your example URL ntfys://xxxx:xxxx@ntfy.xxxxx.net:443/loic/jeedom, the Ntfy topics addressed would be loic and jeedom. Isn't that working?

Outlook

Please let me know if you would have a need to supply the topic parameter dynamically / by other means than coding it into the Apprise URL within the mqttwarn.ini configuration file. For example, if you are aiming to submit it as a separate item within the JSON data dictionary to the MQTT broker, and as such, to mqttwarn, like {"topics": "topic1,topic2"}, we will probably need to add special handling for that.

As I will be merging this now, please request such a feature at GH-607 again. Thanks!

With kind regards,
Andreas.

Footnotes

  1. https://github.com/caronc/apprise/wiki/Notify_ntfy

@amotl amotl changed the title Improve Apprise integration Improve Apprise integration, with a focus on Ntfy Feb 13, 2023
With the previous variant, it would use the "cloud" mode incorrectly,
because the default code examples should outline the "private" mode
instead.
... by propagating the mqttwarn data dictionary into the Apprise plugin
template arguments.
@amotl amotl merged commit 543f35d into main Feb 13, 2023
@amotl amotl deleted the amo/apprise-improve branch February 13, 2023 16:46
@amotl
Copy link
Member Author

amotl commented Feb 13, 2023

mqttwarn 0.32.0 has been released, including this improvement. Thanks, again!

@zoic21
Copy link

zoic21 commented Feb 14, 2023

Hello,

Thank for the release, I test and it's perfectly work. Topic it's not really needed I can manage it with mqtt topic.

So it's perfect, thank for your fast correction and release.

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.

3 participants