Feature: add webhook notification capability #724

Guts · 2021-05-01T10:55:49Z

I propose here to add the possibility to send a notification to a web hook for each new comment.

Notable changes

creation of a new WebHook class in the notifications module
by default, it sends a POST request with the same metadata as the mail notification (name, mail and site of the author as well as the moderation links)
add requests as dependency
possibility for the end user to pass a JSON template to the webhook (useful for tools allowing formatting, like Slack/Teams/Matrix...). An example of template for Slack is provided (into contrib subfolder) with this render:

Questions

Maybe it is possible with Werkzeug, Flask or the stdlib directly for the POST request? I'm not expert on this side so, don't hesitate on review my code.
I wanted to use f-strings but apparently the project is still tested on Python 3.5 (which has reached EOL), but not 3.9. Is it a voluntary choice?
I had to fix dependencies consistency between setup.py and tox.ini, in order to get CI fixed. Requirements are not well documented and the development workflow is quite unclear. Would you accept a PR to switch to requirements.txt files and/or pyproject.tom (recomended by tox anyway)?

Misc

add cache to Travis to make it faster
Close my question Get signals in order to send a notification through a webhook #722

Guts · 2021-06-17T05:05:46Z

ping @posativ @ix5 @blatinier : any chance to see it merged or reviewed?

ix5 · 2021-06-17T14:25:46Z

Can't say much, but here are my thoughts:

f-strings are pretty dangerous, see Armin Ronacher: Be Careful with Python's New-Style String Format
Avoid adding new dependencies. POST requests can be done with urllib
Squash your commits

Guts · 2021-06-17T16:16:45Z

Thanks for your feedback.

f-strings are pretty dangerous, see Armin Ronacher: Be Careful with Python's New-Style String Format

I did not know that but it's quite old (not sure it's still applicable) and it's a concern only when we format some secret no? If you prefer, we can switch to f"{var}" or "%s" % var.

Avoid adding new dependencies. POST requests can be done with urllib

Sure but it would be quite harder to handle different cases (HTTP warnings, redirections, etc...).

Squash your commits

I will.

Exclude .venv from flake8

Add webhook Fix string formatting Fix string formatting Make it compatible with Python 3.5 ?! Add default webhook section Clean up and docstrings Handle HTTP errors Add webhook template for Slack Add requests as dependency Remove unused var Complete post data without template Enable cache

jelmer · 2021-06-21T23:46:34Z

It would be great if this had some unit tests.

Guts · 2021-06-22T07:02:27Z

Sure!

I did not write them because I can't find which tests framework is recommended.

Can you tell me which one please? (pypi package name)

jelmer · 2021-06-22T09:27:38Z

We're using nosetests; you can invoke them with nose or by running "make test"

Guts · 2021-06-22T10:06:47Z

We're using nosetests; you can invoke them with nose or by running "make test"

Ok. What I need is the PyPi package to install it as testing dependency in a virtual env (I don't want to install something widely on my system). I can't find noestests on pypi.org but I've found 2 nose* packages: this one https://pypi.org/project/nose/ or this one https://pypi.org/project/nose2/?

jelmer · 2021-06-22T18:20:21Z

I think the pypi name is just "nose", but you should also be able to just use python's standard unittest framework to run the tests - we don't use anything nose-specific.

Guts · 2021-06-22T19:35:05Z

I think the pypi name is just "nose", but you should also be able to just use python's standard unittest framework to run the tests - we don't use anything nose-specific.

Good news because running make test returns an error (Ubuntu 20.04, after make init with node 14):

:~/Git/External/isso$ make test
python3 setup.py nosetests
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: invalid command 'nosetests'
make: *** [Makefile:71 : test] Erreur 1

Thanks for your help. I'll try to write some unit tests with the standard lib.

vincentbernat · 2021-06-22T21:17:47Z

nose is only a runner. You can also run them with python3 -m unittest discover isso/tests or by using pytest which also includes a runner.

…hook

mcnaveen · 2021-10-09T13:43:08Z

I & @mskian ended up creating this - https://github.com/mskian/isso-telegram-notifier

Guts · 2021-10-09T14:49:53Z

I & @mskian ended up creating this - https://github.com/mskian/isso-telegram-notifier

Nice work! Why not develop it right into Isso?

jelmer · 2021-10-09T20:37:31Z

I & @mskian ended up creating this - https://github.com/mskian/isso-telegram-notifier

Nice work! Why not develop it right into Isso?

If this was added to mainline isso, it would be hard to make sure that it kept working - the maintainers can't test with lots of different configurations.

ix5 · 2021-12-21T22:37:28Z

Nice work! I have not verified the functionality myself, but the code looks neat.

Please add (unit) tests as requested by @jelmer

isso/ext/notifications.py

share/isso.conf

ix5 · 2021-12-21T22:19:13Z

share/isso.conf

+# webhook URL
+url = 
+
+# path to the JSON template. Optional.


Can users supply an absolute or relative path here? Please state so clearly.

isso/ext/notifications.py

jelmer · 2022-01-04T19:30:44Z

isso/tests/test_vote.py

    def setUp(self):
        self.path = tempfile.NamedTemporaryFile().name

    def makeClient(self, ip):

-        conf = config.load(pkg_resources.resource_filename('isso', 'defaults.ini'))


Please don't include these unrelated formatting changes - they make the PR harder to review.

ix5 · 2022-02-10T20:57:27Z

@Guts this is a great feature, would you mind rebasing and addressing the points raised by jelmer and myself?

abhin4v · 2023-02-25T14:50:17Z

It'll be great to have this feature. It'll enable me to trigger a build of my static website in which I render the Isso comments at build time.

Guts added 17 commits April 29, 2021 23:33

Add webhook

8ad2796

Exclude .venv from flake8

6317ad7

Fix string formatting

c3ae475

Fix string formatting

1a8f146

Fix flake8

107860c

Make it compatible with Python 3.5 ?!

c791ad4

Add myself

2dd097a

Add default webhook section

e60e83b

Clean up and docstrings

e3d0641

Handle HTTP errors

4d36604

Add webhook template for Slack

78a0fa4

Align tox dependencies with package

ad64322

Add requests as dependency

f122ac6

Remove unused var

4818192

Complete post data without template

8f07874

Enable cache

386ec00

Use json loads/dumps to prevent encoding aleas

92beeb8

Guts force-pushed the notify/webhook branch from 92beeb8 to c6c46c1 Compare June 19, 2021 11:28

Guts added 3 commits June 19, 2021 13:30

Add myself

24f3a2f

Fix flake8, tox and package config

4acc0e2

Exclude .venv from flake8

Guts force-pushed the notify/webhook branch from c6c46c1 to f38c1c0 Compare June 19, 2021 11:30

Merge branch 'notify/webhook' of github.com:Guts/isso into notify/web…

c379776

…hook

Guts marked this pull request as draft June 23, 2021 05:35

ix5 requested changes Dec 21, 2021

View reviewed changes

This was referenced Dec 22, 2021

Please add more maintainers #752

Closed

[Feature Request] integration with telegram instead of emails. #574

Closed

ix5 self-assigned this Dec 27, 2021

ix5 added feature server (Python) server code labels Dec 31, 2021

jelmer reviewed Jan 4, 2022

View reviewed changes

ix5 mentioned this pull request Feb 10, 2022

XMPP notifications #31

Closed

ix5 added this to the 0.13 milestone Feb 13, 2022

ix5 modified the milestones: 0.13, 0.14 May 24, 2022

posativ force-pushed the master branch from 13f7a93 to 2518982 Compare January 3, 2023 15:24

ix5 modified the milestones: 0.14, 0.13.1 May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add webhook notification capability #724

Feature: add webhook notification capability #724

Guts commented May 1, 2021 •

edited

Loading

Guts commented Jun 17, 2021

ix5 commented Jun 17, 2021

Guts commented Jun 17, 2021 •

edited

Loading

jelmer commented Jun 21, 2021

Guts commented Jun 22, 2021

jelmer commented Jun 22, 2021

Guts commented Jun 22, 2021

jelmer commented Jun 22, 2021

Guts commented Jun 22, 2021

vincentbernat commented Jun 22, 2021

mcnaveen commented Oct 9, 2021

Guts commented Oct 9, 2021

jelmer commented Oct 9, 2021

ix5 commented Dec 21, 2021

ix5 Dec 21, 2021

jelmer Jan 4, 2022

ix5 commented Feb 10, 2022

abhin4v commented Feb 25, 2023

Feature: add webhook notification capability #724

Are you sure you want to change the base?

Feature: add webhook notification capability #724

Conversation

Guts commented May 1, 2021 • edited Loading

Notable changes

Questions

Misc

Guts commented Jun 17, 2021

ix5 commented Jun 17, 2021

Guts commented Jun 17, 2021 • edited Loading

jelmer commented Jun 21, 2021

Guts commented Jun 22, 2021

jelmer commented Jun 22, 2021

Guts commented Jun 22, 2021

jelmer commented Jun 22, 2021

Guts commented Jun 22, 2021

vincentbernat commented Jun 22, 2021

mcnaveen commented Oct 9, 2021

Guts commented Oct 9, 2021

jelmer commented Oct 9, 2021

ix5 commented Dec 21, 2021

ix5 Dec 21, 2021

Choose a reason for hiding this comment

jelmer Jan 4, 2022

Choose a reason for hiding this comment

ix5 commented Feb 10, 2022

abhin4v commented Feb 25, 2023

Guts commented May 1, 2021 •

edited

Loading

Guts commented Jun 17, 2021 •

edited

Loading