Merge "Fix SEND_REPLY timer."
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 844f4c6..8aa5c7f 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2916,6 +2916,11 @@
The custom keyed values to add or remove must be provided in the request body
inside a link:#custom-keyed-values-input[CustomKeyedValuesInput] entity.
+Note that custom keyed values are expected to be small in both key and value.
+A typical use-case would be storing the ID to some external system, in which
+case the key would be something unique to that system and the value would be
+the ID.
+
.Request
----
POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/custom_keyed_values HTTP/1.0
diff --git a/java/com/google/gerrit/server/git/TagSet.java b/java/com/google/gerrit/server/git/TagSet.java
index a528c8f..bffc479 100644
--- a/java/com/google/gerrit/server/git/TagSet.java
+++ b/java/com/google/gerrit/server/git/TagSet.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.git;
+import static com.google.common.base.Preconditions.checkNotNull;
+
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
@@ -41,6 +43,7 @@
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevFlag;
import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
import org.roaringbitmap.RoaringBitmap;
@@ -209,6 +212,7 @@
try (TagWalk rw = new TagWalk(git)) {
rw.setRetainBody(false);
+ RevFlag isTag = rw.newFlag("tag");
for (Ref ref :
git.getRefDatabase()
.getRefsByPrefixWithExclusions(RefDatabase.ALL, SKIPPABLE_REF_PREFIXES)) {
@@ -218,9 +222,9 @@
} else if (isTag(ref)) {
// For a tag, remember where it points to.
try {
- addTag(rw, git.getRefDatabase().peel(ref));
+ addTag(rw, git.getRefDatabase().peel(ref), isTag);
} catch (IOException e) {
- addTag(rw, ref);
+ addTag(rw, ref, isTag);
}
} else {
@@ -229,17 +233,10 @@
}
}
- // Traverse the complete history. Copy any flags from a commit to
- // all of its ancestors. This automatically updates any Tag object
- // as the TagCommit and the stored Tag object share the same
- // underlying bit set.
+ // Traverse the complete history and propagate reachability to parents.
TagCommit c;
while ((c = (TagCommit) rw.next()) != null) {
- RoaringBitmap mine = c.refFlags;
- int pCnt = c.getParentCount();
- for (int pIdx = 0; pIdx < pCnt; pIdx++) {
- ((TagCommit) c.getParent(pIdx)).refFlags.or(mine);
- }
+ c.propagateReachabilityToParents(isTag);
}
} catch (IOException e) {
logger.atWarning().withCause(e).log("Error building tags for repository %s", projectName);
@@ -356,9 +353,7 @@
refs.putAll(old.refs);
for (Tag srcTag : old.tags) {
- RoaringBitmap mine = new RoaringBitmap();
- mine.or(srcTag.refFlags);
- tags.add(new Tag(srcTag, mine));
+ tags.add(new Tag(srcTag));
}
for (TagMatcher.LostRef lost : m.lostRefs) {
@@ -369,7 +364,7 @@
}
}
- private void addTag(TagWalk rw, Ref ref) {
+ private void addTag(TagWalk rw, Ref ref, RevFlag isTag) {
ObjectId id = ref.getPeeledObjectId();
if (id == null) {
id = ref.getObjectId();
@@ -378,7 +373,12 @@
if (!tags.contains(id)) {
RoaringBitmap flags;
try {
- flags = ((TagCommit) rw.parseCommit(id)).refFlags;
+ TagCommit commit = ((TagCommit) rw.parseCommit(id));
+ commit.add(isTag);
+ if (commit.refFlags == null) {
+ commit.refFlags = new RoaringBitmap();
+ }
+ flags = commit.refFlags;
} catch (IncorrectObjectTypeException notCommit) {
flags = new RoaringBitmap();
} catch (IOException e) {
@@ -395,6 +395,9 @@
rw.markStart(commit);
int flag = refs.size();
+ if (commit.refFlags == null) {
+ commit.refFlags = new RoaringBitmap();
+ }
commit.refFlags.add(flag);
refs.put(ref.getName(), new CachedRef(ref, flag));
} catch (IncorrectObjectTypeException notCommit) {
@@ -432,8 +435,13 @@
// RoaringBitmap in TagCommit.refFlags.
@VisibleForTesting final RoaringBitmap refFlags;
+ Tag(Tag src) {
+ this(src, src.refFlags.clone());
+ }
+
Tag(AnyObjectId id, RoaringBitmap flags) {
super(id);
+ checkNotNull(flags);
this.refFlags = flags;
}
@@ -485,13 +493,50 @@
}
// TODO(hanwen): this would be better named as CommitWithReachability, as it also holds non-tags.
+ // However, non-tags will have a null refFlags field.
private static final class TagCommit extends RevCommit {
/** CachedRef.flag => isVisible, indicating if this commit is reachable from the ref. */
- final RoaringBitmap refFlags;
+ RoaringBitmap refFlags;
TagCommit(AnyObjectId id) {
super(id);
- refFlags = new RoaringBitmap();
+ }
+
+ /**
+ * Copy any flags from this commit to all of its ancestors.
+ *
+ * <p>Do not maintain a reference to the flags on non-tag commits after copying their flags to
+ * their ancestors. The flag copying automatically updates any Tag object as the TagCommit and
+ * the stored Tag object share the same underlying RoaringBitmap.
+ *
+ * @param isTag {@code RevFlag} indicating if this TagCommit is a tag
+ */
+ void propagateReachabilityToParents(RevFlag isTag) {
+ RoaringBitmap mine = refFlags;
+ if (mine != null) {
+ boolean canMoveBitmap = false;
+ if (!has(isTag)) {
+ refFlags = null;
+ canMoveBitmap = true;
+ }
+ int pCnt = getParentCount();
+ for (int pIdx = 0; pIdx < pCnt; pIdx++) {
+ TagCommit commit = (TagCommit) getParent(pIdx);
+ RoaringBitmap parentFlags = commit.refFlags;
+ if (parentFlags == null) {
+ if (canMoveBitmap) {
+ // This commit is not itself a Tag, so in order to reduce cloning overhead, migrate
+ // its refFlags object to its first parent with null refFlags
+ commit.refFlags = mine;
+ canMoveBitmap = false;
+ } else {
+ commit.refFlags = mine.clone();
+ }
+ } else {
+ parentFlags.or(mine);
+ }
+ }
+ }
}
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 6d4d74d..14e40bd 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -23,10 +23,12 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.DraftCommentsReader;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.change.HashtagsUtil;
import com.google.gerrit.server.index.change.ChangeField;
+import com.google.inject.ImplementedBy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
@@ -61,12 +63,25 @@
return new ChangeIndexPredicate(ChangeField.COMMENTBY_SPEC, id.toString());
}
+ @ImplementedBy(IndexEditByPredicateProvider.class)
+ public interface EditByPredicateProvider {
+
+ /**
+ * Returns a predicate that matches changes where the provided {@link
+ * com.google.gerrit.entities.Account.Id} has a pending change edit.
+ */
+ Predicate<ChangeData> editBy(Account.Id id) throws QueryParseException;
+ }
+
/**
- * Returns a predicate that matches changes where the provided {@link
- * com.google.gerrit.entities.Account.Id} has a pending change edit.
+ * A default implementation of {@link EditByPredicateProvider}, based on th {@link
+ * ChangeField#EDITBY_SPEC} index field.
*/
- public static Predicate<ChangeData> editBy(Account.Id id) {
- return new ChangeIndexPredicate(ChangeField.EDITBY_SPEC, id.toString());
+ public static class IndexEditByPredicateProvider implements EditByPredicateProvider {
+ @Override
+ public Predicate<ChangeData> editBy(Account.Id id) {
+ return new ChangeIndexPredicate(ChangeField.EDITBY_SPEC, id.toString());
+ }
}
/**
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index b3fa087..f8a4a99 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -89,6 +89,7 @@
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.ChildProjects;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.query.change.ChangePredicates.EditByPredicateProvider;
import com.google.gerrit.server.query.change.PredicateArgs.ValOp;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.submit.SubmitDryRun;
@@ -284,6 +285,8 @@
private final Provider<CurrentUser> self;
+ private final EditByPredicateProvider editByPredicateProvider;
+
@Inject
@VisibleForTesting
public Arguments(
@@ -318,7 +321,8 @@
ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
- PluginSetContext<SubmitRule> submitRules) {
+ PluginSetContext<SubmitRule> submitRules,
+ EditByPredicateProvider editByPredicateProvider) {
this(
queryProvider,
rewriter,
@@ -352,7 +356,8 @@
experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
- submitRules);
+ submitRules,
+ editByPredicateProvider);
}
private Arguments(
@@ -388,7 +393,8 @@
ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
- PluginSetContext<SubmitRule> submitRules) {
+ PluginSetContext<SubmitRule> submitRules,
+ EditByPredicateProvider editByPredicateProvider) {
this.queryProvider = queryProvider;
this.rewriter = rewriter;
this.opFactories = opFactories;
@@ -422,6 +428,7 @@
this.experimentFeatures = experimentFeatures;
this.hasOperandAliasConfig = hasOperandAliasConfig;
this.submitRules = submitRules;
+ this.editByPredicateProvider = editByPredicateProvider;
}
public Arguments asUser(CurrentUser otherUser) {
@@ -458,7 +465,8 @@
experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
- submitRules);
+ submitRules,
+ editByPredicateProvider);
}
Arguments asUser(Account.Id otherId) {
@@ -646,7 +654,7 @@
}
if ("edit".equalsIgnoreCase(value)) {
- return ChangePredicates.editBy(self());
+ return this.args.editByPredicateProvider.editBy(self());
}
if ("attention".equalsIgnoreCase(value)) {
@@ -1847,7 +1855,7 @@
}
/** Returns {@link com.google.gerrit.entities.Account.Id} of the identified calling user. */
- public Account.Id self() throws QueryParseException {
+ private Account.Id self() throws QueryParseException {
return args.getIdentifiedUser().getAccountId();
}
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 3e471fb..5993c76 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.index.query.Predicate.and;
import static com.google.gerrit.index.query.Predicate.not;
import static com.google.gerrit.index.query.Predicate.or;
+import static com.google.gerrit.server.query.change.ChangePredicates.EditByPredicateProvider;
import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open;
import com.google.common.annotations.VisibleForTesting;
@@ -35,6 +36,7 @@
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.query.InternalQuery;
import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -75,8 +77,8 @@
return ChangeStatusPredicate.forStatus(status);
}
- private static Predicate<ChangeData> editBy(Account.Id accountId) {
- return ChangePredicates.editBy(accountId);
+ private Predicate<ChangeData> editBy(Account.Id accountId) throws QueryParseException {
+ return editByPredicateProvider.editBy(accountId);
}
private static Predicate<ChangeData> commit(String id) {
@@ -85,6 +87,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
+ private final EditByPredicateProvider editByPredicateProvider;
@Inject
InternalChangeQuery(
@@ -92,10 +95,12 @@
ChangeIndexCollection indexes,
IndexConfig indexConfig,
ChangeData.Factory changeDataFactory,
- ChangeNotes.Factory notesFactory) {
+ ChangeNotes.Factory notesFactory,
+ EditByPredicateProvider editByPredicateProvider) {
super(queryProcessor, indexes, indexConfig);
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
+ this.editByPredicateProvider = editByPredicateProvider;
}
public List<ChangeData> byKey(Change.Key key) {
@@ -221,7 +226,7 @@
return query(and(ChangePredicates.exactTopic(topic), open()));
}
- public List<ChangeData> byOpenEditByUser(Account.Id accountId) {
+ public List<ChangeData> byOpenEditByUser(Account.Id accountId) throws QueryParseException {
return query(editBy(accountId));
}
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteAccount.java b/java/com/google/gerrit/server/restapi/account/DeleteAccount.java
index 9b4c0a6..0831ff9 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteAccount.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteAccount.java
@@ -26,6 +26,7 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.gpg.PublicKeyStoreUtil;
+import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -166,7 +167,7 @@
}
}
- private void deleteChangeEdits(Account.Id accountId) throws IOException {
+ private void deleteChangeEdits(Account.Id accountId) throws IOException, QueryParseException {
// Note: in case of a stale index, the results of this query might be incomplete.
List<ChangeData> changesWithEdits = queryProvider.get().byOpenEditByUser(accountId);
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 8b99e1c..677279e 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -175,7 +175,8 @@
null,
null,
null,
- null);
+ null,
+ Optional.empty());
}
/**
@@ -206,7 +207,17 @@
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
return cherryPick(
- sourceChange, project, sourceCommit, input, dest, TimeUtil.now(), null, null, null, null);
+ sourceChange,
+ project,
+ sourceCommit,
+ input,
+ dest,
+ TimeUtil.now(),
+ null,
+ null,
+ null,
+ null,
+ Optional.empty());
}
/**
@@ -228,6 +239,9 @@
* @param idForNewChange The ID that the new change of the cherry pick will have. If provided and
* the cherry-pick doesn't result in creating a new change, then
* InvalidChangeOperationException is thrown.
+ * @param verifiedBaseCommit - base commit for the cherry-pick, which is guaranteed to be
+ * associated with exactly one change and belong to a {@code dest} branch. This is currently
+ * only used when this base commit was created in the same API call.
* @return Result object that describes the cherry pick.
* @throws IOException Unable to open repository or read from the database.
* @throws InvalidChangeOperationException Parent or branch don't exist, or two changes with same
@@ -248,7 +262,8 @@
@Nullable Change.Id revertedChange,
@Nullable ObjectId changeIdForNewChange,
@Nullable Change.Id idForNewChange,
- @Nullable Boolean workInProgress)
+ @Nullable Boolean workInProgress,
+ Optional<RevCommit> verifiedBaseCommit)
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
IdentifiedUser identifiedUser = user.get();
@@ -266,8 +281,9 @@
}
RevCommit baseCommit =
- CommitUtil.getBaseCommit(
- project.get(), queryProvider.get(), revWalk, destRef, input.base);
+ verifiedBaseCommit.orElse(
+ CommitUtil.getBaseCommit(
+ project.get(), queryProvider.get(), revWalk, destRef, input.base));
CodeReviewCommit commitToCherryPick = revWalk.parseCommit(sourceCommit);
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 691fc75..3851e82 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -86,6 +86,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
+import java.util.Optional;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -310,6 +311,13 @@
cherryPickInput.message = revertInput.message;
ObjectId generatedChangeId = CommitMessageUtil.generateChangeId();
Change.Id cherryPickRevertChangeId = Change.id(seq.nextChangeId());
+ RevCommit baseCommit = null;
+ if (cherryPickInput.base != null) {
+ try (Repository git = repoManager.openRepository(changeNotes.getProjectName());
+ RevWalk revWalk = new RevWalk(git.newObjectReader())) {
+ baseCommit = revWalk.parseCommit(ObjectId.fromString(cherryPickInput.base));
+ }
+ }
try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
try (BatchUpdate bu = updateFactory.create(project, user.get(), TimeUtil.now())) {
bu.setNotify(
@@ -323,7 +331,8 @@
generatedChangeId,
cherryPickRevertChangeId,
timestamp,
- revertInput.workInProgress));
+ revertInput.workInProgress,
+ baseCommit));
if (!revertInput.workInProgress) {
commitUtil.addChangeRevertedNotificationOps(
bu, changeNotes.getChangeId(), cherryPickRevertChangeId, generatedChangeId.name());
@@ -548,18 +557,21 @@
private final Change.Id cherryPickRevertChangeId;
private final Instant timestamp;
private final boolean workInProgress;
+ private final RevCommit baseCommit;
CreateCherryPickOp(
ObjectId revCommitId,
ObjectId computedChangeId,
Change.Id cherryPickRevertChangeId,
Instant timestamp,
- Boolean workInProgress) {
+ Boolean workInProgress,
+ RevCommit baseCommit) {
this.revCommitId = revCommitId;
this.computedChangeId = computedChangeId;
this.cherryPickRevertChangeId = cherryPickRevertChangeId;
this.timestamp = timestamp;
this.workInProgress = workInProgress;
+ this.baseCommit = baseCommit;
}
@Override
@@ -577,7 +589,8 @@
change.getId(),
computedChangeId,
cherryPickRevertChangeId,
- workInProgress);
+ workInProgress,
+ Optional.ofNullable(baseCommit));
// save the commit as base for next cherryPick of that branch
cherryPickInput.base =
changeNotesFactory
diff --git a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
index 7d3fe98..97b08f0 100644
--- a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
+++ b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
@@ -77,7 +77,7 @@
bu.addOp(rsrc.getChange().getId(), opFactory.create(false, input));
if (change.getRevertOf() != null) {
commitUtil.addChangeRevertedNotificationOps(
- bu, change.getRevertOf(), change.getId(), change.getKey().get());
+ bu, change.getRevertOf(), change.getId(), change.getKey().get().substring(1));
}
bu.execute();
return Response.ok();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index c920843..3a835b7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -448,7 +448,7 @@
List<ChangeMessageInfo> sourceMessages =
new ArrayList<>(gApi.changes().id(r.getChangeId()).get().messages);
assertThat(sourceMessages).hasSize(4);
- String expectedMessage = String.format("Created a revert of this change as I%s", changeId);
+ String expectedMessage = String.format("Created a revert of this change as %s", changeId);
assertThat(sourceMessages.get(3).message).isEqualTo(expectedMessage);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index 4855ba4..7c50e93 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -225,6 +225,12 @@
List<ChangeMessageInfo> sourceMessages =
new ArrayList<>(gApi.changes().id(r.getChangeId()).get().messages);
assertThat(sourceMessages).hasSize(3);
+ // Publishing creates a revert message
+ gApi.changes().id(revertChange.changeId).setReadyForReview();
+ sourceMessages = new ArrayList<>(gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(sourceMessages).hasSize(4);
+ assertThat(sourceMessages.get(3).message)
+ .isEqualTo("Created a revert of this change as " + revertChange.changeId);
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 9e85d8c..c0531e5 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -875,6 +875,47 @@
"refs/tags/new-tag");
}
+ // rcMaster (c1 master master-tag) <- rcBranch (c2 branch branch-tag) <- rcBranch (c2 branch) <-
+ // newcommit1 <- newcommit2 (new-branch)
+ @Test
+ public void uploadPackReachableTagVisibleFromLeafBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ // rcBranch (c2 branch) <- newcommit1 (new-branch)
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), testRepo)
+ .setParent(rcBranch)
+ .to("refs/heads/new-branch");
+ r.assertOkStatus();
+ RevCommit branchRc = r.getCommit();
+
+ // rcBranch (c2) <- newcommit1 <- newcommit2 (new-branch)
+ r =
+ pushFactory
+ .create(admin.newIdent(), testRepo)
+ .setParent(branchRc)
+ .to("refs/heads/new-branch");
+ r.assertOkStatus();
+ }
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(deny(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(deny(Permission.READ).ref("refs/heads/branch").group(REGISTERED_USERS))
+ .add(allow(Permission.READ).ref("refs/heads/new-branch").group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+ assertUploadPackRefs(
+ "refs/heads/new-branch",
+ // 'master' and 'branch' branches are not visible but 'master-tag' and 'branch-tag' are
+ // reachable from new-branch (since PushOneCommit always bases changes on each other).
+ "refs/tags/branch-tag",
+ "refs/tags/master-tag");
+ // tree-tag not visible. See comment in subsetOfBranchesVisibleIncludingHead.
+ }
+
// first ls-remote: rcBranch (c2 branch) <- newcommit1 (updated-tag)
// second ls-remote: rcBranch (c2 branch updated-tag)
@Test
diff --git a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
index ea8e0a7..90a9b9d 100644
--- a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
+++ b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
@@ -59,6 +59,7 @@
null,
null,
null,
+ null,
null));
}
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
index eef2a44..04887ad 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
@@ -74,8 +74,6 @@
@customElement('gr-admin-view')
export class GrAdminView extends LitElement {
- private account?: AccountDetailInfo;
-
@state()
view?: GerritView;
@@ -459,13 +457,18 @@
async reload() {
try {
this.reloading = true;
+ // There is async barrier inside reload function, we need to clear
+ // subsectionLinks now, because the element might render while waiting for
+ // RestApi responses breaking the invariant that this.view is part of
+ // subsectionLinks if non-empty.
+ this.subsectionLinks = [];
const promises: [Promise<AccountDetailInfo | undefined>, Promise<void>] =
[
this.restApiService.getAccount(),
this.getPluginLoader().awaitPluginsLoaded(),
];
const result = await Promise.all(promises);
- this.account = result[0];
+ const account = result[0];
let options: AdminNavLinksOption | undefined = undefined;
if (this.repoName) {
options = {repoName: this.repoName};
@@ -484,7 +487,7 @@
}
const res = await getAdminLinks(
- this.account,
+ account,
() =>
this.restApiService.getAccountCapabilities().then(capabilities => {
if (!capabilities) {
@@ -501,7 +504,6 @@
: '';
if (!res.expandedSection) {
- this.subsectionLinks = [];
return;
}
this.subsectionLinks = [res.expandedSection]
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
index ee4a179..8cf4e05 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
@@ -111,10 +111,10 @@
assert.isNotOk(element.filteredLinks![0].subsection);
// Groups
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![1].subsection);
// Plugins
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![2].subsection);
});
test('filteredLinks non admin authenticated', async () => {
@@ -123,7 +123,7 @@
// Repos
assert.isNotOk(element.filteredLinks![0].subsection);
// Groups
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![1].subsection);
});
test('filteredLinks non admin unathenticated', async () => {
@@ -262,6 +262,7 @@
test('Needs reload when changing from repo to group', async () => {
element.repoName = 'Test Repo' as RepoName;
+ element.view = GerritView.REPO;
stubRestApi('getAccount').returns(
Promise.resolve({
name: 'test-user',
@@ -278,9 +279,12 @@
const groupId = '1' as GroupId;
element.view = GerritView.GROUP;
element.groupViewState = {groupId, view: GerritView.GROUP};
+ // Check for reload before update. This would normally be done as part of
+ // subscribe method that updates the view/viewState.
+ assert.isTrue(element.needsReload());
+ element.reload();
await element.updateComplete;
- assert.isTrue(element.needsReload());
assert.equal(element.groupId, groupId);
});
diff --git a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
index 57f9ee6..3f99416 100644
--- a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
@@ -117,10 +117,12 @@
return;
}
- this.restApiService.getAccountDetails(userId).then(details => {
- this._accountDetails = details ?? undefined;
- this._status = details?.status ?? '';
- });
+ this.restApiService
+ .getAccountDetails(userId, () => {})
+ .then(details => {
+ this._accountDetails = details ?? undefined;
+ this._status = details?.status ?? '';
+ });
}
_computeDetail(
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 57bb875..aa569ad 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
@@ -1025,23 +1025,17 @@
}
private actionsChanged() {
- this.hidden =
- Object.keys(this.actions).length === 0 &&
- Object.keys(this.revisionActions).length === 0 &&
- this.additionalActions.length === 0;
this.actionLoadingMessage = '';
this.disabledMenuActions = [];
- if (Object.keys(this.revisionActions).length !== 0) {
- if (!this.revisionActions.download) {
- this.revisionActions = {
- ...this.revisionActions,
- download: DOWNLOAD_ACTION,
- };
- fire(this, 'revision-actions-changed', {
- value: this.revisionActions,
- });
- }
+ if (!this.revisionActions.download) {
+ this.revisionActions = {
+ ...this.revisionActions,
+ download: DOWNLOAD_ACTION,
+ };
+ fire(this, 'revision-actions-changed', {
+ value: this.revisionActions,
+ });
}
if (
!this.actions.includedIn &&
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 ae60449..f083609 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
@@ -390,7 +390,7 @@
</gr-copy-clipboard>
</div>
<div class="commitActions">
- <gr-change-actions hidden="" id="actions"> </gr-change-actions>
+ <gr-change-actions id="actions"> </gr-change-actions>
</div>
</div>
<h2 class="assistive-tech-only">Change metadata</h2>
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 97b8de3..d6e7336 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
@@ -128,7 +128,7 @@
maxDeleted: number;
maxAdditionWidth: number;
maxDeletionWidth: number;
- deletionOffset: number;
+ additionOffset: number;
}
function createDefaultSizeBarLayout(): SizeBarLayout {
@@ -139,7 +139,7 @@
maxDeleted: 0,
maxAdditionWidth: 0,
maxDeletionWidth: 0,
- deletionOffset: 0,
+ additionOffset: 0,
};
}
@@ -463,13 +463,13 @@
}
.added {
color: var(--positive-green-text-color);
- }
- .removed {
- color: var(--negative-red-text-color);
text-align: left;
min-width: 4em;
padding-left: var(--spacing-s);
}
+ .removed {
+ color: var(--negative-red-text-color);
+ }
.drafts {
color: var(--error-foreground);
font-weight: var(--font-weight-bold);
@@ -1321,19 +1321,19 @@
<div class=${this.computeSizeBarsClass(file.__path)} aria-hidden="true">
<svg width="61" height="8">
<rect
- x=${this.computeBarAdditionX(file, sizeBarLayout)}
- y="0"
- height="8"
- fill="var(--positive-green-text-color)"
- width=${this.computeBarAdditionWidth(file, sizeBarLayout)}
- ></rect>
- <rect
- x=${this.computeBarDeletionX(sizeBarLayout)}
+ x=${this.computeBarDeletionX(file, sizeBarLayout)}
y="0"
height="8"
fill="var(--negative-red-text-color)"
width=${this.computeBarDeletionWidth(file, sizeBarLayout)}
></rect>
+ <rect
+ x=${this.computeBarAdditionX(sizeBarLayout)}
+ y="0"
+ height="8"
+ fill="var(--positive-green-text-color)"
+ width=${this.computeBarAdditionWidth(file, sizeBarLayout)}
+ ></rect>
</svg>
</div>
</div>`;
@@ -1348,14 +1348,6 @@
-->
<div class=${this.computeClass('', file.__path)}>
<span
- class="added"
- tabindex="0"
- aria-label=${`${file.lines_inserted} added`}
- ?hidden=${file.binary}
- >
- +${file.lines_inserted}
- </span>
- <span
class="removed"
tabindex="0"
aria-label=${`${file.lines_deleted} removed`}
@@ -1364,6 +1356,14 @@
-${file.lines_deleted}
</span>
<span
+ class="added"
+ tabindex="0"
+ aria-label=${`${file.lines_inserted} added`}
+ ?hidden=${file.binary}
+ >
+ +${file.lines_inserted}
+ </span>
+ <span
class=${ifDefined(this.computeBinaryClass(file.size_delta))}
?hidden=${!file.binary}
>
@@ -1542,19 +1542,19 @@
<div class="total-stats">
<div>
<span
- class="added"
- tabindex="0"
- aria-label="Total ${patchChange.inserted} lines added"
- >
- +${patchChange.inserted}
- </span>
- <span
class="removed"
tabindex="0"
aria-label="Total ${patchChange.deleted} lines removed"
>
-${patchChange.deleted}
</span>
+ <span
+ class="added"
+ tabindex="0"
+ aria-label="Total ${patchChange.inserted} lines added"
+ >
+ +${patchChange.inserted}
+ </span>
</div>
</div>
${when(showDynamicColumns, () =>
@@ -1586,16 +1586,6 @@
<div class="row totalChanges">
<div class="total-stats">
<span
- class="added"
- aria-label="Total bytes inserted: ${deltaInserted}"
- >
- ${deltaInserted}
- ${this.formatPercentage(
- patchChange.total_size,
- patchChange.size_delta_inserted
- )}
- </span>
- <span
class="removed"
aria-label="Total bytes removed: ${deltaDeleted}"
>
@@ -1605,6 +1595,16 @@
patchChange.size_delta_deleted
)}
</span>
+ <span
+ class="added"
+ aria-label="Total bytes inserted: ${deltaInserted}"
+ >
+ ${deltaInserted}
+ ${this.formatPercentage(
+ patchChange.total_size,
+ patchChange.size_delta_inserted
+ )}
+ </span>
</div>
</div>
`;
@@ -2470,7 +2470,7 @@
(SIZE_BAR_MAX_WIDTH - SIZE_BAR_GAP_WIDTH) * ratio;
stats.maxDeletionWidth =
SIZE_BAR_MAX_WIDTH - SIZE_BAR_GAP_WIDTH - stats.maxAdditionWidth;
- stats.deletionOffset = stats.maxAdditionWidth + SIZE_BAR_GAP_WIDTH;
+ stats.additionOffset = stats.maxDeletionWidth + SIZE_BAR_GAP_WIDTH;
}
return stats;
}
@@ -2498,9 +2498,8 @@
* Get the x-offset of the addition bar for a file.
* Private but used in tests.
*/
- computeBarAdditionX(file?: NormalizedFileInfo, stats?: SizeBarLayout) {
- if (!file || !stats) return;
- return stats.maxAdditionWidth - this.computeBarAdditionWidth(file, stats);
+ computeBarAdditionX(stats: SizeBarLayout) {
+ return stats.additionOffset;
}
/**
@@ -2524,9 +2523,11 @@
/**
* Get the x-offset of the deletion bar for a file.
+ * Private but used in tests.
*/
- private computeBarDeletionX(stats: SizeBarLayout) {
- return stats.deletionOffset;
+ computeBarDeletionX(file?: NormalizedFileInfo, stats?: SizeBarLayout) {
+ if (!file || !stats) return;
+ return stats.maxDeletionWidth - this.computeBarDeletionWidth(file, stats);
}
// Private but used in tests.
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
index ab9d038..b2cd430 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -76,7 +76,6 @@
suite('gr-file-list tests', () => {
let element: GrFileList;
-
let saveStub: sinon.SinonStub;
suite('basic tests', async () => {
@@ -106,11 +105,9 @@
element.numFilesShown = 200;
element.basePatchNum = PARENT;
element.patchNum = 2 as RevisionPatchSetNum;
- saveStub = sinon
- .stub(element, '_saveReviewedState')
- .callsFake(() => Promise.resolve());
- await element.updateComplete;
+ saveStub = sinon.stub(element, '_saveReviewedState').resolves();
element.showSizeBars = true;
+ await element.updateComplete;
// Wait for expandedFilesChanged to complete.
await waitEventLoop();
});
@@ -207,14 +204,16 @@
</div>
</div>
<div class="desktop" role="gridcell">
- <div aria-hidden="true" class="sizeBars"></div>
+ <div aria-hidden="true" class="sizeBars">
+ <svg><!-- contents asserted separately below --></svg>
+ </div>
</div>
<div class="stats" role="gridcell">
<div>
- <span aria-label="9 added" class="added" tabindex="0"> +9 </span>
<span aria-label="0 removed" class="removed" tabindex="0">
-0
</span>
+ <span aria-label="9 added" class="added" tabindex="0"> +9 </span>
<span hidden=""> +/-0 B </span>
</div>
</div>
@@ -262,6 +261,31 @@
</div>
</div>`
);
+ // <svg> and contents are ignored by assert.dom.equal() above, so we need
+ // a separate assert.lightDom.equal() for it here.
+ const sizeBarsSVG = queryAndAssert<SVGSVGElement>(
+ element,
+ '.sizeBars > svg'
+ );
+ assert.lightDom.equal(
+ sizeBarsSVG,
+ /* HTML */ `
+ <rect
+ height="8"
+ width="0"
+ x="0"
+ y="0"
+ fill="var(--negative-red-text-color)"
+ ></rect>
+ <rect
+ height="8"
+ width="60"
+ x="1"
+ y="0"
+ fill="var(--positive-green-text-color)"
+ ></rect>
+ `
+ );
});
test('renders file paths', async () => {
@@ -1763,7 +1787,7 @@
maxDeleted: 0,
maxAdditionWidth: 0,
maxDeletionWidth: 0,
- deletionOffset: 0,
+ additionOffset: 0,
};
element.files = [];
@@ -1811,7 +1835,7 @@
maxDeleted: 0,
maxAdditionWidth: 60,
maxDeletionWidth: 0,
- deletionOffset: 60,
+ additionOffset: 60,
};
// Uses half the space when file is half the largest addition and there
@@ -1839,22 +1863,33 @@
assert.equal(element.computeBarAdditionWidth(file, stats), 1.5);
});
- test('_computeBarAdditionX', () => {
+ test('computeBarDeletionX', () => {
const file = {
__path: 'foo/bar.baz',
- lines_inserted: 5,
- lines_deleted: 0,
+ lines_inserted: 0,
+ lines_deleted: 5,
size: 0,
size_delta: 0,
};
const stats = {
+ maxInserted: 0,
+ maxDeleted: 10,
+ maxAdditionWidth: 0,
+ maxDeletionWidth: 60,
+ additionOffset: 60,
+ };
+ assert.equal(element.computeBarDeletionX(file, stats), 30);
+ });
+
+ test('computeBarAdditionX', () => {
+ const stats = {
maxInserted: 10,
maxDeleted: 0,
maxAdditionWidth: 60,
maxDeletionWidth: 0,
- deletionOffset: 60,
+ additionOffset: 60,
};
- assert.equal(element.computeBarAdditionX(file, stats), 30);
+ assert.equal(element.computeBarAdditionX(stats), 60);
});
test('computeBarDeletionWidth', () => {
@@ -1870,7 +1905,7 @@
maxDeleted: 10,
maxAdditionWidth: 30,
maxDeletionWidth: 30,
- deletionOffset: 31,
+ additionOffset: 31,
};
// Uses a quarter the space when file is half the largest deletions and
@@ -1898,7 +1933,7 @@
assert.equal(element.computeBarDeletionWidth(file, stats), 1.5);
});
- test('_computeSizeBarsClass', () => {
+ test('computeSizeBarsClass', () => {
element.showSizeBars = false;
assert.equal(
element.computeSizeBarsClass('foo/bar.baz'),
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 9225ce8..8ae1c36 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
@@ -330,6 +330,12 @@
newAttentionSet: Set<UserId> = new Set();
@state()
+ manuallyAddedAttentionSet: Set<UserId> = new Set();
+
+ @state()
+ manuallyDeletedAttentionSet: Set<UserId> = new Set();
+
+ @state()
patchsetLevelDraftIsResolved = true;
@state()
@@ -477,12 +483,14 @@
#pluginMessage:empty {
display: none;
}
- .attention .edit-attention-button {
+ .edit-attention-button {
vertical-align: top;
--gr-button-padding: 0px 4px;
}
- .attention .edit-attention-button gr-icon {
+ .edit-attention-button gr-icon {
color: inherit;
+
+ .attentionSummary .edit-attention-button gr-icon {
/* The line-height:26px hack (see below) requires us to do this.
Normally the gr-icon would account for a proper positioning
within the standard line-height:20px context. */
@@ -999,30 +1007,9 @@
)}
`
)}
- <gr-tooltip-content
- has-tooltip
- title=${this.computeAttentionButtonTitle()}
- >
- <gr-button
- class="edit-attention-button"
- @click=${this.handleAttentionModify}
- ?disabled=${this.isSendDisabled()}
- link
- position-below
- data-label="Edit"
- data-action-type="change"
- data-action-key="edit"
- role="button"
- tabindex="0"
- >
- <div>
- <gr-icon icon="edit" filled small></gr-icon>
- <span>Modify</span>
- </div>
- </gr-button>
- </gr-tooltip-content>
</div>
<div>
+ ${this.renderModifyAttentionSetButton()}
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1036,6 +1023,30 @@
`;
}
+ private renderModifyAttentionSetButton() {
+ return html` <gr-button
+ class="edit-attention-button"
+ @click=${this.toggleAttentionModify}
+ ?disabled=${this.isSendDisabled()}
+ link
+ position-below
+ data-label="Edit"
+ data-action-type="change"
+ data-action-key="edit"
+ role="button"
+ tabindex="0"
+ >
+ <div>
+ <gr-icon
+ icon=${this.attentionExpanded ? 'expand_circle_up' : 'edit'}
+ filled
+ small
+ ></gr-icon>
+ <span>${this.attentionExpanded ? 'Collapse' : 'Modify'}</span>
+ </div>
+ </gr-button>`;
+ }
+
private renderAttentionDetailsSection() {
if (!this.attentionExpanded) return;
return html`
@@ -1044,8 +1055,10 @@
<div>
<span>Modify attention to</span>
</div>
+
<div></div>
<div>
+ ${this.renderModifyAttentionSetButton()}
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1137,7 +1150,7 @@
`
)}
${when(
- this.computeShowAttentionTip(),
+ this.computeShowAttentionTip(3),
() => html`
<div class="attentionTip">
<gr-icon icon="lightbulb"></gr-icon>
@@ -1535,8 +1548,8 @@
this.reviewers = getAccounts(ReviewerState.REVIEWER);
}
- handleAttentionModify() {
- this.attentionExpanded = true;
+ toggleAttentionModify() {
+ this.attentionExpanded = !this.attentionExpanded;
}
onAttentionExpandedChange() {
@@ -1545,13 +1558,6 @@
fire(this, 'iron-resize', {});
}
- computeAttentionButtonTitle() {
- return this.isSendDisabled()
- ? 'Modify the attention set by adding a comment or use the account ' +
- 'hovercard in the change page.'
- : 'Edit attention set changes';
- }
-
handleAttentionClick(e: Event) {
const targetAccount = (e.target as GrAccountChip)?.account;
if (!targetAccount) return;
@@ -1563,11 +1569,15 @@
if (this.newAttentionSet.has(id)) {
this.newAttentionSet.delete(id);
+ this.manuallyAddedAttentionSet.delete(id);
+ this.manuallyDeletedAttentionSet.add(id);
this.reporting.reportInteraction(Interaction.ATTENTION_SET_CHIP, {
action: `REMOVE${self}${role}`,
});
} else {
this.newAttentionSet.add(id);
+ this.manuallyAddedAttentionSet.add(id);
+ this.manuallyDeletedAttentionSet.delete(id);
this.reporting.reportInteraction(Interaction.ATTENTION_SET_CHIP, {
action: `ADD${self}${role}`,
});
@@ -1657,18 +1667,24 @@
.map(a => getUserId(a))
.filter(id => !!id);
this.newAttentionSet = new Set([
- ...[...newAttention].filter(id => allAccountIds.includes(id)),
+ ...this.manuallyAddedAttentionSet,
+ ...[...newAttention].filter(
+ id =>
+ allAccountIds.includes(id) &&
+ !this.manuallyDeletedAttentionSet.has(id)
+ ),
]);
-
- this.attentionExpanded = this.computeShowAttentionTip();
+ // Possibly expand if need be, never collapse as this is jarring to the user.
+ this.attentionExpanded =
+ this.attentionExpanded || this.computeShowAttentionTip(1);
}
- computeShowAttentionTip() {
+ computeShowAttentionTip(minimum: number) {
if (!this.currentAttentionSet || !this.newAttentionSet) return false;
const addedIds = [...this.newAttentionSet].filter(
id => !this.currentAttentionSet.has(id)
);
- return this.isOwner && addedIds.length > 2;
+ return this.isOwner && addedIds.length >= minimum;
}
computeCommentAccounts(threads: CommentThread[]) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 3c3c246..5766d80 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -255,30 +255,25 @@
<div class="attentionSummary">
<div>
<span> No changes to the attention set. </span>
- <gr-tooltip-content
- has-tooltip=""
- title="Modify the attention set by adding a comment or use the account hovercard in the change page."
- >
- <gr-button
- aria-disabled="true"
- disabled=""
- class="edit-attention-button"
- data-action-key="edit"
- data-action-type="change"
- data-label="Edit"
- link=""
- position-below=""
- role="button"
- tabindex="-1"
- >
- <div>
- <gr-icon icon="edit" filled small></gr-icon>
- <span>Modify</span>
- </div>
- </gr-button>
- </gr-tooltip-content>
</div>
<div>
+ <gr-button
+ aria-disabled="true"
+ disabled=""
+ class="edit-attention-button"
+ data-action-key="edit"
+ data-action-type="change"
+ data-label="Edit"
+ link=""
+ position-below=""
+ role="button"
+ tabindex="-1"
+ >
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Modify</span>
+ </div>
+ </gr-button>
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1732,19 +1727,17 @@
element._ccs = [...element.ccs, makeAccount()];
await element.updateComplete;
- // The 'attention modified' section collapses and resets when reviewers or
- // ccs change.
- assert.isFalse(element.attentionExpanded);
-
- queryAndAssert<GrButton>(element, '.edit-attention-button').click();
- await element.updateComplete;
-
assert.isTrue(element.attentionExpanded);
accountLabels = Array.from(
queryAll(element, '.attention-detail gr-account-label')
);
assert.equal(accountLabels.length, 7);
+ // Verify that toggling the attention-set-button collapses.
+ queryAndAssert<GrButton>(element, '.edit-attention-button').click();
+ await element.updateComplete;
+ assert.isFalse(element.attentionExpanded);
+
element.reviewers.pop();
element.reviewers.pop();
element._ccs.pop();
@@ -2556,6 +2549,59 @@
});
});
+ test('manually added users are not lost when view updates.', async () => {
+ assert.sameMembers([...element.newAttentionSet], []);
+
+ element.reviewers = [
+ createAccountWithId(1),
+ createAccountWithId(2),
+ createAccountWithId(3),
+ ];
+ element.patchsetLevelDraftMessage = 'abc';
+
+ await element.updateComplete;
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+
+ const modifyButton = queryAndAssert<GrButton>(
+ element,
+ '.edit-attention-button'
+ );
+
+ modifyButton.click();
+ assert.isTrue(element.attentionExpanded);
+ await element.updateComplete;
+
+ const accountsChips = Array.from(
+ queryAll<GrAccountLabel>(element, '.attention-detail gr-account-label')
+ );
+ assert.equal(accountsChips.length, 4);
+ for (let i = 0; i < 4; ++i) {
+ if (accountsChips[i].account?._account_id === 1) {
+ accountsChips[i].click();
+ break;
+ }
+ }
+
+ await element.updateComplete;
+
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [1 as AccountId, 2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+
+ // Doesn't get reset when message changes.
+ element.patchsetLevelDraftMessage = 'def';
+ await element.updateComplete;
+
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [1 as AccountId, 2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+ });
+
suite('mention users', () => {
setup(async () => {
element.account = createAccountWithId(1);
@@ -2692,6 +2738,13 @@
await element.updateComplete;
assert.sameMembers([...element.newAttentionSet], [999 as AccountId]);
+
+ // Random update
+ element.patchsetLevelDraftMessage = 'abc';
+ await element.updateComplete;
+
+ assert.sameMembers([...element.newAttentionSet], [999 as AccountId]);
+ element.patchsetLevelDraftMessage = 'abc';
});
test('mention user who is already CCed', async () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
index b6af123..3dcba9e 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
@@ -187,8 +187,11 @@
}
protected override willUpdate(changedProperties: PropertyValues): void {
+ if (changedProperties.has('value') || changedProperties.has('items')) {
+ this.updateText();
+ }
if (changedProperties.has('value')) {
- this.handleValueChange();
+ fireNoBubble(this, 'value-change', {value: this.value});
}
}
@@ -307,7 +310,7 @@
}, 1);
}
- private handleValueChange() {
+ private updateText() {
if (this.value === undefined || this.items === undefined) {
return;
}
@@ -318,7 +321,6 @@
this.text = selectedObj.triggerText
? selectedObj.triggerText
: selectedObj.text;
- fireNoBubble(this, 'value-change', {value: this.value});
}
/**
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.ts
index 70e653a..e557ca8 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.ts
@@ -147,20 +147,17 @@
test('move action button to overflow', async () => {
const key = changeActions.add(ActionType.REVISION, 'Bork!');
await element.updateComplete;
- assert.isTrue(queryAndAssert<GrDropdown>(element, '#moreActions').hidden);
- assert.isOk(
- queryAndAssert<GrButton>(element, `[data-action-key="${key}"]`)
- );
+
+ let items = queryAndAssert<GrDropdown>(element, '#moreActions').items;
+ assert.isFalse(items?.some(item => item.name === 'Bork!'));
+ assert.isOk(query<GrButton>(element, `[data-action-key="${key}"]`));
+
changeActions.setActionOverflow(ActionType.REVISION, key, true);
await element.updateComplete;
+
+ items = queryAndAssert<GrDropdown>(element, '#moreActions').items;
+ assert.isTrue(items?.some(item => item.name === 'Bork!'));
assert.isNotOk(query<GrButton>(element, `[data-action-key="${key}"]`));
- assert.isFalse(
- queryAndAssert<GrDropdown>(element, '#moreActions').hidden
- );
- assert.strictEqual(
- queryAndAssert<GrDropdown>(element, '#moreActions').items![0].name,
- 'Bork!'
- );
});
test('change actions priority', async () => {
