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

save invoice ID on credit memo when using API method salesRefundInvoiceV1 #11670

Merged
merged 8 commits into from
Nov 25, 2017

Conversation

AntonEvers
Copy link
Contributor

@AntonEvers AntonEvers commented Oct 24, 2017

Description

  • Sets the invoice ID when an invoice is set on the credit memo
  • Loads the invoice by id on getInvoice if an invoice ID is set on the credit memo

Fixed Issues (if relevant)

  1. API salesRefundInvoiceV1 does no save invoice ID on credit memo #11669: API salesRefundInvoiceV1 does no save invoice ID on credit memo

Manual testing scenarios

  1. Run testing scenario from API salesRefundInvoiceV1 does no save invoice ID on credit memo #11669

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

* @deprecated
*/
private function getInvoiceRepository() {
return $this->invoiceRepository ?: ObjectManager::getInstance()->get(InvoiceRepositoryInterface::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid such methods creation and move this logic to constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

Anton Evers added 5 commits October 24, 2017 12:21
In the IpnTest creates a creditmemo for the invoice in $ipnModel->processIpnRequest() already
So if Magento/Paypal/_files/order_express_with_invoice_and_creditmemo.php is used, the invoice is already refunded before the start of the test
@AntonEvers
Copy link
Contributor Author

@orlangur tests passing

@ishakhsuvarov
Copy link
Contributor

Fixes same issue as #11687

@ishakhsuvarov
Copy link
Contributor

@ajpevers I would suggest aligning solutions as #11687 seems to be more lightweight (however from code review only I can not see if it really fixes all cases), but this PR includes integration test, which is preferred.

@ishakhsuvarov ishakhsuvarov self-assigned this Oct 26, 2017
@AntonEvers
Copy link
Contributor Author

Hello @ishakhsuvarov,

Although the solution there is more lightweight, it is not in line with the rest of the code. The problem in #11669 brings to light that a credit memo - invoice relation is not saved automatically. In this case in the salesRefundInvoiceV1 API call. Other relations are always saved, such als the credit memo - order relation. No matter how or from which part of the code you save the credit memo, because it is done in the same _beforeSave as my addition. My solution is in line with the credit memo - order relation persistence code. It works in all instances. Not only in the API. Especially with these essential entities in the system I would not go for end of pipe solutions such as #11687.

As far as the unit testing goes. I would not know how to test the _beforeSave of a resource model. For now that goes beyond my knowledge of unit testing. As far as I can see the unit test for checking if the order ID is persisted on the credit memo is also lacking. So it would be a shame to keep this PR open for that reason in my opinion.

@magento-engcom-team magento-engcom-team added 2.2.x Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017
@ishakhsuvarov ishakhsuvarov added this to the November 2017 milestone Nov 9, 2017
@@ -379,6 +389,9 @@ public function canRefund()
*/
public function getInvoice()
{
if (!$this->getData('invoice') instanceof \Magento\Sales\Model\Order\Invoice && $this->getInvoiceId()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to use \Magento\Sales\Api\Data\InvoiceInterface here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. No there is no reason.

@magento-team magento-team merged commit f2ba426 into magento:2.2-develop Nov 25, 2017
magento-team pushed a commit that referenced this pull request Nov 25, 2017
magento-team pushed a commit that referenced this pull request Nov 25, 2017
[EngCom] Public Pull Requests - 2.2-develop
 - MAGETWO-83552: save invoice ID on credit memo when using API method salesRefundInvoiceV1 #11670
 - MAGETWO-82577: [Backport 2.2] Translate order getCreatedAtFormatted() to store locale #11422
 - MAGETWO-84474: 10128: New Orders not being saved to order grid #12241
 - MAGETWO-83783: Shipping method fixtures not compatible with getShippingMethod(true) in OrderCreateTest #12227
 - MAGETWO-83290: Add swatch option: Prevent loosing data and default value if data is not populated via adminhtml #12036
 - MAGETWO-83741: 11740: Sending emails from Admin in Multi-Store Environment defaults to Primary Store #11992
 - MAGETWO-83399: Fix for remove 'product_list_toolbar' block from layout in XML #9413 #11473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: accept Release Line: 2.2 Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants