opendev/gerrit
Code Issues Proposed changes

Remove new-change-summary feature flag from gr-change-view

Browse Source 
Change-Id: I971bc62bacc61b1985adb5de8d4fd36da68f2f46
(cherry picked from commit b9d49ce3c53cbeab6b7274dcf6754bd0c1321dcd)
This commit is contained in:
Milutin Kristofic
2021-04-22 20:27:49 +02:00
parent 9b04467e63
commit c08843cc27
4 changed files with 7 additions and 324 deletions
Download Patch File Download Diff File Expand all files Collapse all files

108
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
View File

@@ -163,7 +163,6 @@ import {
fireDialogChange,
fireTitleChange,
} from '../../../utils/event-util';
import {KnownExperimentId} from '../../../services/flags/flags';
import {GerritView} from '../../../services/router/router-model';
import {takeUntil} from 'rxjs/operators';
import {aPluginHasRegistered$} from '../../../services/checks/checks-model';
@@ -171,13 +170,7 @@ import {Subject} from 'rxjs';
import {debounce, DelayedTask} from '../../../utils/async-util';
import {Timing} from '../../../constants/reporting';
const CHANGE_ID_ERROR = {
MISMATCH: 'mismatch',
MISSING: 'missing',
};
const CHANGE_ID_REGEX_PATTERN = /^(Change-Id:\s|Link:.*\/id\/)(I[0-9a-f]{8,40})/gm;
const MIN_LINES_FOR_COMMIT_COLLAPSE = 30;
const MIN_LINES_FOR_COMMIT_COLLAPSE = 17;
const REVIEWERS_REGEX = /^(R|CC)=/gm;
const MIN_CHECK_INTERVAL_SECS = 0;
@@ -251,8 +244,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
private readonly reporting = appContext.reportingService;
private readonly flagsService = appContext.flagsService;
private readonly jsAPI = appContext.jsApiService;
private readonly changeService = appContext.changeService;
@@ -356,8 +347,7 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
type: Boolean,
computed:
'_computeHideEditCommitMessage(_loggedIn, ' +
'_editingCommitMessage, _change, _editMode, _commitCollapsed, ' +
'_commitCollapsible)',
'_editingCommitMessage, _change, _editMode)',
})
_hideEditCommitMessage?: boolean;
@@ -379,13 +369,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
@property({type: Number})
_lineHeight?: number;
@property({
type: String,
computed:
'_computeChangeIdCommitMessageError(_latestCommitMessage, _change)',
})
_changeIdCommitMessageError?: string;
@property({type: Object})
_patchRange?: ChangeViewPatchRange;
@@ -525,9 +508,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
@property({type: Boolean})
_showChecksTab = false;
@property({type: Boolean})
_isNewChangeSummaryUiEnabled = false;
@property({type: String})
_tabState?: TabState;
@@ -569,9 +549,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
aPluginHasRegistered$.pipe(takeUntil(this.disconnected$)).subscribe(b => {
this._showChecksTab = b;
});
this._isNewChangeSummaryUiEnabled = this.flagsService.isEnabled(
KnownExperimentId.NEW_CHANGE_SUMMARY_UI
);
}
constructor() {
@@ -841,11 +818,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
}
}
_handleEditCommitMessage() {
this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea();
}
_handleCommitMessageSave(e: EditableContentSaveEvent) {
assertIsDefined(this._change, '_change');
if (!this._changeNum)
@@ -907,19 +879,13 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
loggedIn: boolean,
editing: boolean,
change: ChangeInfo,
editMode?: boolean,
collapsed?: boolean,
collapsible?: boolean
editMode?: boolean
) {
const hideWhenCollapsed = this._isNewChangeSummaryUiEnabled
? false
: collapsed && collapsible;
if (
!loggedIn ||
editing ||
(change && change.status === ChangeStatus.MERGED) ||
editMode ||
hideWhenCollapsed
editMode
) {
return true;
}
@@ -1540,53 +1506,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
);
}
_computeChangeIdClass(displayChangeId: string) {
return displayChangeId === CHANGE_ID_ERROR.MISMATCH ? 'warning' : '';
}
_computeTitleAttributeWarning(displayChangeId: string) {
if (displayChangeId === CHANGE_ID_ERROR.MISMATCH) {
return 'Change-Id mismatch';
} else if (displayChangeId === CHANGE_ID_ERROR.MISSING) {
return 'No Change-Id in commit message';
}
return undefined;
}
_computeChangeIdCommitMessageError(
commitMessage?: string,
change?: ChangeInfo
) {
if (change === undefined) {
return undefined;
}
if (!commitMessage) {
return CHANGE_ID_ERROR.MISSING;
}
// Find the last match in the commit message:
let changeId;
let changeIdArr;
while ((changeIdArr = CHANGE_ID_REGEX_PATTERN.exec(commitMessage))) {
changeId = changeIdArr[2];
}
if (changeId) {
// A change-id is detected in the commit message.
if (changeId === change.change_id) {
// The change-id found matches the real change-id.
return null;
}
// The change-id found does not match the change-id.
return CHANGE_ID_ERROR.MISMATCH;
}
// There is no change-id in the commit message.
return CHANGE_ID_ERROR.MISSING;
}
_computeReplyButtonLabel(
changeRecord?: ElementPropertyDeepChange<
GrChangeView,
@@ -2347,13 +2266,6 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
return `Change ${changeNum}`;
}
_computeCommitMessageCollapsed(collapsed?: boolean, collapsible?: boolean) {
if (this._isNewChangeSummaryUiEnabled) {
return false;
}
return collapsible && collapsed;
}
/**
* Returns the text to be copied when
* click the copy icon next to change subject
@@ -2366,21 +2278,11 @@ export class GrChangeView extends KeyboardShortcutMixin(PolymerElement) {
);
}
_toggleCommitCollapsed() {
this._commitCollapsed = !this._commitCollapsed;
if (this._commitCollapsed) {
window.scrollTo(0, 0);
}
}
_computeCommitCollapsible(commitMessage?: string) {
if (!commitMessage) {
return false;
}
const MIN_LINES = this._isNewChangeSummaryUiEnabled
? 17
: MIN_LINES_FOR_COMMIT_COLLAPSE;
return commitMessage.split('\n').length >= MIN_LINES;
return commitMessage.split('\n').length >= MIN_LINES_FOR_COMMIT_COLLAPSE;
}
_startUpdateCheckTimer() {

72
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
View File

@@ -91,11 +91,6 @@ export const htmlTemplate = html`
background-color: var(--view-background-color);
box-shadow: var(--elevation-level-1);
}
.changeId {
color: var(--deemphasized-text-color);
font-family: var(--font-family);
margin-top: var(--spacing-l);
}
.changeMetadata {
/* Limit meta section to half of the screen at max */
max-width: 50%;
@@ -115,18 +110,8 @@ export const htmlTemplate = html`
#commitMessageEditor {
/* Account for border and padding and rounding errors. */
min-width: calc(72ch + 2px + 2 * var(--spacing-m) + 0.4px);
--collapsed-max-height: 36em;
}
.new-change-summary-true #commitMessageEditor {
--collapsed-max-height: 300px;
}
.editCommitMessage {
margin-top: var(--spacing-l);
--gr-button: {
padding: 5px 0px;
}
}
.changeStatuses,
.commitActions,
.statusText {
@@ -156,9 +141,6 @@ export const htmlTemplate = html`
.mobile {
display: none;
}
.warning {
color: var(--error-text-color);
}
hr {
border: 0;
border-top: 1px solid var(--border-color);
@@ -175,13 +157,6 @@ export const htmlTemplate = html`
margin: var(--spacing-l) 0;
padding: 0 var(--spacing-l);
}
.collapseToggleContainer {
display: flex;
margin-bottom: 8px;
}
.collapseToggleContainer gr-button {
display: block;
}
.showOnEdit {
display: none;
}
@@ -223,7 +198,7 @@ export const htmlTemplate = html`
padding-top: var(--spacing-l);
width: 100%;
}
gr-change-summary.new-change-summary-true {
gr-change-summary {
/* temporary for old checks status */
margin-bottom: var(--spacing-m);
}
@@ -435,10 +410,7 @@ export const htmlTemplate = html`
>[[_replyButtonLabel]]</gr-button
>
</div>
<div
id="commitMessage"
class$="commitMessage new-change-summary-[[_isNewChangeSummaryUiEnabled]]"
>
<div id="commitMessage" class="commitMessage">
<gr-editable-content
id="commitMessageEditor"
editing="{{_editingCommitMessage}}"
@@ -447,7 +419,6 @@ export const htmlTemplate = html`
hide-edit-commit-message="[[_hideEditCommitMessage]]"
commit-collapsible="[[_commitCollapsible]]"
remove-zero-width-space=""
collapsed$="[[_computeCommitMessageCollapsed(_commitCollapsed, _commitCollapsible)]]"
>
<gr-linked-text
pre=""
@@ -456,46 +427,7 @@ export const htmlTemplate = html`
remove-zero-width-space=""
></gr-linked-text>
</gr-editable-content>
<template is="dom-if" if="[[!_isNewChangeSummaryUiEnabled]]">
<gr-button
link=""
class="editCommitMessage"
title="Edit commit message"
on-click="_handleEditCommitMessage"
hidden$="[[_hideEditCommitMessage]]"
>Edit</gr-button
>
<div
class="changeId"
hidden$="[[!_changeIdCommitMessageError]]"
>
<hr />
Change-Id:
<span
class$="[[_computeChangeIdClass(_changeIdCommitMessageError)]]"
title$="[[_computeTitleAttributeWarning(_changeIdCommitMessageError)]]"
>
[[_change.change_id]]
</span>
</div>
</template>
</div>
<template is="dom-if" if="[[!_isNewChangeSummaryUiEnabled]]">
<div
id="commitCollapseToggle"
class="collapseToggleContainer"
hidden$="[[!_commitCollapsible]]"
>
<gr-button
link=""
id="commitCollapseToggleButton"
class="collapseToggleButton"
on-click="_toggleCommitCollapsed"
>
[[_computeCollapseText(_commitCollapsed)]]
</gr-button>
</div>
</template>
<gr-change-summary
change-comments="[[_changeComments]]"
comment-threads="[[_commentThreads]]"

150
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
View File

@@ -54,7 +54,6 @@ import {
createUserConfig,
TEST_NUMERIC_CHANGE_ID,
TEST_PROJECT_NAME,
getCurrentRevision,
createEditRevision,
createAccountWithIdNameAndEmail,
createChangeViewChange,
@@ -1662,104 +1661,6 @@ suite('gr-change-view tests', () => {
assert.equal(putStub.lastCall.args[1], '\n\n\n\n\n\n\n\n');
});
test('_computeChangeIdCommitMessageError', () => {
let commitMessage = 'Change-Id: I4ce18b2395bca69d7a9aa48bf4554faa56282483';
let change: ChangeInfo = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282483' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
null
);
change = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282484' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
'mismatch'
);
commitMessage = 'This is the greatest change.';
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
'missing'
);
});
test('multiple change Ids in commit message picks last', () => {
const commitMessage = [
'Change-Id: I4ce18b2395bca69d7a9aa48bf4554faa56282484',
'Change-Id: I4ce18b2395bca69d7a9aa48bf4554faa56282483',
].join('\n');
let change: ChangeInfo = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282483' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
null
);
change = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282484' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
'mismatch'
);
});
test('does not count change Id that starts mid line', () => {
const commitMessage = [
'Change-Id: I4ce18b2395bca69d7a9aa48bf4554faa56282484',
'Change-Id: I4ce18b2395bca69d7a9aa48bf4554faa56282483',
].join(' and ');
let change: ChangeInfo = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282484' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
null
);
change = {
...createChangeViewChange(),
change_id: 'I4ce18b2395bca69d7a9aa48bf4554faa56282483' as ChangeId,
};
assert.equal(
element._computeChangeIdCommitMessageError(commitMessage, change),
'mismatch'
);
});
test('_computeTitleAttributeWarning', () => {
let changeIdCommitMessageError = 'missing';
assert.equal(
element._computeTitleAttributeWarning(changeIdCommitMessageError),
'No Change-Id in commit message'
);
changeIdCommitMessageError = 'mismatch';
assert.equal(
element._computeTitleAttributeWarning(changeIdCommitMessageError),
'Change-Id mismatch'
);
});
test('_computeChangeIdClass', () => {
let changeIdCommitMessageError = 'missing';
assert.equal(element._computeChangeIdClass(changeIdCommitMessageError), '');
changeIdCommitMessageError = 'mismatch';
assert.equal(
element._computeChangeIdClass(changeIdCommitMessageError),
'warning'
);
});
test('topic is coalesced to null', done => {
sinon.stub(element, '_changeChanged');
stubRestApi('getChangeDetail').returns(
@@ -2113,57 +2014,6 @@ suite('gr-change-view tests', () => {
});
});
suite('commit message expand/collapse', () => {
setup(() => {
element._change = {
...createChangeViewChange(),
revisions: createRevisions(1),
messages: createChangeMessages(1),
};
element._change.labels = {};
stubRestApi('getChangeDetail').callsFake(() =>
Promise.resolve({
...createChangeViewChange(),
// new patchset was uploaded
revisions: createRevisions(2),
current_revision: getCurrentRevision(2),
messages: createChangeMessages(1),
})
);
});
test('commitCollapseToggle hidden for short commit message', () => {
element._latestCommitMessage = '';
flush();
const commitCollapseToggle = element.shadowRoot!.querySelector(
'#commitCollapseToggle'
);
assert.isTrue(commitCollapseToggle?.hasAttribute('hidden'));
});
test('commitCollapseToggle shown for long commit message', () => {
element._latestCommitMessage = _.times(31, String).join('\n');
const commitCollapseToggle = element.shadowRoot!.querySelector(
'#commitCollapseToggle'
);
assert.isFalse(commitCollapseToggle?.hasAttribute('hidden'));
});
test('commitCollapseToggle functions', () => {
element._latestCommitMessage = _.times(35, String).join('\n');
assert.isTrue(element._commitCollapsed);
assert.isTrue(element._commitCollapsible);
assert.isTrue(element.$.commitMessageEditor.hasAttribute('collapsed'));
const commitCollapseToggleButton = element.shadowRoot!.querySelector(
'#commitCollapseToggleButton'
)!;
tap(commitCollapseToggleButton);
assert.isFalse(element._commitCollapsed);
assert.isTrue(element._commitCollapsible);
assert.isFalse(element.$.commitMessageEditor.hasAttribute('collapsed'));
});
});
test('header class computation', () => {
assert.equal(element._computeHeaderClass(), 'header');
assert.equal(element._computeHeaderClass(true), 'header editMode');

1
polygerrit-ui/app/services/flags/flags.ts
View File

@@ -28,7 +28,6 @@ export enum KnownExperimentId {
// be used by plugins. The new Checks UI will show up, if a plugin registers
// with the new Checks plugin API.
CI_REBOOT_CHECKS = 'UiFeature__ci_reboot_checks',
NEW_CHANGE_SUMMARY_UI = 'UiFeature__new_change_summary_ui',
NEW_IMAGE_DIFF_UI = 'UiFeature__new_image_diff_ui',
COMMENT_CONTEXT = 'UiFeature__comment_context',
}