-
Notifications
You must be signed in to change notification settings - Fork 49
Added documentation for eperson patch operations. #29
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
Conversation
|
|
||
| For example, starting with the following eperson field data: | ||
| ```json | ||
| "password": "oldpassword", |
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.
just to note that this is not a real scenario as we will be unable to know the password of existent user
| ``` | ||
| the replace operation `[{ "op": "replace", "path": "/password", "value": "newpassword"]` will result in : | ||
| ```json | ||
| "password": "newpassword", |
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.
I'm not sure to like that... I know that this is what happen right now as we have the password in-memory but we should eventually consider the need to reload the entity after the update to be able to serialize information automatically updated for some reason (think about a last_modified or other generated value). In this later case we will be unable to provide the password after the update....
I'm not sure about what to suggest here... maybe we can add a custom serialized that will always expose the password as ***** or we can exclude the password from the returned object (I tend to prefer this solution)
|
@abollini, I added the note as you suggested. |
abollini
left a comment
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.
Thanks!
|
@mspalti : it looks like we have a minor merge conflict in this PR. Could you resolve this, so we can merge? |
No description provided.