From f22909904c98a03267b8738b442c531ef7622b9d Mon Sep 17 00:00:00 2001
From: Ben Rohlfs <brohlfs@google.com>
Date: Fri, 7 Feb 2020 14:56:12 +0100
Subject: [PATCH] Fix issues with caching edited commit message

As described in issue 12031 it can happen that content restored from
local browser storage overwrites valuable content. This change prevents
this from happening by only restoring from local browser storage on
*first* edit.

Using local storage as a browser wide backup for drafts is still not
fool proof, see added comments in gr-editable-content.js. But we rarely
get reports about this being an issue, so we believe that this fix is
enough for the moment.

Bug: Issue 12031
Change-Id: Ie34410a16cda8f76c7979804dd924c0bdc367d6c
(cherry picked from commit d839910b25cbc1dd1a9a387adafb01ec5743f20f)
---
 .../gr-editable-content.js                    | 28 +++++++++++++++++--
 .../gr-editable-content_test.html             |  6 ++--
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js
index 482cacf11f..e5c58e0ba2 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js
@@ -81,13 +81,34 @@
         if (newContent.length) {
           this.$.storage.setEditableContentItem(this.storageKey, newContent);
         } else {
+          // This does not really happen, because we don't clear newContent
+          // after saving (see below). So this only occurs when the user clears
+          // all the content in the editable textarea. But <gr-storage> cleans
+          // up itself after one day, so we are not so concerned about leaving
+          // some garbage behind.
           this.$.storage.eraseEditableContentItem(this.storageKey);
         }
       }, STORAGE_DEBOUNCE_INTERVAL_MS);
     },
 
     _editingChanged(editing) {
-      if (!editing) { return; }
+      // This method is for initializing _newContent when you start editing.
+      // Restoring content from local storage is not perfect and has
+      // some issues:
+      //
+      // 1. When you start editing in multiple tabs, then we are vulnerable to
+      // race conditions between the tabs.
+      // 2. The stored content is keyed by revision, so when you upload a new
+      // patchset and click "reload" and then click "cancel" on the content-
+      // editable, then you won't be able to recover the content anymore.
+      //
+      // Because of these issues we believe that it is better to only recover
+      // content from local storage when you enter editing mode for the first
+      // time. Otherwise it is better to just keep the last editing state from
+      // the same session.
+      if (!editing || this._newContent) {
+        return;
+      }
 
       let content;
       if (this.storageKey) {
@@ -110,12 +131,15 @@
     },
 
     _computeSaveDisabled(disabled, content, newContent) {
-      return disabled || (content === newContent);
+      return disabled || !newContent || content === newContent;
     },
 
     _handleSave(e) {
       e.preventDefault();
       this.fire('editable-content-save', {content: this._newContent});
+      // It would be nice, if we would set this._newContent = undefined here,
+      // but we can only do that when we are sure that the save operation has
+      // succeeded.
     },
 
     _handleCancel(e) {
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html
index cc44d9b6d0..ca500cbd21 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html
@@ -62,11 +62,11 @@ limitations under the License.
       MockInteractions.tap(element.$$('gr-button:not([primary])'));
     });
 
-    test('enabling editing updates edit field contents', () => {
+    test('enabling editing keeps old content', () => {
       element.content = 'current content';
-      element._newContent = 'stale content';
+      element._newContent = 'old content';
       element.editing = true;
-      assert.equal(element._newContent, 'current content');
+      assert.equal(element._newContent, 'old content');
     });
 
     test('disabling editing does not update edit field contents', () => {