adds support for scan based merchant returns service #291
Conversation
| @@ -98,7 +99,7 @@ Active Shipping is currently being used and improved in a production environment | |||
|
|
|||
| ## Running the tests | |||
|
|
|||
| After installing dependencies with `bundle install`, you can run the unit tests with `rake test:units` and the remote tests with `rake test:remote`. The unit tests mock out requests and responses so that everything runs locally, while the remote tests actually hit the carrier servers. For the remote tests, you'll need valid test credentials for any carriers' tests you want to run. The credentials should go in ~/.active_shipping/credentials.yml, and the format of that file can be seen in the included [credentials.yml](https://github.com/Shopify/active_shipping/blob/master/test/credentials.yml). For some carriers, we have public credentials you can use for testing: see `.travis.yml`. | |||
| After installing dependencies with `bundle install`, you can run the unit tests with `rake`. The unit tests mock out requests and responses so that everything runs locally, while the remote tests actually hit the carrier servers. For the remote tests, you'll need valid test credentials for any carriers' tests you want to run. The credentials should go in ~/.active_shipping/credentials.yml, and the format of that file can be seen in the included [credentials.yml](https://github.com/Shopify/active_shipping/blob/master/test/credentials.yml). For some carriers, we have public credentials you can use for testing: see `.travis.yml`. | |||
There was a problem hiding this comment.
FYI, this will conflict with https://github.com/Shopify/active_shipping/pull/290/files#diff-04c6e90faac2675aa89e2176d2eec7d8R101
There was a problem hiding this comment.
Yeah, sorry, I modified this text because it doesn't look like you have a task called test:units or test:remote. I will revert, however.
There was a problem hiding this comment.
That's exactly what the other PR noticed and fixed. 😛
The other one is smaller and closer to being merged though.
|
A few questions and comments. Mostly good work. If you can address them and get the build green and we'll get this merged soon for you. Thank you for your contribution. |
| LIVE_DOMAIN = 'returns.usps.com' | ||
| LIVE_RESOURCE = 'Services/ExternalCreateReturnLabel.svc/ExternalCreateReturnLabel' | ||
|
|
||
| TEST_DOMAINS = { #indexed by security; e.g. TEST_DOMAINS[USE_SSL[:rates]] |
There was a problem hiding this comment.
Why is this a hash if both possibilities are the same?
|
@kmcphillips and @trishume thank you so much for your feedback. I really appreciate it. |
orenf
force-pushed
the
wip-usps-return-to-shopify
branch
from
7315774
to
56ea4ee
Compare
|
Still waiting on test credentials that I can pass on from USPS in order to get the build green. Otherwise, I believe this pull includes all the changes you suggested. @kmcphillips what is the typical process here for running remote tests that require credentials? |
|
This is currently a pain point in active_shipping. Our CI only includes a small set of credentials which we have obtained and know are safe to release publicly. We're working on expanding that set but for now most of the tests are set to skip on authentication failure and I just run the remote tests locally before I do a release with Shopify's test credentials set. I'm fine with merging this without public credentials provided you could point me in the right direction for me to get some for Shopify's internal use. If you gave us a set of credentials along with this PR that would be amazing and you would join the list of my favorite contributors ever ❤️ |
| if node = document.at('*/errors') | ||
| if node.at('ExternalReturnLabelError') | ||
| if message = node.at('ExternalReturnLabelError/InternalErrorDescription').try(:text) | ||
| if code = node.at('ExternalReturnLabelError/InternalErrorNumber').try(:text) |
There was a problem hiding this comment.
code = node.at('ExternalReturnLabelError/InternalErrorNumber').try(:text) || ''
res = {:error => {:code => code, :message => message}}and then get rid of the if statement, makes this method way simpler. Same for the other branch.
There was a problem hiding this comment.
This is a no brainer. I already fixed. Thanks for your suggestion.
|
Just realized I hadn't actually reviewed this yet. Found some coding style things that could do with some refactoring. This PR is just outstanding though, the level of documentation, validation, testing and everything else blows me away. And the fact that you without even being prompted say you're looking into getting us credentials. And the fact that you say it's been tested in production for a long time. Just wow. In fact since this PR is so great I'm willing to do the refactors I suggested myself if you'd like. I have pro editor macro skills so it shouldn't take long. |
orenf
force-pushed
the
wip-usps-return-to-shopify
branch
from
56ea4ee
to
320a2b3
Compare
|
@trishume thank you for your very kind words !!! I think you're right that I can and should refactor some of the validations. I need a couple of min to take care of those changes. Also, I realized that remote tests were rescue'ing with NoCredentialsFound, which is something I failed to do. And lastly, I'm still waiting on a response from USPS. I received an e-mail earlier that they were looking into getting me some creds. |
|
@trishume , I applied your recommendations for dry'ing up validations. I had the validations themselves executing |
|
Amazing 👍 I'll merge this when I do the next release. If USPS gets back to you with credentials and you can make them public you can submit another PR later (if I've already merged this one) or if they can't be public but you'd like to donate them to Shopify you can email shipping@shopify.com. |
|
Thank you @trishume, I sent another e-mail to USPS to request temporary credentials for the sake of testing that you can do on your end. I'll continue to follow up with them and will let you know accordingly. Would you like me to squash these commits and force push a single commit? |
|
Sure that would be great. |
orenf
force-pushed
the
wip-usps-return-to-shopify
branch
from
6d51522
to
3fa7623
Compare
|
|
Great. shipping@shopify.com goes to the entire shipping team. If you want to have a specific contact I'm tristan.hume@shopify.com but I'm an intern and will only be here for 2 more months. If you want a more permanent contact kevin.mcphillips@shopify.com is the shipping team lead. |
adds support for scan based merchant returns service
adds support for scan based merchant returns service
https://ribbs.usps.gov/merchantreturns/documents/tech_guides/MerchantReturnsDomesticAPI.pdf