search

beardon / mws-api

Amazon Marketplace Web Services client for Node.js.
79 stars 55 forks source link

Missing MerchantIdentifier in header using new FeedGen. #82

Closed BarryCarlyon closed 7 years ago

BarryCarlyon commented 7 years ago

Basically a clone of #75 but for the Ack. data instead.

Added the associated test.

Probably could so with adding a test for dealing with item not being provided as it's optional. But Test coverage isn't my strong point

dciccale commented 7 years ago

LGTM so far đź‘Ť

BarryCarlyon commented 7 years ago

As per notes in 75, feeds are being rejected by amazon due to no ability for the module to create a valid header with a MerchantIdentifer

dciccale commented 7 years ago

Hm.. I'll see how it was done before, since I am using the previous version still. And it is working correctly. Maybe I could bring this as a fix

BarryCarlyon commented 7 years ago

Follow up in #83

BarryCarlyon commented 7 years ago

I suppose this makes it a breaking change?

Version bump needed?

dciccale commented 7 years ago

Actually, the current npm version 2.1.0 was not broken. And 2.2.0 (current package.json on github) was not released yet to npm, so we could just roll out 2.2.0 with all these fixes as a minor version since there are no actual breaking changes from 2.1.0.

does make sense?

BarryCarlyon commented 7 years ago

Indeed yes.

I wondered why I got 2.1 from NPM last time when 2.2 was here

dciccale commented 7 years ago

Ok I was wrong on my previous comment, actually it is a breaking change. Not for the output part, but for the input part of the feeds where now the header must be passed.

I correct myself, this should go to 3.0 directly due to this change.

@BarryCarlyon Before merging, aren't we breaking other Feeds where the new header argument should be passed in?

@tyscorp @aaronbean just to keep you in the loop, changes Introduced by me on the multi message feature had some bugs (sorry) now are being fixed on this PR + Order ack being multi messages as well. For making multi messages work properly on Amazon we had to introduce a breaking change by adding MerchantIdentifier to a header object being passed to the Feed generator function, to separate this from the part of the output (before it was all together).

BarryCarlyon commented 7 years ago

Technically no. We are not breaking any other feeds as ALL the other feeds that are defined are just throwing back a errorNotImplemented(FEEDNAME) the only Feeds with a "object maker", in the four files in lib/feeds are orders.js with _POST_ORDER_ACKNOWLEDGEMENT_DATA_ and _POST_ORDER_FULFILLMENT_DATA_

So, breaking change yes, breaking other feeds no, because none of them are implemented.

dciccale commented 7 years ago

OK then this should be good to merge.

Let's see if some of the authors have any more comments on this.

If we're going to 3.0.0, I have a branch with ESLint implemented based on airbnb styleguide. Which should help us improve a bit potential errors and having a normalized style. That we could ship on this major also.

BarryCarlyon commented 7 years ago

In other news, jsut setup my first orderfulfillment attempt. And discovered that the definition is lacking some keys so patching and testing, extending this PR again tomorrow after some more testing!

_POST_ORDER_FULFILLMENTDATA lackings keys under item and item should be able to support more than one item.

dciccale commented 7 years ago

Good catch. However Items is optional here. And should be optional in the library too. I am currently using this in production without sending items.

On May 30, 2017 5:43 PM, "Barry Carlyon" notifications@github.com wrote:

In other news, jsut setup my first orderfulfillment attempt. And discovered that the definition is lacking some keys so patching and testing, extending this PR again tomorrow after some more testing!

POST_ORDER_FULFILLMENT_DATA lackings keys under item and item should be able to support more than one item.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/beardon/mws-api/pull/82#issuecomment-304920424, or mute the thread https://github.com/notifications/unsubscribe-auth/AAg7miZYqwKmUIGWuPDCQGT0aAgfhSzPks5r_DkigaJpZM4NmPNL .

BarryCarlyon commented 7 years ago

Yeah the XSD https://images-na.ssl-images-amazon.com/images/G/01/rainier/help/xsd/release_1_9/OrderFulfillment.xsd has Item set to minOccurs 0 max infinity.

I'll revise the PR

BarryCarlyon commented 7 years ago

The map function should handle it happily the same as _POST_ORDER_ACKNOWLEDGEMENT_DATA_ and my feed has now been accepted by amazon.

BarryCarlyon commented 7 years ago

Oh I didn't patch the tests…

I wonder if we should consider adding some XSD validation.

EG, you can specify one of AmazonOrderID or MerchantOrderID in OrderFulfillment but not both?

dciccale commented 7 years ago

Yep tests would be good.

Reg. XSD validation, seems a good idea however it could be rather complex to implement. Plus we should be confident relying on the validation amazon is doing when posting the generated file, and error responses are descriptive enough I think to know how to fix the document.

dciccale commented 7 years ago

@BarryCarlyon could you bump package.json to 3.0.0? Is there anything else to be added on this PR? It looks good from my side

BarryCarlyon commented 7 years ago

I'm gonna throw a configuration in for refunds (thats this weeks $dayjob task)

Also could do with #66 being considered?

Why are we forcing binary which trashes foreign chars?

dciccale commented 7 years ago

That we should discuss on the designated issue / pr.

Reg. this PR I assume it is good to go, will merge it later today if no more comments

BarryCarlyon commented 7 years ago

Sounds good. I'll bump the version real quick then

dciccale commented 7 years ago

Awesome job @BarryCarlyon thanks