opendev
/
gerrit
Code Issues Proposed changes

Merge "Clean up Add Patchset Description from File List header" into stable-3.4

This commit is contained in:
Ben Rohlfs 2021-04-23 17:17:32 +00:00 committed by Gerrit Code Review
parent 91a15fabef 608f452287
commit 9093a638bb
3 changed files with 2 additions and 254 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

127
polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
View File

@ -18,24 +18,17 @@ import '../../../styles/shared-styles';
import '../../diff/gr-diff-mode-selector/gr-diff-mode-selector';
import '../../diff/gr-patch-range-select/gr-patch-range-select';
import '../../edit/gr-edit-controls/gr-edit-controls';
import '../../shared/gr-editable-label/gr-editable-label';
import '../../shared/gr-linked-chip/gr-linked-chip';
import '../../shared/gr-select/gr-select';
import '../../shared/gr-button/gr-button';
import '../../shared/gr-icons/gr-icons';
import '../gr-commit-info/gr-commit-info';
import {dom, EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-file-list-header_html';
import {KeyboardShortcutMixin} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {FilesExpandedState} from '../gr-file-list-constants';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {
computeLatestPatchNum,
getRevisionByPatchNum,
PatchSet,
} from '../../../utils/patch-set-util';
import {property, computed, observe, customElement} from '@polymer/decorators';
import {computeLatestPatchNum, PatchSet} from '../../../utils/patch-set-util';
import {property, customElement} from '@polymer/decorators';
import {
AccountInfo,
ChangeInfo,
@ -51,11 +44,8 @@ import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {GrDiffModeSelector} from '../../diff/gr-diff-mode-selector/gr-diff-mode-selector';
import {ChangeStatus, DiffViewMode} from '../../../constants/constants';
import {GrButton} from '../../shared/gr-button/gr-button';
import {appContext} from '../../../services/app-context';
import {fireEvent} from '../../../utils/event-util';
// Maximum length for patch set descriptions.
const PATCH_DESC_MAX_LENGTH = 500;
const MERGED_STATUS = 'MERGED';
declare global {
@ -154,30 +144,9 @@ export class GrFileListHeader extends KeyboardShortcutMixin(PolymerElement) {
@property({type: Number})
readonly _maxFilesForBulkActions = 225;
@property({type: String})
_patchsetDescription = '';
@property({type: Object})
revisionInfo?: RevisionInfo;
private readonly restApiService = appContext.restApiService;
@computed('loggedIn', 'change', 'account')
get _descriptionReadOnly(): boolean {
if (
this.loggedIn === undefined ||
this.change === undefined ||
this.account === undefined
) {
return true;
}
return !(
this.loggedIn &&
this.account._account_id === this.change.owner._account_id
);
}
setDiffViewMode(mode: DiffViewMode) {
this.$.modeSelect.setMode(mode);
}
@ -202,98 +171,6 @@ export class GrFileListHeader extends KeyboardShortcutMixin(PolymerElement) {
return classes.join(' ');
}
_computeDescriptionPlaceholder(readOnly: boolean) {
return (readOnly ? 'No' : 'Add') + ' patchset description';
}
@observe('change', 'patchNum')
_computePatchSetDescription(change: ChangeInfo, patchNum: PatchSetNum) {
// Polymer 2: check for undefined
if (
change === undefined ||
change.revisions === undefined ||
patchNum === undefined
) {
return;
}
const rev = getRevisionByPatchNum(
Object.values(change.revisions),
patchNum
);
this._patchsetDescription = rev?.description
? rev.description.substring(0, PATCH_DESC_MAX_LENGTH)
: '';
}
_handleDescriptionRemoved(e: CustomEvent) {
return this._updateDescription('', e);
}
/**
* @param revisions The revisions object keyed by revision hashes
* @param patchSet A revision already fetched from {revisions}
* @return the SHA hash corresponding to the revision.
*/
_getPatchsetHash(
revisions: {[revisionId: string]: RevisionInfo},
patchSet: RevisionInfo
) {
for (const sha of Object.keys(revisions)) {
if (revisions[sha] === patchSet) {
return sha;
}
}
throw new Error('patchset hash not found');
}
_handleDescriptionChanged(e: CustomEvent) {
const desc = e.detail.trim();
this._updateDescription(desc, e);
}
/**
* Update the patchset description with the rest API.
*/
_updateDescription(desc: string, e: CustomEvent) {
if (
!this.change ||
!this.change.revisions ||
!this.patchNum ||
!this.changeNum
)
return;
// target can be either gr-editable-label or gr-linked-chip
const target = (dom(e) as EventApi).rootTarget as HTMLElement & {
disabled: boolean;
};
if (target) {
target.disabled = true;
}
const rev = getRevisionByPatchNum(
Object.values(this.change.revisions),
this.patchNum
)!;
const sha = this._getPatchsetHash(this.change.revisions, rev);
return this.restApiService
.setDescription(this.changeNum, this.patchNum, desc)
.then((res: Response) => {
if (res.ok) {
if (target) {
target.disabled = false;
}
this.set(['change', 'revisions', sha, 'description'], desc);
this._patchsetDescription = desc;
}
})
.catch(() => {
if (target) {
target.disabled = false;
}
return;
});
}
_computePrefsButtonHidden(
prefs: DiffPreferencesInfo,
diffPrefsDisabled: boolean

32
polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
View File

@ -53,9 +53,6 @@ export const htmlTemplate = html`
.latestPatchContainer a {
text-decoration: none;
}
gr-editable-label.descriptionLabel {
max-width: 100%;
}
.mobile {
display: none;
}
@ -89,9 +86,6 @@ export const htmlTemplate = html`
.someExpanded #expandBtn {
margin-right: 8px;
}
gr-linked-chip {
--linked-chip-text-color: var(--primary-text-color);
}
.someExpanded #collapseBtn,
.allExpanded #collapseBtn,
.openFile .fileViewActions {
@ -165,32 +159,6 @@ export const htmlTemplate = html`
<span class="separator"></span>
<a href$="[[changeUrl]]">Go to latest patch set</a>
</span>
<span class="container descriptionContainer hideOnEdit">
<span class="separator"></span>
<template is="dom-if" if="[[_patchsetDescription]]">
<gr-linked-chip
id="descriptionChip"
text="[[_patchsetDescription]]"
removable="[[!_descriptionReadOnly]]"
on-remove="_handleDescriptionRemoved"
></gr-linked-chip>
</template>
<template
is="dom-if"
if="[[_showAddPatchsetDescription(_patchsetDescription, change)]]"
>
<gr-editable-label
id="descriptionLabel"
uppercase=""
class="descriptionLabel"
label-text="Add patchset description"
value="[[_patchsetDescription]]"
placeholder="[[_computeDescriptionPlaceholder(_descriptionReadOnly)]]"
read-only="[[_descriptionReadOnly]]"
on-changed="_handleDescriptionChanged"
></gr-editable-label>
</template>
</span>
</div>
</div>
<div class$="rightControls [[_computeExpandedClass(filesExpanded)]]">

97
polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
View File

@ -59,99 +59,6 @@ suite('gr-file-list-header tests', () => {
assert.isFalse(element.$.diffPrefsContainer.hidden);
});
test('_computeDescriptionReadOnly', () => {
element.loggedIn = false;
element.change = {owner: {_account_id: 1}};
element.account = {_account_id: 1};
assert.equal(element._descriptionReadOnly, true);
element.loggedIn = true;
element.change = {owner: {_account_id: 0}};
element.account = {_account_id: 1};
assert.equal(element._descriptionReadOnly, true);
element.loggedIn = true;
element.change = {owner: {_account_id: 1}};
element.account = {_account_id: 1};
assert.equal(element._descriptionReadOnly, false);
});
test('_computeDescriptionPlaceholder', () => {
assert.equal(element._computeDescriptionPlaceholder(true),
'No patchset description');
assert.equal(element._computeDescriptionPlaceholder(false),
'Add patchset description');
});
test('description editing', () => {
const putDescStub = stubRestApi('setDescription')
.returns(Promise.resolve({ok: true}));
element.changeNum = '42';
element.basePatchNum = 'PARENT';
element.patchNum = 1;
element.change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1, description: 'test', commit: {commit: 'rev1'}},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
actions: {},
owner: {_account_id: 1},
};
element.account = {_account_id: 1};
element.owner = {_account_id: 1};
element.loggedIn = true;
flush();
// The element has a description, so the account chip should be visible
element.owner = {_account_id: 1};
// and the description label should not exist.
const chip = element.root.querySelector('#descriptionChip');
let label = element.root.querySelector('#descriptionLabel');
assert.equal(chip.text, 'test');
assert.isNotOk(label);
// Simulate tapping the remove button, but call function directly so that
// can determine what happens after the promise is resolved.
return element._handleDescriptionRemoved()
.then(() => {
// The API stub should be called with an empty string for the new
// description.
assert.equal(putDescStub.lastCall.args[2], '');
assert.equal(element.change.revisions.rev1.description, '');
flush();
// The editable label should now be visible and the chip hidden.
label = element.root.querySelector('#descriptionLabel');
assert.isOk(label);
assert.equal(getComputedStyle(chip).display, 'none');
assert.notEqual(getComputedStyle(label).display, 'none');
assert.isFalse(label.readOnly);
// Edit the label to have a new value of test2, and save.
label.editing = true;
label._inputText = 'test2';
label._save();
flush();
// The API stub should be called with an `test2` for the new
// description.
assert.equal(putDescStub.callCount, 2);
assert.equal(putDescStub.lastCall.args[2], 'test2');
})
.then(() => {
flush();
// The chip should be visible again, and the label hidden.
assert.equal(element.change.revisions.rev1.description, 'test2');
assert.equal(getComputedStyle(label).display, 'none');
assert.notEqual(getComputedStyle(chip).display, 'none');
});
});
test('expandAllDiffs called when expand button clicked', () => {
element.shownFileCount = 1;
flush();
@ -280,14 +187,10 @@ suite('gr-file-list-header tests', () => {
flush();
assert.isFalse(isVisible(element.$.diffPrefsContainer));
assert.isFalse(isVisible(element.shadowRoot
.querySelector('.descriptionContainer')));
element.editMode = false;
flush();
assert.isTrue(isVisible(element.shadowRoot
.querySelector('.descriptionContainer')));
assert.isTrue(isVisible(element.$.diffPrefsContainer));
});