-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposing minors improvements and more test coverage #4
base: master
Are you sure you want to change the base?
Conversation
* Intro improved * No more TODOs
models/user.js
Outdated
@@ -32,6 +32,7 @@ module.exports = function User(ldapUserArray) { | |||
userEntry.mail.map(function(mail) { | |||
userModel.emails.push(mail); | |||
}); | |||
userModel.emails = [...new Set(userModel.emails)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
le spread operator (aka ...
) est supporte a partir de la version 5.x... Du coup la doc annonce version 4... (juste au cas ou une vielle image docker n'a pas ete mise a jour 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La doc dit: "⚠ this library "try" to use ES2015 (or ES6) capabilities, don't use it with nodejs under 4.x?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, la doc devrait dire under 5.x
😄
models/user.js
Outdated
@@ -32,6 +32,7 @@ module.exports = function User(ldapUserArray) { | |||
userEntry.mail.map(function(mail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note pour plus tard:
Je pense que c'est pas la bonne utilisation map, je que ca pourrait etre remplace par:
userEntry.mail.map(function(mail) { | |
userEntry.mail.forEach(function(mail) { |
ou mieux remplacer le tout par:
userModel.emails = [...new Set(userEntry.mail)];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suis aussi pour le spread operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spread, mais sur userEntry.mail
(et pas l'autre) en etant sur que userEntry existe...
Co-Authored-By: stefanonepa <stefano.nepa@outlook.com>
…p-js into test/fullcoverage
* Now one can query the ldap using user's email * Querying with phone will now handle "12345" and "+41 21 6912345"
The cache wasn't taking into consideration the param "isResultUniq", meaning that getUser and getUsers would return the same data, regardless of the isResultUniq param that should either return an Array or a single result.
Better tests and bugs fix
Close #3 * Get rid of cache callbacks * Use the same context for all the tests
[FIX] WARNING! node-cache legacy callback support will drop in v6.x
…dap-js into fix-ldap-connection-close
[RFA] epfl-idevelop → epfl-si
…dap-js into fix-ldap-connection-close
* clean up the test.js * improve the package.json script
Fix ldap connection close
Among other things, this PR:
unitsRepo.getUnitById