diff --git a/application-changerequest-api/src/main/java/org/xwiki/contrib/changerequest/ChangeRequestConfiguration.java b/application-changerequest-api/src/main/java/org/xwiki/contrib/changerequest/ChangeRequestConfiguration.java index a5cf13e3..867d3123 100644 --- a/application-changerequest-api/src/main/java/org/xwiki/contrib/changerequest/ChangeRequestConfiguration.java +++ b/application-changerequest-api/src/main/java/org/xwiki/contrib/changerequest/ChangeRequestConfiguration.java @@ -223,4 +223,19 @@ default boolean acceptOnlyAllowedApprovers() { return false; } + + /** + * Define if the reference of the pages impacted by a change request should also be refactored when that change + * request already has the status {@link ChangeRequestStatus#MERGED}, in case those pages get renamed or moved. + * Note that this only updates the reference of the impacted pages stored in the change request: the content of + * the change request itself is never modified. + * + * @return {@code true} if merged change requests should also be refactored when one of their pages is renamed + * or moved. + * @since 1.24 + */ + default boolean isMergedChangeRequestRefactoringEnabled() + { + return false; + } } diff --git a/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/DefaultChangeRequestConfiguration.java b/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/DefaultChangeRequestConfiguration.java index 1e8bfb57..6c7dbb64 100644 --- a/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/DefaultChangeRequestConfiguration.java +++ b/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/DefaultChangeRequestConfiguration.java @@ -232,4 +232,10 @@ public boolean acceptOnlyAllowedApprovers() { return this.configurationSource.getProperty("acceptOnlyAllowedApprovers", false); } + + @Override + public boolean isMergedChangeRequestRefactoringEnabled() + { + return this.configurationSource.getProperty("refactorMergedChangeRequests", false); + } } diff --git a/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListener.java b/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListener.java index 5a1d18e9..b675b23a 100644 --- a/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListener.java +++ b/application-changerequest-default/src/main/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListener.java @@ -29,6 +29,7 @@ import org.slf4j.Logger; import org.xwiki.component.annotation.Component; import org.xwiki.contrib.changerequest.ChangeRequest; +import org.xwiki.contrib.changerequest.ChangeRequestConfiguration; import org.xwiki.contrib.changerequest.ChangeRequestException; import org.xwiki.contrib.changerequest.ChangeRequestStatus; import org.xwiki.contrib.changerequest.discussions.ChangeRequestDiscussionService; @@ -68,6 +69,9 @@ public class DocumentRenamedListener extends AbstractLocalEventListener @Inject private ChangeRequestDiscussionService changeRequestDiscussionService; + @Inject + private ChangeRequestConfiguration configuration; + @Inject private Logger logger; @@ -99,10 +103,12 @@ private void updateChangeRequests(DocumentReference source, DocumentReference ta try { changeRequests = this.storageManager.findChangeRequestTargeting(source); - changeRequests = - changeRequests.stream() - .filter(changeRequest -> changeRequest.getStatus() != ChangeRequestStatus.MERGED) - .collect(Collectors.toList()); + if (!this.configuration.isMergedChangeRequestRefactoringEnabled()) { + changeRequests = + changeRequests.stream() + .filter(changeRequest -> changeRequest.getStatus() != ChangeRequestStatus.MERGED) + .collect(Collectors.toList()); + } } catch (ChangeRequestException e) { this.logger.error("Failed to find change requests using document [{}].", source, e); } diff --git a/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListenerTest.java b/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListenerTest.java index 43674c4f..b17fed4a 100644 --- a/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListenerTest.java +++ b/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/listeners/DocumentRenamedListenerTest.java @@ -25,6 +25,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.xwiki.contrib.changerequest.ChangeRequest; +import org.xwiki.contrib.changerequest.ChangeRequestConfiguration; import org.xwiki.contrib.changerequest.ChangeRequestException; import org.xwiki.contrib.changerequest.ChangeRequestStatus; import org.xwiki.contrib.changerequest.discussions.ChangeRequestDiscussionService; @@ -76,6 +77,9 @@ class DocumentRenamedListenerTest @MockComponent private ChangeRequestDiscussionService changeRequestDiscussionService; + @MockComponent + private ChangeRequestConfiguration configuration; + @RegisterExtension private LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.INFO); @@ -139,6 +143,41 @@ void processLocalEventRefactorsMatchingChangeRequests() throws ChangeRequestExce assertEquals("Updating change request [openCR].", this.logCapture.getMessage(1)); } + @Test + void processLocalEventRefactorsMergedChangeRequestsWhenEnabled() throws ChangeRequestException + { + when(this.configuration.isMergedChangeRequestRefactoringEnabled()).thenReturn(true); + + MoveRequest moveRequest = mock(MoveRequest.class); + when(moveRequest.isUpdateLinks()).thenReturn(true); + when(moveRequest.isDeep()).thenReturn(true); + + DocumentReference source = new DocumentReference("wiki", "Space", "Source"); + DocumentReference target = new DocumentReference("wiki", "Space", "Target"); + DocumentRenamedEvent event = new DocumentRenamedEvent(source, target); + + ChangeRequest mergedChangeRequest = mock(ChangeRequest.class); + when(mergedChangeRequest.getStatus()).thenReturn(ChangeRequestStatus.MERGED); + when(mergedChangeRequest.getId()).thenReturn("mergedCR"); + + when(this.storageManager.findChangeRequestTargeting(source)).thenReturn(Collections.singletonList( + mergedChangeRequest)); + + this.listener.processLocalEvent(event, null, moveRequest); + + verify(this.storageManager).refactorTargetEntity(mergedChangeRequest, source, target, true); + verify(this.changeRequestDiscussionService) + .refactorDiscussionFileReference("mergedCR", source, target, true); + verify(this.observationManager).notify(any(ChangeRequestRefactoringEvent.class), eq("mergedCR")); + verify(this.observationManager).notify(any(ChangeRequestRefactoredEvent.class), eq("mergedCR")); + + verify(this.progressManager).pushLevelProgress(1, this.listener); + + assertEquals("Updating the change requests to refactor document [wiki:Space.Source] to " + + "[wiki:Space.Target].", this.logCapture.getMessage(0)); + assertEquals("Updating change request [mergedCR].", this.logCapture.getMessage(1)); + } + @Test void processLocalEventWhenStorageManagerFails() throws ChangeRequestException { diff --git a/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/storage/DefaultChangeRequestStorageManagerTest.java b/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/storage/DefaultChangeRequestStorageManagerTest.java index 88620623..d44848aa 100644 --- a/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/storage/DefaultChangeRequestStorageManagerTest.java +++ b/application-changerequest-default/src/test/java/org/xwiki/contrib/changerequest/internal/storage/DefaultChangeRequestStorageManagerTest.java @@ -27,6 +27,7 @@ import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -85,6 +86,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -235,6 +237,57 @@ void save() throws Exception verify(document).clone(); } + @Test + void refactorTargetEntityPreservesStatusOfMergedChangeRequest() throws Exception + { + DocumentReference source = new DocumentReference("wiki", "Space", "Source", Locale.ROOT); + DocumentReference target = new DocumentReference("wiki", "Space", "Target", Locale.ROOT); + + ChangeRequest changeRequest = new ChangeRequest() + .setId("mergedCR") + .setStatus(ChangeRequestStatus.MERGED) + .setCreationDate(new Date(12)); + + FileChange fileChange = mock(FileChange.class); + when(fileChange.getTargetEntity()).thenReturn(source); + when(fileChange.getType()).thenReturn(FileChange.FileChangeType.EDITION); + FileChange fileChangeClone = mock(FileChange.class); + when(fileChange.clone()).thenReturn(fileChangeClone); + when(fileChangeClone.getTargetEntity()).thenReturn(target); + changeRequest.addFileChange(fileChange); + + FileChange newFileChangeVersion = mock(FileChange.class); + when(newFileChangeVersion.getTargetEntity()).thenReturn(target); + when(this.fileChangeStorageManager.refactorFileChangeEntity(fileChange, target)) + .thenReturn(newFileChangeVersion); + + DocumentReference crDocumentReference = mock(DocumentReference.class); + when(this.changeRequestDocumentReferenceResolver.resolve(any(ChangeRequest.class))) + .thenReturn(crDocumentReference); + XWikiDocument document = mock(XWikiDocument.class); + when(document.clone()).thenReturn(document); + when(this.wiki.getDocument(crDocumentReference, this.context)).thenReturn(document); + when(document.isNew()).thenReturn(false); + DocumentAuthors documentAuthors = mock(DocumentAuthors.class); + when(document.getAuthors()).thenReturn(documentAuthors); + DocumentReference userDocRef = mock(DocumentReference.class); + when(this.context.getUserReference()).thenReturn(userDocRef); + UserReference userReference = mock(UserReference.class); + when(this.userReferenceResolver.resolve(userDocRef)).thenReturn(userReference); + BaseObject xobject = mock(BaseObject.class); + when(document.getXObject(CHANGE_REQUEST_XCLASS, 0, true, this.context)).thenReturn(xobject); + when(document.isMetaDataDirty()).thenReturn(true); + + this.storageManager.refactorTargetEntity(changeRequest, source, target, false); + + // The status of a merged change request must remain "merged" even though its file changes are rewritten + // to point to the new document reference. + verify(xobject).set("status", "merged", this.context); + verify(xobject, never()).set(eq("status"), eq("draft"), any()); + verify(xobject, never()).set(eq("status"), eq("ready_for_review"), any()); + verify(this.wiki).saveDocument(eq(document), any(String.class), eq(this.context)); + } + @Test void load() throws Exception { diff --git a/application-changerequest-test/application-changerequest-test-docker/src/test/it/org/xwiki/contrib/changerequest/test/ui/RefactoringChangeRequestIT.java b/application-changerequest-test/application-changerequest-test-docker/src/test/it/org/xwiki/contrib/changerequest/test/ui/RefactoringChangeRequestIT.java index efe420f7..565783c1 100644 --- a/application-changerequest-test/application-changerequest-test-docker/src/test/it/org/xwiki/contrib/changerequest/test/ui/RefactoringChangeRequestIT.java +++ b/application-changerequest-test/application-changerequest-test-docker/src/test/it/org/xwiki/contrib/changerequest/test/ui/RefactoringChangeRequestIT.java @@ -19,6 +19,7 @@ */ package org.xwiki.contrib.changerequest.test.ui; +import java.util.Arrays; import java.util.List; import org.junit.jupiter.api.BeforeAll; @@ -290,4 +291,150 @@ void refactoringSpace(TestUtils testUtils, TestReference testReference) assertFalse( fileChangesPane.isDiffOutdated(createdPageReferenceRefactored.getLocalDocumentReference().toString())); } + + /** + * Same kind of scenario as {@link #refactoringSpace(TestUtils, TestReference)} but with the + * {@code refactorMergedChangeRequests} configuration enabled: contrary to the default behavior, a merged change + * request must also have its page reference refactored when that page is renamed, while its status and content + * remain untouched. The merged change request also contains a page creation and a page deletion, none of which + * should be impacted by the refactoring of the unrelated edited page. The published page is also further edited + * after the merge, to check that the file change diff of the merged and refactored change request correctly + * reports it as outdated, while still displaying the actual change performed by the change request itself. + */ + @Test + void refactoringMergedChangeRequestWhenConfigurationEnabled(TestUtils testUtils, TestReference testReference) + throws Exception + { + testUtils.loginAsSuperAdmin(); + testUtils.updateObject(Arrays.asList("ChangeRequest", "Code"), "Configuration", + "ChangeRequest.Code.ConfigurationClass", 0, + "refactorMergedChangeRequests", 1); + + try { + DocumentReference originalPage = new DocumentReference("MergedRefactoringPage", + testReference.getLastSpaceReference()); + testUtils.createPage(originalPage, "Content before refactoring."); + DocumentReference pageToDelete = new DocumentReference("MergedRefactoringPageToDelete", + testReference.getLastSpaceReference()); + testUtils.createPage(pageToDelete, "Page to be deleted."); + + // Create a change request editing the page, approve it and merge it. + testUtils.login(CR_USER, CR_USER); + testUtils.gotoPage(originalPage); + ExtendedViewPage extendedViewPage = new ExtendedViewPage(); + ExtendedEditPage extendedEditPage = extendedViewPage.clickStandardEdit(true); + extendedEditPage.getWrappedEditor().getRichTextArea() + .setContent("Content before refactoring.\nSome change."); + ChangeRequestSaveModal changeRequestSaveModal = extendedEditPage.clickSaveAsChangeRequest(); + changeRequestSaveModal.setChangeRequestTitle("Refactoring_MergedCR"); + ChangeRequestPage changeRequestPage = changeRequestSaveModal.clickSave(); + + // Add a page creation request to the same change request. + testUtils.gotoPage(originalPage); + extendedViewPage = new ExtendedViewPage(); + ExtendedCreatePage extendedCreatePage = extendedViewPage.clickStandardCreate(); + extendedCreatePage.getDocumentPicker().setTitle("MergedRefactoringPageCreated"); + extendedEditPage = extendedCreatePage.clickChangeRequestCreateButton(true); + extendedEditPage.getWrappedEditor().getRichTextArea().setContent("Content of the created page."); + changeRequestSaveModal = extendedEditPage.clickSaveAsChangeRequest(); + changeRequestSaveModal.openAddChangesToExistingChangeRequestCollapse(); + changeRequestSaveModal.selectExistingChangeRequest("Refactoring_MergedCR").select(); + changeRequestPage = changeRequestSaveModal.clickSave(); + DocumentReference createdPage = new DocumentReference("WebHome", + new SpaceReference("MergedRefactoringPageCreated", testReference.getLastSpaceReference())); + + // Add a page deletion request to the same change request. + testUtils.gotoPage(pageToDelete); + extendedViewPage = new ExtendedViewPage(); + ExtendedDeleteConfirmationPage extendedDeleteConfirmationPage = extendedViewPage.clickRequestForDeletion(); + changeRequestSaveModal = extendedDeleteConfirmationPage.clickChangeRequestDelete(); + changeRequestSaveModal.openAddChangesToExistingChangeRequestCollapse(); + changeRequestSaveModal.selectExistingChangeRequest("Refactoring_MergedCR").select(); + changeRequestPage = changeRequestSaveModal.clickSave(); + + // Approve and merge the change request containing the edition, the creation and the deletion. + ReviewContainer reviewContainer = changeRequestPage.clickReviewButton(); + reviewContainer.selectApprove(); + changeRequestPage = reviewContainer.save(); + changeRequestPage = changeRequestPage.clickMergeButton(); + assertEquals("Published", changeRequestPage.getStatusLabel()); + + // Check that the creation and the deletion have actually been applied. + testUtils.gotoPage(createdPage); + assertFalse(new ExtendedViewPage().isNewDocument()); + testUtils.gotoPage(pageToDelete); + assertTrue(new ExtendedViewPage().isNewDocument()); + + // Perform a direct edition of the published page, after the merge, so that the file change of the + // merged change request becomes outdated with regard to the published version of the page. + testUtils.gotoPage(originalPage); + ExtendedViewPage publishedViewPage = new ExtendedViewPage(); + ExtendedEditPage publishedEditPage = publishedViewPage.clickStandardEdit(true); + publishedEditPage.getWrappedEditor().getRichTextArea() + .setContent("Content before refactoring.\nSome change.\nDirect edit after merge."); + publishedEditPage.clickSaveAndView(); + + // Rename the page: with the configuration enabled, the merged change request should follow. + testUtils.loginAsSuperAdmin(); + ViewPage viewPage = testUtils.gotoPage(originalPage); + RenamePage renamePage = viewPage.rename(); + DocumentReference renamedPage = new DocumentReference("MergedRefactoringPageRenamed", + testReference.getLastSpaceReference()); + renamePage.getDocumentPicker().setTitle("MergedRefactoringPageRenamed"); + CopyOrRenameOrDeleteStatusPage renameStatusPage = renamePage.clickRenameButton(); + renameStatusPage = renameStatusPage.waitUntilFinished(); + assertEquals("Done.", renameStatusPage.getInfoMessage()); + + testUtils.gotoPage(renamedPage); + ExtendedViewPage renamedViewPage = new ExtendedViewPage(); + assertFalse(renamedViewPage.isNewDocument()); + ChangeRequestLiveDataElement changeRequestLiveDataElement = renamedViewPage.openChangeRequestTab(); + // The merged change request is now attached to the renamed page. + assertEquals(1, changeRequestLiveDataElement.countRows()); + + ChangeRequestLiveDataElement.ChangeRequestRowElement rowElement = + changeRequestLiveDataElement.getChangeRequests().get(0); + assertEquals("Refactoring_MergedCR", rowElement.getTitle()); + // The status of the change request is not affected by the refactoring. + assertEquals("Published", rowElement.getStatus()); + + changeRequestPage = rowElement.gotoChangeRequest(); + assertEquals("Published", changeRequestPage.getStatusLabel()); + + FileChangesPane fileChangesPane = changeRequestPage.openFileChanges(); + List listOfChangedFiles = fileChangesPane.getListOfChangedFilesReferences(); + // The edited page (now refactored), the created page and the deleted page. + assertEquals(3, listOfChangedFiles.size()); + assertTrue(listOfChangedFiles.contains(renamedPage.getLocalDocumentReference().toString()), + String.format("List of changed files [%s] didn't contain [%s]", listOfChangedFiles, + renamedPage.getLocalDocumentReference())); + assertFalse(listOfChangedFiles.contains(originalPage.getLocalDocumentReference().toString()), + String.format("List of changed files [%s] still contained the old reference [%s]", + listOfChangedFiles, originalPage.getLocalDocumentReference())); + assertTrue(listOfChangedFiles.contains(createdPage.getLocalDocumentReference().toString()), + String.format("List of changed files [%s] didn't contain the created page [%s]", listOfChangedFiles, + createdPage.getLocalDocumentReference())); + assertTrue(listOfChangedFiles.contains(pageToDelete.getLocalDocumentReference().toString()), + String.format("List of changed files [%s] didn't contain the deleted page [%s]", listOfChangedFiles, + pageToDelete.getLocalDocumentReference())); + + // The diff of the refactored file change still reflects the actual content change performed by the + // change request, and it wasn't altered by the refactoring. + ChangeRequestEntityDiff contentEntityDiff = + fileChangesPane.getEntityDiff(renamedPage.getLocalDocumentReference().toString(), "Page properties"); + List contentDiff = contentEntityDiff.getDiff("Content"); + assertFalse(contentDiff.isEmpty()); + assertTrue(contentDiff.stream().anyMatch(line -> line.contains("Some change")), + String.format("Diff [%s] doesn't contain the expected change.", contentDiff)); + + // The refactored file change is outdated since the published page was edited after the merge. + assertTrue(fileChangesPane.isDiffOutdated(renamedPage.getLocalDocumentReference().toString())); + } finally { + // Restore the default configuration so it doesn't leak onto other tests. + testUtils.loginAsSuperAdmin(); + testUtils.updateObject(Arrays.asList("ChangeRequest", "Code"), "Configuration", + "ChangeRequest.Code.ConfigurationClass", 0, + "refactorMergedChangeRequests", 0); + } + } } diff --git a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ChangeRequestTranslations.xml b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ChangeRequestTranslations.xml index 270da86e..6da5ad76 100644 --- a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ChangeRequestTranslations.xml +++ b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ChangeRequestTranslations.xml @@ -169,6 +169,8 @@ ChangeRequest.Code.ConfigurationClass_renderedDiffComponent=Security policy to u ChangeRequest.Code.ConfigurationClass_renderedDiffComponent.hint=The default policy is the safest but it prevents all scripts from being rendered, even if the scripts are part of the underlying document sheets. This posture can be relaxed by selecting one of the other approaches. ChangeRequest.Code.ConfigurationClass_acceptOnlyAllowedApprovers=Only accept approvers that have the approval right ChangeRequest.Code.ConfigurationClass_acceptOnlyAllowedApprovers.hint=When checked, users who are proposed as approvers are systematically validated to ensure they have approval right. +ChangeRequest.Code.ConfigurationClass_refactorMergedChangeRequests=Refactor references in merged Change Requests +ChangeRequest.Code.ConfigurationClass_refactorMergedChangeRequests.hint=When checked, renaming or moving a page will also update the reference of that page in Change Requests that have already been merged. Only the reference is updated: the content of the merged Change Request is never modified. changerequest.diffRender.name.default=Default policy changerequest.diffRender.description.default=Safest policy for the rendered diff: it prevents any script from being executed. diff --git a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/Configuration.xml b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/Configuration.xml index 169ba10e..1cde5574 100644 --- a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/Configuration.xml +++ b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/Configuration.xml @@ -275,6 +275,21 @@ $escapetool.xml($services.localization.render("changerequest.mergeApprovalStrate com.xpn.xwiki.objects.classes.BooleanClass + + + + 0 + checkbox + + + refactorMergedChangeRequests + 17 + refactorMergedChangeRequests + 0 + + + com.xpn.xwiki.objects.classes.BooleanClass + {{velocity}} #if ($type == 'edit') @@ -395,6 +410,9 @@ $escapetool.xml($services.localization.render("changerequest.diffRender.name.$va + + 0 + @@ -667,6 +685,7 @@ $escapetool.xml($services.localization.render("changerequest.diffRender.name.$va renderedDiffEnabled renderedDiffComponent acceptOnlyAllowedApprovers + refactorMergedChangeRequests diff --git a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ConfigurationClass.xml b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ConfigurationClass.xml index e3861cfc..8bbfa0c7 100644 --- a/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ConfigurationClass.xml +++ b/application-changerequest-ui/src/main/resources/ChangeRequest/Code/ConfigurationClass.xml @@ -270,6 +270,21 @@ $escapetool.xml($services.localization.render("changerequest.mergeApprovalStrate com.xpn.xwiki.objects.classes.BooleanClass + + + + 0 + checkbox + + + refactorMergedChangeRequests + 17 + refactorMergedChangeRequests + 0 + + + com.xpn.xwiki.objects.classes.BooleanClass + {{velocity}} #if ($type == 'edit')