raincatcher-file enhancements - #15
Conversation
col1985
commented
Mar 14, 2017
col1985
commented
- Added fileSystem module for reading and writing files to local fileSystem
- Added ftp queue CRUD module for managing file transfer items
nialldonnellyfh
left a comment
nialldonnellyfh
left a comment
There was a problem hiding this comment.
Generally looks good. Added some comments.
- Needs some unit tests for the queue etc.
- Also, can you add some comments to the new functions in the file
@wtrocki When you get a chance, can you also take a look as this will need to move to your refactor. Would be good to get your input.
|
|
||
| function _requestFileSystem() { | ||
| var d = q.defer(); | ||
| navigator.webkitPersistentStorage.requestQuota(1024 * 1024, function(grantedBytes) { |
There was a problem hiding this comment.
This will work fine in the browser, but not on mobile device.
This file (https://github.com/feedhenry/fh-js-sdk/blob/master/src/appforms/src/core/020-utils01fileSystem.js#L377) might prove useful for switching between available file systems.
There was a problem hiding this comment.
thanks I will make the adjustment
There was a problem hiding this comment.
Best to use local storage adapter which would switch to be best adapter available on the device. Example from our JS SDK: https://github.com/feedhenry/fh-js-sdk/tree/master/libs/lawnchair
| fileSystem.readFile = function(name) { | ||
| var deferredRead = q.defer(); | ||
|
|
||
| if (fileSystem.ready) { |
There was a problem hiding this comment.
There is no else here. The promise will never resolve.
| } | ||
|
|
||
| client.initPromise = q.all([deferredFhinit.promise, deferredReady.promise]); | ||
| var deferredInitQueues = q.defer(); |
There was a problem hiding this comment.
Should there be one for the config.queue.downloadsLabel also?
| var deferredWrite = q.defer(); | ||
| var fileData = JSON.stringify(data); | ||
|
|
||
| if (fileSystem.ready) { |
There was a problem hiding this comment.
There is no else here, the promise will never resolve.
You might consider adding a promise to the fileSystem.init function to return a promise instead of setting a ready flag.
In that case, anything using that promise would immediately reject with a useful error message.
There was a problem hiding this comment.
Makes sense, il make the change
| } | ||
| }; | ||
|
|
||
| client.processQueue = function() { |
There was a problem hiding this comment.
Where is this function called from? It it called from the application consuming the module?
There was a problem hiding this comment.
Yes, the intention is to call this function from the app.js or where ever the developer would prefer. There is also an expectation the developer will handle offline and online events, saving and processing the queue as needed.
I will update README detailing this. What do you think?
| q = require('q'), | ||
| _ = require('lodash'); | ||
|
|
||
| // meta data item model schema |
There was a problem hiding this comment.
Are all of these fields required for the file upload?
There was a problem hiding this comment.
@nialldonnellyfh do you mean proposed model? This was just an example., however I have considered adding validations on required fields.
| var msg = ''; | ||
|
|
||
| switch (e.code) { | ||
| case FileError.QUOTA_EXCEEDED_ERR: |
There was a problem hiding this comment.
This would crash on modern webview. See: brianleroux/lawnchair#229
|
PR looks ok, but module is under heavy refactoring.. I'm going to bring this changes into new component once it's finished. I would also adapt changes to make them more generic etc. |
|
@nialldonnellyfh Going to create ticket to make sure that this work would be tracked in jira. |
|
@col1985 Created https://issues.jboss.org/browse/RAINCATCH-636 |
|
Thanks, @wtrocki I will update the ticket shortly. I am currently writing some unit tests for the Queue and fileSystem modules. @wtrocki based off @nialldonnellyfh feedback earlier, I have added downloadFile and retryDownloadFile functions to file.js. Is this required or will you cover this in your refactor? |
|
@col1985 I would take care of the integration part. Let's make sure that all requirements are in jira. |
|
@nialldonnellyfh @wtrocki Would someone be ale to review PR today? |
|
@col1985 I did reviewed this, but there is no point to integrate changes as I would need manually adapt changes to new structure and verify them. Leave that with us. |
|
@wtrocki perfect thanks |
|
Closing PR as I have made a number of changes since I plan to resubmit cleaner PR at a later date. |
3 participants
