Skip to content
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

[FIX] payment: error logs + [FIX] payment_stripe_sca: less aggressive raise #39370

Closed

Conversation

@jev-odoo
Copy link
Contributor

jev-odoo commented Oct 25, 2019

Before this commit, trying to put a transaction in a state
where it was already in was causing an error.

For example: Putting a transaction in'Done' when it was
already in'Done' caused an error
'Only draft/authorized transaction can be posted.'

Now, we just log a note that the transaction is
already in the required state and pass over.

return False
return True

tx_to_process = self.filtered(filter_trans)

This comment has been minimized.

Copy link
@dbo-odoo

dbo-odoo Oct 28, 2019

Contributor

I really dislike that your filtering method logs stuff... for me, it should only handle the recordset and the logging should be done in the flow

tx_to_process = self.filtered(lambda tx: tx.state == 'draft')
tx_already_pending = self.filtered(lambda tx: tx.state == 'pending')
tx_bad_state = self - tx_to_process - tx_already_pending
for pending_tx in tx_already_pending:
  _logger.info('blablabla')
for bad_tx in tx_bad_state:
  _logger.warning('machinmachinmachin')
# then go one with the tx_to_process recordset

This comment has been minimized.

Copy link
@dbo-odoo

dbo-odoo Oct 28, 2019

Contributor

same comment applies for all methods that use this

'date': datetime.now().strftime(DEFAULT_SERVER_DATETIME_FORMAT),
'state_message': '',
})
tx_to_process._log_payment_transaction_received()

This comment has been minimized.

Copy link
@dbo-odoo

dbo-odoo Oct 28, 2019

Contributor

just checked to be sure: this is indeed a multi method (note to self)

@dbo-odoo dbo-odoo force-pushed the odoo-dev:12.0-better_payment_error_handling-jev branch from f43b4a6 to f3087c0 Dec 2, 2019
@robodoo robodoo removed the CI 🤖 label Dec 2, 2019
@dbo-odoo dbo-odoo force-pushed the odoo-dev:12.0-better_payment_error_handling-jev branch from f3087c0 to 3c976db Dec 2, 2019
@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 2, 2019

@jev-odoo i've hijacked this PR for a fix in payment_stripe_sca since I needed it to work properly. Will merge soon
hijack in progress

@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 2, 2019

note to self: cfr https://stripe.com/docs/error-codes

the smart thing would be to raise_for_status if error code is not present

Edit: I have now done the smart thing

jev-odoo and others added 2 commits Oct 25, 2019
Before this commit, trying to put a transaction in a state
where it was already in was causing an error.

For example: Putting a transaction in'Done' when it was
already in'Done' caused an error
'Only draft/authorized transaction can be posted.'

Now, we just log a note that the transaction is
already in the good state and pass over.
In some cases, Stripe will return the result of a transaction with
a faulty HTTP Status (e.g. 4XX statuses) - not because the request
was malformed, but because the payment failed. It is a bit unfortunate
that Stripe would not differentiate between payment status and request
validity, but that's the state of things.

This commit ensures that such a response will be logged correctly, by
making the call to `raise_for_status` conditionnal on the response object's
status code and internal structure, forwarding the response's json to the
validation flow if the status is in the 4XX range and a `code` key is
found in the response, as described in https://stripe.com/docs/error-codes

While testing this fix, it became apparent that some error message
processing in the frontend was not correctly handled as well - instead
of purely rejecting the promise, a failed payment should still send its
payload to the backend, to allow the server to put the transaction in
the correct state according to the response.
@dbo-odoo dbo-odoo force-pushed the odoo-dev:12.0-better_payment_error_handling-jev branch from 3c976db to 64551ac Dec 2, 2019
@robodoo robodoo added the CI 🤖 label Dec 2, 2019
@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 3, 2019

@jev-odoo I would appreciate your opinion when you come back on wednesday :)
i'll wait for you before merging

@dbo-odoo dbo-odoo changed the title [FIX] payment: No error logged if no real error in transaction [FIX] payment: error logs + [FIX] payment_stripe_sca: less aggressive raise Dec 3, 2019
@jev-odoo

This comment has been minimized.

Copy link
Contributor Author

jev-odoo commented Dec 4, 2019

@dbo-odoo
Seems good to me.
Sorry to not have done this earlier, I had a serious lack of time ...

Thanks for this ;)

@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 4, 2019

@robodoo robodoo added the r+ 👌 label Dec 4, 2019
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Dec 4, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward
@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 4, 2019

@robodoo rebase-ff

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Dec 4, 2019

Merge method set to rebase and fast-forward

robodoo pushed a commit that referenced this pull request Dec 4, 2019
In some cases, Stripe will return the result of a transaction with
a faulty HTTP Status (e.g. 4XX statuses) - not because the request
was malformed, but because the payment failed. It is a bit unfortunate
that Stripe would not differentiate between payment status and request
validity, but that's the state of things.

This commit ensures that such a response will be logged correctly, by
making the call to `raise_for_status` conditionnal on the response object's
status code and internal structure, forwarding the response's json to the
validation flow if the status is in the 4XX range and a `code` key is
found in the response, as described in https://stripe.com/docs/error-codes

While testing this fix, it became apparent that some error message
processing in the frontend was not correctly handled as well - instead
of purely rejecting the promise, a failed payment should still send its
payload to the backend, to allow the server to put the transaction in
the correct state according to the response.

closes #39370

Signed-off-by: Damien Bouvy (dbo) <dbo@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Dec 4, 2019

Staging failed: ci/runbot (view more at http://runbot.odoo.com/runbot/build/718657)

@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 4, 2019

@robodoo retry

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Dec 4, 2019

Staging failed: ci/runbot on 01a5f0d9627d970bda7218cbadb8a73148e194df (view more at http://runbot.odoo.com/runbot/build/719196)

@dbo-odoo

This comment has been minimized.

Copy link
Contributor

dbo-odoo commented Dec 4, 2019

@robodoo retry

robodoo pushed a commit that referenced this pull request Dec 4, 2019
In some cases, Stripe will return the result of a transaction with
a faulty HTTP Status (e.g. 4XX statuses) - not because the request
was malformed, but because the payment failed. It is a bit unfortunate
that Stripe would not differentiate between payment status and request
validity, but that's the state of things.

This commit ensures that such a response will be logged correctly, by
making the call to `raise_for_status` conditionnal on the response object's
status code and internal structure, forwarding the response's json to the
validation flow if the status is in the 4XX range and a `code` key is
found in the response, as described in https://stripe.com/docs/error-codes

While testing this fix, it became apparent that some error message
processing in the frontend was not correctly handled as well - instead
of purely rejecting the promise, a failed payment should still send its
payload to the backend, to allow the server to put the transaction in
the correct state according to the response.

closes #39370

Signed-off-by: Damien Bouvy (dbo) <dbo@odoo.com>
@robodoo robodoo added merged 🎉 and removed merging 👷 labels Dec 4, 2019
@robodoo robodoo closed this Dec 4, 2019
@robodoo robodoo deployed to merge Dec 4, 2019 Active
@fw-bot

This comment has been minimized.

Copy link
Contributor

fw-bot commented Dec 8, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #41375

@fw-bot

This comment has been minimized.

Copy link
Contributor

fw-bot commented Dec 9, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #41389

2 similar comments
@fw-bot

This comment has been minimized.

Copy link
Contributor

fw-bot commented Dec 10, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #41389

@fw-bot

This comment has been minimized.

Copy link
Contributor

fw-bot commented Dec 11, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #41389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.