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

[Order][Payment] Mark payment as refunded #6555

Merged
merged 6 commits into from
Oct 28, 2016

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Related tickets
License MIT

This is for now just state machine work with ui implementation, no business refund logic included.

And the customer bought a single "Green Arrow"
And the customer chose "Free" shipping method to "United States" with "Offline" payment
And this order is already paid
And this order has already been shipped
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for that

*/
public function iShouldBeNotifiedThatTheOrderSPaymentHasBeenSuccessfullyRefunded()
{
$this->notificationChecker->checkNotification('Payment has been successfully refunded.', NotificationType::success());
Copy link
Contributor

Choose a reason for hiding this comment

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

Too long line

const STATE_PAID = 'paid';
const STATE_PARTIALLY_PAID = 'partially_paid';
const STATE_REFUNDED = 'refunded';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should decide if we should order these constants alphabetically or the order should be the same as in state machine and then correct it everywhere to stay consistent. Right now I would leave the previous order (same as in state machine) as it's applied everywhere else.

$refundedPaymentsCount = $this->getPaymentsWithState($order, PaymentInterface::STATE_REFUNDED)->count();

if ($refundedPaymentsCount === $paymentsCount) {
return OrderPaymentTransitions::TRANSITION_REFUND;
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is not true. Among paid payments we can also have failed payments and then this condition will never be true. We should have almost the same check as with paid. This time we should check if the refunded amount is equal to the amount paid.

@@ -0,0 +1,30 @@
@managing_orders
Feature: Refunding order payment
In order to refund order payment
Copy link
Member

Choose a reason for hiding this comment

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

...an order payment?

@michalmarcinkowski
Copy link
Contributor

We should add more specs to OrderPaymentStateResolver, we're missing at least the following cases:

  1. Two payments, one payment failed, one payment refunded - order payment state refunded
  2. Two payments, one payment paid for an amount equal to some part of the total, one payment refunded for the amount equal to the rest of the total - order payment state partially refunded
  3. Two payments, one payment failed, one payment paid - order payment state paid

Please add them (and maybe some more?) in a separate PR.

@michalmarcinkowski
Copy link
Contributor

Wrong button sorry 😕

@michalmarcinkowski michalmarcinkowski merged commit 69a0bb4 into Sylius:master Oct 28, 2016
@michalmarcinkowski
Copy link
Contributor

Thank you Mateusz! Please apply my comment in a separate PR.

@Zales0123 Zales0123 deleted the mark-payment-as-refunded branch October 28, 2016 13:30
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.

None yet

4 participants