Security object #58
Security object #58
Conversation
AnthonySendra
commented
Feb 9, 2016
AnthonySendra
commented
- CRUD User
- CRUD Profile
- CRUD Role
…ject Conflicts: src/kuzzle.js
…ascript into kuz-343-security-object
…/sdk-javascript into kuz-343-security-object
…ascript into kuz-343-security-object
…/sdk-javascript into kuz-343-security-object
…ject Conflicts: dist/kuzzle.min.js dist/kuzzle.min.map
Current coverage is
|
…ject Conflicts: dist/kuzzle.min.js dist/kuzzle.min.map
|
+1111 |
src/security/kuzzleUser.js
Outdated
| data = this.serialize(), | ||
| self = this; | ||
|
|
||
| self.kuzzle.query(this.kuzzleSecurity.buildQueryArgs('createOrReplaceUser'), data, null, function (error) { |
There was a problem hiding this comment.
nitpicking: For all calls to kuzzle.query, shouldn't we include the optional options in the query?
There was a problem hiding this comment.
No you're right... But I think it's more clear to call the function with null instead. It works better with my IDE :)
But if you are convinced, I can change it. What do you think?
There was a problem hiding this comment.
To avoid any warning raised by your IDE, skipping the options argument altogether should do the trick: I made this argument optional in the JSDoc of the query method.
But another approach would be to add an optional options argument to this method (and maybe to other methods too). Because by default, nearly all methods should support the queuable option, asking to (not) queue this query during offline mode.
The only exception to this rule I can think of should be the login and logout functions (and all other functions not sending any requests, like addListener, obviously).
src/kuzzle.js
Outdated
|
|
||
| /* | ||
| /** | ||
| * Create an attribute security that embed all methods for manage Role, Profile and User |
|
Apart from the |
|
👍 👍 👍 👍 👍 👍 👍 👍 |
|
I'm the third validation, should I merge or would you like to discuss about the |
|
@AnthonySendra and I discussed the matter, and he agreed to add the New commits should be soon submitted |
|
@scottinet could you have a new look at this PR please ? |
|
lgtm! |
6 participants
