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

feat: add refunds GraphQL query #5352

Merged
merged 30 commits into from
Aug 2, 2019
Merged

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Jul 22, 2019

Part of #5158
Impact: minor
Type: feature

A new listRefunds query allows you to query refunds of payments on a particular order. This was previously a meteor only method. The meteor method will be removed with #5158.

Breaking changes

None

Testing

  1. Place an order using both the Stripe payment method and IOU method
  2. Create a refund for each type of payment
  3. Use the new listRefunds query to see the refunds
listRefunds(orderId: {ORDER-ID}, shopId: {SHOP-ID}) {
   _id
    amount
    ...otherFieldsYouWant
  }

I've added tests here to test the permission / requirements of variables, but did not add tests to check a result, as the payment method has it's own tests for this element: https://github.com/reactioncommerce/reaction/blob/feat-kieckhafer-listRefundsGQLQuery/imports/plugins/included/payments-stripe/server/no-meteor/util/stripeListRefunds.test.js#L10

Signed-off-by: Erik Kieckhafer <[email protected]>
(cherry picked from commit 77d8b75)
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer kieckhafer requested a review from aldeed July 22, 2019 20:25
@kieckhafer kieckhafer marked this pull request as ready for review July 22, 2019 20:25
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
(cherry picked from commit 938b458)
@kieckhafer
Copy link
Member Author

kieckhafer commented Jul 22, 2019

@aldeed I wasn't sure if this should go into Payments or Orders. I put it in Orders because each specific payment method has it's own way of calculating the refunds, and this is just a list that is displayed on the Orders operator UI. Also, all of the old meteor versions of this are in the Orders package.

What do you think about it's location?

Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@aldeed
Copy link
Contributor

aldeed commented Jul 26, 2019

@kieckhafer I think I have the same thoughts as you, could go either place. Since payments are currently coupled with orders, leaving it in orders seems fine.

I'm assigning @focusaurus to do the full code review here, but my only comment is that for GraphQL, we try to omit words like get and list from the query names. That's pretty standard GQL best practice. So I'd go with just refunds.

Also keep in mind that something like this fits pretty well into the payment resolver. In other words, rather than (or in addition to) Query.refunds resolver, you could have a Payment.refunds resolver. This is nice because you'd already know order ID and payment ID in that resolver, plus you could avoid doing an extra query in Catalyst.

{
  order {
    payments {
      refunds {
        amount
      }
    }
  }
}

Or for a combined list for all payments, it could be similarly done as Order.refunds resolver. There can be several resolvers that all call the same query function giving clients various ways to get the same info.

@kieckhafer kieckhafer changed the title feat: add listRefunds GraphQL query feat: add refunds GraphQL query Jul 26, 2019
@kieckhafer
Copy link
Member Author

kieckhafer commented Jul 30, 2019

@focusaurus I added a new query here, refundsByPaymentId, and also added a couple resolvers so that you can query refunds on an order, and also refunds per payment, so there is some additional functionality to check since your last review.

query {
  orderByReferenceId(id: "ORDER-ID", shopId: "SHOP-ID") {
    _id
    payments {
      _id
      method {
        name
      }
      refunds {
        amount {
          displayAmount
        }
      }
    }
    refunds {
       amount {
        displayAmount
      }
      paymentDisplayName
    }
    shop {
      _id
    }
  }
}

Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

@kieckhafer A few last comments

@kieckhafer kieckhafer requested a review from aldeed July 31, 2019 00:10
@kieckhafer
Copy link
Member Author

@aldeed comments addressed, ready for a final look

Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@aldeed aldeed merged commit 9f73d76 into develop Aug 2, 2019
@aldeed aldeed deleted the feat-kieckhafer-listRefundsGQLQuery branch August 2, 2019 14:46
@kieckhafer kieckhafer mentioned this pull request Aug 28, 2019
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.

5 participants