opendev/gerrit
Code Issues Proposed changes

Fix issues with caching edited commit message

Browse Source 
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 d839910b25)
This commit is contained in:
Ben Rohlfs
2020-02-07 14:56:12 +01:00
committed by Paladox none
parent edd57a21d6
commit f22909904c
2 changed files with 29 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js
View File

@@ -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) {

6
polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html
View File

@@ -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', () => {