Closed blakescholl closed 10 years ago
Thanks, Blake.
The arguments of #create_fulfillment_order
will need to change to remain consistent with the rest of the library. My rule of thumb is to turn the required parameters into individual arguments and push the rest into an optional opts
Hash. If you want to complete this, there are also two other relatively minor tasks: Documenting the source code and merging my master back into your branch.
Regarding the integration tests, I chose not to commit the VCR fixtures into the repo because some output has relatively private data (order, customer details etc.) and it would have been too much of a hassle to manually scrub those.
I try to keep the integration tests simple and generally skip integration-testing methods that require account-specific input or some additional setup beyond regular the MWS subscription.
If you have issues with any of the above, I will squeeze in time next week to merge this in.
Hey guy,
Thanks for the feedback. All of your comments on the code make total sense to me (I'm more of a product guy, and my coding is super rusty). I'll clean this stuff up and send you a fresh pull request in a day or two.
Regarding the integration tests: I hear you, but maybe it would be valuable to come up with a set of VCR tapes that are sufficiently sanitized that they can be checked in. Otherwise, it just seems really easy to break something. It looks like you can get Amazon to enable your account for staging access.http://www.otreva.com/amazon-marketplace-web-services-mws-staging-account-login/ So you can set up a set of fake data to work with.
--Blake
On Thu, May 1, 2014 at 11:52 AM, Hakan Ensari notifications@github.comwrote:
Thanks, Blake.
The arguments of #create_fulfillment_order will need to change to remain consistent with the rest of the library. My rule of thumb is to turn the required parameters into individual arguments and push the rest into an optional opts Hash. If you want to complete this, there are also two other relatively minor tasks: Documenting the source code and merging my master back into your branch.
Regarding the integration tests, I chose not to commit the VCR fixtures into the repo because some output has relatively private data (order, customer details etc.) and it would have been too much of a hassle to manually scrub those.
I try to keep the integration tests simple and generally skip integration-testing methods that require account-specific input or some additional setup beyond regular the MWS subscription.
If you have issues with any of the above, I will squeeze in time next week to merge this in.
— Reply to this email directly or view it on GitHubhttps://github.com/hakanensari/peddler/pull/19#issuecomment-41943264 .
Pull request updated w/ the API approach you suggested. LMK if you want anything else tweaked here.
I've done some minor refactoring and merged your commits.
MWS::FulfillmentOutboundShipment#create_fulfillment_order
, with seven (!) required arguments, is a beast and begs for abstraction, but that's possibly outside of the scope of Peddler.
If you implement the remaining methods, I'll be more than happy to merge back in.
Thanks for the link on staging accounts, which I'll look into in the near future. Testing against real accounts is definitely hairy.
Draft support for fulfillment orders below - likely needs tweaks before you can merge this.
In particular, I'm not sure exactly how you are approaching tests... the test here only works against my MWS account (as it references a particular item). We could move that reference out into a yml file, but then it won't work against any account that doesn't have FBA.
FWIW, I'm having a similar issue getting your tests to pass against my account—the recommendations test is failing, I think because I don't have enough activity on my MWS account to generate any recommendations.
P.S. Thanks for an awesome library