Merge "Ignore duplicate entries on project config updates via the Set Access EP"
diff --git a/java/com/google/gerrit/entities/PermissionRule.java b/java/com/google/gerrit/entities/PermissionRule.java
index 9a2d31e..1665c1c 100644
--- a/java/com/google/gerrit/entities/PermissionRule.java
+++ b/java/com/google/gerrit/entities/PermissionRule.java
@@ -202,6 +202,9 @@
int dotdot = range.indexOf("..");
int min = parseInt(range.substring(0, dotdot));
int max = parseInt(range.substring(dotdot + 2));
+ if (min > max) {
+ throw new IllegalArgumentException("Invalid range in rule: " + orig);
+ }
rule.setRange(min, max);
} else {
throw new IllegalArgumentException("Invalid range in rule: " + orig);
diff --git a/java/com/google/gerrit/server/restapi/project/SetAccess.java b/java/com/google/gerrit/server/restapi/project/SetAccess.java
index 23d60fe..6957275 100644
--- a/java/com/google/gerrit/server/restapi/project/SetAccess.java
+++ b/java/com/google/gerrit/server/restapi/project/SetAccess.java
@@ -19,6 +19,9 @@
import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.InvalidNameException;
+import com.google.gerrit.extensions.api.access.AccessSectionInfo;
+import com.google.gerrit.extensions.api.access.PermissionInfo;
+import com.google.gerrit.extensions.api.access.PermissionRuleInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -39,6 +42,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.List;
+import java.util.Map;
import org.eclipse.jgit.errors.ConfigInvalidException;
@Singleton
@@ -80,6 +84,8 @@
throws Exception {
MetaDataUpdate.User metaDataUpdateUser = metaDataUpdateFactory.get();
+ validateInput(input);
+
ProjectConfig config;
List<AccessSection> removals =
@@ -137,4 +143,65 @@
return Response.ok(getAccess.apply(rsrc.getNameKey()));
}
+
+ private static void validateInput(ProjectAccessInput input) throws BadRequestException {
+ if (input.add != null) {
+ for (Map.Entry<String, AccessSectionInfo> accessSectionEntry : input.add.entrySet()) {
+ validateAccessSection(accessSectionEntry.getKey(), accessSectionEntry.getValue());
+ }
+ }
+ }
+
+ private static void validateAccessSection(String ref, AccessSectionInfo accessSectionInfo)
+ throws BadRequestException {
+ if (accessSectionInfo != null) {
+ for (Map.Entry<String, PermissionInfo> permissionEntry :
+ accessSectionInfo.permissions.entrySet()) {
+ validatePermission(ref, permissionEntry.getKey(), permissionEntry.getValue());
+ }
+ }
+ }
+
+ private static void validatePermission(
+ String ref, String permission, PermissionInfo permissionInfo) throws BadRequestException {
+ if (permissionInfo != null) {
+ for (Map.Entry<String, PermissionRuleInfo> permissionRuleEntry :
+ permissionInfo.rules.entrySet()) {
+ validatePermissionRule(
+ ref, permission, permissionRuleEntry.getKey(), permissionRuleEntry.getValue());
+ }
+ }
+ }
+
+ private static void validatePermissionRule(
+ String ref, String permission, String groupId, PermissionRuleInfo permissionRuleInfo)
+ throws BadRequestException {
+ if (permissionRuleInfo != null) {
+ if (permissionRuleInfo.min != null || permissionRuleInfo.max != null) {
+ if (permissionRuleInfo.min == null) {
+ throw new BadRequestException(
+ String.format(
+ "Invalid range for permission rule that assigns %s to group %s on ref %s:"
+ + " ..%d (min is required if max is set)",
+ permission, groupId, ref, permissionRuleInfo.max));
+ }
+
+ if (permissionRuleInfo.max == null) {
+ throw new BadRequestException(
+ String.format(
+ "Invalid range for permission rule that assigns %s to group %s on ref %s:"
+ + " %d.. (max is required if min is set)",
+ permission, groupId, ref, permissionRuleInfo.min));
+ }
+
+ if (permissionRuleInfo.min > permissionRuleInfo.max) {
+ throw new BadRequestException(
+ String.format(
+ "Invalid range for permission rule that assigns %s to group %s on ref %s:"
+ + " %d..%d (min must be <= max)",
+ permission, groupId, ref, permissionRuleInfo.min, permissionRuleInfo.max));
+ }
+ }
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
index 630958c..6bd2b68 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/AccessIT.java
@@ -359,6 +359,79 @@
}
@Test
+ public void addAccessSectionWithInvalidLabelRange_minGreaterThanMax() throws Exception {
+ ProjectAccessInput accessInput = newProjectAccessInput();
+ AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+ PermissionInfo permissionInfo = newPermissionInfo();
+ PermissionRuleInfo permissionRuleInfo =
+ new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+ permissionInfo.rules.put(SystemGroupBackend.REGISTERED_USERS.get(), permissionRuleInfo);
+ permissionRuleInfo.min = 1;
+ permissionRuleInfo.max = -1;
+ accessSectionInfo.permissions.put("label-Code-Review", permissionInfo);
+
+ accessInput.add.put(REFS_HEADS, accessSectionInfo);
+ BadRequestException ex =
+ assertThrows(BadRequestException.class, () -> pApi().access(accessInput));
+ assertThat(ex)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Invalid range for permission rule that assigns label-Code-Review to group %s"
+ + " on ref refs/heads/*: 1..-1 (min must be <= max)",
+ SystemGroupBackend.REGISTERED_USERS.get()));
+ }
+
+ @Test
+ public void addAccessSectionWithInvalidLabelRange_minSetMaxMissing() throws Exception {
+ ProjectAccessInput accessInput = newProjectAccessInput();
+ AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+ PermissionInfo permissionInfo = newPermissionInfo();
+ PermissionRuleInfo permissionRuleInfo =
+ new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+ permissionInfo.rules.put(SystemGroupBackend.REGISTERED_USERS.get(), permissionRuleInfo);
+ permissionRuleInfo.min = -1;
+ accessSectionInfo.permissions.put("label-Code-Review", permissionInfo);
+
+ accessInput.add.put(REFS_HEADS, accessSectionInfo);
+ BadRequestException ex =
+ assertThrows(BadRequestException.class, () -> pApi().access(accessInput));
+ assertThat(ex)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Invalid range for permission rule that assigns label-Code-Review to group %s"
+ + " on ref refs/heads/*: -1.. (max is required if min is set)",
+ SystemGroupBackend.REGISTERED_USERS.get()));
+ }
+
+ @Test
+ public void addAccessSectionWithInvalidLabelRange_maxSetMinMissing() throws Exception {
+ ProjectAccessInput accessInput = newProjectAccessInput();
+ AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+ PermissionInfo permissionInfo = newPermissionInfo();
+ PermissionRuleInfo permissionRuleInfo =
+ new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+ permissionInfo.rules.put(SystemGroupBackend.REGISTERED_USERS.get(), permissionRuleInfo);
+ permissionRuleInfo.max = 1;
+ accessSectionInfo.permissions.put("label-Code-Review", permissionInfo);
+
+ accessInput.add.put(REFS_HEADS, accessSectionInfo);
+ BadRequestException ex =
+ assertThrows(BadRequestException.class, () -> pApi().access(accessInput));
+ assertThat(ex)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Invalid range for permission rule that assigns label-Code-Review to group %s"
+ + " on ref refs/heads/*: ..1 (min is required if max is set)",
+ SystemGroupBackend.REGISTERED_USERS.get()));
+ }
+
+ @Test
public void createAccessChangeNop() throws Exception {
ProjectAccessInput accessInput = newProjectAccessInput();
assertThrows(BadRequestException.class, () -> pApi().accessChange(accessInput));
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 5fd2159..28a0196 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.acceptance.GitUtil.fetch;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -32,6 +33,8 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.git.ObjectIds;
+import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.project.GroupList;
import com.google.gerrit.server.project.LabelConfigValidator;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.Inject;
@@ -155,6 +158,81 @@
}
@Test
+ public void rejectCreatingLabelPermissionWithInvalidRange_minGreaterThanMax() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ ProjectConfig.PROJECT_CONFIG,
+ "[access \"refs/heads/*\"]\n label-Code-Review = 1..-1 group Registered-Users",
+ GroupList.FILE_NAME,
+ String.format("%s\tRegistered-Users", SystemGroupBackend.REGISTERED_USERS.get())));
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: invalid rule in"
+ + " access.refs/heads/*.label-Code-Review:"
+ + " invalid range in rule: 1..-1 group Registered-Users",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectCreatingLabelPermissionWithInvalidRange_minSetMaxMissing() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ ProjectConfig.PROJECT_CONFIG,
+ "[access \"refs/heads/*\"]\n label-Code-Review = -1.. group Registered-Users",
+ GroupList.FILE_NAME,
+ String.format("%s\tRegistered-Users", SystemGroupBackend.REGISTERED_USERS.get())));
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: invalid rule in"
+ + " access.refs/heads/*.label-Code-Review:"
+ + " invalid range in rule: -1.. group Registered-Users",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectCreatingLabelPermissionWithInvalidRange_maxSetMinMissing() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ ProjectConfig.PROJECT_CONFIG,
+ "[access \"refs/heads/*\"]\n label-Code-Review = ..1 group Registered-Users",
+ GroupList.FILE_NAME,
+ String.format("%s\tRegistered-Users", SystemGroupBackend.REGISTERED_USERS.get())));
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: invalid rule in"
+ + " access.refs/heads/*.label-Code-Review:"
+ + " invalid range in rule: ..1 group Registered-Users",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
public void rejectSettingCopyMinScore() throws Exception {
testRejectSettingLabelFlag(
LabelConfigValidator.KEY_COPY_MIN_SCORE, /* value= */ true, "is:MIN");
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);
