Merge changes Ia6dfd182,Ib3755469
* changes:
Reject invalid label ranges for permission when pushing a project.config file
Set Access REST endpoint: Reject invalid label ranges
diff --git a/java/com/google/gerrit/entities/Permission.java b/java/com/google/gerrit/entities/Permission.java
index e3e3a57..6d2fa32 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -278,7 +278,10 @@
public Permission build() {
setRules(
- rulesBuilders.stream().map(PermissionRule.Builder::build).collect(toImmutableList()));
+ rulesBuilders.stream()
+ .map(PermissionRule.Builder::build)
+ .distinct()
+ .collect(toImmutableList()));
return autoBuild();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
index 311d4be..a2c96b6 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
@@ -67,6 +67,7 @@
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.schema.GrantRevertPermission;
import com.google.inject.Inject;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -263,6 +264,32 @@
}
@Test
+ public void addDuplicatedAccessSection_doesNotAddDuplicateEntry() throws Exception {
+ ProjectAccessInput accessInput = newProjectAccessInput();
+ AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+ accessInput.add.put(REFS_HEADS, accessSectionInfo);
+ pApi().access(accessInput);
+ List<String> projectConfigLines =
+ Arrays.asList(projectOperations.project(newProjectName).getConfig().toText().split("\n"));
+ assertThat(projectConfigLines)
+ .containsExactly(
+ "[submit]",
+ "\taction = inherit",
+ "[access \"refs/heads/*\"]",
+ "\tlabel-Code-Review = deny group Registered Users",
+ "\tlabel-Code-Review = -1..+1 group Project Owners",
+ "\tpush = group Registered Users");
+
+ // Apply the same permission once more
+ pApi().access(accessInput);
+
+ List<String> newProjectConfigLines =
+ Arrays.asList(projectOperations.project(newProjectName).getConfig().toText().split("\n"));
+ assertThat(projectConfigLines).isEqualTo(newProjectConfigLines);
+ }
+
+ @Test
public void addAccessSectionForPluginPermission() throws Exception {
try (Registration registration =
extensionRegistry
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index fed22f2..c1a7627 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -329,7 +329,7 @@
}
@Test
- public void addDuplicatePermissions() throws Exception {
+ public void addDuplicatePermissions_isIgnored() throws Exception {
TestPermission permission =
TestProjectUpdate.allow(Permission.ABANDON).ref("refs/foo").group(REGISTERED_USERS).build();
Project.NameKey key = projectOperations.newProject().create();
@@ -340,9 +340,8 @@
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
- .containsExactly(
- "abandon", "group global:Registered-Users",
- "abandon", "group global:Registered-Users");
+ // Duplicated permission was recorded only once
+ .containsExactly("abandon", "group global:Registered-Users");
projectOperations.project(key).forUpdate().add(permission).update();
config = projectOperations.project(key).getConfig();
@@ -350,10 +349,8 @@
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
- .containsExactly(
- "abandon", "group global:Registered-Users",
- "abandon", "group global:Registered-Users",
- "abandon", "group global:Registered-Users");
+ // Duplicated permission in request was dropped
+ .containsExactly("abandon", "group global:Registered-Users");
}
@Test
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
index af4e87e..1be5fa3 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
@@ -129,6 +129,7 @@
constructor() {
super();
+ this.addEventListener('reload', () => window.location.reload());
subscribe(
this,
() => this.getAdminViewModel().state$,
diff --git a/polygerrit-ui/app/elements/admin/gr-create-pointer-dialog/gr-create-pointer-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-pointer-dialog/gr-create-pointer-dialog.ts
index 889a859..558d571 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-pointer-dialog/gr-create-pointer-dialog.ts
+++ b/polygerrit-ui/app/elements/admin/gr-create-pointer-dialog/gr-create-pointer-dialog.ts
@@ -6,8 +6,6 @@
import '@polymer/iron-input/iron-input';
import '../../shared/gr-button/gr-button';
import '../../shared/gr-select/gr-select';
-import {encodeURL, getBaseUrl} from '../../../utils/url-util';
-import {page} from '../../../utils/page-wrapper-utils';
import {BranchName, RepoName} from '../../../types/common';
import {getAppContext} from '../../../services/app-context';
import {formStyles} from '../../../styles/gr-form-styles';
@@ -15,7 +13,7 @@
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
-import {fireEvent} from '../../../utils/event-util';
+import {fireAlert, fireEvent, fireReload} from '../../../utils/event-util';
import {RepoDetailView} from '../../../models/views/repo';
declare global {
@@ -126,13 +124,13 @@
throw new Error('itemName name is not set');
}
const USE_HEAD = this.itemRevision ? this.itemRevision : 'HEAD';
- const url = `${getBaseUrl()}/admin/repos/${encodeURL(this.repoName, true)}`;
if (this.itemDetail === RepoDetailView.BRANCHES) {
return this.restApiService
.createRepoBranch(this.repoName, this.itemName, {revision: USE_HEAD})
.then(itemRegistered => {
if (itemRegistered.status === 201) {
- page.show(`${url},branches`);
+ fireAlert(this, 'Branch created successfully. Reloading...');
+ fireReload(this);
}
});
} else if (this.itemDetail === RepoDetailView.TAGS) {
@@ -143,7 +141,8 @@
})
.then(itemRegistered => {
if (itemRegistered.status === 201) {
- page.show(`${url},tags`);
+ fireAlert(this, 'Tag created successfully. Reloading...');
+ fireReload(this);
}
});
}
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
index 64bfe91..2b7d1ff 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
@@ -528,6 +528,9 @@
this.reporting.reportErrorDialog(message);
this.errorDialog.text = message;
this.errorDialog.showSignInButton = !!options && !!options.showSignInButton;
+ if (this.errorModal.hasAttribute('open')) {
+ this.errorModal.close();
+ }
this.errorModal.showModal();
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
index 090d125..5270603 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
@@ -347,7 +347,8 @@
createTextEl(
lineNumberEl: HTMLElement | null,
line: GrDiffLine,
- side?: Side
+ side?: Side,
+ twoSlots?: boolean
) {
const td = createElementDiff('td');
if (line.type !== GrDiffLineType.BLANK) {
@@ -403,9 +404,19 @@
const threadGroupEl = document.createElement('div');
threadGroupEl.className = 'thread-group';
threadGroupEl.setAttribute('data-side', side);
+
const slot = document.createElement('slot');
slot.name = `${side}-${lineNumber}`;
threadGroupEl.appendChild(slot);
+
+ // For line.type === BOTH in unified diff we want two slots.
+ if (twoSlots) {
+ const slot = document.createElement('slot');
+ const otherSide = side === Side.LEFT ? Side.RIGHT : Side.LEFT;
+ slot.name = `${otherSide}-${line.lineNumber(otherSide)}`;
+ threadGroupEl.appendChild(slot);
+ }
+
td.appendChild(threadGroupEl);
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-unified.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-unified.ts
index f760232..ffc2dcd 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-unified.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-unified.ts
@@ -142,7 +142,8 @@
.join(' ')
.trim()
);
- row.appendChild(this.createTextEl(lineNumberEl, line, side));
+ const twoSlots = line.type === GrDiffLineType.BOTH;
+ row.appendChild(this.createTextEl(lineNumberEl, line, side, twoSlots));
return row;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
index e496cca..14b8280 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
@@ -348,7 +348,7 @@
if (lineNumber)
fire(this, 'line-mouse-leave', {lineNum: lineNumber, side});
}}
- >${this.renderText(side)}${this.renderThreadGroup(side, lineNumber)}</td>
+ >${this.renderText(side)}${this.renderThreadGroup(side)}</td>
`;
}
@@ -369,11 +369,21 @@
return html`<td class=${diffClasses(...extras)}>${sign}</td>`;
}
- private renderThreadGroup(side: Side, lineNumber?: LineNumber) {
- if (!lineNumber) return;
- // .content has `white-space: pre`, so prettier must not add spaces.
- // prettier-ignore
- return html`<div class="thread-group" data-side=${side}><slot name="${side}-${lineNumber}"></slot></div>`;
+ private renderThreadGroup(side: Side) {
+ const lineNumber = this.lineNumber(side);
+ if (!lineNumber) return nothing;
+ return html`<div class="thread-group" data-side=${side}>
+ <slot name="${side}-${lineNumber}"></slot>
+ ${this.renderSecondSlot()}
+ </div>`;
+ }
+
+ private renderSecondSlot() {
+ if (!this.unifiedDiff) return nothing;
+ if (this.line(Side.LEFT)?.type !== GrDiffLineType.BOTH) return nothing;
+ return html`<slot
+ name="${Side.LEFT}-${this.lineNumber(Side.LEFT)}"
+ ></slot>`;
}
private contentRef(side: Side) {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
index 27fc920..1c7b311 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
@@ -93,6 +93,66 @@
);
});
+ test('both unified', async () => {
+ const line = new GrDiffLine(GrDiffLineType.BOTH, 1, 1);
+ line.text = 'lorem ipsum';
+ element.left = line;
+ element.right = line;
+ element.unifiedDiff = true;
+ await element.updateComplete;
+ assert.lightDom.equal(
+ element,
+ /* HTML */ `
+ <table>
+ <tbody>
+ <tr
+ aria-labelledby="left-button-1 right-button-1 right-content-1"
+ class="both diff-row gr-diff unified"
+ tabindex="-1"
+ >
+ <td class="blame gr-diff" data-line-number="1"></td>
+ <td class="gr-diff left lineNum" data-value="1">
+ <button
+ aria-label="1 unmodified"
+ class="gr-diff left lineNumButton"
+ data-value="1"
+ id="left-button-1"
+ tabindex="-1"
+ >
+ 1
+ </button>
+ </td>
+ <td class="gr-diff lineNum right" data-value="1">
+ <button
+ aria-label="1 unmodified"
+ class="gr-diff lineNumButton right"
+ data-value="1"
+ id="right-button-1"
+ tabindex="-1"
+ >
+ 1
+ </button>
+ </td>
+ <td class="both content gr-diff no-intraline-info right">
+ <div
+ class="contentText gr-diff"
+ data-side="right"
+ id="right-content-1"
+ >
+ <gr-diff-text> lorem ipsum </gr-diff-text>
+ </div>
+ <div class="thread-group" data-side="right">
+ <slot name="right-1"> </slot>
+ <slot name="left-1"> </slot>
+ </div>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ `
+ );
+ });
+
test('add', async () => {
const line = new GrDiffLine(GrDiffLineType.ADD, 0, 1);
line.text = 'lorem ipsum';
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index 92175eb..38bd707 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -171,18 +171,20 @@
cssClass: string,
firstPart?: boolean
) {
- if (this.getLength(node) === offset || offset === 0) {
- return this.wrapInHighlight(node, cssClass);
- } else {
- if (firstPart) {
- this.splitNode(node, offset);
- // Node points to first part of the Text, second one is sibling.
- } else {
- // if node is Text then splitNode will return a Text
- node = this.splitNode(node, offset) as Text;
- }
+ if (
+ (this.getLength(node) === offset && firstPart) ||
+ (offset === 0 && !firstPart)
+ ) {
return this.wrapInHighlight(node, cssClass);
}
+ if (firstPart) {
+ this.splitNode(node, offset);
+ // Node points to first part of the Text, second one is sibling.
+ } else {
+ // if node is Text then splitNode will return a Text
+ node = this.splitNode(node, offset) as Text;
+ }
+ return this.wrapInHighlight(node, cssClass);
},
/**
@@ -225,7 +227,6 @@
*/
splitTextNode(node: Text, offset: number) {
if (node.textContent?.match(REGEX_ASTRAL_SYMBOL)) {
- // TODO (viktard): Polyfill Array.from for IE10.
const head = Array.from(node.textContent);
const tail = head.splice(offset);
const parent = node.parentNode;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
index fdf1785..f319a3c 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
@@ -27,79 +27,77 @@
str = textNode.textContent!;
});
+ test('_annotateText length:0 offset:0', () => {
+ GrAnnotation._annotateText(textNode, 0, 0, 'foobar');
+
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ '<hl class="foobar"></hl>Lorem ipsum dolor sit amet, suspendisse inceptos vehicula'
+ );
+ });
+
+ test('_annotateText length:0 offset:1', () => {
+ GrAnnotation._annotateText(textNode, 1, 0, 'foobar');
+
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ 'L<hl class="foobar"></hl>orem ipsum dolor sit amet, suspendisse inceptos vehicula'
+ );
+ });
+
+ test('_annotateText length:0 offset:str.length', () => {
+ GrAnnotation._annotateText(textNode, str.length, 0, 'foobar');
+
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ 'Lorem ipsum dolor sit amet, suspendisse inceptos vehicula<hl class="foobar"></hl>'
+ );
+ });
+
test('_annotateText Case 1', () => {
GrAnnotation._annotateText(textNode, 0, str.length, 'foobar');
- assert.equal(parent.childNodes.length, 1);
- assert.instanceOf(parent.childNodes[0], HTMLElement);
- const firstChild = parent.childNodes[0] as HTMLElement;
- assert.equal(firstChild.className, 'foobar');
- assert.instanceOf(firstChild.childNodes[0], Text);
- assert.equal(firstChild.childNodes[0].textContent, str);
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ '<hl class="foobar">Lorem ipsum dolor sit amet, suspendisse inceptos vehicula</hl>'
+ );
});
test('_annotateText Case 2', () => {
- const length = 12;
- const substr = str.substr(0, length);
- const remainder = str.substr(length);
+ GrAnnotation._annotateText(textNode, 0, 12, 'foobar');
- GrAnnotation._annotateText(textNode, 0, length, 'foobar');
-
- assert.equal(parent.childNodes.length, 2);
-
- assert.instanceOf(parent.childNodes[0], HTMLElement);
- const firstChild = parent.childNodes[0] as HTMLElement;
- assert.equal(firstChild.className, 'foobar');
- assert.instanceOf(firstChild.childNodes[0], Text);
- assert.equal(firstChild.childNodes[0].textContent, substr);
-
- assert.instanceOf(parent.childNodes[1], Text);
- assert.equal(parent.childNodes[1].textContent, remainder);
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ '<hl class="foobar">Lorem ipsum </hl>dolor sit amet, suspendisse inceptos vehicula'
+ );
});
test('_annotateText Case 3', () => {
- const index = 12;
- const length = str.length - index;
- const remainder = str.substr(0, index);
- const substr = str.substr(index);
+ GrAnnotation._annotateText(textNode, 12, str.length - 12, 'foobar');
- GrAnnotation._annotateText(textNode, index, length, 'foobar');
-
- assert.equal(parent.childNodes.length, 2);
-
- assert.instanceOf(parent.childNodes[0], Text);
- assert.equal(parent.childNodes[0].textContent, remainder);
-
- const secondChild = parent.childNodes[1] as HTMLElement;
- assert.instanceOf(secondChild, HTMLElement);
- assert.equal(secondChild.className, 'foobar');
- assert.instanceOf(secondChild.childNodes[0], Text);
- assert.equal(secondChild.childNodes[0].textContent, substr);
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ 'Lorem ipsum <hl class="foobar">dolor sit amet, suspendisse inceptos vehicula</hl>'
+ );
});
test('_annotateText Case 4', () => {
const index = str.indexOf('dolor');
const length = 'dolor '.length;
- const remainderPre = str.substr(0, index);
- const substr = str.substr(index, length);
- const remainderPost = str.substr(index + length);
-
GrAnnotation._annotateText(textNode, index, length, 'foobar');
- assert.equal(parent.childNodes.length, 3);
-
- assert.instanceOf(parent.childNodes[0], Text);
- assert.equal(parent.childNodes[0].textContent, remainderPre);
-
- const secondChild = parent.childNodes[1] as HTMLElement;
- assert.instanceOf(secondChild, HTMLElement);
- assert.equal(secondChild.className, 'foobar');
- assert.instanceOf(secondChild.childNodes[0], Text);
- assert.equal(secondChild.childNodes[0].textContent, substr);
-
- assert.instanceOf(parent.childNodes[2], Text);
- assert.equal(parent.childNodes[2].textContent, remainderPost);
+ assert.equal(parent.textContent, str);
+ assert.equal(
+ parent.innerHTML,
+ 'Lorem ipsum <hl class="foobar">dolor </hl>sit amet, suspendisse inceptos vehicula'
+ );
});
test('_annotateElement design doc example', () => {
@@ -116,45 +114,9 @@
});
assert.equal(parent.textContent, str);
-
- // Layer 1:
- const layer1 = parent.querySelectorAll<HTMLElement>('.layer-1');
- assert.equal(layer1.length, 1);
- assert.equal(layer1[0].textContent, layers[0]);
- assert.equal(layer1[0].parentElement, parent);
-
- // Layer 2:
- const layer2 = parent.querySelectorAll<HTMLElement>('.layer-2');
- assert.equal(layer2.length, 1);
- assert.equal(layer2[0].textContent, layers[1]);
- assert.equal(layer2[0].parentElement, parent);
-
- // Layer 3:
- const layer3 = parent.querySelectorAll<HTMLElement>('.layer-3');
- assert.equal(layer3.length, 1);
- assert.equal(layer3[0].textContent, layers[2]);
- assert.equal(layer3[0].parentElement, layer1[0]);
-
- // Layer 4:
- const layer4 = parent.querySelectorAll<HTMLElement>('.layer-4');
- assert.equal(layer4.length, 3);
-
- assert.equal(layer4[0].textContent, 'et, ');
- assert.equal(layer4[0].parentElement, layer3[0]);
-
- assert.equal(layer4[1].textContent, 'suspendisse ');
- assert.equal(layer4[1].parentElement, parent);
-
- assert.equal(layer4[2].textContent, 'ince');
- assert.equal(layer4[2].parentElement, layer2[0]);
-
assert.equal(
- [
- layer4[0].textContent,
- layer4[1].textContent,
- layer4[2].textContent,
- ].join(''),
- layers[3]
+ parent.innerHTML,
+ 'Lorem ipsum dolor sit <hl class="layer-1"><hl class="layer-3">am<hl class="layer-4">et, </hl></hl></hl><hl class="layer-4">suspendisse </hl><hl class="layer-2"><hl class="layer-4">ince</hl>ptos </hl>vehicula'
);
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index bbe091f..ce1393d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -3218,6 +3218,7 @@
</div>
<div class="thread-group" data-side="right">
<slot name="right-FILE"> </slot>
+ <slot name="left-FILE"> </slot>
</div>
</td>
</tr>
diff --git a/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.ts b/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.ts
index 58f3f75..38eecfa 100644
--- a/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.ts
+++ b/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.ts
@@ -144,12 +144,13 @@
side,
range,
operation: (forLine, startChar, endChar) => {
- forLine.push({
- start: startChar,
- end: endChar,
- id: id(commentRange),
- longRange,
- });
+ if (startChar !== endChar)
+ forLine.push({
+ start: startChar,
+ end: endChar,
+ id: id(commentRange),
+ longRange,
+ });
},
});
}
@@ -202,7 +203,7 @@
// Normalize invalid ranges where the start is after the end but the
// start still makes sense. Set the end to the end of the line.
// @see Issue 5744
- if (range.start >= range.end && range.start < line.text.length) {
+ if (range.start > range.end && range.start < line.text.length) {
range.end = line.text.length;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.ts b/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.ts
index 33515b25..7feda47 100644
--- a/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.ts
@@ -69,6 +69,16 @@
},
};
+const rangeF: CommentRangeLayer = {
+ side: Side.RIGHT,
+ range: {
+ end_character: 0,
+ end_line: 24,
+ start_character: 0,
+ start_line: 23,
+ },
+};
+
suite('gr-ranged-comment-layer', () => {
let element: GrRangedCommentLayer;
@@ -79,6 +89,7 @@
rangeC,
rangeD,
rangeE,
+ rangeF,
];
element = new GrRangedCommentLayer();
@@ -219,6 +230,16 @@
);
});
+ test('do not annotate lines with end_character 0', () => {
+ line = new GrDiffLine(GrDiffLineType.BOTH);
+ line.afterNumber = 24;
+ el.setAttribute('data-side', Side.RIGHT);
+
+ element.annotate(el, lineNumberEl, line);
+
+ assert.isFalse(annotateElementStub.called);
+ });
+
test('updateRanges remove all', () => {
assertHasRange(rangeA, true);
assertHasRange(rangeB, true);
