OFBiz
  1. OFBiz
  2. OFBIZ-896

Important issue with rollbacks on payment processing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: accounting, order
    • Labels:
      None

      Description

      From a recent thread on the dev list:
      ===========================================
      I really think I'll need (at least) some help to fix this; here are some more details.

      I don't think that we can fix this by requiring a new transaction.
      Let's focus for example the capturing process (but the same issue is present in the auth/release/etc... transactions):

      a) the main method involved is PaymentGatewayServices.capturePayment(...)
      b) this method internally pulls the service name to use to process the payment from the store settings and it runs the service requiring a new transaction (see line 1413)
      c) the above method, "capturePayment", is called by other methods of the PaymentGatewayServices class, for example by the captureOrderPayments service/method (see line 1147)

      The issue is that the captureOrderPayments method, after that the call to the capturePayment method is (succesfully) done (i.e. the funds have been captured) performs other logics, and for example a few lines below it calls another method that could fail and if it happens it returns an error causing a rollback (line 1166).
      Of course the rollback will not rollback the remote transaction.
      The problem is more complex than this, since, even if the captureOrderPayments service will return a success, there are other services triggered by it that could fail; this would be partially addressed requiring a new transaction for the captureOrderPayments service though.
      Ideally, but I know it is not possible, nothing should happen (that could fail) after the transaction has been issued

      How can we fix this?

      Jacopo

      David E. Jones wrote:
      >
      > Jacopo,
      >
      > Yeah, that's a bit of a scary bug. What should probably happen is that the code that stores information related to the credit card transaction should be separated from everything else. If it is in it's own service already then that service can just have the require-new-transaction attribute set to true. If not, then it should be split out into its own service with that attribute set so that it runs independently (in terms of success/failure) of the other stuff going on.
      >
      > This is a pretty big issue... Is something you're looking at working on? If you're short on time and could write up whatever details you've found we can probably get one of our guys on it.
      >
      > -David
      >
      >
      > On Apr 13, 2007, at 10:55 AM, Jacopo Cappellato wrote:
      >
      >> I think there is an important issue with online payment processors in OFBiz.
      >> I'm not going into the details, because I think it is better to discuss this issue in general, but this is something that really happened at least with one of my customers.
      >>
      >> When an order payment is processed (authorized/captured), in OFbiz a chain of events/service calls (ecas etc...) is triggered.
      >> Everything is wrapped inside one transaction so, if something goes wrong, the rollback brings everything back to the original situation.
      >> Unfortunately, if the error happens after the online transaction has been issued, OFbiz will not know that the payment has been authorized and/or captured.
      >>
      >> What is in your opinion the best way to handle this scenario?
      >>
      >> Jacopo
      >>
      >

        Issue Links

          Activity

          Hide
          Jacopo Cappellato added a comment -

          A workaround for this issue could be the following (I'm using the example of the capturePayment service):

          1) require a new transaction for the service "capturePayment" (PaymentGatewayServices.capturePayment(...))
          2) move (if possible) the code that performs the call to the remote service at the bottom of the method (ideally nothing should happen after it in the method)
          3) no ecas should be attached to the capturePayment service, if they share the same transaction and are run after it

          what do you think?

          Show
          Jacopo Cappellato added a comment - A workaround for this issue could be the following (I'm using the example of the capturePayment service): 1) require a new transaction for the service "capturePayment" (PaymentGatewayServices.capturePayment(...)) 2) move (if possible) the code that performs the call to the remote service at the bottom of the method (ideally nothing should happen after it in the method) 3) no ecas should be attached to the capturePayment service, if they share the same transaction and are run after it what do you think?
          Hide
          Andrew Zeneski added a comment -

          I am in the process of pulling out all the code which stores the payment processing information and putting them into their own service which will require new transaction. I agree with David this is the best way to go.

          This was already done for authorizations and somewhat for captures. However, there was still a lot of updating done in the capture method (now being moved to processCaptureSplitPayment; which is related to partial captures).

          Releases and refunds have been moved over; I'm in the processing of testing this code and will commit it when it is finished.

          Show
          Andrew Zeneski added a comment - I am in the process of pulling out all the code which stores the payment processing information and putting them into their own service which will require new transaction. I agree with David this is the best way to go. This was already done for authorizations and somewhat for captures. However, there was still a lot of updating done in the capture method (now being moved to processCaptureSplitPayment; which is related to partial captures). Releases and refunds have been moved over; I'm in the processing of testing this code and will commit it when it is finished.
          Hide
          Jacopo Cappellato added a comment -

          Andrew,

          is this issue resolved now?

          Show
          Jacopo Cappellato added a comment - Andrew, is this issue resolved now?

            People

            • Assignee:
              Andrew Zeneski
              Reporter:
              Jacopo Cappellato
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development