From d2bf06b4961611a1bb2176def44abb346ad8d712 Mon Sep 17 00:00:00 2001 From: Marius Dumitru Florea Date: Fri, 17 Oct 2025 17:34:31 +0300 Subject: [PATCH 1/2] XWIKI-2470: Improve saving to not save data that has not changed * Remove duplicated code between SaveAction and SaveAndContinueAction that builds the JSON response. Put the "JSON" map object on the XWiki context and let various parts of the code add stuff that needs to be returned to the editor. * Remove the redirect object before updating the authors, in order to be able to detect if the document is dirty before the authors are set. * Move the line that sets the metadata dirty right before saving, because it is needed only to force incrementing the version. * Small code improvements and more comments added. * Let the editor know when the save doesn't create a new revision in order to not show the "Saved" notification because it can mislead the user into thinking that a new version was created. * Add page objects for the history dropdown from the realtime toolbar. * Add functional test. * Don't propagate the version on save (to the other collaborators) if there's no new version. * Bulletproof the history dropdown to not duplicate versions. --- .../java/com/xpn/xwiki/web/SaveAction.java | 142 ++++++++------ .../xpn/xwiki/web/SaveAndContinueAction.java | 25 +-- .../com/xpn/xwiki/web/SaveActionTest.java | 11 +- .../{Coeditor.java => CoeditorElement.java} | 4 +- .../realtime/test/po/CoeditorsDropdown.java | 8 +- .../realtime/test/po/HistoryDropdown.java | 104 ++++++++++ .../realtime/test/po/RealtimeEditToolbar.java | 22 ++- .../xwiki/realtime/test/po/SummaryModal.java | 15 +- .../realtime/test/po/VersionElement.java | 71 +++++++ .../XWiki/Realtime/Configuration.xml | 1 + .../src/main/webjar/saver.js | 5 +- .../src/main/webjar/toolbar.js | 15 +- .../test/ui/RealtimeWYSIWYGEditorIT.java | 179 +++++++++++++++++- .../js/xwiki/actionbuttons/actionButtons.js | 20 +- 14 files changed, 504 insertions(+), 118 deletions(-) rename xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/{Coeditor.java => CoeditorElement.java} (97%) create mode 100644 xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java create mode 100644 xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java index a8cb1f23fa86..445c69d8cbc2 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java @@ -93,14 +93,9 @@ public class SaveAction extends EditAction private static final Logger LOGGER = LoggerFactory.getLogger(SaveAction.class); /** - * The key to retrieve the saved object version from the context. + * The key used to store the JSON answer for the save action on the XWiki context. */ - private static final String SAVED_OBJECT_VERSION_KEY = "SaveAction.savedObjectVersion"; - - /** - * The context key to know if a document has been merged for saving it. - */ - private static final String MERGED_DOCUMENTS = "SaveAction.mergedDocuments"; + static final String JSON_ANSWER_KEY = "SaveAction.jsonAnswer"; /** * Parameter value used with forceSave to specify that the merge should be performed even if there is conflicts. @@ -238,64 +233,97 @@ public boolean save(XWikiContext context) throws XWikiException tdoc.readFromForm(form, context); } + // Remove the redirect object if the save request doesn't update it. This allows users to easily overwrite + // redirect place-holders that are created when we move pages around. + if (tdoc.getXObject(RedirectClassDocumentInitializer.REFERENCE) != null + && request.getParameter("XWiki.RedirectClass_0_location") == null) { + tdoc.removeXObjects(RedirectClassDocumentInitializer.REFERENCE); + } + + // There are cases (e.g. when leaving a realtime collaboration session) when we don't want to create a new + // document revision unless there are actual changes. This is indicated by setting the preventEmptyRevision + // request parameter to true. Changing the document authors marks the document metadata as dirty, which is why + // we need to remember the dirty state before updating the authors, if we want to prevent empty revisions. Note + // that the document can be further modified by the save event listeners called below, but we expect them to set + // a version summary comment if they want to force the save even when preventEmptyRevision is true. + boolean dirtyBeforeSettingAuthors = tdoc.isContentDirty() || tdoc.isMetaDataDirty(); + + // Update the document authors. UserReference currentUserReference = this.currentUserResolver.resolve(CurrentUserReference.INSTANCE); tdoc.getAuthors().setOriginalMetadataAuthor(currentUserReference); request.getEffectiveAuthor().ifPresent(tdoc.getAuthors()::setEffectiveMetadataAuthor); - if (tdoc.isNew()) { tdoc.getAuthors().setCreator(currentUserReference); } - // Make sure we have at least the meta data dirty status - tdoc.setMetaDataDirty(true); - - // Validate the document if we have xvalidate=1 in the request - if ("1".equals(request.getParameter("xvalidate"))) { - boolean validationResult = tdoc.validate(context); - // If the validation fails we should show the "Inline form" edit mode - if (validationResult == false) { - // Set display context to 'edit' - context.put("display", "edit"); - // Set the action used by the "Inline form" edit mode as the context action. See #render(XWikiContext). - context.setAction(tdoc.getDefaultEditMode(context)); - // Set the document in the context - context.put("doc", doc); - context.put("cdoc", tdoc); - context.put("tdoc", tdoc); - // Force the "Inline form" edit mode. - getCurrentScriptContext().setAttribute("editor", "inline", ScriptContext.ENGINE_SCOPE); - - return true; - } + // Validate the document if we have xvalidate=1 in the request. + if ("1".equals(request.getParameter("xvalidate")) && !tdoc.validate(context)) { + // Validation failed. Redirect to "Inline form" edit mode. + // Set display context to "edit". + context.put("display", "edit"); + // Set the action used by the "Inline form" edit mode as the context action. See #render(XWikiContext). + context.setAction(tdoc.getDefaultEditMode(context)); + // Set the document in the context. + context.put("doc", doc); + context.put("cdoc", tdoc); + context.put("tdoc", tdoc); + // Force the "Inline form" edit mode. + getCurrentScriptContext().setAttribute("editor", "inline", ScriptContext.ENGINE_SCOPE); + return true; } - // Remove the redirect object if the save request doesn't update it. This allows users to easily overwrite - // redirect place-holders that are created when we move pages around. - if (tdoc.getXObject(RedirectClassDocumentInitializer.REFERENCE) != null - && request.getParameter("XWiki.RedirectClass_0_location") == null) { - tdoc.removeXObjects(RedirectClassDocumentInitializer.REFERENCE); - } + // Prepare the JSON answer that will be sent to the editor in case of an async request. We put it on the XWiki + // context so that other parts of the code (e.g. event listeners) can add more information if needed. + Map jsonAnswer = new LinkedHashMap<>(); + context.put(JSON_ANSWER_KEY, jsonAnswer); - // We only proceed on the check between versions in case of AJAX request, so we currently stay in the edit form - // This can be improved later by displaying a nice UI with some merge options in a sync request. - // For now we don't want our user to loose their changes. - if (isConflictCheckEnabled() && Utils.isAjaxRequest(context) - && request.getParameter("previousVersion") != null) { - if (isConflictingWithVersion(context, originalDoc, tdoc)) { - return true; - } + // Detect merge conflicts. We do this only if the previous version is known, otherwise a 3-way merge is not + // possible, and the save request is asynchronous, i.e. the user is waiting in edit mode for the save result. If + // a merge conflict is detected the editor will display the merge conflict modal. + if (isConflictCheckEnabled() && Utils.isAjaxRequest(context) && request.getParameter("previousVersion") != null + && isConflictingWithVersion(context, originalDoc, tdoc)) { + return true; } - // Make sure the user is allowed to make this modification + // Make sure the user is allowed to save the document. This triggers an event whose listeners can block (cancel) + // the save if needed. We trigger the event even if the document is not dirty (i.e. doesn't need to be saved) + // because: + // * the listeners can change the document (i.e. even if the document is not dirty now, it may become dirty + // after the event listeners are called) + // * the listeners may want to block a hierarchy template from being applied, even if the root document is not + // itself affected. xwiki.checkSavingDocument(context.getUserReference(), tdoc, tdoc.getComment(), tdoc.isMinorEdit(), context); - // We get the comment to be used from the document - // It was read using readFromForm - xwiki.saveDocument(tdoc, tdoc.getComment(), tdoc.isMinorEdit(), context); - this.temporaryAttachmentSessionsManager.removeUploadedAttachments(tdoc.getDocumentReference()); + // Note that users may save an existing document without making any changes just to update the author, e.g. to + // change access rights. In this or other similar cases the version summary comment can be used to justify the + // action, which is why we force the save if a comment is provided. We consider preventEmptyRevision to be false + // by default for backward compatibility. This is currently used by realtime collaboration to avoid creating + // empty revisions when leaving the editing session. + if (tdoc.isNew() || dirtyBeforeSettingAuthors || StringUtils.isNotEmpty(tdoc.getComment()) + || !"true".equals(request.getParameter("preventEmptyRevision"))) { + // Make sure we have at least the meta data dirty otherwise the version is not incremented. + tdoc.setMetaDataDirty(true); + + // We take the version summary comment from the document being saved because it may have been modified by an + // event listener after it was copied from the EditForm by the call to readFromForm. + xwiki.saveDocument(tdoc, tdoc.getComment(), tdoc.isMinorEdit(), context); + + // Return the new version number to the editor that triggered the save. + jsonAnswer.put("newVersion", tdoc.getRCSVersion().toString()); + } else { + // Let the editor know that the document was not saved because there were no changes. + jsonAnswer.put("noChanges", "true"); + } - context.put(SAVED_OBJECT_VERSION_KEY, tdoc.getRCSVersion()); + // Cleanup temporary attachments that were uploaded while editing the document. We do this even if we didn't + // create a new revision (e.g. if there were no changes) because those attachments are not needed anymore (e.g. + // the user may have discarded the changes that lead to the upload of those attachments). + this.temporaryAttachmentSessionsManager.removeUploadedAttachments(tdoc.getDocumentReference()); + // If a template hierarchy is specified we start a job to copy the child pages. Even if we didn't create a new + // revision for the root document (because there were no changes) the child pages may still need to be created + // or updated, because the template can be applied on existing documents, even on documents where the template + // was previously applied. Job createJob = startCreateJob(tdoc.getDocumentReference(), form); if (createJob != null) { if (isAsync(request)) { @@ -479,7 +507,7 @@ private boolean isConflictingWithVersion(XWikiContext context, XWikiDocument ori // then we pursue to save the document. if (FORCE_SAVE_MERGE.equals(request.getParameter("forceSave")) || !mergeDocumentResult.hasConflicts()) { - context.put(MERGED_DOCUMENTS, "true"); + getJSONAnswer(context).put("mergedDocument", "true"); return false; // If we got merge conflicts and we don't want to force it, then we record the conflict in @@ -509,6 +537,12 @@ private boolean isConflictingWithVersion(XWikiContext context, XWikiDocument ori return false; } + @SuppressWarnings("unchecked") + Map getJSONAnswer(XWikiContext context) + { + return (Map) context.get(JSON_ANSWER_KEY); + } + @Override public boolean action(XWikiContext context) throws XWikiException { @@ -529,13 +563,7 @@ public boolean action(XWikiContext context) throws XWikiException // forward to view if (isAjaxRequest) { - Map jsonAnswer = new LinkedHashMap<>(); - Version newVersion = (Version) context.get(SAVED_OBJECT_VERSION_KEY); - jsonAnswer.put("newVersion", newVersion.toString()); - if ("true".equals(context.get(MERGED_DOCUMENTS))) { - jsonAnswer.put("mergedDocument", "true"); - } - answerJSON(context, HttpStatus.SC_OK, jsonAnswer); + answerJSON(context, HttpStatus.SC_OK, getJSONAnswer(context)); } else { sendRedirect(context.getResponse(), Utils.getRedirect("view", context)); } diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java index 70da4c3ae8b9..60ac16e8d700 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAndContinueAction.java @@ -20,7 +20,6 @@ package com.xpn.xwiki.web; import java.io.IOException; -import java.util.LinkedHashMap; import java.util.Map; import javax.inject.Inject; @@ -32,7 +31,6 @@ import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.suigeneris.jrcs.rcs.Version; import org.xwiki.component.annotation.Component; import com.xpn.xwiki.XWikiContext; @@ -53,16 +51,6 @@ public class SaveAndContinueAction extends XWikiAction /** Key for storing the wrapped action in the context. */ private static final String WRAPPED_ACTION_CONTEXT_KEY = "SaveAndContinueAction.wrappedAction"; - /** - * The key to retrieve the saved object version from the context. - */ - private static final String SAVED_OBJECT_VERSION_KEY = "SaveAction.savedObjectVersion"; - - /** - * The context key to know if a document has been merged for saving it. - */ - private static final String MERGED_DOCUMENTS = "SaveAction.mergedDocuments"; - /** * The default context value to put with {@link #MERGED_DOCUMENTS} key. */ @@ -175,15 +163,10 @@ public boolean action(XWikiContext context) throws XWikiException // If this is an ajax request, no need to redirect. if (isAjaxRequest) { - Version newVersion = (Version) context.get(SAVED_OBJECT_VERSION_KEY); - - // in case of property update, SaveAction has not been called, so we don't get the new version. - if (newVersion != null) { - Map jsonAnswer = new LinkedHashMap<>(); - jsonAnswer.put("newVersion", newVersion.toString()); - if (MERGED_DOCUMENTS_VALUE.equals(context.get(MERGED_DOCUMENTS))) { - jsonAnswer.put("mergedDocument", MERGED_DOCUMENTS_VALUE); - } + @SuppressWarnings("unchecked") + Map jsonAnswer = (Map) context.get(SaveAction.JSON_ANSWER_KEY); + // In case of property update, SaveAction is not called, so there's no JSON answer. + if (jsonAnswer != null) { answerJSON(context, HttpStatus.SC_OK, jsonAnswer); } else { context.getResponse().setStatus(HttpServletResponse.SC_NO_CONTENT); diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/SaveActionTest.java b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/SaveActionTest.java index 32205a86d23b..d9ba0b11844d 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/SaveActionTest.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/SaveActionTest.java @@ -33,6 +33,7 @@ import java.util.Collections; import java.util.Date; import java.util.List; +import java.util.Map; import java.util.Optional; import javax.inject.Named; @@ -197,7 +198,7 @@ void validSave() throws Exception when(mockForm.getTemplate()).thenReturn(""); assertFalse(saveAction.save(this.context)); - assertEquals(new Version("1.2"), this.context.get("SaveAction.savedObjectVersion")); + assertEquals(Map.of("newVersion", "1.2"), saveAction.getJSONAnswer(context)); verify(mockAuthors).setOriginalMetadataAuthor(this.currentUserReference); verify(mockAuthors).setEffectiveMetadataAuthor(this.effectiveAuthor); @@ -220,7 +221,7 @@ void validSaveNewTranslation() throws Exception when(mockRequest.getParameter("previousVersion")).thenReturn("1.1"); when(mockRequest.getParameter("isNew")).thenReturn("true"); assertFalse(saveAction.save(this.context)); - assertEquals(new Version("1.1"), this.context.get("SaveAction.savedObjectVersion")); + assertEquals(Map.of("newVersion", "1.1"), saveAction.getJSONAnswer(context)); verify(this.xWiki).checkSavingDocument(eq(USER_REFERENCE), any(XWikiDocument.class), eq(""), eq(false), eq(this.context)); verify(this.xWiki).saveDocument(any(XWikiDocument.class), eq(""), eq(false), eq(this.context)); @@ -244,7 +245,7 @@ void validSaveOldTranslation() throws Exception when(mockClonedDocument.getRCSVersion()).thenReturn(new Version("1.4")); when(mockClonedDocument.getComment()).thenReturn("My Changes"); assertFalse(saveAction.save(this.context)); - assertEquals(new Version("1.4"), this.context.get("SaveAction.savedObjectVersion")); + assertEquals(Map.of("newVersion", "1.4"), saveAction.getJSONAnswer(context)); verify(this.xWiki).checkSavingDocument(USER_REFERENCE, mockClonedDocument, "My Changes", false, this.context); verify(this.xWiki).saveDocument(mockClonedDocument, "My Changes", false, this.context); } @@ -275,7 +276,7 @@ void validSaveRequestImageUploadAndConflictCheck() throws Exception when(mockDocument.getObjectDiff("1.1", "1.2", context)).thenReturn(Collections.emptyList()); assertFalse(saveAction.save(this.context)); - assertEquals(new Version("1.2"), this.context.get("SaveAction.savedObjectVersion")); + assertEquals(Map.of("newVersion", "1.2"), saveAction.getJSONAnswer(context)); verify(mockAuthors).setOriginalMetadataAuthor(this.currentUserReference); verify(mockAuthors).setEffectiveMetadataAuthor(this.effectiveAuthor); @@ -288,6 +289,7 @@ void validSaveRequestImageUploadAndConflictCheck() throws Exception @Test void saveFromTemplate() throws Exception { + when(mockClonedDocument.getRCSVersion()).thenReturn(new Version("3.2")); when(this.mockForm.getTemplate()).thenReturn("TemplateSpace.TemplateDocument"); DocumentReference templateReference = new DocumentReference(context.getWikiId(), "TemplateSpace", "TemplateDocument"); @@ -329,6 +331,7 @@ void saveSectionWithAttachmentUpload() throws Exception String comment = "Some comment"; when(sectionDoc.getComment()).thenReturn(comment); when(sectionDoc.isMinorEdit()).thenReturn(true); + when(mockClonedDocument.getRCSVersion()).thenReturn(new Version("4.1")); assertFalse(this.saveAction.save(this.context)); diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/Coeditor.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorElement.java similarity index 97% rename from xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/Coeditor.java rename to xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorElement.java index 96356648eb53..8bdaadb86f20 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/Coeditor.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorElement.java @@ -30,7 +30,7 @@ * @since 16.10.6 * @since 17.3.0RC1 */ -public class Coeditor extends BaseElement +public class CoeditorElement extends BaseElement { private WebElement container; @@ -39,7 +39,7 @@ public class Coeditor extends BaseElement * * @param container the WebElement used to display the coeditor */ - public Coeditor(WebElement container) + public CoeditorElement(WebElement container) { this.container = container; } diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorsDropdown.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorsDropdown.java index a504dc519fff..bc79049c1a3e 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorsDropdown.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/CoeditorsDropdown.java @@ -85,19 +85,19 @@ private WebElement getToggle() /** * @return the list of users that are currently editing the page in realtime */ - public List getCoeditors() + public List getCoeditors() { return getDriver().findElements(By.cssSelector(".realtime-users-dropdown .realtime-user")).stream() - .map(Coeditor::new).toList(); + .map(CoeditorElement::new).toList(); } /** * @param coeditorId the coeditor identifier * @return the coeditor with the specified identifier */ - public Coeditor getCoeditor(String coeditorId) + public CoeditorElement getCoeditor(String coeditorId) { - return new Coeditor( + return new CoeditorElement( getDriver().findElement(By.cssSelector(".realtime-users-dropdown[data-id='" + coeditorId + "']"))); } } diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java new file mode 100644 index 000000000000..e05d2c11b796 --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java @@ -0,0 +1,104 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.realtime.test.po; + +import java.util.List; + +import org.openqa.selenium.By; +import org.openqa.selenium.Keys; +import org.openqa.selenium.WebElement; +import org.openqa.selenium.support.ui.ExpectedConditions; +import org.xwiki.test.ui.po.BaseElement; + +/** + * The dropdown that lists recent versions of the edited document. + * + * @version $Id$ + * @since 17.9.0RC1 + * @since 17.4.6 + * @since 16.10.13 + */ +public class HistoryDropdown extends BaseElement +{ + private static final By DROPDOWN_MENU = By.cssSelector(".realtime-versions.dropdown-menu"); + + /** + * Clicks on the dropdown toggle to open the dropdown. + * + * @return this instance + */ + public HistoryDropdown open() + { + getToggle().click(); + getDriver().waitUntilElementIsVisible(DROPDOWN_MENU); + return this; + } + + /** + * Uses the ESCAPE key to close the dropdown. + * + * @return this instance + */ + public HistoryDropdown close() + { + getToggle().sendKeys(Keys.ESCAPE); + getDriver().waitUntilElementDisappears(DROPDOWN_MENU); + return this; + } + + private WebElement getToggle() + { + return getDriver().findElement(By.cssSelector(".realtime-action-history.dropdown-toggle")); + } + + /** + * @return the document versions listed in the history dropdown + */ + public List getVersions() + { + return getDriver().findElements(By.cssSelector(".realtime-versions .realtime-version")).stream() + .map(VersionElement::new).toList(); + } + + /** + * Waits for the version with the given version number to be listed in the history dropdown. + * + * @param versionNumber the version number to wait for + * @return this instance + */ + public HistoryDropdown waitForVersion(String versionNumber) + { + getDriver().waitUntilCondition(ExpectedConditions.presenceOfElementLocated( + By.cssSelector(".realtime-version[data-version*=\"\\\"" + versionNumber + "\\\"\"]"))); + return this; + } + + /** + * Clicks on the "Summarize Changes" entry from the history dropdown and waits for the corresponding modal to be + * displayed. + * + * @return the modal used to review and summarize the changes + */ + public SummaryModal summarizeChanges() + { + getDriver().findElement(By.cssSelector(".realtime-versions .realtime-action-summarize")).click(); + return new SummaryModal(); + } +} diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java index ec3faeb5b349..3babf8175f0f 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java @@ -126,9 +126,7 @@ public SummaryModal clickSummarizeAndDone() { openDoneDropdown(); getDriver().findElement(By.cssSelector(".realtime-edit-toolbar .realtime-action-summarize")).click(); - SummaryModal summaryModal = new SummaryModal(); - getDriver().waitUntilCondition(it -> summaryModal.isDisplayed()); - return summaryModal; + return new SummaryModal(); } /** @@ -142,10 +140,10 @@ public String getUserId() /** * @return the list of coeditors listed directly on the toolbar */ - public List getVisibleCoeditors() + public List getVisibleCoeditors() { return getDriver().findElements(By.cssSelector(".realtime-edit-toolbar .realtime-users .realtime-user")) - .stream().map(Coeditor::new).toList(); + .stream().map(CoeditorElement::new).toList(); } /** @@ -154,12 +152,12 @@ public List getVisibleCoeditors() * @param coeditorId the coeditor identifier * @return this instance */ - public Coeditor waitForCoeditor(String coeditorId) + public CoeditorElement waitForCoeditor(String coeditorId) { By coeditorSelector = By.cssSelector(".realtime-edit-toolbar .realtime-user[data-id='" + coeditorId + "']"); // The coeditor can be either displayed directly on the toolbar or hidden in the dropdown. getDriver().waitUntilCondition(ExpectedConditions.presenceOfElementLocated(coeditorSelector)); - return new Coeditor(getDriver().findElement(coeditorSelector)); + return new CoeditorElement(getDriver().findElement(coeditorSelector)); } /** @@ -168,7 +166,7 @@ public Coeditor waitForCoeditor(String coeditorId) */ public boolean isEditingAlone() { - List visibleCoeditors = getVisibleCoeditors(); + List visibleCoeditors = getVisibleCoeditors(); return visibleCoeditors.size() == 1 && visibleCoeditors.get(0).getId().equals(getUserId()); } @@ -250,4 +248,12 @@ public RealtimeEditToolbar waitForConcurrentEditingWarning() } return this; } + + /** + * @return the dropdown listing recent versions of the edited document, and the "Summarize Changes" action + */ + public HistoryDropdown getHistoryDropdown() + { + return new HistoryDropdown(); + } } diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/SummaryModal.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/SummaryModal.java index 9e6a0e4328af..8372afecc610 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/SummaryModal.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/SummaryModal.java @@ -20,6 +20,7 @@ package org.xwiki.realtime.test.po; import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; import org.xwiki.test.ui.po.BaseModal; /** @@ -35,16 +36,20 @@ public class SummaryModal extends BaseModal public SummaryModal() { super(By.id("realtime-changeSummaryModal")); + getDriver().waitUntilCondition(driver -> this.isDisplayed()); } /** * Fill the summary textarea with the given content. + * * @param summary the text to put in the summary textarea */ public void setSummary(String summary) { - getDriver().findElementWithoutWaiting(this.container, By.id("realtime-changeSummaryModal-summary")) - .sendKeys(summary); + WebElement textarea = + getDriver().findElementWithoutWaiting(this.container, By.id("realtime-changeSummaryModal-summary")); + textarea.clear(); + textarea.sendKeys(summary); } /** @@ -52,12 +57,14 @@ public void setSummary(String summary) */ public void toggleMinorEdit() { - getDriver().findElementWithoutWaiting(this.container, - By.cssSelector("input[type='checkbox'][name='minorChange']")).click(); + getDriver() + .findElementWithoutWaiting(this.container, By.cssSelector("input[type='checkbox'][name='minorChange']")) + .click(); } /** * Click on the save button and eventually wait for the saved success message. + * * @param waitSuccess if {@code true} wait for the "Saved" success message */ public void clickSave(boolean waitSuccess) diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java new file mode 100644 index 000000000000..6fb7bc012ead --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java @@ -0,0 +1,71 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.realtime.test.po; + +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; +import org.xwiki.test.ui.po.BaseElement; + +/** + * Represents a document version listed in the history dropdown from the realtime editing toolbar. + * + * @version $Id$ + * @since 17.9.0RC1 + * @since 17.4.6 + * @since 16.10.13 + */ +public class VersionElement extends BaseElement +{ + private WebElement container; + + /** + * Creates a new instance based on the given version element. + * + * @param container the WebElement used to display the version + */ + public VersionElement(WebElement container) + { + this.container = container; + } + + /** + * @return the author of this version + */ + public CoeditorElement getAuthor() + { + return new CoeditorElement(this.container.findElement(By.className("realtime-user"))); + } + + /** + * @return the version number + */ + public String getNumber() + { + return this.container.findElement(By.className("realtime-version-number")).getText(); + } + + /** + * @return the version date + */ + public String getDate() + { + return this.container.findElement(By.className("realtime-version-date")).getText(); + } +} diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-ui/src/main/resources/XWiki/Realtime/Configuration.xml b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-ui/src/main/resources/XWiki/Realtime/Configuration.xml index 1e76fc06b0a7..429bda8df141 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-ui/src/main/resources/XWiki/Realtime/Configuration.xml +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-ui/src/main/resources/XWiki/Realtime/Configuration.xml @@ -565,6 +565,7 @@ ul.realtime-versions > li.divider:last-child { <template id="realtime-edit-toolbar"> <div class="realtime-edit-toolbar" data-user-id=""> <div class="realtime-edit-toolbar-left"> + <input type="hidden" name="preventEmptyRevision" value="true" /> <div class="btn-group dropup"> <button type="button" diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js index 47c3df4ab8b9..3fa1eef0e50d 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js @@ -667,8 +667,11 @@ define('xwiki-realtime-saver', [ _afterSave({newVersion}) { if (newVersion === '1.1') { debug('Created document version 1.1'); - } else { + } else if (newVersion) { debug(`Version bumped from ${xwikiDocument.version} to ${newVersion}.`); + } else { + // There's no new version because there were no changes. + return; } this._state.version = newVersion; this._config.onCreateVersion({ diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/toolbar.js b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/toolbar.js index 4cfe629a3b0a..e548ddde5f13 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/toolbar.js +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/toolbar.js @@ -272,12 +272,23 @@ define('xwiki-realtime-toolbar', [ if (!this._lastReviewedVersion) { this._lastReviewedVersion = version.number; } - const versions = this._toolbar.querySelectorAll('.realtime-version'); + // Make sure we don't add the same version twice. This can happen if some of the previous versions were deleted + // and their version numbers are being reused. + const versions = [...this._toolbar.querySelectorAll('.realtime-version')].filter(existingVersion => { + if (JSON.parse(existingVersion.dataset.version).number === version.number) { + // The version element is wrapped in a list item element, that we need to remove as well. + existingVersion.parentElement.remove(); + return false; + } + return true; + }); + // Limit the number of versions shown in the dropdown. const limit = Number.parseInt(this._toolbar.querySelector('.realtime-versions').dataset.limit) || 5; if (versions.length >= limit) { - // The version element is wrapped in a list item element. + // The version element is wrapped in a list item element, that we need to remove as well. versions[0].parentElement.remove(); } + // Insert the new version. const versionWrapper = this._createVersionElement(version); this._toolbar.querySelector('.realtime-versions > .divider').before(versionWrapper); } diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java index 0838bfafcbb5..c3a2bf1eac1d 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java @@ -51,11 +51,13 @@ import org.xwiki.model.reference.EntityReference; import org.xwiki.panels.test.po.DocumentInformationPanel; import org.xwiki.realtime.test.RealtimeTestUtils; -import org.xwiki.realtime.test.po.Coeditor; +import org.xwiki.realtime.test.po.CoeditorElement; +import org.xwiki.realtime.test.po.HistoryDropdown; import org.xwiki.realtime.test.po.RealtimeEditToolbar; import org.xwiki.realtime.test.po.RealtimeInplaceEditablePage; import org.xwiki.realtime.test.po.SaveStatus; import org.xwiki.realtime.test.po.SummaryModal; +import org.xwiki.realtime.test.po.VersionElement; import org.xwiki.realtime.wysiwyg.test.po.RealtimeCKEditor; import org.xwiki.realtime.wysiwyg.test.po.RealtimeRichTextAreaElement; import org.xwiki.realtime.wysiwyg.test.po.RealtimeRichTextAreaElement.CoeditorPosition; @@ -179,7 +181,7 @@ void editAlone(TestReference testReference, TestUtils setup) inplaceEditablePage.getToolbar().waitForSaveStatus(SaveStatus.UNSAVED); inplaceEditablePage.getToolbar().waitForSaveStatus(SaveStatus.SAVED); - inplaceEditablePage.done(); + setup.leaveEditMode(); assertEquals("zero\ntwo\nthree", inplaceEditablePage.getContent()); // edit again and test the summarize & done @@ -195,11 +197,11 @@ void editAlone(TestReference testReference, TestUtils setup) summaryModal.clickSave(true); inplaceEditablePage.waitForView(); assertEquals("zero\ntwo\nthree\nfour", inplaceEditablePage.getContent()); - HistoryPane historyPane = inplaceEditablePage.openHistoryDocExtraPane(); + HistoryPane historyPane = inplaceEditablePage.openHistoryDocExtraPane().showMinorEdits(); assertEquals(4, historyPane.getNumberOfVersions()); assertEquals("Summarize changes", historyPane.getCurrentVersionComment()); - // delete the page to test creation with summarize & done (regression test for XWIKI-23136) + // Delete the page to test creation with summarize & done (regression test for XWIKI-23136). setup.deletePage(testReference); editPage = RealtimeWYSIWYGEditPage.gotoPage(testReference); editor = editPage.getContenEditor(); @@ -253,7 +255,7 @@ void editWithSelf(TestReference testReference, TestUtils setup, MultiUserTestUti String secondCoeditorId = secondEditPage.getToolbar().getUserId(); // Verify the list of coeditors. - Coeditor self = secondEditPage.getToolbar().waitForCoeditor(firstCoeditorId); + CoeditorElement self = secondEditPage.getToolbar().waitForCoeditor(firstCoeditorId); assertTrue(self.isDisplayed()); // The name is not visible when the user is displayed directly on the toolbar. assertEquals("", self.getName()); @@ -1441,7 +1443,7 @@ void editPageTranslations(TestUtils setup, TestReference testReference, MultiUse assertEquals("Deutsch content", firstTextArea.getText()); assertEquals("John, superadmin", firstEditPage.getToolbar().getVisibleCoeditors().stream() - .map(Coeditor::getAvatarHint).reduce((a, b) -> a + ", " + b).get()); + .map(CoeditorElement::getAvatarHint).reduce((a, b) -> a + ", " + b).get()); } @Test @@ -2332,6 +2334,171 @@ void syntaxChange(TestUtils setup, TestReference testReference, MultiUserTestUti assertEquals("[[label>>||anchor=\"target\"]] after", source); } + @Test + @Order(25) + void preventEmptyRevisions(TestUtils setup, TestReference testReference, MultiUserTestUtils multiUserSetup) + { + // + // First Tab + // + + // Start fresh. + setup.loginAsSuperAdmin(); + setup.deletePage(testReference); + + // Edit the page in the first browser tab (standalone). + RealtimeWYSIWYGEditPage firstEditPage = RealtimeWYSIWYGEditPage.gotoPage(testReference); + + // There are no revisions yet. + HistoryDropdown historyDropdown = firstEditPage.getToolbar().getHistoryDropdown(); + assertTrue(historyDropdown.open().getVersions().isEmpty()); + + // Even if there is no change, the document is new so the Done button should create the first revision. + firstEditPage.clickDone(); + + // + // Second Tab + // + + String secondTabHandle = multiUserSetup.openNewBrowserTab(XWIKI_ALIAS); + loginAsJohn(setup); + + // Edit the page in the second browser tab (in-place). + RealtimeInplaceEditablePage inplaceEditablePage = RealtimeInplaceEditablePage.gotoPage(testReference); + inplaceEditablePage.editInplace(); + RealtimeCKEditor secondEditor = new RealtimeCKEditor(); + RealtimeRichTextAreaElement secondTextArea = secondEditor.getRichTextArea(); + + // Verify that the history dropdown contains only one entry (the initial revision). + List versions = historyDropdown.open().getVersions(); + assertEquals(1, versions.size()); + assertEquals("1.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + + // Try to save without making any changes. Even if the author is different, no new revision should be created. + inplaceEditablePage.getToolbar().sendSaveShortcutKey(); + + // Verify that the history dropdown still contains only one entry (the initial revision). + versions = historyDropdown.open().getVersions(); + assertEquals(1, versions.size()); + assertEquals("1.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + + // + // First Tab + // + + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + // Join the realtime collaboration. + firstEditPage = RealtimeWYSIWYGEditPage.gotoPage(testReference); + RealtimeCKEditor firstEditor = firstEditPage.getContenEditor(); + RealtimeRichTextAreaElement firstTextArea = firstEditor.getRichTextArea(); + + // + // Second Tab + // + + multiUserSetup.switchToBrowserTab(secondTabHandle); + secondTextArea.sendKeys("one"); + inplaceEditablePage.getToolbar().sendSaveShortcutKey(); + + // Verify that the history dropdown contains two versions. + versions = historyDropdown.open().getVersions(); + assertEquals(2, versions.size()); + assertEquals("1.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + assertEquals("1.2", versions.get(1).getNumber()); + assertEquals("Jo", versions.get(1).getAuthor().getAbbreviation()); + historyDropdown.close(); + + // + // First Tab + // + + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + firstTextArea.waitUntilTextContains("one"); + + // Verify that the new version was propagated. + versions = historyDropdown.waitForVersion("1.2").open().getVersions(); + assertEquals(2, versions.size()); + assertEquals("1.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + assertEquals("1.2", versions.get(1).getNumber()); + assertEquals("Jo", versions.get(1).getAuthor().getAbbreviation()); + + // Try to save without making any changes. No new revision should be created. + firstEditPage.getToolbar().sendSaveShortcutKey(); + + // Verify that the history dropdown still contains only two entries. + versions = historyDropdown.open().getVersions(); + assertEquals(2, versions.size()); + + firstTextArea.sendKeys(Keys.END, " two"); + ViewPage viewPage = firstEditPage.clickDone(); + assertEquals("one two", viewPage.getContent()); + + // + // Second Tab + // + + multiUserSetup.switchToBrowserTab(secondTabHandle); + secondTextArea.waitUntilTextContains("two"); + + // Verify that the new version was propagated. + versions = historyDropdown.waitForVersion("2.1").open().getVersions(); + assertEquals(3, versions.size()); + assertEquals("1.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + assertEquals("1.2", versions.get(1).getNumber()); + assertEquals("Jo", versions.get(1).getAuthor().getAbbreviation()); + assertEquals("2.1", versions.get(2).getNumber()); + assertEquals("Su", versions.get(2).getAuthor().getAbbreviation()); + + // Leave the editing session without making any changes. No new revision should be created. + HistoryPane historyPane = inplaceEditablePage.done().openHistoryDocExtraPane().showMinorEdits(); + assertEquals(3, historyPane.getNumberOfVersions()); + assertEquals("2.1", historyPane.getCurrentVersion()); + assertEquals("superadmin", historyPane.getCurrentAuthor()); + + // + // First Tab + // + + // Edit again to check that we can force a new empty revision by specifying a version summary. + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + firstEditPage = RealtimeWYSIWYGEditPage.gotoPage(testReference); + + assertEquals(1, historyDropdown.open().getVersions().size()); + + SummaryModal summaryModal = historyDropdown.summarizeChanges(); + summaryModal.setSummary("An empty revision"); + summaryModal.clickSave(true); + + versions = historyDropdown.open().getVersions(); + assertEquals(2, versions.size()); + assertEquals("2.2", versions.get(1).getNumber()); + assertEquals("Su", versions.get(1).getAuthor().getAbbreviation()); + + // Try again, this time without a summary. + summaryModal = historyDropdown.summarizeChanges(); + summaryModal.setSummary(""); + summaryModal.clickSave(false); + firstEditPage.getToolbar().waitForSaveStatus(SaveStatus.SAVED); + + assertEquals(2, historyDropdown.open().getVersions().size()); + + // Leave the editing session with a summary. This should create a new revision. + summaryModal = firstEditPage.getToolbar().clickSummarizeAndDone(); + summaryModal.setSummary("Another empty revision"); + summaryModal.clickSave(true); + + historyPane = new ViewPage().openHistoryDocExtraPane().showMinorEdits(); + assertEquals(5, historyPane.getNumberOfVersions()); + assertEquals("3.1", historyPane.getCurrentVersion()); + assertEquals("superadmin", historyPane.getCurrentAuthor()); + assertEquals("Another empty revision", historyPane.getCurrentVersionComment()); + } + private void setMultiLingual(TestUtils setup, boolean isMultiLingual, String... supportedLanguages) { AdministrationPage adminPage = AdministrationPage.gotoPage(); diff --git a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js index 0c85b244f7ab..d77f5329a098 100644 --- a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js +++ b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js @@ -372,22 +372,24 @@ var XWiki = (function(XWiki) { $$('input[name=mergeChoices]').forEach(function (item) {item.remove();}); $$('input[name=customChoices]').forEach(function (item) {item.remove();}); - var hasBeenSaved = false; + let hasBeenSaved = false; if (state.isCreateFromTemplate) { // We might have a responseJSON containing other information than links, if the template cannot be accessed. - if (response.responseJSON && response.responseJSON.links) { + if (response.responseJSON?.links) { // Start the progress display. this.getStatus(response.responseJSON.links[0].href, state); } else { this.progressBox.hide(); this.savingBox.replace(this.savedBox); - // in such case the page is saved, so we'll need to maybe redirect + // In such case the page is saved, so we'll need to maybe redirect. hasBeenSaved = true; } } else { this.progressBox.hide(); - if (response.responseJSON && response.responseJSON.mergedDocument) { + if (response.responseJSON?.mergedDocument) { this.savingBox.replace(this.savedWithMergeBox); + } else if (response.responseJSON?.noChanges) { + this.savingBox.hide(); } else { this.savingBox.replace(this.savedBox); } @@ -401,23 +403,23 @@ var XWiki = (function(XWiki) { } } - if (response.responseJSON && response.responseJSON.newVersion) { - // update the version + if (response.responseJSON?.newVersion) { + // Update the version. require(['xwiki-meta'], function (xm) { xm.setVersion(response.responseJSON.newVersion); }); // We only update this field since the other ones are updated by the callback of setVersion. if (editingVersionDateField) { - editingVersionDateField.setValue(new Date().getTime()); + editingVersionDateField.setValue(Date.now()); } } - // Announce that the document has been saved + // Announce that the document has been saved. state.saveButton.fire("xwiki:document:saved", response.responseJSON); // If documents have been merged we need to reload to get latest saved version. - if (response.responseJSON && response.responseJSON.mergedDocument) { + if (response.responseJSON?.mergedDocument) { this.reloadEditor(); } }, From 05750805ac2e7dcfa6486ee5482f0926a0a84823 Mon Sep 17 00:00:00 2001 From: Marius Dumitru Florea Date: Mon, 20 Oct 2025 15:41:23 +0300 Subject: [PATCH 2/2] XWIKI-2470: Improve saving to not save data that has not changed * Return the new (latest) version even when we don't create a new revision because the editor that sent the save request might have an outdated version number. This can happen if two users save the same document state, either because they didn't modify the document or because they made the same modifications. * Update since version * Fix RealtimeEditToolbar#waitForConcurrentEditingWarning() that was causing both the existing #editFullScreen() test and the new #preventEmptyRevisions() test to flicker (we need to wait for the fade-in / fade-out popover animation to end). * Display the revision author, not modifier (last content updater), in the history dropdown. * The auto-saver should check if the new version is different from the current version. * Extend the #preventEmptyRevisions() functional test to check what happens when both users save the same document state. * Simplify the version update handler in actionButtons.js * Modify meta.js to not trigger the 'xwiki:document:changeVersion' event if the version didn't change. --- .../java/com/xpn/xwiki/web/SaveAction.java | 10 ++- .../realtime/test/po/HistoryDropdown.java | 2 +- .../realtime/test/po/RealtimeEditToolbar.java | 21 +++-- .../realtime/test/po/VersionElement.java | 2 +- .../src/main/webjar/saver.js | 14 +-- .../test/ui/RealtimeWYSIWYGEditorIT.java | 90 ++++++++++++++++++- .../js/xwiki/actionbuttons/actionButtons.js | 34 ++----- .../main/webapp/resources/js/xwiki/meta.js | 12 +-- 8 files changed, 132 insertions(+), 53 deletions(-) diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java index 445c69d8cbc2..fa42f50d63f2 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/SaveAction.java @@ -307,14 +307,18 @@ && isConflictingWithVersion(context, originalDoc, tdoc)) { // We take the version summary comment from the document being saved because it may have been modified by an // event listener after it was copied from the EditForm by the call to readFromForm. xwiki.saveDocument(tdoc, tdoc.getComment(), tdoc.isMinorEdit(), context); - - // Return the new version number to the editor that triggered the save. - jsonAnswer.put("newVersion", tdoc.getRCSVersion().toString()); } else { // Let the editor know that the document was not saved because there were no changes. jsonAnswer.put("noChanges", "true"); } + // Return the latest version number to the editor that triggered the save. We do this even if we didn't create a + // new revision because the editor may have an outdated version number. This can happen for instance if two + // users save the same document state one after the other (either because both didn't make any changes, or + // because both made the same changes). Having an up-to-date version number is important for properly detecting + // and handling merge conflicts on save. + jsonAnswer.put("newVersion", tdoc.getRCSVersion().toString()); + // Cleanup temporary attachments that were uploaded while editing the document. We do this even if we didn't // create a new revision (e.g. if there were no changes) because those attachments are not needed anymore (e.g. // the user may have discarded the changes that lead to the upload of those attachments). diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java index e05d2c11b796..0b2d8bc054ec 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/HistoryDropdown.java @@ -31,7 +31,7 @@ * The dropdown that lists recent versions of the edited document. * * @version $Id$ - * @since 17.9.0RC1 + * @since 17.9.0 * @since 17.4.6 * @since 16.10.13 */ diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java index 3babf8175f0f..2783539403d9 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/RealtimeEditToolbar.java @@ -237,15 +237,20 @@ public void sendSaveShortcutKey(boolean wait) public RealtimeEditToolbar waitForConcurrentEditingWarning() { String toggleSelector = "button.realtime-warning[data-toggle=\"popover\"]"; - getDriver().waitUntilElementIsVisible(By.cssSelector(toggleSelector)); String popoverSelector = toggleSelector + " + .popover"; - if (!getDriver().findElementsWithoutWaiting(By.cssSelector(popoverSelector)).isEmpty()) { - // The popover is displayed. Let's hide it as it can cover other UI elements. - WebElement toggle = getDriver().findElementWithoutWaiting(By.cssSelector(toggleSelector)); - // We have to click twice because the popover was displayed without focusing the toggle. - toggle.click(); - toggle.click(); - } + + // Wait for the popover to be fully displayed, because it uses a fade-in animation. + getDriver().waitUntilElementIsVisible(By.cssSelector(popoverSelector + ".fade.in")); + + // Hide the popover because it can cover other UI elements. + WebElement toggle = getDriver().findElementWithoutWaiting(By.cssSelector(toggleSelector)); + // We have to click twice because the popover was displayed without focusing the toggle. + toggle.click(); + toggle.click(); + + // Wait for the popover to be fully hidden, because it uses a fade-out animation. + getDriver().waitUntilCondition(ExpectedConditions.numberOfElementsToBe(By.cssSelector(popoverSelector), 0)); + return this; } diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java index 6fb7bc012ead..efe639395456 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-test-pageobjects/src/main/java/org/xwiki/realtime/test/po/VersionElement.java @@ -27,7 +27,7 @@ * Represents a document version listed in the history dropdown from the realtime editing toolbar. * * @version $Id$ - * @since 17.9.0RC1 + * @since 17.9.0 * @since 17.4.6 * @since 16.10.13 */ diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js index 3fa1eef0e50d..53a1b81fe910 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-webjar/src/main/webjar/saver.js @@ -524,8 +524,8 @@ define('xwiki-realtime-saver', [ number: revision.version, date: new Date(revision.modified).getTime(), author: { - reference: this._getAbsoluteUserReference(revision.modifier), - name: revision.modifierName + reference: this._getAbsoluteUserReference(revision.author), + name: revision.authorName } }); }).catch(error => { @@ -665,13 +665,13 @@ define('xwiki-realtime-saver', [ } _afterSave({newVersion}) { - if (newVersion === '1.1') { + if (newVersion === xwikiDocument.version) { + // The version didn't change because the document hasn't been modified. + return; + } else if (newVersion === '1.1') { debug('Created document version 1.1'); - } else if (newVersion) { - debug(`Version bumped from ${xwikiDocument.version} to ${newVersion}.`); } else { - // There's no new version because there were no changes. - return; + debug(`Version bumped from ${xwikiDocument.version} to ${newVersion}.`); } this._state.version = newVersion; this._config.onCreateVersion({ diff --git a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java index c3a2bf1eac1d..59c911c9f37e 100644 --- a/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java +++ b/xwiki-platform-core/xwiki-platform-realtime/xwiki-platform-realtime-wysiwyg/xwiki-platform-realtime-wysiwyg-test/xwiki-platform-realtime-wysiwyg-test-docker/src/test/it/org/xwiki/realtime/wysiwyg/test/ui/RealtimeWYSIWYGEditorIT.java @@ -2434,8 +2434,7 @@ void preventEmptyRevisions(TestUtils setup, TestReference testReference, MultiUs assertEquals(2, versions.size()); firstTextArea.sendKeys(Keys.END, " two"); - ViewPage viewPage = firstEditPage.clickDone(); - assertEquals("one two", viewPage.getContent()); + assertEquals("one two", firstEditPage.clickDone().getContent()); // // Second Tab @@ -2497,6 +2496,93 @@ void preventEmptyRevisions(TestUtils setup, TestReference testReference, MultiUs assertEquals("3.1", historyPane.getCurrentVersion()); assertEquals("superadmin", historyPane.getCurrentAuthor()); assertEquals("Another empty revision", historyPane.getCurrentVersionComment()); + + // + // Second Tab + // + + multiUserSetup.switchToBrowserTab(secondTabHandle); + // Edit again to verify that the latest version is returned even when there are no changes. The plan is to save + // the same document state with both users, before making any changes and after making the same changes. + inplaceEditablePage = RealtimeInplaceEditablePage.gotoPage(testReference); + inplaceEditablePage.editInplace(); + secondEditor = new RealtimeCKEditor(); + secondTextArea = secondEditor.getRichTextArea(); + + // + // First Tab + // + + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + firstEditPage = RealtimeWYSIWYGEditPage.gotoPage(testReference); + firstEditor = firstEditPage.getContenEditor(); + firstTextArea = firstEditor.getRichTextArea(); + + // Verify that there is a single revision listed in the history dropdown. + versions = historyDropdown.open().getVersions(); + assertEquals(1, versions.size()); + assertEquals("3.1", versions.get(0).getNumber()); + assertEquals("Su", versions.get(0).getAuthor().getAbbreviation()); + + // + // Second Tab + // + + multiUserSetup.switchToBrowserTab(secondTabHandle); + // Focus the editing area to get the floating toolbar. + secondTextArea.click(); + secondEditor.getToolBar().toggleSourceMode(); + // Close the concurrent editing warning popover because it may cover other UI elements (like the Save button if + // the window width is too small for the toolbar to fit in a single line). + inplaceEditablePage.getToolbar().waitForConcurrentEditingWarning(); + // Save without making any changes. + inplaceEditablePage.save(); + + // + // First Tab + // + + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + // Save without making any changes. + firstEditPage.getToolbar().sendSaveShortcutKey(); + // Verify that we have two revisions listed in the history dropdown. + versions = historyDropdown.open().getVersions(); + assertEquals(2, versions.size()); + assertEquals("3.2", versions.get(1).getNumber()); + // The save action doesn't return the latest revision author so we assume the current user is the author. + assertEquals("Su", versions.get(1).getAuthor().getAbbreviation()); + + // + // Second Tab + // + + multiUserSetup.switchToBrowserTab(secondTabHandle); + // Make some changes and save. We'll do the same changes in the first tab afterwards. + secondEditor.getSourceTextArea().sendKeys(Keys.END, " three"); + inplaceEditablePage.save(); + + // + // First Tab + // + + multiUserSetup.switchToBrowserTab(multiUserSetup.getFirstTabHandle()); + // Make the same changes and save. + firstTextArea.sendKeys(Keys.END, " three"); + firstEditPage.getToolbar().sendSaveShortcutKey(); + // Verify that we have three revisions listed in the history dropdown. + versions = historyDropdown.open().getVersions(); + assertEquals(3, versions.size()); + assertEquals("3.3", versions.get(2).getNumber()); + // Again, the current user is not the real author of the new revision but the save action doesn't help us. + assertEquals("Su", versions.get(2).getAuthor().getAbbreviation()); + + // Make some more changes to verify we don't get a merge conflict. + firstTextArea.clear(); + firstTextArea.sendKeys("final"); + historyPane = firstEditPage.clickDone().openHistoryDocExtraPane().showMinorEdits(); + assertEquals(8, historyPane.getNumberOfVersions()); + assertEquals("4.1", historyPane.getCurrentVersion()); + assertEquals("superadmin", historyPane.getCurrentAuthor()); } private void setMultiLingual(TestUtils setup, boolean isMultiLingual, String... supportedLanguages) diff --git a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js index d77f5329a098..453dfb477e7f 100644 --- a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js +++ b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/actionbuttons/actionButtons.js @@ -23,24 +23,15 @@ var XWiki = (function(XWiki) { // Start XWiki augmentation. - var actionButtons = XWiki.actionButtons = XWiki.actionButtons || {}; - - // we need to handle the creation of document - var currentDocument; - var editingVersionDateField = $('editingVersionDate'); - var previousVersionField = $('previousVersion'); - var isNewField = $('isNew'); - - var refreshVersion = function (event) { - if (currentDocument.equals(event.memo.documentReference)) { - if (previousVersionField) { - previousVersionField.setValue(event.memo.version); - } - if (isNewField) { - isNewField.setValue(false); - } + const actionButtons = XWiki.actionButtons = XWiki.actionButtons || {}; + + function refreshVersion(event) { + if (XWiki.currentDocument.documentReference.equals(event.memo.documentReference)) { + $('previousVersion')?.setValue(event.memo.version); + $('editingVersionDate')?.setValue(Date.now()); + $('isNew')?.setValue(false); } - }; + } /** * Allow custom validation messages to be set on the validated field usin data attributes. @@ -408,11 +399,6 @@ var XWiki = (function(XWiki) { require(['xwiki-meta'], function (xm) { xm.setVersion(response.responseJSON.newVersion); }); - - // We only update this field since the other ones are updated by the callback of setVersion. - if (editingVersionDateField) { - editingVersionDateField.setValue(Date.now()); - } } // Announce that the document has been saved. @@ -808,10 +794,6 @@ var XWiki = (function(XWiki) { }); function init() { - require(['xwiki-meta'], function (xm) { - currentDocument = xm.documentReference; - }); - new actionButtons.EditActions(); new actionButtons.AjaxSaveAndContinue(); return true; diff --git a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/meta.js b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/meta.js index b9399292649b..693170ffbc27 100644 --- a/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/meta.js +++ b/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/meta.js @@ -74,11 +74,13 @@ define(['jquery', 'xwiki-entityReference', 'xwiki-events-bridge'], function($, X } setVersion(newVersion) { - this.version = newVersion; - $(document).trigger('xwiki:document:changeVersion', { - 'version': this.version, - 'documentReference': this.documentReference - }); + if (newVersion !== this.version) { + this.version = newVersion; + $(document).trigger('xwiki:document:changeVersion', { + 'version': this.version, + 'documentReference': this.documentReference + }); + } } /**