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

Migration updates to fix Localization, uol, and paymentMethod errors on 1.0 > 1.5 #3020

Merged
merged 11 commits into from
Oct 9, 2017

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Oct 3, 2017

Fixes:

  1. Reported error on Localization dashboard in comments and localization tab is broken on latest marketplace branch #2965
  2. Fixes errors with "Units of length" when creating new shops
  3. Updates shipping migration with new status (picked and labeled - introduced in [Marketplace] Set order(s) to a specific status #2613)
  4. Error on refunds (an order with captured payment (at <1.4 Reaction), when migrated to marketplace/release1.5 branch, throws an error if you try to refund (caused by changes introduced in Cancel Order #2022)

How to test

  1. Follow instructions in Create migration files for upgrade from 1.0 to 1.5 #2962 about creating orders in v 1.0 and moving back to marketplace version. While in v1.0 capture payment in one of the orders (we'll use it to test refund later)
  2. After moving from <1.4 to 1.5, confirm that:
  • Localization dashboard works. No errors should be thrown when it opened
  • Creating new shops work fine without any Units of Length errors.
  1. In the Order dashboard, visit the order with captured payment and try to do a refund. You should get a success message and no errors.

@impactmass
Copy link
Contributor Author

Currently working on fixing the last item on this (i.e the refunds issue)

@spencern
Copy link
Contributor

spencern commented Oct 6, 2017

@impactmass great to hear, thanks for the update

@spencern spencern changed the base branch from marketplace to release-1.5.0 October 6, 2017 15:55
@impactmass impactmass changed the title [WIP] Migration updates 1.0 > 1.5 Migration updates to fix Localization, uol, and paymentMethod errors on 1.0 > 1.5 Oct 6, 2017
@impactmass impactmass requested a review from kieha October 6, 2017 22:48
@impactmass
Copy link
Contributor Author

@spencern you can test this out now.
@kieha I tagged you as well because of the picked/labeled status you spotted. Help check if the update on that looks good

@brent-hoover
Copy link
Collaborator

brent-hoover commented Oct 7, 2017

When I go into the Localization panel and select the dropdown for "Base Unit of Length" I get "no results" and can't set it. Also probably not related to this PR but isn't there supposed to be a "save" button somewhere?

cart_completed

@brent-hoover
Copy link
Collaborator

I haven't tested if this also happens on the main branch but doing a refund on an order processed with Paypal Express gives me this error

Exception in delivering result of invoking 'orders/refunds/create': TypeError: Cannot read property 'split' of undefined
    at setParameters (http://localhost:3000/packages/modules.js?hash=b2faf2cb75c484b5324fa66592304113bc3ffa4f:1782:36)
    at sweetAlert (http://localhost:3000/packages/modules.js?hash=b2faf2cb75c484b5324fa66592304113bc3ffa4f:2116:3)
    at Object.alert (http://localhost:3000/app/app.js?hash=82043065b1802510af5eae8cbe0b84fb55332c82:60406:14)
    at http://localhost:3000/app/app.js?hash=82043065b1802510af5eae8cbe0b84fb55332c82:70505:24
    at MethodInvoker._callback (http://localhost:3000/packages/meteor.js?hash=6d285d84547b3dad9717a7c89c664b61b45ea3d8:1117:22)
    at MethodInvoker._maybeInvokeCallback (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3682:12)
    at MethodInvoker.receiveResult (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3702:10)
    at Connection._livedata_result (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:4824:9)
    at onMessage (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3528:206)
    at http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:2908:9

@brent-hoover
Copy link
Collaborator

Units of Length seems to work fine after reset so something is still not right.

@spencern
Copy link
Contributor

spencern commented Oct 7, 2017

@impactmass I've identified the issue with UOL and added a migration for that.

I've been playing around with the refunds for a bit and have not been able to successfully get a refund to process for an order placed and captured on 1.0.0 to refund on this branch.

This commit tweaks 76a03de the billing migration a bit, but I'm still unable to process a refund for paypal-express.

Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

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

I think we still need a little bit of work to make this work for refunds.

@impactmass
Copy link
Contributor Author

impactmass commented Oct 7, 2017 via email

@spencern
Copy link
Contributor

spencern commented Oct 7, 2017

@impactmass I did not test with any of those, so it's possible this is an issue exclusively related to Paypal, but I do think in this particular instance it would be good to check each payment provider individually.

@brent-hoover
Copy link
Collaborator

Without even looking at the code I am going to guess that the split that's failing there is trying to parse a credit card number that doesn't exist with Paypal Express. Always need to test PPE when testing payment methods since it's the one outlier.

@brent-hoover
Copy link
Collaborator

Tested Paypal refunds on both the release branch and v1.0.0 and they work correctly.

@brent-hoover
Copy link
Collaborator

brent-hoover commented Oct 9, 2017

Tested placing orders on v1.0.0 and migrating to this branch and placing refunds multiple times and did not get an error. 🤔

@impactmass
Copy link
Contributor Author

impactmass commented Oct 9, 2017

Yes. I also tested a few hours ago. Three times with Paypal Express. With refunds working on this branch. I'm going to do another run of tests to be sure that all is working correctly (with all current Payment methods).

The split error that came up in previous test might have been due to the way the response is processed here:

const pieces = response.content.split("&");
pieces.forEach(function (piece) {
const subpieces = piece.split("=");
const decodedResult = result[subpieces[0]] = decodeURIComponent(subpieces[1]);

@spencern
Copy link
Contributor

spencern commented Oct 9, 2017

@zenweasel @impactmass are you suggesting that it's migrating correctly now?

@impactmass
Copy link
Contributor Author

impactmass commented Oct 9, 2017

I think it was migrating correctly before. I haven't made any change and refunds worked in my tests for PayPal (Flow and Express), Stripe, Example. I'm currently testing AuthNet and Braintree.

If you can, test again, and see if you can reproduce the error you got before.

Update: I've tested all current payment providers.

@spencern
Copy link
Contributor

spencern commented Oct 9, 2017

@impactmass I'll run through some tests as soon as I get out of meetings this am

@spencern spencern merged commit fd5e585 into release-1.5.0 Oct 9, 2017
@spencern
Copy link
Contributor

spencern commented Oct 9, 2017

This worked for me with PayPal Express this am. I think the issues I was experiencing were probably because of the refund not appearing reactively on the order detail action view.

My guess is that this is because we do not store refunds, but make a separate call to the payment provider for them with orders/refunds/list. This is necessary because a user could grant a refund in the payment provider dashboard as well as within the Reaction instance, so we can't rely on our list of order refunds being accurate without building some type of webhook that the payment API can push to.

Merging 👍

@spencern spencern deleted the seun-migration-updates-2 branch October 9, 2017 21:46
@spencern spencern mentioned this pull request Oct 11, 2017
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