Group and eperson management #41

benbosman · 2018-12-19T12:01:12Z

Related ticket: https://jira.lyrasis.org/browse/DS-4026

Created a Rest Contract for EPerson Groups (some of the API behavior already exists in the API)
Updated the Rest Contract for EPersons (some of the API behavior already exists in the API)
Created a Rest Contract for adding child Groups to parent groups (and deleting, querying)
Created a Rest Contract for adding EPersons to parent groups (and deleting, querying)

abollini

Thanks to provide the contract also for existing implementation this will help to reduce our documentation gap. Please review the contract for the mapping according to the practice of Spring Data Rest using the text-uri/list format and the association links (some need to be introduced as stated in the comment)

epersongroups.md

epersons.md

group2eperson.md

epersongroups.md

epersons.md

tdonohue

@benbosman : I gave this a review today. Sorry for the questions, but there are a few things I don't understand. If you and @abollini have previously discussed any of these points & come to a different decision, feel free to let me know. I'm not trying to slow down this PR, but I did come away from the code review with many questions.

epersongroups.md

epersons.md

epersongroups.md

epersons.md

cwilper · 2019-02-28T12:29:13Z

Note that metadata-as-map has since been merged, so examples given in this PR should be updated to use that new structure, e.g. what I did here: #53

# Conflicts: # epersons.md

get groups of eperson Updating metadata layout

Removing duplicate section

…ons until it's sorted out

tdonohue

@benbosman : Overall this looks good to me. I noted below that I think some of the subresources here could use better names... I'm not sure about "parentgroup" (but if others prefer this name, I can live with it).

Previously (a long time ago) I had also asked a question about whether all these endpoints should be under /api/core/epersons and /api/core/groups (as /api/eperson/epersons is a bit odd looking for an endpoint). As EPersons and Groups are related more to Authorization, we could also consider putting these at /api/authz/epersons and /api/authz/groups alongside the current /api/authz/resourcepolicies endpoint. All that said, I'm OK with leaving this as-is if others feel EPersons and Groups are better grouped under /api/eperson/epersons and /api/eperson/groups.

epersongroups.md

tdonohue · 2019-09-26T20:54:37Z

epersongroups.md

+**GET /api/eperson/groups/<:uuid>/parentgroups**
+
+Retrieves direct parent groups of the given group.
+Requesting indirect parent groups is not supported


As noted above, I wonder if calling this endpoint memberships would simplify the description here? This is a list of the other Groups that this Group is a direct member of.

As noted above, I suggest to remove this association endpoint and add a search method

/api/eperson/groups/search/hasMember?:uuid

that will retrun the list of groups that contains the specified group by uuid

epersons.md

abollini

See inline comments. The ones that are imho blocking are the ones related to parent groups and children groups in the Groups endpoint.

epersongroups.md

abollini · 2019-10-03T13:18:22Z

epersongroups.md

+**GET /api/eperson/groups/<:uuid>/parentgroups**
+
+Retrieves direct parent groups of the given group.
+Requesting indirect parent groups is not supported


As noted above, I suggest to remove this association endpoint and add a search method

/api/eperson/groups/search/hasMember?:uuid

that will retrun the list of groups that contains the specified group by uuid

epersongroups.md

epersons.md

epersongroups.md

…t this point in time

benbosman · 2020-02-03T16:53:05Z

I've updated this PR to prepare it for the beta 2 sprint implementation.
There are quite a few small fixes to the contract, and I've removed the parentgroups endpoint completely since it's not even implemented in the service, so most likely not relevant

tdonohue

@benbosman : Thanks for the updates here! Honestly, I think the overall contract looks good to me. I do have a number of (very minor) comments below...these are all more like tiny cleanup / renaming rather than any significant changes. Overall though, I think this contract is close to being done.

tdonohue · 2020-02-04T15:52:33Z

epersongroups.md

+* 201 Created - if the operation succeed
+* 401 Unauthorized - if you are not authenticated
+* 403 Forbidden - if you are not logged in with sufficient permissions
+* 422 Unprocessable Entity - if the name was omitted or already exists, if permanent was set to true


Minor thing, I don't think a POST can fail if permanent is set to true. A POST is creating a new Group, and a new Group cannot be a "permanent" one. So we can likely just remove the last statement on this line.

@tdonohue how I see this is:
permanent is a property on the group which is part of the JSON response. It's possible this is set to true during the POST (Angular should not be implemented to set it to true, but it can still be sent as such to REST from other applications). Since we should not allow creating permanent groups via REST, it should fail

@benbosman : Oh, I understand what you mean now. You are correct, but I previously misunderstood the statement without the additional context you just added. I'd suggest we simply reword this slightly and say:

422 Unprocessable Entity - if the name was omitted or already exists, *or* if permanent was set to true (permanent groups can only be created by the system).

epersongroups.md

tdonohue · 2020-02-04T16:10:01Z

epersongroups.md

+
+_Unsupported._ If you want detailed information about a single group, use the `/api/eperson/groups/<:uuid>` endpoint.
+
+**PUT /api/eperson/groups/<:parent_group_uuid>/subgroups**


This PUT section seems like it should be moved up alongside the POST that is similar. It's a bit oddly placed under a header called "Remove a Group from a parent group".

I've moved this to make it more clear which methods are supported and which not for changing the subgroups

tdonohue · 2020-02-04T16:11:00Z

epersongroups.md

+
+### Remove an EPerson from a parent Group
+
+**PUT /api/eperson/groups/<:groupuuid>/epersons**


Again, I think this PUT should be in the section above next to the POST

I've moved this to make it more clear which methods are supported and which not for changing the epeople

tdonohue · 2020-02-04T16:11:30Z

epersongroups.md

+
+**GET /api/eperson/groups/<:parent_group_uuid>/subgroups/<:sub_group_uuid>**
+
+_Unsupported._ If you want detailed information about a single group, use the `/api/eperson/groups/<:uuid>` endpoint.


We can probably remove this entire GET section as it's unsupported. I don't think it's necessary here.

I've removed this GET section

epersons.md

tdonohue

👍 @benbosman Thanks for the quick updates to this contract. Also your explanations to a few previous request make sense & I've resolved those conversations leaving the contract as-is. One final minor request is to enhance the wording of this 422 error description: https://github.com/DSpace/Rest7Contract/pull/41/files#r375304904

Beyond that, this looks good to me. I'm already casting my +1 vote as the final change I've requested is really minor in nature.

abollini

thanks @benbosman It looks good to me, I have made just few inline note about minor typos that I expect to be fixed quick

epersongroups.md

abollini · 2020-02-13T16:13:38Z

epersons.md

+
+This supports a basic search in the metadata.
+It will search in:
+* UUID (exact match)


if this is the already implemented behavior I'm fine to keep it, otherwise I would suggest to exclude UUID from the search as it is "by Metadata"

I did indeed base this on the current implementation to avoid having to create a different service implementation

tdonohue · 2020-02-13T16:37:40Z

Merging, as this is at +2

[CST-11043] submission notify panel Approved-by: Andrea Bollini

benbosman added 8 commits December 18, 2018 16:17

POST and minor additions

c0d86b8

Update epersons.md

e055c66

Update epersons.md

84c2cc1

Creating REST contract

29ad374

Create group2group.md

76ec4c8

Update group2group.md

3ddc2d5

Update group2group.md

076934b

Create group2eperson.md

72458fa

abollini requested changes Dec 31, 2018

View reviewed changes

epersongroups.md Outdated Show resolved Hide resolved

epersongroups.md Outdated Show resolved Hide resolved

epersongroups.md Outdated Show resolved Hide resolved

epersons.md Outdated Show resolved Hide resolved

group2eperson.md Outdated Show resolved Hide resolved

AlexanderS reviewed Jan 29, 2019

View reviewed changes

epersongroups.md Show resolved Hide resolved

AlexanderS reviewed Jan 29, 2019

View reviewed changes

epersons.md Outdated Show resolved Hide resolved

Feedback processing

c614a94

tdonohue reviewed Feb 8, 2019

View reviewed changes

epersongroups.md Show resolved Hide resolved

epersongroups.md Outdated Show resolved Hide resolved

epersongroups.md Outdated Show resolved Hide resolved

epersongroups.md Outdated Show resolved Hide resolved

epersons.md Show resolved Hide resolved

abollini requested changes Feb 10, 2019

View reviewed changes

Feedback processing

4757769

benbosman added 6 commits September 25, 2019 17:58

Merge branch 'master' into group-person

a3a4f4d

# Conflicts: # epersons.md

Missing search endpoints

a65a31d

get groups of eperson Updating metadata layout

HAL links

9b9925a

Removing duplicate section

HAL links

ef8b3e6

HAL links, parent groups, removing the feature to get indirect relati…

750a6ec

…ons until it's sorted out

json error

0dc014b

tdonohue reviewed Sep 26, 2019

View reviewed changes

removing duplicate section

fd35735

abollini requested changes Oct 3, 2019

View reviewed changes

benbosman added 5 commits February 3, 2020 17:03

Merge branch 'master' into group-person

f472b6c

Removing parent groups since there's also no Service implementation a…

fdce239

…t this point in time

fixed typing error in link

eeae209

POST status codes

15ddef9

POST status codes

38b6b84

benbosman added 2 commits February 3, 2020 17:30

explaining no PUT support

c0c8e4b

The group name is not in metadata (not dc.title)

e3743b8

benbosman requested a review from abollini February 4, 2020 13:14

tdonohue reviewed Feb 4, 2020

View reviewed changes

benbosman added 3 commits February 5, 2020 11:31

Explaining permanent property

bce04b6

Moving cyclic reference to 422

eb9368c

Moving the order of the endpoints

69ef989

tdonohue approved these changes Feb 5, 2020

View reviewed changes

abollini approved these changes Feb 13, 2020

View reviewed changes

wording updates

94a9252

tdonohue merged commit d487d80 into DSpace:master Feb 13, 2020

benbosman mentioned this pull request Feb 21, 2020

Manage groups and EPeople DSpace/DSpace#2686

Merged

benbosman deleted the group-person branch March 6, 2020 09:15

abollini pushed a commit to 4Science/Rest7Contract that referenced this pull request Nov 10, 2023

Merged in CST-11043 (pull request DSpace#41)

b089b2b

[CST-11043] submission notify panel Approved-by: Andrea Bollini

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group and eperson management #41

Group and eperson management #41

benbosman commented Dec 19, 2018 •

edited by tdonohue

abollini left a comment

tdonohue left a comment

cwilper commented Feb 28, 2019

tdonohue left a comment

tdonohue Sep 26, 2019

abollini Oct 3, 2019

abollini left a comment

abollini Oct 3, 2019

benbosman commented Feb 3, 2020

tdonohue left a comment

tdonohue Feb 4, 2020

benbosman Feb 5, 2020

tdonohue Feb 5, 2020

tdonohue Feb 4, 2020

benbosman Feb 5, 2020

tdonohue Feb 4, 2020

benbosman Feb 5, 2020

tdonohue Feb 4, 2020

benbosman Feb 5, 2020

tdonohue left a comment

abollini left a comment

abollini Feb 13, 2020

benbosman Feb 13, 2020

tdonohue commented Feb 13, 2020


		_Unsupported._ If you want detailed information about a single group, use the `/api/eperson/groups/<:uuid>` endpoint.

		PUT /api/eperson/groups/<:parent_group_uuid>/subgroups


		### Remove an EPerson from a parent Group

		PUT /api/eperson/groups/<:groupuuid>/epersons


		GET /api/eperson/groups/<:parent_group_uuid>/subgroups/<:sub_group_uuid>

		_Unsupported._ If you want detailed information about a single group, use the `/api/eperson/groups/<:uuid>` endpoint.

Group and eperson management #41

Group and eperson management #41

Conversation

benbosman commented Dec 19, 2018 • edited by tdonohue

abollini left a comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

cwilper commented Feb 28, 2019

tdonohue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abollini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbosman commented Feb 3, 2020

tdonohue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

abollini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue commented Feb 13, 2020

benbosman commented Dec 19, 2018 •

edited by tdonohue