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

[UPD] account_payment_adyen_refund: compute payment reference #1

Open
wants to merge 3 commits into
base: 13.0-adyen-refund-payment
Choose a base branch
from

Conversation

ntsirintanis
Copy link

@NL66278 please take a look

@ntsirintanis ntsirintanis force-pushed the 13.0-adyen-refund-payment_compute_reference branch from 067f6ba to 525a1ac Compare December 4, 2023 11:37
@@ -70,3 +71,12 @@ def _get_acquirer(self):
_("No payment acquirer linked to journal %s") % journal.name
)
return acquirer

def _compute_reference(self, reference, suffix="", iteration=""):
Copy link

@NL66278 NL66278 Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simpler:

 def _compute_reference(self): 
        """Adjust reference to avoid uniqueness constraint"""
        existing_count = self.env["payment.transaction"].search_count([("reference", "=", reference)])
        if not existing_count:
            return reference
        return reference + "-" + str(existing_count)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here existing_count is doomed to be 1 or 0, forever, since the reference constraint exists. So if we have a reference A, next one by this method will be A-1. If we try to create another reference from A, it will still return A-1, triggering the constraint. So I restored this to its previous version

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not quite see this. The first 0 existing, so reference will be A. Next one will find 1 existing, so reference will be A1, one after that will find 2 existing, so reference will be A2 etc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha I see the problem, the '=' in the search should be changed into a like as well...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be exact: [("reference", "=like", reference + "%")]) to only search on reference that have the same starting characters.

@ntsirintanis ntsirintanis force-pushed the 13.0-adyen-refund-payment_compute_reference branch from 525a1ac to 3e5f221 Compare December 4, 2023 14:31
@@ -58,6 +58,8 @@ def _send_adyen_refund_request(self):
}
refund_tx = transaction_model.create(values)
self.write({"payment_transaction_id": refund_tx.id})
invoice.write({"refund_status": "send"})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NL66278 we tested this too; if sending to adyen fails, the refund_status remains on send

@ntsirintanis ntsirintanis force-pushed the 13.0-adyen-refund-payment_compute_reference branch from 4ac3636 to 1b1d50f Compare December 4, 2023 17:00
@ntsirintanis ntsirintanis force-pushed the 13.0-adyen-refund-payment_compute_reference branch from 1b1d50f to b6c4d97 Compare December 4, 2023 17:08
@@ -57,6 +58,8 @@ def _send_adyen_refund_request(self):
}
refund_tx = transaction_model.create(values)
self.write({"payment_transaction_id": refund_tx.id})
invoice.write({"refund_status": "send"})
self.env.cr.commit()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some doubts about this. Not on the need for a commit, but maybe it should be in a separate cursor, dedicated to updating the invoice.refund_status. In this way, all other changes will still be rolled back on failure. The update of the refund status could be part of a separate method (in the account.invoice model) like so:

from odoo import registry (add to existing imports from Odoo)
...
def _update_refund_status(self, status):
        self.ensure_one()
        self.flush()  # Not needed from 15.0 upwards
        with registry(self.env.cr.dbname).cursor() as cr:
            self.with_env(self.env(cr)).write({"refund_status":"refund_status})

See also this discussion: odoo/odoo#82928

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what you are proposing here:

  • A payment will be created
  • The invoice status will change to send
  • The payment cannot be sent to adyen, because of some error during the sending process
  • everything rolls back (including the payment creation) except the invoice status

Is this correct?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntsirintanis Indeed I think it should work like that. Note that you might still need to disable the pylint check for not having commits (and check the other stuff in the pre-commit log, but those are minor things). Just to be sure check also with @lfreeke . When processing in batch (server action or cron in customer code) it should still be possible to commit each successfully completed transaction (and rollback the unsuccesfull ones) depending or not you want the cron to proceed with other transactions when one fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants