[14.0] webservice: add webserviceConsumerMixin #22
Conversation
| def _request( | ||
| self, method, url=None, url_params=None, consumer_record=None, **kwargs | ||
| ): | ||
| if self.collection.save_response and isinstance( |
There was a problem hiding this comment.
You should wrap the whole existing code of this method w/ ctx manager to:
- check this only once
- store the reponse content even when
request.raise_for_statusat the end will brake the transaction
Also, why do you need this consumer_record?
The adapter should be agnostic and know nothing about such records.
I would wrap call instead and make it accept this param.
You can then define a method on the backend _must_save_response which checks the save_response flag and the consumer record if given.
There was a problem hiding this comment.
Thanks for your review and feedback. I've refactor the code with this commit: 8862170 does it looks better ?
There was a problem hiding this comment.
hum... in fact while using with #oca/edi/787 I'm getting something weird as the main transction try to write on the same record as the new transcation to save the result, in the test I've done only the result paylaod was saved but the state of the edi.exchange.record was output_pending (I expected output_sent) 🤔
There was a problem hiding this comment.
logs confirmed that hypotesis
ERROR ** odoo.sql_db: bad query: UPDATE "edi_exchange_record" SET "edi_exchange_state"='output_sent',"exchanged_on"='2023-09-05 12:59:11',"write_uid"=62,"write_date"=(now() at time zone 'UTC') WH│
│ERROR: could not serialize access due to concurrent update
Not sure what's the best to do 🤔
There was a problem hiding this comment.
I'm going to test this new version on real environement
| def _compute_ws_response_content_filename(self): | ||
| for rec in self: | ||
| rec.ws_response_content_filename = ( | ||
| "response_%s.json" % (str(rec.ws_response_status_code),) |
There was a problem hiding this comment.
I would add the formatted datetime string too
There was a problem hiding this comment.
at the moment this is a computed field not stored 🤔 I could add a response_at datetime field 🤔
There was a problem hiding this comment.
An alternative would be to relate the ir.attachment and read the write date from there...
No strong opinion on this but I think it would be nice to know when the response was recorded.
| def _compute_ws_response_content_filename(self): | ||
| for rec in self: | ||
| rec.ws_response_content_filename = ( | ||
| "response_%s.json" % (str(rec.ws_response_status_code),) |
There was a problem hiding this comment.
An alternative would be to relate the ir.attachment and read the write date from there...
No strong opinion on this but I think it would be nice to know when the response was recorded.
| def call(self, method, *args, **kwargs): | ||
| return getattr(self._get_adapter(), method)(*args, **kwargs) | ||
| @contextmanager | ||
| def _consumer_record_env(self, record, new_cursor=True): |
There was a problem hiding this comment.
I'm not sure to understand why we need new_cursor flag. See below.
petrus-v
force-pushed
the
14.0-webservice-http_error_handler
branch
from
b5f8215
to
8a29f55
Compare
|
I've squashed commits and rebase the PR, also I've move the way to change the state on the consumer record adding method on the abstract method to make it easier the way to overwrite it as discussed OCA/edi#787 (comment) |
Models inhering from this abstract model will get the webservice response saved on it.
petrus-v
force-pushed
the
14.0-webservice-http_error_handler
branch
from
8a29f55
to
d8350b8
Compare
| formatted_response_date = slugify( | ||
| rec.ws_response_date.strftime(DATETIME_FORMAT) | ||
| ) | ||
| rec.ws_response_content_filename = "response_%s_%s.json" % ( |
There was a problem hiding this comment.
Not sure assuming this is always json is ok. It can be pure txt or even html (unfortunately).
I'd leave it w/o ext for now. Not a blocker tho.
| for rec in self: | ||
| if rec.ws_response_date and rec.ws_response_status_code: | ||
| formatted_response_date = slugify( | ||
| rec.ws_response_date.strftime(DATETIME_FORMAT) |
There was a problem hiding this comment.
| rec.ws_response_date.strftime(DATETIME_FORMAT) | |
| fields.Datetime.to_string(rec.ws_response_date) |
| content = request_error.response.content | ||
| status_code = request_error.response.status_code | ||
| raise request_error from request_error |
There was a problem hiding this comment.
| content = request_error.response.content | |
| status_code = request_error.response.status_code | |
| raise request_error from request_error | |
| content = request_error.response.content | |
| status_code = request_error.response.status_code | |
| raise |
This is part of what has been discussed here: OCA/edi#776 (review)
In draft at the moment, I'd like to