feat: remove audited methods from api client and add them directly to… #182
Conversation
There was a problem hiding this comment.
LGTM. It takes advantage that pipe step doesn't receive the credentials in kwargs.
"soapenv:Envelope": {
"soapenv:Header": {
"wsse:Security": {
"wsse:UsernameToken"
...
}I thought that we would refactor it into a step pipe like set_pearson_credentials.
But in this case we have to adapt or adjust to the eox_audit model, that save the kwargs.
|
|
||
| # There are multiple kind of errors, some of them generates a soapenv:Fault section and others | ||
| # generates an status error section, the following condition handles the first case. | ||
| if fault: | ||
| return { |
There was a problem hiding this comment.
Maybe here we could add an exception. To try to add the eox_audit with the failure status instead of success.
I was thinking of the audit error raising a pearsonExceptionm but it would be twice I think so... or may be not.
We have to view it after the PearsonException PR works...
There was a problem hiding this comment.
@johanseto please check the last commit, I moved the exception inside the auditable method and now the error output looks like this
|
|
||
| return { | ||
| "status": "unexpected error", | ||
| "response": xml_response, |
There was a problem hiding this comment.
I like that any error would be returned without changes.
There was a problem hiding this comment.
This is the current implementation :)
| # pylint: disable=broad-exception-raised | ||
| raise Exception("Error trying to process import exam authorization request.") | ||
| raise Exception( |
There was a problem hiding this comment.
I like that this Exception gives the Fail status, but what do you think of the retry of the error like 4 times because it is en celery async environment.
I think that it would retry 4 times, saving the 4 Fail record. The good news is that now it is easier to distinguish than the success records.
There was a problem hiding this comment.
Short answer out of the scope, long answer I was thinking to implement a PearsonBaseError here and limit the retrying process just to the ping pipeline since other case will need that a user o someone changes something so it's not necessary multiple attempts
There was a problem hiding this comment.
I really like that comment.
|
Approved |
andrey-canon
force-pushed
the
and/remove_sensitive_data
branch
from
48c61c3
to
d539bc0
Compare
Description
This removes the auditable wrappers from the api_client file and move them to the corresponding pipe, this preserves the current output format however the input format has changed, this uses a dict format instead of a SOAP string.
Before
CDD
EAD
After
CDD
EAD
Additional information
Include anything else that will help reviewers and consumers understand the change.
Checklist for Merge