Skip to content

Conversation

@mdwRepository
Copy link

The pull request addresses "Enhance EPerson and Group management" #44

It includes functions based on the API documentation: https://github.com/DSpace/RestContract/blob/main/epersons.md and https://github.com/DSpace/RestContract/blob/main/epersongroups.md.

A sample example_eperson_group.py is included showing the usage of these functions.

@kshepherd
Copy link
Collaborator

hi @mdwRepository , sorry your pull requests have been sitting neglected for so long - I am now trying to get more time for reviewing and discussing!

All of these methods look really great to have, my only initial concern is whether we need to think about some more systematic / structured way of organising and naming the methods... right now if I wanted to know how to do something, I would need to search the code for the method name based on keywords.. there is no intuitive way to guess or derive "how to do X".

As a (totally made up) brainstorm, something like a more generic search function wrapper that accepts an object type enum and some other params, so you can do something like search(type=EPERSON, by=email, value='abc@abc.com')? So we can use the same kind of thing for GROUP, COMMUNITY, or so on as well?
Or am I perhaps overthinking how much we can 'flatten' or abstract for these?

In any case I wouldn't want to block this necessarily - we can always merge it first and then talk about restructing for a later major release, but I hope we can avoid eventually ending up with 500 functions that are all named in different ways with no easy way for a script developer to learn usage beyond just scrolling through the pydoc or manaully searching...

@kshepherd kshepherd self-requested a review March 27, 2025 17:47
Copy link
Collaborator

@kshepherd kshepherd left a comment

Choose a reason for hiding this comment

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

@mdwRepository do you mind if I try refactor this a bit to consolidate the search / get operations down and set the "by" values as parameters instead of separate functions? I could either push here, or open as a separate PR for us to compare / discuss.

Would that be a difficult change to merge into your client implementations?

response_json = parse_json(response=response)
groups = []

if "_embedded" in response_json and "groups" in response_json["_embedded"]:
Copy link

Choose a reason for hiding this comment

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

Testing against v8 (no longer have a v7 to test against), the response will have "subgroups" instead of "groups"

groups = []

if "_embedded" in response_json and "groups" in response_json["_embedded"]:
for group_data in response_json["_embedded"]["groups"]:
Copy link

Choose a reason for hiding this comment

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

(here as well)

Copy link

@dheles dheles left a comment

Choose a reason for hiding this comment

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

Aside from comments on subgroups, testing the example worked for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants