Merge "Show summary for flows similar to checks"
diff --git a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
index 2ef499f..2ed32e7 100644
--- a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
@@ -19,6 +19,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
@@ -27,12 +28,14 @@
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.accounts.AccountApi;
import com.google.gerrit.extensions.api.config.Server;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.httpd.raw.IndexPreloadingUtil.RequestedPage;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gson.Gson;
import com.google.template.soy.data.SanitizedContent;
import java.net.URI;
@@ -41,6 +44,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
@@ -68,10 +72,13 @@
String requestedURL)
throws URISyntaxException, RestApiException {
ImmutableMap.Builder<String, Object> data = ImmutableMap.builder();
+ boolean asyncSubmitRequirements =
+ experimentFeatures.isFeatureEnabled(ExperimentFeaturesConstants.ASYNC_SUBMIT_REQUIREMENTS);
data.putAll(
staticTemplateData(
canonicalURL, cdnPath, faviconPath, urlParameterMap, urlInScriptTagOrdainer))
- .putAll(dynamicTemplateData(gerritApi, requestedURL, canonicalURL));
+ .putAll(
+ dynamicTemplateData(gerritApi, requestedURL, canonicalURL, asyncSubmitRequirements));
Set<String> enabledExperiments = new HashSet<>();
enabledExperiments.addAll(experimentFeatures.getEnabledExperimentFeatures());
// Add all experiments enabled through url
@@ -107,7 +114,10 @@
/** Returns dynamic parameters of {@code index.html}. */
public static ImmutableMap<String, Object> dynamicTemplateData(
- GerritApi gerritApi, String requestedURL, String canonicalURL)
+ GerritApi gerritApi,
+ String requestedURL,
+ String canonicalURL,
+ boolean asyncSubmitRequirements)
throws RestApiException, URISyntaxException {
ImmutableMap.Builder<String, Object> data = ImmutableMap.builder();
Map<String, SanitizedContent> initialData = new HashMap<>();
@@ -127,15 +137,21 @@
Integer basePatchNum = computeBasePatchNum(requestedPath);
switch (page) {
case CHANGE, DIFF -> {
- if (basePatchNum.equals(0)) {
+ LinkedHashSet<ListChangesOption> changeDetailOptions =
+ new LinkedHashSet<>(
+ basePatchNum.equals(0)
+ ? IndexPreloadingUtil.CHANGE_DETAIL_OPTIONS_WITHOUT_PARENTS
+ : IndexPreloadingUtil.CHANGE_DETAIL_OPTIONS_WITH_PARENTS);
+ if (asyncSubmitRequirements) {
+ changeDetailOptions.remove(ListChangesOption.SUBMIT_REQUIREMENTS);
+ changeDetailOptions.remove(ListChangesOption.SUBMITTABLE);
data.put(
- "defaultChangeDetailHex",
- ListOption.toHex(IndexPreloadingUtil.CHANGE_DETAIL_OPTIONS_WITHOUT_PARENTS));
- } else {
- data.put(
- "defaultChangeDetailHex",
- ListOption.toHex(IndexPreloadingUtil.CHANGE_DETAIL_OPTIONS_WITH_PARENTS));
+ "submitRequirementsHex",
+ ListOption.toHex(
+ ImmutableSet.of(
+ ListChangesOption.SUBMIT_REQUIREMENTS, ListChangesOption.SUBMITTABLE)));
}
+ data.put("defaultChangeDetailHex", ListOption.toHex(changeDetailOptions));
data.put(
"changeRequestsPath",
IndexPreloadingUtil.computeChangeRequestsPath(requestedPath, page).get());
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index cd91745..ce96132 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -60,4 +60,7 @@
/** Whether we allow fix suggestions in HumanComments. */
public static final String ALLOW_FIX_SUGGESTIONS_IN_COMMENTS =
"GerritBackendFeature__allow_fix_suggestions_in_comments";
+
+ /** Whether UI should request Submit Requirements separately from change detail. */
+ public static final String ASYNC_SUBMIT_REQUIREMENTS = "UiFeature__async_submit_requirements";
}
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index ff729b7..291d5a9 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -321,7 +321,16 @@
}
if (footersToAdd.length() > 0) {
- message = message.trim() + "\n\n" + footersToAdd.toString().trim();
+ String trimmedMsg = message.trim();
+ List<String> lines = Splitter.on('\n').splitToList(trimmedMsg);
+ String lastLine = lines.isEmpty() ? "" : lines.get(lines.size() - 1);
+ boolean endsWithFooter = lastLine.matches("^[a-zA-Z0-9-]+:.*");
+
+ if (endsWithFooter) {
+ message = trimmedMsg + "\n" + footersToAdd.toString().trim();
+ } else {
+ message = trimmedMsg + "\n\n" + footersToAdd.toString().trim();
+ }
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index a8f3db4..b3b56db 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -1757,10 +1757,54 @@
// Check that the footers are not duplicated
String commitMessage = gApi.changes().id(revertChange.id).current().commit(false).message;
- int bugOccurrences = commitMessage.split("Bug: 12345", -1).length - 1;
- assertThat(bugOccurrences).isEqualTo(1);
- int issueOccurrences = commitMessage.split("Issue: 67890", -1).length - 1;
- assertThat(issueOccurrences).isEqualTo(1);
+ String expectedMessage =
+ String.format(
+ """
+ Reverting this change
+
+ Bug: 12345
+ Issue: 67890
+ Change-Id: %s
+ """,
+ revertChange.changeId);
+ assertThat(commitMessage).isEqualTo(expectedMessage);
+ }
+
+ @Test
+ public void revertChangeWithSkipPresubmitFooter() throws Exception {
+ // Create a change with bug and issue footers
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Change with bug and issue\n" + "\n" + "Bug: 12345\n" + "Issue: 67890",
+ "a.txt",
+ "content");
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+
+ // Revert the change
+ RevertInput revertInput = new RevertInput();
+ revertInput.message = "Reverting this change\n\nSkip-Presubmit: true";
+ ChangeInfo revertChange = gApi.changes().id(r.getChangeId()).revert(revertInput).get();
+
+ // Check that the revert commit message contains all footers and no extra newline
+ String commitMessage = gApi.changes().id(revertChange.id).current().commit(false).message;
+ String expectedMessage =
+ String.format(
+ """
+ Reverting this change
+
+ Skip-Presubmit: true
+ Bug: 12345
+ Issue: 67890
+ Change-Id: %s
+ """,
+ revertChange.changeId);
+ assertThat(commitMessage).isEqualTo(expectedMessage);
}
@Test
@@ -1780,8 +1824,6 @@
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
// Revert the submission with a message that already contains the footers
- RevertInput revertInput = new RevertInput();
- revertInput.message = "Reverting this change\n\nBug: 12345\nIssue: 67890";
List<ChangeInfo> revertChanges =
gApi.changes().id(r.getChangeId()).revertSubmission().revertChanges;
assertThat(revertChanges).hasSize(1);
@@ -1789,10 +1831,19 @@
// Check that the footers are not duplicated
String commitMessage = gApi.changes().id(revertChange.id).current().commit(false).message;
- int bugOccurrences = commitMessage.split("Bug: 12345", -1).length - 1;
- assertThat(bugOccurrences).isEqualTo(1);
- int issueOccurrences = commitMessage.split("Issue: 67890", -1).length - 1;
- assertThat(issueOccurrences).isEqualTo(1);
+ String expectedMessage =
+ String.format(
+ """
+ Revert "Change with bug and issue"
+
+ This reverts commit %s.
+
+ Bug: 12345
+ Issue: 67890
+ Change-Id: %s
+ """,
+ r.getCommit().name(), revertChange.changeId);
+ assertThat(commitMessage).isEqualTo(expectedMessage);
}
@Override
diff --git a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
index fe3cb00..8dcd0f3 100644
--- a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
@@ -52,7 +52,8 @@
.GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE,
- ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE)
+ ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE,
+ ExperimentFeaturesConstants.ASYNC_SUBMIT_REQUIREMENTS)
.inOrder();
// "GerritBackendFeature__check_implicit_merges_on_merge",
diff --git a/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java b/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
index 2f6d565..68be60b 100644
--- a/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
+++ b/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
@@ -117,7 +117,7 @@
String requestedPath = "/c/project/+/123/4..6";
assertThat(IndexHtmlUtil.computeBasePatchNum(requestedPath)).isEqualTo(4);
- assertThat(dynamicTemplateData(gerritApi, requestedPath, ""))
+ assertThat(dynamicTemplateData(gerritApi, requestedPath, "", false))
.containsAtLeast(
"defaultChangeDetailHex", "9996394",
"changeRequestsPath", "changes/project~123");
@@ -145,12 +145,41 @@
String requestedPath = "/c/project/+/123";
assertThat(IndexHtmlUtil.computeBasePatchNum(requestedPath)).isEqualTo(0);
- assertThat(dynamicTemplateData(gerritApi, requestedPath, ""))
+ assertThat(dynamicTemplateData(gerritApi, requestedPath, "", false))
.containsAtLeast(
"defaultChangeDetailHex", "1996394",
"changeRequestsPath", "changes/project~123");
}
+ @Test
+ public void usePreloadRestWithAsyncSRs() throws Exception {
+ Accounts accountsApi = mock(Accounts.class);
+ when(accountsApi.self()).thenThrow(new AuthException("user needs to be authenticated"));
+
+ Server serverApi = mock(Server.class);
+ when(serverApi.getVersion()).thenReturn("123");
+ when(serverApi.topMenus()).thenReturn(ImmutableList.of());
+ ServerInfo serverInfo = new ServerInfo();
+ serverInfo.defaultTheme = "my-default-theme";
+ when(serverApi.getInfo()).thenReturn(serverInfo);
+
+ Config configApi = mock(Config.class);
+ when(configApi.server()).thenReturn(serverApi);
+
+ GerritApi gerritApi = mock(GerritApi.class);
+ when(gerritApi.accounts()).thenReturn(accountsApi);
+ when(gerritApi.config()).thenReturn(configApi);
+
+ String requestedPath = "/c/project/+/123";
+ assertThat(IndexHtmlUtil.computeBasePatchNum(requestedPath)).isEqualTo(0);
+
+ assertThat(dynamicTemplateData(gerritApi, requestedPath, "", true))
+ .containsAtLeast(
+ "defaultChangeDetailHex", "896394",
+ "submitRequirementsHex", "1100000",
+ "changeRequestsPath", "changes/project~123");
+ }
+
private static SanitizedContent ordain(String s) {
return UnsafeSanitizedContentOrdainer.ordainAsSafe(
s, SanitizedContent.ContentKind.TRUSTED_RESOURCE_URI);
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_screenshot_test.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_screenshot_test.ts
index f45f5ae..87b50a2 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_screenshot_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_screenshot_test.ts
@@ -60,10 +60,11 @@
await element.updateComplete;
});
- test('normal view', async () => {
- await visualDiff(element, 'gr-change-metadata');
- await visualDiffDarkTheme(element, 'gr-change-metadata-dark');
- });
+ // TODO(b/447590232): Fix test flakiness and re-enable
+ // test('normal view', async () => {
+ // await visualDiff(element, 'gr-change-metadata');
+ // await visualDiffDarkTheme(element, 'gr-change-metadata-dark');
+ // });
test('show all sections with more data', async () => {
const changeModel = testResolver(changeModelToken);
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
index 8a58551..3963888 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
@@ -47,6 +47,7 @@
countRunningRunsForLabel,
} from '../../checks/gr-checks-util';
import {subscribe} from '../../lit/subscription-controller';
+import {when} from 'lit/directives/when.js';
/**
* @attr {Boolean} suppress-title - hide titles, currently for hovercard view
@@ -153,7 +154,6 @@
const submit_requirements = orderSubmitRequirements(
getRequirements(this.change)
);
- if (submit_requirements.length === 0) return nothing;
const requirementKey = (req: SubmitRequirementResultInfo, index: number) =>
`${index}-${req.name}`;
@@ -162,34 +162,44 @@
id="submit-requirements-caption"
>
Submit Requirements
+ ${submit_requirements.length === 0 ? '(Loading...)' : ''}
</h3>
- <table class="requirements" aria-labelledby="submit-requirements-caption">
- <thead hidden>
- <tr>
- <th>Status</th>
- <th>Name</th>
- <th>Votes</th>
- </tr>
- </thead>
- <tbody>
- ${repeat(submit_requirements, requirementKey, (requirement, index) =>
- this.renderRequirement(requirement, index)
- )}
- </tbody>
- </table>
- ${this.disableHovercards
- ? ''
- : submit_requirements.map(
- (requirement, index) => html`
- <gr-submit-requirement-hovercard
- for="requirement-${index}-${charsOnly(requirement.name)}"
- .requirement=${requirement}
- .change=${this.change}
- .account=${this.account}
- .mutable=${this.mutable ?? false}
- ></gr-submit-requirement-hovercard>
- `
- )}`;
+ ${when(
+ submit_requirements.length !== 0,
+ () => html`<table
+ class="requirements"
+ aria-labelledby="submit-requirements-caption"
+ >
+ <thead hidden>
+ <tr>
+ <th>Status</th>
+ <th>Name</th>
+ <th>Votes</th>
+ </tr>
+ </thead>
+ <tbody>
+ ${repeat(
+ submit_requirements,
+ requirementKey,
+ (requirement, index) =>
+ this.renderRequirement(requirement, index)
+ )}
+ </tbody>
+ </table>
+ ${this.disableHovercards
+ ? ''
+ : submit_requirements.map(
+ (requirement, index) => html`
+ <gr-submit-requirement-hovercard
+ for="requirement-${index}-${charsOnly(requirement.name)}"
+ .requirement=${requirement}
+ .change=${this.change}
+ .account=${this.account}
+ .mutable=${this.mutable ?? false}
+ ></gr-submit-requirement-hovercard>
+ `
+ )}`
+ )}`;
}
private renderRequirement(
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
index 6812251..aee2017 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
@@ -23,24 +23,33 @@
SubmitRequirementResultInfo,
SubmitRequirementStatus,
} from '../../../api/rest-api';
-import {ParsedChangeInfo} from '../../../types/types';
+import {LoadingStatus, ParsedChangeInfo} from '../../../types/types';
import {RunStatus} from '../../../api/checks';
+import {testResolver} from '../../../test/common-test-setup';
+import {
+ ChangeModel,
+ changeModelToken,
+} from '../../../models/change/change-model';
suite('gr-submit-requirements tests', () => {
let element: GrSubmitRequirements;
let change: ParsedChangeInfo;
+ let changeModel: ChangeModel;
+
setup(async () => {
const submitRequirement: SubmitRequirementResultInfo = {
...createSubmitRequirementResultInfo(),
description: 'Test Description',
submittability_expression_result: createSubmitRequirementExpressionInfo(),
};
+ const submitRequirements: SubmitRequirementResultInfo[] = [
+ submitRequirement,
+ createNonApplicableSubmitRequirementResultInfo(),
+ ];
change = {
...createParsedChange(),
- submit_requirements: [
- submitRequirement,
- createNonApplicableSubmitRequirementResultInfo(),
- ],
+ submittable: false,
+ submit_requirements: submitRequirements,
labels: {
Verified: {
...createDetailedLabelInfo(),
@@ -54,12 +63,23 @@
},
};
const account = createAccountWithIdNameAndEmail();
+ changeModel = testResolver(changeModelToken);
+ changeModel.setState({
+ change,
+ submittabilityInfo: {
+ changeNum: change._number,
+ submitRequirements,
+ submittable: false,
+ },
+ loadingStatus: LoadingStatus.LOADED,
+ });
element = await fixture<GrSubmitRequirements>(
html`<gr-submit-requirements
.change=${change}
.account=${account}
></gr-submit-requirements>`
);
+ await element.updateComplete;
});
test('renders', () => {
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index df09209..5577075 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -43,7 +43,10 @@
ParsedChangeInfo,
} from '../../types/types';
import {fire, fireAlert, fireTitleChange} from '../../utils/event-util';
-import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
+import {
+ RestApiService,
+ SubmittabilityInfo,
+} from '../../services/gr-rest-api/gr-rest-api';
import {select} from '../../utils/observable-util';
import {assertIsDefined} from '../../utils/common-util';
import {Model} from '../base/model';
@@ -86,6 +89,12 @@
loadingStatus: LoadingStatus;
change?: ParsedChangeInfo;
/**
+ * Information about submittablity and evaluation of SRs
+ *
+ * Corresponding values in `change` are always kept in sync.
+ */
+ submittabilityInfo?: SubmittabilityInfo;
+ /**
* The list of reviewed files, kept in the model because we want changes made
* in one view to reflect on other views without re-rendering the other views.
* Undefined means it's still loading and empty set means no files reviewed.
@@ -254,6 +263,32 @@
}
/**
+ * Returns new change object with the fields with submittability related fields
+ * updated.
+ *
+ * - if change is undefined return undefined.
+ * - if change number is different than the one in submittability info, no
+ * updates made
+ */
+export function fillFromSubmittabilityInfo(
+ change?: ParsedChangeInfo,
+ submittabilityInfo?: SubmittabilityInfo
+): ParsedChangeInfo | undefined {
+ if (
+ !change ||
+ !submittabilityInfo ||
+ submittabilityInfo.changeNum !== change._number
+ ) {
+ return change;
+ }
+ return {
+ ...change,
+ submittable: submittabilityInfo.submittable,
+ submit_requirements: submittabilityInfo.submitRequirements,
+ };
+}
+
+/**
* Derives the base patchset number from all the data that can potentially
* influence it. Mostly just returns `viewModelBasePatchNum` or PARENT, but has
* some special logic when looking at merge commits.
@@ -308,6 +343,8 @@
export class ChangeModel extends Model<ChangeState> {
private change?: ParsedChangeInfo;
+ private submittabilityInfo?: SubmittabilityInfo;
+
private patchNum?: RevisionPatchSetNum;
private basePatchNum?: BasePatchSetNum;
@@ -319,6 +356,21 @@
changeState => changeState.change
);
+ public readonly submittabilityInfo$ = select(
+ this.state$,
+ changeState => changeState.submittabilityInfo
+ );
+
+ public readonly submittable$ = select(
+ this.state$,
+ changeState => changeState.submittabilityInfo?.submittable
+ );
+
+ public readonly submitRequirements$ = select(
+ this.state$,
+ changeState => changeState.submittabilityInfo?.submitRequirements
+ );
+
public readonly changeLoadingStatus$ = select(
this.state$,
changeState => changeState.loadingStatus
@@ -551,6 +603,7 @@
);
this.subscriptions = [
this.loadChange(),
+ this.loadSubmittabilityInfo(),
this.loadMergeable(),
this.loadReviewedFiles(),
this.setOverviewTitle(),
@@ -561,6 +614,9 @@
this.refuseEditForOpenChange(),
this.refuseEditForClosedChange(),
this.change$.subscribe(change => (this.change = change)),
+ this.submittabilityInfo$.subscribe(
+ submittabilityInfo => (this.submittabilityInfo = submittabilityInfo)
+ ),
this.patchNum$.subscribe(patchNum => (this.patchNum = patchNum)),
this.basePatchNum$.subscribe(
basePatchNum => (this.basePatchNum = basePatchNum)
@@ -742,6 +798,36 @@
.subscribe(mergeable => this.updateState({mergeable}));
}
+ private loadSubmittabilityInfo() {
+ // Use the same trigger as loadChange, to run SR loading in parallel.
+ return this.viewModel.changeNum$
+ .pipe(
+ switchMap(changeNum => {
+ if (!changeNum) {
+ // On reload changeNum is set to undefined to reset change state.
+ // We propagate undefined and reset the state in this case.
+ return of(undefined);
+ }
+ return from(this.restApiService.getSubmittabilityInfo(changeNum));
+ })
+ )
+ .subscribe(submittabilityInfo => {
+ // TODO(b/445644919): Remove once the submit_requirements is never
+ // requested as part of the change detail.
+ if (this.change?.submit_requirements) {
+ return;
+ }
+ const change = fillFromSubmittabilityInfo(
+ this.change,
+ submittabilityInfo
+ );
+ this.updateState({
+ change,
+ submittabilityInfo,
+ });
+ });
+ }
+
private loadChange() {
return this.viewModel.changeNum$
.pipe(
@@ -1006,11 +1092,32 @@
if (this.change && change?._number !== this.change?._number) {
return;
}
+ if (!change) {
+ this.updateState({
+ change: undefined,
+ loadingStatus: LoadingStatus.NOT_LOADED,
+ });
+ return;
+ }
change = updateRevisionsWithCommitShas(change);
+ // TODO(b/445644919): Remove once the submit_requirements is never requested
+ // as part of the change detail.
+ if (change!.submit_requirements) {
+ this.updateState({
+ change,
+ submittabilityInfo: {
+ changeNum: change!._number,
+ submittable: change!.submittable!,
+ submitRequirements: change!.submit_requirements,
+ },
+ loadingStatus: LoadingStatus.LOADED,
+ });
+ return;
+ }
+ change = fillFromSubmittabilityInfo(change, this.submittabilityInfo);
this.updateState({
change,
- loadingStatus:
- change === undefined ? LoadingStatus.NOT_LOADED : LoadingStatus.LOADED,
+ loadingStatus: LoadingStatus.LOADED,
});
}
}
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index 2fc8f8a..a58a070 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -16,6 +16,7 @@
createMergeable,
createParsedChange,
createRevision,
+ createSubmitRequirementResultInfo,
TEST_NUMERIC_CHANGE_ID,
} from '../../test/test-data-generators';
import {
@@ -60,6 +61,7 @@
import {SinonStub} from 'sinon';
import {pluginLoaderToken} from '../../elements/shared/gr-js-api-interface/gr-plugin-loader';
import {ShowChangeDetail} from '../../elements/shared/gr-js-api-interface/gr-js-api-types';
+import {SubmittabilityInfo} from '../../services/gr-rest-api/gr-rest-api';
suite('updateRevisionsWithCommitShas() tests', () => {
test('undefined edit', async () => {
@@ -487,6 +489,97 @@
assert.deepEqual(state?.change, updateRevisionsWithCommitShas(knownChange));
});
+ test('load submit requirements (SRs load first)', async () => {
+ const promiseDetail = mockPromise<ParsedChangeInfo | undefined>();
+ const stubDetail = stubRestApi('getChangeDetail').callsFake(
+ () => promiseDetail
+ );
+ const promiseSrs = mockPromise<SubmittabilityInfo | undefined>();
+ const stubSrs = stubRestApi('getSubmittabilityInfo').callsFake(
+ () => promiseSrs
+ );
+ testResolver(changeViewModelToken).setState(createChangeViewState());
+ promiseSrs.resolve({
+ changeNum: knownChange._number,
+ submittable: false,
+ submitRequirements: [createSubmitRequirementResultInfo()],
+ });
+ await waitUntilObserved(
+ changeModel.state$,
+ state => state.submittabilityInfo !== undefined,
+ 'SubmitRequirements was never loaded'
+ );
+ promiseDetail.resolve(knownChange);
+ const state = await waitForLoadingStatus(LoadingStatus.LOADED);
+ assert.isTrue(state.submittabilityInfo?.submittable === false);
+ assert.isTrue(state.submittabilityInfo?.submitRequirements.length === 1);
+ assert.isTrue(state.change?.submittable === false);
+ assert.isTrue(state.change?.submit_requirements?.length === 1);
+ assert.equal(stubDetail.callCount, 1);
+ assert.equal(stubSrs.callCount, 1);
+ });
+
+ test('load submit requirements (Detail load first, experiment enabled)', async () => {
+ const promiseDetail = mockPromise<ParsedChangeInfo | undefined>();
+ const stubDetail = stubRestApi('getChangeDetail').callsFake(
+ () => promiseDetail
+ );
+ const promiseSrs = mockPromise<SubmittabilityInfo | undefined>();
+ const stubSrs = stubRestApi('getSubmittabilityInfo').callsFake(
+ () => promiseSrs
+ );
+ let state: ChangeState;
+ testResolver(changeViewModelToken).setState(createChangeViewState());
+ promiseDetail.resolve(knownChange);
+ state = await waitForLoadingStatus(LoadingStatus.LOADED);
+ promiseSrs.resolve({
+ changeNum: knownChange._number,
+ submittable: false,
+ submitRequirements: [createSubmitRequirementResultInfo()],
+ });
+ state = await waitUntilObserved(
+ changeModel.state$,
+ state => state.submittabilityInfo !== undefined,
+ 'SubmitRequirements was never loaded'
+ );
+ assert.isTrue(state.submittabilityInfo?.submittable === false);
+ assert.isTrue(state.submittabilityInfo?.submitRequirements.length === 1);
+ assert.isTrue(state.change?.submittable === false);
+ assert.isTrue(state.change?.submit_requirements?.length === 1);
+ assert.equal(stubDetail.callCount, 1);
+ assert.equal(stubSrs.callCount, 1);
+ });
+
+ test('load submit requirements (Detail load first, experiment disabled)', async () => {
+ const promiseDetail = mockPromise<ParsedChangeInfo | undefined>();
+ const stubDetail = stubRestApi('getChangeDetail').callsFake(
+ () => promiseDetail
+ );
+ const promiseSrs = mockPromise<SubmittabilityInfo | undefined>();
+ const stubSrs = stubRestApi('getSubmittabilityInfo').callsFake(
+ () => promiseSrs
+ );
+ let state: ChangeState;
+ testResolver(changeViewModelToken).setState(createChangeViewState());
+ promiseDetail.resolve({
+ ...knownChange,
+ submittable: false,
+ submit_requirements: [createSubmitRequirementResultInfo()],
+ });
+ state = await waitForLoadingStatus(LoadingStatus.LOADED);
+ promiseSrs.resolve(undefined);
+ state = await waitUntilObserved(
+ changeModel.state$,
+ state => state.submittabilityInfo !== undefined,
+ 'SubmitRequirements was never loaded'
+ );
+ // Validate that submit requirements didn't get reset to undefined.
+ assert.isTrue(state.submittabilityInfo?.submittable === false);
+ assert.isTrue(state.submittabilityInfo?.submitRequirements.length === 1);
+ assert.equal(stubDetail.callCount, 1);
+ assert.equal(stubSrs.callCount, 1);
+ });
+
test('navigating to another change', async () => {
// setting up a loaded change
let promise = mockPromise<ParsedChangeInfo | undefined>();
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 33fa6f2..2c7993c 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -24,4 +24,5 @@
ML_SUGGESTED_EDIT_FEEDBACK = 'UiFeature__ml_suggested_edit_feedback',
ML_SUGGESTED_EDIT_EDITABLE_SUGGESTION = 'UiFeature__ml_suggested_edit_editable_suggestion',
SHOW_FLOWS_TAB = 'UiFeature__show_flows_tab',
+ ASYNC_SUBMIT_REQUIREMENTS = 'UiFeature__async_submit_requirements',
}
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index deae598..ada11f9 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -110,7 +110,11 @@
DiffPreferencesInfo,
IgnoreWhitespaceType,
} from '../../types/diff';
-import {GetDiffCommentsOutput, RestApiService} from './gr-rest-api';
+import {
+ GetDiffCommentsOutput,
+ RestApiService,
+ SubmittabilityInfo,
+} from './gr-rest-api';
import {
CommentSide,
createDefaultDiffPrefs,
@@ -152,6 +156,7 @@
SiteBasedCache,
throwingErrorCallback,
} from '../../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {getAppContext} from '../app-context';
const MAX_PROJECT_RESULTS = 25;
@@ -265,6 +270,8 @@
// Used to serialize requests for certain RPCs
readonly _serialScheduler: Scheduler<Response>;
+ private readonly flags = getAppContext().flagsService;
+
constructor(
private readonly authService: AuthService,
private readonly flagService: FlagsService
@@ -1419,15 +1426,17 @@
ListChangesOption.DOWNLOAD_COMMANDS,
ListChangesOption.MESSAGES,
ListChangesOption.REVIEWER_UPDATES,
- ListChangesOption.SUBMITTABLE,
ListChangesOption.WEB_LINKS,
ListChangesOption.SKIP_DIFFSTAT,
- ListChangesOption.SUBMIT_REQUIREMENTS,
ListChangesOption.PARENTS,
];
if (config?.receive?.enable_signed_push) {
options.push(ListChangesOption.PUSH_CERTIFICATES);
}
+ if (!this.flags.isEnabled(KnownExperimentId.ASYNC_SUBMIT_REQUIREMENTS)) {
+ options.push(ListChangesOption.SUBMITTABLE);
+ options.push(ListChangesOption.SUBMIT_REQUIREMENTS);
+ }
return options;
}
@@ -1486,6 +1495,35 @@
);
}
+ async getSubmittabilityInfo(
+ changeNum: NumericChangeId
+ ): Promise<SubmittabilityInfo | undefined> {
+ if (!this.flags.isEnabled(KnownExperimentId.ASYNC_SUBMIT_REQUIREMENTS)) {
+ return undefined;
+ }
+ const optionsHex = listChangesOptionsToHex(
+ ListChangesOption.SUBMITTABLE,
+ ListChangesOption.SUBMIT_REQUIREMENTS
+ );
+ const change = await this.getChange(
+ changeNum,
+ /* errFn=*/ undefined,
+ optionsHex
+ );
+ if (
+ !change ||
+ change.submittable === undefined ||
+ change.submit_requirements === undefined
+ ) {
+ return undefined;
+ }
+ return {
+ changeNum,
+ submittable: change.submittable,
+ submitRequirements: change.submit_requirements,
+ };
+ }
+
async getChangeCommitInfo(
changeNum: NumericChangeId,
patchNum: PatchSetNum
@@ -3503,15 +3541,10 @@
)
) as Promise<ChangeInfo | undefined>;
} else {
- const params: FetchParams = {q: `change:${changeNum}`};
- if (optionsHex) {
- params['O'] = optionsHex;
- }
return this._restApiHelper
.fetchJSON(
{
url: `/changes/?q=change:${changeNum}`,
- params,
errFn,
anonymizedUrl: '/changes/?q=change:*',
},
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index f874886..3a37599 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -108,6 +108,7 @@
LabelDefinitionInfo,
LabelDefinitionInput,
SubmitRequirementInput,
+ SubmitRequirementResultInfo,
} from '../../api/rest-api';
export interface GetDiffCommentsOutput {
@@ -115,6 +116,12 @@
comments: CommentInfo[];
}
+export interface SubmittabilityInfo {
+ changeNum: NumericChangeId;
+ submittable: boolean;
+ submitRequirements: SubmitRequirementResultInfo[];
+}
+
export interface RestApiService extends Finalizable {
getConfig(
noCache?: boolean,
@@ -207,6 +214,13 @@
): Promise<ParsedChangeInfo | undefined>;
/**
+ * Returns information about submittability and Submit Requirements.
+ */
+ getSubmittabilityInfo(
+ changeNum: NumericChangeId
+ ): Promise<SubmittabilityInfo | undefined>;
+
+ /**
* For every revision of the change returns the list of FileInfo for files
* which are modified compared to revision's parent.
*/
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index 1aead7d..818dbef 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -3,7 +3,10 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
+import {
+ RestApiService,
+ SubmittabilityInfo,
+} from '../../services/gr-rest-api/gr-rest-api';
import {FlowInfo} from '../../api/rest-api';
import {
AccountCapabilityInfo,
@@ -248,6 +251,9 @@
if (changeNum === undefined) return Promise.resolve(undefined);
return Promise.resolve(createChange() as ParsedChangeInfo);
},
+ getSubmittabilityInfo(): Promise<SubmittabilityInfo | undefined> {
+ return Promise.resolve(undefined);
+ },
getChangeEdit(): Promise<EditInfo | undefined> {
return Promise.resolve(undefined);
},
diff --git a/polygerrit-ui/app/utils/label-util.ts b/polygerrit-ui/app/utils/label-util.ts
index db02f29..ee7aa3a 100644
--- a/polygerrit-ui/app/utils/label-util.ts
+++ b/polygerrit-ui/app/utils/label-util.ts
@@ -351,7 +351,16 @@
* Show only applicable.
*/
export function getRequirements(change?: ParsedChangeInfo | ChangeInfo) {
- return (change?.submit_requirements ?? []).filter(
+ return getApplicableRequirements(change?.submit_requirements);
+}
+
+/**
+ * Get applicable requirements.
+ */
+export function getApplicableRequirements(
+ requirements?: SubmitRequirementResultInfo[]
+) {
+ return (requirements ?? []).filter(
req => req.status !== SubmitRequirementStatus.NOT_APPLICABLE
);
}
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-dark-dark.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-dark-dark.png
index 10423eb..b94a510 100644
--- a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-dark-dark.png
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-dark-dark.png
Binary files differ
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all-dark-dark.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all-dark-dark.png
index 6da3f5a..d09e5cd 100644
--- a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all-dark-dark.png
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all-dark-dark.png
Binary files differ
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all.png
index 173cdb4..fb4c9cf 100644
--- a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all.png
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata-show-all.png
Binary files differ
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata.png
index c2eaf99..7bea9b3 100644
--- a/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata.png
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-change-metadata.png
Binary files differ
diff --git a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
index bc6f619..eb84a2c 100644
--- a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
+++ b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
@@ -33,6 +33,7 @@
{@param? defaultDashboardHex: ?}
{@param? dashboardQuery: ?}
{@param? userIsAuthenticated: ?}
+ {@param? submitRequirementsHex: ?}
<!DOCTYPE html>{\n}
<html lang="en">{\n}
<meta charset="utf-8">{\n}
@@ -95,6 +96,9 @@
{if $userIsAuthenticated}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/edit/?download-commands=true" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
{/if}
+ {if $submitRequirementsHex}
+ <link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}?O={$submitRequirementsHex}" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
+ {/if}
{/if}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/comments?enable-context=true&context-padding=3" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
<link rel="preload" href="{$canonicalPath}/changes/?q=change:{$changeNum}" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml
index 6815993..73305c1 100644
--- a/tools/maven/gerrit-acceptance-framework_pom.xml
+++ b/tools/maven/gerrit-acceptance-framework_pom.xml
@@ -2,7 +2,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>com.google.gerrit</groupId>
<artifactId>gerrit-acceptance-framework</artifactId>
- <version>3.12.3-SNAPSHOT</version>
+ <version>3.13.0-SNAPSHOT</version>
<packaging>jar</packaging>
<name>Gerrit Code Review - Acceptance Test Framework</name>
<description>Framework for Gerrit's acceptance tests</description>
diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml
index 9a9ed45..8a12804 100644
--- a/tools/maven/gerrit-extension-api_pom.xml
+++ b/tools/maven/gerrit-extension-api_pom.xml
@@ -2,7 +2,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>com.google.gerrit</groupId>
<artifactId>gerrit-extension-api</artifactId>
- <version>3.12.3-SNAPSHOT</version>
+ <version>3.13.0-SNAPSHOT</version>
<packaging>jar</packaging>
<name>Gerrit Code Review - Extension API</name>
<description>API for Gerrit Extensions</description>
diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml
index 40349e3..0d254b3 100644
--- a/tools/maven/gerrit-plugin-api_pom.xml
+++ b/tools/maven/gerrit-plugin-api_pom.xml
@@ -2,7 +2,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>com.google.gerrit</groupId>
<artifactId>gerrit-plugin-api</artifactId>
- <version>3.12.3-SNAPSHOT</version>
+ <version>3.13.0-SNAPSHOT</version>
<packaging>jar</packaging>
<name>Gerrit Code Review - Plugin API</name>
<description>API for Gerrit Plugins</description>
diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml
index ddc370b..045a594 100644
--- a/tools/maven/gerrit-war_pom.xml
+++ b/tools/maven/gerrit-war_pom.xml
@@ -2,7 +2,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>com.google.gerrit</groupId>
<artifactId>gerrit-war</artifactId>
- <version>3.12.3-SNAPSHOT</version>
+ <version>3.13.0-SNAPSHOT</version>
<packaging>war</packaging>
<name>Gerrit Code Review - WAR</name>
<description>Gerrit WAR</description>
diff --git a/version.bzl b/version.bzl
index 02ae444..65c00f3 100644
--- a/version.bzl
+++ b/version.bzl
@@ -2,4 +2,4 @@
# Used by :api_install and :api_deploy targets
# when talking to the destination repository.
#
-GERRIT_VERSION = "3.12.3-SNAPSHOT"
+GERRIT_VERSION = "3.13.0-SNAPSHOT"
