ahmader / node-zoho

Zoho API access for NodeJS
21 stars 15 forks source link

Delete file #46

Closed pahan35 closed 6 years ago

pahan35 commented 6 years ago

I add some code processRecord cause it returned empty object on processing delete file response.

Please check how it works and let me to know if there is something wrong or maybe you have better suggestion how to parse it.

Preproccessed response object structure and my parsed result

image

ahmader commented 6 years ago

@pahan35 Can you please provide a working example for uploadFile and deleteFile ?

I am trying to create integration test for it to understand the responses better before merging deleteFile

And does uploadFile work for all modules, do I need to change any settings in Zoho for it to work ?

ahmader commented 6 years ago

I am thinking maybe we can add it to lib/response.coffee because there is the main parser for the response.

Also I see some changes in zoho API version=4 where <success> is heavily used for inersertRecords and updateRecords incase if multiple records.

pahan35 commented 6 years ago

@ahmader I use this method for file upload

function uploadFile(id, file, module) {
    return new Promise((resolve, reject) => {
        const method = 'uploadFile';
      const url = require("url"),
        path = require("path"),
        request = require("request");
      let filename;
      request(file, (error, res, body) => {
        if (error) return reject(error);
        const zoho = getZoho();
        zoho.execute('crm', module, 'uploadFile', id, body, {filename}, (err, result) => {
          let error;
          if (err !== null) {
            error = err;
          } else if (result.isError()) {
            error = result.message;
          }
          if (error) {
            reject(error);
          } else {
            resolve(result);
          }
        });
      });
      const parsed = url.parse(file);
      filename = path.basename(parsed.pathname);
    })
}

I've tested it only with Contacts module

pahan35 commented 6 years ago

deleteFile usage is similar

deleteFile(id) {
    return new Promise((resolve, reject) => {
      const zoho = getZoho();
      zoho.execute('crm', 'Contacts', 'deleteFile', id, (err, result) => {
        let error;
        if (err !== null) {
          error = err;
        } else if (result.isError()) {
          error = result.message;
        }
        if (error) {
          reject(error);
        } else {
          resolve(result);
        }
      });
    })
  }
pahan35 commented 6 years ago

@ahmader due to all this differences I prefer to use packages and libraries.

And in this case I have possibility to contribute to one of this :)

ahmader commented 6 years ago

@pahan35 can you check it out in your application, if it still behave the same?

Also do one more pull from master before we merge this.

Thanks

pahan35 commented 6 years ago

@ahmader upload file works as earlier.

Delete file has a bit another response format but this is even better.

Also code looks much better.

Thanks for your correction!

pahan35 commented 6 years ago

@ahmader I noticed that in uploadFile we have response.message = ['Message body'].

Maybe it's better to mark places with that kind of format amd rewrite them in future to similar structure (response.message = 'Message Body')?

ahmader commented 6 years ago

@pahan35 I posted this as a code review, I guess you did not see it.

I dont like the fact we return the object {data: {success: ...}} because in other cases like deleteRecords the response does not have <success>

This is example response from deleteRecords

<response uri="/crm/private/xml/Leads/deleteRecords">
  <result>
    <code>5000</code>
    <message>Record Id(s) : 2216462000000221069,Record(s) deleted successfully</message>
  </result>
</response>

Comparing to deleteFile

<response uri="/crm/private/xml/Events/deleteFile">
  <success>
    <code>4800</code>
    <message>Attachment deleted successfully.</message>
  </success>
</response>

This will make the user ends up with many conditions in his code. (Which Zoho API do)

Maybe if we return {data: {code: ..., message:...}} will be easier to check if the code matches the case the user want.

P.S these codes are not documented at Zoho API docs 😞