Merge changes Idd5107ff,Ie06b9d83,Icc81c944,I3a0694e5,Ice8ec80d
* changes:
Refactor <gr-confirm-submit-dialog>
Don't hand down the ChangeComment object, instead subscribe to the model
Refactoring of <gr-patch-range-select>
Make commets-service responsible for loading comments based on state
Get rid of <gr-comment-api> element
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0a203cc..33e7804 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -8263,19 +8263,19 @@
The `SubmitRequirementExpressionInfo` describes the result of evaluating a
single submit requirement expression, for example `label:code-review=+2`.
-[options="header",cols="1,6"]
+[options="header",cols="1,^1,5"]
|===========================
-|Field Name |Description
-|`expression`|
+|Field Name ||Description
+|`expression`|optional|
The submit requirement expression as a string, for example
`branch:refs/heads/foo and label:verified=+1`.
-|`fulfilled`|
+|`fulfilled`||
True if the submit requirement is fulfilled for the change.
-|`passing_atoms`|
+|`passing_atoms`|optional|
A list of passing atoms as strings. For the above expression,
`passing_atoms` can contain ["branch:refs/heads/foo"] if the branch predicate is
fulfilled for the change.
-|`failing_atoms`|
+|`failing_atoms`|optional|
A list of failing atoms. This is similar to `passing_atoms` except that it
contains the list of predicates that are not fulfilled for the change.
|===========================
@@ -8329,6 +8329,8 @@
A link:#submit-requirement-expression-info[SubmitRequirementExpressionInfo]
containing the result of evaluating the applicability expression. Not set if the
submit requirement did not define an applicability expression.
+Note that fields `expression`, `passing_atoms` and `failing_atoms` are always
+omitted for the `applicability_expression_result`.
|`submittability_expression_result`||
A link:#submit-requirement-expression-info[SubmitRequirementExpressionInfo]
containing the result of evaluating the submittability expression.
diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt
index 128bae6..20ad07c 100644
--- a/Documentation/user-notify.txt
+++ b/Documentation/user-notify.txt
@@ -11,9 +11,9 @@
Those are the available recipient types:
-* `to`: The standard To field is used; addresses are visible to all.
-* `cc`: The standard CC field is used; addresses are visible to all.
-* `bcc`: SMTP RCPT TO is used to hide the address.
+* `TO`: The standard To field is used; addresses are visible to all.
+* `CC`: The standard CC field is used; addresses are visible to all.
+* `BCC`: SMTP RCPT TO is used to hide the address.
[[user]]
== User Level Settings
diff --git a/java/com/google/gerrit/server/change/BatchAbandon.java b/java/com/google/gerrit/server/change/BatchAbandon.java
index e0a72ac4..9a3c388 100644
--- a/java/com/google/gerrit/server/change/BatchAbandon.java
+++ b/java/com/google/gerrit/server/change/BatchAbandon.java
@@ -20,6 +20,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.config.ChangeCleanupConfig;
+import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
import com.google.gerrit.server.plugincontext.PluginItemContext;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
@@ -34,15 +35,18 @@
private final AbandonOp.Factory abandonOpFactory;
private final ChangeCleanupConfig cfg;
private final PluginItemContext<AccountPatchReviewStore> accountPatchReviewStore;
+ private final StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory;
@Inject
BatchAbandon(
AbandonOp.Factory abandonOpFactory,
ChangeCleanupConfig cfg,
- PluginItemContext<AccountPatchReviewStore> accountPatchReviewStore) {
+ PluginItemContext<AccountPatchReviewStore> accountPatchReviewStore,
+ StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory) {
this.abandonOpFactory = abandonOpFactory;
this.cfg = cfg;
this.accountPatchReviewStore = accountPatchReviewStore;
+ this.storeSubmitRequirementsOpFactory = storeSubmitRequirementsOpFactory;
}
/**
@@ -74,6 +78,7 @@
change.project().get(), project.get()));
}
u.addOp(change.getId(), abandonOpFactory.create(accountState, msgTxt));
+ u.addOp(change.getId(), storeSubmitRequirementsOpFactory.create());
}
u.execute();
diff --git a/java/com/google/gerrit/server/change/SubmitRequirementsJson.java b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
index 8eeec62..4833197 100644
--- a/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
+++ b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
@@ -36,28 +36,36 @@
if (req.applicabilityExpression().isPresent()) {
info.applicabilityExpressionResult =
submitRequirementExpressionToInfo(
- req.applicabilityExpression().get(), result.applicabilityExpressionResult().get());
+ req.applicabilityExpression().get(),
+ result.applicabilityExpressionResult().get(),
+ /* hide= */ true); // Always hide applicability expressions on the API
}
if (req.overrideExpression().isPresent()) {
info.overrideExpressionResult =
submitRequirementExpressionToInfo(
- req.overrideExpression().get(), result.overrideExpressionResult().get());
+ req.overrideExpression().get(),
+ result.overrideExpressionResult().get(),
+ /* hide= */ false);
}
info.submittabilityExpressionResult =
submitRequirementExpressionToInfo(
- req.submittabilityExpression(), result.submittabilityExpressionResult());
+ req.submittabilityExpression(),
+ result.submittabilityExpressionResult(),
+ /* hide= */ false);
info.status = SubmitRequirementResultInfo.Status.valueOf(result.status().toString());
info.isLegacy = result.isLegacy();
return info;
}
private static SubmitRequirementExpressionInfo submitRequirementExpressionToInfo(
- SubmitRequirementExpression expression, SubmitRequirementExpressionResult result) {
+ SubmitRequirementExpression expression,
+ SubmitRequirementExpressionResult result,
+ boolean hide) {
SubmitRequirementExpressionInfo info = new SubmitRequirementExpressionInfo();
- info.expression = expression.expressionString();
+ info.expression = hide ? null : expression.expressionString();
info.fulfilled = result.status().equals(SubmitRequirementExpressionResult.Status.PASS);
- info.passingAtoms = result.passingAtoms();
- info.failingAtoms = result.failingAtoms();
+ info.passingAtoms = hide ? null : result.passingAtoms();
+ info.failingAtoms = hide ? null : result.failingAtoms();
return info;
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Abandon.java b/java/com/google/gerrit/server/restapi/change/Abandon.java
index 2cfc3f5..affe947 100644
--- a/java/com/google/gerrit/server/restapi/change/Abandon.java
+++ b/java/com/google/gerrit/server/restapi/change/Abandon.java
@@ -30,6 +30,7 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
@@ -48,6 +49,7 @@
private final AbandonOp.Factory abandonOpFactory;
private final NotifyResolver notifyResolver;
private final PatchSetUtil patchSetUtil;
+ private final StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory;
@Inject
Abandon(
@@ -55,12 +57,14 @@
ChangeJson.Factory json,
AbandonOp.Factory abandonOpFactory,
NotifyResolver notifyResolver,
- PatchSetUtil patchSetUtil) {
+ PatchSetUtil patchSetUtil,
+ StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory) {
this.updateFactory = updateFactory;
this.json = json;
this.abandonOpFactory = abandonOpFactory;
this.notifyResolver = notifyResolver;
this.patchSetUtil = patchSetUtil;
+ this.storeSubmitRequirementsOpFactory = storeSubmitRequirementsOpFactory;
}
@Override
@@ -119,7 +123,9 @@
AbandonOp op = abandonOpFactory.create(accountState, msgTxt);
try (BatchUpdate u = updateFactory.create(notes.getProjectName(), user, TimeUtil.nowTs())) {
u.setNotify(notify);
- u.addOp(notes.getChangeId(), op).execute();
+ u.addOp(notes.getChangeId(), op);
+ u.addOp(notes.getChangeId(), storeSubmitRequirementsOpFactory.create());
+ u.execute();
}
return op.getChange();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 74407c0..58c222a 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -89,6 +89,7 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.Account;
@@ -153,6 +154,7 @@
import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.common.SubmitRecordInfo;
+import com.google.gerrit.extensions.common.SubmitRequirementExpressionInfo;
import com.google.gerrit.extensions.common.SubmitRequirementInput;
import com.google.gerrit.extensions.common.SubmitRequirementResultInfo;
import com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status;
@@ -4642,6 +4644,131 @@
ExperimentFeaturesConstants
.GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
})
+ public void submitRequirement_storedForAbandonedChanges() throws Exception {
+ for (SubmitType submitType : SubmitType.values()) {
+ Project.NameKey project = createProjectForPush(submitType);
+ TestRepository<InMemoryRepository> repo = cloneProject(project);
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r =
+ createChange(repo, "master", "Add a file", "foo", "content", "topic");
+ String changeId = r.getChangeId();
+
+ voteLabel(changeId, "Code-Review", 2);
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ false);
+
+ gApi.changes().id(r.getChangeId()).abandon();
+ ChangeNotes notes = notesFactory.create(project, r.getChange().getId());
+ SubmitRequirementResult result =
+ notes.getSubmitRequirementsResult().stream().collect(MoreCollectors.onlyElement());
+ assertThat(result.status()).isEqualTo(SubmitRequirementResult.Status.SATISFIED);
+ assertThat(result.submittabilityExpressionResult().status())
+ .isEqualTo(SubmitRequirementExpressionResult.Status.PASS);
+ assertThat(result.submittabilityExpressionResult().expression().expressionString())
+ .isEqualTo("label:Code-Review=+2");
+ }
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
+ public void submitRequirement_retrievedFromNoteDbForAbandonedChanges() throws Exception {
+ for (SubmitType submitType : SubmitType.values()) {
+ Project.NameKey project = createProjectForPush(submitType);
+ TestRepository<InMemoryRepository> repo = cloneProject(project);
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r =
+ createChange(repo, "master", "Add a file", "foo", "content", "topic");
+ String changeId = r.getChangeId();
+ voteLabel(changeId, "Code-Review", 2);
+ gApi.changes().id(changeId).abandon();
+
+ // Add another submit requirement. This will not get returned for the abandoned change, since
+ // we return the state of the SR results when the change was abandoned.
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("New-Requirement")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("-has:unresolved"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ ChangeInfo changeInfo =
+ gApi.changes().id(changeId).get(ListChangesOption.SUBMIT_REQUIREMENTS);
+ assertThat(changeInfo.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(
+ changeInfo.submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:Code-Review=+2");
+
+ // Restore the change, the new requirement will show up
+ gApi.changes().id(changeId).restore();
+ changeInfo = gApi.changes().id(changeId).get(ListChangesOption.SUBMIT_REQUIREMENTS);
+ assertThat(changeInfo.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ changeInfo.submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:Code-Review=+2");
+ assertSubmitRequirementStatus(
+ changeInfo.submitRequirements,
+ "New-Requirement",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "-has:unresolved");
+
+ // Abandon again, make sure the new requirement was persisted
+ gApi.changes().id(changeId).abandon();
+ changeInfo = gApi.changes().id(changeId).get(ListChangesOption.SUBMIT_REQUIREMENTS);
+ assertThat(changeInfo.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ changeInfo.submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:Code-Review=+2");
+ assertSubmitRequirementStatus(
+ changeInfo.submitRequirements,
+ "New-Requirement",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "-has:unresolved");
+ }
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
public void submitRequirement_retrievedFromNoteDbForClosedChanges() throws Exception {
configSubmitRequirement(
project,
@@ -4934,6 +5061,35 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
+ public void submitRequirement_applicabilityExpressionIsAlwaysHidden() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setApplicabilityExpression(SubmitRequirementExpression.of("branch:refs/heads/master"))
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:Code-Review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ voteLabel(changeId, "Code-Review", 2);
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ SubmitRequirementResultInfo requirement =
+ changeInfo.submitRequirements.stream().collect(MoreCollectors.onlyElement());
+ assertSubmitRequirementExpression(
+ requirement.applicabilityExpressionResult,
+ /* expression= */ null,
+ /* passingAtoms= */ null,
+ /* failingAtoms= */ null,
+ /* fulfilled= */ true);
+ }
+
+ @Test
public void submitRequirements_notServedIfExperimentNotEnabled() throws Exception {
configSubmitRequirement(
project,
@@ -5550,6 +5706,26 @@
.collect(toImmutableList())));
}
+ private void assertSubmitRequirementExpression(
+ SubmitRequirementExpressionInfo result,
+ @Nullable String expression,
+ @Nullable List<String> passingAtoms,
+ @Nullable List<String> failingAtoms,
+ boolean fulfilled) {
+ assertThat(result.expression).isEqualTo(expression);
+ if (passingAtoms == null) {
+ assertThat(result.passingAtoms).isNull();
+ } else {
+ assertThat(result.passingAtoms).containsExactlyElementsIn(passingAtoms);
+ }
+ if (failingAtoms == null) {
+ assertThat(result.failingAtoms).isNull();
+ } else {
+ assertThat(result.failingAtoms).containsExactlyElementsIn(failingAtoms);
+ }
+ assertThat(result.fulfilled).isEqualTo(fulfilled);
+ }
+
private Project.NameKey createProjectForPush(SubmitType submitType) throws Exception {
Project.NameKey project = projectOperations.newProject().submitType(submitType).create();
projectOperations
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
index 3e292fe..a47db20 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
@@ -408,6 +408,7 @@
range: Text | Element | Range
) {
if (startLine > 1) {
+ actionBox.positionBelow = false;
actionBox.placeAbove(range);
return;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
index 551889f..0f64d9e 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -18,7 +18,6 @@
import '../../shared/gr-tooltip/gr-tooltip';
import {GrTooltip} from '../../shared/gr-tooltip/gr-tooltip';
import {customElement, property} from '@polymer/decorators';
-import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-selection-action-box_html';
import {fireEvent} from '../../../utils/event-util';
@@ -56,8 +55,8 @@
this.addEventListener('mousedown', e => this._handleMouseDown(e));
}
- placeAbove(el: Text | Element | Range) {
- flush();
+ async placeAbove(el: Text | Element | Range) {
+ await this.$.tooltip.updateComplete;
const rect = this._getTargetBoundingRect(el);
const boxRect = this.$.tooltip.getBoundingClientRect();
const parentRect = this._getParentBoundingClientRect();
@@ -70,8 +69,8 @@
}px`;
}
- placeBelow(el: Text | Element | Range) {
- flush();
+ async placeBelow(el: Text | Element | Range) {
+ await this.$.tooltip.updateComplete;
const rect = this._getTargetBoundingRect(el);
const boxRect = this.$.tooltip.getBoundingClientRect();
const parentRect = this._getParentBoundingClientRect();
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
index 24d63b3..558742a 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
@@ -23,6 +23,9 @@
font-family: var(--font-family);
position: absolute;
white-space: nowrap;
+ /* This prevents the mouse over the tooltip from interfering with the
+ selection. */
+ pointer-events: none;
}
</style>
<gr-tooltip
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
index 81cf0d6..c978c37 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
@@ -83,35 +83,35 @@
{width: 10, height: 10});
});
- test('placeAbove for Element argument', () => {
- element.placeAbove(target);
+ test('placeAbove for Element argument', async () => {
+ await element.placeAbove(target);
assert.equal(element.style.top, '25px');
assert.equal(element.style.left, '72px');
});
- test('placeAbove for Text Node argument', () => {
- element.placeAbove(target.firstChild);
+ test('placeAbove for Text Node argument', async () => {
+ await element.placeAbove(target.firstChild);
assert.equal(element.style.top, '25px');
assert.equal(element.style.left, '72px');
});
- test('placeBelow for Element argument', () => {
- element.placeBelow(target);
+ test('placeBelow for Element argument', async () => {
+ await element.placeBelow(target);
assert.equal(element.style.top, '45px');
assert.equal(element.style.left, '72px');
});
- test('placeBelow for Text Node argument', () => {
- element.placeBelow(target.firstChild);
+ test('placeBelow for Text Node argument', async () => {
+ await element.placeBelow(target.firstChild);
assert.equal(element.style.top, '45px');
assert.equal(element.style.left, '72px');
});
- test('uses document.createRange', () => {
+ test('uses document.createRange', async () => {
sinon.spy(document, 'createRange');
element._getTargetBoundingRect.restore();
sinon.spy(element, '_getTargetBoundingRect');
- element.placeAbove(target.firstChild);
+ await element.placeAbove(target.firstChild);
assert.isTrue(document.createRange.called);
});
});
