diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommand.java index 94bcfa2f5b7..bee5dc648b9 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommand.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommand.java @@ -7,8 +7,10 @@ import edu.harvard.iq.dataverse.Dataset; import edu.harvard.iq.dataverse.DatasetLinkingDataverse; +import edu.harvard.iq.dataverse.DatasetLock; import edu.harvard.iq.dataverse.Dataverse; import edu.harvard.iq.dataverse.Guestbook; +import edu.harvard.iq.dataverse.authorization.DataverseRole; import edu.harvard.iq.dataverse.authorization.Permission; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; import edu.harvard.iq.dataverse.engine.command.AbstractVoidCommand; @@ -135,7 +137,14 @@ public void executeImpl(CommandContext ctxt) throws CommandException { } throw new UnforcedCommandException(errorString.toString(), this); } - + + // 6575 if dataset is submitted for review and the default contributor + // role includes dataset publish then remove the lock + + if (moved.isLockedFor(DatasetLock.Reason.InReview) + && destination.getDefaultContributorRole().permissions().contains(Permission.PublishDataset)) { + ctxt.datasets().removeDatasetLocks(moved, DatasetLock.Reason.InReview); + } // OK, move moved.setOwner(destination); diff --git a/src/test/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommandTest.java b/src/test/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommandTest.java index ed6112539ed..380a4bbcf18 100644 --- a/src/test/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommandTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommandTest.java @@ -6,7 +6,10 @@ package edu.harvard.iq.dataverse.engine.command.impl; import edu.harvard.iq.dataverse.Dataset; +import edu.harvard.iq.dataverse.DatasetLock; +import edu.harvard.iq.dataverse.DatasetServiceBean; import edu.harvard.iq.dataverse.Dataverse; +import edu.harvard.iq.dataverse.DataverseRoleServiceBean; import edu.harvard.iq.dataverse.DataverseServiceBean; import edu.harvard.iq.dataverse.DvObject; import edu.harvard.iq.dataverse.Guestbook; @@ -14,6 +17,8 @@ import edu.harvard.iq.dataverse.GuestbookServiceBean; import edu.harvard.iq.dataverse.MetadataBlock; import edu.harvard.iq.dataverse.PermissionServiceBean; +import edu.harvard.iq.dataverse.RoleAssignment; +import edu.harvard.iq.dataverse.authorization.DataverseRole; import edu.harvard.iq.dataverse.authorization.RoleAssignee; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; import edu.harvard.iq.dataverse.authorization.users.GuestUser; @@ -25,6 +30,8 @@ import edu.harvard.iq.dataverse.engine.command.exception.IllegalCommandException; import edu.harvard.iq.dataverse.engine.command.exception.PermissionException; import static edu.harvard.iq.dataverse.mocks.MocksFactory.makeAuthenticatedUser; +import static edu.harvard.iq.dataverse.mocks.MocksFactory.makeRole; +import static edu.harvard.iq.dataverse.mocks.MocksFactory.nextId; import edu.harvard.iq.dataverse.search.IndexServiceBean; import java.sql.Timestamp; import java.util.ArrayList; @@ -48,6 +55,9 @@ import jakarta.persistence.metamodel.Metamodel; import jakarta.servlet.http.HttpServletRequest; import jakarta.ws.rs.core.Context; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.*; import static org.junit.jupiter.api.Assertions.*; @@ -61,16 +71,17 @@ * @author skraffmi */ public class MoveDatasetCommandTest { - Dataset moved, movedResponses; - Dataverse root, childA, childB, grandchildAA, childDraft, grandchildBB; - DataverseEngine testEngine; - MetadataBlock blockA, blockB, blockC, blockD; - AuthenticatedUser auth, nobody; - Guestbook gbA, gbB, gbC; - GuestbookResponse gbResp; - @Context - protected HttpServletRequest httpRequest; - + + Dataset moved, movedResponses, movedPerms, movedSamePerms; + Dataverse root, childA, childB, grandchildAA, childDraft, grandchildBB, childEditor, sibEditor; + DataverseEngine testEngine; + MetadataBlock blockA, blockB, blockC, blockD; + AuthenticatedUser auth, nobody; + Guestbook gbA, gbB, gbC; + GuestbookResponse gbResp; + @Context + protected HttpServletRequest httpRequest; + @BeforeEach public void setUp() { @@ -79,43 +90,60 @@ public void setUp() { nobody = makeAuthenticatedUser("Nick", "Nobody"); nobody.setSuperuser(false); - - root = new Dataverse(); root.setName("root"); root.setId(1l); root.setPublicationDate(new Timestamp(new Date().getTime())); - + root.setDefaultContributorRole(roles.findBuiltinRoleByAlias(DataverseRole.CURATOR)); + childA = new Dataverse(); childA.setName("childA"); childA.setId(2l); childA.setPublicationDate(new Timestamp(new Date().getTime())); - + childB = new Dataverse(); childB.setName("childB"); childB.setId(3l); - childB.setPublicationDate(new Timestamp(new Date().getTime())); - + childB.setPublicationDate(new Timestamp(new Date().getTime())); + grandchildAA = new Dataverse(); grandchildAA.setName("grandchildAA"); grandchildAA.setId(4l); grandchildAA.setPublicationDate(new Timestamp(new Date().getTime())); - + childDraft = new Dataverse(); childDraft.setName("childDraft"); childDraft.setId(5l); - + grandchildBB = new Dataverse(); grandchildBB.setName("grandchildBB"); grandchildBB.setId(6l); grandchildBB.setPublicationDate(new Timestamp(new Date().getTime())); - + childEditor = new Dataverse(); + childEditor.setName("childEditor"); + childEditor.setId(7l); + childEditor.setDefaultContributorRole(roles.findBuiltinRoleByAlias(DataverseRole.EDITOR)); + + sibEditor = new Dataverse(); + sibEditor.setName("sibEditor"); + sibEditor.setId(8l); + sibEditor.setDefaultContributorRole(roles.findBuiltinRoleByAlias(DataverseRole.EDITOR)); + + movedPerms = new Dataset(); + movedPerms.setOwner(childEditor); + DatasetLock lock = new DatasetLock(DatasetLock.Reason.InReview, nobody, null); + movedPerms.addLock(lock); + + movedSamePerms = new Dataset(); + movedSamePerms.setOwner(childEditor); + movedSamePerms.addLock(lock); + moved = new Dataset(); moved.setOwner(root); moved.setPublicationDate(new Timestamp(new Date().getTime())); moved.setId(1l); - + movedResponses = new Dataset(); movedResponses.setOwner(root); movedResponses.setPublicationDate(new Timestamp(new Date().getTime())); @@ -126,39 +154,39 @@ public void setUp() { grandchildAA.setOwner(childA); grandchildBB.setOwner(childA); childDraft.setOwner(childA); - - gbA= new Guestbook(); + + gbA = new Guestbook(); gbA.setId(1l); - gbB= new Guestbook(); + gbB = new Guestbook(); gbB.setId(2l); - gbC= new Guestbook(); + gbC = new Guestbook(); gbC.setId(3l); - + moved.setGuestbook(gbA); movedResponses.setGuestbook(gbA); - - GuestbookResponse gbResp = new GuestbookResponse(); + + GuestbookResponse gbResp = new GuestbookResponse(); gbResp.setGuestbook(gbA); gbResp.setDataset(movedResponses); - + List includeA = new ArrayList(); includeA.add(gbA); includeA.add(gbB); - + grandchildAA.setGuestbooks(includeA); - + List notIncludeA = new ArrayList(); notIncludeA.add(gbC); notIncludeA.add(gbB); - + childB.setGuestbooks(notIncludeA); - - List none = new ArrayList(); + + List none = new ArrayList(); root.setGuestbooks(none); grandchildBB.setGuestbooks(none); grandchildBB.setGuestbookRoot(false); childA.setGuestbooks(includeA); - + testEngine = new TestDataverseEngine(new TestCommandContext() { @Override public DataverseServiceBean dataverses() { @@ -170,31 +198,46 @@ public Dataverse save(Dataverse dataverse) { } }; } - + + @Override + public DatasetServiceBean datasets() { + return new DatasetServiceBean() { + @Override + public void removeDatasetLocks(Dataset dataset, DatasetLock.Reason aReason) { + new HashSet<>(dataset.getLocks()).stream() + .filter(l -> l.getReason() == aReason) + .forEach(lock -> { + dataset.removeLock(lock); + }); + + } + }; + } + @Override public GuestbookServiceBean guestbooks() { return new GuestbookServiceBean() { @Override public Long findCountResponsesForGivenDataset(Long guestbookId, Long datasetId) { //We're going to fake a response for a dataset with responses - if(datasetId == 1){ + if (datasetId == 1) { return new Long(0); - } else{ + } else { return new Long(1); } } }; } - + @Override - public IndexServiceBean index(){ - return new IndexServiceBean(){ + public IndexServiceBean index() { + return new IndexServiceBean() { @Override - public void asyncIndexDataset(Dataset dataset, boolean doNormalSolrDocCleanUp){ + public void asyncIndexDataset(Dataset dataset, boolean doNormalSolrDocCleanUp) { } }; } - + @Override public EntityManager em() { return new MockEntityManager() { @@ -217,14 +260,55 @@ public boolean isUserAllowedOn(RoleAssignee roleAssignee, Command command, Dv } }; } - + }); } - - /** - * Moving ChildB to ChildA - * @throws Exception - should not throw an exception - */ + + DataverseRoleServiceBean roles = new DataverseRoleServiceBean() { + + List assignments = new LinkedList<>(); + + Map builtInRoles; + + { + builtInRoles = new HashMap<>(); + builtInRoles.put(DataverseRole.EDITOR, makeRole("default-editor", false)); + builtInRoles.put(DataverseRole.ADMIN, makeRole("default-admin")); + builtInRoles.put(DataverseRole.MANAGER, makeRole("default-manager")); + builtInRoles.put(DataverseRole.CURATOR, makeRole("curator")); + } + + @Override + public DataverseRole findBuiltinRoleByAlias(String alias) { + return builtInRoles.get(alias); + } + + @Override + public RoleAssignment save(RoleAssignment assignment) { + assignment.setId(nextId()); + assignments.add(assignment); + return assignment; + } + + @Override + public RoleAssignment save(RoleAssignment assignment, boolean index) { + return save(assignment); + } + + @Override + public List directRoleAssignments(DvObject dvo) { + // works since there's only one dataverse involved in the context + // of this unit test. + return assignments; + } + + }; + + /** + * Moving ChildB to ChildA + * + * @throws Exception - should not throw an exception + */ @Test public void testValidMove() throws Exception { @@ -234,11 +318,10 @@ public void testValidMove() throws Exception { assertEquals(childA, moved.getOwner()); } - + /** - * Moving grandchildAA - * Guestbook is not null because target includes it. - */ + * Moving grandchildAA Guestbook is not null because target includes it. + */ @Test public void testKeepGuestbook() throws Exception { @@ -248,12 +331,10 @@ public void testKeepGuestbook() throws Exception { assertNotNull(moved.getGuestbook()); } - - /** - * Moving to grandchildBB - * Guestbook is not null because target inherits it. - */ - + + /** + * Moving to grandchildBB Guestbook is not null because target inherits it. + */ @Test public void testKeepGuestbookInherit() throws Exception { @@ -263,39 +344,53 @@ public void testKeepGuestbookInherit() throws Exception { assertNotNull(moved.getGuestbook()); } - - + /** - * Moving to ChildB - * Guestbook is null because target does not include it - */ + * Moving to ChildB Guestbook is null because target does not include it + */ @Test public void testRemoveGuestbook() throws Exception { DataverseRequest aRequest = new DataverseRequest(auth, httpRequest); testEngine.submit(new MoveDatasetCommand(aRequest, moved, childB, true)); - assertNull( moved.getGuestbook()); + assertNull(moved.getGuestbook()); } - - - /** - * Moving DS to its owning DV - * @throws IllegalCommandException - */ + + @Test + public void testMoveToDifferentPerms() throws Exception { + DataverseRequest aRequest = new DataverseRequest(auth, httpRequest); + testEngine.submit(new MoveDatasetCommand(aRequest, movedPerms, root, true)); + assertTrue(movedPerms.getLocks().isEmpty()); + assertTrue(movedPerms.getOwner().equals(root)); + } + + @Test + public void testMoveToSamePerms() throws Exception { + DataverseRequest aRequest = new DataverseRequest(auth, httpRequest); + testEngine.submit(new MoveDatasetCommand(aRequest, movedSamePerms, sibEditor, true)); + assertTrue(movedSamePerms.getLocks().size() == 1); + assertTrue(movedSamePerms.getOwner().equals(sibEditor)); + } + + /** + * Moving DS to its owning DV + * + * @throws IllegalCommandException + */ @Test void testInvalidMove() { DataverseRequest aRequest = new DataverseRequest(auth, httpRequest); assertThrows(IllegalCommandException.class, - () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, root, false))); + () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, root, false))); } - + /** * Moving a dataset without having enough permission fails with * PermissionException. * * @throws java.lang.Exception - * + * * Ignoring after permissions change in 47fb045. Did that change make this * case untestable? Unclear. */ @@ -305,7 +400,7 @@ void testAuthenticatedUserWithNoRole() { DataverseRequest aRequest = new DataverseRequest(nobody, httpRequest); assertThrows(IllegalCommandException.class, - () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, childA, null))); + () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, childA, null))); } /** @@ -319,22 +414,22 @@ void testNotAuthenticatedUser() { DataverseRequest aRequest = new DataverseRequest(GuestUser.get(), httpRequest); assertThrows(PermissionException.class, - () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, root, null))); + () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, root, null))); } - - /** - * Moving published DS to unpublished DV - * @throws IllegalCommandException - */ + + /** + * Moving published DS to unpublished DV + * + * @throws IllegalCommandException + */ @Test void testInvalidMovePublishedToUnpublished() { DataverseRequest aRequest = new DataverseRequest(auth, httpRequest); assertThrows(IllegalCommandException.class, - () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, childDraft, null))); + () -> testEngine.submit(new MoveDatasetCommand(aRequest, moved, childDraft, null))); } - - - private static class EntityManagerImpl implements EntityManager { + + private static class EntityManagerImpl implements EntityManager { @Override public void persist(Object entity) { @@ -591,8 +686,8 @@ public List> getEntityGraphs(Class entityClass) { throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates. } - } - + } + private static class MockEntityManager extends EntityManagerImpl { @Override diff --git a/src/test/java/edu/harvard/iq/dataverse/mocks/MocksFactory.java b/src/test/java/edu/harvard/iq/dataverse/mocks/MocksFactory.java index 1b66ca56b23..9bda917a9bc 100644 --- a/src/test/java/edu/harvard/iq/dataverse/mocks/MocksFactory.java +++ b/src/test/java/edu/harvard/iq/dataverse/mocks/MocksFactory.java @@ -202,7 +202,11 @@ public static DatasetFieldType makeDatasetFieldType() { return retVal; } - public static DataverseRole makeRole( String name ) { + public static DataverseRole makeRole( String name ) { + return makeRole(name, true); + } + + public static DataverseRole makeRole( String name, Boolean includePublishDataset ) { DataverseRole dvr = new DataverseRole(); dvr.setId( nextId() ); @@ -212,7 +216,10 @@ public static DataverseRole makeRole( String name ) { dvr.addPermission(Permission.ManageDatasetPermissions); dvr.addPermission(Permission.EditDataset); - dvr.addPermission(Permission.PublishDataset); + if (includePublishDataset){ + dvr.addPermission(Permission.PublishDataset); + } + dvr.addPermission(Permission.ViewUnpublishedDataset); return dvr;