Merge "Remove all generic CustomEvents"
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 70352dc..e6494e6 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -199,6 +199,14 @@
=== Change
+* `change/count_rebases`: Total number of rebases
+** `on_behalf_of_uploader`:
+ Whether the rebase was done on behalf of the uploader.
+** `rebase_chain`:
+ Whether a chain was rebased.
+* `change/submitted_with_rebaser_approval`: Number of rebased changes that were
+ submitted with a Code-Review approval of the rebaser that would not have been
+ submittable if the rebase was not done on behalf of the uploader.
* `change/submit_rule_evaluation`: Latency for evaluating submit rules on a
change.
* `change/submit_type_evaluation`: Latency for evaluating the submit type on a
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
index 32a8fdf..9120069 100644
--- a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
@@ -99,6 +99,15 @@
return new EqualsLabelPredicate(args, label, value, account, count);
}
+ public String getLabel() {
+ return magicLabelVote.label();
+ }
+
+ public boolean ignoresUploaderApprovals() {
+ return account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
+ }
+
@Nullable
protected static LabelType type(LabelTypes types, String toFind) {
if (types.byLabel(toFind).isPresent()) {
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index fd51fbc..5368c75 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -84,6 +84,7 @@
private final PatchSetUtil patchSetUtil;
private final IdentifiedUser.GenericFactory userFactory;
private final ChangeResource.Factory changeResourceFactory;
+ private final RebaseMetrics rebaseMetrics;
@Inject
public Rebase(
@@ -96,7 +97,8 @@
ProjectCache projectCache,
PatchSetUtil patchSetUtil,
IdentifiedUser.GenericFactory userFactory,
- ChangeResource.Factory changeResourceFactory) {
+ ChangeResource.Factory changeResourceFactory,
+ RebaseMetrics rebaseMetrics) {
this.serverIdent = serverIdent;
this.updateFactory = updateFactory;
this.repoManager = repoManager;
@@ -107,6 +109,7 @@
this.patchSetUtil = patchSetUtil;
this.userFactory = userFactory;
this.changeResourceFactory = changeResourceFactory;
+ this.rebaseMetrics = rebaseMetrics;
}
@Override
@@ -147,6 +150,8 @@
bu.addOp(change.getId(), rebaseOp);
bu.execute();
+ rebaseMetrics.countRebase(input.onBehalfOfUploader);
+
ChangeInfo changeInfo = json.create(OPTIONS).format(change.getProject(), change.getId());
changeInfo.containsGitConflicts =
!rebaseOp.getRebasedCommit().getFilesWithGitConflicts().isEmpty() ? true : null;
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 34a2623..1949c89 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -86,6 +86,7 @@
private final ProjectCache projectCache;
private final PatchSetUtil patchSetUtil;
private final ChangeJson.Factory json;
+ private final RebaseMetrics rebaseMetrics;
@Inject
RebaseChain(
@@ -99,7 +100,8 @@
ChangeNotes.Factory notesFactory,
ProjectCache projectCache,
PatchSetUtil patchSetUtil,
- ChangeJson.Factory json) {
+ ChangeJson.Factory json,
+ RebaseMetrics rebaseMetrics) {
this.repoManager = repoManager;
this.getRelatedChangesUtil = getRelatedChangesUtil;
this.changeDataFactory = changeDataFactory;
@@ -111,6 +113,7 @@
this.projectCache = projectCache;
this.patchSetUtil = patchSetUtil;
this.json = json;
+ this.rebaseMetrics = rebaseMetrics;
}
@Override
@@ -194,6 +197,8 @@
}
}
+ rebaseMetrics.countRebaseChain(input.onBehalfOfUploader);
+
RebaseChainInfo res = new RebaseChainInfo();
res.rebasedChanges = new ArrayList<>();
ChangeJson changeJson = json.create(OPTIONS);
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
new file mode 100644
index 0000000..114a112
--- /dev/null
+++ b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
@@ -0,0 +1,54 @@
+// Copyright (C) 2023 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.
+
+package com.google.gerrit.server.restapi.change;
+
+import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/** Metrics for the rebase REST endpoints ({@link Rebase} and {@link RebaseChain}). */
+@Singleton
+public class RebaseMetrics {
+ private final Counter2<Boolean, Boolean> countRebases;
+
+ @Inject
+ public RebaseMetrics(MetricMaker metricMaker) {
+ this.countRebases =
+ metricMaker.newCounter(
+ "change/count_rebases",
+ new Description("Total number of rebases").setRate(),
+ Field.ofBoolean("on_behalf_of_uploader", (metadataBuilder, isOnBehalfOfUploader) -> {})
+ .description("Whether the rebase was done on behalf of the uploader.")
+ .build(),
+ Field.ofBoolean("rebase_chain", (metadataBuilder, isRebaseChain) -> {})
+ .description("Whether a chain was rebased.")
+ .build());
+ }
+
+ public void countRebase(boolean isOnBehalfOfUploader) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ false);
+ }
+
+ public void countRebaseChain(boolean isOnBehalfOfUploader) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ true);
+ }
+
+ private void countRebase(boolean isOnBehalfOfUploader, boolean isRebaseChain) {
+ countRebases.increment(/* field1= */ isOnBehalfOfUploader, /* field2= */ isRebaseChain);
+ }
+}
diff --git a/java/com/google/gerrit/server/submit/MergeMetrics.java b/java/com/google/gerrit/server/submit/MergeMetrics.java
new file mode 100644
index 0000000..9eb8061
--- /dev/null
+++ b/java/com/google/gerrit/server/submit/MergeMetrics.java
@@ -0,0 +1,134 @@
+// Copyright (C) 2023 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.
+
+package com.google.gerrit.server.submit;
+
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.MagicLabelPredicate;
+import com.google.gerrit.server.query.change.SubmitRequirementChangeQueryBuilder;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+/** Metrics are recorded when a change is merged (aka submitted). */
+public class MergeMetrics {
+ private final Provider<SubmitRequirementChangeQueryBuilder> submitRequirementChangequeryBuilder;
+
+ // TODO: This metric is for measuring the impact of allowing users to rebase changes on behalf of
+ // the uploader. Once this feature has been rolled out and its impact as been measured, we may
+ // remove this metric.
+ private final Counter0 countChangesThatWereSubmittedWithRebaserApproval;
+
+ @Inject
+ public MergeMetrics(
+ Provider<SubmitRequirementChangeQueryBuilder> submitRequirementChangequeryBuilder,
+ MetricMaker metricMaker) {
+ this.submitRequirementChangequeryBuilder = submitRequirementChangequeryBuilder;
+
+ this.countChangesThatWereSubmittedWithRebaserApproval =
+ metricMaker.newCounter(
+ "change/submitted_with_rebaser_approval",
+ new Description(
+ "Number of rebased changes that were submitted with a Code-Review approval of"
+ + " the rebaser that would not have been submittable if the rebase was not"
+ + " done on behalf of the uploader.")
+ .setRate());
+ }
+
+ public void countChangesThatWereSubmittedWithRebaserApproval(ChangeData cd) {
+ if (isRebaseOnBehalfOfUploader(cd)
+ && hasCodeReviewApprovalOfRealUploader(cd)
+ && ignoresCodeReviewApprovalsOfUploader(cd)) {
+ // 1. The patch set that is being submitted was created by rebasing on behalf of the uploader.
+ // The uploader of the patch set is the original uploader on whom's behalf the rebase was
+ // done. The real uploader is the user that did the rebase on behalf of the uploader (e.g. by
+ // clicking on the rebase button).
+ //
+ // 2. The change has Code-Review approvals of the real uploader (aka the rebaser).
+ //
+ // 3. Code-Review approvals of the uploader are ignored.
+ //
+ // If instead of a rebase on behalf of the uploader a normal rebase would have been done the
+ // rebaser would have been the uploader of the patch set. In this case the Code-Review
+ // approval of the rebaser would not have counted since Code-Review approvals of the uploader
+ // are ignored.
+ //
+ // In this case we assume that the change would not be submittable if a normal rebase had been
+ // done. This is not always correct (e.g. if there are approvals of multiple reviewers) but
+ // it's good enough for the metric.
+ countChangesThatWereSubmittedWithRebaserApproval.increment();
+ }
+ }
+
+ private boolean isRebaseOnBehalfOfUploader(ChangeData cd) {
+ // If the uploader differs from the real uploader the upload of the patch set has been
+ // impersonated. Impersonating the uploader is only allowed on rebase by rebasing on behalf of
+ // the uploader. Hence if the current patch set has different accounts as uploader and real
+ // uploader we can assume that it was created by rebase on behalf of the uploader.
+ return !cd.currentPatchSet().uploader().equals(cd.currentPatchSet().realUploader());
+ }
+
+ private boolean hasCodeReviewApprovalOfRealUploader(ChangeData cd) {
+ return cd.currentApprovals().stream()
+ .anyMatch(psa -> psa.accountId().equals(cd.currentPatchSet().realUploader()));
+ }
+
+ private boolean ignoresCodeReviewApprovalsOfUploader(ChangeData cd) {
+ for (SubmitRequirement submitRequirement : cd.submitRequirements().keySet()) {
+ try {
+ Predicate<ChangeData> predicate =
+ submitRequirementChangequeryBuilder
+ .get()
+ .parse(submitRequirement.submittabilityExpression().expressionString());
+ return ignoresCodeReviewApprovalsOfUploader(predicate);
+ } catch (QueryParseException e) {
+ return false;
+ }
+ }
+ return false;
+ }
+
+ private boolean ignoresCodeReviewApprovalsOfUploader(Predicate<ChangeData> predicate) {
+ if (predicate.getChildCount() == 0) {
+ // Submit requirements may require a Code-Review approval but ignore approvals by the
+ // uploader. This is done by using a label predicate with 'user=non_uploader' or
+ // 'user=non_contributor', e.g. 'label:Code-Review=+2,user=non_uploader'. After the submit
+ // requirement expression has been parsed these label predicates are represented by
+ // MagicLabelPredicate in the predicate tree. Hence to know whether Code-Review approvals of
+ // the uploader are ignored, we must check if there is any MagicLabelPredicate for the
+ // Code-Review label that ignores approvals of the uploader (aka has user set to non_uploader
+ // or non_contributor).
+ if (predicate instanceof MagicLabelPredicate) {
+ MagicLabelPredicate magicLabelPredicate = (MagicLabelPredicate) predicate;
+ if (magicLabelPredicate.getLabel().equalsIgnoreCase("Code-Review")
+ && magicLabelPredicate.ignoresUploaderApprovals()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ for (Predicate<ChangeData> childPredicate : predicate.getChildren()) {
+ if (ignoresCodeReviewApprovalsOfUploader(childPredicate)) {
+ return true;
+ }
+ }
+ return false;
+ }
+}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 1d3ec73..d299614 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -246,6 +246,7 @@
private final RetryHelper retryHelper;
private final ChangeData.Factory changeDataFactory;
private final StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory;
+ private final MergeMetrics mergeMetrics;
// Changes that were updated by this MergeOp.
private final Map<Change.Id, Change> updatedChanges;
@@ -280,7 +281,8 @@
TopicMetrics topicMetrics,
RetryHelper retryHelper,
ChangeData.Factory changeDataFactory,
- StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory) {
+ StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory,
+ MergeMetrics mergeMetrics) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.internalUserFactory = internalUserFactory;
@@ -298,6 +300,7 @@
this.changeDataFactory = changeDataFactory;
this.updatedChanges = new HashMap<>();
this.storeSubmitRequirementsOpFactory = storeSubmitRequirementsOpFactory;
+ this.mergeMetrics = mergeMetrics;
}
@Override
@@ -376,6 +379,7 @@
commitStatus.problem(cd.getId(), "Change " + cd.getId() + " is work in progress");
} else {
checkSubmitRequirements(cd);
+ mergeMetrics.countChangesThatWereSubmittedWithRebaserApproval(cd);
}
} catch (ResourceConflictException e) {
commitStatus.problem(cd.getId(), e.getMessage());
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index b1fb575..3531234 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.RawInputUtil;
@@ -89,6 +90,7 @@
@Inject protected RequestScopeOperations requestScopeOperations;
@Inject protected ProjectOperations projectOperations;
@Inject protected ExtensionRegistry extensionRegistry;
+ @Inject protected TestMetricMaker testMetricMaker;
@FunctionalInterface
protected interface RebaseCall {
@@ -771,6 +773,28 @@
assertThat(gApi.changes().id(id3.get()).revision(ri3._number).related().changes).isEmpty();
}
+
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ // Create two changes both with the same parent
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Rebase the second change
+ testMetricMaker.reset();
+ rebaseCallWithInput.call(r2.getChangeId(), new RebaseInput());
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ }
}
public static class RebaseViaRevisionApi extends Rebase {
@@ -1125,6 +1149,36 @@
assertThat(thrown).hasMessageThat().contains("recursion not allowed");
}
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ // Create changes with the following hierarchy:
+ // * HEAD
+ // * r1
+ // * r2
+ // * r3
+ // * r4
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+ PushOneCommit.Result r3 = createChange();
+ PushOneCommit.Result r4 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Rebase the chain.
+ testMetricMaker.reset();
+ verifyRebaseChainResponse(
+ gApi.changes().id(r4.getChangeId()).rebaseChain(), false, r2, r3, r4);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
+ }
+
private void verifyRebaseChainResponse(
Response<RebaseChainInfo> res,
boolean shouldHaveConflicts,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index 7030804..5a5402b 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -27,6 +27,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
@@ -70,6 +71,7 @@
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ExtensionRegistry extensionRegistry;
+ @Inject private TestMetricMaker testMetricMaker;
@Test
public void cannotRebaseOnBehalfOfUploaderWithAllowConflicts() throws Exception {
@@ -1021,6 +1023,95 @@
assertThat(gApi.changes().id(changeToBeRebased.get()).get().submittable).isFalse();
}
+ @Test
+ public void testSubmittedWithRebaserApprovalMetric() throws Exception {
+ // Require a Code-Review approval from a non-uploader for submit.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .upsertSubmitRequirement(
+ SubmitRequirement.builder()
+ .setName(TestLabels.codeReview().getName())
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create(
+ String.format(
+ "label:%s=MAX,user=non_uploader", TestLabels.codeReview().getName())))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ u.save();
+ }
+
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ testMetricMaker.reset();
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+ assertThat(testMetricMaker.getCount("change/submitted_with_rebaser_approval")).isEqualTo(0);
+
+ // Rebase it on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ // Approve the change as the rebaser.
+ allowVotingOnCodeReviewToAllUsers();
+ gApi.changes().id(changeToBeRebased.get()).current().review(ReviewInput.approve());
+
+ // The change is submittable because the approval is from a user (the rebaser) that is not the
+ // uploader.
+ allowPermissionToAllUsers(Permission.SUBMIT);
+ testMetricMaker.reset();
+ gApi.changes().id(changeToBeRebased.get()).current().submit();
+ assertThat(testMetricMaker.getCount("change/submitted_with_rebaser_approval")).isEqualTo(1);
+ }
+
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id uploader = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase it on behalf of the uploader
+ testMetricMaker.reset();
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ }
+
private void allowPermissionToAllUsers(String permission) {
allowPermission(permission, REGISTERED_USERS);
}
diff --git a/modules/jgit b/modules/jgit
index 66b871b..596c445 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit 66b871b777c1a58337e80dd03db68bb76b145a93
+Subproject commit 596c445af22ed9b6e0b7e35de44c127fcb8ecf7d
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 7091310..4c7c857 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1545,7 +1545,6 @@
id="fileList"
.change=${this.change}
.changeNum=${this.changeNum}
- .patchRange=${this.patchRange}
.editMode=${this.getEditMode()}
@files-shown-changed=${(e: CustomEvent<{length: number}>) => {
this.shownFileCount = e.detail.length;
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 6eaf7ae..b9850dd 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -42,6 +42,7 @@
NumericChangeId,
PARENT,
PatchRange,
+ RevisionPatchSetNum,
} from '../../../types/common';
import {DiffPreferencesInfo} from '../../../types/diff';
import {GrDiffHost} from '../../diff/gr-diff-host/gr-diff-host';
@@ -181,11 +182,21 @@
@query('#diffPreferencesDialog')
diffPreferencesDialog?: GrDiffPreferencesDialog;
- @property({type: Object})
- patchRange?: PatchRange;
+ get patchRange(): PatchRange | undefined {
+ if (!this.patchNum) return undefined;
+ return {
+ patchNum: this.patchNum,
+ basePatchNum: this.basePatchNum,
+ };
+ }
- @property({type: String})
- patchNum?: string;
+ // Private but used in tests.
+ @state()
+ patchNum?: RevisionPatchSetNum;
+
+ // Private but used in tests.
+ @state()
+ basePatchNum: BasePatchSetNum = PARENT;
@property({type: Number})
changeNum?: NumericChangeId;
@@ -811,6 +822,16 @@
this.reviewed = reviewedFiles ?? [];
}
);
+ subscribe(
+ this,
+ () => this.getChangeModel().patchNum$,
+ x => (this.patchNum = x)
+ );
+ subscribe(
+ this,
+ () => this.getChangeModel().basePatchNum$,
+ x => (this.basePatchNum = x)
+ );
}
override willUpdate(changedProperties: PropertyValues): void {
@@ -1136,7 +1157,7 @@
const hasExtendedStatus = this.filesLeftBase.length > 0;
// no file means "header row"
if (!file) {
- const psNum = this.patchRange?.patchNum;
+ const psNum = this.patchNum;
return hasExtendedStatus
? this.renderDivWithTooltip(`${psNum}`, `Patchset ${psNum}`)
: nothing;
@@ -1154,8 +1175,8 @@
const status = fileIsReverted
? FileInfoStatus.REVERTED
: file?.status ?? FileInfoStatus.MODIFIED;
- const left = `patchset ${this.patchRange?.basePatchNum}`;
- const right = `patchset ${this.patchRange?.patchNum}`;
+ const left = `patchset ${this.basePatchNum}`;
+ const right = `patchset ${this.patchNum}`;
const postfix = ` between ${left} and ${right}`;
return html`<gr-file-status
@@ -1175,7 +1196,7 @@
></gr-icon>
`;
// no path means "header row"
- const psNum = this.patchRange?.basePatchNum;
+ const psNum = this.basePatchNum;
if (!path) {
return html`
${this.renderDivWithTooltip(`${psNum}`, `Patchset ${psNum}`)} ${arrow}
@@ -1187,7 +1208,7 @@
const status = file.status ?? FileInfoStatus.MODIFIED;
const left = 'base';
- const right = `patchset ${this.patchRange?.basePatchNum}`;
+ const right = `patchset ${this.basePatchNum}`;
const postfix = ` between ${left} and ${right}`;
return html`
@@ -1664,16 +1685,16 @@
if (
this.change &&
this.changeNum &&
- this.patchRange?.patchNum &&
- new RevisionInfo(this.change).isMergeCommit(this.patchRange.patchNum) &&
- this.patchRange.basePatchNum === PARENT &&
- this.patchRange.patchNum !== EDIT
+ this.patchNum &&
+ new RevisionInfo(this.change).isMergeCommit(this.patchNum) &&
+ this.basePatchNum === PARENT &&
+ this.patchNum !== EDIT
) {
const allFilesByPath = await this.restApiService.getChangeOrEditFiles(
this.changeNum,
{
basePatchNum: -1 as BasePatchSetNum, // -1 is first (target) parent
- patchNum: this.patchRange.patchNum,
+ patchNum: this.patchNum,
}
);
if (!allFilesByPath) return;
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
index c79be96..8fb5622 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -102,10 +102,8 @@
ignore_whitespace: 'IGNORE_NONE',
};
element.numFilesShown = 200;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
saveStub = sinon
.stub(element, '_saveReviewedState')
.callsFake(() => Promise.resolve());
@@ -365,7 +363,7 @@
test('renders file status column header', async () => {
element.files = createFiles(1, {lines_inserted: 9});
element.filesLeftBase = createFiles(1, {lines_inserted: 9});
- element.patchRange!.basePatchNum = 1 as PatchSetNumber;
+ element.basePatchNum = 1 as PatchSetNumber;
await element.updateComplete;
const fileRows = queryAll<HTMLDivElement>(element, '.header-row');
const statusCol = queryAndAssert(fileRows?.[0], '.status');
@@ -695,22 +693,9 @@
test('comment filtering', () => {
element.changeComments = createChangeComments();
- const parentTo1 = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
- const parentTo2 = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
-
- const _1To2 = {
- basePatchNum: 1 as BasePatchSetNum,
- patchNum: 2 as RevisionPatchSetNum,
- };
-
- element.patchRange = parentTo1;
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: '/COMMIT_MSG',
@@ -719,7 +704,9 @@
}),
'2c'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: '/COMMIT_MSG',
@@ -728,7 +715,9 @@
}),
'3c'
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: 'unresolved.file',
@@ -737,7 +726,9 @@
}),
'1 draft'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: 'unresolved.file',
@@ -746,7 +737,9 @@
}),
'1 draft'
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'unresolved.file',
@@ -755,7 +748,9 @@
}),
'1d'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'unresolved.file',
@@ -764,7 +759,9 @@
}),
'1d'
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'myfile.txt',
@@ -773,7 +770,9 @@
}),
'1c'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'myfile.txt',
@@ -782,16 +781,9 @@
}),
'3c'
);
- element.patchRange = parentTo1;
- assert.equal(
- element.computeDraftsString({
- __path: 'myfile.txt',
- size: 0,
- size_delta: 0,
- }),
- ''
- );
- element.patchRange = _1To2;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: 'myfile.txt',
@@ -801,7 +793,19 @@
''
);
- element.patchRange = parentTo1;
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
+ assert.equal(
+ element.computeDraftsString({
+ __path: 'myfile.txt',
+ size: 0,
+ size_delta: 0,
+ }),
+ ''
+ );
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'myfile.txt',
@@ -810,7 +814,9 @@
}),
''
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'myfile.txt',
@@ -819,7 +825,9 @@
}),
''
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'file_added_in_rev2.txt',
@@ -828,7 +836,9 @@
}),
''
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'file_added_in_rev2.txt',
@@ -837,7 +847,9 @@
}),
''
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: 'file_added_in_rev2.txt',
@@ -846,7 +858,9 @@
}),
''
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: 'file_added_in_rev2.txt',
@@ -855,7 +869,9 @@
}),
''
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'file_added_in_rev2.txt',
@@ -864,7 +880,9 @@
}),
''
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'file_added_in_rev2.txt',
@@ -873,7 +891,9 @@
}),
''
);
- element.patchRange = parentTo2;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: '/COMMIT_MSG',
@@ -882,7 +902,9 @@
}),
'1c'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: '/COMMIT_MSG',
@@ -891,7 +913,9 @@
}),
'3c'
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: '/COMMIT_MSG',
@@ -900,7 +924,9 @@
}),
'2 drafts'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsString({
__path: '/COMMIT_MSG',
@@ -909,7 +935,9 @@
}),
'2 drafts'
);
- element.patchRange = parentTo1;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: '/COMMIT_MSG',
@@ -918,7 +946,9 @@
}),
'2d'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: '/COMMIT_MSG',
@@ -927,7 +957,9 @@
}),
'2d'
);
- element.patchRange = parentTo2;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'myfile.txt',
@@ -936,7 +968,9 @@
}),
'2c'
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeCommentsStringMobile({
__path: 'myfile.txt',
@@ -945,7 +979,9 @@
}),
'3c'
);
- element.patchRange = parentTo2;
+
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'myfile.txt',
@@ -954,7 +990,9 @@
}),
''
);
- element.patchRange = _1To2;
+
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
assert.equal(
element.computeDraftsStringMobile({
__path: 'myfile.txt',
@@ -973,10 +1011,8 @@
normalize({}, 'myfile.txt'),
];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
element.change = {
_number: 42 as NumericChangeId,
project: 'test-project',
@@ -1197,10 +1233,8 @@
normalize({}, 'myfile.txt'),
];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
element.fileCursor.setCursorAtIndex(0);
const reviewSpy = sinon.spy(element, 'reviewFile');
@@ -1263,10 +1297,8 @@
normalize({}, 'f2.txt'),
];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
await element.updateComplete;
const clickSpy = sinon.spy(element, 'handleFileListClick');
@@ -1303,10 +1335,8 @@
normalize({}, 'f2.txt'),
];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
element.editMode = true;
await element.updateComplete;
@@ -1324,10 +1354,8 @@
test('checkbox shows/hides diff inline', async () => {
element.files = [normalize({}, 'myfile.txt')];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
element.fileCursor.setCursorAtIndex(0);
sinon.stub(element, 'expandedFilesChanged');
await element.updateComplete;
@@ -1353,10 +1381,8 @@
test('diff mode correctly toggles the diffs', async () => {
element.files = [normalize({}, 'myfile.txt')];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
const updateDiffPrefSpy = sinon.spy(element, 'updateDiffPreferences');
element.fileCursor.setCursorAtIndex(0);
await element.updateComplete;
@@ -1383,10 +1409,8 @@
test('tapping row ignores links', async () => {
element.files = [normalize({}, '/COMMIT_MSG')];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
sinon.stub(element, 'expandedFilesChanged');
await element.updateComplete;
const commitMsgFile = queryAll<HTMLAnchorElement>(
@@ -1680,10 +1704,8 @@
};
element.changeNum = changeWithMultipleParents._number;
element.change = changeWithMultipleParents;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
await element.updateComplete;
await waitEventLoop();
});
@@ -1744,10 +1766,8 @@
});
test('not shown for non-Auto Merge base parents', async () => {
- element.patchRange = {
- basePatchNum: 1 as BasePatchSetNum,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = 2 as RevisionPatchSetNum;
await element.updateCleanlyMergedPaths();
await element.updateComplete;
@@ -1756,10 +1776,8 @@
});
test('not shown in edit mode', async () => {
- element.patchRange = {
- basePatchNum: 1 as BasePatchSetNum,
- patchNum: EDIT,
- };
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.patchNum = EDIT;
await element.updateCleanlyMergedPaths();
await element.updateComplete;
@@ -1776,10 +1794,8 @@
_number: 1 as NumericChangeId,
project: 'gerrit' as RepoName,
};
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
const path = 'index.php';
element.editMode = false;
assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1/index.php');
@@ -1791,10 +1807,8 @@
_number: 1 as NumericChangeId,
project: 'gerrit' as RepoName,
};
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
element.editMode = false;
const path = '/COMMIT_MSG';
assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1//COMMIT_MSG');
@@ -1806,10 +1820,8 @@
_number: 1 as NumericChangeId,
project: 'gerrit' as RepoName,
};
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
element.editMode = true;
const path = 'index.php';
assert.equal(
@@ -1824,10 +1836,8 @@
_number: 1 as NumericChangeId,
project: 'gerrit' as RepoName,
};
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 1 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 1 as RevisionPatchSetNum;
element.editMode = true;
const path = '/COMMIT_MSG';
assert.equal(
@@ -2113,10 +2123,8 @@
];
element.reviewed = ['/COMMIT_MSG', 'myfile.txt'];
element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 2 as RevisionPatchSetNum,
- };
+ element.basePatchNum = PARENT;
+ element.patchNum = 2 as RevisionPatchSetNum;
sinon
.stub(window, 'fetch')
.callsFake(() => Promise.resolve(new Response()));
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index 1608c22..9d29cea 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -36,7 +36,6 @@
import {GrSyntaxLayerWorker} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
import {anyLineTooLong} from '../../../embed/diff/gr-diff/gr-diff-utils';
-import {changeModelToken} from '../../../models/change/change-model';
import {fireReload} from '../../../utils/event-util';
interface FilePreview {
@@ -92,9 +91,6 @@
diffPrefs?: DiffPreferencesInfo;
@state()
- isOwner = false;
-
- @state()
onCloseFixPreviewCallbacks: ((fixapplied: boolean) => void)[] = [];
private readonly restApiService = getAppContext().restApiService;
@@ -103,8 +99,6 @@
private readonly getNavigation = resolve(this, navigationToken);
- private readonly getChangeModel = resolve(this, changeModelToken);
-
private readonly syntaxLayer = new GrSyntaxLayerWorker(
resolve(this, highlightServiceToken),
() => getAppContext().reportingService
@@ -114,11 +108,6 @@
super();
subscribe(
this,
- () => this.getChangeModel().isOwner$,
- x => (this.isOwner = x)
- );
- subscribe(
- this,
() => this.getUserModel().preferences$,
preferences => {
const layers: DiffLayer[] = [this.syntaxLayer];
@@ -341,7 +330,6 @@
private computeTooltip() {
if (!this.change || !this.patchNum) return '';
- if (!this.isOwner) return 'Fix can only be applied by author';
const latestPatchNum =
this.change.revisions[this.change.current_revision]._number;
return latestPatchNum !== this.patchNum
@@ -351,7 +339,6 @@
private computeDisableApplyFixButton() {
if (!this.change || !this.patchNum) return true;
- if (!this.isOwner) return true;
const latestPatchNum =
this.change.revisions[this.change.current_revision]._number;
return this.patchNum !== latestPatchNum || this.isApplyFixLoading;
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
index 9284fb2..a686b20 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
@@ -71,7 +71,6 @@
element.changeNum = change._number;
element.patchNum = change.revisions[change.current_revision]._number;
element.change = change;
- element.isOwner = true;
element.diffPrefs = {
...createDefaultDiffPrefs(),
font_size: 12,
@@ -161,22 +160,8 @@
assert.equal(button.getAttribute('title'), '');
});
- test('apply fix button is disabled for non-author', async () => {
- element.isOwner = false;
- await element.updateComplete;
- await open(TWO_FIXES);
- assert.equal(element.currentFix!.fix_id, 'fix_1');
- assert.equal(element.currentPreviews.length, 2);
- const button = getConfirmButton();
- assert.isTrue(button.hasAttribute('disabled'));
- assert.equal(
- button.getAttribute('title'),
- 'Fix can only be applied by author'
- );
- });
-
test('apply fix button is disabled on older patchset', async () => {
- element.change = {
+ element.change = element.change = {
...createParsedChange(),
revisions: createRevisions(2),
current_revision: getCurrentRevision(0),
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 446822f..ad5b217 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -26,6 +26,7 @@
import {
computeAllPatchSets,
computeLatestPatchNum,
+ computeLatestPatchNumWithEdit,
} from '../../utils/patch-set-util';
import {ParsedChangeInfo} from '../../types/types';
import {fireAlert} from '../../utils/event-util';
@@ -191,6 +192,10 @@
computeLatestPatchNum(patchsets)
);
+ public readonly latestPatchNumWithEdit$ = select(this.patchsets$, patchsets =>
+ computeLatestPatchNumWithEdit(patchsets)
+ );
+
/**
* Emits the current patchset number. If the route does not define the current
* patchset num, then this selector waits for the change to be defined and
@@ -203,7 +208,7 @@
combineLatest([
this.viewModel.state$,
this.state$,
- this.latestPatchNum$,
+ this.latestPatchNumWithEdit$,
]).pipe(
/**
* If you depend on both, view model and change state, then you want to
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 183671f..7e16ad9 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -265,6 +265,16 @@
return latest;
}
+// Basically is computeLatestPatchNum but allows "edits".
+export function computeLatestPatchNumWithEdit(
+ allPatchSets?: PatchSet[]
+): RevisionPatchSetNum | undefined {
+ if (!allPatchSets || !allPatchSets.length) {
+ return undefined;
+ }
+ return allPatchSets[0].num;
+}
+
export function computePredecessor(
patchset?: PatchSetNum
): BasePatchSetNum | undefined {
