Add prefs changetimeout (issue #99) #122
Conversation
|
@prasadtalasila Sir I haven't written the tests for this yet, however I was able to run it properly and it seemed to work fine (for now)...The cli-prefs.json file is also getting properly updated |
|
Also I've used 'timeouttype' instead of 'type' because type is already being used to denote the server type |
|
@mukkachaitanya please review this PR. |
There was a problem hiding this comment.
The PR looks great. However, there are some minor changes and comment. Please have a look at them @rrgodhorus and sir, @prasadtalasila.
lib/controller/validate/prefs.js
Outdated
| const { type, time } = changeTimeoutEvent.details; | ||
| const supportedTimeoutTypes = Object.keys(defaultPrefs.timeouts); | ||
| if (!supportedTimeoutTypes.includes(type)) { | ||
| console.log('DEbug'); |
There was a problem hiding this comment.
Please remove this debug log.
lib/controller/validate/prefs.js
Outdated
| @@ -79,6 +79,26 @@ const validateLogger = (changeLoggerEvent) => { | |||
| return changeLoggerEvent; | |||
| }; | |||
|
|
|||
| const validateChangeTimeout = (changeTimeoutEvent) => { | |||
| const { type, time } = changeTimeoutEvent.details; | |||
| const supportedTimeoutTypes = Object.keys(defaultPrefs.timeouts); | |||
There was a problem hiding this comment.
Please use lodash 's function .keys() to have uniformity across the project for such functionality.
lib/controller/validate/prefs.js
Outdated
| }, | ||
| }; | ||
| } | ||
| if (!time || !validator.isInt(time) || !(parseInt(time, 10) > 0)) { |
There was a problem hiding this comment.
A good check for the timeout to be positive 👍
There was a problem hiding this comment.
Also maybe you can consider using the options field of the validator. So the whole statement reduces to
if(!time || !validator.isInt(time, { gt: '0' }){
...
}| 'cliPrefs', | ||
| { | ||
| timeouts: { | ||
| session: +time, |
There was a problem hiding this comment.
Okay,I'll change it to parseInt() then ?
There was a problem hiding this comment.
I used + because it is used in line 65, maxSize: +details.maxSize
| // const SECONDS = 60; | ||
| // const MINUTES = 60; | ||
| // const HOURS = 2; | ||
|
|
There was a problem hiding this comment.
These comments can be removed.
| @@ -10,6 +10,7 @@ const eventForShowPref = (changePrefEvent) => { | |||
| logger_dir: cliPrefs.logger.logDirectory, | |||
| logger_location: cliPrefs.logger.logLocation, | |||
| logger_blacklist: cliPrefs.logger.blacklist, | |||
| timeouts_session: cliPrefs.timeouts.session, | |||
There was a problem hiding this comment.
We might need to decouple the model and output for prefs. What do you think, sir @prasadtalasila?
Well, it's not of concern with regard to this PR, can go in another one later.
lib/controller/prefs.js
Outdated
| @@ -27,6 +27,8 @@ const addTo = (program) => { | |||
| .option('--lang <lang>', 'Change language') | |||
| .option('--maxsize <size>', 'Change logger file maxsize') | |||
| .option('--blacklist <keyword>', 'Add keyword to logger blacklist') | |||
| .option('--timeouttype <type>', 'Timeout type ( session ) ') | |||
There was a problem hiding this comment.
An option of the form --timeout-type would be preferred. Caporal does support such options, see here
Codecov Report
@@ Coverage Diff @@
## dev #122 +/- ##
==========================================
- Coverage 99.28% 91.95% -7.33%
==========================================
Files 24 24
Lines 695 746 +51
==========================================
- Hits 690 686 -4
- Misses 5 60 +55
Continue to review full report at Codecov.
|
Requirements
Description of the Change
Added 'changetimeout' pref which currently accepts session as timeout type and time in seconds.
Benefits
Session timeout is no longer a hard-coded value
Possible Drawbacks
Applicable Issues