cancel
Showing results for 
Search instead for 
Did you mean: 

Sales Order Payment is complicated

0 Kudos

Sales Order Payment is complicated

Feature request from ihor-sviziev, posted on GitHub Nov 14, 2014

For now this class is very complicated and hard to understand their logic https://github.com/magento/magento2/blob/master/app/code/Magento/Sales/Model/Order/Payment.php#L1036

8 Comments
apiuser
New Member
Status changed to: Investigating
 
apiuser
New Member

Comment from ooxi, posted on GitHub Nov 18, 2014

I like how trivalent logic is implemented using true, false and -1. However it would be even better if we could replace -1 with file not found Smiley Wink

apiuser
New Member

Comment from mcspronko, posted on GitHub Nov 18, 2014

@ihor-sviziev, thank you for your note. We have plan to reducing complexity in the system. This issue will be added to our backlog. We will update you when it will be ready. Also, you may want to provide proposal on how you see this class might be refactored.

Thanks.

apiuser
New Member

Comment from vpelipenko, posted on GitHub Jan 27, 2015

Internal ticket: MAGETWO-31585

apiuser
New Member

Comment from ihor-sviziev, posted on GitHub May 30, 2015

Now this class logic looks much better, but now it still contains ~2600 lines of code, I think we may separate class functionality to two or more classes and use delegation. I think first of all we should move methods like place, capture, registerCaptureNotification, refund, registerRefundNotification, accept, deny, update, authorize.

apiuser
New Member

Comment from vpelipenko, posted on GitHub Jun 03, 2015

@ihor-sviziev, when we have time we try to decrease code complexity and re-implement complicated classes in sales/payment infrastructure, but this work doesn't have high priority, that's why we can't promise that suggested refactoring will be done in the near future.

apiuser
New Member

Comment from vpelipenko, posted on GitHub Aug 11, 2015

@ihor-sviziev, we've done some work around complicated Sales Order Payment. If you compare this class in develop and master branch, you will see that about 400 lines of code were removed from it. Please, look at our changes. All feedback from your side is welcome.

apiuser
New Member

Comment from ihor-sviziev, posted on GitHub Aug 18, 2015

@vpelipenko really good progress, now it looks better, but not enough. What I think we should improve:

  1. There is a lot of methods have public methods (not public only), that will receive object, but object type is not declared in method. As result we may got not expected method and fatal error if retrieved null. Example methods: pay(), cancelInvoice(), refund(), cancelCreditmemo(), updateBaseAmountPaidOnlineTotal(), _updateTotals(), prepareCreditMemo(),
  2. In few methods we have comaration "==" instead of "===", as result we may have some unexpected behavior. Examples: updateOrder(), _void(), isCaptureFinal(), isSameCurrency(), _getInvoiceForTransactionId(), setOrderStatePaymentReview(), registerRefundNotification(),
  3. This class still contains a lot of getters, setters and business logic (as example place(), pay(), cancelInvoice() ). Would be great move this business logic to some another class(es) and just execute methods (as it done for capture() method).