ahmader / node-zoho

Zoho API access for NodeJS
21 stars 15 forks source link

Implement upload file to Contacts module #26

Closed pahan35 closed 6 years ago

pahan35 commented 6 years ago

Add possibility to upload file to Contacts based on Leads module.

Allow send file via attachmentUrl.

Possibly it's better to move this method to some helpers and use it in modules which allow to do this

ahmader commented 6 years ago

Thanks for contribution.

I want to run an integration test on this before accepting.

pahan35 commented 6 years ago

Ok.

BTW for some reason I can't run test via grunt cause it throw error for me.

image

Have you any idea how to fix it?

ahmader commented 6 years ago

What is the command you used for testing ?

pahan35 commented 6 years ago

I tried to run grunt and grunt integration and got error Task spec:[name] not found

When I leave only coffeelint in task I got SyntaxError: Invalid or unexpected token in grunt-jasmine-file.

image

When I comment line //grunt.loadNpmTasks('grunt-jasmine-bundle'); it works but with warnings about missed task spec:[name]

image

Also I noticed that WebStorm also doesn't show tasks spec:[name] in his list

ahmader commented 6 years ago

Honestly, I never tried to compile/test or use this package on Windows.

Nevertheless, have you ever tried to use grunt/test on this machine for other projects?

However, I am interested to know what is the cause of the problem, I will try to get my hands on a Windows machine and run the test.

BTW, which Windows version are you using ?

pahan35 commented 6 years ago

I use Windows 10 Pro x64

ahmader commented 6 years ago

@pahan35 Can you please try v0.0.29 for this too ?

pahan35 commented 6 years ago

@ahmader thanks. After update to last branch state all works correctly image

ahmader commented 6 years ago

@pahan35 glad to hear that.

Are you planing to create the unit tests for this function ?

pahan35 commented 6 years ago

@ahmader actually at the beginning I think only about running integration test but I can add some unit tests based on the lead-module-test.js. Actually I need to add some new test which will check possibility of loading file via attachmentUrl.

pahan35 commented 6 years ago

I add basic test but I need to learn how to add new feature test.

Actually I get familiar with Javascript unit test only a week.

And my first written lines on CoffeeScript is on this branch. So I will try to add new check of attachment url in input

P.S: I've started write on Node.js using this package. It was first package which I install via npm. I used it to pass test task :)

pahan35 commented 6 years ago

@ahmader I done it. Will push it in a few minutes :)

pahan35 commented 6 years ago

I think we can create something like CrmFileModule from which we will extend modules which can upload files: Leads, Account, Contacts (maybe something else, I not very familiar with Zoho API).

By this action we can resolve this issue #7 : maybe particularly, maybe totally.

ahmader commented 6 years ago

I am reviewing Zoho API docs and as per the method uploadFile:

Files can be attached to records in all modules except Reports, Dashboards and Forecasts.

So I am thinking why to not have uploadFile implemented directly in class CrmModule.

This mean we will have it for all modules and would be easier to maintain.

P.S: I guess this is my only CoffeeScript project (Maybe one day we move it to ES6). Thanks to @interlock who started this project and now we are the maintainers 👨‍💻

And welcome to Nodejs and Npm it is a wonderful world 😄

pahan35 commented 6 years ago

@ahmader I'm glad to help you improve this amazing package.

Node.js is really wonderful thing. At first time, more then 1.5 years ago, I write simple fetching records to file based on your module. Actually there is repo.

During this period I work on Node.js above 1-3 months. But now I receive new project, which I do on Sails, and I suggest I can contribute to this module during implementing CRM features which required to project but missing now.

So I move this method to CrmModule and test method to this component test.

pahan35 commented 6 years ago

I have moved method successfully but when I tried to move tests I got some problem.

image

I will try manage with this problem tomorrow cause now it's too late.

pahan35 commented 6 years ago

I solved the problem. I just made extra tab before describe 'uploadFile', -> and it caused that problem

pahan35 commented 6 years ago

@ahmader I don't know how this file really can help.

I can fix it by soft wrap to my root changes commit and commit changes without this file.

pahan35 commented 6 years ago

I rebuild my commit. Please check it

ahmader commented 6 years ago

This branch still have node-shrinkwrap.json

ahmader commented 6 years ago

@pahan35 remember to pull from master, as I have updated some stuff too.

pahan35 commented 6 years ago

@ahmader ok. Usually I do this and start implement new features from latest master commit

ahmader commented 6 years ago

@pahan35 I can migrate now ?

pahan35 commented 6 years ago

@ahmader yes. There are only uploadFile changes merged with master branch

ahmader commented 6 years ago

@pahan35 Please update the README.md to add next to uploadFile and we are ready to go.

pahan35 commented 6 years ago

@ahmader done.