-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Make approve/capture/refund/listRefunds multishop-aware #2912
Conversation
@zenweasel I know you were pushing to get this ready before you finished up last week, but as it's still [WIP] and no reviewers have been assigned, I'd imagine it's not quite done yet? |
@spencern Yes. The methods are working but tests are still broken. Should have this ready for review today |
Got all tests working but when I pulled in changes all the order filtering/dashboard is broken. Trying to figure out what is going on |
Still working on fixing up orders but hope to have this ready tomorrow morning some time |
Ok @spencern this should be ready to look at now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm able to capture and refund stripe connect orders, and that's the major accomplishment of this PR.
I'm also seeing errors in the client when I capture on merchant shop, then capture and start to fulfill on primary shop. This error prevents me from using the orders dashboard from the merchant shop. But I was able to successfully capture the charge before getting locked out.
TypeError: Cannot read property 'Icon' of undefined
at OrderDashboard.render (orderDashboard.js:118)
at modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:108703
at measureLifeCyclePerf (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:107983)
at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:108702)
at ReactCompositeComponentWrapper._renderValidatedComponent (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:108729)
at ReactCompositeComponentWrapper.performInitialMount (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:108269)
at ReactCompositeComponentWrapper.mountComponent (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:108165)
at Object.mountComponent (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:100926)
at ReactDOMComponent.mountChildren (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:107281)
at ReactDOMComponent._createInitialChildren (modules.js?hash=7a1e98364f0a9d66bf273a939be2089560be603f:104480)
I'm seeing i18n issues on the merchant shop orders page, but not the primary shop, but based on the code it seems unlikely that's the fault of this PR, and should probably be addressed in another PR.
This PR highlights a some other marketplace related issues I think - around the display and fulfillment of part of an order by multiple shops and admins.
I'm missing the name of the customer in the merchant shop order list.
There are other issues that are known and already have tickets that are present in this PR:
- Shipping charge is multiple of actual shipping cost
- Extra boxes on orders list
I'm tempted to approve this despite all the issues I'm seeing as I do think it accomplishes what it sets out to accomplish, but should probably create issues and verify they are present in the marketplace
branch before doing so and merging.
@@ -28,17 +25,33 @@ function unformatFromStripe(amount) { | |||
return (amount / 100); | |||
} | |||
|
|||
export const utils = {}; | |||
|
|||
utils.getStripeApi = function (paymentPackageId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for abstracting this. Needed to happen
transactionId: paymentMethod.transactionId, | ||
captureDetails: captureDetails | ||
}); | ||
const capturePromise = stripe.charges.capture(paymentMethod.transactionId, captureDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should change my current createCharges
method to use Promises instead of async/await? Feels weird to me to do this in different ways within the same plugin.
@@ -185,7 +185,10 @@ export function buildOrderSearchRecord(orderId) { | |||
orderSearch.billingName = shopBilling.address && shopBilling.address.fullName; | |||
orderSearch.billingPhone = shopBilling.address && shopBilling.address.phone.replace(/\D/g, ""); | |||
orderSearch.shippingName = shopShipping.address && shopShipping.address.fullName; | |||
orderSearch.shippingPhone = shopShipping.address && shopShipping.address.phone.replace(/\D/g, ""); | |||
if (shopShipping.address && shopShipping.address.phone) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this sneak in from a different PR? Or just fixing an undocumented issue that you came across?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running into this throwing a can't find replace of undefined
when trying to place an order. Probably just something that hadn't come up in testing before? Sort of just a workaround but required to keep moving.
// than one shop | ||
/** | ||
* ordersInventoryAdjustByShop | ||
* adjust inventory for a particular shop when an order is approved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of removing the "approval" step, I think that inventory should be reduced when an order is placed (after copyCartToOrder) so that we avoid the possibility of multiple orders for the same inventory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing Approval should probably be separate from this PR. Created a separate issue for it here: #2988
@spencern Yeah, these other (I believe unrelated) issues are why this took me longer than expected (to fix some and document others). But I think this PR does what it's supposed to without introducing any new errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more issues that I think I've found, but want to make sure I'm not misunderstanding where something might be stored
-
When the
capture
is performed, it appears to be stored a level deeper than it should be in thetransactions
array. Show here with thecharge
response and thecapture
response wrapped in aresponse
object.
-
I'm not seeing where the refund transaction is stored. After issuing two refunds, I'm seeing both of them in the UI, but neither in the Payment object transactions array. As I understand it, the
transactions
array is supposed to be a record of every transaction that has occurred with that payment, so I was expecting to find two refund transactions. It's clear something is being stored somewhere though, so perhaps I'm just looking in the wrong place?
It does seem to work otherwise at this point, so if we can clarify these two issues, I think we're set to merge.
Refunds are not stored anywhere for any payment methods. That's essentially why we have the |
Regarding the transaction being wrapped in a "response" object. It appears that other payment providers do this sometimes, but it's the raw response from the payment provider. Since each provider returns different data in different formats, I'm going to say this is not a blocker and we'll merge and reconcile later if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @zenweasel and I's convo and looking into the specific code around the transaction objects, I think this is good to go.
Close #2767, #2766, #2765
To Test