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

WooCommerce: Enable refunds on orders #15589

Merged
merged 17 commits into from
Jun 30, 2017
Merged

WooCommerce: Enable refunds on orders #15589

merged 17 commits into from
Jun 30, 2017

Conversation

ryelle
Copy link
Member

@ryelle ryelle commented Jun 28, 2017

This PR splits the OrderDetails component into OrderDetailsTable and OrderRefundCard. In OrderRefundCard, I've added a refund modal so that store owners can refund customers.

Screenshot of the modal, where a user can refund based on items quantities, shipping cost, or enter a custom refund amount:

screen shot 2017-06-27 at 7 43 04 pm

If they try to refund more than the cost of the order, they see an error:

screen shot 2017-06-27 at 8 07 17 pm

If the refund is successful, there's a global notice after the modal closes (if the refund fails on the server side, it's an error notice), and the order details update with the new refund amount.

screen shot 2017-06-27 at 8 18 54 pm

Left to do:

  • Entering in a custom refund amount is broken right now, either I'll fix this or convert it to read only. Custom amount has been switched to read-only, formatting the price breaks the input
  • The credit card display needs to be hooked up. See WooCommerce: Enable refunds on orders #15589 (comment), this will be coming out and replaced with payment method info
  • A complete refund should toggle the order status to refunded. Does this automatically 🎉

To test

  1. Make sure you have the latest master API plugin on your site
  2. Visit your orders list /store/orders/:site
  3. Click into an order
  4. Click the "Submit Refund" button
  5. Create a refund, try with various refund amounts (0, higher than total, partial refund, exact...)

Run npm test to test the new action/reducer/selector (for tracking refund requests).

Fixes #15672

@ryelle ryelle self-assigned this Jun 28, 2017
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Jun 28, 2017
@matticbot
Copy link
Contributor


return (
<div className="order__details-total-refund">
<div className="order__details-totals-label">{ translate( 'Refunded' ) }</div>
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 37 times:
translate( 'Refunded To' ) ES Score: 12
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

@ryelle
Copy link
Member Author

ryelle commented Jun 28, 2017

So, it turns out we don't know just from the API whether a payment method supports automated refunds. For example, check payments don't support automated refunds, but PayPal does. Right now if a store owner tries to refund an order paid by check, it will error. It's fine if the customer paid with paypal, though.

The credit card display here also doesn't work, the automated refund should happen from the payment gateway - so orders paid via PayPal would refund from paypal, stripe from stripe, etc. This could be "Refunding payment from PayPal" or "Refunding payment from Stripe" instead.

const refundObj = {
amount: this.state.refundTotal + '', // API expects a string
reason: this.state.refundNote,
// api_refund
Copy link
Member Author

Choose a reason for hiding this comment

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

This line will turn on/off automated refunds, depending on whether this payment method supports it

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been handled in #15702 (which builds on this PR)

@ryelle ryelle added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 29, 2017
@ryelle
Copy link
Member Author

ryelle commented Jun 29, 2017

I've just realized I want to change my approach here for the state, so I'm putting this back into in-progress.

@ryelle ryelle added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 29, 2017
@ryelle ryelle added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 30, 2017
Copy link
Contributor

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

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

LGTM. My only required change is using PriceInput. Other comments are minor. Pre-approving.

Tested with Stripe and it worked.


onChange = ( event ) => {
if ( 'shipping_total' === event.target.name ) {
const shippingValue = event.target.value.replace( /[^0-9,.]/g, '' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Just call this shippingTotal, so you can save some characters in the line below.

const shippingValue = event.target.value.replace( /[^0-9,.]/g, '' );
this.setState( { shippingTotal: shippingValue }, this.recalculateRefund );
} else {
const i = event.target.name.split( '-' )[ 1 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a small comment above this block explaining that it's splitting thequantity-Xfields. I know that it becomes clearer the further down you get (since it is setting quantity a few lines below), but it still took me a a second to figure it out.

<div className="order__details-totals-label">{ translate( 'Shipping' ) }</div>
<div className="order__details-totals-value">
{ isEditable
? <FormTextInputWithAffixes
Copy link
Contributor

Choose a reason for hiding this comment

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

Use PriceInput here, and you can get the correct currency pulled in: https://github.com/Automattic/wp-calypso/blob/82524bf32455d4340bda9d9cdd258dbc8e0e4d35/client/extensions/woocommerce/components/price-input/index.js

<PriceInput currency={ order.currency } value={ this.state.shippingTotal } .... />

<FormFieldset className="order__refund-details">
<FormLabel className="order__refund-amount">
<span className="order__refund-amount-label">{ translate( 'Total refund amount' ) }</span>
<div className="order__refund-amount-value">
Copy link
Contributor

Choose a reason for hiding this comment

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

PriceInput.


/**
* Returns the updated order requests state after an action has been
* dispatched. The state reflects a mapping of query (page number) to a
Copy link
Contributor

Choose a reason for hiding this comment

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

page number should be order id

/**
* Returns the updated order requests state after an action has been
* dispatched. The state reflects a mapping of query (page number) to a
* boolean reflecting whether a request for that page is in progress.
Copy link
Contributor

Choose a reason for hiding this comment

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

page = order

data: {
message: 'No route was found matching the URL and request method',
error: 'rest_no_route',
status: 400,
Copy link
Contributor

Choose a reason for hiding this comment

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

A second status wouldn't be returned unless using _envelope

- Switch to PriceInput
- Fix up docs & variable names
- Fix failing request action test
@ryelle ryelle added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 30, 2017
@ryelle ryelle merged commit 91e4d9a into master Jun 30, 2017
@ryelle ryelle deleted the update/woo-orders-refund branch October 3, 2017 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] XL Probably needs to be broken down into multiple smaller issues Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WooCommerce Orders: Refunding payment
5 participants