[add][s] add push method and fix tests - #11 #18
Conversation
lib/index.js
Outdated
| @@ -73,15 +75,20 @@ class Client { | |||
| return response.data | |||
| } | |||
|
|
|||
| put(actionType, data) { | |||
| return ActionApi.action(this.api, this.apiKey, actionType, data) | |||
| push(actionType, payload) { | |||
There was a problem hiding this comment.
According to #11 we should have only payload param. actionType should be hardcoded in the push() function. So basically you should pass create_package to action() function. @rufuspollock can confirm this
lib/index.js
Outdated
| put(actionType, data) { | ||
| return ActionApi.action(this.api, this.apiKey, actionType, data) | ||
| push(actionType, payload) { | ||
| const file = f11s.open(payload) |
There was a problem hiding this comment.
Also please add comments or some basic JSDoc comments, so the payload will be more clear what it should be. Is this anything that we pass datajs.open() function? Like it can be path, File or whatever we pass in data.js
lib/index.js
Outdated
| } | ||
|
|
||
| /** | ||
| * @param {File} file | ||
| * @param {string} token | ||
| */ | ||
| async push(file, token, onProgress) { | ||
| async pushBlob(file, token, onProgress) { |
There was a problem hiding this comment.
I think we shouldn't include token here. It can be retrieved inside this function. Something like const token = await this.ckanAuthz(). Like you did in the tests, but move it inside this function.
lib/index.js
Outdated
| ...file.descriptor, | ||
| resources: [], | ||
| }) | ||
| return this.action(actionType, dataset.descriptor) | ||
| } | ||
|
|
||
| /** | ||
| * @param {File} file |
There was a problem hiding this comment.
I think you should update this JSDoc. Also we talked about this, just want to confirm, file is a browser File? I remember we agreed to have file as datajs file, so all the funcitons from datajs will be available, but now I'm thinking maybe we should get anything like we get as in case of datajs.open() and convert it to datajs file inside pushBlob() function? In other words, the question is, should we expect from the users to pass datajs like file or anything which is convertible datajs so we will convert ourselves? @rufuspollock please let us know your thoughts
test/methods.test.js
Outdated
| const client = new Client( | ||
| config.authToken, | ||
| config.organizationId, | ||
| config.datasetId, | ||
| config.api | ||
| ) | ||
|
|
||
| const response = await client.put('package_create', { "name": "my_dataset", "owner_org": "myorg"}) | ||
| const response = await client.action('package_create', { "name": "my_dataset", "owner_org": "myorg"}) |
There was a problem hiding this comment.
In action.test.js there is already tests for that, I think we don't need this.
There was a problem hiding this comment.
@mariorodeghiero you haven't changed this in your last commit.
|
@mariorodeghiero did you read the signature of the method in the issue #11 - the naming of variables and method signature aren't accidental but specific. If you think we should add or change that's ok but have a good reason and explicitly flag that in the commits and PR summary. |
…ckan and call `package_update` api.
|
I've pushed some commits so now we have |
|
FYI @kmanaseryan is working on this. |
|
Creating a new PR |
Acceptance
Tasks
npm run testrun all files with*.test.js