[DS-4036] Delete EPersons even if they are referenced #2928

benbosman · 2020-08-13T12:25:24Z

References

Replaces REST PR
Fixes JIRA ticket
Fixes DS-4036: Delete EPersons in DSpace #2803

Description

This PR:

Includes [DS-4036] Delete EPersons even if they are referenced #2229
It's updated with the latest master
The broken tests have been fixed
Tests for the workflow behavior have been created

This PR doesn't include tests for the submission yet. We'd first want to verify whether the approach taken here is approved.

This PR is focussed on denying creation of an inconsistent state of your repository/items. This implies:

If there's a workflow group with items assigned and only one member => removing that member from the group is not allowed. This is because the item would otherwise remain assigned to the workflow group but no-one can claim it
If there's a workflow group with items assigned and multiple members => removing a member from the group is no problem
If there's a workflow group without items and only one member => removing that member from the group is no problem (new items will skip this step)
If the user has claimed a task, the task will be returned to the pool when deleting the user. This should not pose any issues because the pool will contain other users which are not deleted

Instructions for Reviewers

There's a large amount of scenarios in EPersonInWorkflowTest.java. Please read them to see how this has been approached.
The general principle is to avoid an inconsistent state. If deleting the user would bring the system to an inconsistent state, that will need to be solved first, and the delete is refused.

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 & integration tests). Exceptions may be made if previously agreed upon.
My PR passes Checkstyle validation based on the Code Style Guide.
My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

DSpace references EPersons in different database tables like the submitter of an item or like the EPerson that gets special rights granted in the resourcepolicy table. This PR changes DSpace so it can handle references that are set null instead of referencing an actual EPerson. This is important to be able to delete EPersons which is demanded by several data protection laws like GDPR in the European Union.

…erson

tdonohue

@benbosman : I gave this an early review today, per request of @lieven-atmire in today's meeting.

I added a number of minor code cleanup notes...nothing major. The logic here mostly makes sense, but I'd like us to think more about the deletion related to Workflow Role Groups (see this comment of mine).

I'm worried we are potentially making it harder on DSpace Administrators to delete an EPerson if they are a member of many Workflow Role Groups. But, it's also possible I'm misunderstanding the code here. If possible, I'd like us to consider if there's a way to make deletion of EPersons slightly easier (to avoid forcing Admins to remove them one by one from a potentially large number of groups).

Overall, I'm not against the approach you've taken here, but I'd like to understand why you took this approach as it seems slightly unexpected to me at a first glance.

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemHelpdeskStrategy.java

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemMetadataStrategy.java

tdonohue · 2020-08-20T19:19:49Z

dspace-api/src/main/java/org/dspace/authorize/dao/ResourcePolicyDAO.java

@@ -66,6 +68,8 @@

    public void deleteByDsoEPersonPolicies(Context context, DSpaceObject dso, EPerson ePerson) throws SQLException;

+    public void deleteAllEPersonPolicies(Context context, EPerson ePerson) throws SQLException;


probably should be renamed deleteByEPerson() to match the other deleteBy* methods in this same DAO. We should also take this opportunity to add Javadocs here & to the new findByEPerson() method above.

This feedback hasn't been addressed yet

This is another change from #2229. We can modify it

dspace-api/src/main/java/org/dspace/eperson/EPersonCLITool.java

dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java

dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java

dspace-api/src/main/java/org/dspace/workflowbasic/BasicWorkflowServiceImpl.java

tdonohue · 2020-08-20T20:08:21Z

dspace-api/src/test/java/org/dspace/eperson/EPersonInWorkflowTest.java

+     * - verify the delete is refused
+     * - remove user B from step 1
+     * - delete user B
+     * - verify the delete succeeds


@benbosman : The scenario above doesn't make total sense to me. I don't see why deletion of user B must fail until they are removed from both approval groups? Why wouldn't a deletion just remove them from those approval groups (and let the approval proceed with other users in those groups)?

Does this have to do with whether a EPerson is the only member of a workflow role Group?

Simply put, I think we need to think about this more:

If an EPerson is only one (of many) member of a workflow role Group, then it seems like deleting them should just remove them from the group. I.e. no error should occur.

If an EPerson is the only member of a workflow role Group, then we have two options:

We delete both the EPerson & the Workflow Role Group (i.e. you are deleting that workflow step by deleting the only member of it). We could warn an Admin here if needed by saying: "By deleting this person, you will also remove these workflow steps of which they are the only member: "

We refuse to delete the EPerson until they are removed from each of the Workflow Role Groups. This is easier for us, but potentially more work for the DSpace Admin, who may have to go in and edit/delete the EPerson from a number of Groups one by one.

I now realize (after you added more details on the new "rules" here to the PR description) that my above comment is a misunderstanding of this test code. I had previously (wrongly) assumed this test was verifying that "no workflow group can be empty".

But, now I see that this test is likely verifying that the user cannot be removed if they are the only member of a Workflow Group that has items assigned.

I think this PR would be much easier to review if these tests all documented the scenario they are trying to tests. So, while the list of checks is useful here, it'd be nice to summarize them at the top by saying something like "This test verifies that an EPerson cannot be removed if they are the only member of a Workflow Group that has tasks currently assigned to it."

So, please disregard my prior request/comment..the only action here is to add a summary comment at the top of each test to summarize the scenario you are testing for.

Most of this prior feedback can be ignored, except as noted, it'd be useful to update the comments above these tests to simply summarize the scenario that each test is attempting to verify. The detailed list of steps is sometimes confusing to follow. So, I think this test could say something like: This test verifies that an EPerson cannot be removed if they are the only member of a Workflow Group that has tasks currently assigned to it.

The same is true for all tests in this class...it's sometimes hard to follow exactly why they are performing the steps they are performing. So, a simple summary sentence would help describe the scenario much more clearly.

AFAIK this has already been done as well

@benbosman : I'm not seeing the requested changes to this EPersonInWorkflowTest class methods.

I'd still appreciate simple summary notes at the beginning of each of these tests. It's very hard to follow these tests & the detailed "This test will perform the following checks:" does not always make it clear which scenario is being tested for.

As I noted previously, I think this test method can be easily summarized as "This test verifies that an EPerson cannot be removed if they are the only member of a Workflow Group that has tasks currently assigned to it." However, I'm not sure that's accurate, as it also seems like it might be testing that a user with claimed tasks cannot be deleted?

I think these tests would be a lot easier to understand if the JavaDocs just had a brief summary of the scenarios tested, and you moved the step-by-step summary into inline comments. Otherwise, I'm stuck reading a line and then having to scroll up to the JavaDocs to see which step it might be referencing, then reading the next line, and scroll back up, etc.

It's also extremely difficult determining the differences between testDeleteUserWhenOnlyUserInGroup1, testDeleteUserWhenOnlyUserInGroup2, ``testDeleteUserWhenOnlyUserInGroup3`...all the way to 7. Again, I think it'd be easier to understand if the JavaDocs had a simple summary of what makes this tests unique, and you move more of the details to inline (to describe what each line is doing).

I realize this would take work, but without these changes, this test class is very hard to review. I've taken 30mins to just check the first method looks correct (it does). But, I don't know how it differs from all the rest yet. (And, at a glance many of these test methods look like they run the same check over and over again...for instance, each one keeps checking that User B cannot be deleted if they are the last member of the first step group... it seems like we only need to make that check once)

tdonohue · 2020-08-25T16:59:30Z

@benbosman : Thanks for the updates to the PR Description to describe the "rules" you've put in place around deleting EPersons...specifically:

If there's a workflow group with items assigned and only one member => removing that member from the group is not allowed. This is because the item would otherwise remain assigned to the workflow group but no-one can claim it

If there's a workflow group with items assigned and multiple members => removing a member from the group is no problem

If there's a workflow group without items and only one member => removing that member from the group is no problem (new items will skip this step)

If the user has claimed a task, the task will be returned to the pool when deleting the user. This should not pose any issues because the pool will contain other users which are not deleted

I now realize that my prior feedback was slightly misguided, as I had misunderstood one of the rules... I had previously thought that "no workflow group can be empty" was a rule you were attempting to achieve (and that was the major concern I had).

Therefore, I generally approve of the direction of this draft PR. I've updated my prior inline comments where I was misunderstanding the code previously to clarify what would help this PR along. I think all the inline requests are now relatively minor. Mostly it'd be improved by more inline comments & clearer error messages. Thanks!

72774: Delete eperson: workspace

…erson

tdonohue

@benbosman : I was going to review this today, but I realized that my prior (mostly minor) feedback from Aug 20 hasn't been addressed yet. So, I've added inline comments here again as a reminder of which feedback still needs addressing.

In the meantime, I'll try and test this out with the (new) corresponding Angular PR in the coming days, so that I can also provide feedback on how it's working.

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemHelpdeskStrategy.java

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemMetadataStrategy.java

tdonohue · 2020-09-23T21:51:26Z

dspace-api/src/main/java/org/dspace/authorize/dao/ResourcePolicyDAO.java

@@ -66,6 +68,8 @@

    public void deleteByDsoEPersonPolicies(Context context, DSpaceObject dso, EPerson ePerson) throws SQLException;

+    public void deleteAllEPersonPolicies(Context context, EPerson ePerson) throws SQLException;


This feedback hasn't been addressed yet

tdonohue · 2020-09-23T21:51:39Z

dspace-api/src/main/java/org/dspace/eperson/EPersonCLITool.java

-        } catch (IOException ex) {
+            List<String> tableList = ePersonService.getDeleteConstraints(context, eperson);
+            if (!tableList.isEmpty()) {
+                System.out.printf("This EPerson with ID: %s is referring to this tables:%n ",


This feedback hasn't been addressed yet

dspace-api/src/main/java/org/dspace/eperson/EPersonCLITool.java

tdonohue · 2020-09-23T21:52:13Z

dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java

+        } catch (AuthorizeException ex) {
+            log.error("This AuthorizeException: " + ex + " occured while deleting Eperson with the ID: " +
+                      ePerson.getID());
+            throw new AuthorizeException(ex.getMessage());


This feedback hasn't been addressed yet

dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java

dspace-api/src/main/java/org/dspace/workflowbasic/BasicWorkflowServiceImpl.java

tdonohue · 2020-09-23T21:55:36Z

dspace-api/src/test/java/org/dspace/eperson/EPersonInWorkflowTest.java

+     * - verify the delete is refused
+     * - remove user B from step 1
+     * - delete user B
+     * - verify the delete succeeds


Most of this prior feedback can be ignored, except as noted, it'd be useful to update the comments above these tests to simply summarize the scenario that each test is attempting to verify. The detailed list of steps is sometimes confusing to follow. So, I think this test could say something like: This test verifies that an EPerson cannot be removed if they are the only member of a Workflow Group that has tasks currently assigned to it.

The same is true for all tests in this class...it's sometimes hard to follow exactly why they are performing the steps they are performing. So, a simple summary sentence would help describe the scenario much more clearly.

benbosman · 2020-09-24T09:07:29Z

@tdonohue I noticed that only the changes specific to our customizations were processed, but not the ones from #2229
We don't know why some of the changes were made in that PR either, but we'll try to adjust it where possible. I did add some questions inline for those as well

tdonohue · 2020-09-24T13:30:54Z

@benbosman : Thanks for the clarification. In general, I think we do need to clean up the original code from #2229 in this PR as well.... I fully realize your team didn't write that code. But as you are helping steward this work along, I'd appreciate it if we can still cleanup (where reasonable) code that was originally written in #2229 which doesn't align well with our current DSpace 7 best practices. So, I'd definitely appreciate it if your team could help clean up the code throughout this PR....that said, we can decide to make exceptions around minor things (like the changes to the now obsolete Workflow framework, as that will be removed soon anyhow).

…erson

lgtm-com · 2020-10-01T10:26:44Z

This pull request fixes 1 alert when merging d5e28e8 into 8d57d71 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

tdonohue

@benbosman : I gave this another code review today. Much of my feedback has been addressed. However, I'm still finding the tests in EPersonInWorkflowTest to be very "dense" and difficult to understand. They are very repetitive and don't describe well which different scenarios they are attempting to test for. I've added more comments/questions inline to explain further.

I will also note that this EPersonInWorkflowTest takes a really long time to run (for a Unit Test). Your Travis build shows it taking 21.396 seconds, while most every other unit test completes in about 5 seconds or less. I'm not sure what to suggest here, but I'd like to understand if we need the repetitive tests in this class, as I'm worried they are just adding unnecessary time to the build/testing process.

I will note that I lightly tested this with the Angular PR and it looks like the basics are working. So, I think this mostly just needs some test cleanup to simplify things (if possible) and better describe exactly what each test is doing differently

tdonohue · 2020-10-01T21:45:56Z

dspace-api/src/test/java/org/dspace/eperson/EPersonInWorkflowTest.java

+     * @throws Exception
+     */
+    @Test
+    public void testDeleteUserWhenOnlyUserInGroup2() throws Exception {


How does this differ from testDeleteUserWhenOnlyUserInGroup1()? I'm not seeing clear differences in why we need a second test here & the description doesn't describe any differences...both methods are described identically as: This test verifies that an EPerson cannot be removed if they are the only member of a Workflow Group that has tasks currently assigned to it. This test also verifies that after user has been removed from the workflow group and the task has been passed, the EPerson can be removed.

It's very hard to tell the differences between these methods, and (to me) it looks like we are running some of the same tests as in testDeleteUserWhenOnlyUserInGroup1(). Duplicative tests are not really necessary, as they just take up more time (and will cause this overall Test class to take longer to complete/pass). It's possible I'm misunderstanding the purpose of these test methods...but the purpose is just not clear to me as a reviewer, so I'm finding these test methods very difficult to review.

This same comment applies to all the other testDeleteUserWhenOnlyUserInGroup() tests below (numbered 2-7). They all have had the exact same summary added to them, & the code is similar but not exactly the same. So, I'd like to understand why they are all needed. Are there different scenarios here that you are trying to test are not causing a different behavior? If so, we need to describe those different scenarios better in the comments, so that it's easier for reviewers to understand what they are attempting to test for.

tdonohue · 2020-10-01T21:58:25Z

dspace-api/src/test/java/org/dspace/eperson/EPersonInWorkflowTest.java

+     * @throws Exception
+     */
+    @Test
+    public void testDeleteUserAfterReplacingUser2() throws Exception {


The same questions above apply to these methods. I'm finding it hard to determine the differences between testDeleteUserAfterReplacingUser2() and testDeleteUserAfterReplacingUser1(). I can see the descriptions are identical & the setup is identical. The steps seem like they are very similar but in a slightly different order. And, why does this test need to verify that the Item can be archived after approvals occur? It seems like it that's beyond the scope of testing whether User B can be deleted after another user is added?

So, again, this is confusing to me to review as I'm not understanding what this test is attempting to test for that is different from the prior test. It also seems like these tests are doing more than they need to, as I don't think they need to complete the entire workflow process (unless I'm misunderstanding the goals of these tests)

This same comment applies to the other tests named testDeleteUserAfterReplacingUser*() as they are all quite similar in nature.

benbosman · 2020-10-02T16:00:39Z

Hi @tdonohue ,
The tests do verify only slightly different scenarios, e.g. deleting a user when there's a task in their pool, or when the task still has to move to the pool, …
The reason why we created so many tests was because so many things were broken with the initial implementation we had to start from. We did discover a huge amount of bugs, and did want to make sure that any solved bugs are not re-introduced.
It's possible to disable some of the tests to reduce the test duration, but the trade-off is that there's a risk bugs will be re-introduced in later development. AFAIK, there are no duplicate tests. There's just a lot of different scenarios possible.

If you do want to have some tests disabled due to the duration in Travis, perhaps it would be possible to:

set them to disabled, but with a message not to touch them
create a separate branch at e.g. https://github.com/DSpace/DSpace/tree/additional-tests which re-enables them
automatically update https://github.com/DSpace/DSpace/tree/additional-tests with the main branch after each main update, so they're still executed afterwards (and we're sure they won't break)

tdonohue · 2020-10-02T16:09:32Z

@benbosman : I think you've misunderstood my prior requests. I'm not asking to disable tests or even to remove some of the tests. But, I would like it if tests described their goal(s) better. Currently, this PR has been hard for me to review as the test methods all look extremely similar to me. So, I'm unable to determine why you've chosen to test the same idea (e.g. blocking delete of a EPerson who is the only member of a review group) from several different "angles" or "approaches". The differences between the tests are unclear to me, so I don't know what bugs you are trying to prove no longer exist.

I've also found that existing tests seem to do a lot more work then they need to. For instance, after proving that you can only delete an EPerson after removing them from the review group, why does every test need to then continue the workflow step-by-step until the Item is archived? Continuing to test the workflow itself seems unrelated to the EPerson deletion codebase -- unless I'm misunderstanding the goals of these tests.

If, after the tests are better documented & we've removed unnecessary code from the tests, these tests still take a while to run, I'm OK with that. However, currently, as they look duplicative to me & they are taking a long time to run, I have to ask these questions to understand what I'm missing.

mwoodiupui · 2020-10-02T17:05:58Z

dspace-api/src/test/java/org/dspace/eperson/EPersonInWorkflowTest.java

+/**
+ * Class to test interaction between EPerson deletion and tasks present in the workflow
+ */
+public class EPersonInWorkflowTest extends AbstractIntegrationTestWithDatabase {


If this class is an AbstractIntegrationTestWithDatabase then it is not a unit test and should be named EPersonInWorkflowIT.

Being an IT would help to explain why it runs so long. Actually, as our ITs go, its execution is rather brief.

@mwoodiupui : good catch! I somehow overlooked that.

I agree with @mwoodiupui here, @benbosman ....this class is misnamed. Since it's obviously an IT, it must be named EPersonInWorkflowIT.java in order to be run as an IT. Just a reminder, classes with "Test" in them are run as Unit Tests, while classes with "IT" in them are run as Integration Tests.

lgtm-com · 2020-10-05T08:26:44Z

This pull request fixes 1 alert when merging d6046dc into 737d3e6 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

lgtm-com · 2020-10-05T09:11:18Z

This pull request fixes 1 alert when merging 01cc856 into 737d3e6 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

tdonohue

@benbosman : As noted via Slack, the new IT code comments are looking good to me. So, if the other methods can be updated, I'd appreciate it.

I also gave this another test today with DSpace/dspace-angular#874, and I stumbled on one (seemingly minor) bug. If I delete a submitter while they have an item in Workflow, the deletion succeeds, but the item in Workflow is no longer accessible from the UI (it throws a NullPointerException as the submitter is now null).

Here's the full error stacktrace that I see in the dspace.log:

2020-10-05 16:38:57,968 ERROR org.dspace.app.rest.exception.DSpaceApiExceptionControllerAdvice @ An exception has occurred (status:500)
java.lang.NullPointerException: null
        at org.dspace.app.rest.converter.ConverterService.toRest(ConverterService.java:110) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.AInprogressItemConverter.fillFromModel(AInprogressItemConverter.java:70) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.WorkflowItemConverter.convert(WorkflowItemConverter.java:36) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.WorkflowItemConverter.convert(WorkflowItemConverter.java:24) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.ConverterService.toRest(ConverterService.java:111) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.PoolTaskConverter.convert(PoolTaskConverter.java:38) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.PoolTaskConverter.convert(PoolTaskConverter.java:24) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.DiscoverResultConverter.convertDSpaceObject(DiscoverResultConverter.java:98) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.DiscoverResultConverter.addSearchResults(DiscoverResultConverter.java:78) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.converter.DiscoverResultConverter.convert(DiscoverResultConverter.java:57) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.repository.DiscoveryRestRepository.getSearchObjects(DiscoveryRestRepository.java:115) ~[classes/:7.0-beta4-SNAPSHOT]
        at org.dspace.app.rest.DiscoveryRestController.getSearchObjects(DiscoveryRestController.java:153) ~[classes/:7.0-beta4-SNAPSHOT]

I think the error is here: https://github.com/DSpace/DSpace/blob/main/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/AInprogressItemConverter.java#L70 It probably should be checking that the submitter is not null before using it.

Here's how to reproduce it:

Create a new EPerson (as needed) who is an Administrator
Find a Collection that has workflow approval enabled. Submit to it.
Complete the submission...it goes into workflow approval
Delete the EPerson (it will succeed)
Now, login as a user who can approve/reject the submission created. The Workflow item list in that person's MyDSpace will "hang" indefinitely...in the dspace.log you'll see the above NullPointerException.

Overall, I'll admit this PR looks good to me. This is the only issue I've found. I've successfully done the following:

I can delete EPerson who submitted past Items. Those Items are kept & still viewable in the UI
I cannot delete an EPerson who is the last/only member of a Workflow group. However, I get the proper error. I can delete them if I delete the group first, or add another person to that group.

lgtm-com · 2020-10-06T15:58:46Z

This pull request fixes 1 alert when merging 4103b01 into 737d3e6 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

tdonohue

👍 Overall, this is looking good now, so I'll give it my +1. Thanks for the IT comments cleanup @benbosman and @YanaDePauw -- it really helped to clarify to me what each test is doing differently.

There are just two outstanding issues I'm aware of...one it'd be nice to clean up in this PR, and the other could wait for a follow-up PR:

As noted inline below, there's one test that has an @Ignore. I think this @Ignore can be removed and the test corrected (as I noted below). This should be very quick to fix & likely could be done in this PR. If for some reason you disagree, let me know or add a "TODO" comment to that test to describe why it's currently Ignored.
The minor NullPointerException that I mentioned previously could be done in a follow-up PR. I think this PR as-is is a great improvement, so I'd like to avoid holding it up further. If you agree, I'll simply log a bug ticket for the NPE & we can reschedule that follow-up fix for beta 5.

Thanks again!

tdonohue · 2020-10-06T16:43:28Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/eperson/DeleteEPersonSubmitterIT.java

+
+    @Ignore
+    @Test
+    public void testWorkflowItemSubmitterDelete() throws Exception {


Minor thing. This test seems to have an @Ignore. Is there a reason for that? If so, we should add a TODO comment here.

Also, I think this test may be incorrect. In EPersonTest.testCascadingDeleteSubmitterPreservesWorkflowItems(), the test proves that a WorkflowItem is kept even if the Submitter is deleted. This test seems to be trying to prove the opposite as the final line in this method is trying to prove that the WorkflowItem isNotFound() after the Submitter is deleted. I suspect that last line should be changed to isOk(), as the WorkflowItem should still exist even if the Submitter is deleted.

I've logged the above Ignored test as part of #2994. It's related to the minor bug that I described there.

lgtm-com · 2020-10-07T08:26:03Z

This pull request fixes 1 alert when merging 9ae4239 into 737d3e6 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

benbosman · 2020-10-07T08:30:21Z

Thanks @tdonohue
The test you linked to was disabled due to an exception. This, combined with the NullPointerException were risks with the approach picked here. They can indeed be logged in a ticket to ensure a test is created to prove the problem, solve the problem, and a test is created to prove the solution

tdonohue · 2020-10-07T13:57:20Z

@benbosman : thanks for the response. I completely overlooked the fact that this ignore test was related to the small bug I previously reported.

I've logged both in a new ticket #2994, which I've scheduled for beta 5. Therefore, I'm OK with this PR as-is. The bug I found & ignored test can both be cleaned up in beta 5.

abollini

thanks @benbosman for the PR, I made a code review and the code looks good to me with test to proof it works. I have also made some minimal manual testing so I'm +1 with merge it eventually also as is. There are few inline comments about minor cleanup

abollini · 2020-10-04T18:15:07Z

dspace-api/src/main/java/org/dspace/authorize/dao/ResourcePolicyDAO.java

@@ -33,6 +33,8 @@
    public List<ResourcePolicy> findByDsoAndType(Context context, DSpaceObject dSpaceObject, String type)
        throws SQLException;

+    public List<ResourcePolicy> findByEPerson(Context context, EPerson ePerson) throws SQLException;


nothing serious, basic javadoc is missing

abollini · 2020-10-08T06:11:23Z

dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java

+     * referencing object - e.g. WorkspaceItem, ResourcePolicy - or by setting
+     * the foreign key null - e.g. archived Items). If cascade is set to false
+     * and the EPerson is referenced somewhere, this leads to an
+     * AuthorizeException. EPersons may be referenced by Items, ResourcePolicies


this seems to be not fully accurate. In the code below there are scenario where the EPersonDeletionException is thrown directly (btw it should be also added to the list of throws in the javadoc) and in another case it is wrapped inside an IllegalArgumentException. Can we always return it directly? any reason to wrap it in another exception?

dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java

tdonohue · 2020-10-08T17:06:39Z

Merging this as-is since it's at +2. I also created a separate ticket around cleaning up the Basic Workflow code (see #2995) to ensure that isn't forgotten (as both @abollini and I noticed that in comments above).

Thanks @benbosman for helping move this effort along (and a thank you to @pnbecker whose prior work was pulled into this PR)

marsa and others added 5 commits November 30, 2018 11:43

[DS-4036] Resolving changes requested enduring review.

a3d37c3

[DS-4036] Resolving merge conflicts.

3890c11

Merge remote-tracking branch 'upstream/main' into w2p-72472_Delete-ep…

435235a

…erson

Delete eperson when in workflow

705b2b5

benbosman added this to Needs Reviewers Assigned in DSpace 7 Beta 4 via automation Aug 13, 2020

benbosman added this to the 7.0beta4 milestone Aug 13, 2020

tdonohue self-requested a review August 20, 2020 14:16

heathergreerklein assigned benbosman Aug 20, 2020

heathergreerklein added the high priority label Aug 20, 2020

heathergreerklein moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Aug 20, 2020

tdonohue reviewed Aug 20, 2020

View reviewed changes

72750: Finalize Delete eperson: workflow

e25cea2

tdonohue moved this from Under Review to In Progress in DSpace 7 Beta 4 Aug 27, 2020

72774: Delete eperson: workspace

548e035

github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Sep 3, 2020

benbosman and others added 3 commits September 16, 2020 12:44

Merge pull request #32 from atmire/w2p-72774_Delete-eperson-workspace

81f58b4

72774: Delete eperson: workspace

Merge remote-tracking branch 'upstream/main' into w2p-72472_Delete-ep…

3977fe7

…erson

Fix to tests post merge

f71202c

benbosman marked this pull request as ready for review September 17, 2020 13:17

benbosman removed the merge conflict PR has a merge conflict that needs resolution label Sep 17, 2020

benbosman moved this from In Progress to Needs Reviewers Assigned in DSpace 7 Beta 4 Sep 17, 2020

tdonohue requested review from tdonohue and abollini September 17, 2020 14:14

tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Sep 17, 2020

tdonohue requested changes Sep 23, 2020

View reviewed changes

73422: Delete eperson: community feedback

3b563e4

YanaDePauw added 2 commits October 1, 2020 10:48

73783: Delete eperson: community feedback part 2

9b169af

Merge remote-tracking branch 'upstream/main' into w2p-72472_Delete-ep…

d5e28e8

…erson

benbosman requested a review from tdonohue October 1, 2020 10:42

tdonohue mentioned this pull request Oct 1, 2020

Delete eperson DSpace/dspace-angular#874

Merged

5 tasks

tdonohue requested changes Oct 1, 2020

View reviewed changes

mwoodiupui reviewed Oct 2, 2020

View reviewed changes

updated JavaDocs to explain the goal of the tests in more detail

d6046dc

checkstyle

01cc856

tdonohue requested changes Oct 5, 2020

View reviewed changes

73977: Delete eperson: community feedback part 3

4103b01

DSpace 7 Beta 4 automation moved this from Under Review to Reviewer Approved Oct 6, 2020

tdonohue approved these changes Oct 6, 2020

View reviewed changes

add TODO message

9ae4239

benbosman force-pushed the w2p-72472_Delete-eperson branch from 62fd8b0 to 9ae4239 Compare October 7, 2020 07:52

tdonohue mentioned this pull request Oct 7, 2020

NullPointerException occurs when accessing WorkflowItem after deleting the Submitter #2994

Closed

abollini approved these changes Oct 8, 2020

View reviewed changes

tdonohue merged commit d645ec3 into DSpace:main Oct 8, 2020

DSpace 7 Beta 4 automation moved this from Reviewer Approved to Done Oct 8, 2020

benbosman mentioned this pull request Nov 26, 2020

NullPointerException occurs when accessing WorkflowItem after deleting the Submitter #3067

Merged

6 tasks

tdonohue added the authentication: EPerson Related to how the EPerson object works during authentication label Mar 24, 2022

		@@ -66,6 +68,8 @@

		public void deleteByDsoEPersonPolicies(Context context, DSpaceObject dso, EPerson ePerson) throws SQLException;

		public void deleteAllEPersonPolicies(Context context, EPerson ePerson) throws SQLException;

[DS-4036] Delete EPersons even if they are referenced #2928

[DS-4036] Delete EPersons even if they are referenced #2928

Conversation

benbosman commented Aug 13, 2020 • edited Loading

References

Description

Instructions for Reviewers

Checklist

tdonohue left a comment • edited Loading

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 Aug 25, 2020 • edited Loading

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 commented Aug 25, 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

benbosman commented Sep 24, 2020

tdonohue commented Sep 24, 2020

lgtm-com bot commented Oct 1, 2020

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbosman commented Oct 2, 2020

tdonohue commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 5, 2020

lgtm-com bot commented Oct 5, 2020

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Oct 6, 2020

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

tdonohue Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 7, 2020

benbosman commented Oct 7, 2020

tdonohue commented Oct 7, 2020

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 Oct 8, 2020

benbosman commented Aug 13, 2020 •

edited

Loading

tdonohue left a comment •

edited

Loading

tdonohue Aug 25, 2020 •

edited

Loading

tdonohue left a comment •

edited

Loading

tdonohue left a comment •

edited

Loading

tdonohue left a comment •

edited

Loading

tdonohue Oct 6, 2020 •

edited

Loading