Fix delete and edit for Groups

[artlowel]

References

• Fixes Delete group is unfinished #893
• Fixes Edit group not implemented #911

Description

This PR implements the delete and edit features for groups that were incomplete up until now.

Instructions for Reviewers

You can test this PR by authenticating as an admin and trying to delete groups from both the list view and the edit group page.

Verify that the deletes work, that the groups aren't shown in the lists anymore afterwards and that you no longer get notifications saying the feature isn't implemented.

Also verify that you can edit the group's name and description and persist those changes

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR doesn't introduce circular dependencies
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[LucaGiamminonni]

Hi @artlowel , logging in as admin I cannot delete the groups: in the list view the buttons are not visible and in the edit page the button is disabled. Do I need to configure something?

[atarix83]

thanks @artlowel for this PR,

I've just had some test using rest demo server, here some issue that i've found :

• as noted by @LucaGiamminonni not all goups have the delete button. Since the authorizations endpoint return at least one result, the button should be visible. Is our assumption correct?
  

• When I try to edit title of a group, both a success and an error notifications are shown. However the edit seems to work properly
  

• finally something that does not strictly concern this PR, but could be related. Indeed editing epersons metadata doesn't work as well. Maybe we can make a follow-up PR for this problem

[artlowel]

as noted by @LucaGiamminonni not all goups have the delete button. Since the authorizations endpoint return at least one result, the button should be visible. Is our assumption correct?

@MarieVerdonck noticed that the authorization system currently claims the deleting of persistent groups is allowed. But it isn't. Since we can check for that using the persisent property on the group, we hide the delete buttons both for groups that are persistent and for groups for which you aren't authorized.

When I try to edit title of a group, both a success and an error notifications are shown. However the edit seems to work properly

That's an issue, we'll take a look

finally something that does not strictly concern this PR, but could be related. Indeed editing epersons metadata doesn't work as well. Maybe we can make a follow-up PR for this problem

Indeed, that should be a separate ticket and a separate PR.

[MarieVerdonck]

I recreated the issue where both success and nameInUse error message was shown. NameInUse was meant to check if there was a group found with the same name as the one being attempted, but it just used a search on name => inUse if totalElements of result > 0, but the search isn’t on exact name.
Example: So when you rename a group to Test, it will succeed if Test name is not already in use, but the nameInUse check showed error because for example Test 2 group existed, and got returned with search ‘Test’.


It would be slow to check for all the groups in the search result if their names is an Exact match to the one we are attempting, since could be great number of results, and there is not an endpoint for get group by exact name… (Implemented a similar check with eperson with email but the search there with scope email is exact I believe)

A patch request with a group name that is already in use results in a 500 Internal Server error response with a stack trace (javax.persistence.PersistenceException: org.hibernate.exception.ConstraintViolationException: could not execute batch), which can’t really be used to form a meaningful message as to Why your edit request failed.

The check has been removed for now since as it is now it is not guaranteed to be correctly shown, and the only way for it to be correctly shown would be to check each search result group for an exact name match…

[atarix83]

@MarieVerdonck
thanks for fixing the issue, I've tested it again and it's resolved

[tdonohue]

@artlowel and @MarieVerdonck : Overall, the code here looks good & it works for general, manually created Groups.

However the delete functionality does not work for COLLECTION_* or COMMUNITY_* groups. When I attempt to delete one of those groups, I get a 422 Unprocessable or Invalid Entity exception, even though I can successfully delete groups which are not associated with a Collection or Community.

See the note on 422 exceptions here: https://github.com/DSpace/Rest7Contract/blob/main/epersongroups.md#deleting-an-eperson-group

These types of Groups must be deleted via the /communities or /collections endpoint, e.g.

• https://github.com/DSpace/Rest7Contract/blob/main/communities.md#delete-the-community-administrators-group
• https://github.com/DSpace/Rest7Contract/blob/main/collections.md#delete-the-collection-administrators-group

[artlowel]

@tdonohue I asked @benbosman about why this split happened, and he said it's been that way since DSpace 6, and was done in order to make it harder to accidentally delete those groups using the rest api. If that's the case, then the UI hiding that added complexity for the user is likely not wanted, otherwise might as well simply allow it on the groups endpoint.

You can already delete these groups from the assign roles tab on edit collection and community pages

I noticed that editing them the same on the groups endpoint fails as well. I tried to PATCH one of them on the collection endpoint using postman, and I got a 405: method not allowed. So this likely means the edit form shouldn't be shown for these groups either. Adding and removing members does work though.

So perhaps the best thing we can do is: if a group has an object link, don't show the delete button in the list view or detail page. Disable the edit form on the detail page and add a message that explains this group can't be modified because it corresponds to a community or collection role, with a link to the "assign roles" tab for the linked object.

I can ask Marie to do that for this PR, but you should know that the scope has expanded multiple times since my original estimate of 4h. We already have 7 hours logged on this task. Doing this will likely add another one or two.

[tdonohue]

@artlowel : Your suggested approach sounds fine. I'll update the estimates (which currently total 6hrs as this PR resolves two tickets...however, I'll update them to total 9hrs by adding a few hours to the delete task).

One minor tweak to this part of your suggested approach:

Disable the edit form on the detail page and add a message that explains this group can't be modified because it corresponds to a community or collection role, with a link to the "assign roles" tab for the linked object.

If possible, this same sort of message should be shown with regards to deleting the group. So, I agree we shouldn't show the delete button (or show a disabled button), and it'd be nice if we provided a message describing why it cannot be deleted as normal & link the user to the "assign roles" tab where that group can be deleted.

[tdonohue]

@artlowel and @MarieVerdonck : I've re-tested this today. It's working much better overall, but I'm now hitting some odd 404 errors (which never appear in the UI, but are visible from Chrome DevTools). Since they never appear in the UI, the behavior of the UI is unaffected....so, these are minor overall, but they appear to be unnecessary requests to the REST API.

Here's how to reproduce what I'm setting (I'm running yarn start):

• First, open Chrome Dev Tools
• Click the "+ Add group" button. This triggers a 404 response from this path http://localhost:8080/server/api/eperson/groups/newGroup (which doesn't exist in the REST API, as the "/newGroup" path is a UI-level path). Everything still works though, you can create a new group
• From the list of Groups (/access-control/groups) click the Delete button. This triggers three 404 Reponses, all related to the group you just deleted, as that group's /object, /epersons and /subgroups paths are all requested from the REST API and they now return 404s.
  • It's worth noting these 404s do NOT get triggered if you click the delete button from the edit group page.

Everything else honestly works great here. And, technically, these 404 responses do not change the behavior of the UI. However, they seem unnecessary (almost as if something is making unnecessary requests to the REST API), so I thought it best to report them.

For now, I'm submitting this as a "comment" just cause I want your feedback on this issue. Is it something we can fix easily? If so, let's do it. If it's something more complex, we could merge this as-is & simply log a separate ticket to fix the underlying issue.

[artlowel]

The reason for the newGroup call is that the component is used for both editing existing groups and creating new groups. It tries to get the id for the current group from the route, and does a GET to the backend with that id. So it needs to take in to account the newGroup case

The reason for the object, epersons and subgroups calls is that the group list template contains methods that fetch those endpoints. When a group is deleted the alternative links to those objects are also removed from the index. For a split second the template is still showing the freshly deleted group though, so it tries to re-retrieve those links. It's bad for performance in any case to put non-trivial methods in a template, it's definitely a mistake to put methods that perform backend calls in a template as they may be executed many times a second. The only reason that doesn't happen all the time right now, is because of the cache, but we're still generating tons of needless request objects that never get sent because the result is already cached. The solution here is to move the members and subgroup counts to the DTO, so the template has that data available without method calls.

Both of these issues are present in the main branch already, so I'd be in favor of solving them in a separate PR.
Fixing them would take an estimated 4h

[tdonohue]

Per @artlowel 's suggestion (above), I've created a separate ticket for the 404 errors. See #962

Therefore, merging this as-is. The 404 errors will be fixed separately.