lineofflight / peddler

Amazon Selling Partner API (SP-API) in Ruby
https://lineofflight.github.io/peddler/
MIT License
307 stars 130 forks source link

Invoice generation and upload #138

Closed jackscotti closed 4 years ago

jackscotti commented 5 years ago

Hi Hakan,

First, let me say, this gem is great! You made our lives so much easier!

Amazon just started to request invoice upload for every order, are you by any chance adding this feature to Peddler?

VAT Calculation Service (VCS): Developer Guide

hakanensari commented 5 years ago

Hi @jackscotti

Sorry for the late reply. I skimmed through the guide. If I understand correctly, they require you to base64-encode the PDFs and upload with a feed.

I would expect you to implement this on your end—there are quite a few other feeds where Amazon expects non-text data, and Peddler doesn't support any at this point.

They have some Java sample code in the guide; you could reimplement something similar in Ruby?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ziptofaf commented 4 years ago

@hakanensari Hello, I am encountering a similar issue as I am trying to send a binary PDF file through Peddler and feeds API.

I have forked your project and added few lines that accept an optional binary argument when trying to submit a feed and set Content-Type to application/octet-stream in that situation. Will you accept a pull request or do I have to stick to my fork forever now?

https://github.com/hakanensari/peddler/compare/master...ziptofaf:master

On the end user side it requires a rather minimal change and it's backwards compatible:

f = File.open('example.csv').read
client.submit_feed(f, 'FeedType')

vs:

f = File.open('example.pdf', 'rb').read
client.submit_feed(f, 'FeedType', binary: true)
hakanensari commented 4 years ago

@ziptofaf yes, handling the content type header definitely makes sense.

Maybe we could check if file is binary in the code and do all this transparently in the background? Off the top of my head, would the mime-types gem handle this?

ziptofaf commented 4 years ago

@hakanensari Hmm, not necessarily mime-types (since after all we are pushing file contents, not it's name and from Ruby perspective it's just a string). However I can do this:

f = File.open('example.pdf', 'rb').read
f.encoding.name

That returns 'ASCII-8BIT' which is enough to guess that we are dealing with a binary file (there's also .names which returns ['ASCII-8BIT', 'BINARY']). Amazon doesn't really expect to see "application/pdf" there, it just wants a binary "application/octet-stream" and figures out what exactly you have uploaded to it by itself afterwards. At least in my experience this works just fine for pdf invoices.

So I guess that's an option too to check encoding and if it's binary make sure we don't transform it in any way and send it as is.

hakanensari commented 4 years ago

Sounds good, you probably want to read only a bit of the file not to bloat memory?

ziptofaf commented 4 years ago

Am I missing something in the API of peddler that you can only send it a filename/path? I mean, I don't see a place for optimization that would check only first few bits as looking at the submit_feed it only accepts a file content, the whole thing already loaded from file into string. Plus it has to send it to amazon anyway in the next step so whether it's preloaded or not doesn't really change much (or well, I guess there could be a use for a buffer and whatnot but that sounds like a potentially breaking change in your library as it would need to handle file streaming correctly).

Overall my change would look like this, essentially accounting to 5 lines of code (and a test to make sure that binary is treated as binary and doesn't crash eg. if something tried to treat it as normal text):

https://github.com/hakanensari/peddler/compare/master...ziptofaf:master

hakanensari commented 4 years ago

You’re absolutely right, I was writing on my phone without looking at the original code 😀 This looks great, let’s merge this with a pull request? Could you add an entry to CHANGELOG as well?

ziptofaf commented 4 years ago

@hakanensari Sure, added and made a pull request :) I assumed it that it will be 2.4.1 version entry since it's effectively a small fix.

hakanensari commented 4 years ago

@ziptofaf :+1::+1::+1:

philsmy commented 3 years ago

Sorry for commenting on this closed issue, but, does this mean we can now use Peddler to upload VAT invoices? If so, is there an example somewhere?

ziptofaf commented 3 years ago

@philsmy

I think it would be something like this:

contents = File.open('invoice.pdf', 'rb').read
client = MWS::Feeds::Client.new(your_credentials)
feed_options = "metadata:OrderId=#{amazon_id};metadata:InvoiceNumber=#{your_internal_invoice_number}"
client.submit_feed(contents, '_UPLOAD_VAT_INVOICE_', FeedOptions: feed_options)

Or something along these lines anyway.

philsmy commented 3 years ago

Thanks!