-
-
Notifications
You must be signed in to change notification settings - Fork 25
[16.0][ADD] webhook_outgoing: send outgoing webhooks #13
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: 16.0
Are you sure you want to change the base?
Conversation
28220c0 to
208010b
Compare
|
Great! Using queue job also seems interesting. I didn't dig deeply into the review yet though, but I might use it and help. For reference here is the webhook code In Odoo 17.0+ CE (so not available on Odoo 16.0) (usage can be seen here https://medium.com/@karan.bk/simplify-integration-with-odoo-17-using-webhooks-4ec56c46fcb3) EDIT: to achieve some kind of compatibility, from a quick overview:
cc @pedrobaeza @sebastienbeau @hparfr @sbidoul @lmignon @simahawk @etobella |
|
there is also possibly some overlap with https://github.com/OCA/web-api/blob/14.0/webservice/models/webservice_backend.py from the https://github.com/OCA/web-api/tree/14.0 OCA project. In any case, quick and dirty web hooks with no code is certainly something important to have for simple and quick automation. |
|
it can also be installed as an option, but eventually this server_action_logging module might help. |
|
BTW, @gurneyalex pointed that the security of the native Odoo v17 web hooks is probably not so good: https://x.com/gurneyalex/status/1817900552329560150 ... So good if this extra module could bring some real security... Is it safe now https://github.com/OCA/webhook/pull/13/files#diff-ca6b9e897fddbb1f998aefd239a9630cef55fbdcd6de7a2b20043e2faff24b29R28 ? Eventually, we might use base_rest_auth_api_key https://github.com/OCA/rest-framework/tree/16.0/base_rest_auth_api_key may be ? |
|
@hoangtrann @rvalyi looks like `codecov/project' action is still running. It will consume tons of minutes |
|
also @sbidoul please have a look at the codecove job here |
208010b to
5d832a1
Compare
|
@ivs-cetmix force push to trigger actions seem to solve the issue |
ivs-cetmix
left a comment
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.
Thank you once again for your contribution. Some thoughts here:
- Probably the first module should be named something like
webhook_outgoingfor better matching the module designation. Same for the module name and description - Maybe it would be better to have 2 different PR's per module. You can add dependencies in the second PR using
test-requirements.txt. This will make reviews and merging modules faster - Test coverage is something that is a must so tests should be added in order to proceed further
- Same with documentation. However this is something we can also assist with
5d832a1 to
a03cb3d
Compare
|
thanks @ivs-cetmix @rvalyi for your feedback, I've updated the PR to only include the I've made changes accordingly as well:
I'm going to update the tests as well as creating another PR for incoming webhook |
a03cb3d to
59c9c13
Compare
|
@hoangtrann lets get it merged ! it's really great. |
lmignon
left a comment
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.
Thank you for this addons. Can you add tests plz? Unittest are required to ensure code quality and maintainability.
801e35e to
086853a
Compare
|
@lmignon I've added tests, sorry that it takes a while though |
|
ping @ivs-cetmix @rvalyi as well for review |
|
@OCA/tools-maintainers appreciate some reviews here |
|
EDIT: hum, this PR is now only for the outgoing webhooks, so there is no such overlap issue here as I initially mentioned below here... @simahawk @etobella @LoisRForgeFlow @lmignon @pedrobaeza @ivs-cetmix how do you guys see these modules here fit together with https://github.com/OCA/web-api (specially the endpoint and webservice API), seems there is some overlap for a webhook server. This proposal here is however closer to the Odoo native webhook thing that comes at v17+. Ideally we don't duplicate efforts. But I didn't investigate too much if the webhook server here would still make sense or if convergence/compatibility is somewhat possible... |
I would partially agree with you, as the |
|
@ivs-cetmix I'm sorry I just found out that the scope of this PR changed and as for now only the webhook_outgoing module is proposed for review. So my previous comment was not pertinent anymore (I added an EDIT line). |
|
Yup I've separated this PR and made another one for incoming webhooks for ease of review. The major features this bring are render payload using Jinja template and execute using queue job. This allows user to quickly build an integration to send custom Slack notification for example. |
086853a to
b52ded6
Compare
|
@lmignon @ivs-cetmix I've addressed the changes. Would love to get this merge and I will forward the changes to newer versions. @rrebollo force push seems to fix the runbot, you can now test this on the runbot rather than the 17.0 version since it's outdated. |
|
@hoangtrann the PR name should be something along "[16.0][ADD] webhook_outgoing: ...." |
ivs-cetmix
left a comment
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.
So far looks good, thank you!
Important notice: please structure the module documentation according to the OCA documentation layout: https://github.com/OCA/maintainer-tools/tree/master/template/module/readme
I would recommend to use .md format for docs as it's better from both readability and maintenance perspective.
| :return str headers: headers object in string format | ||
| """ | ||
| self.ensure_one() | ||
| headers = dict(json.loads(self.headers.strip())) if self.headers else {} |
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.
Non an issue but a question: json.loads() returns a dict, is there an actual need for another dict() operator?
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 let me make an update for this
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've updated the doc and make change to eval the headers. Since headers is just text, save_eval is used to handle both case if headers is a python dict or it's a json
| """ | ||
| self.ensure_one() | ||
| headers = dict(json.loads(self.headers.strip())) if self.headers else {} | ||
| return str(headers) |
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.
actually, you are doing from text to dict (json.loads), then dict to str (str) and again str to dict with safe_eval, maybe you could simplify that....
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 I must be handling something at that point which I now totally forgot, let me simplify it
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've made the update, since headers is just text, save_eval is used to handle both case if headers is a python dict or it's a json.
b52ded6 to
fa92a21
Compare
No description provided.