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.

Sign up for GitHub

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

Jump to bottom

Account profile management #2747

Merged
merged 6 commits into from
May 10, 2020
Merged

Account profile management #2747

merged 6 commits into from
May 10, 2020

Conversation

jonas-atmire
Copy link
Contributor

This PR is related to the rest contract update mentioned in: DSpace/RestContract#113

  • Admin users can edit both the non-metadata information (email, netid etc) as well as the metadata information of all users
  • Non admin user can edit their own metadata information
  • Non admin user can not edit their non-metadata information (email, netid etc)
  • Non admin users can not edit another person's information

tdonohue
tdonohue reviewed May 4, 2020
View reviewed changes
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonas-atmire : Overall, this PR looks fine to me. However, I didn't notice some minor things in the ITs, and I'm not sure I understand the changes to the EPersonRestRepository itself, and the newly imported util class seems unused? (maybe I'm overlooking something though?)

dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Show resolved Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Outdated

String token = getAuthToken(ePerson.getEmail(), password);

// updates password
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect comment. Say something like: "// should be allowed (as an Eperson can update their own metadata), and eperson.firstname should be replaced.

dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Outdated

String token = getAuthToken(ePerson2.getEmail(), password);

// updates password
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect comment. Should say something like // not allowed, as a non-Admin cannot modify other users

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java Outdated
@@ -44,6 +45,9 @@
@Autowired
AuthorizeService authorizeService;

@Autowired
DSpaceObjectMetadataPatchUtils metadataPatchUtils;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear why this is now @Autowired and imported into this class. I don't see any new code that uses it? Are these changes to EPersonRestRepository necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct @tdonohue, this should not be present in this part of the code (Seems to be a remnant of an older commit that has been overlooked)
I'll get these feedback-points looked at. Thanks for the review

@abollini abollini added the interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) label May 5, 2020
abollini
abollini reviewed May 5, 2020
View reviewed changes
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jonas-atmire
thanks for this PR, it looks mostly ok to me, I have added few inline comments to improve the ITs

...-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestPermissionEvaluatorPlugin.java Outdated Show resolved Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Show resolved Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Show resolved Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Show resolved Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java Show resolved Hide resolved
abollini
abollini reviewed May 7, 2020
View reviewed changes
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my feedback have been processed but unfortunately the latest commit have introduced a lot of unnecessary changes in the IT class due to formatting... I would like to have them reverted as otherwise they will cause conflict and more effort in coming PR about IT

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java Outdated
@@ -54,7 +54,7 @@ public EPersonRestRepository(EPersonService dsoService) {

@Override
protected EPersonRest createAndReturn(Context context)
throws AuthorizeException {
throws AuthorizeException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are only formatting change in this class, can you revert them please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree these seem like non-standard formatting changes. It'd be best to just revert all changes to EPersonRestRepository. Could that be done quickly @jonas-atmire or @MarieVerdonck ?

@MarieVerdonck MarieVerdonck force-pushed the w2p-70402_Manage-account-profile branch from 3edff6e to bfabe86 Compare May 8, 2020 08:13
@MarieVerdonck MarieVerdonck force-pushed the w2p-70402_Manage-account-profile branch 2 times, most recently from 1390070 to 0017f90 Compare May 8, 2020 08:25
@MarieVerdonck MarieVerdonck force-pushed the w2p-70402_Manage-account-profile branch from 0017f90 to 36914e1 Compare May 8, 2020 08:27
tdonohue
tdonohue approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me now. All feedback has been addressed. Thanks @jonas-atmire and @MarieVerdonck

abollini
abollini approved these changes May 10, 2020
View reviewed changes
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the cleanup, it looks better now!

@abollini abollini merged commit ea746e2 into DSpace:master May 10, 2020
@tdonohue tdonohue added this to Needs Reviewers Assigned in DSpace 7 Beta 3 via automation Jun 30, 2020
@tdonohue tdonohue moved this from Needs Reviewers Assigned to Done in DSpace 7 Beta 3 Jun 30, 2020
@tdonohue tdonohue added this to the 7.0beta3 milestone Jun 30, 2020
@benbosman benbosman deleted the w2p-70402_Manage-account-profile branch September 11, 2020 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdonohue tdonohue tdonohue approved these changes

@abollini abollini abollini approved these changes

Assignees
No one assigned
Labels
interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module)
Projects
No open projects
DSpace 7 Beta 3
  
Done
Milestone
7.0beta3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jonas-atmire @tdonohue @abollini @MarieVerdonck