Merge "Render all comments initially if user wants to scroll to some"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 240b0ed..870e194 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1857,7 +1857,8 @@
* The given change.
* If link:config-gerrit.html#change.submitWholeTopic[`change.submitWholeTopic`]
- is enabled, include all open changes with the same topic.
+ is enabled OR if the `o=TOPIC_CLOSURE` query parameter is passed, include all
+ open changes with the same topic.
* For each change whose submit type is not CHERRY_PICK, include unmerged
ancestors targeting the same branch.
@@ -1884,7 +1885,7 @@
Standard link:#query-options[formatting options] can be specified
with the `o` parameter, as well as the `submitted_together` specific
-option `NON_VISIBLE_CHANGES`.
+options `NON_VISIBLE_CHANGES` and `TOPIC_CLOSURE`.
.Response
----
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 9ca881d..2fb2127 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -26,6 +26,7 @@
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
+import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.TOPIC_CLOSURE;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -41,6 +42,7 @@
import com.google.common.base.Ticker;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.jimfs.Jimfs;
@@ -80,6 +82,7 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo;
+import com.google.gerrit.extensions.api.changes.SubmittedTogetherOption;
import com.google.gerrit.extensions.api.projects.BranchApi;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.api.projects.BranchInput;
@@ -1203,9 +1206,37 @@
}
protected void assertSubmittedTogether(String chId, String... expected) throws Exception {
- List<ChangeInfo> actual = gApi.changes().id(chId).submittedTogether();
+ assertSubmittedTogether(chId, ImmutableSet.of(), expected);
+ }
+
+ protected void assertSubmittedTogetherWithTopicClosure(String chId, String... expected)
+ throws Exception {
+ assertSubmittedTogether(chId, ImmutableSet.of(TOPIC_CLOSURE), expected);
+ }
+
+ protected void assertSubmittedTogether(
+ String chId,
+ ImmutableSet<SubmittedTogetherOption> submittedTogetherOptions,
+ String... expected)
+ throws Exception {
+ // This does not include NON_VISIBILE_CHANGES
+ List<ChangeInfo> actual =
+ submittedTogetherOptions.isEmpty()
+ ? gApi.changes().id(chId).submittedTogether()
+ : gApi.changes()
+ .id(chId)
+ .submittedTogether(EnumSet.copyOf(submittedTogetherOptions))
+ .changes;
+
+ EnumSet enumSetIncludingNonVisibleChanges =
+ submittedTogetherOptions.isEmpty()
+ ? EnumSet.of(NON_VISIBLE_CHANGES)
+ : EnumSet.copyOf(submittedTogetherOptions);
+ enumSetIncludingNonVisibleChanges.add(NON_VISIBLE_CHANGES);
+
+ // This includes NON_VISIBLE_CHANGES for comparison.
SubmittedTogetherInfo info =
- gApi.changes().id(chId).submittedTogether(EnumSet.of(NON_VISIBLE_CHANGES));
+ gApi.changes().id(chId).submittedTogether(enumSetIncludingNonVisibleChanges);
assertThat(info.nonVisibleChanges).isEqualTo(0);
assertThat(Iterables.transform(actual, i1 -> i1.changeId))
diff --git a/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java b/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java
index e2cab4d..68a4e88 100644
--- a/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java
+++ b/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java
@@ -16,5 +16,6 @@
/** Output options available for submitted_together requests. */
public enum SubmittedTogetherOption {
- NON_VISIBLE_CHANGES;
+ NON_VISIBLE_CHANGES,
+ TOPIC_CLOSURE;
}
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index 5155a0d..154e45a 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -304,7 +304,10 @@
return null; // submit not visible
}
- ChangeSet cs = mergeSuperSet.get().completeChangeSet(cd.change(), resource.getUser());
+ ChangeSet cs =
+ mergeSuperSet
+ .get()
+ .completeChangeSet(cd.change(), resource.getUser(), /*includingTopicClosure= */ false);
String topic = change.getTopic();
int topicSize = 0;
if (!Strings.isNullOrEmpty(topic)) {
diff --git a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java
index 214a001..c18e7c2 100644
--- a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java
+++ b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
+import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.TOPIC_CLOSURE;
import static java.util.Collections.reverseOrder;
import static java.util.stream.Collectors.toList;
@@ -127,7 +128,10 @@
int hidden;
if (c.isNew()) {
- ChangeSet cs = mergeSuperSet.get().completeChangeSet(c, resource.getUser());
+ ChangeSet cs =
+ mergeSuperSet
+ .get()
+ .completeChangeSet(c, resource.getUser(), options.contains(TOPIC_CLOSURE));
cds = ensureRequiredDataIsLoaded(cs.changes().asList());
hidden = cs.nonVisibleChanges().size();
} else if (c.isMerged()) {
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 64b60bb..b431299 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -480,7 +480,9 @@
logger.atFine().log("Beginning integration of %s", change);
try {
ChangeSet indexBackedChangeSet =
- mergeSuperSet.setMergeOpRepoManager(orm).completeChangeSet(change, caller);
+ mergeSuperSet
+ .setMergeOpRepoManager(orm)
+ .completeChangeSet(change, caller, /* includingTopicClosure= */ false);
if (!indexBackedChangeSet.ids().contains(change.getId())) {
// indexBackedChangeSet contains only open changes, if the change is missing in this set
// it might be that the change was concurrently submitted in the meantime.
diff --git a/java/com/google/gerrit/server/submit/MergeSuperSet.java b/java/com/google/gerrit/server/submit/MergeSuperSet.java
index 67f2907..8581e20 100644
--- a/java/com/google/gerrit/server/submit/MergeSuperSet.java
+++ b/java/com/google/gerrit/server/submit/MergeSuperSet.java
@@ -92,7 +92,19 @@
return this;
}
- public ChangeSet completeChangeSet(Change change, CurrentUser user)
+ /**
+ * Gets the ChangeSet of this {@code change} based on visiblity of the {@code user}. if
+ * change.submitWholeTopic is true, we return the topic closure as well as the dependent changes
+ * of the topic closure. Otherwise, we return just the dependent changes.
+ *
+ * @param change the change for which we get the dependent changes / topic closure.
+ * @param user the current user for visibility purposes.
+ * @param includingTopicClosure when true, return as if change.submitWholeTopic = true, so we
+ * return the topic closure.
+ * @return {@link ChangeSet} object that represents the dependent changes and/or topic closure of
+ * the requested change.
+ */
+ public ChangeSet completeChangeSet(Change change, CurrentUser user, boolean includingTopicClosure)
throws IOException, PermissionBackendException {
try {
if (orm == null) {
@@ -113,7 +125,7 @@
}
ChangeSet changeSet = new ChangeSet(cd, visible);
- if (wholeTopicEnabled(cfg)) {
+ if (wholeTopicEnabled(cfg) || includingTopicClosure) {
return completeChangeSetIncludingTopics(changeSet, user);
}
try (TraceContext traceContext = PluginContext.newTrace(mergeSuperSetComputation)) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
index a63d60a..0a9a098 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
@@ -306,7 +306,10 @@
private void assertChangeSetMergeable(ChangeData change, boolean expected)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
PermissionBackendException {
- ChangeSet cs = mergeSuperSet.get().completeChangeSet(change.change(), user(admin));
+ ChangeSet cs =
+ mergeSuperSet
+ .get()
+ .completeChangeSet(change.change(), user(admin), /* includingTopicClosure= */ false);
assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected);
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
index a97fb49..7e0bce9 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
@@ -130,6 +130,8 @@
} else {
assertSubmittedTogether(id1);
assertSubmittedTogether(id2);
+ assertSubmittedTogetherWithTopicClosure(id1, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id2, id2, id1);
}
}
@@ -152,6 +154,8 @@
} else {
assertSubmittedTogether(id1);
assertSubmittedTogether(id2);
+ assertSubmittedTogetherWithTopicClosure(id1, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id2, id2, id1);
}
}
@@ -180,6 +184,9 @@
assertSubmittedTogether(id1);
assertSubmittedTogether(id2);
assertSubmittedTogether(id3, id3, id2);
+ assertSubmittedTogetherWithTopicClosure(id1, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id2, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id3, id3, id2, id1);
}
}
@@ -227,6 +234,13 @@
assertSubmittedTogether(id4, id4, id3, id2);
assertSubmittedTogether(id5);
assertSubmittedTogether(id6, id6, id5);
+
+ assertSubmittedTogetherWithTopicClosure(id1, id6, id5, id3, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id2, id6, id5, id2);
+ assertSubmittedTogetherWithTopicClosure(id3, id6, id5, id3, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id4, id6, id5, id4, id3, id2, id1);
+ assertSubmittedTogetherWithTopicClosure(id5);
+ assertSubmittedTogetherWithTopicClosure(id6, id6, id5, id2);
}
}
diff --git a/plugins/replication b/plugins/replication
index b6f4011..d9c59a0 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit b6f4011070a8f0bf31fb1158ed12be71374eb47a
+Subproject commit d9c59a0be1c423706b2c4c74c955ebbeca5a894d
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts
index 172f807..f811ee2 100644
--- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts
+++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts
@@ -71,7 +71,7 @@
];
interface Rule {
- value: RuleValue;
+ value?: RuleValue;
}
interface RuleValue {
@@ -158,17 +158,17 @@
// Observer _handleValueChange is called after the ready()
// method finishes. Original values must be set later to
// avoid set .modified flag to true
- this._setOriginalRuleValues(this.rule.value);
+ this._setOriginalRuleValues(this.rule?.value);
}
}
- _setupValues(rule: Rule) {
- if (!rule.value) {
+ _setupValues(rule?: Rule) {
+ if (!rule?.value) {
this._setDefaultRuleValues();
}
}
- _computeForce(permission: AccessPermissionId, action: string) {
+ _computeForce(permission: AccessPermissionId, action?: string) {
if (AccessPermissionId.PUSH === permission && action !== Action.DENY) {
return true;
}
@@ -176,7 +176,7 @@
return AccessPermissionId.EDIT_TOPIC_NAME === permission;
}
- _computeForceClass(permission: AccessPermissionId, action: string) {
+ _computeForceClass(permission: AccessPermissionId, action?: string) {
return this._computeForce(permission, action) ? 'force' : '';
}
@@ -213,7 +213,7 @@
return classList.join(' ');
}
- _computeForceOptions(permission: string, action: string) {
+ _computeForceOptions(permission: string, action?: string) {
if (permission === AccessPermissionId.PUSH) {
if (action === Action.ALLOW) {
return ForcePushOptions.ALLOW;
@@ -259,7 +259,7 @@
}
_handleRemoveRule() {
- if (!this.rule) return;
+ if (!this.rule?.value) return;
if (this.rule.value.added) {
fireEvent(this, 'added-rule-removed');
}
@@ -269,13 +269,13 @@
}
_handleUndoRemove() {
- if (!this.rule) return;
+ if (!this.rule?.value) return;
this._deleted = false;
delete this.rule.value.deleted;
}
_handleUndoChange() {
- if (!this.rule) return;
+ if (!this.rule?.value) return;
// gr-permission will take care of removing rules that were added but
// unsaved. We need to keep the added bit for the filter.
if (this.rule.value.added) {
@@ -289,7 +289,7 @@
@observe('rule.value.*')
_handleValueChange() {
- if (!this._originalRuleValues || !this.rule) {
+ if (!this._originalRuleValues || !this.rule?.value) {
return;
}
this.rule.value.modified = true;
@@ -297,7 +297,8 @@
fireEvent(this, 'access-modified');
}
- _setOriginalRuleValues(value: RuleValue) {
+ _setOriginalRuleValues(value?: RuleValue) {
+ if (value === undefined) return;
this._originalRuleValues = {...value};
}
}
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.js b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.js
deleted file mode 100644
index f3df132..0000000
--- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.js
+++ /dev/null
@@ -1,586 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 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.
- */
-
-import '../../../test/common-test-setup-karma.js';
-import './gr-rule-editor.js';
-
-const basicFixture = fixtureFromElement('gr-rule-editor');
-
-suite('gr-rule-editor tests', () => {
- let element;
-
- setup(() => {
- element = basicFixture.instantiate();
- });
-
- suite('unit tests', () => {
- test('_computeForce, _computeForceClass, and _computeForceOptions',
- () => {
- const ForcePushOptions = {
- ALLOW: [
- {name: 'Allow pushing (but not force pushing)', value: false},
- {name: 'Allow pushing with or without force', value: true},
- ],
- BLOCK: [
- {name: 'Block pushing with or without force', value: false},
- {name: 'Block force pushing', value: true},
- ],
- };
-
- const FORCE_EDIT_OPTIONS = [
- {
- name: 'No Force Edit',
- value: false,
- },
- {
- name: 'Force Edit',
- value: true,
- },
- ];
- let permission = 'push';
- let action = 'ALLOW';
- assert.isTrue(element._computeForce(permission, action));
- assert.equal(element._computeForceClass(permission, action),
- 'force');
- assert.deepEqual(element._computeForceOptions(permission, action),
- ForcePushOptions.ALLOW);
-
- action = 'BLOCK';
- assert.isTrue(element._computeForce(permission, action));
- assert.equal(element._computeForceClass(permission, action),
- 'force');
- assert.deepEqual(element._computeForceOptions(permission, action),
- ForcePushOptions.BLOCK);
-
- action = 'DENY';
- assert.isFalse(element._computeForce(permission, action));
- assert.equal(element._computeForceClass(permission, action), '');
- assert.equal(
- element._computeForceOptions(permission, action).length, 0);
-
- permission = 'editTopicName';
- assert.isTrue(element._computeForce(permission));
- assert.equal(element._computeForceClass(permission), 'force');
- assert.deepEqual(element._computeForceOptions(permission),
- FORCE_EDIT_OPTIONS);
- permission = 'submit';
- assert.isFalse(element._computeForce(permission));
- assert.equal(element._computeForceClass(permission), '');
- assert.deepEqual(element._computeForceOptions(permission), []);
- });
-
- test('_computeSectionClass', () => {
- let deleted = true;
- let editing = false;
- assert.equal(element._computeSectionClass(editing, deleted), 'deleted');
-
- deleted = false;
- assert.equal(element._computeSectionClass(editing, deleted), '');
-
- editing = true;
- assert.equal(element._computeSectionClass(editing, deleted), 'editing');
-
- deleted = true;
- assert.equal(element._computeSectionClass(editing, deleted),
- 'editing deleted');
- });
-
- test('_getDefaultRuleValues', () => {
- let permission = 'priority';
- let label;
- assert.deepEqual(element._getDefaultRuleValues(permission, label),
- {action: 'BATCH'});
- permission = 'label-Code-Review';
- label = {values: [
- {value: -2, text: 'This shall not be merged'},
- {value: -1, text: 'I would prefer this is not merged as is'},
- {value: -0, text: 'No score'},
- {value: 1, text: 'Looks good to me, but someone else must approve'},
- {value: 2, text: 'Looks good to me, approved'},
- ]};
- assert.deepEqual(element._getDefaultRuleValues(permission, label),
- {action: 'ALLOW', max: 2, min: -2});
- permission = 'push';
- label = undefined;
- assert.deepEqual(element._getDefaultRuleValues(permission, label),
- {action: 'ALLOW', force: false});
- permission = 'submit';
- assert.deepEqual(element._getDefaultRuleValues(permission, label),
- {action: 'ALLOW'});
- });
-
- test('_setDefaultRuleValues', () => {
- element.rule = {id: 123};
- const defaultValue = {action: 'ALLOW'};
- sinon.stub(element, '_getDefaultRuleValues').returns(defaultValue);
- element._setDefaultRuleValues();
- assert.isTrue(element._getDefaultRuleValues.called);
- assert.equal(element.rule.value, defaultValue);
- });
-
- test('_computeOptions', () => {
- const PRIORITY_OPTIONS = [
- 'BATCH',
- 'INTERACTIVE',
- ];
- const DROPDOWN_OPTIONS = [
- 'ALLOW',
- 'DENY',
- 'BLOCK',
- ];
- let permission = 'priority';
- assert.deepEqual(element._computeOptions(permission), PRIORITY_OPTIONS);
- permission = 'submit';
- assert.deepEqual(element._computeOptions(permission), DROPDOWN_OPTIONS);
- });
-
- test('_handleValueChange', () => {
- const modifiedHandler = sinon.stub();
- element.rule = {value: {}};
- element.addEventListener('access-modified', modifiedHandler);
- element._handleValueChange();
- assert.isNotOk(element.rule.value.modified);
- element._originalRuleValues = {};
- element._handleValueChange();
- assert.isTrue(element.rule.value.modified);
- assert.isTrue(modifiedHandler.called);
- });
-
- test('_handleAccessSaved', () => {
- const originalValue = {action: 'DENY'};
- const newValue = {action: 'ALLOW'};
- element._originalRuleValues = originalValue;
- element.rule = {value: newValue};
- element._handleAccessSaved();
- assert.deepEqual(element._originalRuleValues, newValue);
- });
-
- test('_setOriginalRuleValues', () => {
- const value = {
- action: 'ALLOW',
- force: false,
- };
- element._setOriginalRuleValues(value);
- assert.deepEqual(element._originalRuleValues, value);
- });
- });
-
- suite('already existing generic rule', () => {
- setup(async () => {
- element.group = 'Group Name';
- element.permission = 'submit';
- element.rule = {
- id: '123',
- value: {
- action: 'ALLOW',
- force: false,
- },
- };
- element.section = 'refs/*';
-
- // Typically called on ready since elements will have properties defined
- // by the parent element.
- element._setupValues(element.rule);
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('values are set correctly', () => {
- assert.equal(element.$.action.bindValue, element.rule.value.action);
- assert.isNotOk(element.root.querySelector('#labelMin'));
- assert.isNotOk(element.root.querySelector('#labelMax'));
- assert.isFalse(element.$.force.classList.contains('force'));
- });
-
- test('modify and cancel restores original values', () => {
- element.editing = true;
- assert.notEqual(getComputedStyle(element.$.removeBtn).display, 'none');
- assert.isNotOk(element.rule.value.modified);
- element.$.action.bindValue = 'DENY';
- assert.isTrue(element.rule.value.modified);
- element.editing = false;
- assert.equal(getComputedStyle(element.$.removeBtn).display, 'none');
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- assert.equal(element.$.action.bindValue, 'ALLOW');
- assert.isNotOk(element.rule.value.modified);
- });
-
- test('modify value', () => {
- assert.isNotOk(element.rule.value.modified);
- element.$.action.bindValue = 'DENY';
- flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('all selects are disabled when not in edit mode', () => {
- const selects = element.root.querySelectorAll('select');
- for (const select of selects) {
- assert.isTrue(select.disabled);
- }
- element.editing = true;
- for (const select of selects) {
- assert.isFalse(select.disabled);
- }
- });
-
- test('remove rule and undo remove', () => {
- element.editing = true;
- element.rule = {id: 123, value: {action: 'ALLOW'}};
- assert.isFalse(
- element.$.deletedContainer.classList.contains('deleted'));
- MockInteractions.tap(element.$.removeBtn);
- assert.isTrue(element.$.deletedContainer.classList.contains('deleted'));
- assert.isTrue(element._deleted);
- assert.isTrue(element.rule.value.deleted);
-
- MockInteractions.tap(element.$.undoRemoveBtn);
- assert.isFalse(element._deleted);
- assert.isNotOk(element.rule.value.deleted);
- });
-
- test('remove rule and cancel', () => {
- element.editing = true;
- assert.notEqual(getComputedStyle(element.$.removeBtn).display, 'none');
- assert.equal(getComputedStyle(element.$.deletedContainer).display,
- 'none');
-
- element.rule = {id: 123, value: {action: 'ALLOW'}};
- MockInteractions.tap(element.$.removeBtn);
- assert.notEqual(getComputedStyle(element.$.removeBtn).display, 'none');
- assert.notEqual(getComputedStyle(element.$.deletedContainer).display,
- 'none');
- assert.isTrue(element._deleted);
- assert.isTrue(element.rule.value.deleted);
-
- element.editing = false;
- assert.isFalse(element._deleted);
- assert.isNotOk(element.rule.value.deleted);
- assert.isNotOk(element.rule.value.modified);
-
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- assert.equal(getComputedStyle(element.$.removeBtn).display, 'none');
- assert.equal(getComputedStyle(element.$.deletedContainer).display,
- 'none');
- });
-
- test('_computeGroupPath', () => {
- const group = '123';
- assert.equal(element._computeGroupPath(group),
- `/admin/groups/123`);
- });
- });
-
- suite('new edit rule', () => {
- setup(async () => {
- element.group = 'Group Name';
- element.permission = 'editTopicName';
- element.rule = {
- id: '123',
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.rule.value.added = true;
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- // Since the element does not already have default values, they should
- // be set. The original values should be set to those too.
- assert.isNotOk(element.rule.value.modified);
- const expectedRuleValue = {
- action: 'ALLOW',
- force: false,
- added: true,
- };
- assert.deepEqual(element.rule.value, expectedRuleValue);
- test('values are set correctly', () => {
- assert.equal(element.$.action.bindValue, expectedRuleValue.action);
- assert.equal(element.$.force.bindValue, expectedRuleValue.action);
- });
- });
-
- test('modify value', () => {
- assert.isNotOk(element.rule.value.modified);
- element.$.force.bindValue = true;
- flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('remove value', () => {
- element.editing = true;
- const removeStub = sinon.stub();
- element.addEventListener('added-rule-removed', removeStub);
- MockInteractions.tap(element.$.removeBtn);
- flush();
- assert.isTrue(removeStub.called);
- });
- });
-
- suite('already existing rule with labels', () => {
- setup(async () => {
- element.label = {values: [
- {value: -2, text: 'This shall not be merged'},
- {value: -1, text: 'I would prefer this is not merged as is'},
- {value: -0, text: 'No score'},
- {value: 1, text: 'Looks good to me, but someone else must approve'},
- {value: 2, text: 'Looks good to me, approved'},
- ]};
- element.group = 'Group Name';
- element.permission = 'label-Code-Review';
- element.rule = {
- id: '123',
- value: {
- action: 'ALLOW',
- force: false,
- max: 2,
- min: -2,
- },
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('values are set correctly', () => {
- assert.equal(element.$.action.bindValue, element.rule.value.action);
- assert.equal(
- element.root.querySelector('#labelMin').bindValue,
- element.rule.value.min);
- assert.equal(
- element.root.querySelector('#labelMax').bindValue,
- element.rule.value.max);
- assert.isFalse(element.$.force.classList.contains('force'));
- });
-
- test('modify value', () => {
- const removeStub = sinon.stub();
- element.addEventListener('added-rule-removed', removeStub);
- assert.isNotOk(element.rule.value.modified);
- element.root.querySelector('#labelMin').bindValue = 1;
- flush();
- assert.isTrue(element.rule.value.modified);
- assert.isFalse(removeStub.called);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
- });
-
- suite('new rule with labels', () => {
- setup(async () => {
- sinon.spy(element, '_setDefaultRuleValues');
- element.label = {values: [
- {value: -2, text: 'This shall not be merged'},
- {value: -1, text: 'I would prefer this is not merged as is'},
- {value: -0, text: 'No score'},
- {value: 1, text: 'Looks good to me, but someone else must approve'},
- {value: 2, text: 'Looks good to me, approved'},
- ]};
- element.group = 'Group Name';
- element.permission = 'label-Code-Review';
- element.rule = {
- id: '123',
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.rule.value.added = true;
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- // Since the element does not already have default values, they should
- // be set. The original values should be set to those too.
- assert.isNotOk(element.rule.value.modified);
- assert.isTrue(element._setDefaultRuleValues.called);
-
- const expectedRuleValue = {
- max: element.label.values[element.label.values.length - 1].value,
- min: element.label.values[0].value,
- action: 'ALLOW',
- added: true,
- };
- assert.deepEqual(element.rule.value, expectedRuleValue);
- test('values are set correctly', () => {
- assert.equal(
- element.$.action.bindValue,
- expectedRuleValue.action);
- assert.equal(
- element.root.querySelector('#labelMin').bindValue,
- expectedRuleValue.min);
- assert.equal(
- element.root.querySelector('#labelMax').bindValue,
- expectedRuleValue.max);
- });
- });
-
- test('modify value', () => {
- assert.isNotOk(element.rule.value.modified);
- element.root.querySelector('#labelMin').bindValue = 1;
- flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
- });
-
- suite('already existing push rule', () => {
- setup(async () => {
- element.group = 'Group Name';
- element.permission = 'push';
- element.rule = {
- id: '123',
- value: {
- action: 'ALLOW',
- force: true,
- },
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('values are set correctly', () => {
- assert.isTrue(element.$.force.classList.contains('force'));
- assert.equal(element.$.action.bindValue, element.rule.value.action);
- assert.equal(
- element.root.querySelector('#force').bindValue,
- element.rule.value.force);
- assert.isNotOk(element.root.querySelector('#labelMin'));
- assert.isNotOk(element.root.querySelector('#labelMax'));
- });
-
- test('modify value', () => {
- assert.isNotOk(element.rule.value.modified);
- element.$.action.bindValue = false;
- flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
- });
-
- suite('new push rule', () => {
- setup(async () => {
- element.group = 'Group Name';
- element.permission = 'push';
- element.rule = {
- id: '123',
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.rule.value.added = true;
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- // Since the element does not already have default values, they should
- // be set. The original values should be set to those too.
- assert.isNotOk(element.rule.value.modified);
- const expectedRuleValue = {
- action: 'ALLOW',
- force: false,
- added: true,
- };
- assert.deepEqual(element.rule.value, expectedRuleValue);
- test('values are set correctly', () => {
- assert.equal(element.$.action.bindValue, expectedRuleValue.action);
- assert.equal(element.$.force.bindValue, expectedRuleValue.action);
- });
- });
-
- test('modify value', () => {
- assert.isNotOk(element.rule.value.modified);
- element.$.force.bindValue = true;
- flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
- });
-
- suite('already existing edit rule', () => {
- setup(async () => {
- element.group = 'Group Name';
- element.permission = 'editTopicName';
- element.rule = {
- id: '123',
- value: {
- action: 'ALLOW',
- force: true,
- },
- };
- element.section = 'refs/*';
- element._setupValues(element.rule);
- await flush();
- element.connectedCallback();
- });
-
- test('_ruleValues and _originalRuleValues are set correctly', () => {
- assert.deepEqual(element._originalRuleValues, element.rule.value);
- });
-
- test('values are set correctly', () => {
- assert.isTrue(element.$.force.classList.contains('force'));
- assert.equal(element.$.action.bindValue, element.rule.value.action);
- assert.equal(
- element.root.querySelector('#force').bindValue,
- element.rule.value.force);
- assert.isNotOk(element.root.querySelector('#labelMin'));
- assert.isNotOk(element.root.querySelector('#labelMax'));
- });
-
- test('modify value', async () => {
- assert.isNotOk(element.rule.value.modified);
- element.$.action.bindValue = false;
- await flush();
- assert.isTrue(element.rule.value.modified);
-
- // The original value should now differ from the rule values.
- assert.notDeepEqual(element._originalRuleValues, element.rule.value);
- });
- });
-});
-
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.ts b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.ts
new file mode 100644
index 0000000..1afd123
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.ts
@@ -0,0 +1,685 @@
+/**
+ * @license
+ * Copyright (C) 2017 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.
+ */
+
+import '../../../test/common-test-setup-karma';
+import './gr-rule-editor';
+import {GrRuleEditor} from './gr-rule-editor';
+import {AccessPermissionId} from '../../../utils/access-util';
+import {query, queryAll, queryAndAssert} from '../../../test/test-utils';
+import {GrButton} from '../../shared/gr-button/gr-button';
+import {GrSelect} from '../../shared/gr-select/gr-select';
+import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
+
+const basicFixture = fixtureFromElement('gr-rule-editor');
+
+suite('gr-rule-editor tests', () => {
+ let element: GrRuleEditor;
+
+ setup(() => {
+ element = basicFixture.instantiate();
+ });
+
+ suite('unit tests', () => {
+ test('_computeForce, _computeForceClass, and _computeForceOptions', () => {
+ const ForcePushOptions = {
+ ALLOW: [
+ {name: 'Allow pushing (but not force pushing)', value: false},
+ {name: 'Allow pushing with or without force', value: true},
+ ],
+ BLOCK: [
+ {name: 'Block pushing with or without force', value: false},
+ {name: 'Block force pushing', value: true},
+ ],
+ };
+
+ const FORCE_EDIT_OPTIONS = [
+ {
+ name: 'No Force Edit',
+ value: false,
+ },
+ {
+ name: 'Force Edit',
+ value: true,
+ },
+ ];
+ let permission = 'push' as AccessPermissionId;
+ let action = 'ALLOW';
+ assert.isTrue(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action), 'force');
+ assert.deepEqual(
+ element._computeForceOptions(permission, action),
+ ForcePushOptions.ALLOW
+ );
+
+ action = 'BLOCK';
+ assert.isTrue(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action), 'force');
+ assert.deepEqual(
+ element._computeForceOptions(permission, action),
+ ForcePushOptions.BLOCK
+ );
+
+ action = 'DENY';
+ assert.isFalse(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action), '');
+ assert.equal(element._computeForceOptions(permission, action).length, 0);
+
+ permission = 'editTopicName' as AccessPermissionId;
+ assert.isTrue(element._computeForce(permission));
+ assert.equal(element._computeForceClass(permission), 'force');
+ assert.deepEqual(
+ element._computeForceOptions(permission),
+ FORCE_EDIT_OPTIONS
+ );
+ permission = 'submit' as AccessPermissionId;
+ assert.isFalse(element._computeForce(permission));
+ assert.equal(element._computeForceClass(permission), '');
+ assert.deepEqual(element._computeForceOptions(permission), []);
+ });
+
+ test('_computeSectionClass', () => {
+ let deleted = true;
+ let editing = false;
+ assert.equal(element._computeSectionClass(editing, deleted), 'deleted');
+
+ deleted = false;
+ assert.equal(element._computeSectionClass(editing, deleted), '');
+
+ editing = true;
+ assert.equal(element._computeSectionClass(editing, deleted), 'editing');
+
+ deleted = true;
+ assert.equal(
+ element._computeSectionClass(editing, deleted),
+ 'editing deleted'
+ );
+ });
+
+ test('_getDefaultRuleValues', () => {
+ let permission = 'priority' as AccessPermissionId;
+ let label;
+ assert.deepEqual(element._getDefaultRuleValues(permission, label), {
+ action: 'BATCH',
+ });
+ permission = 'label-Code-Review' as AccessPermissionId;
+ label = {
+ values: [
+ {value: -2, text: 'This shall not be merged'},
+ {value: -1, text: 'I would prefer this is not merged as is'},
+ {value: -0, text: 'No score'},
+ {value: 1, text: 'Looks good to me, but someone else must approve'},
+ {value: 2, text: 'Looks good to me, approved'},
+ ],
+ };
+ assert.deepEqual(element._getDefaultRuleValues(permission, label), {
+ action: 'ALLOW',
+ max: 2,
+ min: -2,
+ });
+ permission = 'push' as AccessPermissionId;
+ label = undefined;
+ assert.deepEqual(element._getDefaultRuleValues(permission, label), {
+ action: 'ALLOW',
+ force: false,
+ });
+ permission = 'submit' as AccessPermissionId;
+ assert.deepEqual(element._getDefaultRuleValues(permission, label), {
+ action: 'ALLOW',
+ });
+ });
+
+ test('_setDefaultRuleValues', async () => {
+ element.rule = {value: {}};
+ const defaultValue = {action: 'ALLOW'};
+ const getDefaultRuleValuesStub = sinon
+ .stub(element, '_getDefaultRuleValues')
+ .returns(defaultValue);
+ element._setDefaultRuleValues();
+ assert.isTrue(getDefaultRuleValuesStub.called);
+ assert.equal(element.rule!.value, defaultValue);
+ });
+
+ test('_computeOptions', () => {
+ const PRIORITY_OPTIONS = ['BATCH', 'INTERACTIVE'];
+ const DROPDOWN_OPTIONS = ['ALLOW', 'DENY', 'BLOCK'];
+ let permission = 'priority';
+ assert.deepEqual(element._computeOptions(permission), PRIORITY_OPTIONS);
+ permission = 'submit';
+ assert.deepEqual(element._computeOptions(permission), DROPDOWN_OPTIONS);
+ });
+
+ test('_handleValueChange', () => {
+ const modifiedHandler = sinon.stub();
+ element.rule = {value: {}};
+ element.addEventListener('access-modified', modifiedHandler);
+ element._handleValueChange();
+ assert.isNotOk(element.rule!.value!.modified);
+ element._originalRuleValues = {};
+ element._handleValueChange();
+ assert.isTrue(element.rule!.value!.modified);
+ assert.isTrue(modifiedHandler.called);
+ });
+
+ test('_handleAccessSaved', () => {
+ const originalValue = {action: 'DENY'};
+ const newValue = {action: 'ALLOW'};
+ element._originalRuleValues = originalValue;
+ element.rule = {value: newValue};
+ element._handleAccessSaved();
+ assert.deepEqual(element._originalRuleValues, newValue);
+ });
+
+ test('_setOriginalRuleValues', () => {
+ const value = {
+ action: 'ALLOW',
+ force: false,
+ };
+ element._setOriginalRuleValues(value);
+ assert.deepEqual(element._originalRuleValues, value);
+ });
+ });
+
+ suite('already existing generic rule', () => {
+ setup(async () => {
+ element.groupName = 'Group Name';
+ element.permission = 'submit' as AccessPermissionId;
+ element.rule = {
+ value: {
+ action: 'ALLOW',
+ force: false,
+ },
+ };
+ element.section = 'refs/*';
+
+ // Typically called on ready since elements will have properties defined
+ // by the parent element.
+ element._setupValues(element.rule);
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('values are set correctly', () => {
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ element.rule!.value!.action
+ );
+ assert.isNotOk(query<GrSelect>(element, '#labelMin'));
+ assert.isNotOk(query<GrSelect>(element, '#labelMax'));
+ assert.isFalse(
+ queryAndAssert<GrSelect>(element, '#force').classList.contains('force')
+ );
+ });
+
+ test('modify and cancel restores original values', () => {
+ element.editing = true;
+ assert.notEqual(
+ getComputedStyle(queryAndAssert<GrButton>(element, '#removeBtn'))
+ .display,
+ 'none'
+ );
+ assert.isNotOk(element.rule!.value!.modified);
+ const actionBindValue = queryAndAssert<GrSelect>(element, '#action');
+ actionBindValue.bindValue = 'DENY';
+ assert.isTrue(element.rule!.value!.modified);
+ element.editing = false;
+ assert.equal(
+ getComputedStyle(queryAndAssert<GrButton>(element, '#removeBtn'))
+ .display,
+ 'none'
+ );
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ 'ALLOW'
+ );
+ assert.isNotOk(element.rule!.value!.modified);
+ });
+
+ test('modify value', () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const actionBindValue = queryAndAssert<GrSelect>(element, '#action');
+ actionBindValue.bindValue = 'DENY';
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('all selects are disabled when not in edit mode', () => {
+ const selects = queryAll<HTMLSelectElement>(element, 'select');
+ for (const select of selects) {
+ assert.isTrue(select.disabled);
+ }
+ element.editing = true;
+ for (const select of selects) {
+ assert.isFalse(select.disabled);
+ }
+ });
+
+ test('remove rule and undo remove', () => {
+ element.editing = true;
+ element.rule = {value: {action: 'ALLOW'}};
+ assert.isFalse(
+ queryAndAssert<HTMLDivElement>(
+ element,
+ '#deletedContainer'
+ ).classList.contains('deleted')
+ );
+ MockInteractions.tap(queryAndAssert<GrButton>(element, '#removeBtn'));
+ assert.isTrue(
+ queryAndAssert<HTMLDivElement>(
+ element,
+ '#deletedContainer'
+ ).classList.contains('deleted')
+ );
+ assert.isTrue(element._deleted);
+ assert.isTrue(element.rule!.value!.deleted);
+
+ MockInteractions.tap(queryAndAssert<GrButton>(element, '#undoRemoveBtn'));
+ assert.isFalse(element._deleted);
+ assert.isNotOk(element.rule!.value!.deleted);
+ });
+
+ test('remove rule and cancel', () => {
+ element.editing = true;
+ assert.notEqual(
+ getComputedStyle(queryAndAssert<GrButton>(element, '#removeBtn'))
+ .display,
+ 'none'
+ );
+ assert.equal(
+ getComputedStyle(
+ queryAndAssert<HTMLDivElement>(element, '#deletedContainer')
+ ).display,
+ 'none'
+ );
+
+ element.rule = {value: {action: 'ALLOW'}};
+ MockInteractions.tap(queryAndAssert<GrButton>(element, '#removeBtn'));
+ assert.notEqual(
+ getComputedStyle(queryAndAssert<GrButton>(element, '#removeBtn'))
+ .display,
+ 'none'
+ );
+ assert.notEqual(
+ getComputedStyle(
+ queryAndAssert<HTMLDivElement>(element, '#deletedContainer')
+ ).display,
+ 'none'
+ );
+ assert.isTrue(element._deleted);
+ assert.isTrue(element.rule!.value!.deleted);
+
+ element.editing = false;
+ assert.isFalse(element._deleted);
+ assert.isNotOk(element.rule!.value!.deleted);
+ assert.isNotOk(element.rule!.value!.modified);
+
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ assert.equal(
+ getComputedStyle(queryAndAssert<GrButton>(element, '#removeBtn'))
+ .display,
+ 'none'
+ );
+ assert.equal(
+ getComputedStyle(
+ queryAndAssert<HTMLDivElement>(element, '#deletedContainer')
+ ).display,
+ 'none'
+ );
+ });
+
+ test('_computeGroupPath', () => {
+ const group = '123';
+ assert.equal(element._computeGroupPath(group), '/admin/groups/123');
+ });
+ });
+
+ suite('new edit rule', () => {
+ setup(async () => {
+ element.groupName = 'Group Name';
+ element.permission = 'editTopicName' as AccessPermissionId;
+ element.rule = {};
+ element.section = 'refs/*';
+ element._setupValues(element.rule!);
+ await flush();
+ element.rule!.value!.added = true;
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ // Since the element does not already have default values, they should
+ // be set. The original values should be set to those too.
+ assert.isNotOk(element.rule!.value!.modified);
+ const expectedRuleValue = {
+ action: 'ALLOW',
+ force: false,
+ added: true,
+ };
+ assert.deepEqual(element.rule!.value, expectedRuleValue);
+ test('values are set correctly', () => {
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ expectedRuleValue.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#force').bindValue,
+ expectedRuleValue.action
+ );
+ });
+ });
+
+ test('modify value', () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const forceBindValue = queryAndAssert<GrSelect>(element, '#force');
+ forceBindValue.bindValue = true;
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('remove value', () => {
+ element.editing = true;
+ const removeStub = sinon.stub();
+ element.addEventListener('added-rule-removed', removeStub);
+ MockInteractions.tap(queryAndAssert<GrButton>(element, '#removeBtn'));
+ flush();
+ assert.isTrue(removeStub.called);
+ });
+ });
+
+ suite('already existing rule with labels', () => {
+ setup(async () => {
+ element.label = {
+ values: [
+ {value: -2, text: 'This shall not be merged'},
+ {value: -1, text: 'I would prefer this is not merged as is'},
+ {value: -0, text: 'No score'},
+ {value: 1, text: 'Looks good to me, but someone else must approve'},
+ {value: 2, text: 'Looks good to me, approved'},
+ ],
+ };
+ element.groupName = 'Group Name';
+ element.permission = 'label-Code-Review' as AccessPermissionId;
+ element.rule = {
+ value: {
+ action: 'ALLOW',
+ force: false,
+ max: 2,
+ min: -2,
+ },
+ };
+ element.section = 'refs/*';
+ element._setupValues(element.rule);
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('values are set correctly', () => {
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ element.rule!.value!.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#labelMin').bindValue,
+ element.rule!.value!.min
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#labelMax').bindValue,
+ element.rule!.value!.max
+ );
+ assert.isFalse(
+ queryAndAssert<GrSelect>(element, '#force').classList.contains('force')
+ );
+ });
+
+ test('modify value', () => {
+ const removeStub = sinon.stub();
+ element.addEventListener('added-rule-removed', removeStub);
+ assert.isNotOk(element.rule!.value!.modified);
+ const labelMinBindValue = queryAndAssert<GrSelect>(element, '#labelMin');
+ labelMinBindValue.bindValue = 1;
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+ assert.isFalse(removeStub.called);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+ });
+
+ suite('new rule with labels', () => {
+ let setDefaultRuleValuesSpy: sinon.SinonSpy;
+
+ setup(async () => {
+ setDefaultRuleValuesSpy = sinon.spy(element, '_setDefaultRuleValues');
+ element.label = {
+ values: [
+ {value: -2, text: 'This shall not be merged'},
+ {value: -1, text: 'I would prefer this is not merged as is'},
+ {value: -0, text: 'No score'},
+ {value: 1, text: 'Looks good to me, but someone else must approve'},
+ {value: 2, text: 'Looks good to me, approved'},
+ ],
+ };
+ element.groupName = 'Group Name';
+ element.permission = 'label-Code-Review' as AccessPermissionId;
+ element.rule = {};
+ element.section = 'refs/*';
+ element._setupValues(element.rule!);
+ await flush();
+ element.rule!.value!.added = true;
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ // Since the element does not already have default values, they should
+ // be set. The original values should be set to those too.
+ assert.isNotOk(element.rule!.value!.modified);
+ assert.isTrue(setDefaultRuleValuesSpy.called);
+
+ const expectedRuleValue = {
+ max: element.label!.values![element.label!.values.length - 1].value,
+ min: element.label!.values![0].value,
+ action: 'ALLOW',
+ added: true,
+ };
+ assert.deepEqual(element.rule!.value, expectedRuleValue);
+ test('values are set correctly', () => {
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ expectedRuleValue.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#labelMin').bindValue,
+ expectedRuleValue.min
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#labelMax').bindValue,
+ expectedRuleValue.max
+ );
+ });
+ });
+
+ test('modify value', () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const labelMinBindValue = queryAndAssert<GrSelect>(element, '#labelMin');
+ labelMinBindValue.bindValue = 1;
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+ });
+
+ suite('already existing push rule', () => {
+ setup(async () => {
+ element.groupName = 'Group Name';
+ element.permission = 'push' as AccessPermissionId;
+ element.rule = {
+ value: {
+ action: 'ALLOW',
+ force: true,
+ },
+ };
+ element.section = 'refs/*';
+ element._setupValues(element.rule!);
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('values are set correctly', () => {
+ assert.isTrue(
+ queryAndAssert<GrSelect>(element, '#force').classList.contains('force')
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ element.rule!.value!.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#force').bindValue,
+ element.rule!.value!.force
+ );
+ assert.isNotOk(query<GrSelect>(element, '#labelMin'));
+ assert.isNotOk(query<GrSelect>(element, '#labelMax'));
+ });
+
+ test('modify value', () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const actionBindValue = queryAndAssert<GrSelect>(element, '#action');
+ actionBindValue.bindValue = false;
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+ });
+
+ suite('new push rule', () => {
+ setup(async () => {
+ element.groupName = 'Group Name';
+ element.permission = 'push' as AccessPermissionId;
+ element.rule = {};
+ element.section = 'refs/*';
+ element._setupValues(element.rule!);
+ await flush();
+ element.rule!.value!.added = true;
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ // Since the element does not already have default values, they should
+ // be set. The original values should be set to those too.
+ assert.isNotOk(element.rule!.value!.modified);
+ const expectedRuleValue = {
+ action: 'ALLOW',
+ force: false,
+ added: true,
+ };
+ assert.deepEqual(element.rule!.value, expectedRuleValue);
+ test('values are set correctly', () => {
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ expectedRuleValue.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#force').bindValue,
+ expectedRuleValue.action
+ );
+ });
+ });
+
+ test('modify value', () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const forceBindValue = queryAndAssert<GrSelect>(element, '#force');
+ forceBindValue.bindValue = true;
+ flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+ });
+
+ suite('already existing edit rule', () => {
+ setup(async () => {
+ element.groupName = 'Group Name';
+ element.permission = 'editTopicName' as AccessPermissionId;
+ element.rule = {
+ value: {
+ action: 'ALLOW',
+ force: true,
+ },
+ };
+ element.section = 'refs/*';
+ element._setupValues(element.rule);
+ await flush();
+ element.connectedCallback();
+ });
+
+ test('_ruleValues and _originalRuleValues are set correctly', () => {
+ assert.deepEqual(element._originalRuleValues, element.rule!.value);
+ });
+
+ test('values are set correctly', () => {
+ assert.isTrue(
+ queryAndAssert<GrSelect>(element, '#force').classList.contains('force')
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#action').bindValue,
+ element.rule!.value!.action
+ );
+ assert.equal(
+ queryAndAssert<GrSelect>(element, '#force').bindValue,
+ element.rule!.value!.force
+ );
+ assert.isNotOk(query<GrSelect>(element, '#labelMin'));
+ assert.isNotOk(query<GrSelect>(element, '#labelMax'));
+ });
+
+ test('modify value', async () => {
+ assert.isNotOk(element.rule!.value!.modified);
+ const actionBindValue = queryAndAssert<GrSelect>(element, '#action');
+ actionBindValue.bindValue = false;
+ await flush();
+ assert.isTrue(element.rule!.value!.modified);
+
+ // The original value should now differ from the rule values.
+ assert.notDeepEqual(element._originalRuleValues, element.rule!.value);
+ });
+ });
+});
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
index 80608da..d4b0328 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
@@ -44,10 +44,10 @@
} from '../../../types/common';
import {assertNever, hasOwnProperty} from '../../../utils/common-util';
import {pluralize} from '../../../utils/string-util';
-import {KnownExperimentId} from '../../../services/flags/flags';
import {
getRequirements,
iconForStatus,
+ showNewSubmitRequirements,
StandardLabels,
} from '../../../utils/label-util';
import {SubmitRequirementStatus} from '../../../api/rest-api';
@@ -113,17 +113,12 @@
@state() private dynamicCellEndpoints?: string[];
- @state() private isSubmitRequirementsUiEnabled = false;
-
reporting: ReportingService = getAppContext().reportingService;
private readonly flagsService = getAppContext().flagsService;
override connectedCallback() {
super.connectedCallback();
- this.isSubmitRequirementsUiEnabled = this.flagsService.isEnabled(
- KnownExperimentId.SUBMIT_REQUIREMENTS_UI
- );
getPluginLoader()
.awaitPluginsLoaded()
.then(() => {
@@ -537,7 +532,7 @@
}
private renderCommentsInfoWithLabel(labelName: string) {
- if (!this.isSubmitRequirementsUiEnabled) return;
+ if (!showNewSubmitRequirements(this.flagsService, this.change)) return;
if (labelName !== StandardLabels.CODE_REVIEW) return;
if (!this.change?.unresolved_comment_count) return;
return html`<iron-icon
@@ -593,7 +588,7 @@
// private but used in test
computeLabelClass(labelName: string) {
const classes = ['cell', 'label'];
- if (this.isSubmitRequirementsUiEnabled) {
+ if (showNewSubmitRequirements(this.flagsService, this.change)) {
const requirements = getRequirements(this.change).filter(
sr => sr.name === labelName
);
@@ -644,7 +639,7 @@
// private but used in test
computeLabelIcon(labelName: string): string {
- if (this.isSubmitRequirementsUiEnabled) {
+ if (showNewSubmitRequirements(this.flagsService, this.change)) {
const requirements = getRequirements(this.change).filter(
sr => sr.name === labelName
);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
index 58f9dcc..7d618f5 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -51,8 +51,10 @@
import {fireEvent, fireReload} from '../../../utils/event-util';
import {ScrollMode} from '../../../constants/constants';
import {listen} from '../../../services/shortcuts/shortcuts-service';
-import {KnownExperimentId} from '../../../services/flags/flags';
-import {PRIORITY_REQUIREMENTS_ORDER} from '../../../utils/label-util';
+import {
+ PRIORITY_REQUIREMENTS_ORDER,
+ showNewSubmitRequirements,
+} from '../../../utils/label-util';
import {addGlobalShortcut, Key} from '../../../utils/dom-util';
const NUMBER_FIXED_COLUMNS = 3;
@@ -205,20 +207,22 @@
return column.toLowerCase();
}
- @observe('account', 'preferences', '_config')
+ @observe('account', 'preferences', '_config', 'sections')
_computePreferences(
account?: AccountInfo,
preferences?: PreferencesInput,
- config?: ServerInfo
+ config?: ServerInfo,
+ sections?: ChangeListSection[]
) {
if (!config) {
return;
}
+ const changes = (sections ?? []).map(section => section.results).flat();
this.changeTableColumns = columnNames;
this.showNumber = false;
this.visibleChangeTableColumns = this.changeTableColumns.filter(col =>
- this._isColumnEnabled(col, config, this.flagsService.enabledExperiments)
+ this._isColumnEnabled(col, config, changes)
);
if (account && preferences) {
this.showNumber = !!(
@@ -229,11 +233,7 @@
column === 'Project' ? 'Repo' : column
);
this.visibleChangeTableColumns = prefColumns.filter(col =>
- this._isColumnEnabled(
- col,
- config,
- this.flagsService.enabledExperiments
- )
+ this._isColumnEnabled(col, config, changes)
);
}
}
@@ -242,12 +242,15 @@
/**
* Is the column disabled by a server config or experiment?
*/
- _isColumnEnabled(column: string, config: ServerInfo, experiments: string[]) {
+ _isColumnEnabled(column: string, config: ServerInfo, changes?: ChangeInfo[]) {
if (!columnNames.includes(column)) return false;
if (!config || !config.change) return true;
- if (column === 'Comments') return experiments.includes('comments-column');
+ if (column === 'Comments')
+ return this.flagsService.isEnabled('comments-column');
if (column === 'Requirements')
- return experiments.includes(KnownExperimentId.SUBMIT_REQUIREMENTS_UI);
+ return (changes ?? []).every(change =>
+ showNewSubmitRequirements(this.flagsService, change)
+ );
return true;
}
@@ -302,9 +305,10 @@
labels = labels.concat(currentLabels.filter(nonExistingLabel));
}
}
+ const changes = sections.map(section => section.results).flat();
if (
- this.flagsService.enabledExperiments.includes(
- KnownExperimentId.SUBMIT_REQUIREMENTS_UI
+ (changes ?? []).every(change =>
+ showNewSubmitRequirements(this.flagsService, change)
)
) {
labels = labels.filter(l => PRIORITY_REQUIREMENTS_ORDER.includes(l));
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index e635613..83808af 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -386,7 +386,7 @@
// Accessed in tests
readonly jsAPI = getAppContext().jsApiService;
- private readonly changeService = getAppContext().changeService;
+ private readonly changeModel = getAppContext().changeModel;
@property({type: Object})
change?: ChangeViewChangeInfo;
@@ -1716,7 +1716,7 @@
new Error('Properties change and changeNum must be set.')
);
}
- return this.changeService.fetchChangeUpdates(change).then(result => {
+ return this.changeModel.fetchChangeUpdates(change).then(result => {
if (!result.isLatest) {
this.dispatchEvent(
new CustomEvent<ShowAlertEventDetail>('show-alert', {
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 416bd43..d0126fd 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -87,7 +87,7 @@
} from '../../shared/gr-autocomplete/gr-autocomplete';
import {getRevertCreatedChangeIds} from '../../../utils/message-util';
import {Interaction} from '../../../constants/reporting';
-import {KnownExperimentId} from '../../../services/flags/flags';
+import {showNewSubmitRequirements} from '../../../utils/label-util';
const HASHTAG_ADD_MESSAGE = 'Add Hashtag';
@@ -213,9 +213,6 @@
@property({type: Object})
queryTopic?: AutocompleteQuery;
- @property({type: Boolean})
- _isSubmitRequirementsUiEnabled = false;
-
restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -225,9 +222,6 @@
override ready() {
super.ready();
this.queryTopic = (input: string) => this._getTopicSuggestions(input);
- this._isSubmitRequirementsUiEnabled = this.flagsService.isEnabled(
- KnownExperimentId.SUBMIT_REQUIREMENTS_UI
- );
}
@observe('change.labels')
@@ -713,13 +707,7 @@
}
_showNewSubmitRequirements(change?: ParsedChangeInfo) {
- if (!this._isSubmitRequirementsUiEnabled) return false;
- return (change?.submit_requirements ?? []).length > 0;
- }
-
- _showNewSubmitRequirementWarning(change?: ParsedChangeInfo) {
- if (!this._isSubmitRequirementsUiEnabled) return false;
- return (change?.submit_requirements ?? []).length === 0;
+ return showNewSubmitRequirements(this.flagsService, change);
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
index 15631e4..46303a9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
@@ -113,10 +113,6 @@
--iron-icon-height: 18px;
--iron-icon-width: 18px;
}
- .submit-requirement-error {
- color: var(--deemphasized-text-color);
- padding-left: var(--metadata-horizontal-padding);
- }
</style>
<gr-external-style id="externalStyle" name="change-metadata">
<div class="metadata-header">
@@ -479,11 +475,6 @@
mutable="[[_mutable]]"
></gr-change-requirements>
</template>
- <template is="dom-if" if="[[_showNewSubmitRequirementWarning(change)]]">
- <div class="submit-requirement-error">
- New Submit Requirements don't work on this change.
- </div>
- </template>
</div>
<section
id="webLinks"
diff --git a/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.ts b/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.ts
index 6991c03..8161592 100644
--- a/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.ts
@@ -154,7 +154,6 @@
mutable="[[mutable]]"
label="[[item.labelName]]"
label-info="[[item.labelInfo]]"
- showAlwaysOldUI
></gr-label-info>
</div>
</section>
@@ -205,7 +204,6 @@
mutable="[[mutable]]"
label="[[item.labelName]]"
label-info="[[item.labelInfo]]"
- showAlwaysOldUI
></gr-label-info>
</div>
</section>
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
index e8c5916..02d1beb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -59,10 +59,6 @@
import {modifierPressed} from '../../../utils/dom-util';
import {DropdownLink} from '../../shared/gr-dropdown/gr-dropdown';
import {fontStyles} from '../../../styles/gr-font-styles';
-import {
- changeComments$,
- threads$,
-} from '../../../services/comments/comments-model';
export enum SummaryChipStyles {
INFO = 'info',
@@ -405,6 +401,8 @@
private showAllChips = new Map<RunStatus | Category, boolean>();
+ private commentsModel = getAppContext().commentsModel;
+
private userModel = getAppContext().userModel;
private checksModel = getAppContext().checksModel;
@@ -441,8 +439,16 @@
this.checksModel.topLevelActionsLatest$,
x => (this.actions = x)
);
- subscribe(this, changeComments$, x => (this.changeComments = x));
- subscribe(this, threads$, x => (this.commentThreads = x));
+ subscribe(
+ this,
+ this.commentsModel.changeComments$,
+ x => (this.changeComments = x)
+ );
+ subscribe(
+ this,
+ this.commentsModel.threads$,
+ x => (this.commentThreads = x)
+ );
subscribe(this, this.userModel.account$, x => (this.selfAccount = x));
}
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 5bd0579..46e1647 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
@@ -191,20 +191,12 @@
import {ChangeStates} from '../../shared/gr-change-status/gr-change-status';
import {getRevertCreatedChangeIds} from '../../../utils/message-util';
import {
- changeComments$,
- drafts$,
-} from '../../../services/comments/comments-model';
-import {
getAddedByReason,
getRemovedByReason,
hasAttention,
} from '../../../utils/attention-set-util';
import {listen} from '../../../services/shortcuts/shortcuts-service';
-import {
- change$,
- changeLoadingStatus$,
- LoadingStatus,
-} from '../../../services/change/change-model';
+import {LoadingStatus} from '../../../services/change/change-model';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -281,15 +273,6 @@
* @event show-auth-required
*/
- // Accessed in tests.
- readonly reporting = getAppContext().reportingService;
-
- readonly jsAPI = getAppContext().jsApiService;
-
- private readonly changeService = getAppContext().changeService;
-
- private readonly checksModel = getAppContext().checksModel;
-
/**
* URL params passed from the router.
*/
@@ -560,15 +543,6 @@
})
resolveWeblinks?: GeneratedWebLink[];
- readonly restApiService = getAppContext().restApiService;
-
- // Private but used in tests.
- readonly userModel = getAppContext().userModel;
-
- private readonly commentsService = getAppContext().commentsService;
-
- private readonly shortcuts = getAppContext().shortcutsService;
-
override keyboardShortcuts(): ShortcutListener[] {
return [
listen(Shortcut.SEND_REPLY, _ => {}), // docOnly
@@ -615,6 +589,25 @@
];
}
+ // Accessed in tests.
+ readonly reporting = getAppContext().reportingService;
+
+ readonly jsAPI = getAppContext().jsApiService;
+
+ private readonly checksModel = getAppContext().checksModel;
+
+ readonly restApiService = getAppContext().restApiService;
+
+ // Private but used in tests.
+ readonly userModel = getAppContext().userModel;
+
+ // Private but used in tests.
+ readonly changeModel = getAppContext().changeModel;
+
+ private readonly commentsModel = getAppContext().commentsModel;
+
+ private readonly shortcuts = getAppContext().shortcutsService;
+
private subscriptions: Subscription[] = [];
private replyRefitTask?: DelayedTask;
@@ -644,7 +637,7 @@
})
);
this.subscriptions.push(
- drafts$.subscribe(drafts => {
+ this.commentsModel.drafts$.subscribe(drafts => {
this._diffDrafts = {...drafts};
})
);
@@ -654,12 +647,12 @@
})
);
this.subscriptions.push(
- changeComments$.subscribe(changeComments => {
+ this.commentsModel.changeComments$.subscribe(changeComments => {
this._changeComments = changeComments;
})
);
this.subscriptions.push(
- change$.subscribe(change => {
+ this.changeModel.change$.subscribe(change => {
// The change view is tied to a specific change number, so don't update
// _change to undefined.
if (change) this._change = change;
@@ -1889,7 +1882,7 @@
}
const detailCompletes = until(
- changeLoadingStatus$,
+ this.changeModel.changeLoadingStatus$,
status => status === LoadingStatus.LOADED
);
const editCompletes = this._getEdit();
@@ -2222,13 +2215,13 @@
const promises = [this._getCommitInfo(), this.$.fileList.reload()];
if (patchNumChanged) {
promises.push(
- this.commentsService.reloadPortedComments(
+ this.commentsModel.reloadPortedComments(
this._changeNum,
this._patchRange?.patchNum
)
);
promises.push(
- this.commentsService.reloadPortedDrafts(
+ this.commentsModel.reloadPortedDrafts(
this._changeNum,
this._patchRange?.patchNum
)
@@ -2338,7 +2331,7 @@
return;
}
const change = this._change;
- this.changeService.fetchChangeUpdates(change).then(result => {
+ this.changeModel.fetchChangeUpdates(change).then(result => {
let toastMessage = null;
if (!result.isLatest) {
toastMessage = ReloadToastMessage.NEWER_REVISION;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index b7a3eda..e8cce2d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -104,10 +104,7 @@
import {ParsedChangeInfo} from '../../../types/types';
import {GrRelatedChangesList} from '../gr-related-changes-list/gr-related-changes-list';
import {ChangeStates} from '../../shared/gr-change-status/gr-change-status';
-import {
- LoadingStatus,
- _testOnly_setState as setChangeState,
-} from '../../../services/change/change-model';
+import {LoadingStatus} from '../../../services/change/change-model';
import {FocusTarget, GrReplyDialog} from '../gr-reply-dialog/gr-reply-dialog';
import {GrOverlay} from '../../shared/gr-overlay/gr-overlay';
import {GrChangeStar} from '../../shared/gr-change-star/gr-change-star';
@@ -1524,7 +1521,7 @@
test('topic is coalesced to null', async () => {
sinon.stub(element, '_changeChanged');
- setChangeState({
+ element.changeModel.setState({
loadingStatus: LoadingStatus.LOADED,
change: {
...createChangeViewChange(),
@@ -1540,7 +1537,7 @@
test('commit sha is populated from getChangeDetail', async () => {
sinon.stub(element, '_changeChanged');
- setChangeState({
+ element.changeModel.setState({
loadingStatus: LoadingStatus.LOADED,
change: {
...createChangeViewChange(),
@@ -1557,7 +1554,7 @@
test('edit is added to change', () => {
sinon.stub(element, '_changeChanged');
const changeRevision = createRevision();
- setChangeState({
+ element.changeModel.setState({
loadingStatus: LoadingStatus.LOADED,
change: {
...createChangeViewChange(),
@@ -1978,7 +1975,7 @@
test('_selectedRevision updates when patchNum is changed', () => {
const revision1: RevisionInfo = createRevision(1);
const revision2: RevisionInfo = createRevision(2);
- setChangeState({
+ element.changeModel.setState({
loadingStatus: LoadingStatus.LOADED,
change: {
...createChangeViewChange(),
@@ -2009,7 +2006,7 @@
const revision1 = createRevision(1);
const revision2 = createRevision(2);
const revision3 = createEditRevision();
- setChangeState({
+ element.changeModel.setState({
loadingStatus: LoadingStatus.LOADED,
change: {
...createChangeViewChange(),
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
index 1778b2b..c24e054 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
@@ -29,8 +29,7 @@
import {customElement, property, query, state} from 'lit/decorators';
import {fontStyles} from '../../../styles/gr-font-styles';
import {subscribe} from '../../lit/subscription-controller';
-import {change$} from '../../../services/change/change-model';
-import {threads$} from '../../../services/comments/comments-model';
+import {getAppContext} from '../../../services/app-context';
import {ParsedChangeInfo} from '../../../types/types';
@customElement('gr-confirm-submit-dialog')
@@ -62,6 +61,10 @@
@state()
initialised = false;
+ private commentsModel = getAppContext().commentsModel;
+
+ private changeModel = getAppContext().changeModel;
+
static override get styles() {
return [
sharedStyles,
@@ -90,10 +93,10 @@
constructor() {
super();
- subscribe(this, change$, x => (this.change = x));
+ subscribe(this, this.changeModel.change$, x => (this.change = x));
subscribe(
this,
- threads$,
+ this.commentsModel.threads$,
x => (this.unresolvedThreads = x.filter(isUnresolved))
);
}
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 f286098..112077a 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
@@ -83,7 +83,6 @@
import {ParsedChangeInfo, PatchSetFile} from '../../../types/types';
import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
-import {changeComments$} from '../../../services/comments/comments-model';
import {listen} from '../../../services/shortcuts/shortcuts-service';
import {select} from '../../../utils/observable-util';
@@ -316,6 +315,8 @@
private readonly userModel = getAppContext().userModel;
+ private readonly commentsModel = getAppContext().commentsModel;
+
private readonly browserModel = getAppContext().browserModel;
private subscriptions: Subscription[] = [];
@@ -375,7 +376,7 @@
override connectedCallback() {
super.connectedCallback();
this.subscriptions = [
- changeComments$.subscribe(changeComments => {
+ this.commentsModel.changeComments$.subscribe(changeComments => {
this.changeComments = changeComments;
}),
this.browserModel.diffViewMode$.subscribe(
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 38875f5..fd6b0d4 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -1473,8 +1473,7 @@
}
async function renderAndGetNewDiffs(index) {
- const diffs =
- element.root.querySelectorAll('gr-diff-host');
+ const diffs = element.root.querySelectorAll('gr-diff-host');
for (let i = index; i < diffs.length; i++) {
await setupDiff(diffs[i]);
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
index 3c4702c..be04e6e 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
@@ -33,10 +33,13 @@
LabelValuesMap,
} from '../gr-label-score-row/gr-label-score-row';
import {getAppContext} from '../../../services/app-context';
-import {getTriggerVotes, labelCompare} from '../../../utils/label-util';
+import {
+ getTriggerVotes,
+ labelCompare,
+ showNewSubmitRequirements,
+} from '../../../utils/label-util';
import {Execution} from '../../../constants/reporting';
import {ChangeStatus} from '../../../constants/constants';
-import {KnownExperimentId} from '../../../services/flags/flags';
import {fontStyles} from '../../../styles/gr-font-styles';
@customElement('gr-label-scores')
@@ -90,7 +93,7 @@
}
override render() {
- if (this.flagsService.isEnabled(KnownExperimentId.SUBMIT_REQUIREMENTS_UI)) {
+ if (showNewSubmitRequirements(this.flagsService, this.change)) {
return this.renderNewSubmitRequirements();
} else {
return this.renderOldSubmitRequirements();
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
index c3ad0b6..36d0ce6 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
@@ -51,12 +51,6 @@
FormattedReviewerUpdateInfo,
ParsedChangeInfo,
} from '../../../types/types';
-import {threads$} from '../../../services/comments/comments-model';
-import {
- change$,
- changeNum$,
- repo$,
-} from '../../../services/change/change-model';
/**
* The content of the enum is also used in the UI for the button text.
@@ -257,6 +251,11 @@
private readonly userModel = getAppContext().userModel;
+ // Private but used in tests.
+ readonly commentsModel = getAppContext().commentsModel;
+
+ private readonly changeModel = getAppContext().changeModel;
+
private readonly reporting = getAppContext().reportingService;
private readonly shortcuts = getAppContext().shortcutsService;
@@ -266,12 +265,12 @@
override connectedCallback() {
super.connectedCallback();
this.subscriptions.push(
- threads$.subscribe(x => {
+ this.commentsModel.threads$.subscribe(x => {
this.commentThreads = x;
})
);
this.subscriptions.push(
- change$.subscribe(x => {
+ this.changeModel.change$.subscribe(x => {
this.change = x;
})
);
@@ -281,12 +280,12 @@
})
);
this.subscriptions.push(
- repo$.subscribe(x => {
+ this.changeModel.repo$.subscribe(x => {
this.projectName = x;
})
);
this.subscriptions.push(
- changeNum$.subscribe(x => {
+ this.changeModel.changeNum$.subscribe(x => {
this.changeNum = x;
})
);
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js
index 50e1a38..65c3c74 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js
@@ -22,7 +22,6 @@
import {MessageTag} from '../../../constants/constants.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
import {stubRestApi} from '../../../test/test-utils.js';
-import {updateStateComments} from '../../../services/comments/comments-model.js';
createCommentApiMockWithTemplateElement(
'gr-messages-list-comment-mock-api', html`
@@ -129,7 +128,7 @@
};
suite('basic tests', () => {
- setup(() => {
+ setup(async () => {
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
stubRestApi('getDiffComments').returns(Promise.resolve(comments));
stubRestApi('getDiffRobotComments').returns(Promise.resolve({}));
@@ -140,9 +139,9 @@
// comment API.
commentApiWrapper = basicFixture.instantiate();
element = commentApiWrapper.$.messagesList;
- updateStateComments(comments);
+ await element.commentsModel.reloadComments();
element.messages = messages;
- flush();
+ await flush();
});
test('expand/collapse all', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index dca0d65..52a5dac 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -218,7 +218,7 @@
private readonly reporting = getAppContext().reportingService;
- private readonly changeService = getAppContext().changeService;
+ private readonly changeModel = getAppContext().changeModel;
@property({type: Object})
change?: ChangeInfo;
@@ -435,7 +435,7 @@
open(focusTarget?: FocusTarget, quote?: string) {
assertIsDefined(this.change, 'change');
this.knownLatestState = LatestPatchState.CHECKING;
- this.changeService.fetchChangeUpdates(this.change).then(result => {
+ this.changeModel.fetchChangeUpdates(this.change).then(result => {
this.knownLatestState = result.isLatest
? LatestPatchState.LATEST
: LatestPatchState.NOT_LATEST;
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index 84a5fa7..490162b 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -43,7 +43,6 @@
import {customElement, property, queryAll, state} from 'lit/decorators';
import {sharedStyles} from '../../../styles/shared-styles';
import {subscribe} from '../../lit/subscription-controller';
-import {change$, changeNum$} from '../../../services/change/change-model';
import {ParsedChangeInfo} from '../../../types/types';
import {repeat} from 'lit/directives/repeat';
import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread';
@@ -200,12 +199,14 @@
@state()
draftsOnly = false;
+ private readonly changeModel = getAppContext().changeModel;
+
private readonly userModel = getAppContext().userModel;
constructor() {
super();
- subscribe(this, changeNum$, x => (this.changeNum = x));
- subscribe(this, change$, x => (this.change = x));
+ subscribe(this, this.changeModel.changeNum$, x => (this.changeNum = x));
+ subscribe(this, this.changeModel.change$, x => (this.change = x));
subscribe(this, this.userModel.account$, x => (this.account = x));
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 24c06e0..de2099e 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -55,7 +55,6 @@
LabelNameToInfoMap,
PatchSetNumber,
} from '../../types/common';
-import {labels$, latestPatchNum$} from '../../services/change/change-model';
import {getAppContext} from '../../services/app-context';
import {spinnerStyles} from '../../styles/gr-spinner-styles';
import {
@@ -88,11 +87,13 @@
@state()
labels?: LabelNameToInfoMap;
+ private changeModel = getAppContext().changeModel;
+
private checksModel = getAppContext().checksModel;
constructor() {
super();
- subscribe(this, labels$, x => (this.labels = x));
+ subscribe(this, this.changeModel.labels$, x => (this.labels = x));
}
static override get styles() {
@@ -530,7 +531,7 @@
@state()
repoConfig?: ConfigInfo;
- private changeService = getAppContext().changeService;
+ private changeModel = getAppContext().changeModel;
private configModel = getAppContext().configModel;
@@ -618,7 +619,7 @@
const end = pointer?.range?.end_line;
if (start) rangeText += `#${start}`;
if (end && start !== end) rangeText += `-${end}`;
- const change = this.changeService.getChange();
+ const change = this.changeModel.getChange();
assertIsDefined(change);
const path = pointer.path;
const patchset = this.result?.patchset as PatchSetNumber | undefined;
@@ -726,6 +727,8 @@
*/
private isSectionExpandedByUser = new Map<Category, boolean>();
+ private readonly changeModel = getAppContext().changeModel;
+
private readonly checksModel = getAppContext().checksModel;
constructor() {
@@ -745,7 +748,11 @@
this.checksModel.checksSelectedPatchsetNumber$,
x => (this.checksPatchsetNumber = x)
);
- subscribe(this, latestPatchNum$, x => (this.latestPatchsetNumber = x));
+ subscribe(
+ this,
+ this.changeModel.latestPatchNum$,
+ x => (this.latestPatchsetNumber = x)
+ );
subscribe(
this,
this.checksModel.someProvidersAreLoadingSelected$,
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
index 16c9bfe..a9c30c5 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
@@ -20,7 +20,6 @@
import {CheckResult, CheckRun} from '../../services/checks/checks-model';
import './gr-checks-runs';
import './gr-checks-results';
-import {changeNum$, latestPatchNum$} from '../../services/change/change-model';
import {NumericChangeId, PatchSetNumber} from '../../types/common';
import {ActionTriggeredEvent} from '../../services/checks/checks-util';
import {AttemptSelectedEvent, RunSelectedEvent} from './gr-checks-util';
@@ -62,6 +61,8 @@
number | undefined
>();
+ private readonly changeModel = getAppContext().changeModel;
+
private readonly checksModel = getAppContext().checksModel;
constructor() {
@@ -81,8 +82,12 @@
this.checksModel.checksSelectedPatchsetNumber$,
x => (this.checksPatchsetNumber = x)
);
- subscribe(this, latestPatchNum$, x => (this.latestPatchsetNumber = x));
- subscribe(this, changeNum$, x => (this.changeNum = x));
+ subscribe(
+ this,
+ this.changeModel.latestPatchNum$,
+ x => (this.latestPatchsetNumber = x)
+ );
+ subscribe(this, this.changeModel.changeNum$, x => (this.changeNum = x));
this.addEventListener('action-triggered', (e: ActionTriggeredEvent) =>
this.handleActionTriggered(e.detail.action, e.detail.run)
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 9c62d1a..25a1a00 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -85,7 +85,6 @@
import {DiffContextExpandedEventDetail} from '../gr-diff-builder/gr-diff-builder';
import {TokenHighlightLayer} from '../gr-diff-builder/token-highlight-layer';
import {Timing} from '../../../constants/reporting';
-import {changeComments$} from '../../../services/comments/comments-model';
import {ChangeComments} from '../gr-comment-api/gr-comment-api';
import {Subscription} from 'rxjs';
import {DisplayLine, RenderPreferences} from '../../../api/diff';
@@ -268,6 +267,8 @@
private readonly browserModel = getAppContext().browserModel;
+ private readonly commentsModel = getAppContext().commentsModel;
+
private readonly reporting = getAppContext().reportingService;
private readonly flags = getAppContext().flagsService;
@@ -320,7 +321,7 @@
this._loggedIn = loggedIn;
});
this.subscriptions.push(
- changeComments$.subscribe(changeComments => {
+ this.commentsModel.changeComments$.subscribe(changeComments => {
this.changeComments = changeComments;
})
);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 50d6b24..e527134 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -113,20 +113,10 @@
import {addGlobalShortcut, Key, toggleClass} from '../../../utils/dom-util';
import {CursorMoveResult} from '../../../api/core';
import {isFalse, throttleWrap, until} from '../../../utils/async-util';
-import {
- changeComments$,
- commentsLoading$,
-} from '../../../services/comments/comments-model';
import {filter, take} from 'rxjs/operators';
import {Subscription, combineLatest} from 'rxjs';
import {listen} from '../../../services/shortcuts/shortcuts-service';
-import {
- diffPath$,
- currentPatchNum$,
- change$,
- changeLoadingStatus$,
- LoadingStatus,
-} from '../../../services/change/change-model';
+import {LoadingStatus} from '../../../services/change/change-model';
import {DisplayLine} from '../../../api/diff';
import {GrDownloadDialog} from '../../change/gr-download-dialog/gr-download-dialog';
@@ -368,14 +358,14 @@
// Private but used in tests.
readonly userModel = getAppContext().userModel;
- private readonly changeService = getAppContext().changeService;
+ // Private but used in tests.
+ readonly changeModel = getAppContext().changeModel;
// Private but used in tests.
readonly browserModel = getAppContext().browserModel;
- // We just want to make sure that CommentsService is instantiated.
- // Otherwise subscribing to the model won't emit any data.
- private readonly _commentsService = getAppContext().commentsService;
+ // Private but used in tests.
+ readonly commentsModel = getAppContext().commentsModel;
private readonly shortcuts = getAppContext().shortcutsService;
@@ -387,11 +377,6 @@
private subscriptions: Subscription[] = [];
- constructor() {
- super();
- this._commentsService;
- }
-
override connectedCallback() {
super.connectedCallback();
this._throttledToggleFileReviewed = throttleWrap(_ =>
@@ -405,7 +390,7 @@
});
this.subscriptions.push(
- changeComments$.subscribe(changeComments => {
+ this.commentsModel.changeComments$.subscribe(changeComments => {
this._changeComments = changeComments;
})
);
@@ -421,7 +406,7 @@
})
);
this.subscriptions.push(
- change$.subscribe(change => {
+ this.changeModel.change$.subscribe(change => {
// The diff view is tied to a specfic change number, so don't update
// _change to undefined.
if (change) this._change = change;
@@ -433,12 +418,12 @@
// properties since the method will be called anytime a property updates
// but we only want to call this on the initial load.
this.subscriptions.push(
- combineLatest(
- currentPatchNum$,
+ combineLatest([
+ this.changeModel.currentPatchNum$,
routerView$,
- diffPath$,
- this.userModel.diffPreferences$
- )
+ this.changeModel.diffPath$,
+ this.userModel.diffPreferences$,
+ ])
.pipe(
filter(
([currentPatchNum, routerView, path, diffPrefs]) =>
@@ -453,7 +438,9 @@
this.setReviewedStatus(currentPatchNum!, path!, diffPrefs);
})
);
- this.subscriptions.push(diffPath$.subscribe(path => (this._path = path)));
+ this.subscriptions.push(
+ this.changeModel.diffPath$.subscribe(path => (this._path = path))
+ );
this.addEventListener('open-fix-preview', e => this._onOpenFixPreview(e));
this.cursor.replaceDiffs([this.$.diffHost]);
this._onRenderHandler = (_: Event) => {
@@ -1069,7 +1056,7 @@
GerritNav.navigateToChange(this._change);
return;
}
- this.changeService.updatePath(comment.path);
+ this.changeModel.updatePath(comment.path);
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!latestPatchNum) throw new Error('Missing _allPatchSets');
@@ -1079,7 +1066,7 @@
this._focusLineNum = comment.line;
} else {
if (this.params.path) {
- this.changeService.updatePath(this.params.path);
+ this.changeModel.updatePath(this.params.path);
}
if (this.params.patchNum) {
this._patchRange = {
@@ -1145,7 +1132,7 @@
}
this._files = {sortedFileList: [], changeFilesByPath: {}};
- this.changeService.updatePath(undefined);
+ this.changeModel.updatePath(undefined);
this._patchRange = undefined;
this._commitRange = undefined;
this._focusLineNum = undefined;
@@ -1169,11 +1156,15 @@
}
const promises: Promise<unknown>[] = [];
- if (!this._change)
+ if (!this._change) {
promises.push(
- until(changeLoadingStatus$, status => status === LoadingStatus.LOADED)
+ until(
+ this.changeModel.changeLoadingStatus$,
+ status => status === LoadingStatus.LOADED
+ )
);
- promises.push(until(commentsLoading$, isFalse));
+ }
+ promises.push(until(this.commentsModel.commentsLoading$, isFalse));
promises.push(
this._getChangeEdit().then(edit => {
if (edit) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index d47a03e..4c4361d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -31,8 +31,6 @@
import {EditPatchSetNum} from '../../../types/common.js';
import {CursorMoveResult} from '../../../api/core.js';
import {Side} from '../../../api/diff.js';
-import {_testOnly_setState as setChangeModelState} from '../../../services/change/change-model.js';
-import {_testOnly_setState as setCommentState} from '../../../services/comments/comments-model.js';
const basicFixture = fixtureFromElement('gr-diff-view');
@@ -93,7 +91,7 @@
]});
await flush();
- setCommentState({
+ element.commentsModel.setState({
comments: {},
robotComments: {},
drafts: {},
@@ -139,14 +137,15 @@
sinon.stub(element.reporting, 'diffViewDisplayed');
sinon.stub(element.$.diffHost, 'reload').returns(Promise.resolve());
sinon.spy(element, '_paramsChanged');
- setChangeModelState({change: {
- ...createChange(),
- revisions: createRevisions(11),
- }});
+ element.changeModel.setState({
+ change: {
+ ...createChange(),
+ revisions: createRevisions(11),
+ }});
});
test('comment url resolves to comment.patch_set vs latest', () => {
- setCommentState({
+ element.commentsModel.setState({
comments: {
'/COMMIT_MSG': [
{
@@ -220,7 +219,7 @@
test('unchanged diff X vs latest from comment links navigates to base vs X'
, () => {
const diffNavStub = sinon.stub(GerritNav, 'navigateToDiff');
- setCommentState({
+ element.commentsModel.setState({
comments: {
'/COMMIT_MSG': [
{
@@ -248,10 +247,11 @@
sinon.stub(element.$.diffHost, 'reload').returns(Promise.resolve());
sinon.stub(element, '_isFileUnchanged').returns(true);
sinon.spy(element, '_paramsChanged');
- setChangeModelState({change: {
- ...createChange(),
- revisions: createRevisions(11),
- }});
+ element.changeModel.setState({
+ change: {
+ ...createChange(),
+ revisions: createRevisions(11),
+ }});
element.params = {
view: GerritNav.View.DIFF,
changeNum: '42',
@@ -272,7 +272,7 @@
test('unchanged diff Base vs latest from comment does not navigate'
, () => {
const diffNavStub = sinon.stub(GerritNav, 'navigateToDiff');
- setCommentState({
+ element.commentsModel.setState({
comments: {
'/COMMIT_MSG': [
{
@@ -300,10 +300,11 @@
sinon.stub(element.$.diffHost, 'reload').returns(Promise.resolve());
sinon.stub(element, '_isFileUnchanged').returns(true);
sinon.spy(element, '_paramsChanged');
- setChangeModelState({change: {
- ...createChange(),
- revisions: createRevisions(11),
- }});
+ element.changeModel.setState({
+ change: {
+ ...createChange(),
+ revisions: createRevisions(11),
+ }});
element.params = {
view: GerritNav.View.DIFF,
changeNum: '42',
@@ -352,7 +353,7 @@
});
test('diff toast to go to latest is shown and not base', async () => {
- setCommentState({
+ element.commentsModel.setState({
comments: {
'/COMMIT_MSG': [
{
@@ -381,10 +382,11 @@
sinon.stub(element.$.diffHost, 'reload').returns(Promise.resolve());
sinon.spy(element, '_paramsChanged');
element._change = undefined;
- setChangeModelState({change: {
- ...createChange(),
- revisions: createRevisions(11),
- }});
+ element.changeModel.setState({
+ change: {
+ ...createChange(),
+ revisions: createRevisions(11),
+ }});
element._patchRange = {
patchNum: 2,
basePatchNum: 1,
@@ -1200,7 +1202,9 @@
manual_review: true,
};
element.userModel.setDiffPreferences(diffPreferences);
- setChangeModelState({change: createChange(), diffPath: '/COMMIT_MSG'});
+ element.changeModel.setState({
+ change: createChange(),
+ diffPath: '/COMMIT_MSG'});
setRouterModelState({
changeNum: TEST_NUMERIC_CHANGE_ID, view: GerritView.DIFF, patchNum: 2}
@@ -1237,7 +1241,8 @@
manual_review: false,
};
element.userModel.setDiffPreferences(diffPreferences);
- setChangeModelState({change: createChange(),
+ element.changeModel.setState({
+ change: createChange(),
diffPath: '/COMMIT_MSG'});
setRouterModelState({
@@ -1262,7 +1267,9 @@
sinon.stub(element.$.diffHost, 'reload');
element.userModel.setDiffPreferences(createDefaultDiffPrefs());
- setChangeModelState({change: createChange(), diffPath: '/COMMIT_MSG'});
+ element.changeModel.setState({
+ change: createChange(),
+ diffPath: '/COMMIT_MSG'});
setRouterModelState({
changeNum: TEST_NUMERIC_CHANGE_ID, view: GerritView.DIFF, patchNum: 2}
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
index 3a2def0..71fee62 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -51,7 +51,6 @@
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, property, query, state} from 'lit/decorators';
import {subscribe} from '../../lit/subscription-controller';
-import {changeComments$} from '../../../services/comments/comments-model';
// Maximum length for patch set descriptions.
const PATCH_DESC_MAX_LENGTH = 500;
@@ -127,9 +126,15 @@
private readonly reporting: ReportingService =
getAppContext().reportingService;
+ private readonly commentsModel = getAppContext().commentsModel;
+
constructor() {
super();
- subscribe(this, changeComments$, x => (this.changeComments = x));
+ subscribe(
+ this,
+ this.commentsModel.changeComments$,
+ x => (this.changeComments = x)
+ );
}
static override get styles() {
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.ts b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.ts
index 8b47437..0a9fbbf 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.ts
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.ts
@@ -24,6 +24,7 @@
import {ServerInfo} from '../../../types/common';
import {getAppContext} from '../../../services/app-context';
import {columnNames} from '../../change-list/gr-change-list/gr-change-list';
+import {KnownExperimentId} from '../../../services/flags/flags';
@customElement('gr-change-table-editor')
export class GrChangeTableEditor extends PolymerElement {
@@ -48,24 +49,25 @@
@observe('serverConfig')
_configChanged(config: ServerInfo) {
this.defaultColumns = columnNames.filter(col =>
- this._isColumnEnabled(col, config, this.flagsService.enabledExperiments)
+ this._isColumnEnabled(col, config)
);
if (!this.displayedColumns) return;
this.displayedColumns = this.displayedColumns.filter(column =>
- this._isColumnEnabled(
- column,
- config,
- this.flagsService.enabledExperiments
- )
+ this._isColumnEnabled(column, config)
);
}
/**
* Is the column disabled by a server config or experiment?
*/
- _isColumnEnabled(column: string, config: ServerInfo, experiments: string[]) {
+ _isColumnEnabled(column: string, config: ServerInfo) {
if (!config || !config.change) return true;
- if (column === 'Comments') return experiments.includes('comments-column');
+ if (column === 'Comments')
+ return this.flagsService.isEnabled('comments-column');
+ if (column === 'Requirements')
+ return this.flagsService.isEnabled(
+ KnownExperimentId.SUBMIT_REQUIREMENTS_UI
+ );
return true;
}
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.ts b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.ts
index 3813213..c2bcec2 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.ts
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.ts
@@ -104,7 +104,7 @@
test('_getDisplayedColumns', () => {
const enabledColumns = columns.filter(column =>
- element._isColumnEnabled(column, element.serverConfig!, [])
+ element._isColumnEnabled(column, element.serverConfig!)
);
assert.deepEqual(element._getDisplayedColumns(), enabledColumns);
const input = queryAndAssert<HTMLInputElement>(
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 1e99b7f..de8a02d 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -67,7 +67,6 @@
import {subscribe} from '../../lit/subscription-controller';
import {repeat} from 'lit/directives/repeat';
import {classMap} from 'lit/directives/class-map';
-import {changeNum$, repo$} from '../../../services/change/change-model';
import {ShortcutController} from '../../lit/shortcut-controller';
import {ValueChangedEvent} from '../../../types/events';
@@ -236,7 +235,9 @@
@state()
saving = false;
- private readonly commentsService = getAppContext().commentsService;
+ private readonly commentsModel = getAppContext().commentsModel;
+
+ private readonly changeModel = getAppContext().changeModel;
private readonly userModel = getAppContext().userModel;
@@ -246,9 +247,9 @@
constructor() {
super();
- subscribe(this, changeNum$, x => (this.changeNum = x));
+ subscribe(this, this.changeModel.changeNum$, x => (this.changeNum = x));
subscribe(this, this.userModel.account$, x => (this.account = x));
- subscribe(this, repo$, x => (this.repoName = x));
+ subscribe(this, this.changeModel.repo$, x => (this.repoName = x));
subscribe(this, this.userModel.diffPreferences$, x =>
this.syntaxLayer.setEnabled(!!x.syntax_highlighting)
);
@@ -769,7 +770,7 @@
} else {
try {
this.saving = true;
- await this.commentsService.saveDraft(unsaved);
+ await this.commentsModel.saveDraft(unsaved);
} finally {
this.saving = false;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index aa5a328..1411c88 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -58,7 +58,6 @@
import {subscribe} from '../../lit/subscription-controller';
import {ShortcutController} from '../../lit/shortcut-controller';
import {classMap} from 'lit/directives/class-map';
-import {changeNum$, repo$} from '../../../services/change/change-model';
import {LineNumber} from '../../../api/diff';
import {CommentSide} from '../../../constants/constants';
import {getRandomInt} from '../../../utils/math-util';
@@ -228,7 +227,9 @@
private readonly reporting = getAppContext().reportingService;
- private readonly commentsService = getAppContext().commentsService;
+ private readonly changeModel = getAppContext().changeModel;
+
+ private readonly commentsModel = getAppContext().commentsModel;
private readonly userModel = getAppContext().userModel;
@@ -263,8 +264,8 @@
this.configModel.repoCommentLinks$,
x => (this.commentLinks = x)
);
- subscribe(this, repo$, x => (this.repoName = x));
- subscribe(this, changeNum$, x => (this.changeNum = x));
+ subscribe(this, this.changeModel.repo$, x => (this.repoName = x));
+ subscribe(this, this.changeModel.changeNum$, x => (this.changeNum = x));
subscribe(
this,
this.autoSaveTrigger$.pipe(debounceTime(AUTO_SAVE_DEBOUNCE_DELAY_MS)),
@@ -1085,7 +1086,7 @@
if (messageToSave === '') {
// Don't try to discard UnsavedInfo. Nothing to do then.
if (this.comment.id) {
- await this.commentsService.discardDraft(this.comment.id);
+ await this.commentsModel.discardDraft(this.comment.id);
}
} else {
// No need to make a backend call when nothing has changed.
@@ -1108,7 +1109,7 @@
/** For sharing between save() and autoSave(). */
private rawSave(message: string, options: {showToast: boolean}) {
if (!isDraftOrUnsaved(this.comment)) throw new Error('not a draft');
- return this.commentsService.saveDraft(
+ return this.commentsModel.saveDraft(
{
...this.comment,
message,
@@ -1126,7 +1127,7 @@
private async openDeleteCommentOverlay() {
this.showConfirmDeleteOverlay = true;
await this.updateComplete;
- this.confirmDeleteOverlay?.open();
+ await this.confirmDeleteOverlay?.open();
}
private closeDeleteCommentOverlay() {
diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
index 888f477..70de130 100644
--- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
+++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
@@ -44,6 +44,7 @@
getVotingRangeOrDefault,
hasNeutralStatus,
hasVoted,
+ showNewSubmitRequirements,
valueString,
} from '../../../utils/label-util';
import {getAppContext} from '../../../services/app-context';
@@ -53,7 +54,6 @@
import {votingStyles} from '../../../styles/gr-voting-styles';
import {ifDefined} from 'lit/directives/if-defined';
import {fireReload} from '../../../utils/event-util';
-import {KnownExperimentId} from '../../../services/flags/flags';
import {sortReviewers} from '../../../utils/attention-set-util';
declare global {
@@ -104,10 +104,6 @@
@property({type: Boolean})
showAllReviewers = true;
- /** temporary until submit requirements are finished */
- @property({type: Boolean})
- showAlwaysOldUI = false;
-
private readonly restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -214,10 +210,7 @@
}
override render() {
- if (
- this.flagsService.isEnabled(KnownExperimentId.SUBMIT_REQUIREMENTS_UI) &&
- !this.showAlwaysOldUI
- ) {
+ if (showNewSubmitRequirements(this.flagsService, this.change)) {
return this.renderNewSubmitRequirements();
} else {
return this.renderOldSubmitRequirements();
diff --git a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
index 44db793..9b3e75d 100644
--- a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
+++ b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
@@ -75,11 +75,11 @@
restApiService: (_ctx: Partial<AppContext>) => {
throw new Error('restApiService is not implemented');
},
- changeService: (_ctx: Partial<AppContext>) => {
- throw new Error('changeService is not implemented');
+ changeModel: (_ctx: Partial<AppContext>) => {
+ throw new Error('changeModel is not implemented');
},
- commentsService: (_ctx: Partial<AppContext>) => {
- throw new Error('commentsService is not implemented');
+ commentsModel: (_ctx: Partial<AppContext>) => {
+ throw new Error('commentsModel is not implemented');
},
checksModel: (_ctx: Partial<AppContext>) => {
throw new Error('checksModel is not implemented');
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index c43ed4e..46d2178 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -21,12 +21,12 @@
import {EventEmitter} from './gr-event-interface/gr-event-interface_impl';
import {Auth} from './gr-auth/gr-auth_impl';
import {GrRestApiServiceImpl} from '../elements/shared/gr-rest-api-interface/gr-rest-api-impl';
-import {ChangeService} from './change/change-service';
+import {ChangeModel} from './change/change-model';
import {ChecksModel} from './checks/checks-model';
import {GrJsApiInterface} from '../elements/shared/gr-js-api-interface/gr-js-api-interface-element';
import {GrStorageService} from './storage/gr-storage_impl';
import {UserModel} from './user/user-model';
-import {CommentsService} from './comments/comments-service';
+import {CommentsModel} from './comments/comments-model';
import {ShortcutsService} from './shortcuts/shortcuts-service';
import {BrowserModel} from './browser/browser-model';
import {assertIsDefined} from '../utils/common-util';
@@ -53,20 +53,25 @@
assertIsDefined(ctx.flagsService, 'flagsService)');
return new GrRestApiServiceImpl(ctx.authService!, ctx.flagsService!);
},
- changeService: (ctx: Partial<AppContext>) => {
+ changeModel: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.restApiService, 'restApiService');
- return new ChangeService(ctx.restApiService!);
+ return new ChangeModel(ctx.restApiService!);
},
- commentsService: (ctx: Partial<AppContext>) => {
- const restApi = ctx.restApiService;
+ commentsModel: (ctx: Partial<AppContext>) => {
+ const changeModel = ctx.changeModel;
+ const restApiService = ctx.restApiService;
const reporting = ctx.reportingService;
- assertIsDefined(restApi, 'restApiService');
+ assertIsDefined(changeModel, 'changeModel');
+ assertIsDefined(restApiService, 'restApiService');
assertIsDefined(reporting, 'reportingService');
- return new CommentsService(restApi, reporting);
+ return new CommentsModel(changeModel, restApiService, reporting);
},
checksModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.reportingService, 'reportingService');
- return new ChecksModel(ctx.reportingService!);
+ const changeModel = ctx.changeModel;
+ const reporting = ctx.reportingService;
+ assertIsDefined(changeModel, 'changeModel');
+ assertIsDefined(reporting, 'reportingService');
+ return new ChecksModel(changeModel, reporting);
},
jsApiService: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.reportingService, 'reportingService');
@@ -74,8 +79,11 @@
},
storageService: (_ctx: Partial<AppContext>) => new GrStorageService(),
configModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.restApiService, 'restApiService');
- return new ConfigModel(ctx.restApiService!);
+ const changeModel = ctx.changeModel;
+ const restApiService = ctx.restApiService;
+ assertIsDefined(changeModel, 'changeModel');
+ assertIsDefined(restApiService, 'restApiService');
+ return new ConfigModel(changeModel, restApiService);
},
userModel: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.restApiService, 'restApiService');
diff --git a/polygerrit-ui/app/services/app-context.ts b/polygerrit-ui/app/services/app-context.ts
index 4488b12..bea371f 100644
--- a/polygerrit-ui/app/services/app-context.ts
+++ b/polygerrit-ui/app/services/app-context.ts
@@ -20,12 +20,12 @@
import {ReportingService} from './gr-reporting/gr-reporting';
import {AuthService} from './gr-auth/gr-auth';
import {RestApiService} from './gr-rest-api/gr-rest-api';
-import {ChangeService} from './change/change-service';
+import {ChangeModel} from './change/change-model';
import {ChecksModel} from './checks/checks-model';
import {JsApiService} from '../elements/shared/gr-js-api-interface/gr-js-api-types';
import {StorageService} from './storage/gr-storage';
import {UserModel} from './user/user-model';
-import {CommentsService} from './comments/comments-service';
+import {CommentsModel} from './comments/comments-model';
import {ShortcutsService} from './shortcuts/shortcuts-service';
import {BrowserModel} from './browser/browser-model';
import {ConfigModel} from './config/config-model';
@@ -36,8 +36,8 @@
eventEmitter: EventEmitterService;
authService: AuthService;
restApiService: RestApiService;
- changeService: ChangeService;
- commentsService: CommentsService;
+ changeModel: ChangeModel;
+ commentsModel: CommentsModel;
checksModel: ChecksModel;
jsApiService: JsApiService;
storageService: StorageService;
diff --git a/polygerrit-ui/app/services/change/change-model.ts b/polygerrit-ui/app/services/change/change-model.ts
index 2c98320..6a819f8 100644
--- a/polygerrit-ui/app/services/change/change-model.ts
+++ b/polygerrit-ui/app/services/change/change-model.ts
@@ -16,12 +16,21 @@
*/
import {NumericChangeId, PatchSetNum} from '../../types/common';
-import {BehaviorSubject, combineLatest, Observable} from 'rxjs';
+import {
+ combineLatest,
+ from,
+ fromEvent,
+ BehaviorSubject,
+ Observable,
+ Subscription,
+} from 'rxjs';
import {
map,
filter,
withLatestFrom,
distinctUntilChanged,
+ startWith,
+ switchMap,
} from 'rxjs/operators';
import {routerPatchNum$, routerState$} from '../router/router-model';
import {
@@ -30,6 +39,12 @@
} from '../../utils/patch-set-util';
import {ParsedChangeInfo} from '../../types/types';
+import {routerChangeNum$} from '../router/router-model';
+import {ChangeInfo} from '../../types/common';
+import {RestApiService} from '../gr-rest-api/gr-rest-api';
+import {Finalizable} from '../registry';
+import {select} from '../../utils/observable-util';
+
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
LOADING = 'LOADING',
@@ -58,125 +73,189 @@
loadingStatus: LoadingStatus.NOT_LOADED,
};
-const privateState$ = new BehaviorSubject(initialState);
+export class ChangeModel implements Finalizable {
+ private readonly privateState$ = new BehaviorSubject(initialState);
-export function _testOnly_resetState() {
- // We cannot assign a new subject to privateState$, because all the selectors
- // have already subscribed to the original subject. So we have to emit the
- // initial state on the existing subject.
- privateState$.next({...initialState});
-}
+ public readonly changeState$: Observable<ChangeState> =
+ this.privateState$.asObservable();
-export function _testOnly_setState(state: ChangeState) {
- privateState$.next(state);
-}
-
-export function _testOnly_getState() {
- return privateState$.getValue();
-}
-
-// Re-exporting as Observable so that you can only subscribe, but not emit.
-export const changeState$: Observable<ChangeState> = privateState$;
-
-export function updateStateChange(change?: ParsedChangeInfo) {
- const current = privateState$.getValue();
- privateState$.next({
- ...current,
- change,
- loadingStatus:
- change === undefined ? LoadingStatus.NOT_LOADED : LoadingStatus.LOADED,
- });
-}
-
-/**
- * Called when change detail loading is initiated.
- *
- * If the change number matches the current change in the state, then
- * this is a reload. If not, then we not just want to set the state to
- * LOADING instead of RELOADING, but we also want to set the change to
- * undefined right away. Otherwise components could see inconsistent state:
- * a new change number, but an old change.
- */
-export function updateStateLoading(changeNum: NumericChangeId) {
- const current = privateState$.getValue();
- const reloading = current.change?._number === changeNum;
- privateState$.next({
- ...current,
- change: reloading ? current.change : undefined,
- loadingStatus: reloading ? LoadingStatus.RELOADING : LoadingStatus.LOADING,
- });
-}
-
-export function updateStatePath(diffPath?: string) {
- const current = privateState$.getValue();
- privateState$.next({...current, diffPath});
-}
-
-/**
- * If you depend on both, router and change state, then you want to filter out
- * inconsistent state, e.g. router changeNum already updated, change not yet
- * reset to undefined.
- */
-export const changeAndRouterConsistent$ = combineLatest([
- routerState$,
- changeState$,
-]).pipe(
- filter(([routerState, changeState]) => {
- const changeNum = changeState.change?._number;
- const routerChangeNum = routerState.changeNum;
- return changeNum === undefined || changeNum === routerChangeNum;
- }),
- distinctUntilChanged()
-);
-
-export const change$ = changeState$.pipe(
- map(changeState => changeState.change),
- distinctUntilChanged()
-);
-
-export const changeLoadingStatus$ = changeState$.pipe(
- map(changeState => changeState.loadingStatus),
- distinctUntilChanged()
-);
-
-export const diffPath$ = changeState$.pipe(
- map(changeState => changeState?.diffPath),
- distinctUntilChanged()
-);
-
-export const changeNum$ = change$.pipe(
- map(change => change?._number),
- distinctUntilChanged()
-);
-
-export const repo$ = change$.pipe(
- map(change => change?.project),
- distinctUntilChanged()
-);
-
-export const labels$ = change$.pipe(
- map(change => change?.labels),
- distinctUntilChanged()
-);
-
-export const latestPatchNum$ = change$.pipe(
- map(change => computeLatestPatchNum(computeAllPatchSets(change))),
- distinctUntilChanged()
-);
-
-/**
- * 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
- * returns the number of the latest patchset.
- *
- * Note that this selector can emit a patchNum without the change being
- * available!
- */
-export const currentPatchNum$: Observable<PatchSetNum | undefined> =
- changeAndRouterConsistent$.pipe(
- withLatestFrom(routerPatchNum$, latestPatchNum$),
- map(
- ([_, routerPatchNum, latestPatchNum]) => routerPatchNum || latestPatchNum
- ),
- distinctUntilChanged()
+ public readonly change$ = select(
+ this.privateState$,
+ changeState => changeState.change
);
+
+ public readonly changeLoadingStatus$ = select(
+ this.privateState$,
+ changeState => changeState.loadingStatus
+ );
+
+ public readonly diffPath$ = select(
+ this.privateState$,
+ changeState => changeState?.diffPath
+ );
+
+ public readonly changeNum$ = select(this.change$, change => change?._number);
+
+ public readonly repo$ = select(this.change$, change => change?.project);
+
+ public readonly labels$ = select(this.change$, change => change?.labels);
+
+ public readonly latestPatchNum$ = select(this.change$, change =>
+ computeLatestPatchNum(computeAllPatchSets(change))
+ );
+
+ /**
+ * 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
+ * returns the number of the latest patchset.
+ *
+ * Note that this selector can emit a patchNum without the change being
+ * available!
+ */
+ public readonly currentPatchNum$: Observable<PatchSetNum | undefined> =
+ /**
+ * If you depend on both, router and change state, then you want to filter
+ * out inconsistent state, e.g. router changeNum already updated, change not
+ * yet reset to undefined.
+ */
+ combineLatest([routerState$, this.changeState$])
+ .pipe(
+ filter(([routerState, changeState]) => {
+ const changeNum = changeState.change?._number;
+ const routerChangeNum = routerState.changeNum;
+ return changeNum === undefined || changeNum === routerChangeNum;
+ }),
+ distinctUntilChanged()
+ )
+ .pipe(
+ withLatestFrom(routerPatchNum$, this.latestPatchNum$),
+ map(([_, routerPatchN, latestPatchN]) => routerPatchN || latestPatchN),
+ distinctUntilChanged()
+ );
+
+ private subscriptions: Subscription[] = [];
+
+ // For usage in `combineLatest` we need `startWith` such that reload$ has an
+ // initial value.
+ private readonly reload$: Observable<unknown> = fromEvent(
+ document,
+ 'reload'
+ ).pipe(startWith(undefined));
+
+ constructor(readonly restApiService: RestApiService) {
+ this.subscriptions = [
+ combineLatest([routerChangeNum$, this.reload$])
+ .pipe(
+ map(([changeNum, _]) => changeNum),
+ switchMap(changeNum => {
+ if (changeNum !== undefined) this.updateStateLoading(changeNum);
+ return from(this.restApiService.getChangeDetail(changeNum));
+ })
+ )
+ .subscribe(change => {
+ // The change service is currently a singleton, so we have to be
+ // careful to avoid situations where the application state is
+ // partially set for the old change where the user is coming from,
+ // and partially for the new change where the user is navigating to.
+ // So setting the change explicitly to undefined when the user
+ // moves away from diff and change pages (changeNum === undefined)
+ // helps with that.
+ this.updateStateChange(change ?? undefined);
+ }),
+ ];
+ }
+
+ finalize() {
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
+ }
+
+ // Temporary workaround until path is derived in the model itself.
+ updatePath(diffPath?: string) {
+ const current = this.getState();
+ this.setState({...current, diffPath});
+ }
+
+ /**
+ * Typically you would just subscribe to change$ yourself to get updates. But
+ * sometimes it is nice to also be able to get the current ChangeInfo on
+ * demand. So here it is for your convenience.
+ */
+ getChange() {
+ return this.getState().change;
+ }
+
+ /**
+ * Check whether there is no newer patch than the latest patch that was
+ * available when this change was loaded.
+ *
+ * @return A promise that yields true if the latest patch
+ * has been loaded, and false if a newer patch has been uploaded in the
+ * meantime. The promise is rejected on network error.
+ */
+ fetchChangeUpdates(change: ChangeInfo | ParsedChangeInfo) {
+ const knownLatest = computeLatestPatchNum(computeAllPatchSets(change));
+ return this.restApiService.getChangeDetail(change._number).then(detail => {
+ if (!detail) {
+ const error = new Error('Change detail not found.');
+ return Promise.reject(error);
+ }
+ const actualLatest = computeLatestPatchNum(computeAllPatchSets(detail));
+ if (!actualLatest || !knownLatest) {
+ const error = new Error('Unable to check for latest patchset.');
+ return Promise.reject(error);
+ }
+ return {
+ isLatest: actualLatest <= knownLatest,
+ newStatus: change.status !== detail.status ? detail.status : null,
+ newMessages:
+ (change.messages || []).length < (detail.messages || []).length
+ ? detail.messages![detail.messages!.length - 1]
+ : undefined,
+ };
+ });
+ }
+
+ /**
+ * Called when change detail loading is initiated.
+ *
+ * If the change number matches the current change in the state, then
+ * this is a reload. If not, then we not just want to set the state to
+ * LOADING instead of RELOADING, but we also want to set the change to
+ * undefined right away. Otherwise components could see inconsistent state:
+ * a new change number, but an old change.
+ */
+ private updateStateLoading(changeNum: NumericChangeId) {
+ const current = this.getState();
+ const reloading = current.change?._number === changeNum;
+ this.setState({
+ ...current,
+ change: reloading ? current.change : undefined,
+ loadingStatus: reloading
+ ? LoadingStatus.RELOADING
+ : LoadingStatus.LOADING,
+ });
+ }
+
+ // Private but used in tests.
+ updateStateChange(change?: ParsedChangeInfo) {
+ const current = this.getState();
+ this.setState({
+ ...current,
+ change,
+ loadingStatus:
+ change === undefined ? LoadingStatus.NOT_LOADED : LoadingStatus.LOADED,
+ });
+ }
+
+ getState(): ChangeState {
+ return this.privateState$.getValue();
+ }
+
+ // Private but used in tests
+ setState(state: ChangeState) {
+ this.privateState$.next(state);
+ }
+}
diff --git a/polygerrit-ui/app/services/change/change-services_test.ts b/polygerrit-ui/app/services/change/change-model_test.ts
similarity index 86%
rename from polygerrit-ui/app/services/change/change-services_test.ts
rename to polygerrit-ui/app/services/change/change-model_test.ts
index d05df85a..099a11f 100644
--- a/polygerrit-ui/app/services/change/change-services_test.ts
+++ b/polygerrit-ui/app/services/change/change-model_test.ts
@@ -32,15 +32,15 @@
GerritView,
_testOnly_setState as setRouterState,
} from '../router/router-model';
-import {ChangeState, changeState$, LoadingStatus} from './change-model';
-import {ChangeService} from './change-service';
+import {ChangeState, LoadingStatus} from './change-model';
+import {ChangeModel} from './change-model';
suite('change service tests', () => {
- let changeService: ChangeService;
+ let changeModel: ChangeModel;
let knownChange: ParsedChangeInfo;
const testCompleted = new Subject<void>();
setup(() => {
- changeService = new ChangeService(getAppContext().restApiService);
+ changeModel = new ChangeModel(getAppContext().restApiService);
knownChange = {
...createChange(),
revisions: {
@@ -62,8 +62,8 @@
});
teardown(() => {
- changeService.finalize();
testCompleted.next();
+ changeModel.finalize();
});
test('load a change', async () => {
@@ -72,7 +72,9 @@
let state: ChangeState | undefined = {
loadingStatus: LoadingStatus.NOT_LOADED,
};
- changeState$.pipe(takeUntil(testCompleted)).subscribe(s => (state = s));
+ changeModel.changeState$
+ .pipe(takeUntil(testCompleted))
+ .subscribe(s => (state = s));
await waitUntil(() => state?.loadingStatus === LoadingStatus.NOT_LOADED);
assert.equal(stub.callCount, 0);
@@ -96,7 +98,9 @@
let state: ChangeState | undefined = {
loadingStatus: LoadingStatus.NOT_LOADED,
};
- changeState$.pipe(takeUntil(testCompleted)).subscribe(s => (state = s));
+ changeModel.changeState$
+ .pipe(takeUntil(testCompleted))
+ .subscribe(s => (state = s));
setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -120,7 +124,9 @@
let state: ChangeState | undefined = {
loadingStatus: LoadingStatus.NOT_LOADED,
};
- changeState$.pipe(takeUntil(testCompleted)).subscribe(s => (state = s));
+ changeModel.changeState$
+ .pipe(takeUntil(testCompleted))
+ .subscribe(s => (state = s));
setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -150,7 +156,9 @@
let state: ChangeState | undefined = {
loadingStatus: LoadingStatus.NOT_LOADED,
};
- changeState$.pipe(takeUntil(testCompleted)).subscribe(s => (state = s));
+ changeModel.changeState$
+ .pipe(takeUntil(testCompleted))
+ .subscribe(s => (state = s));
setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -174,15 +182,15 @@
assert.equal(state?.change, knownChange);
});
- test('changeService.fetchChangeUpdates on latest', async () => {
+ test('changeModel.fetchChangeUpdates on latest', async () => {
stubRestApi('getChangeDetail').returns(Promise.resolve(knownChange));
- const result = await changeService.fetchChangeUpdates(knownChange);
+ const result = await changeModel.fetchChangeUpdates(knownChange);
assert.isTrue(result.isLatest);
assert.isNotOk(result.newStatus);
assert.isNotOk(result.newMessages);
});
- test('changeService.fetchChangeUpdates not on latest', async () => {
+ test('changeModel.fetchChangeUpdates not on latest', async () => {
const actualChange = {
...knownChange,
revisions: {
@@ -195,31 +203,31 @@
},
};
stubRestApi('getChangeDetail').returns(Promise.resolve(actualChange));
- const result = await changeService.fetchChangeUpdates(knownChange);
+ const result = await changeModel.fetchChangeUpdates(knownChange);
assert.isFalse(result.isLatest);
assert.isNotOk(result.newStatus);
assert.isNotOk(result.newMessages);
});
- test('changeService.fetchChangeUpdates new status', async () => {
+ test('changeModel.fetchChangeUpdates new status', async () => {
const actualChange = {
...knownChange,
status: ChangeStatus.MERGED,
};
stubRestApi('getChangeDetail').returns(Promise.resolve(actualChange));
- const result = await changeService.fetchChangeUpdates(knownChange);
+ const result = await changeModel.fetchChangeUpdates(knownChange);
assert.isTrue(result.isLatest);
assert.equal(result.newStatus, ChangeStatus.MERGED);
assert.isNotOk(result.newMessages);
});
- test('changeService.fetchChangeUpdates new messages', async () => {
+ test('changeModel.fetchChangeUpdates new messages', async () => {
const actualChange = {
...knownChange,
messages: [{...createChangeMessageInfo(), message: 'blah blah'}],
};
stubRestApi('getChangeDetail').returns(Promise.resolve(actualChange));
- const result = await changeService.fetchChangeUpdates(knownChange);
+ const result = await changeModel.fetchChangeUpdates(knownChange);
assert.isTrue(result.isLatest);
assert.isNotOk(result.newStatus);
assert.deepEqual(result.newMessages, {
diff --git a/polygerrit-ui/app/services/change/change-service.ts b/polygerrit-ui/app/services/change/change-service.ts
deleted file mode 100644
index bcc62b2..0000000
--- a/polygerrit-ui/app/services/change/change-service.ts
+++ /dev/null
@@ -1,126 +0,0 @@
-/**
- * @license
- * Copyright (C) 2020 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.
- */
-import {combineLatest, from, fromEvent, Observable, Subscription} from 'rxjs';
-import {map, startWith, switchMap} from 'rxjs/operators';
-import {routerChangeNum$} from '../router/router-model';
-import {
- change$,
- updateStateChange,
- updateStateLoading,
- updateStatePath,
-} from './change-model';
-import {ParsedChangeInfo} from '../../types/types';
-import {ChangeInfo} from '../../types/common';
-import {
- computeAllPatchSets,
- computeLatestPatchNum,
-} from '../../utils/patch-set-util';
-import {RestApiService} from '../gr-rest-api/gr-rest-api';
-import {Finalizable} from '../registry';
-
-export class ChangeService implements Finalizable {
- private change?: ParsedChangeInfo;
-
- private readonly subscriptions: Subscription[] = [];
-
- // For usage in `combineLatest` we need `startWith` such that reload$ has an
- // initial value.
- private readonly reload$: Observable<unknown> = fromEvent(
- document,
- 'reload'
- ).pipe(startWith(undefined));
-
- constructor(readonly restApiService: RestApiService) {
- this.subscriptions.push(
- combineLatest([routerChangeNum$, this.reload$])
- .pipe(
- map(([changeNum, _]) => changeNum),
- switchMap(changeNum => {
- if (changeNum !== undefined) updateStateLoading(changeNum);
- return from(this.restApiService.getChangeDetail(changeNum));
- })
- )
- .subscribe(change => {
- // The change service is currently a singleton, so we have to be
- // careful to avoid situations where the application state is
- // partially set for the old change where the user is coming from,
- // and partially for the new change where the user is navigating to.
- // So setting the change explicitly to undefined when the user
- // moves away from diff and change pages (changeNum === undefined)
- // helps with that.
- updateStateChange(change ?? undefined);
- })
- );
- this.subscriptions.push(
- change$.subscribe(change => {
- this.change = change;
- })
- );
- }
-
- finalize() {
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions.splice(0, this.subscriptions.length);
- }
-
- // Temporary workaround until path is derived in the model itself.
- updatePath(path?: string) {
- updateStatePath(path);
- }
-
- /**
- * Typically you would just subscribe to change$ yourself to get updates. But
- * sometimes it is nice to also be able to get the current ChangeInfo on
- * demand. So here it is for your convenience.
- */
- getChange() {
- return this.change;
- }
-
- /**
- * Check whether there is no newer patch than the latest patch that was
- * available when this change was loaded.
- *
- * @return A promise that yields true if the latest patch
- * has been loaded, and false if a newer patch has been uploaded in the
- * meantime. The promise is rejected on network error.
- */
- fetchChangeUpdates(change: ChangeInfo | ParsedChangeInfo) {
- const knownLatest = computeLatestPatchNum(computeAllPatchSets(change));
- return this.restApiService.getChangeDetail(change._number).then(detail => {
- if (!detail) {
- const error = new Error('Change detail not found.');
- return Promise.reject(error);
- }
- const actualLatest = computeLatestPatchNum(computeAllPatchSets(detail));
- if (!actualLatest || !knownLatest) {
- const error = new Error('Unable to check for latest patchset.');
- return Promise.reject(error);
- }
- return {
- isLatest: actualLatest <= knownLatest,
- newStatus: change.status !== detail.status ? detail.status : null,
- newMessages:
- (change.messages || []).length < (detail.messages || []).length
- ? detail.messages![detail.messages!.length - 1]
- : undefined,
- };
- });
- }
-}
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index 6036d07..4024747 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -48,7 +48,7 @@
FetchResponse,
ResponseCode,
} from '../../api/checks';
-import {change$, changeNum$, latestPatchNum$} from '../change/change-model';
+import {ChangeModel} from '../change/change-model';
import {ChangeInfo, NumericChangeId, PatchSetNumber} from '../../types/common';
import {getCurrentRevision} from '../../utils/change-util';
import {getShaByPatchNum} from '../../utils/patch-set-util';
@@ -321,24 +321,28 @@
.filter(r => r !== undefined)
);
- constructor(readonly reporting: ReportingService) {
+ constructor(
+ readonly changeModel: ChangeModel,
+ readonly reporting: ReportingService
+ ) {
this.subscriptions = [
- changeNum$.subscribe(x => (this.changeNum = x)),
+ this.changeModel.changeNum$.subscribe(x => (this.changeNum = x)),
this.checkToPluginMap$.subscribe(map => {
this.checkToPluginMap = map;
}),
- combineLatest([routerPatchNum$, latestPatchNum$]).subscribe(
- ([routerPs, latestPs]) => {
- this.latestPatchNum = latestPs;
- if (latestPs === undefined) {
- this.setPatchset(undefined);
- } else if (typeof routerPs === 'number') {
- this.setPatchset(routerPs);
- } else {
- this.setPatchset(latestPs);
- }
+ combineLatest([
+ routerPatchNum$,
+ this.changeModel.latestPatchNum$,
+ ]).subscribe(([routerPs, latestPs]) => {
+ this.latestPatchNum = latestPs;
+ if (latestPs === undefined) {
+ this.setPatchset(undefined);
+ } else if (typeof routerPs === 'number') {
+ this.setPatchset(routerPs);
+ } else {
+ this.setPatchset(latestPs);
}
- ),
+ }),
];
this.visibilityChangeListener = () => {
this.documentVisibilityChange$.next(undefined);
@@ -634,9 +638,9 @@
// 4. A hidden Gerrit tab becoming visible.
this.subscriptions.push(
combineLatest([
- changeNum$,
+ this.changeModel.changeNum$,
patchset === ChecksPatchset.LATEST
- ? latestPatchNum$
+ ? this.changeModel.latestPatchNum$
: this.checksSelectedPatchsetNumber$,
this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
timer(0, pollIntervalMs),
@@ -645,7 +649,7 @@
.pipe(
takeWhile(_ => !!this.providers[pluginName]),
filter(_ => document.visibilityState !== 'hidden'),
- withLatestFrom(change$),
+ withLatestFrom(this.changeModel.change$),
switchMap(
([[changeNum, patchNum], change]): Observable<FetchResponse> => {
if (!change || !changeNum || !patchNum) return of(this.empty());
diff --git a/polygerrit-ui/app/services/checks/checks-model_test.ts b/polygerrit-ui/app/services/checks/checks-model_test.ts
index 390f6ad..0d46289 100644
--- a/polygerrit-ui/app/services/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/services/checks/checks-model_test.ts
@@ -18,7 +18,7 @@
import './checks-model';
import {ChecksModel, ChecksPatchset, ChecksProviderState} from './checks-model';
import {Category, CheckRun, RunStatus} from '../../api/checks';
-import {grReportingMock} from '../gr-reporting/gr-reporting_mock';
+import {getAppContext} from '../app-context';
const PLUGIN_NAME = 'test-plugin';
@@ -45,7 +45,10 @@
let current: ChecksProviderState;
setup(() => {
- model = new ChecksModel(grReportingMock);
+ model = new ChecksModel(
+ getAppContext().changeModel,
+ getAppContext().reportingService
+ );
model.checksLatest$.subscribe(c => (current = c[PLUGIN_NAME]));
});
diff --git a/polygerrit-ui/app/services/comments/comments-model.ts b/polygerrit-ui/app/services/comments/comments-model.ts
index 62aecbc..d46e08a 100644
--- a/polygerrit-ui/app/services/comments/comments-model.ts
+++ b/polygerrit-ui/app/services/comments/comments-model.ts
@@ -15,19 +15,42 @@
* limitations under the License.
*/
-import {BehaviorSubject, Observable} from 'rxjs';
+import {BehaviorSubject} from 'rxjs';
import {ChangeComments} from '../../elements/diff/gr-comment-api/gr-comment-api';
import {
+ CommentBasics,
CommentInfo,
+ NumericChangeId,
+ PatchSetNum,
+ RevisionId,
+ UrlEncodedCommentId,
PathToCommentsInfoMap,
RobotCommentInfo,
- UrlEncodedCommentId,
} from '../../types/common';
-import {addPath, DraftInfo, isDraft, isUnsaved} from '../../utils/comment-util';
+import {
+ addPath,
+ DraftInfo,
+ isDraft,
+ isUnsaved,
+ reportingDetails,
+ UnsavedInfo,
+} from '../../utils/comment-util';
import {deepEqual} from '../../utils/deep-util';
import {select} from '../../utils/observable-util';
+import {routerChangeNum$} from '../router/router-model';
+import {Finalizable} from '../registry';
+import {combineLatest, Subscription} from 'rxjs';
+import {fire, fireAlert, fireEvent} from '../../utils/event-util';
+import {CURRENT} from '../../utils/patch-set-util';
+import {RestApiService} from '../gr-rest-api/gr-rest-api';
+import {ChangeModel} from '../change/change-model';
+import {Interaction} from '../../constants/reporting';
+import {assertIsDefined} from '../../utils/common-util';
+import {debounce, DelayedTask} from '../../utils/async-util';
+import {pluralize} from '../../utils/string-util';
+import {ReportingService} from '../gr-reporting/gr-reporting';
-interface CommentState {
+export interface CommentState {
/** undefined means 'still loading' */
comments?: PathToCommentsInfoMap;
/** undefined means 'still loading' */
@@ -60,134 +83,93 @@
discardedDrafts: [],
};
-const privateState$ = new BehaviorSubject(initialState);
+const TOAST_DEBOUNCE_INTERVAL = 200;
-export function _testOnly_resetState() {
- // We cannot assign a new subject to privateState$, because all the selectors
- // have already subscribed to the original subject. So we have to emit the
- // initial state on the existing subject.
- privateState$.next({...initialState});
+function getSavingMessage(numPending: number, requestFailed?: boolean) {
+ if (requestFailed) {
+ return 'Unable to save draft';
+ }
+ if (numPending === 0) {
+ return 'All changes saved';
+ }
+ return `Saving ${pluralize(numPending, 'draft')}...`;
}
-// Re-exporting as Observable so that you can only subscribe, but not emit.
-export const commentState$: Observable<CommentState> = privateState$;
-
-export function _testOnly_getState() {
- return privateState$.getValue();
-}
-
-export function _testOnly_setState(state: CommentState) {
- privateState$.next(state);
-}
-
-export const commentsLoading$ = select(
- commentState$,
- commentState =>
- commentState.comments === undefined ||
- commentState.robotComments === undefined ||
- commentState.drafts === undefined
-);
-
-export const comments$ = select(
- commentState$,
- commentState => commentState.comments
-);
-
-export const drafts$ = select(
- commentState$,
- commentState => commentState.drafts
-);
-
-export const portedComments$ = select(
- commentState$,
- commentState => commentState.portedComments
-);
-
-export const discardedDrafts$ = select(
- commentState$,
- commentState => commentState.discardedDrafts
-);
-
-// Emits a new value even if only a single draft is changed. Components should
-// aim to subsribe to something more specific.
-export const changeComments$ = select(
- commentState$,
- commentState =>
- new ChangeComments(
- commentState.comments,
- commentState.robotComments,
- commentState.drafts,
- commentState.portedComments,
- commentState.portedDrafts
- )
-);
-
-export const threads$ = select(changeComments$, changeComments =>
- changeComments.getAllThreadsForChange()
-);
-
-export function thread$(id: UrlEncodedCommentId) {
- return select(threads$, threads => threads.find(t => t.rootId === id));
-}
-
-function publishState(state: CommentState) {
- privateState$.next(state);
-}
-
-/** Called when the change number changes. Wipes out all data from the state. */
-export function updateStateReset() {
- publishState({...initialState});
-}
-
-export function updateStateComments(comments?: {
- [path: string]: CommentInfo[];
-}) {
- const nextState = {...privateState$.getValue()};
- if (deepEqual(comments, nextState.comments)) return;
+// Private but used in tests.
+export function setComments(
+ state: CommentState,
+ comments?: {
+ [path: string]: CommentInfo[];
+ }
+): CommentState {
+ const nextState = {...state};
+ if (deepEqual(comments, nextState.comments)) return state;
nextState.comments = addPath(comments) || {};
- publishState(nextState);
+ return nextState;
}
-export function updateStateRobotComments(robotComments?: {
- [path: string]: RobotCommentInfo[];
-}) {
- const nextState = {...privateState$.getValue()};
- if (deepEqual(robotComments, nextState.robotComments)) return;
+// Private but used in tests.
+export function setRobotComments(
+ state: CommentState,
+ robotComments?: {
+ [path: string]: RobotCommentInfo[];
+ }
+): CommentState {
+ if (deepEqual(robotComments, state.robotComments)) return state;
+ const nextState = {...state};
nextState.robotComments = addPath(robotComments) || {};
- publishState(nextState);
+ return nextState;
}
-export function updateStateDrafts(drafts?: {[path: string]: DraftInfo[]}) {
- const nextState = {...privateState$.getValue()};
- if (deepEqual(drafts, nextState.drafts)) return;
+// Private but used in tests.
+export function setDrafts(
+ state: CommentState,
+ drafts?: {[path: string]: DraftInfo[]}
+): CommentState {
+ if (deepEqual(drafts, state.drafts)) return state;
+ const nextState = {...state};
nextState.drafts = addPath(drafts);
- publishState(nextState);
+ return nextState;
}
-export function updateStatePortedComments(
+// Private but used in tests.
+export function setPortedComments(
+ state: CommentState,
portedComments?: PathToCommentsInfoMap
-) {
- const nextState = {...privateState$.getValue()};
- if (deepEqual(portedComments, nextState.portedComments)) return;
+): CommentState {
+ if (deepEqual(portedComments, state.portedComments)) return state;
+ const nextState = {...state};
nextState.portedComments = portedComments || {};
- publishState(nextState);
+ return nextState;
}
-export function updateStatePortedDrafts(portedDrafts?: PathToCommentsInfoMap) {
- const nextState = {...privateState$.getValue()};
- if (deepEqual(portedDrafts, nextState.portedDrafts)) return;
+// Private but used in tests.
+export function setPortedDrafts(
+ state: CommentState,
+ portedDrafts?: PathToCommentsInfoMap
+): CommentState {
+ if (deepEqual(portedDrafts, state.portedDrafts)) return state;
+ const nextState = {...state};
nextState.portedDrafts = portedDrafts || {};
- publishState(nextState);
+ return nextState;
}
-export function updateStateSetDiscardedDraft(draft: DraftInfo) {
- const nextState = {...privateState$.getValue()};
+// Private but used in tests.
+export function setDiscardedDraft(
+ state: CommentState,
+ draft: DraftInfo
+): CommentState {
+ const nextState = {...state};
nextState.discardedDrafts = [...nextState.discardedDrafts, draft];
- publishState(nextState);
+ return nextState;
}
-export function updateStateDeleteDiscardedDraft(draftID?: string) {
- const nextState = {...privateState$.getValue()};
+// Private but used in tests.
+export function deleteDiscardedDraft(
+ state: CommentState,
+ draftID?: string
+): CommentState {
+ const nextState = {...state};
const drafts = [...nextState.discardedDrafts];
const index = drafts.findIndex(d => d.id === draftID);
if (index === -1) {
@@ -195,12 +177,12 @@
}
drafts.splice(index, 1);
nextState.discardedDrafts = drafts;
- publishState(nextState);
+ return nextState;
}
/** Adds or updates a draft. */
-export function updateStateSetDraft(draft: DraftInfo) {
- const nextState = {...privateState$.getValue()};
+export function setDraft(state: CommentState, draft: DraftInfo): CommentState {
+ const nextState = {...state};
if (!draft.path) throw new Error('draft path undefined');
if (!isDraft(draft)) throw new Error('draft is not a draft');
if (isUnsaved(draft)) throw new Error('unsaved drafts dont belong to model');
@@ -215,11 +197,14 @@
} else {
drafts[draft.path].push(draft);
}
- publishState(nextState);
+ return nextState;
}
-export function updateStateDeleteDraft(draft: DraftInfo) {
- const nextState = {...privateState$.getValue()};
+export function deleteDraft(
+ state: CommentState,
+ draft: DraftInfo
+): CommentState {
+ const nextState = {...state};
if (!draft.path) throw new Error('draft path undefined');
if (!isDraft(draft)) throw new Error('draft is not a draft');
if (isUnsaved(draft)) throw new Error('unsaved drafts dont belong to model');
@@ -228,10 +213,333 @@
const index = (drafts[draft.path] || []).findIndex(
d => d.id && d.id === draft.id
);
- if (index === -1) return;
+ if (index === -1) return state;
const discardedDraft = drafts[draft.path][index];
drafts[draft.path] = [...drafts[draft.path]];
drafts[draft.path].splice(index, 1);
- publishState(nextState);
- updateStateSetDiscardedDraft(discardedDraft);
+ return setDiscardedDraft(nextState, discardedDraft);
+}
+
+export class CommentsModel implements Finalizable {
+ private readonly privateState$: BehaviorSubject<CommentState> =
+ new BehaviorSubject(initialState);
+
+ public readonly commentsLoading$ = select(
+ this.privateState$,
+ commentState =>
+ commentState.comments === undefined ||
+ commentState.robotComments === undefined ||
+ commentState.drafts === undefined
+ );
+
+ public readonly comments$ = select(
+ this.privateState$,
+ commentState => commentState.comments
+ );
+
+ public readonly drafts$ = select(
+ this.privateState$,
+ commentState => commentState.drafts
+ );
+
+ public readonly portedComments$ = select(
+ this.privateState$,
+ commentState => commentState.portedComments
+ );
+
+ public readonly discardedDrafts$ = select(
+ this.privateState$,
+ commentState => commentState.discardedDrafts
+ );
+
+ // Emits a new value even if only a single draft is changed. Components should
+ // aim to subsribe to something more specific.
+ public readonly changeComments$ = select(
+ this.privateState$,
+ commentState =>
+ new ChangeComments(
+ commentState.comments,
+ commentState.robotComments,
+ commentState.drafts,
+ commentState.portedComments,
+ commentState.portedDrafts
+ )
+ );
+
+ public readonly threads$ = select(this.changeComments$, changeComments =>
+ changeComments.getAllThreadsForChange()
+ );
+
+ public thread$(id: UrlEncodedCommentId) {
+ return select(this.threads$, threads => threads.find(t => t.rootId === id));
+ }
+
+ private numPendingDraftRequests = 0;
+
+ private changeNum?: NumericChangeId;
+
+ private patchNum?: PatchSetNum;
+
+ private readonly reloadListener: () => void;
+
+ private readonly subscriptions: Subscription[] = [];
+
+ private drafts: {[path: string]: DraftInfo[]} = {};
+
+ private draftToastTask?: DelayedTask;
+
+ private discardedDrafts: DraftInfo[] = [];
+
+ constructor(
+ readonly changeModel: ChangeModel,
+ readonly restApiService: RestApiService,
+ readonly reporting: ReportingService
+ ) {
+ this.subscriptions.push(
+ this.discardedDrafts$.subscribe(x => (this.discardedDrafts = x))
+ );
+ this.subscriptions.push(
+ this.drafts$.subscribe(x => (this.drafts = x ?? {}))
+ );
+ this.subscriptions.push(
+ this.changeModel.currentPatchNum$.subscribe(x => (this.patchNum = x))
+ );
+ this.subscriptions.push(
+ routerChangeNum$.subscribe(changeNum => {
+ this.changeNum = changeNum;
+ this.setState({...initialState});
+ this.reloadAllComments();
+ })
+ );
+ this.subscriptions.push(
+ combineLatest([
+ this.changeModel.changeNum$,
+ this.changeModel.currentPatchNum$,
+ ]).subscribe(([changeNum, patchNum]) => {
+ this.changeNum = changeNum;
+ this.patchNum = patchNum;
+ this.reloadAllPortedComments();
+ })
+ );
+ this.reloadListener = () => {
+ this.reloadAllComments();
+ this.reloadAllPortedComments();
+ };
+ document.addEventListener('reload', this.reloadListener);
+ }
+
+ finalize() {
+ document.removeEventListener('reload', this.reloadListener!);
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
+ }
+
+ // Note that this does *not* reload ported comments.
+ async reloadAllComments() {
+ if (!this.changeNum) return;
+ await Promise.all([
+ this.reloadComments(this.changeNum),
+ this.reloadRobotComments(this.changeNum),
+ this.reloadDrafts(this.changeNum),
+ ]);
+ }
+
+ async reloadAllPortedComments() {
+ if (!this.changeNum) return;
+ if (!this.patchNum) return;
+ await Promise.all([
+ this.reloadPortedComments(this.changeNum, this.patchNum),
+ this.reloadPortedDrafts(this.changeNum, this.patchNum),
+ ]);
+ }
+
+ // visible for testing
+ updateState(reducer: (state: CommentState) => CommentState) {
+ const current = this.privateState$.getValue();
+ this.setState(reducer({...current}));
+ }
+
+ // visible for testing
+ setState(state: CommentState) {
+ this.privateState$.next(state);
+ }
+
+ async reloadComments(changeNum: NumericChangeId): Promise<void> {
+ const comments = await this.restApiService.getDiffComments(changeNum);
+ this.updateState(s => setComments(s, comments));
+ }
+
+ async reloadRobotComments(changeNum: NumericChangeId): Promise<void> {
+ const robotComments = await this.restApiService.getDiffRobotComments(
+ changeNum
+ );
+ this.updateState(s => setRobotComments(s, robotComments));
+ }
+
+ async reloadDrafts(changeNum: NumericChangeId): Promise<void> {
+ const drafts = await this.restApiService.getDiffDrafts(changeNum);
+ this.updateState(s => setDrafts(s, drafts));
+ }
+
+ async reloadPortedComments(
+ changeNum: NumericChangeId,
+ patchNum = CURRENT as RevisionId
+ ): Promise<void> {
+ const portedComments = await this.restApiService.getPortedComments(
+ changeNum,
+ patchNum
+ );
+ this.updateState(s => setPortedComments(s, portedComments));
+ }
+
+ async reloadPortedDrafts(
+ changeNum: NumericChangeId,
+ patchNum = CURRENT as RevisionId
+ ): Promise<void> {
+ const portedDrafts = await this.restApiService.getPortedDrafts(
+ changeNum,
+ patchNum
+ );
+ this.updateState(s => setPortedDrafts(s, portedDrafts));
+ }
+
+ async restoreDraft(id: UrlEncodedCommentId) {
+ const found = this.discardedDrafts?.find(d => d.id === id);
+ if (!found) throw new Error('discarded draft not found');
+ const newDraft = {
+ ...found,
+ id: undefined,
+ updated: undefined,
+ __draft: undefined,
+ __unsaved: true,
+ };
+ await this.saveDraft(newDraft);
+ this.updateState(s => deleteDiscardedDraft(s, id));
+ }
+
+ /**
+ * Saves a new or updates an existing draft.
+ * The model will only be updated when a successful response comes back.
+ */
+ async saveDraft(draft: DraftInfo | UnsavedInfo, showToast = true) {
+ assertIsDefined(this.changeNum, 'change number');
+ assertIsDefined(draft.patch_set, 'patchset number of comment draft');
+ if (!draft.message?.trim()) throw new Error('Cannot save empty draft.');
+
+ // Saving the change number as to make sure that the response is still
+ // relevant when it comes back. The user maybe have navigated away.
+ const changeNum = this.changeNum;
+ this.report(Interaction.SAVE_COMMENT, draft);
+ if (showToast) this.showStartRequest();
+ const result = await this.restApiService.saveDiffDraft(
+ changeNum,
+ draft.patch_set,
+ draft
+ );
+ if (changeNum !== this.changeNum) throw new Error('change changed');
+ if (!result.ok) {
+ if (showToast) this.handleFailedDraftRequest();
+ throw new Error(
+ `Failed to save draft comment: ${JSON.stringify(result)}`
+ );
+ }
+ const obj = await this.restApiService.getResponseObject(result);
+ const savedComment = obj as unknown as CommentInfo;
+ const updatedDraft = {
+ ...draft,
+ id: savedComment.id,
+ updated: savedComment.updated,
+ __draft: true,
+ __unsaved: undefined,
+ };
+ if (showToast) this.showEndRequest();
+ this.updateState(s => setDraft(s, updatedDraft));
+ this.report(Interaction.COMMENT_SAVED, updatedDraft);
+ }
+
+ async discardDraft(draftId: UrlEncodedCommentId) {
+ const draft = this.lookupDraft(draftId);
+ assertIsDefined(this.changeNum, 'change number');
+ assertIsDefined(draft, `draft not found by id ${draftId}`);
+ assertIsDefined(draft.patch_set, 'patchset number of comment draft');
+
+ if (!draft.message?.trim()) throw new Error('saved draft cant be empty');
+ // Saving the change number as to make sure that the response is still
+ // relevant when it comes back. The user maybe have navigated away.
+ const changeNum = this.changeNum;
+ this.report(Interaction.DISCARD_COMMENT, draft);
+ this.showStartRequest();
+ const result = await this.restApiService.deleteDiffDraft(
+ changeNum,
+ draft.patch_set,
+ {id: draft.id}
+ );
+ if (changeNum !== this.changeNum) throw new Error('change changed');
+ if (!result.ok) {
+ this.handleFailedDraftRequest();
+ throw new Error(
+ `Failed to discard draft comment: ${JSON.stringify(result)}`
+ );
+ }
+ this.showEndRequest();
+ this.updateState(s => deleteDraft(s, draft));
+ // We don't store empty discarded drafts and don't need an UNDO then.
+ if (draft.message?.trim()) {
+ fire(document, 'show-alert', {
+ message: 'Draft Discarded',
+ action: 'Undo',
+ callback: () => this.restoreDraft(draft.id),
+ });
+ }
+ this.report(Interaction.COMMENT_DISCARDED, draft);
+ }
+
+ private report(interaction: Interaction, comment: CommentBasics) {
+ const details = reportingDetails(comment);
+ this.reporting.reportInteraction(interaction, details);
+ }
+
+ private showStartRequest() {
+ this.numPendingDraftRequests += 1;
+ this.updateRequestToast();
+ }
+
+ private showEndRequest() {
+ this.numPendingDraftRequests -= 1;
+ this.updateRequestToast();
+ }
+
+ private handleFailedDraftRequest() {
+ this.numPendingDraftRequests -= 1;
+ this.updateRequestToast(/* requestFailed=*/ true);
+ }
+
+ private updateRequestToast(requestFailed?: boolean) {
+ if (this.numPendingDraftRequests === 0 && !requestFailed) {
+ fireEvent(document, 'hide-alert');
+ return;
+ }
+ const message = getSavingMessage(
+ this.numPendingDraftRequests,
+ requestFailed
+ );
+ this.draftToastTask = debounce(
+ this.draftToastTask,
+ () => {
+ // Note: the event is fired on the body rather than this element because
+ // this element may not be attached by the time this executes, in which
+ // case the event would not bubble.
+ fireAlert(document.body, message);
+ },
+ TOAST_DEBOUNCE_INTERVAL
+ );
+ }
+
+ private lookupDraft(id: UrlEncodedCommentId): DraftInfo | undefined {
+ return Object.values(this.drafts)
+ .flat()
+ .find(d => d.id === id);
+ }
}
diff --git a/polygerrit-ui/app/services/comments/comments-model_test.ts b/polygerrit-ui/app/services/comments/comments-model_test.ts
index 30fc7cf..f39cad8 100644
--- a/polygerrit-ui/app/services/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/services/comments/comments-model_test.ts
@@ -19,17 +19,28 @@
import {UrlEncodedCommentId} from '../../types/common';
import {DraftInfo} from '../../utils/comment-util';
import './comments-model';
+import {CommentsModel} from './comments-model';
+import {deleteDraft} from './comments-model';
+import {Subscription} from 'rxjs';
+import '../../test/common-test-setup-karma';
import {
- updateStateDeleteDraft,
- _testOnly_getState,
- _testOnly_setState,
-} from './comments-model';
+ createComment,
+ createParsedChange,
+ TEST_NUMERIC_CHANGE_ID,
+} from '../../test/test-data-generators';
+import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils';
+import {getAppContext} from '../app-context';
+import {
+ GerritView,
+ updateState as updateRouterState,
+} from '../router/router-model';
+import {PathToCommentsInfoMap} from '../../types/common';
suite('comments model tests', () => {
test('updateStateDeleteDraft', () => {
const draft = createDraft();
draft.id = '1' as UrlEncodedCommentId;
- _testOnly_setState({
+ const state = {
comments: {},
robotComments: {},
drafts: {
@@ -38,9 +49,9 @@
portedComments: {},
portedDrafts: {},
discardedDrafts: [],
- });
- updateStateDeleteDraft(draft);
- assert.deepEqual(_testOnly_getState(), {
+ };
+ const output = deleteDraft(state, draft);
+ assert.deepEqual(output, {
comments: {},
robotComments: {},
drafts: {
@@ -52,3 +63,65 @@
});
});
});
+
+suite('change service tests', () => {
+ let subscriptions: Subscription[] = [];
+
+ teardown(() => {
+ for (const s of subscriptions) {
+ s.unsubscribe();
+ }
+ subscriptions = [];
+ });
+
+ test('loads comments', async () => {
+ const model = new CommentsModel(
+ getAppContext().changeModel,
+ getAppContext().restApiService,
+ getAppContext().reportingService
+ );
+ const diffCommentsSpy = stubRestApi('getDiffComments').returns(
+ Promise.resolve({'foo.c': [createComment()]})
+ );
+ const diffRobotCommentsSpy = stubRestApi('getDiffRobotComments').returns(
+ Promise.resolve({})
+ );
+ const diffDraftsSpy = stubRestApi('getDiffDrafts').returns(
+ Promise.resolve({})
+ );
+ const portedCommentsSpy = stubRestApi('getPortedComments').returns(
+ Promise.resolve({'foo.c': [createComment()]})
+ );
+ const portedDraftsSpy = stubRestApi('getPortedDrafts').returns(
+ Promise.resolve({})
+ );
+ let comments: PathToCommentsInfoMap = {};
+ subscriptions.push(model.comments$.subscribe(c => (comments = c ?? {})));
+ let portedComments: PathToCommentsInfoMap = {};
+ subscriptions.push(
+ model.portedComments$.subscribe(c => (portedComments = c ?? {}))
+ );
+
+ updateRouterState(GerritView.CHANGE, TEST_NUMERIC_CHANGE_ID);
+ model.changeModel.updateStateChange(createParsedChange());
+
+ await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy');
+ await waitUntilCalled(diffRobotCommentsSpy, 'diffRobotCommentsSpy');
+ await waitUntilCalled(diffDraftsSpy, 'diffDraftsSpy');
+ await waitUntilCalled(portedCommentsSpy, 'portedCommentsSpy');
+ await waitUntilCalled(portedDraftsSpy, 'portedDraftsSpy');
+ await waitUntil(
+ () => Object.keys(comments).length > 0,
+ 'comment in model not set'
+ );
+ await waitUntil(
+ () => Object.keys(portedComments).length > 0,
+ 'ported comment in model not set'
+ );
+
+ assert.equal(comments['foo.c'].length, 1);
+ assert.equal(comments['foo.c'][0].id, '12345');
+ assert.equal(portedComments['foo.c'].length, 1);
+ assert.equal(portedComments['foo.c'][0].id, '12345');
+ });
+});
diff --git a/polygerrit-ui/app/services/comments/comments-service.ts b/polygerrit-ui/app/services/comments/comments-service.ts
deleted file mode 100644
index ac32258..0000000
--- a/polygerrit-ui/app/services/comments/comments-service.ts
+++ /dev/null
@@ -1,315 +0,0 @@
-/**
- * @license
- * Copyright (C) 2021 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.
- */
-import {combineLatest, Subscription} from 'rxjs';
-import {
- CommentBasics,
- CommentInfo,
- NumericChangeId,
- PatchSetNum,
- RevisionId,
- UrlEncodedCommentId,
-} from '../../types/common';
-import {
- reportingDetails,
- DraftInfo,
- UnsavedInfo,
-} from '../../utils/comment-util';
-import {fire, fireAlert, fireEvent} from '../../utils/event-util';
-import {CURRENT} from '../../utils/patch-set-util';
-import {RestApiService} from '../gr-rest-api/gr-rest-api';
-import {
- updateStateSetDraft,
- updateStateDeleteDraft,
- updateStateComments,
- updateStateRobotComments,
- updateStateDrafts,
- updateStatePortedComments,
- updateStatePortedDrafts,
- updateStateDeleteDiscardedDraft,
- discardedDrafts$,
- updateStateReset,
- drafts$,
-} from './comments-model';
-import {changeNum$, currentPatchNum$} from '../change/change-model';
-import {Finalizable} from '../registry';
-import {routerChangeNum$} from '../router/router-model';
-import {Interaction} from '../../constants/reporting';
-import {assertIsDefined} from '../../utils/common-util';
-import {debounce, DelayedTask} from '../../utils/async-util';
-import {pluralize} from '../../utils/string-util';
-import {ReportingService} from '../gr-reporting/gr-reporting';
-
-const TOAST_DEBOUNCE_INTERVAL = 200;
-
-function getSavingMessage(numPending: number, requestFailed?: boolean) {
- if (requestFailed) {
- return 'Unable to save draft';
- }
- if (numPending === 0) {
- return 'All changes saved';
- }
- return `Saving ${pluralize(numPending, 'draft')}...`;
-}
-
-export class CommentsService implements Finalizable {
- private numPendingDraftRequests = 0;
-
- private changeNum?: NumericChangeId;
-
- private patchNum?: PatchSetNum;
-
- private readonly reloadListener: () => void;
-
- private readonly subscriptions: Subscription[] = [];
-
- private drafts: {[path: string]: DraftInfo[]} = {};
-
- private draftToastTask?: DelayedTask;
-
- private discardedDrafts: DraftInfo[] = [];
-
- constructor(
- readonly restApiService: RestApiService,
- readonly reporting: ReportingService
- ) {
- this.subscriptions.push(
- discardedDrafts$.subscribe(x => (this.discardedDrafts = x))
- );
- this.subscriptions.push(drafts$.subscribe(x => (this.drafts = x ?? {})));
- this.subscriptions.push(
- currentPatchNum$.subscribe(x => (this.patchNum = x))
- );
- this.subscriptions.push(
- routerChangeNum$.subscribe(changeNum => {
- this.changeNum = changeNum;
- updateStateReset();
- this.reloadAllComments();
- })
- );
- this.subscriptions.push(
- combineLatest([changeNum$, currentPatchNum$]).subscribe(
- ([changeNum, patchNum]) => {
- this.changeNum = changeNum;
- this.patchNum = patchNum;
- this.reloadAllPortedComments();
- }
- )
- );
- this.reloadListener = () => {
- this.reloadAllComments();
- this.reloadAllPortedComments();
- };
- document.addEventListener('reload', this.reloadListener);
- }
-
- finalize() {
- document.removeEventListener('reload', this.reloadListener!);
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions.splice(0, this.subscriptions.length);
- }
-
- // Note that this does *not* reload ported comments.
- reloadAllComments() {
- if (!this.changeNum) return;
- this.reloadComments(this.changeNum);
- this.reloadRobotComments(this.changeNum);
- this.reloadDrafts(this.changeNum);
- }
-
- reloadAllPortedComments() {
- if (!this.changeNum) return;
- if (!this.patchNum) return;
- this.reloadPortedComments(this.changeNum, this.patchNum);
- this.reloadPortedDrafts(this.changeNum, this.patchNum);
- }
-
- reloadComments(changeNum: NumericChangeId): Promise<void> {
- return this.restApiService
- .getDiffComments(changeNum)
- .then(comments => updateStateComments(comments));
- }
-
- reloadRobotComments(changeNum: NumericChangeId): Promise<void> {
- return this.restApiService
- .getDiffRobotComments(changeNum)
- .then(robotComments => updateStateRobotComments(robotComments));
- }
-
- reloadDrafts(changeNum: NumericChangeId): Promise<void> {
- return this.restApiService
- .getDiffDrafts(changeNum)
- .then(drafts => updateStateDrafts(drafts));
- }
-
- reloadPortedComments(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- return this.restApiService
- .getPortedComments(changeNum, patchNum)
- .then(portedComments => updateStatePortedComments(portedComments));
- }
-
- reloadPortedDrafts(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- return this.restApiService
- .getPortedDrafts(changeNum, patchNum)
- .then(portedDrafts => updateStatePortedDrafts(portedDrafts));
- }
-
- async restoreDraft(id: UrlEncodedCommentId) {
- const found = this.discardedDrafts?.find(d => d.id === id);
- if (!found) throw new Error('discarded draft not found');
- const newDraft = {
- ...found,
- id: undefined,
- updated: undefined,
- __draft: undefined,
- __unsaved: true,
- };
- await this.saveDraft(newDraft);
- updateStateDeleteDiscardedDraft(id);
- }
-
- /**
- * Saves a new or updates an existing draft.
- * The model will only be updated when a successful response comes back.
- */
- async saveDraft(draft: DraftInfo | UnsavedInfo, showToast = true) {
- assertIsDefined(this.changeNum, 'change number');
- assertIsDefined(draft.patch_set, 'patchset number of comment draft');
- if (!draft.message?.trim()) throw new Error('Cannot save empty draft.');
-
- // Saving the change number as to make sure that the response is still
- // relevant when it comes back. The user maybe have navigated away.
- const changeNum = this.changeNum;
- this.report(Interaction.SAVE_COMMENT, draft);
- if (showToast) this.showStartRequest();
- const result = await this.restApiService.saveDiffDraft(
- changeNum,
- draft.patch_set,
- draft
- );
- if (changeNum !== this.changeNum) throw new Error('change changed');
- if (!result.ok) {
- if (showToast) this.handleFailedDraftRequest();
- throw new Error(
- `Failed to save draft comment: ${JSON.stringify(result)}`
- );
- }
- const obj = await this.restApiService.getResponseObject(result);
- const savedComment = obj as unknown as CommentInfo;
- const updatedDraft = {
- ...draft,
- id: savedComment.id,
- updated: savedComment.updated,
- __draft: true,
- __unsaved: undefined,
- };
- if (showToast) this.showEndRequest();
- updateStateSetDraft(updatedDraft);
- this.report(Interaction.COMMENT_SAVED, updatedDraft);
- }
-
- async discardDraft(draftId: UrlEncodedCommentId) {
- const draft = this.lookupDraft(draftId);
- assertIsDefined(this.changeNum, 'change number');
- assertIsDefined(draft, `draft not found by id ${draftId}`);
- assertIsDefined(draft.patch_set, 'patchset number of comment draft');
-
- if (!draft.message?.trim()) throw new Error('saved draft cant be empty');
- // Saving the change number as to make sure that the response is still
- // relevant when it comes back. The user maybe have navigated away.
- const changeNum = this.changeNum;
- this.report(Interaction.DISCARD_COMMENT, draft);
- this.showStartRequest();
- const result = await this.restApiService.deleteDiffDraft(
- changeNum,
- draft.patch_set,
- {id: draft.id}
- );
- if (changeNum !== this.changeNum) throw new Error('change changed');
- if (!result.ok) {
- this.handleFailedDraftRequest();
- throw new Error(
- `Failed to discard draft comment: ${JSON.stringify(result)}`
- );
- }
- this.showEndRequest();
- updateStateDeleteDraft(draft);
- // We don't store empty discarded drafts and don't need an UNDO then.
- if (draft.message?.trim()) {
- fire(document, 'show-alert', {
- message: 'Draft Discarded',
- action: 'Undo',
- callback: () => this.restoreDraft(draft.id),
- });
- }
- this.report(Interaction.COMMENT_DISCARDED, draft);
- }
-
- private report(interaction: Interaction, comment: CommentBasics) {
- const details = reportingDetails(comment);
- this.reporting.reportInteraction(interaction, details);
- }
-
- private showStartRequest() {
- this.numPendingDraftRequests += 1;
- this.updateRequestToast();
- }
-
- private showEndRequest() {
- this.numPendingDraftRequests -= 1;
- this.updateRequestToast();
- }
-
- private handleFailedDraftRequest() {
- this.numPendingDraftRequests -= 1;
- this.updateRequestToast(/* requestFailed=*/ true);
- }
-
- private updateRequestToast(requestFailed?: boolean) {
- if (this.numPendingDraftRequests === 0 && !requestFailed) {
- fireEvent(document, 'hide-alert');
- return;
- }
- const message = getSavingMessage(
- this.numPendingDraftRequests,
- requestFailed
- );
- this.draftToastTask = debounce(
- this.draftToastTask,
- () => {
- // Note: the event is fired on the body rather than this element because
- // this element may not be attached by the time this executes, in which
- // case the event would not bubble.
- fireAlert(document.body, message);
- },
- TOAST_DEBOUNCE_INTERVAL
- );
- }
-
- private lookupDraft(id: UrlEncodedCommentId): DraftInfo | undefined {
- return Object.values(this.drafts)
- .flat()
- .find(d => d.id === id);
- }
-}
diff --git a/polygerrit-ui/app/services/comments/comments-service_test.ts b/polygerrit-ui/app/services/comments/comments-service_test.ts
deleted file mode 100644
index 91ac37c..0000000
--- a/polygerrit-ui/app/services/comments/comments-service_test.ts
+++ /dev/null
@@ -1,94 +0,0 @@
-/**
- * @license
- * Copyright (C) 2021 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.
- */
-import {Subscription} from 'rxjs';
-import '../../test/common-test-setup-karma';
-import {
- createComment,
- createParsedChange,
- TEST_NUMERIC_CHANGE_ID,
-} from '../../test/test-data-generators';
-import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils';
-import {getAppContext} from '../app-context';
-import {CommentsService} from './comments-service';
-import {updateStateChange} from '../change/change-model';
-import {
- GerritView,
- updateState as updateRouterState,
-} from '../router/router-model';
-import {comments$, portedComments$} from './comments-model';
-import {PathToCommentsInfoMap} from '../../types/common';
-
-suite('change service tests', () => {
- let subscriptions: Subscription[] = [];
-
- teardown(() => {
- for (const s of subscriptions) {
- s.unsubscribe();
- }
- subscriptions = [];
- });
-
- test('loads comments', async () => {
- new CommentsService(
- getAppContext().restApiService,
- getAppContext().reportingService
- );
- const diffCommentsSpy = stubRestApi('getDiffComments').returns(
- Promise.resolve({'foo.c': [createComment()]})
- );
- const diffRobotCommentsSpy = stubRestApi('getDiffRobotComments').returns(
- Promise.resolve({})
- );
- const diffDraftsSpy = stubRestApi('getDiffDrafts').returns(
- Promise.resolve({})
- );
- const portedCommentsSpy = stubRestApi('getPortedComments').returns(
- Promise.resolve({'foo.c': [createComment()]})
- );
- const portedDraftsSpy = stubRestApi('getPortedDrafts').returns(
- Promise.resolve({})
- );
- let comments: PathToCommentsInfoMap = {};
- subscriptions.push(comments$.subscribe(c => (comments = c ?? {})));
- let portedComments: PathToCommentsInfoMap = {};
- subscriptions.push(
- portedComments$.subscribe(c => (portedComments = c ?? {}))
- );
-
- updateRouterState(GerritView.CHANGE, TEST_NUMERIC_CHANGE_ID);
- updateStateChange(createParsedChange());
-
- await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy');
- await waitUntilCalled(diffRobotCommentsSpy, 'diffRobotCommentsSpy');
- await waitUntilCalled(diffDraftsSpy, 'diffDraftsSpy');
- await waitUntilCalled(portedCommentsSpy, 'portedCommentsSpy');
- await waitUntilCalled(portedDraftsSpy, 'portedDraftsSpy');
- await waitUntil(
- () => Object.keys(comments).length > 0,
- 'comment in model not set'
- );
- await waitUntil(
- () => Object.keys(portedComments).length > 0,
- 'ported comment in model not set'
- );
-
- assert.equal(comments['foo.c'].length, 1);
- assert.equal(comments['foo.c'][0].id, '12345');
- assert.equal(portedComments['foo.c'].length, 1);
- assert.equal(portedComments['foo.c'][0].id, '12345');
- });
-});
diff --git a/polygerrit-ui/app/services/config/config-model.ts b/polygerrit-ui/app/services/config/config-model.ts
index 663935f..91a77b8 100644
--- a/polygerrit-ui/app/services/config/config-model.ts
+++ b/polygerrit-ui/app/services/config/config-model.ts
@@ -19,7 +19,7 @@
import {switchMap} from 'rxjs/operators';
import {Finalizable} from '../registry';
import {RestApiService} from '../gr-rest-api/gr-rest-api';
-import {repo$} from '../change/change-model';
+import {ChangeModel} from '../change/change-model';
import {select} from '../../utils/observable-util';
export interface ConfigState {
@@ -55,12 +55,15 @@
private subscriptions: Subscription[];
- constructor(readonly restApiService: RestApiService) {
+ constructor(
+ readonly changeModel: ChangeModel,
+ readonly restApiService: RestApiService
+ ) {
this.subscriptions = [
from(this.restApiService.getConfig()).subscribe((config?: ServerInfo) => {
this.updateServerConfig(config);
}),
- repo$
+ this.changeModel.repo$
.pipe(
switchMap((repo?: RepoName) => {
if (repo === undefined) return of(undefined);
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index fc621a2..b8a5e49 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -45,8 +45,6 @@
import {_testOnly_allTasks} from '../utils/async-util';
import {cleanUpStorage} from '../services/storage/gr-storage_mock';
-import {_testOnly_resetState as resetChangeState} from '../services/change/change-model';
-import {_testOnly_resetState as resetCommentsState} from '../services/comments/comments-model';
import {_testOnly_resetState as resetRouterState} from '../services/router/router-model';
declare global {
@@ -113,8 +111,6 @@
// tests.
initGlobalVariables(appContext);
- resetChangeState();
- resetCommentsState();
resetRouterState();
const shortcuts = appContext.shortcutsService;
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index 946f813..01d776e 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -25,11 +25,11 @@
import {GrAuthMock} from '../services/gr-auth/gr-auth_mock';
import {FlagsServiceImplementation} from '../services/flags/flags_impl';
import {EventEmitter} from '../services/gr-event-interface/gr-event-interface_impl';
-import {ChangeService} from '../services/change/change-service';
+import {ChangeModel} from '../services/change/change-model';
import {ChecksModel} from '../services/checks/checks-model';
import {GrJsApiInterface} from '../elements/shared/gr-js-api-interface/gr-js-api-interface-element';
import {UserModel} from '../services/user/user-model';
-import {CommentsService} from '../services/comments/comments-service';
+import {CommentsModel} from '../services/comments/comments-model';
import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
import {BrowserModel} from '../services/browser/browser-model';
import {ConfigModel} from '../services/config/config-model';
@@ -45,18 +45,24 @@
return new GrAuthMock(ctx.eventEmitter);
},
restApiService: (_ctx: Partial<AppContext>) => grRestApiMock,
- changeService: (ctx: Partial<AppContext>) => {
+ changeModel: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.restApiService, 'restApiService');
- return new ChangeService(ctx.restApiService!);
+ return new ChangeModel(ctx.restApiService!);
},
- commentsService: (ctx: Partial<AppContext>) => {
+ commentsModel: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.changeModel, 'changeModel');
assertIsDefined(ctx.restApiService, 'restApiService');
assertIsDefined(ctx.reportingService, 'reportingService');
- return new CommentsService(ctx.restApiService!, ctx.reportingService!);
+ return new CommentsModel(
+ ctx.changeModel!,
+ ctx.restApiService!,
+ ctx.reportingService!
+ );
},
checksModel: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.changeModel, 'changeModel');
assertIsDefined(ctx.reportingService, 'reportingService');
- return new ChecksModel(ctx.reportingService!);
+ return new ChecksModel(ctx.changeModel!, ctx.reportingService!);
},
jsApiService: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.reportingService, 'reportingService');
@@ -64,8 +70,9 @@
},
storageService: (_ctx: Partial<AppContext>) => grStorageMock,
configModel: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.changeModel, 'changeModel');
assertIsDefined(ctx.restApiService, 'restApiService');
- return new ConfigModel(ctx.restApiService!);
+ return new ConfigModel(ctx.changeModel!, ctx.restApiService!);
},
userModel: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.restApiService, 'restApiService');
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 57d2b0c..da4601b 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -741,6 +741,7 @@
name: 'Verified',
status: SubmitRequirementStatus.SATISFIED,
submittability_expression_result: createSubmitRequirementExpressionInfo(),
+ is_legacy: false,
};
}
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index 50e08fb..ebeb1db 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -23,7 +23,7 @@
import {StorageService} from '../services/storage/gr-storage';
import {AuthService} from '../services/gr-auth/gr-auth';
import {ReportingService} from '../services/gr-reporting/gr-reporting';
-import {CommentsService} from '../services/comments/comments-service';
+import {CommentsModel} from '../services/comments/comments-model';
import {UserModel} from '../services/user/user-model';
import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
import {queryAndAssert, query} from '../utils/common-util';
@@ -112,8 +112,8 @@
return sinon.spy(getAppContext().restApiService, method);
}
-export function stubComments<K extends keyof CommentsService>(method: K) {
- return sinon.stub(getAppContext().commentsService, method);
+export function stubComments<K extends keyof CommentsModel>(method: K) {
+ return sinon.stub(getAppContext().commentsModel, method);
}
export function stubUsers<K extends keyof UserModel>(method: K) {
diff --git a/polygerrit-ui/app/utils/label-util.ts b/polygerrit-ui/app/utils/label-util.ts
index 8904cac..c154958 100644
--- a/polygerrit-ui/app/utils/label-util.ts
+++ b/polygerrit-ui/app/utils/label-util.ts
@@ -20,6 +20,7 @@
SubmitRequirementResultInfo,
SubmitRequirementStatus,
} from '../api/rest-api';
+import {FlagsService, KnownExperimentId} from '../services/flags/flags';
import {
AccountInfo,
ApprovalInfo,
@@ -262,20 +263,9 @@
* If there is at least one non-legacy requirement, filter legacy requirements.
*/
export function getRequirements(change?: ParsedChangeInfo | ChangeInfo) {
- let submit_requirements = (change?.submit_requirements ?? []).filter(
- req => req.status !== SubmitRequirementStatus.NOT_APPLICABLE
- );
-
- const hasNonLegacyRequirements = submit_requirements.some(
- req => req.is_legacy === false
- );
- if (hasNonLegacyRequirements) {
- submit_requirements = submit_requirements.filter(
- req => req.is_legacy === false
- );
- }
-
- return submit_requirements;
+ return (change?.submit_requirements ?? [])
+ .filter(req => req.status !== SubmitRequirementStatus.NOT_APPLICABLE)
+ .filter(req => req.is_legacy === false);
}
// TODO(milutin): This may be temporary for demo purposes
@@ -309,3 +299,16 @@
label => !labelAssociatedWithSubmitReqs.includes(label)
);
}
+
+export function showNewSubmitRequirements(
+ flagsService: FlagsService,
+ change?: ParsedChangeInfo | ChangeInfo
+) {
+ const isSubmitRequirementsUiEnabled = flagsService.isEnabled(
+ KnownExperimentId.SUBMIT_REQUIREMENTS_UI
+ );
+ if (!isSubmitRequirementsUiEnabled) return false;
+ if ((getRequirements(change) ?? []).length === 0) return false;
+
+ return true;
+}
diff --git a/polygerrit-ui/app/utils/label-util_test.ts b/polygerrit-ui/app/utils/label-util_test.ts
index 6cb04af..1b8ad0c 100644
--- a/polygerrit-ui/app/utils/label-util_test.ts
+++ b/polygerrit-ui/app/utils/label-util_test.ts
@@ -298,7 +298,7 @@
is_legacy: true,
};
const change = createChangeInfoWith([requirement]);
- assert.deepEqual(getRequirements(change), [requirement]);
+ assert.deepEqual(getRequirements(change), []);
});
test('legacy and non-legacy - filter legacy', () => {
const requirement = {
@@ -313,10 +313,7 @@
assert.deepEqual(getRequirements(change), [requirement2]);
});
test('filter not applicable', () => {
- const requirement = {
- ...createSubmitRequirementResultInfo(),
- is_legacy: true,
- };
+ const requirement = createSubmitRequirementResultInfo();
const requirement2 = {
...createSubmitRequirementResultInfo(),
status: SubmitRequirementStatus.NOT_APPLICABLE,
@@ -348,6 +345,7 @@
...createSubmitRequirementExpressionInfo(),
expression: `label:${triggerVote}=MAX`,
},
+ is_legacy: false,
},
],
labels: {
