[WIP] #148 Create instruction of how to create dev/draft copy of our Dialog Flow App and add helpful scripts #278
Conversation
…also ready, having issue with axios for import script
| return zip.zipFolder({folderPath: folderPath, targetFolderPath: zipPath}) | ||
| .then(function (path) { | ||
| return Promise.resolve(base64Encode(path)); | ||
| }, function (err) { |
There was a problem hiding this comment.
it seems you don't need the whole section:
function (err) {
return Promise.reject(err);
}| function fetchAgentFromDF () { | ||
| return getAccessToken() | ||
| .then(accesstoken => { | ||
| return axios('https://dialogflow.googleapis.com/v2/projects/music-a88c1/agent:export?access_token=' + accesstoken, {method: `POST`}); |
There was a problem hiding this comment.
please put project id music-a88c1 and api url https://dialogflow.googleapis.com/v2/projects to config file
There was a problem hiding this comment.
and btw you can use template literals here
functions/package.json
Outdated
| "firebase-admin": "^5.12.0", | ||
| "firebase-functions": "^1.0.2", | ||
| "glob": "^7.1.2", | ||
| "hirestime": "^3.2.1", | ||
| "lodash": "^4.17.10", | ||
| "mathjs": "^4.1.2", | ||
| "mustache": "^2.3.0", | ||
| "n": "^2.1.10", |
functions/package.json
Outdated
| "node-fetch": "^2.1.2", | ||
| "node-zip": "^1.1.1", |
| .then(res => res.data) | ||
| .then(data => { | ||
| debug(data); | ||
| return prepareAgentToFetchFromDF(data.response.agentContent, '../../agent/', 'agent.zip'); |
There was a problem hiding this comment.
'../../agent/', 'agent.zip'
should be in config
| const mustache = require('mustache'); | ||
|
|
||
| function uploadAgent () { | ||
| return Promise.all([getAccessToken(), prepareAgentToPostToDF('../../agent', './')]) |
There was a problem hiding this comment.
'../../agent', './'
should be in config
| const config = require('../config'); | ||
| const mustache = require('mustache'); | ||
|
|
||
| function downloadAgent () { |
There was a problem hiding this comment.
please add jsdoc here, and every other function. Short description and @param and @returns.
| const {prepareAgentToPostToDF} = require('../utils/prepare-agent'); | ||
|
|
||
| const config = require('../config'); | ||
| const mustache = require('mustache'); |
There was a problem hiding this comment.
it is not written rule - but it is more like a recommendation to put all imports in alpha betting order and split them into sections - of local modules (../config) and third party libs (ex axios). I'm trying to follow this rule in every module - it helps to reduce imports duplications and make it more consistent.
| const fs = require('fs'); | ||
|
|
||
| // function to encode file data to base64 encoded string | ||
| function base64Encode (file) { |
There was a problem hiding this comment.
I think the name of function could confuse. Because data flow direction is not clear here -- "from file" or "to file", and why do we need file argument here
| function base64Encode (file) { | ||
| // read binary data | ||
| return new Promise(function (resolve, reject) { | ||
| fs.readFile(file, function (err, bitmap) { |
There was a problem hiding this comment.
Do you think bytes name could be better here?
| .then(function (path) { | ||
| var encodedBase64 = base64Encode(path); | ||
| debug(encodedBase64); | ||
| return Promise.resolve(encodedBase64); |
There was a problem hiding this comment.
Do you really need Promise.resolve here? base64Encode returns Promise, so it should work without safety wrapper Promise.resolve.
| return zip.zipFolder({folderPath: folderPath, targetFolderPath: zipPath}) | ||
| .then(function (path) { | ||
| var encodedBase64 = base64Encode(path); | ||
| debug(encodedBase64); |
There was a problem hiding this comment.
Do you really need to log Promise here? You would get the same Promise log entry here in any case.
| } | ||
|
|
||
| // function to create file from base64 encoded string | ||
| function prepareAgentToFetchFromDF (base64str, folderPath, zipPath) { |
There was a problem hiding this comment.
please add more details in jsdoc
| @@ -13,6 +13,7 @@ describe('uploader', () => { | |||
| it('should be defined', () => { | |||
| expect(accessToken.getAccessToken).to.be.ok; | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
it seems like this new line should be between describe calls.
| it('should return success', () => { | ||
| base64.base64Decode(base64Str, './tests/uploader/utils/fixtures/sample_agent.zip') | ||
| .then(value => { | ||
| expect(value).to.be.eql('successe'); |
| if (err) return reject(err); | ||
| else return resolve(Buffer.from(bitmap).toString('base64')); | ||
| }); | ||
| }) | ||
| .catch(function (err) { | ||
| error(err); | ||
| console.log(err); |
There was a problem hiding this comment.
why don't you use debug module here?
| @@ -22,12 +23,15 @@ function base64Decode (base64str, file) { | |||
| // write buffer to file | |||
| return new Promise(function (resolve, reject) { | |||
| fs.writeFile(file, bitmap, 'utf8', function (err) { | |||
| console.log(file); | |||
There was a problem hiding this comment.
it would be better to use debug module here
#148 Working on script