opendev/gerrit
Code Issues Proposed changes

Cleanup of diff comment draft saving work

Browse Source 
- Refactors the _editDraft property of gr-diff-comment to _messageText.
- Renames the gr-storage element as instantiated in gr-diff-comment from
  "localStorage" to "storage".
- The gr-storage diff comment functions are refactored to include
  "comment" in their names and to accept a location object rather than 4
  separate arguments.
- Added a comment describing the use of a promise in the _loadLocalDraft
  method of gr-diff-comment.
- Throttled the invocation of the _cleanupDrafts method of gr-storage to
  avoid potentially expensive crawls over all localStorage entries.
- Various other smaller fixes.

Bug: Issue 3787
Change-Id: Idf96051c0d56d6ce8b15f55ca2680363bd1ca805
This commit is contained in:
Wyatt Allen 2016-05-23 13:53:10 -07:00
parent 2760e1d9e7
commit 035c74f508
6 changed files with 85 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
View File

@ -129,7 +129,7 @@ limitations under the License.
class="editMessage"
disabled="{{disabled}}"
rows="4"
bind-value="{{_editDraft}}"
bind-value="{{_messageText}}"
on-keydown="_handleTextareaKeydown"></iron-autogrow-textarea>
<gr-linked-text class="message"
pre
@ -141,7 +141,7 @@ limitations under the License.
<gr-button class="action done" on-tap="_handleDone">Done</gr-button>
<gr-button class="action edit" on-tap="_handleEdit">Edit</gr-button>
<gr-button class="action save" on-tap="_handleSave"
disabled$="[[_computeSaveDisabled(_editDraft)]]">Save</gr-button>
disabled$="[[_computeSaveDisabled(_messageText)]]">Save</gr-button>
<gr-button class="action cancel" on-tap="_handleCancel" hidden>Cancel</gr-button>
<div class="danger">
<gr-button class="action discard" on-tap="_handleDiscard">Discard</gr-button>
@ -149,7 +149,7 @@ limitations under the License.
</div>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="localStorage"></gr-storage>
<gr-storage id="storage"></gr-storage>
</template>
<script src="gr-diff-comment.js"></script>
</dom-module>

60
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
View File

@ -69,25 +69,29 @@
projectConfig: Object,
_xhrPromise: Object, // Used for testing.
_editDraft: {
_messageText: {
type: String,
observer: '_editDraftChanged',
observer: '_messageTextChanged',
},
},
ready: function() {
this._loadLocalDraft().then(function(loadedLocal) {
this._editDraft = (this.comment && this.comment.message) || '';
this.editing = !this._editDraft.length || loadedLocal;
this._messageText = (this.comment && this.comment.message) || '';
this.editing = !this._messageText.length || loadedLocal;
}.bind(this));
},
save: function() {
this.comment.message = this._editDraft;
this.comment.message = this._messageText;
this.disabled = true;
this.$.localStorage.eraseDraft(this.changeNum, this.patchNum,
this.comment.path, this.comment.line);
this.$.storage.eraseDraftComment({
changeNum: this.changeNum,
patchNum: this.patchNum,
path: this.comment.path,
line: this.comment.line,
});
this._xhrPromise = this._saveDraft(this.comment).then(function(response) {
this.disabled = false;
@ -147,23 +151,27 @@
}
},
_editDraftChanged: function(newValue, oldValue) {
_messageTextChanged: function(newValue, oldValue) {
if (this.comment && this.comment.id) { return; }
this.debounce('store', function() {
var message = this._editDraft;
var message = this._messageText;
// If the draft has been modified to be empty, then erase the storage
// entry.
if ((!this._editDraft || !this._editDraft.length) && oldValue) {
this.$.localStorage.eraseDraft(this.changeNum, this.patchNum,
this.comment.path, this.comment.line);
return;
var commentLocation = {
changeNum: this.changeNum,
patchNum: this.patchNum,
path: this.comment.path,
line: this.comment.line,
};
if ((!this._messageText || !this._messageText.length) && oldValue) {
// If the draft has been modified to be empty, then erase the storage
// entry.
this.$.storage.eraseDraftComment(commentLocation);
} else {
this.$.storage.setDraftComment(commentLocation, message);
}
this.$.localStorage.setDraft(this.changeNum, this.patchNum,
this.comment.path, this.comment.line, message);
}.bind(this), STORAGE_DEBOUNCE_INTERVAL);
}, STORAGE_DEBOUNCE_INTERVAL);
},
_handleLinkTap: function(e) {
@ -195,7 +203,7 @@
_handleEdit: function(e) {
this._preventDefaultAndBlur(e);
this._editDraft = this.comment.message;
this._messageText = this.comment.message;
this.editing = true;
},
@ -210,7 +218,7 @@
this.fire('comment-discard');
return;
}
this._editDraft = this.comment.message;
this._messageText = this.comment.message;
this.editing = false;
},
@ -252,6 +260,8 @@
},
_loadLocalDraft: function() {
// Use an async promise to avoid blocking render on potentially slow
// localStorage calls.
return new Promise(function(resolve) {
this.async(function() {
// Only apply local drafts to comments that haven't been saved
@ -261,8 +271,12 @@
return;
}
var draft = this.$.localStorage.getDraft(this.changeNum,
this.patchNum, this.comment.path, this.comment.line);
var draft = this.$.storage.getDraftComment({
changeNum: this.changeNum,
patchNum: this.patchNum,
path: this.comment.path,
line: this.comment.line,
});
if (draft) {
this.comment.message = draft.message;

8
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
View File

@ -183,11 +183,11 @@ limitations under the License.
MockInteractions.tap(element.$$('.edit'));
assert.isTrue(element.editing);
element._editDraft = '';
element._messageText = '';
// Save should be disabled on an empty message.
var disabled = element.$$('.save').hasAttribute('disabled');
assert.isTrue(disabled, 'save button should be disabled.');
element._editDraft = ' ';
element._messageText = ' ';
disabled = element.$$('.save').hasAttribute('disabled');
assert.isTrue(disabled, 'save button should be disabled.');
@ -206,7 +206,7 @@ limitations under the License.
test('draft saving/editing', function(done) {
element.draft = true;
MockInteractions.tap(element.$$('.edit'));
element._editDraft = 'good news, everyone!';
element._messageText = 'good news, everyone!';
MockInteractions.tap(element.$$('.save'));
assert.isTrue(element.disabled,
'Element should be disabled when creating draft.');
@ -218,7 +218,7 @@ limitations under the License.
assert.isFalse(element.editing);
}).then(function() {
MockInteractions.tap(element.$$('.edit'));
element._editDraft = 'You’ll be delivering a package to Chapek 9, a ' +
element._messageText = 'You’ll be delivering a package to Chapek 9, a ' +
'world where humans are killed on sight.';
MockInteractions.tap(element.$$('.save'));
assert.isTrue(element.disabled,

1
polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html
View File

@ -15,6 +15,5 @@ limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<dom-module id="gr-storage">
<template></template>
<script src="gr-storage.js"></script>
</dom-module>

29
polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
View File

@ -17,10 +17,14 @@
// Date cutoff is one day:
var DRAFT_MAX_AGE = 24*60*60*1000;
// Clean up old entries no more frequently than one day.
var CLEANUP_THROTTLE_INTERVAL = 24*60*60*1000;
Polymer({
is: 'gr-storage',
properties: {
_lastCleanup: Number,
_storage: {
type: Object,
value: function() {
@ -29,27 +33,34 @@
},
},
getDraft: function(changeNum, patchNum, path, line) {
getDraftComment: function(location) {
this._cleanupDrafts();
return this._getObject(
this._getDraftKey(changeNum, patchNum, path, line));
return this._getObject(this._getDraftKey(location));
},
setDraft: function(changeNum, patchNum, path, line, message) {
var key = this._getDraftKey(changeNum, patchNum, path, line);
setDraftComment: function(location, message) {
var key = this._getDraftKey(location);
this._setObject(key, {message: message, updated: Date.now()});
},
eraseDraft: function(changeNum, patchNum, path, line) {
var key = this._getDraftKey(changeNum, patchNum, path, line);
eraseDraftComment: function(location) {
var key = this._getDraftKey(location);
this._storage.removeItem(key);
},
_getDraftKey: function(changeNum, patchNum, path, line) {
return ['draft', changeNum, patchNum, path, line].join(':');
_getDraftKey: function(location) {
return ['draft', location.changeNum, location.patchNum, location.path,
location.line].join(':');
},
_cleanupDrafts: function() {
// Throttle cleanup to the throttle interval.
if (this._lastCleanup &&
Date.now() - this._lastCleanup < CLEANUP_THROTTLE_INTERVAL) {
return;
}
this._lastCleanup = Date.now();
var draft;
for (var key in this._storage) {
if (key.indexOf('draft:') === 0) {

27
polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html
View File

@ -51,23 +51,29 @@ limitations under the License.
var patchNum = 5;
var path = 'my_source_file.js';
var line = 123;
var location = {
changeNum: changeNum,
patchNum: patchNum,
path: path,
line: line,
};
// The key is in the expected format.
var key = element._getDraftKey(changeNum, patchNum, path, line);
var key = element._getDraftKey(location);
assert.equal(key, ['draft', changeNum, patchNum, path, line].join(':'));
// There should be no draft initially.
var draft = element.getDraft(changeNum, patchNum, path, line);
var draft = element.getDraftComment(location);
assert.isNotOk(draft);
// Setting the draft stores it under the expected key.
element.setDraft(changeNum, patchNum, path, line, 'my comment');
element.setDraftComment(location, 'my comment');
assert.isOk(storage.getItem(key));
assert.equal(JSON.parse(storage.getItem(key)).message, 'my comment');
assert.isOk(JSON.parse(storage.getItem(key)).updated);
// Erasing the draft removes the key.
element.eraseDraft(changeNum, patchNum, path, line);
element.eraseDraftComment(location);
assert.isNotOk(storage.getItem(key));
cleanupStorage();
@ -78,7 +84,16 @@ limitations under the License.
var patchNum = 5;
var path = 'my_source_file.js';
var line = 123;
var key = element._getDraftKey(changeNum, patchNum, path, line);
var location = {
changeNum: changeNum,
patchNum: patchNum,
path: path,
line: line,
};
var key = element._getDraftKey(location);
// Make sure that the call to cleanup doesn't get throttled.
element._lastCleanup = 0;
var cleanupSpy = sinon.spy(element, '_cleanupDrafts');
@ -89,7 +104,7 @@ limitations under the License.
}));
// Getting the draft should cause it to be removed.
var draft = element.getDraft(changeNum, patchNum, path, line);
var draft = element.getDraftComment(location);
assert.isTrue(cleanupSpy.called);
assert.isNotOk(draft);