search

Smartling / api-sdk-nodejs

3 stars 10 forks source link

feature: add callback url to job params #94

Closed fllprbt closed 11 months ago

fllprbt commented 1 year ago

@PavelLoparev regarding commit #2 (change endpoint to download all files translations): Even after the fix, I did not manage to write a .zip to my filesystem although I did not dig much into the code. Is writing non-raw responses supported atm? If not, do you have any plan of working on it in the near future?

For now, I consider it out of this MR's scope (no time) and will fetch translations separately by locales.

Let me know, thanks!

PavelLoparev commented 1 year ago

Hi @fllprbt, it appears Files API doc is wrong about having zipFileName query parameter for download all translations endpoint. We will fix the doc.

This param should be removed from the DownloadFileAllTranslationsParameters. If you try to download without that param api will respond with application/zip;charset=UTF-8 response like:

curl --location 'https://api.smartling.com/files-api/v2/projects/<projectId>/locales/all/file/zip?fileUri=<fileName>.<fileExtension>' \
--header 'Authorization: Bearer ...'

PK��� nW���������������zh-CN/<fileName>.<fileExtension>��<?xml version="1.0" encoding="UTF-8"?>
<zh-CN file content here>
PKg�������PK��� nW���������������da-DK/<fileName>.<fileExtension>��<?xml version="1.0" encoding="UTF-8"?>
<da-DK file content here>
...

And then you probably can save it as you need (didn't dig deeper, I just checked this sdk method returns exactly the same content as curl/postman call)

PavelLoparev commented 1 year ago

Is writing non-raw responses supported atm?

For now, it supports receiving raw response body as response.text() or parsed dtos from response json.

dimitrystd commented 1 year ago

Hi All, I didn't review the PR, but the zip word caught my attention. I strongly recommend not using zip in integrations. Because TMS must prepare all translated files before actual download, downloading a zip file may increase response time. When the user interacts with TMS, using zip makes sense. However, for automation, robustness is the top priority. Tens of short HTTP requests (with predictable response time) are preferable to one request when response time can range from seconds to minutes.

P.S. @conall-smartling for visibility. There are a couple of related tickets #89, #92.

fllprbt commented 1 year ago

Hi @fllprbt, it appears Files API doc is wrong about having zipFileName query parameter for download all translations endpoint. We will fix the doc.

This param should be removed from the DownloadFileAllTranslationsParameters. If you try to download without that param api will respond with application/zip;charset=UTF-8 response like:

curl --location 'https://api.smartling.com/files-api/v2/projects/<projectId>/locales/all/file/zip?fileUri=<fileName>.<fileExtension>' \
--header 'Authorization: Bearer ...'

PK��������� nW����������������zh-CN/<fileName>.<fileExtension>�����<?xml version="1.0" encoding="UTF-8"?>se
<zh-CN file content here>
PK��g�����������PK��������� nW����������������da-DK/<fileName>.<fileExtension>�����<?xml version="1.0" encoding="UTF-8"?>
<da-DK file content here>
...

And then you probably can save it as you need (didn't dig wdeeper, I just checked this sdk method returns exactly the same content as curl/postman

Right, i also had such glyphs back when i had tested it; had tried to parse it then as zip and failed but did not investigate it much...

fllprbt commented 11 months ago

@PavelLoparev could you review this MR please? Added also the list batches helper

fllprbt commented 11 months ago

done!

PavelLoparev commented 11 months ago

@fllprbt Sorry overlooked one thing. The list batches endpoint also receives query parameters. As I see list batches method only receives project id so it's not possible to get all the batches without pagination for now. I think parameters class with available pagination parameters and filters should be added.

fllprbt commented 11 months ago

Done.

Btw we would like eventually have something similar here: https://github.com/Smartling/api-sdk-nodejs/blob/master/api/jobs/index.ts#L57C63-L57C85

Since a params object can be passed, does it mean that we can use params.set to set these manually in case the specific helpers do not exist?

PavelLoparev commented 11 months ago

Done.

Btw we would like eventually have something similar here: https://github.com/Smartling/api-sdk-nodejs/blob/master/api/jobs/index.ts#L57C63-L57C85

Since a params object can be passed, does it mean that we can use params.set to set these manually in case the specific helpers do not exist?

yes, you can use .set() method but I would prefer to just to add anything that is missing/needed for you through the PRs.

PavelLoparev commented 11 months ago

2.4.0 is released

cipsicko commented 11 months ago

Hi, the setCallbackUrl method was added without setCallBackMethod that is mandatory otherwise an error occurs. https://api-reference.smartling.com/#tag/Jobs/operation/addJob

fllprbt commented 11 months ago

Hi, the setCallbackUrl method was added without setCallBackMethod that is mandatory otherwise an error occurs. https://api-reference.smartling.com/#tag/Jobs/operation/addJob

Sorry, missed that from docs. Can fix myself this Friday if no anyone else does

cipsicko commented 11 months ago

Hi, the setCallbackUrl method was added without setCallBackMethod that is mandatory otherwise an error occurs. https://api-reference.smartling.com/#tag/Jobs/operation/addJob

Sorry, missed that from docs. Can fix myself this Friday if no anyone else does

I can do it but i'm stucked here https://github.com/Smartling/api-sdk-nodejs/issues/95

PavelLoparev commented 10 months ago

This is going to be addressed in https://github.com/Smartling/api-sdk-nodejs/pull/100

PavelLoparev commented 1 month ago

As for downloading zip: https://github.com/Smartling/api-sdk-nodejs/pull/117