Merge "Remove old submit requirements from labels and reviewers"
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
index 27c445e..8e757da 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
@@ -24,10 +24,8 @@
LabelNameToValueMap,
} from '../../../types/common';
import {GrLabelScoreRow} from '../gr-label-score-row/gr-label-score-row';
-import {getAppContext} from '../../../services/app-context';
import {
getTriggerVotes,
- showNewSubmitRequirements,
computeLabels,
Label,
computeOrderedLabelValues,
@@ -48,8 +46,6 @@
@property({type: Object})
account?: AccountInfo;
- private readonly flagsService = getAppContext().flagsService;
-
static override get styles() {
return [
fontStyles,
@@ -57,8 +53,6 @@
.scoresTable {
display: table;
width: 100%;
- }
- .scoresTable.newSubmitRequirements {
table-layout: fixed;
}
.mergedMessage,
@@ -91,19 +85,6 @@
}
override render() {
- if (showNewSubmitRequirements(this.flagsService, this.change)) {
- return this.renderNewSubmitRequirements();
- } else {
- return this.renderOldSubmitRequirements();
- }
- }
-
- private renderOldSubmitRequirements() {
- const labels = computeLabels(this.account, this.change);
- return html`${this.renderLabels(labels)}${this.renderErrorMessages()}`;
- }
-
- private renderNewSubmitRequirements() {
return html`${this.renderSubmitReqsLabels()}${this.renderTriggerVotes()}
${this.renderErrorMessages()}`;
}
@@ -145,13 +126,7 @@
}
private renderLabels(labels: Label[]) {
- const newSubReqs = showNewSubmitRequirements(
- this.flagsService,
- this.change
- );
- return html`<div
- class="scoresTable ${newSubReqs ? 'newSubmitRequirements' : ''}"
- >
+ return html`<div class="scoresTable">
${labels
.filter(
label =>
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
index 6740977..d5ce654 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
@@ -29,12 +29,7 @@
LabelInfo,
} from '../../../types/common';
import {hasOwnProperty} from '../../../utils/common-util';
-import {getAppContext} from '../../../services/app-context';
-import {
- getApprovalInfo,
- getCodeReviewLabel,
- showNewSubmitRequirements,
-} from '../../../utils/label-util';
+import {getApprovalInfo, getCodeReviewLabel} from '../../../utils/label-util';
import {sortReviewers} from '../../../utils/attention-set-util';
import {sharedStyles} from '../../../styles/shared-styles';
import {css} from 'lit';
@@ -68,8 +63,6 @@
@state() showAllReviewers = false;
- private readonly flagsService = getAppContext().flagsService;
-
static override get styles() {
return [
sharedStyles,
@@ -166,14 +159,12 @@
.vote=${this.computeVote(reviewer)}
.label=${this.computeCodeReviewLabel()}
>
- ${showNewSubmitRequirements(this.flagsService, this.change)
- ? html`<gr-vote-chip
- slot="vote-chip"
- .vote=${this.computeVote(reviewer)}
- .label=${this.computeCodeReviewLabel()}
- circle-shape
- ></gr-vote-chip>`
- : nothing}
+ <gr-vote-chip
+ slot="vote-chip"
+ .vote=${this.computeVote(reviewer)}
+ .label=${this.computeCodeReviewLabel()}
+ circle-shape
+ ></gr-vote-chip>
</gr-account-chip>
`;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
index b1d3914..ee3bec7 100644
--- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
+++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
@@ -18,11 +18,9 @@
import '../../../styles/gr-voting-styles';
import '../../../styles/shared-styles';
import '../gr-vote-chip/gr-vote-chip';
-import '../gr-account-label/gr-account-label';
import '../gr-account-chip/gr-account-chip';
import '../gr-button/gr-button';
import '../gr-icons/gr-icons';
-import '../gr-label/gr-label';
import '../gr-tooltip-content/gr-tooltip-content';
import {dom, EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {
@@ -30,9 +28,7 @@
LabelInfo,
ApprovalInfo,
AccountId,
- isQuickLabelInfo,
isDetailedLabelInfo,
- LabelNameToInfoMap,
} from '../../../types/common';
import {LitElement, css, html} from 'lit';
import {customElement, property} from 'lit/decorators';
@@ -40,10 +36,8 @@
import {
canVote,
getApprovalInfo,
- getVotingRangeOrDefault,
hasNeutralStatus,
hasVoted,
- showNewSubmitRequirements,
valueString,
} from '../../../utils/label-util';
import {getAppContext} from '../../../services/app-context';
@@ -61,19 +55,6 @@
}
}
-enum LabelClassName {
- NEGATIVE = 'negative',
- POSITIVE = 'positive',
- MIN = 'min',
- MAX = 'max',
-}
-
-interface FormattedLabel {
- className?: LabelClassName;
- account: ApprovalInfo | AccountInfo;
- value: string;
-}
-
@customElement('gr-label-info')
export class GrLabelInfo extends LitElement {
@property({type: Object})
@@ -107,8 +88,6 @@
private readonly reporting = getAppContext().reportingService;
- private readonly flagsService = getAppContext().flagsService;
-
// TODO(TS): not used, remove later
_xhrPromise?: Promise<void>;
@@ -118,9 +97,6 @@
fontStyles,
votingStyles,
css`
- .placeholder {
- color: var(--deemphasized-text-color);
- }
.hidden {
display: none;
}
@@ -132,33 +108,6 @@
margin-right: var(--spacing-s);
padding: 1px;
}
- .max {
- background-color: var(--vote-color-approved);
- }
- .min {
- background-color: var(--vote-color-rejected);
- }
- .positive {
- background-color: var(--vote-color-recommended);
- border-radius: 12px;
- border: 1px solid var(--vote-outline-recommended);
- color: var(--chip-color);
- }
- .negative {
- background-color: var(--vote-color-disliked);
- border-radius: 12px;
- border: 1px solid var(--vote-outline-disliked);
- color: var(--chip-color);
- }
- .hidden {
- display: none;
- }
- td {
- vertical-align: top;
- }
- tr {
- min-height: var(--line-height-normal);
- }
gr-tooltip-content {
display: block;
}
@@ -173,17 +122,10 @@
gr-button[disabled] iron-icon {
color: var(--border-color);
}
- gr-account-label {
- --account-max-length: 100px;
- margin-right: var(--spacing-xs);
- }
iron-icon {
height: calc(var(--line-height-normal) - 2px);
width: calc(var(--line-height-normal) - 2px);
}
- .labelValueContainer:not(:first-of-type) td {
- padding-top: var(--spacing-s);
- }
.reviewer-row {
padding-top: var(--spacing-s);
}
@@ -208,14 +150,6 @@
}
override render() {
- if (showNewSubmitRequirements(this.flagsService, this.change)) {
- return this.renderNewSubmitRequirements();
- } else {
- return this.renderOldSubmitRequirements();
- }
- }
-
- private renderNewSubmitRequirements() {
const labelInfo = this.labelInfo;
if (!labelInfo) return;
const reviewers = (this.change?.reviewers['REVIEWER'] ?? [])
@@ -238,23 +172,6 @@
</div>`;
}
- private renderOldSubmitRequirements() {
- const labelInfo = this.labelInfo;
- return html` <p
- class="placeholder ${this.computeShowPlaceholder(
- labelInfo,
- this.change?.labels
- )}"
- >
- No votes
- </p>
- <table>
- ${this.mapLabelInfo(labelInfo, this.account, this.change?.labels).map(
- mappedLabel => this.renderLabel(mappedLabel)
- )}
- </table>`;
- }
-
renderReviewerVote(reviewer: AccountInfo) {
const labelInfo = this.labelInfo;
if (!labelInfo) return;
@@ -285,30 +202,6 @@
</div>`;
}
- renderLabel(mappedLabel: FormattedLabel) {
- const {labelInfo, change} = this;
- return html` <tr class="labelValueContainer">
- <td>
- <gr-tooltip-content
- has-tooltip
- title=${this._computeValueTooltip(labelInfo, mappedLabel.value)}
- >
- <gr-label class="${mappedLabel.className} voteChip font-small">
- ${mappedLabel.value}
- </gr-label>
- </gr-tooltip-content>
- </td>
- <td>
- <gr-account-label
- clickable
- .account=${mappedLabel.account}
- .change=${change}
- ></gr-account-label>
- </td>
- <td>${this.renderRemoveVote(mappedLabel.account)}</td>
- </tr>`;
- }
-
private renderVoteAbility(reviewer: AccountInfo) {
if (this.labelInfo && isDetailedLabelInfo(this.labelInfo)) {
const approvalInfo = getApprovalInfo(this.labelInfo, reviewer);
@@ -341,83 +234,6 @@
}
/**
- * This method also listens on change.labels.*,
- * to trigger computation when a label is removed from the change.
- *
- * The third parameter is just for *triggering* computation.
- */
- private mapLabelInfo(
- labelInfo?: LabelInfo,
- account?: AccountInfo,
- _?: LabelNameToInfoMap
- ): FormattedLabel[] {
- const result: FormattedLabel[] = [];
- if (!labelInfo) {
- return result;
- }
- if (!isDetailedLabelInfo(labelInfo)) {
- if (
- isQuickLabelInfo(labelInfo) &&
- (labelInfo.rejected || labelInfo.approved)
- ) {
- const ok = labelInfo.approved || !labelInfo.rejected;
- return [
- {
- value: ok ? '👍️' : '👎️',
- className: ok ? LabelClassName.POSITIVE : LabelClassName.NEGATIVE,
- // executed only if approved or rejected is not undefined
- account: ok ? labelInfo.approved! : labelInfo.rejected!,
- },
- ];
- }
- return result;
- }
-
- // Sort votes by positivity.
- // TODO(TS): maybe mark value as required if always present
- const votes = (labelInfo.all || []).sort(
- (a, b) => (a.value || 0) - (b.value || 0)
- );
- const votingRange = getVotingRangeOrDefault(labelInfo);
- for (const label of votes) {
- if (
- label.value &&
- (!isQuickLabelInfo(labelInfo) ||
- label.value !== labelInfo.default_value)
- ) {
- let labelClassName;
- let labelValPrefix = '';
- if (label.value > 0) {
- labelValPrefix = '+';
- if (label.value === votingRange.max) {
- labelClassName = LabelClassName.MAX;
- } else {
- labelClassName = LabelClassName.POSITIVE;
- }
- } else if (label.value < 0) {
- if (label.value === votingRange.min) {
- labelClassName = LabelClassName.MIN;
- } else {
- labelClassName = LabelClassName.NEGATIVE;
- }
- }
- const formattedLabel: FormattedLabel = {
- value: `${labelValPrefix}${label.value}`,
- className: labelClassName,
- account: label,
- };
- if (label._account_id === account?._account_id) {
- // Put self-votes at the top.
- result.unshift(formattedLabel);
- } else {
- result.push(formattedLabel);
- }
- }
- }
- return result;
- }
-
- /**
* A user is able to delete a vote iff the mutable property is true and the
* reviewer that left the vote exists in the list of removable_reviewers
* received from the backend.
@@ -488,39 +304,4 @@
}
return labelInfo.values[score];
}
-
- /**
- * This method also listens change.labels.* in
- * order to trigger computation when a label is removed from the change.
- *
- * The second parameter is just for *triggering* computation.
- */
- private computeShowPlaceholder(
- labelInfo?: LabelInfo,
- _?: LabelNameToInfoMap
- ) {
- if (!labelInfo) {
- return '';
- }
- if (
- !isDetailedLabelInfo(labelInfo) &&
- isQuickLabelInfo(labelInfo) &&
- (labelInfo.rejected || labelInfo.approved)
- ) {
- return 'hidden';
- }
-
- if (isDetailedLabelInfo(labelInfo) && labelInfo.all) {
- for (const label of labelInfo.all) {
- if (
- label.value &&
- (!isQuickLabelInfo(labelInfo) ||
- label.value !== labelInfo.default_value)
- ) {
- return 'hidden';
- }
- }
- }
- return '';
- }
}
diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_test.ts b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_test.ts
index 0ac49a7..f1336b4 100644
--- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_test.ts
@@ -20,20 +20,18 @@
import {
isHidden,
mockPromise,
- queryAll,
queryAndAssert,
stubRestApi,
} from '../../../test/test-utils';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
import {GrLabelInfo} from './gr-label-info';
import {GrButton} from '../gr-button/gr-button';
-import {GrLabel} from '../gr-label/gr-label';
import {
createAccountWithIdNameAndEmail,
+ createDetailedLabelInfo,
createParsedChange,
} from '../../../test/test-data-generators';
-import {LabelInfo} from '../../../types/common';
-import {GrAccountLabel} from '../gr-account-label/gr-account-label';
+import {ApprovalInfo, LabelInfo} from '../../../types/common';
const basicFixture = fixtureFromElement('gr-label-info');
@@ -41,12 +39,51 @@
let element: GrLabelInfo;
const account = createAccountWithIdNameAndEmail(5);
- setup(() => {
+ setup(async () => {
element = basicFixture.instantiate();
// Needed to trigger computed bindings.
element.account = {};
- element.change = {...createParsedChange(), labels: {}};
+ element.change = {
+ ...createParsedChange(),
+ labels: {},
+ reviewers: {
+ REVIEWER: [account],
+ CC: [],
+ },
+ };
+ const approval: ApprovalInfo = {
+ value: 2,
+ _account_id: account._account_id,
+ };
+ element.labelInfo = {
+ ...createDetailedLabelInfo(),
+ all: [approval],
+ };
+ await element.updateComplete;
+ });
+
+ test('renders', () => {
+ expect(element).shadowDom.to.equal(/* HTML */ `<div>
+ <div class="reviewer-row">
+ <gr-account-chip>
+ <gr-vote-chip circle-shape="" slot="vote-chip"> </gr-vote-chip>
+ </gr-account-chip>
+ <gr-tooltip-content has-tooltip="" title="Remove vote">
+ <gr-button
+ aria-disabled="false"
+ aria-label="Remove vote"
+ class="deleteBtn hidden"
+ data-account-id="5"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ <iron-icon icon="gr-icons:delete"> </iron-icon>
+ </gr-button>
+ </gr-tooltip-content>
+ </div>
+ </div>`);
});
suite('remove reviewer votes', () => {
@@ -62,6 +99,10 @@
element.change = {
...createParsedChange(),
labels: {'Code-Review': label},
+ reviewers: {
+ REVIEWER: [account],
+ CC: [],
+ },
};
element.labelInfo = label;
element.label = 'Code-Review';
@@ -108,101 +149,6 @@
});
});
- suite('label color and order', () => {
- test('valueless label rejected', async () => {
- element.labelInfo = {rejected: {name: 'someone'}};
- await element.updateComplete;
- const labels = queryAll<GrLabel>(element, 'gr-label');
- assert.isTrue(labels[0].classList.contains('negative'));
- });
-
- test('valueless label approved', async () => {
- element.labelInfo = {approved: {name: 'someone'}};
- await element.updateComplete;
- const labels = queryAll<GrLabel>(element, 'gr-label');
- assert.isTrue(labels[0].classList.contains('positive'));
- });
-
- test('-2 to +2', async () => {
- element.labelInfo = {
- all: [
- {value: 2, name: 'user 2'},
- {value: 1, name: 'user 1'},
- {value: -1, name: 'user 3'},
- {value: -2, name: 'user 4'},
- ],
- values: {
- '-2': 'Awful',
- '-1': "Don't submit as-is",
- ' 0': 'No score',
- '+1': 'Looks good to me',
- '+2': 'Ready to submit',
- },
- };
- await element.updateComplete;
- const labels = queryAll<GrLabel>(element, 'gr-label');
- assert.isTrue(labels[0].classList.contains('max'));
- assert.isTrue(labels[1].classList.contains('positive'));
- assert.isTrue(labels[2].classList.contains('negative'));
- assert.isTrue(labels[3].classList.contains('min'));
- });
-
- test('-1 to +1', async () => {
- element.labelInfo = {
- all: [
- {value: 1, name: 'user 1'},
- {value: -1, name: 'user 2'},
- ],
- values: {
- '-1': "Don't submit as-is",
- ' 0': 'No score',
- '+1': 'Looks good to me',
- },
- };
- await element.updateComplete;
- const labels = queryAll<GrLabel>(element, 'gr-label');
- assert.isTrue(labels[0].classList.contains('max'));
- assert.isTrue(labels[1].classList.contains('min'));
- });
-
- test('0 to +2', async () => {
- element.labelInfo = {
- all: [
- {value: 1, name: 'user 2'},
- {value: 2, name: 'user '},
- ],
- values: {
- ' 0': "Don't submit as-is",
- '+1': 'No score',
- '+2': 'Looks good to me',
- },
- };
- await element.updateComplete;
- const labels = queryAll<GrLabel>(element, 'gr-label');
- assert.isTrue(labels[0].classList.contains('max'));
- assert.isTrue(labels[1].classList.contains('positive'));
- });
-
- test('self votes at top', async () => {
- const otherAccount = createAccountWithIdNameAndEmail(8);
- element.account = account;
- element.labelInfo = {
- all: [
- {...otherAccount, value: 1},
- {...account, value: -1},
- ],
- values: {
- '-1': "Don't submit as-is",
- ' 0': 'No score',
- '+1': 'Looks good to me',
- },
- };
- await element.updateComplete;
- const chips = queryAll<GrAccountLabel>(element, 'gr-account-label');
- assert.equal(chips[0].account!._account_id, element.account._account_id);
- });
- });
-
test('_computeValueTooltip', () => {
// Existing label.
let labelInfo: LabelInfo = {values: {0: 'Baz'}};
@@ -218,49 +164,4 @@
score = '0';
assert.equal(element._computeValueTooltip(labelInfo, score), '');
});
-
- test('placeholder', async () => {
- const values = {
- '0': 'No score',
- '+1': 'good',
- '+2': 'excellent',
- '-1': 'bad',
- '-2': 'terrible',
- };
- element.labelInfo = {};
- await element.updateComplete;
- assert.isFalse(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {all: [], values};
- await element.updateComplete;
- assert.isFalse(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {all: [{value: 1}], values};
- await element.updateComplete;
- assert.isTrue(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {rejected: account};
- await element.updateComplete;
- assert.isTrue(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {rejected: account, all: [{value: 1}], values};
- await element.updateComplete;
- assert.isTrue(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {approved: account};
- await element.updateComplete;
- assert.isTrue(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- element.labelInfo = {approved: account, all: [{value: 1}], values};
- await element.updateComplete;
- assert.isTrue(
- isHidden(queryAndAssert<HTMLParagraphElement>(element, '.placeholder'))
- );
- });
});
diff --git a/polygerrit-ui/app/elements/shared/gr-label/gr-label.ts b/polygerrit-ui/app/elements/shared/gr-label/gr-label.ts
deleted file mode 100644
index 842b35e..0000000
--- a/polygerrit-ui/app/elements/shared/gr-label/gr-label.ts
+++ /dev/null
@@ -1,42 +0,0 @@
-/**
- * @license
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-/**
- * @fileoverview Consider removing this element as
- * its functionality seems to be duplicated with gr-tooltip and only
- * used in gr-label-info.
- */
-
-import {html, LitElement} from 'lit';
-import {customElement} from 'lit/decorators';
-
-declare global {
- interface HTMLElementTagNameMap {
- 'gr-label': GrLabel;
- }
-}
-
-@customElement('gr-label')
-export class GrLabel extends LitElement {
- static override get styles() {
- return [];
- }
-
- override render() {
- return html` <slot></slot> `;
- }
-}
diff --git a/polygerrit-ui/app/elements/shared/gr-label/gr-label_html.ts b/polygerrit-ui/app/elements/shared/gr-label/gr-label_html.ts
deleted file mode 100644
index 94196df..0000000
--- a/polygerrit-ui/app/elements/shared/gr-label/gr-label_html.ts
+++ /dev/null
@@ -1,19 +0,0 @@
-/**
- * @license
- * Copyright (C) 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-import {html} from '@polymer/polymer/lib/utils/html-tag';
-
-export const htmlTemplate = html` <slot></slot> `;
