-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
[ADD] base_json_request: Module to allow standard JSON requests #3
base: 10.0
Are you sure you want to change the base?
Conversation
* Create a module that circumvents the JSON-RPC requirement for all requests with application/json
'name': 'Base JSON Request', | ||
'summary': 'Allows you to receive JSON requests that are not RPC.', | ||
'version': '10.0.1.0.0', | ||
'category': 'Authentication', |
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.
Not
|
||
This module allows you to receive JSON requests in Odoo that are not | ||
RPC. | ||
|
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.
Add note that route
needs to be defined with type=json
@lasley after |
Thank you for the insight @lmignon
I think I agree with you, and I have no problem making this a lib instead. Thanks for the examples, they will keep me from redesigning wheels. We do have a dissenting opinion on the using of external libs versus Odoo modules in OCA/server-tools#944 (comment) - the arguments I believe could be applied here as well. The overall strategy of using libraries is being questioned, so I'm going to bring this one over here & apply whatever we decide there to here too. |
Oops forgot this point. I could look into this, yes. odoo/odoo#7766 (comment) seemed to allude to Odoo SA being willing to accept a patch, but I need this in v10 unfortunately. I'll likely submit something though, yeah. |
We've observed that it's easier to propose enhancement on the core Odoo with PR on master. By doing this we've a chance to get this functionality supported into the next version of Odoo. In the meantime, the only solution to support this functionality is via an external lib. |
This logic is taken from my REST API work in OCA/server-tools#889. It has been working well on multiple productions for a few months now. Note that none of these productions have multiple databases, which is an important detail in this instance (because this is a monkey patch).
That said, I do not intend to provide compatibility for multi-tenant here. I assert that the concept of a webhook and the concept of a multi-tenant Odoo instance are at odds with each other in most cases.
While multi-tenant is still a possibility while just using this module, the next PR I submit (
base_web_hook
- #4) will have an explicit note that this is not possible. IMO it is worth having the ability for a web hook, as long as the possible sacrifice is known. This may be possible to circumvent with database filters, but I haven't tested.cc @moylop260 (I think you guys were looking for something like this for Gitlab webhooks)
Note: I still need to add tests, but the functionality is here to review.