Show respectful review tip when user writes comments
With a lock-up Math.max(3 days, period between creating comments),
we show this tip with a 30% probability.
When show up, it will pick tip randomly from a pre-defined list.
Once dismissed, it will add another 3 more lock up time before showing up again.
Also added interaction reports for it.
Bug: Issue 11441
Change-Id: I6baffb7369bb2dd59716f496c389971dc7050cc8
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
index ea6c0de..6e5eefd 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
@@ -227,6 +227,29 @@
user-select: none;
}
+ .respectfulReviewTip {
+ justify-content: space-between;
+ display: flex;
+ padding: var(--spacing-m);
+ border: 1px solid var(--border-color);
+ border-radius: var(--border-radius);
+ margin-bottom: var(--spacing-m);
+ }
+ .respectfulReviewTip div {
+ display: flex;
+ }
+ .respectfulReviewTip div iron-icon {
+ margin-right: var(--spacing-s);
+ }
+ .respectfulReviewTip a {
+ white-space: nowrap;
+ margin-right: var(--spacing-s);
+ padding-left: var(--spacing-m);
+ text-decoration: none;
+ }
+ .pointer {
+ cursor: pointer;
+ }
</style>
<div id="container" class="container">
<div class="header" id="header" on-click="_handleToggleCollapsed">
@@ -289,6 +312,30 @@
disabled="{{disabled}}"
rows="4"
text="{{_messageText}}"></gr-textarea>
+ <template is="dom-if" if="[[_computeVisibilityOfTip(_showRespectfulTip, _respectfulTipDismissed)]]">
+ <div class="respectfulReviewTip">
+ <div>
+ <gr-tooltip-content
+ has-tooltip
+ title="Tips for respectful code reviews.">
+ <iron-icon class="pointer" icon="gr-icons:lightbulb-outline"></iron-icon>
+ </gr-tooltip-content>
+ [[_respectfulReviewTip]]
+ </div>
+ <div>
+ <a
+ on-click="_onRespectfulReadMoreClick"
+ href="https://testing.googleblog.com/2019/11/code-health-respectful-reviews-useful.html"
+ target="_blank">
+ Read more
+ </a>
+ <iron-icon
+ class="close pointer"
+ on-click="_dismissRespectfulTip"
+ icon="gr-icons:close"></iron-icon>
+ </div>
+ </div>
+ </template>
</template>
<!--The message class is needed to ensure selectability from
gr-diff-selection.-->
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js
index 3f4cc69..91c88af 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js
@@ -32,6 +32,22 @@
const FILE = 'FILE';
/**
+ * All candidates tips to show, will pick randomly.
+ */
+ const RESPECTFUL_REVIEW_TIPS= [
+ 'DO: Assume competence.',
+ 'DO: Provide rationale or context.',
+ 'DO: Consider how comments may be interpreted.',
+ 'DON’T: Criticize the person.',
+ 'DON’T: Use harsh language.',
+ 'DO: Provide specific and actionable feedback.',
+ 'DO: Clearly mark nitpicks and optional comments.',
+ 'DO: Clarify code or reply to the reviewer’s comment.',
+ 'DO: When disagreeing with feedback, explain the advantage' +
+ ' of your approach.',
+ ];
+
+ /**
* @appliesMixin Gerrit.FireMixin
* @appliesMixin Gerrit.KeyboardShortcutMixin
* @extends Polymer.Element
@@ -167,6 +183,16 @@
type: Object,
value: () => { return {}; },
},
+
+ _showRespectfulTip: {
+ type: Boolean,
+ value: false,
+ },
+ _respectfulReviewTip: String,
+ _respectfulTipDismissed: {
+ type: Boolean,
+ value: false,
+ },
};
}
@@ -177,6 +203,7 @@
'_isRobotComment(comment)',
'_calculateActionstoShow(showActions, isRobotComment)',
'_computeHasHumanReply(comment, comments.*)',
+ '_onEditingChange(editing)',
];
}
@@ -209,6 +236,50 @@
}
}
+ _onEditingChange(editing) {
+ if (!editing) return;
+ // visibility based on cache this will make sure we only and always show
+ // a tip once every Math.max(a day, period between creating comments)
+ const cachedVisibilityOfRespectfulTip =
+ this.$.storage.getRespectfulTipVisibility();
+ if (!cachedVisibilityOfRespectfulTip) {
+ // we still want to show the tip with a probability of 30%
+ if (this.getRandomNum(0, 3) >= 1) return;
+ this._showRespectfulTip = true;
+ const randomIdx = this.getRandomNum(0, RESPECTFUL_REVIEW_TIPS.length);
+ this._respectfulReviewTip = RESPECTFUL_REVIEW_TIPS[randomIdx];
+ this.$.reporting.reportInteraction(
+ 'respectful-tip-appeared',
+ {tip: this._respectfulReviewTip}
+ );
+ // update cache
+ this.$.storage.setRespectfulTipVisibility();
+ }
+ }
+
+ /** Set as a separate method so easy to stub. */
+ getRandomNum(min, max) {
+ return Math.floor(Math.random() * (max - min) + min);
+ }
+
+ _computeVisibilityOfTip(showTip, tipDismissed) {
+ return showTip && !tipDismissed;
+ }
+
+ _dismissRespectfulTip() {
+ this._respectfulTipDismissed = true;
+ this.$.reporting.reportInteraction(
+ 'respectful-tip-dismissed',
+ {tip: this._respectfulReviewTip}
+ );
+ // add a 3 day delay to the tip cache
+ this.$.storage.setRespectfulTipVisibility(/* delayDays= */ 3);
+ }
+
+ _onRespectfulReadMoreClick() {
+ this.$.reporting.reportInteraction('respectful-read-more-clicked');
+ }
+
get textarea() {
return this.shadowRoot.querySelector('#editTextarea');
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.html b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.html
index 8bc9518..6d0a5e5 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.html
@@ -1031,5 +1031,153 @@
MockInteractions.tap(element.$$('.show-fix'));
});
});
+
+ suite('respectful tips', () => {
+ let element;
+ let sandbox;
+ let clock;
+ setup(() => {
+ stub('gr-rest-api-interface', {
+ getAccount() { return Promise.resolve(null); },
+ });
+ clock = sinon.useFakeTimers();
+ sandbox = sinon.sandbox.create();
+ });
+
+ teardown(() => {
+ clock.restore();
+ sandbox.restore();
+ });
+
+ test('show tip when no cached record', done => {
+ // fake stub for storage
+ const respectfulGetStub = sinon.stub();
+ const respectfulSetStub = sinon.stub();
+ stub('gr-storage', {
+ getRespectfulTipVisibility() { return respectfulGetStub(); },
+ setRespectfulTipVisibility() { return respectfulSetStub(); },
+ });
+ respectfulGetStub.returns(null);
+ element = fixture('draft');
+ // fake random
+ element.getRandomNum = () => 0;
+ element.comment = {__editing: true};
+ flush(() => {
+ assert.isTrue(respectfulGetStub.called);
+ assert.isTrue(respectfulSetStub.called);
+ assert.isTrue(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+ done();
+ });
+ });
+
+ test('add 3 day delays once dismissed', done => {
+ // fake stub for storage
+ const respectfulGetStub = sinon.stub();
+ const respectfulSetStub = sinon.stub();
+ stub('gr-storage', {
+ getRespectfulTipVisibility() { return respectfulGetStub(); },
+ setRespectfulTipVisibility(days) { return respectfulSetStub(days); },
+ });
+ respectfulGetStub.returns(null);
+ element = fixture('draft');
+ // fake random
+ element.getRandomNum = () => 0;
+ element.comment = {__editing: true};
+ flush(() => {
+ assert.isTrue(respectfulGetStub.called);
+ assert.isTrue(respectfulSetStub.called);
+ assert.isTrue(respectfulSetStub.lastCall.args[0] === undefined);
+ assert.isTrue(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+
+ MockInteractions.tap(element.shadowRoot
+ .querySelector('.respectfulReviewTip .close'));
+ flushAsynchronousOperations();
+ assert.isTrue(respectfulSetStub.lastCall.args[0] === 3);
+ done();
+ });
+ });
+
+ test('do not show tip when fall out of probability', done => {
+ // fake stub for storage
+ const respectfulGetStub = sinon.stub();
+ const respectfulSetStub = sinon.stub();
+ stub('gr-storage', {
+ getRespectfulTipVisibility() { return respectfulGetStub(); },
+ setRespectfulTipVisibility() { return respectfulSetStub(); },
+ });
+ respectfulGetStub.returns(null);
+ element = fixture('draft');
+ // fake random
+ element.getRandomNum = () => 3;
+ element.comment = {__editing: true};
+ flush(() => {
+ assert.isTrue(respectfulGetStub.called);
+ assert.isFalse(respectfulSetStub.called);
+ assert.isFalse(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+ done();
+ });
+ });
+
+ test('show tip when editing changed to true', done => {
+ // fake stub for storage
+ const respectfulGetStub = sinon.stub();
+ const respectfulSetStub = sinon.stub();
+ stub('gr-storage', {
+ getRespectfulTipVisibility() { return respectfulGetStub(); },
+ setRespectfulTipVisibility() { return respectfulSetStub(); },
+ });
+ respectfulGetStub.returns(null);
+ element = fixture('draft');
+ // fake random
+ element.getRandomNum = () => 0;
+ element.comment = {__editing: false};
+ flush(() => {
+ assert.isFalse(respectfulGetStub.called);
+ assert.isFalse(respectfulSetStub.called);
+ assert.isFalse(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+
+ element.editing = true;
+ flush(() => {
+ assert.isTrue(respectfulGetStub.called);
+ assert.isTrue(respectfulSetStub.called);
+ assert.isTrue(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+ done();
+ });
+ });
+ });
+
+ test('no tip when cached record', done => {
+ // fake stub for storage
+ const respectfulGetStub = sinon.stub();
+ const respectfulSetStub = sinon.stub();
+ stub('gr-storage', {
+ getRespectfulTipVisibility() { return respectfulGetStub(); },
+ setRespectfulTipVisibility() { return respectfulSetStub(); },
+ });
+ respectfulGetStub.returns({});
+ element = fixture('draft');
+ // fake random
+ element.getRandomNum = () => 0;
+ element.comment = {__editing: true};
+ flush(() => {
+ assert.isTrue(respectfulGetStub.called);
+ assert.isFalse(respectfulSetStub.called);
+ assert.isFalse(
+ !!element.shadowRoot.querySelector('.respectfulReviewTip')
+ );
+ done();
+ });
+ });
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
index d36ce3b..fb8f5f8 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
@@ -60,6 +60,8 @@
<g id="comment"><path d="M21.99 4c0-1.1-.89-2-1.99-2H4c-1.1 0-2 .9-2 2v12c0 1.1.9 2 2 2h14l4 4-.01-18z"/><path d="M0 0h24v24H0z" fill="none"/></g>
<!-- This SVG is a copy from iron-icons https://github.com/PolymerElements/iron-icons/blob/master/iron-icons.js -->
<g id="error"><path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-2h2v2zm0-4h-2V7h2v6z"></path></g>
+ <!-- This SVG is a copy from iron-icons https://github.com/PolymerElements/iron-icons/blob/master/iron-icons.js -->
+ <g id="lightbulb-outline"><path d="M9 21c0 .55.45 1 1 1h4c.55 0 1-.45 1-1v-1H9v1zm3-19C8.14 2 5 5.14 5 9c0 2.38 1.19 4.47 3 5.74V17c0 .55.45 1 1 1h6c.55 0 1-.45 1-1v-2.26c1.81-1.27 3-3.36 3-5.74 0-3.86-3.14-7-7-7zm2.85 11.1l-.85.6V16h-4v-2.3l-.85-.6C7.8 12.16 7 10.63 7 9c0-2.76 2.24-5 5-5s5 2.24 5 5c0 1.63-.8 3.16-2.15 4.1z"></path></g>
<!-- This is a custom PolyGerrit SVG -->
<g id="side-by-side"><path d="M17.1578947,10.8888889 L2.84210526,10.8888889 C2.37894737,10.8888889 2,11.2888889 2,11.7777778 L2,17.1111111 C2,17.6 2.37894737,18 2.84210526,18 L17.1578947,18 C17.6210526,18 18,17.6 18,17.1111111 L18,11.7777778 C18,11.2888889 17.6210526,10.8888889 17.1578947,10.8888889 Z M17.1578947,2 L2.84210526,2 C2.37894737,2 2,2.4 2,2.88888889 L2,8.22222222 C2,8.71111111 2.37894737,9.11111111 2.84210526,9.11111111 L17.1578947,9.11111111 C17.6210526,9.11111111 18,8.71111111 18,8.22222222 L18,2.88888889 C18,2.4 17.6210526,2 17.1578947,2 Z M16.1973628,2 L2.78874238,2 C2.35493407,2 2,2.4 2,2.88888889 L2,8.22222222 C2,8.71111111 2.35493407,9.11111111 2.78874238,9.11111111 L16.1973628,9.11111111 C16.6311711,9.11111111 16.9861052,8.71111111 16.9861052,8.22222222 L16.9861052,2.88888889 C16.9861052,2.4 16.6311711,2 16.1973628,2 Z" id="Shape" transform="scale(1.2) translate(10.000000, 10.000000) rotate(-90.000000) translate(-10.000000, -10.000000)"/></g>
<!-- This is a custom PolyGerrit SVG -->
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
index 53311b5..8cc9de9 100644
--- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
+++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
@@ -17,16 +17,17 @@
(function() {
'use strict';
- // Date cutoff is one day:
- const CLEANUP_MAX_AGE = 24 * 60 * 60 * 1000;
+ const DURATION_DAY = 24 * 60 * 60 * 1000;
// Clean up old entries no more frequently than one day.
- const CLEANUP_THROTTLE_INTERVAL = 24 * 60 * 60 * 1000;
+ const CLEANUP_THROTTLE_INTERVAL = DURATION_DAY;
- const CLEANUP_PREFIXES = [
- 'draft:',
- 'editablecontent:',
- ];
+ const CLEANUP_PREFIXES_MAX_AGE_MAP = {
+ // respectfultip has a 3 day expiration
+ 'respectfultip:': 3 * DURATION_DAY,
+ 'draft:': DURATION_DAY,
+ 'editablecontent:': DURATION_DAY,
+ };
/** @extends Polymer.Element */
class GrStorage extends Polymer.GestureEventListeners(
@@ -76,6 +77,19 @@
{message, updated: Date.now()});
}
+ getRespectfulTipVisibility() {
+ this._cleanupItems();
+ return this._getObject('respectfultip:visibility');
+ }
+
+ setRespectfulTipVisibility(delayDays = 0) {
+ this._cleanupItems();
+ this._setObject(
+ 'respectfultip:visibility',
+ {updated: Date.now() + delayDays * DURATION_DAY}
+ );
+ }
+
eraseEditableContentItem(key) {
this._storage.removeItem(this._getEditableContentKey(key));
}
@@ -106,18 +120,17 @@
this._lastCleanup = Date.now();
let item;
- for (const key in this._storage) {
- if (!this._storage.hasOwnProperty(key)) { continue; }
- for (const prefix of CLEANUP_PREFIXES) {
+ Object.keys(this._storage).forEach(key => {
+ Object.keys(CLEANUP_PREFIXES_MAX_AGE_MAP).forEach(prefix => {
if (key.startsWith(prefix)) {
item = this._getObject(key);
- if (Date.now() - item.updated > CLEANUP_MAX_AGE) {
+ const expiration = CLEANUP_PREFIXES_MAX_AGE_MAP[prefix];
+ if (Date.now() - item.updated > expiration) {
this._storage.removeItem(key);
}
- break;
}
- }
- }
+ });
+ });
}
_getObject(key) {
