Skip to content
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

[DS-4062] Endpoint for eperson profile patches #49

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@mspalti
Copy link
Contributor

mspalti commented Jan 30, 2019

This is related to DSpace/DSpace#2257, which gives logged in users the ability to update their eperson profile. That includes the ability to update metadata on the eperson DSO.

Both PR's appear to be in conflict with #46 and work on metadata patches by cwilper. If that approach is taken then I think this PR (and it's related PR) will become smaller in scope, since the only new feature provided is giving logged in users the ability to change their password.

@abollini
Copy link
Member

abollini left a comment

Hi @mspalti
thanks for the PR. I agree with you about the conflict with #46 as the later seems to me the right way to proceed I suggest to revisit this PR later dealing with only the password (and eventually canLogin / email / netid) attributes. Email / netid should be writable only by administrator

epersons.md Outdated
Note: The new password is currently returned after an update but this could be revisited later, see [#30]((https://github.com/DSpace/Rest7Contract/issues/30))

To replace the firstname and lastname metadata fields, `curl -X PATCH http://${dspace.url}/api/epersons/eperson/<:id-eperson> -H Content-Type: application/json" -d '[{ "op": "replace", "path": "metadata/eperson.firstname", "value": "newName"]`. The operation also requires an Authorization header.

This comment has been minimized.

Copy link
@abollini

abollini Feb 5, 2019

Member

The #46 is the right direction here, so we will need only to reference the general metadata patch documentation here

@mspalti mspalti force-pushed the mspalti:master branch from 5de59c1 to 9aeaa49 Feb 12, 2019

@@ -44,6 +44,7 @@ the replace operation `[{ "op": "replace", "path": "/netid", "value": "newNetId"
```json
"netid": "newNetId",
```
#### This operation can be performed by administrators and by the authenticated user.

This comment has been minimized.

Copy link
@abollini

abollini Feb 14, 2019

Member

This is correct but as we are adding this details for the password I would like to have a similar indication for the previous replace operations as well that should be all reserved only to the administrator.
If possible please add also an example to change the email

This comment has been minimized.

Copy link
@mspalti

mspalti Feb 15, 2019

Author Contributor

I added the email patch as an operation that can be performed by administrators and the authenticated user. I will change this if this operation needs to be restricted to administrators.

This comment has been minimized.

Copy link
@mspalti

mspalti Feb 15, 2019

Author Contributor

Updated contract to say the email patch operation is limited to administrators.

This comment has been minimized.

Copy link
@abollini

abollini Feb 15, 2019

Member

yes, this is better. The user is currently not allowed to change the email herself and introduce this feature could be more expensive than what it looks at a first glance as we should have a way to validate the email

@mspalti mspalti force-pushed the mspalti:master branch 2 times, most recently from 709cd2b to 084a18a Feb 15, 2019

@abollini
Copy link
Member

abollini left a comment

Thanks @mspalti it looks good to me now. We will wait for a second quick review/approval

@@ -44,6 +44,7 @@ the replace operation `[{ "op": "replace", "path": "/netid", "value": "newNetId"
```json
"netid": "newNetId",
```
#### This operation can be performed by administrators and by the authenticated user.

This comment has been minimized.

Copy link
@abollini

abollini Feb 15, 2019

Member

yes, this is better. The user is currently not allowed to change the email herself and introduce this feature could be more expensive than what it looks at a first glance as we should have a way to validate the email

@paulo-graca
Copy link

paulo-graca left a comment

Shouldn't the examples be updated to be using the endpoint?
/api/eperson/epersons

epersons.md Outdated
@@ -45,6 +46,19 @@ the replace operation `[{ "op": "replace", "path": "/netid", "value": "newNetId"
"netid": "newNetId",
```

To replace the email value, `curl -X PATCH http://${dspace.url}/api/epersons/eperson/<:id-eperson> -H "Content-Type: application/json" -d '[{ "op": "replace", "path": "/email", "value": "new@email"]'`. The operation also requires an Authorization header.

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 19, 2019

Shouldn't the examples be updated to be using the endpoint?
/api/eperson/epersons

epersons.md Outdated
@@ -12,6 +12,7 @@
### Replace
The replace operation allows to replace *existent* information with new one. Attempt to use the replace operation to set not yet initialized information must return an error. See [general errors on PATCH requests](patch.md)

#### These operations can be performed by administrators.

To replace the certificate required value, `curl -X PATCH http://${dspace.url}/api/epersons/eperson/<:id-eperson> -H "Content-Type: application/json" -d '[{ "op": "replace", "path": "/certificate", "value": "true|false"]'`. The operation also requires an Authorization header.

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 19, 2019

Shouldn't the examples be updated to be using the endpoint?
/api/eperson/epersons

epersons.md Outdated
```

#### This operation can be performed by administrators and by the authenticated user.

To replace the password value, `curl -X PATCH http://${dspace.url}/api/epersons/eperson/<:id-eperson> -H "Content-Type: application/json" -d '[{ "op": "replace", "path": "/password", "value": "newpassword"]'`. The operation also requires an Authorization header.

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 19, 2019

Shouldn't the examples be updated to be using the endpoint?
/api/eperson/epersons

This comment has been minimized.

Copy link
@mspalti

mspalti Feb 20, 2019

Author Contributor

Yes, you are correct! Thanks for noticing this. I will update.

Updated epersons.md
Removed the metadata operations from epersons.md

Added the email patch operation and a note for administrator privilege operations.

The authenticated user is not allowed to update their email address; changed the contract to indicate this.

Corrected the eperson endpoint in examples.

@mspalti mspalti force-pushed the mspalti:master branch from 084a18a to 6870f6a Feb 22, 2019

@paulo-graca
Copy link

paulo-graca left a comment

@mspalti Thank you for adding the fix.

@tdonohue
Copy link
Member

tdonohue left a comment

👍 Looks good to me too! Merging. Thanks @mspalti!

@tdonohue tdonohue merged commit 6f55da8 into DSpace:master Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.