Merge "Revert "Revert "Get rid of ba-linkify."""
diff --git a/Documentation/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index ba20cea..5ff9425 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -246,6 +246,15 @@
name contains the file pattern, or the edits of the file diff contain the edit
pattern.
+[[operator_label]]
+label:labelName=+1,user=non_contributor::
++
+Submit requirements support an additional `user=non_contributor` argument for
+labels that returns true if the change has a label vote matching the specified
+value and the vote is applied from a gerrit account that's not the uploader,
+author or committer of the latest patchset. See the documentation for the labels
+operator in the link:user-search.html[user search] page.
+
[[unsupported_operators]]
=== Unsupported Operators
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index b761687..75d8847 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6878,9 +6878,6 @@
accounts that were in the attention set but were removed. The
link:#attention-set-info[AttentionSetInfo] is the latest and most recent removal
of the account from the attention set.
-|`assignee` |optional|
-The assignee of the change as an link:rest-api-accounts.html#account-info[
-AccountInfo] entity.
|`hashtags` |optional|
List of hashtags that are set on the change.
|`change_id` ||The Change-Id of the change.
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index da033c1..8a9e56a 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -343,7 +343,6 @@
public final TestAccount reviewer;
public final TestAccount ccer;
public final TestAccount starrer;
- public final TestAccount assignee;
public final TestAccount watchingProjectOwner;
private final Map<NotifyType, TestAccount> watchers = new HashMap<>();
private final Map<String, TestAccount> accountsByEmail = new HashMap<>();
@@ -369,7 +368,6 @@
reviewer = reindexAndCopy(existing.reviewer);
ccer = reindexAndCopy(existing.ccer);
starrer = reindexAndCopy(existing.starrer);
- assignee = reindexAndCopy(existing.assignee);
watchingProjectOwner = reindexAndCopy(existing.watchingProjectOwner);
watchers.putAll(existing.watchers);
return;
@@ -381,7 +379,6 @@
uploader = testAccount("uploader");
ccer = testAccount("ccer");
starrer = testAccount("starrer");
- assignee = testAccount("assignee");
watchingProjectOwner = testAccount("watchingProjectOwner", "Administrators");
requestScopeOperations.setApiUser(watchingProjectOwner.id());
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index 3c7ec2b..4238563 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -53,6 +53,7 @@
"//lib/bouncycastle:bcpg",
"//lib/bouncycastle:bcpkix",
"//lib/bouncycastle:bcprov",
+ "//lib/bouncycastle:bcutil",
"//prolog:gerrit-prolog-common",
]
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 55220f3..56fb748 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -477,9 +477,6 @@
*/
@Nullable private String submissionId;
- /** Allows assigning a change to a user. */
- @Nullable private Account.Id assignee;
-
/** Whether the change is private. */
private boolean isPrivate;
@@ -509,7 +506,6 @@
}
public Change(Change other) {
- assignee = other.assignee;
changeId = other.changeId;
changeKey = other.changeKey;
createdOn = other.createdOn;
@@ -548,14 +544,6 @@
changeKey = k;
}
- public Account.Id getAssignee() {
- return assignee;
- }
-
- public void setAssignee(Account.Id a) {
- assignee = a;
- }
-
public Instant getCreatedOn() {
return createdOn;
}
diff --git a/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java b/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
index 4903364..3b772d0 100644
--- a/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
@@ -71,10 +71,6 @@
if (submissionId != null) {
builder.setSubmissionId(submissionId);
}
- Account.Id assignee = change.getAssignee();
- if (assignee != null) {
- builder.setAssignee(accountIdConverter.toProto(assignee));
- }
Change.Id revertOf = change.getRevertOf();
if (revertOf != null) {
builder.setRevertOf(changeIdConverter.toProto(revertOf));
@@ -114,9 +110,6 @@
if (proto.hasSubmissionId()) {
change.setSubmissionId(proto.getSubmissionId());
}
- if (proto.hasAssignee()) {
- change.setAssignee(accountIdConverter.fromProto(proto.getAssignee()));
- }
change.setPrivate(proto.getIsPrivate());
change.setWorkInProgress(proto.getWorkInProgress());
change.setReviewStarted(proto.getReviewStarted());
diff --git a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
index 1ee2cd8..020351b 100644
--- a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
@@ -134,7 +134,6 @@
public DateFormat dateFormat;
public TimeFormat timeFormat;
public Boolean expandInlineDiffs;
- public Boolean highlightAssigneeInChangeTable;
public Boolean relativeDateInChangeTable;
public DiffView diffView;
public Boolean sizeBarInChangeTable;
@@ -195,7 +194,6 @@
p.dateFormat = DateFormat.STD;
p.timeFormat = TimeFormat.HHMM_12;
p.expandInlineDiffs = false;
- p.highlightAssigneeInChangeTable = true;
p.relativeDateInChangeTable = false;
p.diffView = DiffView.SIDE_BY_SIDE;
p.sizeBarInChangeTable = true;
diff --git a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
index 0142e01..f85279c 100644
--- a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
@@ -17,10 +17,8 @@
/** API response containing values from the {@code change} section of {@code gerrit.config}. */
public class ChangeConfigInfo {
public Boolean allowBlame;
- public Boolean showAssigneeInChangesTable;
public Boolean disablePrivateChanges;
public int updateDelay;
public Boolean submitWholeTopic;
public String mergeabilityComputationBehavior;
- public Boolean enableAssignee;
}
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index a865187..b40e100 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -49,7 +49,6 @@
public Map<Integer, AttentionSetInfo> removedFromAttentionSet;
- public AccountInfo assignee;
public Collection<String> hashtags;
public String changeId;
public String subject;
diff --git a/java/com/google/gerrit/server/change/ActionJson.java b/java/com/google/gerrit/server/change/ActionJson.java
index a9d5959..e5a9534 100644
--- a/java/com/google/gerrit/server/change/ActionJson.java
+++ b/java/com/google/gerrit/server/change/ActionJson.java
@@ -123,7 +123,6 @@
changeInfo.removedFromAttentionSet == null
? null
: ImmutableMap.copyOf(changeInfo.removedFromAttentionSet);
- copy.assignee = changeInfo.assignee;
copy.hashtags = changeInfo.hashtags;
copy.changeId = changeInfo.changeId;
copy.submitType = changeInfo.submitType;
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 912d202..f733a7b 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -617,7 +617,6 @@
a -> a.account().get(),
a -> AttentionSetUtil.createAttentionSetInfo(a, accountLoader)));
}
- out.assignee = in.getAssignee() != null ? accountLoader.get(in.getAssignee()) : null;
out.hashtags = cd.hashtags();
out.changeId = in.getKey().get();
if (in.isNew()) {
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 919586e..c5c0be0 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -177,9 +177,6 @@
byte[] buf = new byte[20];
Set<Account.Id> accounts = new HashSet<>();
accounts.add(getChange().getOwner());
- if (getChange().getAssignee() != null) {
- accounts.add(getChange().getAssignee());
- }
try {
patchSetUtil.byChange(getNotes()).stream().map(PatchSet::uploader).forEach(accounts::add);
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 903a4c0..ad0dd8b 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -34,12 +34,14 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.edit.tree.ChangeFileContentModification;
import com.google.gerrit.server.edit.tree.DeleteFileModification;
import com.google.gerrit.server.edit.tree.RenameFileModification;
import com.google.gerrit.server.edit.tree.RestoreFileModification;
import com.google.gerrit.server.edit.tree.TreeCreator;
import com.google.gerrit.server.edit.tree.TreeModification;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -108,15 +110,15 @@
PermissionBackend permissionBackend,
ChangeEditUtil changeEditUtil,
PatchSetUtil patchSetUtil,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ GitReferenceUpdated gitReferenceUpdated) {
this.currentUser = currentUser;
this.permissionBackend = permissionBackend;
this.zoneId = gerritIdent.getZoneId();
this.changeEditUtil = changeEditUtil;
this.patchSetUtil = patchSetUtil;
this.projectCache = projectCache;
-
- noteDbEdits = new NoteDbEdits(zoneId, indexer, currentUser);
+ noteDbEdits = new NoteDbEdits(gitReferenceUpdated, zoneId, indexer, currentUser);
}
/**
@@ -173,10 +175,14 @@
notes.getChangeId(), currentPatchSet.id()));
}
- rebase(repository, changeEdit, currentPatchSet);
+ rebase(notes.getProjectName(), repository, changeEdit, currentPatchSet);
}
- private void rebase(Repository repository, ChangeEdit changeEdit, PatchSet currentPatchSet)
+ private void rebase(
+ Project.NameKey project,
+ Repository repository,
+ ChangeEdit changeEdit,
+ PatchSet currentPatchSet)
throws IOException, MergeConflictException, InvalidChangeOperationException {
RevCommit currentEditCommit = changeEdit.getEditCommit();
if (currentEditCommit.getParentCount() == 0) {
@@ -194,7 +200,13 @@
createCommit(repository, basePatchSetCommit, newTreeId, commitMessage, nowTimestamp);
noteDbEdits.baseEditOnDifferentPatchset(
- repository, changeEdit, currentPatchSet, currentEditCommit, newEditCommitId, nowTimestamp);
+ project,
+ repository,
+ changeEdit,
+ currentPatchSet,
+ currentEditCommit,
+ newEditCommitId,
+ nowTimestamp);
}
/**
@@ -719,11 +731,17 @@
private final ZoneId zoneId;
private final ChangeIndexer indexer;
private final Provider<CurrentUser> currentUser;
+ private final GitReferenceUpdated gitReferenceUpdated;
- NoteDbEdits(ZoneId zoneId, ChangeIndexer indexer, Provider<CurrentUser> currentUser) {
+ NoteDbEdits(
+ GitReferenceUpdated gitReferenceUpdated,
+ ZoneId zoneId,
+ ChangeIndexer indexer,
+ Provider<CurrentUser> currentUser) {
this.zoneId = zoneId;
this.indexer = indexer;
this.currentUser = currentUser;
+ this.gitReferenceUpdated = gitReferenceUpdated;
}
ChangeEdit createEdit(
@@ -753,6 +771,10 @@
return RefNames.refsEdit(me.getAccountId(), change.getId(), basePatchset.id());
}
+ private AccountState getUpdater() {
+ return currentUser.get().asIdentifiedUser().state();
+ }
+
ChangeEdit updateEdit(
Project.NameKey projectName,
Repository repository,
@@ -795,9 +817,11 @@
throw new IOException(message);
}
}
+ gitReferenceUpdated.fire(projectName, ru, getUpdater());
}
void baseEditOnDifferentPatchset(
+ Project.NameKey project,
Repository repository,
ChangeEdit changeEdit,
PatchSet currentPatchSet,
@@ -807,6 +831,7 @@
throws IOException {
String newEditRefName = getEditRefName(changeEdit.getChange(), currentPatchSet);
updateReferenceWithNameChange(
+ project,
repository,
changeEdit.getRefName(),
currentEditCommit,
@@ -817,6 +842,7 @@
}
private void updateReferenceWithNameChange(
+ Project.NameKey projectName,
Repository repository,
String currentRefName,
ObjectId currentObjectId,
@@ -838,6 +864,7 @@
throw new IOException("failed: " + cmd);
}
}
+ gitReferenceUpdated.fire(projectName, batchRefUpdate, getUpdater());
}
static RevCommit lookupCommit(Repository repository, ObjectId commitId) throws IOException {
diff --git a/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/java/com/google/gerrit/server/edit/ChangeEditUtil.java
index 74834ab..3474590 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditUtil.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -70,6 +71,8 @@
private final ChangeKindCache changeKindCache;
private final PatchSetUtil psUtil;
+ private final GitReferenceUpdated gitReferenceUpdated;
+
@Inject
ChangeEditUtil(
GitRepositoryManager gitManager,
@@ -77,13 +80,15 @@
ChangeIndexer indexer,
Provider<CurrentUser> userProvider,
ChangeKindCache changeKindCache,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ GitReferenceUpdated gitReferenceUpdated) {
this.gitManager = gitManager;
this.patchSetInserterFactory = patchSetInserterFactory;
this.indexer = indexer;
this.userProvider = userProvider;
this.changeKindCache = changeKindCache;
this.psUtil = psUtil;
+ this.gitReferenceUpdated = gitReferenceUpdated;
}
/**
@@ -237,7 +242,7 @@
return writeSquashedCommit(rw, inserter, parent, edit);
}
- private static void deleteRef(Repository repo, ChangeEdit edit) throws IOException {
+ private void deleteRef(Repository repo, ChangeEdit edit) throws IOException {
String refName = edit.getRefName();
RefUpdate ru = repo.updateRef(refName, true);
ru.setExpectedOldObjectId(edit.getEditCommit());
@@ -261,6 +266,10 @@
default:
throw new IOException(String.format("Failed to delete ref %s: %s", refName, result));
}
+ gitReferenceUpdated.fire(
+ edit.getChange().getProject(),
+ ru,
+ /* updater= */ userProvider.get().asIdentifiedUser().state());
}
private static RevCommit writeSquashedCommit(
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 2045dba..8e443f82 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -504,9 +504,9 @@
ATTENTION_SET_FULL_FIELD.storedOnly(ChangeQueryBuilder.FIELD_ATTENTION_SET_FULL);
/** The user assigned to the change. */
+ // The getter always returns NO_ASSIGNEE, since assignee field is deprecated.
public static final IndexedField<ChangeData, Integer> ASSIGNEE_FIELD =
- IndexedField.<ChangeData>integerBuilder("Assignee")
- .build(changeGetter(c -> c.getAssignee() != null ? c.getAssignee().get() : NO_ASSIGNEE));
+ IndexedField.<ChangeData>integerBuilder("Assignee").build(changeGetter(c -> NO_ASSIGNEE));
public static final IndexedField<ChangeData, Integer>.SearchSpec ASSIGNEE_SPEC =
ASSIGNEE_FIELD.integer(ChangeQueryBuilder.FIELD_ASSIGNEE);
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 895c4d8..6ddf7a3 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -232,12 +232,21 @@
.build();
/** Add prefixsubject field. */
+ @Deprecated
static final Schema<ChangeData> V81 =
new Schema.Builder<ChangeData>()
.add(V80)
.addSearchSpecs(ChangeField.PREFIX_SUBJECT_SPEC)
.build();
+ /** Remove assignee field. */
+ static final Schema<ChangeData> V82 =
+ new Schema.Builder<ChangeData>()
+ .add(V81)
+ .remove(ChangeField.ASSIGNEE_SPEC)
+ .remove(ChangeField.ASSIGNEE_FIELD)
+ .build();
+
/**
* Name of the change index to be used when contacting index backends or loading configurations.
*/
diff --git a/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java b/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
index ce54708..0eaafb8 100644
--- a/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
+++ b/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
@@ -90,7 +90,7 @@
"RevertedHtml.soy",
};
- private static final SoySauce DEFAULT = getDefault().build().compileTemplates();
+ private static final SoySauce DEFAULT = getDefault(null).build().compileTemplates();
private final SitePaths site;
private final PluginSetContext<MailSoyTemplateProvider> templateProviders;
@@ -106,7 +106,7 @@
return DEFAULT;
}
- SoyFileSet.Builder builder = getDefault();
+ SoyFileSet.Builder builder = getDefault(site);
templateProviders.runEach(
e -> e.getFileNames().forEach(p -> addTemplate(builder, site, e.getPath(), p)));
return builder.build().compileTemplates();
@@ -124,10 +124,10 @@
}
}
- private static SoyFileSet.Builder getDefault() {
+ private static SoyFileSet.Builder getDefault(@Nullable SitePaths site) {
SoyFileSet.Builder builder = SoyFileSet.builder();
for (String name : TEMPLATES) {
- addTemplate(builder, null, "com/google/gerrit/server/mail/", name);
+ addTemplate(builder, site, "com/google/gerrit/server/mail/", name);
}
return builder;
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index f60e9e5..870fc3e 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -155,6 +155,8 @@
PredicateResult predicateResult = evaluatePredicateTree(predicate, changeData);
return SubmitRequirementExpressionResult.create(expression, predicateResult);
} catch (QueryParseException | SubmitRequirementEvaluationException e) {
+ logger.atWarning().withCause(e).log(
+ "Failed to evaluate submit requirement expression: %s", expression.expressionString());
return SubmitRequirementExpressionResult.error(expression, e.getMessage());
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 738eab3..f6fc8db 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -225,9 +225,11 @@
public static final String ARG_ID_GROUP = "group";
public static final String ARG_ID_OWNER = "owner";
public static final String ARG_ID_NON_UPLOADER = "non_uploader";
+ public static final String ARG_ID_NON_CONTRIBUTOR = "non_contributor";
public static final String ARG_COUNT = "count";
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
+ public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
public static final String OPERATOR_MERGED_BEFORE = "mergedbefore";
public static final String OPERATOR_MERGED_AFTER = "mergedafter";
@@ -485,13 +487,13 @@
}
}
- private final Arguments args;
+ protected final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
- private static final Splitter LABEL_SPLITTER = Splitter.on(",");
+ protected static final Splitter LABEL_SPLITTER = Splitter.on(",");
@Inject
protected ChangeQueryBuilder(Arguments args) {
@@ -1038,6 +1040,8 @@
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else if (value.equals(ARG_ID_NON_UPLOADER)) {
accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
accounts = parseAccount(value);
}
@@ -1072,6 +1076,8 @@
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else if (value.equals(ARG_ID_NON_UPLOADER)) {
accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
accounts = parseAccount(value);
}
@@ -1106,9 +1112,16 @@
}
}
+ validateLabelArgs(accounts);
return new LabelPredicate(args, name, accounts, group, count, countOp);
}
+ protected void validateLabelArgs(Set<Account.Id> accounts) throws QueryParseException {
+ if (accounts != null && accounts.contains(NON_CONTRIBUTOR_ACCOUNT_ID)) {
+ throw new QueryParseException("non_contributor arg is not allowed in change queries");
+ }
+ }
+
/** Assert that keys {@code k1} and {@code k2} do not exist in {@code labelArgs} together. */
private void assertDisjunctive(PredicateArgs labelArgs, String k1, String k2)
throws QueryParseException {
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 5662e4d..83dd5ba 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
@@ -23,6 +24,8 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -30,9 +33,15 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData.StorageConstraint;
+import java.io.IOException;
+import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
public class EqualsLabelPredicate extends ChangeIndexPostFilterPredicate {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ protected final AccountResolver accountResolver;
protected final ProjectCache projectCache;
protected final PermissionBackend permissionBackend;
protected final IdentifiedUser.GenericFactory userFactory;
@@ -61,6 +70,7 @@
@Nullable Integer count) {
super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, count));
this.permissionBackend = args.permissionBackend;
+ this.accountResolver = args.accountResolver;
this.projectCache = args.projectCache;
this.userFactory = args.userFactory;
this.count = count;
@@ -155,6 +165,14 @@
&& cd.currentPatchSet().uploader().equals(approver)) {
return false;
}
+
+ if (account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID)) {
+ if ((cd.currentPatchSet().uploader().equals(approver)
+ || matchAccount(cd.getCommitter().getEmailAddress(), approver)
+ || matchAccount(cd.getAuthor().getEmailAddress(), approver))) {
+ return false;
+ }
+ }
}
IdentifiedUser reviewer = userFactory.create(approver);
@@ -176,9 +194,24 @@
}
}
+ /**
+ * Returns true if the {@code email} parameter belongs to the account identified by the {@code
+ * accountId} parameter.
+ */
+ private boolean matchAccount(String email, Account.Id accountId) {
+ try {
+ List<AccountState> accountsList = accountResolver.resolve(email).asList();
+ return accountsList.stream().anyMatch(c -> c.account().id().equals(accountId));
+ } catch (ConfigInvalidException | IOException e) {
+ logger.atWarning().withCause(e).log("Failed to resolve account %s", email);
+ }
+ return false;
+ }
+
private boolean isMagicUser() {
return account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
- || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID);
+ || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/LabelPredicate.java b/java/com/google/gerrit/server/query/change/LabelPredicate.java
index 2a5a47d..d89940d 100644
--- a/java/com/google/gerrit/server/query/change/LabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -23,6 +23,7 @@
import com.google.gerrit.index.query.RangeUtil;
import com.google.gerrit.index.query.RangeUtil.Range;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
@@ -36,6 +37,7 @@
protected static final int MAX_COUNT = 5; // inclusive
protected static class Args {
+ protected final AccountResolver accountResolver;
protected final ProjectCache projectCache;
protected final PermissionBackend permissionBackend;
protected final IdentifiedUser.GenericFactory userFactory;
@@ -46,6 +48,7 @@
protected final PredicateArgs.Operator countOp;
protected Args(
+ AccountResolver accountResolver,
ProjectCache projectCache,
PermissionBackend permissionBackend,
IdentifiedUser.GenericFactory userFactory,
@@ -54,6 +57,7 @@
AccountGroup.UUID group,
@Nullable Integer count,
@Nullable PredicateArgs.Operator countOp) {
+ this.accountResolver = accountResolver;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.userFactory = userFactory;
@@ -89,6 +93,7 @@
super(
predicates(
new Args(
+ a.accountResolver,
a.projectCache,
a.permissionBackend,
a.userFactory,
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index 5632c14..cb92ddd 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.query.change;
import com.google.common.base.Splitter;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
@@ -30,6 +31,7 @@
import com.google.inject.Inject;
import java.util.List;
import java.util.Locale;
+import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -179,6 +181,9 @@
return fileEditsPredicateFactory.create(FileEditsArgs.create(filePattern, contentPattern));
}
+ @Override
+ protected void validateLabelArgs(Set<Account.Id> accountIds) throws QueryParseException {}
+
private static void validateRegularExpression(String pattern, String errorMessage)
throws QueryParseException {
try {
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 3d9d588..b262b46 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -259,10 +259,13 @@
/**
* Bots don't process automatic rules, the only attention set change they do is this rule: Add
- * owner and uploader when a bot votes negatively.
+ * owner and uploader when a bot votes negatively, but only if the change is open.
*/
private void botsWithNegativeLabelsAddOwnerAndUploader(
BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input) {
+ if (changeNotes.getChange().isClosed()) {
+ return;
+ }
if (input.labels != null && input.labels.values().stream().anyMatch(vote -> vote < 0)) {
Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
Account.Id owner = changeNotes.getChange().getOwner();
diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
index 66536dd..903e5b1 100644
--- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
+++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
@@ -55,8 +55,6 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.documentation.QueryDocumentationExecutor;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginItemContext;
import com.google.gerrit.server.plugincontext.PluginMapContext;
@@ -93,7 +91,6 @@
private final QueryDocumentationExecutor docSearcher;
private final ProjectCache projectCache;
private final AgreementJson agreementJson;
- private final ChangeIndexCollection indexes;
private final SitePaths sitePaths;
@Inject
@@ -116,7 +113,6 @@
QueryDocumentationExecutor docSearcher,
ProjectCache projectCache,
AgreementJson agreementJson,
- ChangeIndexCollection indexes,
SitePaths sitePaths) {
this.config = config;
this.accountVisibilityProvider = accountVisibilityProvider;
@@ -136,7 +132,6 @@
this.docSearcher = docSearcher;
this.projectCache = projectCache;
this.agreementJson = agreementJson;
- this.indexes = indexes;
this.sitePaths = sitePaths;
}
@@ -220,11 +215,6 @@
private ChangeConfigInfo getChangeInfo() {
ChangeConfigInfo info = new ChangeConfigInfo();
info.allowBlame = toBoolean(config.getBoolean("change", "allowBlame", true));
- boolean hasAssigneeInIndex =
- indexes.getSearchIndex().getSchema().hasField(ChangeField.ASSIGNEE_SPEC);
- info.showAssigneeInChangesTable =
- toBoolean(
- config.getBoolean("change", "showAssigneeInChangesTable", false) && hasAssigneeInIndex);
info.updateDelay =
(int) ConfigUtil.getTimeUnit(config, "change", null, "updateDelay", 300, TimeUnit.SECONDS);
info.submitWholeTopic = toBoolean(MergeSuperSet.wholeTopicEnabled(config));
@@ -232,7 +222,6 @@
toBoolean(this.config.getBoolean("change", null, "disablePrivateChanges", false));
info.mergeabilityComputationBehavior =
MergeabilityComputationBehavior.fromConfig(config).name();
- info.enableAssignee = false;
return info;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
index f5b311b..5745a4e 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
@@ -78,7 +78,6 @@
i.defaultBaseForMerges = DefaultBase.AUTO_MERGE;
i.disableKeyboardShortcuts = true;
i.expandInlineDiffs ^= true;
- i.highlightAssigneeInChangeTable ^= true;
i.relativeDateInChangeTable ^= true;
i.sizeBarInChangeTable ^= true;
i.legacycidInChangeTable ^= true;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
index 56e23a4..2fe7038 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
@@ -17,6 +17,8 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
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.testing.TestLabels.codeReview;
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -26,6 +28,8 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseTimezone;
import com.google.gerrit.acceptance.VerifyNoPiiInChangeNotes;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -34,15 +38,21 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -294,6 +304,162 @@
"unexpected base value format");
}
+ @Test
+ public void nonContributorLabelVote_match() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ TestRepository<InMemoryRepository> clonedRepo = cloneProject(project, user);
+ PushOneCommit.Result r1 =
+ pushFactory
+ .create(user.newIdent(), clonedRepo, "Subject", "file.txt", "text")
+ .to("refs/for/master");
+
+ Change.Id cId = r1.getChange().getId();
+
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+
+ // Assert on uploader, committer and author
+ assertUploader(changeInfo, user.email());
+ assertCommitter(changeInfo, user.email());
+ assertAuthor(changeInfo, user.email());
+
+ // Vote from admin (a.k.a. non uploader/committer/author) matches
+ requestScopeOperations.setApiUser(admin.id());
+ approve(cId.toString());
+ assertMatching("label:Code-Review=+2,user=non_contributor", cId);
+ // Also make sure magic label votes and > operator work
+ assertMatching("label:Code-Review=MAX,user=non_contributor", cId);
+ assertMatching("label:Code-Review>+1,user=non_contributor", cId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_voteFromUploader_doesNotMatch() throws Exception {
+ PushOneCommit.Result r1 = createNormalCommit(user.newIdent(), "refs/for/master", "file1");
+
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+ assertUploader(changeInfo, admin.email());
+
+ // Vote from admin (a.k.a. uploader) does not match
+ requestScopeOperations.setApiUser(admin.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+ }
+
+ @Test
+ @Sandboxed
+ public void nonContributorLabelVote_voteFromAuthor_doesNotMatch() throws Exception {
+ Account.Id authorId =
+ accountOperations
+ .newAccount()
+ .fullname("author")
+ .preferredEmail("authoremail@example.com")
+ .create();
+ Account.Id committerId =
+ accountOperations
+ .newAccount()
+ .fullname("committer")
+ .preferredEmail("committeremail@example.com")
+ .create();
+
+ Change.Id changeId =
+ changeOperations.newChange().author(authorId).committer(committerId).create();
+ ChangeInfo changeInfo = gApi.changes().id(changeId.get()).get();
+ assertAuthor(changeInfo, "authoremail@example.com");
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from author does not match
+ requestScopeOperations.setApiUser(authorId);
+ approve(changeId.toString());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", changeId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_voteFromCommitter_doesNotMatch() throws Exception {
+ Account.Id authorId =
+ accountOperations
+ .newAccount()
+ .fullname("author")
+ .preferredEmail("authoremail@example.com")
+ .create();
+ Account.Id committerId =
+ accountOperations
+ .newAccount()
+ .fullname("committer")
+ .preferredEmail("committeremail@example.com")
+ .create();
+
+ Change.Id changeId =
+ changeOperations.newChange().author(authorId).committer(committerId).create();
+ ChangeInfo changeInfo = gApi.changes().id(changeId.get()).get();
+ assertCommitter(changeInfo, "committeremail@example.com");
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from committer does not match
+ requestScopeOperations.setApiUser(committerId);
+ approve(changeId.toString());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", changeId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_uploaderAndAuthorDifferent() throws Exception {
+ TestRepository<InMemoryRepository> clonedRepo = cloneProject(project, admin);
+ PushOneCommit.Result r1 =
+ pushFactory
+ .create(user.newIdent(), clonedRepo, "Subject", "file.txt", "text")
+ .to("refs/for/master");
+
+ requestScopeOperations.setApiUser(admin.id());
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+ assertUploader(changeInfo, admin.email());
+ assertAuthor(changeInfo, user.email());
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from admin (a.k.a. uploader) does not match
+ requestScopeOperations.setApiUser(user.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+
+ // Vote from user (a.k.a. author) does not match
+ requestScopeOperations.setApiUser(admin.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+
+ // Vote from user2 (a.k.a. non-author and non-uploader) matches
+ TestAccount user2 = accountCreator.create();
+ requestScopeOperations.setApiUser(user2.id());
+ approve(r1.getChangeId());
+ assertMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+ }
+
+ private static void assertUploader(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).uploader.email)
+ .isEqualTo(email);
+ }
+
+ private static void assertCommitter(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.committer.email)
+ .isEqualTo(email);
+ }
+
+ private static void assertAuthor(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.author.email)
+ .isEqualTo(email);
+ }
+
+ private void allowLabelPermission(
+ String labelName, String refPattern, AccountGroup.UUID group, int minVote, int maxVote) {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel(labelName).ref(refPattern).group(group).range(minVote, maxVote))
+ .update();
+ }
+
private PushOneCommit.Result createGitSubmoduleCommit(String ref) throws Exception {
return pushFactory
.create(admin.newIdent(), testRepo, "subject", ImmutableMap.of())
@@ -302,6 +468,13 @@
.to(ref);
}
+ private PushOneCommit.Result createNormalCommit(
+ PersonIdent personIdent, String ref, String fileName) throws Exception {
+ return pushFactory
+ .create(personIdent, testRepo, "subject", ImmutableMap.of(fileName, fileName))
+ .to(ref);
+ }
+
private PushOneCommit.Result createNormalCommit(String ref, String fileName) throws Exception {
return pushFactory
.create(admin.newIdent(), testRepo, "subject", ImmutableMap.of(fileName, fileName))
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 443e064..1d2ddc2 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -1488,6 +1488,64 @@
}
@Test
+ public void robotReviewWithNegativeLabelDoesntAddOwnerIfChangeIsMerged() throws Exception {
+ TestAccount robot =
+ accountCreator.create(
+ "robot2", "robot2@example.com", "Ro Bot", "Ro", ServiceUserClassifier.SERVICE_USERS);
+
+ PushOneCommit.Result r = createChange();
+
+ // The robot votes with Code-Review-1 on patch set 1.
+ // Without this vote the robot cannot (re-)apply a negative vote on the change after it was
+ // merged change later.
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(1).review(ReviewInput.dislike());
+
+ // Amend the change so that patch set 2 gets created.
+ requestScopeOperations.setApiUser(admin.id());
+ amendChange(r.getChangeId()).assertOkStatus();
+
+ // Approve the change.
+ approve(r.getChangeId());
+
+ // User adds a comment so that the admin user is added to the attention set.
+ // This has to be a comment from a user, since comments from robots do not trigger attention set
+ // updates.
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "A comment";
+ change(r).current().review(reviewInput);
+
+ // Verify that the admin user was added to the attention set.
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Someone else replied on the change");
+
+ // Submit the change.
+ requestScopeOperations.setApiUser(admin.id());
+ change(r).current().submit();
+
+ // Verify that the attention set was cleared on submit.
+ attentionSet = Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Change was submitted");
+
+ // Re-apply the negative robot vote on patch set 1.
+ // Note it's possible to a apply a negative vote on merged changes if it wasn't already present
+ // since we disallow downgrading votes on merged changes (e.g. downgrade from not present aka 0
+ // to -1 is not allowed).
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(1).review(ReviewInput.dislike());
+
+ // Verify that re-applying the negative robot vote on patch set 1 didn't add the admin user
+ // back to the attention set.
+ attentionSet = Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Change was submitted");
+ }
+
+ @Test
public void robotCommentDoesNotAddOwnerOnClosedChanges() throws Exception {
TestAccount robot =
accountCreator.create(
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
index 45a471b..f728995 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
@@ -15,16 +15,25 @@
package com.google.gerrit.acceptance.server.mail;
import static com.google.common.truth.Truth.assertThat;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.entities.EmailHeader;
import com.google.gerrit.entities.EmailHeader.StringEmailHeader;
+import com.google.gerrit.server.config.SitePaths;
import java.net.URI;
+import java.nio.file.Files;
import java.util.Map;
+import javax.inject.Inject;
import org.junit.Test;
+@UseLocalDisk
public class MailSenderIT extends AbstractMailIT {
+ @Inject private SitePaths sitePaths;
+
@Test
@GerritConfig(name = "sendemail.replyToAddress", value = "custom@gerritcodereview.com")
@GerritConfig(name = "receiveemail.protocol", value = "POP3")
@@ -63,6 +72,20 @@
assertThat(headerString(headers, "In-Reply-To")).isEqualTo(threadId);
}
+ @Test
+ @Sandboxed
+ public void useCustomTemplates() throws Exception {
+ String customTemplate =
+ "{namespace com.google.gerrit.server.mail.template.ChangeSubject}\n"
+ + "\n"
+ + "{template ChangeSubject kind=\"text\"}CUSTOM-TEMPLATE{/template}\n";
+ Files.write(sitePaths.mail_dir.resolve("ChangeSubject.soy"), customTemplate.getBytes(UTF_8));
+
+ createChangeWithReview(user);
+ String subject = headerString(sender.getMessages().iterator().next().headers(), "Subject");
+ assertThat(subject).isEqualTo("CUSTOM-TEMPLATE");
+ }
+
private String headerString(Map<String, EmailHeader> headers, String name) {
EmailHeader header = headers.get(name);
assertThat(header).isInstanceOf(StringEmailHeader.class);
diff --git a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
index bd4b2b1..bbf10bd 100644
--- a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
@@ -48,7 +48,6 @@
PatchSet.id(Change.id(14), 23), "subject XYZ", "original subject ABC");
change.setTopic("my topic");
change.setSubmissionId("submission ID 234");
- change.setAssignee(Account.id(100001));
change.setPrivate(true);
change.setWorkInProgress(true);
change.setReviewStarted(true);
@@ -73,7 +72,6 @@
.setTopic("my topic")
.setOriginalSubject("original subject ABC")
.setSubmissionId("submission ID 234")
- .setAssignee(Entities.Account_Id.newBuilder().setId(100001))
.setIsPrivate(true)
.setWorkInProgress(true)
.setReviewStarted(true)
@@ -205,7 +203,6 @@
PatchSet.id(Change.id(14), 23), "subject XYZ", "original subject ABC");
change.setTopic("my topic");
change.setSubmissionId("submission ID 234");
- change.setAssignee(Account.id(100001));
change.setPrivate(true);
change.setWorkInProgress(true);
change.setReviewStarted(true);
@@ -289,7 +286,6 @@
.put("topic", String.class)
.put("originalSubject", String.class)
.put("submissionId", String.class)
- .put("assignee", Account.Id.class)
.put("isPrivate", boolean.class)
.put("workInProgress", boolean.class)
.put("reviewStarted", boolean.class)
@@ -313,7 +309,6 @@
assertThat(change.getTopic()).isEqualTo(expectedChange.getTopic());
assertThat(change.getOriginalSubject()).isEqualTo(expectedChange.getOriginalSubject());
assertThat(change.getSubmissionId()).isEqualTo(expectedChange.getSubmissionId());
- assertThat(change.getAssignee()).isEqualTo(expectedChange.getAssignee());
assertThat(change.isPrivate()).isEqualTo(expectedChange.isPrivate());
assertThat(change.isWorkInProgress()).isEqualTo(expectedChange.isWorkInProgress());
assertThat(change.hasReviewStarted()).isEqualTo(expectedChange.hasReviewStarted());
diff --git a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
index 8a7d25a..3704969 100644
--- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
+++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -94,61 +94,6 @@
}
@Test
- public void getDiff_givenEqualAssignees_returnsNullAssignee() {
- ChangeInfo oldChangeInfo =
- createChangeInfoWithAccount(new AccountInfo("name", "mail@mail.com"));
- ChangeInfo newChangeInfo =
- createChangeInfoWithAccount(
- new AccountInfo(oldChangeInfo.assignee.name, oldChangeInfo.assignee.email));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().assignee).isNull();
- assertThat(diff.removed().assignee).isNull();
- }
-
- @Test
- public void getDiff_givenNewAssignee_returnsAssignee() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo =
- createChangeInfoWithAccount(new AccountInfo("name", "mail@mail.com"));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().assignee).isEqualTo(newChangeInfo.assignee);
- assertThat(diff.removed().assignee).isNull();
- }
-
- @Test
- public void getDiff_withRemovedAssignee_returnsAssignee() {
- ChangeInfo oldChangeInfo =
- createChangeInfoWithAccount(new AccountInfo("name", "mail@mail.com"));
- ChangeInfo newChangeInfo = new ChangeInfo();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().assignee).isNull();
- assertThat(diff.removed().assignee).isEqualTo(oldChangeInfo.assignee);
- }
-
- @Test
- public void getDiff_givenAssigneeWithNewName_returnsNameButNotEmail() {
- ChangeInfo oldChangeInfo =
- createChangeInfoWithAccount(new AccountInfo("old name", "mail@mail.com"));
- ChangeInfo newChangeInfo =
- createChangeInfoWithAccount(new AccountInfo("new name", oldChangeInfo.assignee.email));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().assignee).isNotNull();
- assertThat(diff.added().assignee.name).isEqualTo(newChangeInfo.assignee.name);
- assertThat(diff.added().assignee.email).isNull();
- assertThat(diff.removed().assignee).isNotNull();
- assertThat(diff.removed().assignee.name).isEqualTo(oldChangeInfo.assignee.name);
- assertThat(diff.removed().assignee.email).isNull();
- }
-
- @Test
public void getDiff_whenHashtagsChanged_returnsHashtags() {
String removedHashtag = "removed";
String addedHashtag = "added";
@@ -708,12 +653,6 @@
return changeInfo;
}
- private static ChangeInfo createChangeInfoWithAccount(AccountInfo accountInfo) {
- ChangeInfo changeInfo = new ChangeInfo();
- changeInfo.assignee = accountInfo;
- return changeInfo;
- }
-
private static ChangeInfo createChangeInfoWithHashtags(String... hashtags) {
ChangeInfo changeInfo = new ChangeInfo();
changeInfo.hashtags = ImmutableList.copyOf(hashtags);
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index cd7a97b..0533f3f 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1435,6 +1435,15 @@
// "count" and "group" args cannot be used simultaneously.
assertThrows(
BadRequestException.class, () -> assertQuery("label:Code-Review=+1,group=gerrit,count=2"));
+
+ // "non_contributor arg for the label operator is not allowed in change queries
+ thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> assertQuery("label:Code-Review=+2,user=non_contributor"));
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo("non_contributor arg is not allowed in change queries");
}
@Test
@@ -3898,14 +3907,6 @@
}
@Test
- public void byOwnerInvalidQuery() throws Exception {
- repo = createAndOpenProject("repo");
- insert("repo", newChange(repo), userId);
- String nameEmail = user.asIdentifiedUser().getNameEmail();
- assertQuery("owner: \"" + nameEmail + "\"\\");
- }
-
- @Test
public void byDeletedChange() throws Exception {
repo = createAndOpenProject("repo");
Change change = insert("repo", newChange(repo));
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 5cae012..7f383f9 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -59,16 +59,9 @@
}
@Test
- @Override
- public void byOwnerInvalidQuery() throws Exception {
- repo = createAndOpenProject("repo");
- Change change1 = insert("repo", newChange(repo), userId);
- String nameEmail = user.asIdentifiedUser().getNameEmail();
-
+ public void invalidQuery() throws Exception {
BadRequestException thrown =
- assertThrows(
- BadRequestException.class,
- () -> assertQuery("owner: \"" + nameEmail + "\"\\", change1));
+ assertThrows(BadRequestException.class, () -> newQuery("\\").get());
assertThat(thrown).hasMessageThat().contains("Cannot create full-text query with value: \\");
}
diff --git a/lib/bouncycastle/BUILD b/lib/bouncycastle/BUILD
index 43ba6e1..6a87d73 100644
--- a/lib/bouncycastle/BUILD
+++ b/lib/bouncycastle/BUILD
@@ -22,6 +22,13 @@
)
java_library(
+ name = "bcutil",
+ data = ["//lib:LICENSE-bouncycastle"],
+ visibility = ["//visibility:public"],
+ exports = ["@bcutil//jar"],
+)
+
+java_library(
name = "bcprov-neverlink",
data = ["//lib:LICENSE-bouncycastle"],
neverlink = 1,
@@ -44,3 +51,11 @@
visibility = ["//visibility:public"],
exports = ["@bcpkix//jar"],
)
+
+java_library(
+ name = "bcutil-neverlink",
+ data = ["//lib:LICENSE-bouncycastle"],
+ neverlink = 1,
+ visibility = ["//visibility:public"],
+ exports = ["@bcutil//jar"],
+)
diff --git a/package.json b/package.json
index ae1bb2f..362b9dc 100644
--- a/package.json
+++ b/package.json
@@ -35,8 +35,8 @@
"scripts": {
"setup": "yarn && yarn --cwd=polygerrit-ui && yarn --cwd=polygerrit-ui/app",
"clean": "git clean -fdx && bazel clean --expunge",
- "compile:local": "tsc --project ./polygerrit-ui/app/tsconfig.json",
- "compile:watch": "npm run compile:local -- --preserveWatchOutput --watch",
+ "compile": "tsc --project ./polygerrit-ui/app/tsconfig.json",
+ "compile:watch": "npm run compile -- --preserveWatchOutput --watch",
"start": "run-p -rl compile:watch start:server",
"start:server": "web-dev-server",
"test": "yarn --cwd=polygerrit-ui test",
@@ -50,7 +50,8 @@
"safe_bazelisk": "if which bazelisk >/dev/null; then bazel_bin=bazelisk; else bazel_bin=bazel; fi && $bazel_bin",
"eslint": "npm run safe_bazelisk test polygerrit-ui/app:lint_test",
"eslintfix": "npm run safe_bazelisk run polygerrit-ui/app:lint_bin -- -- --fix $(pwd)/polygerrit-ui/app",
- "litlint": "npm run safe_bazelisk run polygerrit-ui/app:lit_analysis"
+ "litlint": "npm run safe_bazelisk run polygerrit-ui/app:lit_analysis",
+ "lint": "eslint -c polygerrit-ui/app/.eslintrc.js --ignore-path polygerrit-ui/app/.eslintignore polygerrit-ui/app"
},
"repository": {
"type": "git",
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index 510ce54..a89c0b7 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -189,7 +189,7 @@
Compiling code:
```sh
# Compile frontend once to check for type errors:
-yarn compile:local
+yarn compile
# Watch mode:
yarn compile:watch
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index f915432..b9ed56b 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -258,6 +258,8 @@
NONE = 'NONE',
}
+// These defaults should match the defaults in
+// java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
export function createDefaultPreferences(): PreferencesInfo {
return {
changes_per_page: 25,
@@ -265,8 +267,8 @@
size_bar_in_change_table: true,
my: [],
theme: AppTheme.AUTO,
- date_format: DateFormat.EURO,
- time_format: TimeFormat.HHMM_24,
+ date_format: DateFormat.STD,
+ time_format: TimeFormat.HHMM_12,
change_table: [],
email_strategy: EmailStrategy.ATTENTION_SET_ONLY,
default_base_for_merges: DefaultBase.AUTO_MERGE,
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 b726292..77a51db 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
@@ -70,6 +70,8 @@
const DETAILS_QUOTA: Map<RunStatus | Category, number> = new Map();
DETAILS_QUOTA.set(Category.ERROR, 7);
DETAILS_QUOTA.set(Category.WARNING, 2);
+DETAILS_QUOTA.set(Category.INFO, 2);
+DETAILS_QUOTA.set(Category.SUCCESS, 2);
DETAILS_QUOTA.set(RunStatus.RUNNING, 2);
@customElement('gr-change-summary')
@@ -417,8 +419,27 @@
if (hasResultsOf(run, category)) return true;
return category === Category.SUCCESS && hasCompletedWithoutResults(run);
});
+ const hasRunning = this.runs.some(isRunningOrScheduled);
+ const hasWarning = this.runs.some(run =>
+ hasResultsOf(run, Category.WARNING)
+ );
+ const hasError = this.runs.some(run => hasResultsOf(run, Category.ERROR));
const count = (run: CheckRun) => getResultsOf(run, category);
- if (category === Category.SUCCESS || category === Category.INFO) {
+
+ // Sometimes INFO and SUCCESS results should not consume much UI space and
+ // not grab any attention, e.g. when there are errors. Then let's
+ // aggressively collapse them into one small chip. But if INFO and SUCCESS
+ // is all we have, then make use of the one line we have and show expanded
+ // chips.
+ if (
+ category === Category.SUCCESS &&
+ (hasRunning || hasError || hasWarning || runs.length > 3)
+ ) {
+ return this.renderChecksChipsCollapsed(runs, category, count);
+ } else if (
+ category === Category.INFO &&
+ (hasRunning || hasError || runs.length > 3)
+ ) {
return this.renderChecksChipsCollapsed(runs, category, count);
}
return this.renderChecksChipsExpanded(runs, category);
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
index 05036ab..3c5371f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
@@ -6,13 +6,15 @@
import '../../../test/common-test-setup';
import {fixture, html, assert} from '@open-wc/testing';
import {GrChangeSummary} from './gr-change-summary';
-import {queryAndAssert} from '../../../utils/common-util';
+import {queryAll, queryAndAssert} from '../../../utils/common-util';
import {fakeRun0} from '../../../models/checks/checks-fakes';
import {
createAccountWithEmail,
+ createCheckResult,
createComment,
createCommentThread,
createDraft,
+ createRun,
} from '../../../test/test-data-generators';
import {stubFlags} from '../../../test/test-utils';
import {Timestamp} from '../../../api/rest-api';
@@ -22,6 +24,9 @@
CommentsModel,
commentsModelToken,
} from '../../../models/comments/comments-model';
+import {GrChecksChip} from './gr-checks-chip';
+import {CheckRun} from '../../../models/checks/checks-model';
+import {Category, RunStatus} from '../../../api/checks';
suite('gr-change-summary test', () => {
let element: GrChangeSummary;
@@ -117,6 +122,73 @@
);
});
+ suite('checks summary', () => {
+ const checkSummary = async (runs: CheckRun[], texts: string[]) => {
+ element.runs = runs;
+ element.showChecksSummary = true;
+ await element.updateComplete;
+ const chips = queryAll<GrChecksChip>(element, 'gr-checks-chip') ?? [];
+ assert.deepEqual(
+ [...chips].map(c => `${c.statusOrCategory} ${c.text}`),
+ texts
+ );
+ };
+
+ test('single success', async () => {
+ checkSummary([createRun()], ['SUCCESS test-name']);
+ });
+
+ test('single running', async () => {
+ checkSummary(
+ [createRun({status: RunStatus.RUNNING})],
+ ['RUNNING test-name']
+ );
+ });
+
+ test('single info', async () => {
+ checkSummary(
+ [
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.INFO})],
+ }),
+ ],
+ ['INFO test-name']
+ );
+ });
+
+ test('single of each collapses INFO and SUCCESS', async () => {
+ checkSummary(
+ [
+ createRun({status: RunStatus.RUNNING}),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.SUCCESS})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.INFO})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.WARNING})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.ERROR})],
+ }),
+ ],
+ [
+ 'ERROR test-name',
+ 'WARNING test-name',
+ 'INFO 1',
+ 'SUCCESS 1',
+ 'RUNNING test-name',
+ ]
+ );
+ });
+ });
+
test('renders mentions summary', async () => {
stubFlags('isEnabled').returns(true);
// recreate element so that flag protected subscriptions are added
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 371e5b5..721d650 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
@@ -115,7 +115,6 @@
import {GrFileList} from '../gr-file-list/gr-file-list';
import {EditRevisionInfo, ParsedChangeInfo} from '../../../types/types';
import {
- CloseFixPreviewEvent,
EditableContentSaveEvent,
EventType,
OpenFixPreviewEvent,
@@ -590,8 +589,9 @@
this.addEventListener('editable-content-cancel', () =>
this.handleCommitMessageCancel()
);
- this.addEventListener('open-fix-preview', e => this.onOpenFixPreview(e));
- this.addEventListener('close-fix-preview', e => this.onCloseFixPreview(e));
+ this.addEventListener(EventType.OPEN_FIX_PREVIEW, e =>
+ this.onOpenFixPreview(e)
+ );
this.addEventListener(EventType.SHOW_TAB, e => this.setActiveTab(e));
this.addEventListener('reload', e => {
@@ -1675,10 +1675,6 @@
this.applyFixDialog.open(e);
}
- private onCloseFixPreview(e: CloseFixPreviewEvent) {
- if (e.detail.fixApplied) fireReload(this);
- }
-
// Private but used in tests.
handleToggleDiffMode() {
if (this.diffViewMode === DiffViewMode.SIDE_BY_SIDE) {
@@ -2080,13 +2076,7 @@
// Private but used in tests.
viewStateChanged() {
if (!this.viewState) return;
-
- if (this.isChangeObsolete()) {
- // Tell the app element that we are not going to handle the new change
- // number and that they have to create a new change view.
- fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
- return;
- }
+ if (this.isChangeObsolete()) return;
if (this.viewState.basePatchNum === undefined)
this.viewState.basePatchNum = PARENT;
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 9b78e64..ac4c1f9 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
@@ -1557,21 +1557,6 @@
assert.isOk(element.patchRange?.patchNum);
});
- test('do not handle new change numbers', async () => {
- const recreateSpy = sinon.spy();
- element.addEventListener('recreate-change-view', recreateSpy);
-
- const value: ChangeViewState = createChangeViewState();
- element.viewState = value;
- await element.updateComplete;
- assert.isFalse(recreateSpy.calledOnce);
-
- value.changeNum = 555111333 as NumericChangeId;
- element.viewState = {...value};
- await element.updateComplete;
- assert.isTrue(recreateSpy.calledOnce);
- });
-
test('related changes are updated when loadData is called', async () => {
await element.updateComplete;
const relatedChanges = element.shadowRoot!.querySelector(
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 1fbddc5..247b7c3 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
@@ -83,9 +83,6 @@
stubRestApi('getDiffRobotComments').returns(Promise.resolve({}));
stubRestApi('getDiffDrafts').returns(Promise.resolve({}));
stubRestApi('getAccountCapabilities').returns(Promise.resolve({}));
- stubElement('gr-date-formatter', 'loadTimeFormat').callsFake(() =>
- Promise.resolve()
- );
stubElement('gr-diff-host', 'reload').callsFake(() => Promise.resolve());
stubElement('gr-diff-host', 'prefetchDiff').callsFake(() => {});
@@ -2071,9 +2068,6 @@
stubRestApi('getDiffComments').returns(Promise.resolve({}));
stubRestApi('getDiffRobotComments').returns(Promise.resolve({}));
stubRestApi('getDiffDrafts').returns(Promise.resolve({}));
- stubElement('gr-date-formatter', 'loadTimeFormat').callsFake(() =>
- Promise.resolve()
- );
stubRestApi('getDiff').callsFake(() => Promise.resolve(createDiff()));
stubElement('gr-diff-host', 'prefetchDiff').callsFake(() => {});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 4f7d56f..997d9d5 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1310,15 +1310,17 @@
const repo = ctx.params[0] as RepoName;
const commentId = ctx.params[2] as UrlEncodedCommentId;
- const [comments, robotComments, change] = await Promise.all([
+ const [comments, robotComments, drafts, change] = await Promise.all([
this.restApiService.getDiffComments(changeNum),
this.restApiService.getDiffRobotComments(changeNum),
+ this.restApiService.getDiffDrafts(changeNum),
this.restApiService.getChangeDetail(changeNum),
]);
const comment =
findComment(addPath(comments), commentId) ??
- findComment(addPath(robotComments), commentId);
+ findComment(addPath(robotComments), commentId) ??
+ findComment(addPath(drafts), commentId);
const path = comment?.path;
const patchsets = computeAllPatchSets(change);
const latestPatchNum = computeLatestPatchNum(patchsets);
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index b146e93..1608c22 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -20,7 +20,6 @@
import {PROVIDED_FIX_ID} from '../../../utils/comment-util';
import {OpenFixPreviewEvent} from '../../../types/events';
import {getAppContext} from '../../../services/app-context';
-import {fireCloseFixPreview} from '../../../utils/event-util';
import {DiffLayer, ParsedChangeInfo} from '../../../types/types';
import {GrButton} from '../../shared/gr-button/gr-button';
import {TokenHighlightLayer} from '../../../embed/diff/gr-diff-builder/token-highlight-layer';
@@ -37,6 +36,8 @@
import {GrSyntaxLayerWorker} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
import {anyLineTooLong} from '../../../embed/diff/gr-diff/gr-diff-utils';
+import {changeModelToken} from '../../../models/change/change-model';
+import {fireReload} from '../../../utils/event-util';
interface FilePreview {
filepath: string;
@@ -90,12 +91,20 @@
@state()
diffPrefs?: DiffPreferencesInfo;
+ @state()
+ isOwner = false;
+
+ @state()
+ onCloseFixPreviewCallbacks: ((fixapplied: boolean) => void)[] = [];
+
private readonly restApiService = getAppContext().restApiService;
private readonly getUserModel = resolve(this, userModelToken);
private readonly getNavigation = resolve(this, navigationToken);
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
private readonly syntaxLayer = new GrSyntaxLayerWorker(
resolve(this, highlightServiceToken),
() => getAppContext().reportingService
@@ -105,6 +114,11 @@
super();
subscribe(
this,
+ () => this.getChangeModel().isOwner$,
+ x => (this.isOwner = x)
+ );
+ subscribe(
+ this,
() => this.getUserModel().preferences$,
preferences => {
const layers: DiffLayer[] = [this.syntaxLayer];
@@ -234,6 +248,7 @@
open(e: OpenFixPreviewEvent) {
this.patchNum = e.detail.patchNum;
this.fixSuggestions = e.detail.fixSuggestions;
+ this.onCloseFixPreviewCallbacks = e.detail.onCloseFixPreviewCallbacks;
assert(this.fixSuggestions.length > 0, 'no fix in the event');
this.selectedFixIdx = 0;
this.applyFixModal?.showModal();
@@ -319,12 +334,14 @@
this.currentPreviews = [];
this.isApplyFixLoading = false;
- fireCloseFixPreview(this, fixApplied);
+ this.onCloseFixPreviewCallbacks.forEach(fn => fn(fixApplied));
this.applyFixModal?.close();
+ if (fixApplied) fireReload(this);
}
private computeTooltip() {
if (!this.change || !this.patchNum) return '';
+ if (!this.isOwner) return 'Fix can only be applied by author';
const latestPatchNum =
this.change.revisions[this.change.current_revision]._number;
return latestPatchNum !== this.patchNum
@@ -334,6 +351,7 @@
private computeDisableApplyFixButton() {
if (!this.change || !this.patchNum) return true;
+ if (!this.isOwner) return true;
const latestPatchNum =
this.change.revisions[this.change.current_revision]._number;
return this.patchNum !== latestPatchNum || this.isApplyFixLoading;
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
index 24dadf7..9284fb2 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
@@ -17,11 +17,7 @@
} from '../../../test/test-data-generators';
import {createDefaultDiffPrefs} from '../../../constants/constants';
import {DiffInfo} from '../../../types/diff';
-import {
- CloseFixPreviewEventDetail,
- EventType,
- OpenFixPreviewEventDetail,
-} from '../../../types/events';
+import {EventType, OpenFixPreviewEventDetail} from '../../../types/events';
import {GrButton} from '../../shared/gr-button/gr-button';
import {fixture, html, assert} from '@open-wc/testing';
import {SinonStub} from 'sinon';
@@ -37,11 +33,13 @@
createFixSuggestionInfo('fix_1'),
createFixSuggestionInfo('fix_2'),
],
+ onCloseFixPreviewCallbacks: [],
};
const ONE_FIX: OpenFixPreviewEventDetail = {
patchNum: 2 as PatchSetNum,
fixSuggestions: [createFixSuggestionInfo('fix_1')],
+ onCloseFixPreviewCallbacks: [],
};
function getConfirmButton(): GrButton {
@@ -73,6 +71,7 @@
element.changeNum = change._number;
element.patchNum = change.revisions[change.current_revision]._number;
element.change = change;
+ element.isOwner = true;
element.diffPrefs = {
...createDefaultDiffPrefs(),
font_size: 12,
@@ -162,8 +161,22 @@
assert.equal(button.getAttribute('title'), '');
});
+ test('apply fix button is disabled for non-author', async () => {
+ element.isOwner = false;
+ await element.updateComplete;
+ await open(TWO_FIXES);
+ assert.equal(element.currentFix!.fix_id, 'fix_1');
+ assert.equal(element.currentPreviews.length, 2);
+ const button = getConfirmButton();
+ assert.isTrue(button.hasAttribute('disabled'));
+ assert.equal(
+ button.getAttribute('title'),
+ 'Fix can only be applied by author'
+ );
+ });
+
test('apply fix button is disabled on older patchset', async () => {
- element.change = element.change = {
+ element.change = {
...createParsedChange(),
revisions: createRevisions(2),
current_revision: getCurrentRevision(0),
@@ -246,11 +259,7 @@
element.currentFix = createFixSuggestionInfo('123');
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
await element.handleApplyFix(new CustomEvent('confirm'));
@@ -263,14 +272,7 @@
assert.isTrue(setUrlStub.called);
assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/2..edit');
- sinon.assert.calledOnceWithExactly(
- closeFixPreviewEventSpy,
- new CustomEvent<CloseFixPreviewEventDetail>(EventType.CLOSE_FIX_PREVIEW, {
- detail: {
- fixApplied: true,
- },
- })
- );
+ sinon.assert.calledOnceWithExactly(closeFixPreviewEventSpy, true);
// reset gr-apply-fix-dialog and close
assert.equal(element.currentFix, undefined);
assert.equal(element.currentPreviews.length, 0);
@@ -311,11 +313,7 @@
element.currentFix = createFixSuggestionInfo('fix_123');
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
let expectedError;
await element.handleApplyFix(new CustomEvent('click')).catch(e => {
@@ -328,19 +326,8 @@
test('onCancel fires close with correct parameters', () => {
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
element.onCancel(new CustomEvent('cancel'));
- sinon.assert.calledOnceWithExactly(
- closeFixPreviewEventSpy,
- new CustomEvent<CloseFixPreviewEventDetail>(EventType.CLOSE_FIX_PREVIEW, {
- detail: {
- fixApplied: false,
- },
- })
- );
+ sinon.assert.calledOnceWithExactly(closeFixPreviewEventSpy, false);
});
});
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 c74993f..c7d6948 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
@@ -53,11 +53,7 @@
import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
import {GrApplyFixDialog} from '../gr-apply-fix-dialog/gr-apply-fix-dialog';
import {CommentMap} from '../../../utils/comment-util';
-import {
- EventType,
- OpenFixPreviewEvent,
- ValueChangedEvent,
-} from '../../../types/events';
+import {OpenFixPreviewEvent, ValueChangedEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {assertIsDefined, queryAndAssert} from '../../../utils/common-util';
import {Key, toggleClass, whenVisible} from '../../../utils/dom-util';
@@ -417,14 +413,12 @@
changeNum => {
if (!changeNum || this.changeNum === changeNum) return;
- // We are only setting the changeNum of the diff view once!
+ // We are only setting the changeNum of the diff view once.
// Everything in the diff view is tied to the change. It seems better to
// force the re-creation of the diff view when the change number changes.
- if (!this.changeNum) {
- this.changeNum = changeNum;
- } else {
- fireEvent(this, EventType.RECREATE_DIFF_VIEW);
- }
+ // The parent element will make sure that a new change view is created
+ // when the change number changes (using the `keyed` directive).
+ if (!this.changeNum) this.changeNum = changeNum;
}
);
subscribe(
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index eceb05e..b096f76 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -32,7 +32,7 @@
import {navigationToken} from './core/gr-navigation/gr-navigation';
import {getAppContext} from '../services/app-context';
import {routerToken} from './core/gr-router/gr-router';
-import {AccountDetailInfo} from '../types/common';
+import {AccountDetailInfo, NumericChangeId} from '../types/common';
import {
constructServerErrorMsg,
GrErrorManager,
@@ -62,6 +62,7 @@
import {customElement, property, query, state} from 'lit/decorators.js';
import {Shortcut, ShortcutController} from './lit/shortcut-controller';
import {cache} from 'lit/directives/cache.js';
+import {keyed} from 'lit/directives/keyed.js';
import {assertIsDefined} from '../utils/common-util';
import './gr-css-mixins';
import {isDarkTheme, prefersDarkColorScheme} from '../utils/theme-util';
@@ -123,6 +124,9 @@
// TODO: Introduce a wrapper element for CHANGE, DIFF, EDIT view.
@state() private childView?: ChangeChildView;
+ // Used as a key for caching the CHANGE, DIFF, EDIT view.
+ @state() private changeNum?: NumericChangeId;
+
@state() private lastError?: ErrorInfo;
// private but used in test
@@ -148,12 +152,6 @@
// (e.g. shortcut dialog) is open
@state() private mainAriaHidden = false;
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateChangeViewCache = false;
-
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateDiffViewCache = false;
-
@state() private theme = AppTheme.AUTO;
readonly getRouter = resolve(this, routerToken);
@@ -189,12 +187,6 @@
document.addEventListener(EventType.LOCATION_CHANGE, () =>
this.handleLocationChange()
);
- this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
- this.handleRecreateView()
- );
- this.addEventListener(EventType.RECREATE_DIFF_VIEW, () =>
- this.handleRecreateView()
- );
document.addEventListener(EventType.GR_RPC_LOG, e => this.handleRpcLog(e));
this.shortcuts.addAbstract(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, () =>
this.showKeyboardShortcuts()
@@ -246,8 +238,13 @@
subscribe(
this,
() => this.getChangeViewModel().childView$,
- childView => {
- this.childView = childView;
+ childView => (this.childView = childView)
+ );
+ subscribe(
+ this,
+ () => this.getChangeViewModel().changeNum$,
+ changeNum => {
+ this.changeNum = changeNum;
}
);
@@ -375,8 +372,19 @@
${this.renderHeader()}
<main ?aria-hidden=${this.mainAriaHidden}>
${this.renderMobileSearch()} ${this.renderChangeListView()}
- ${this.renderDashboardView()} ${this.renderChangeView()}
- ${this.renderEditorView()} ${this.renderDiffView()}
+ ${this.renderDashboardView()}
+ ${
+ // `keyed(this.changeNum, ...)` makes sure that these views are not
+ // re-used across changes, which is a precaution, because we have run
+ // into issue with that. That could be re-considered at some point.
+ keyed(
+ this.changeNum,
+ html`
+ ${this.renderChangeView()} ${this.renderEditorView()}
+ ${this.renderDiffView()}
+ `
+ )
+ }
${this.renderSettingsView()} ${this.renderAdminView()}
${this.renderPluginScreen()} ${this.renderCLAView()}
${this.renderDocumentationSearch()}
@@ -473,18 +481,15 @@
}
private renderChangeView() {
- if (this.invalidateChangeViewCache) {
- this.updateComplete.then(() => (this.invalidateChangeViewCache = false));
- return nothing;
- }
- return cache(this.isChangeView() ? this.changeViewTemplate() : nothing);
- }
-
- // Template as not to create duplicates, for renderChangeView() only.
- private changeViewTemplate() {
- return html`
- <gr-change-view .backPage=${this.lastSearchPage}></gr-change-view>
- `;
+ // The `cache()` is required for re-using the change view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isChangeView()
+ ? html`<gr-change-view
+ .backPage=${this.lastSearchPage}
+ ></gr-change-view>`
+ : nothing
+ );
}
private isChangeView() {
@@ -494,9 +499,11 @@
);
}
- private isDiffView() {
- return (
- this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ private renderEditorView() {
+ // The `cache()` is required for re-using the editor view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isEditorView() ? html`<gr-editor-view></gr-editor-view>` : nothing
);
}
@@ -506,21 +513,18 @@
);
}
- private renderEditorView() {
- if (!this.isEditorView()) return nothing;
- return html`<gr-editor-view></gr-editor-view>`;
- }
-
private renderDiffView() {
- if (this.invalidateDiffViewCache) {
- this.updateComplete.then(() => (this.invalidateDiffViewCache = false));
- return nothing;
- }
- return cache(this.isDiffView() ? this.diffViewTemplate() : nothing);
+ // The `cache()` is required for re-using the diff view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isDiffView() ? html`<gr-diff-view></gr-diff-view>` : nothing
+ );
}
- private diffViewTemplate() {
- return html`<gr-diff-view></gr-diff-view>`;
+ private isDiffView() {
+ return (
+ this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ );
}
private renderSettingsView() {
@@ -619,15 +623,6 @@
(this.account && this.account._account_id) || null;
}
- /**
- * Throws away the view and re-creates it. The view itself fires an event, if
- * it wants to be re-created.
- */
- private handleRecreateView() {
- this.invalidateChangeViewCache = true;
- this.invalidateDiffViewCache = true;
- }
-
private async viewChanged() {
if (
this.params &&
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 1ac89d6..c844d42 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -1053,6 +1053,11 @@
replacement
),
patchNum: this.comment.patch_set,
+ onCloseFixPreviewCallbacks: [
+ fixApplied => {
+ if (fixApplied) this.handleAppliedFix();
+ },
+ ],
};
}
if (isRobot(this.comment) && this.comment.fix_suggestions.length > 0) {
@@ -1065,6 +1070,7 @@
};
}),
patchNum: this.comment.patch_set,
+ onCloseFixPreviewCallbacks: [],
};
}
throw new Error('unable to create preview fix event');
@@ -1132,6 +1138,18 @@
fire(this, 'reply-to-comment', eventDetail);
}
+ private handleAppliedFix() {
+ const message = this.comment?.message;
+ assert(!!message, 'empty message');
+ const eventDetail: ReplyToCommentEventDetail = {
+ content: 'Fix applied.',
+ userWantsToEdit: false,
+ unresolved: false,
+ };
+ // Handled by <gr-comment-thread>.
+ fire(this, 'reply-to-comment', eventDetail);
+ }
+
private async handleShowFix() {
// Handled top-level in the diff and change view components.
fire(this, 'open-fix-preview', await this.createFixPreview());
diff --git a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.ts b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.ts
index 25ee130..05b7fb5 100644
--- a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.ts
+++ b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.ts
@@ -18,8 +18,10 @@
} from '../../../utils/date-util';
import {TimeFormat, DateFormat} from '../../../constants/constants';
import {assertNever} from '../../../utils/common-util';
-import {Timestamp} from '../../../types/common';
-import {getAppContext} from '../../../services/app-context';
+import {PreferencesInfo, Timestamp} from '../../../types/common';
+import {resolve} from '../../../models/dependency';
+import {userModelToken} from '../../../models/user/user-model';
+import {subscribe} from '../../lit/subscription-controller';
const TimeFormats = {
TIME_12: 'h:mm A', // 2:14 PM
@@ -95,7 +97,7 @@
@state()
relative = false;
- private readonly restApiService = getAppContext().restApiService;
+ private readonly getUserModel = resolve(this, userModelToken);
static override get styles() {
return [
@@ -108,17 +110,30 @@
];
}
- override render() {
- if (!this.withTooltip) {
- return this.renderDateString();
- }
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getUserModel().preferences$,
+ prefs => this.setPreferences(prefs)
+ );
+ }
- const fullDateStr = this.computeFullDateStr();
- if (!fullDateStr) {
- return this.renderDateString();
- }
+ // private but used by tests
+ setPreferences(prefs: PreferencesInfo) {
+ this.decideDateFormat(prefs.date_format);
+ this.decideTimeFormat(prefs.time_format);
+ this.relative =
+ this.forceRelative || Boolean(prefs?.relative_date_in_change_table);
+ }
+
+ override render() {
+ if (!this.withTooltip) return this.renderDateString();
+ const tooltip = this.computeFullDateStr();
+ if (!tooltip) return this.renderDateString();
+
return html`
- <gr-tooltip-content has-tooltip title=${fullDateStr}>
+ <gr-tooltip-content has-tooltip title=${tooltip}>
${this.renderDateString()}
</gr-tooltip-content>
`;
@@ -128,38 +143,11 @@
return html` <span>${this.computeDateStr()}</span>`;
}
- override connectedCallback() {
- super.connectedCallback();
- this.loadPreferences();
- }
-
// private but used by tests
- _getUtcOffsetString() {
+ getUtcOffsetString() {
return utcOffsetString();
}
- // private but used by tests
- async loadPreferences() {
- const loggedIn = await this.restApiService.getLoggedIn();
- if (!loggedIn) {
- this.timeFormat = TimeFormats.TIME_24;
- this.dateFormat = DateFormats.STD;
- this.relative = this.forceRelative;
- return;
- }
- await Promise.all([this.loadTimeFormat(), this.loadRelative()]);
- }
-
- // private but used in gr/file-list_test.ts
- async loadTimeFormat() {
- const preferences = await this.restApiService.getPreferences();
- if (!preferences) {
- throw Error('Preferences is not set');
- }
- this.decideTimeFormat(preferences.time_format);
- this.decideDateFormat(preferences.date_format);
- }
-
private decideTimeFormat(timeFormat: TimeFormat) {
switch (timeFormat) {
case TimeFormat.HHMM_12:
@@ -195,12 +183,6 @@
}
}
- private async loadRelative() {
- const prefs = await this.restApiService.getPreferences();
- this.relative =
- this.forceRelative || Boolean(prefs?.relative_date_in_change_table);
- }
-
private computeDateStr() {
if (!this.dateStr || !this.timeFormat || !this.dateFormat) {
return '';
@@ -222,33 +204,24 @@
if (isWithinHalfYear(now, date)) {
format = this.dateFormat.short;
}
- if (this.showDateAndTime || this.showDateAndTime) {
+ if (this.showDateAndTime) {
format = `${format} ${this.timeFormat}`;
}
}
return formatDate(date, format);
}
- private computeFullDateStr() {
- if (
- [this.dateStr, this.timeFormat].includes(undefined) ||
- !this.dateFormat
- ) {
- return undefined;
- }
-
- if (!this.dateStr) {
- return '';
- }
+ private computeFullDateStr(): string {
+ if (!this.dateStr) return '';
+ if (!this.timeFormat) return '';
+ if (!this.dateFormat) return '';
const date = parseDate(this.dateStr as Timestamp);
- if (!isValidDate(date)) {
- return '';
- }
- let format = this.dateFormat.full + ', ';
- format +=
+ if (!isValidDate(date)) return '';
+ const timeFormat =
this.timeFormat === TimeFormats.TIME_12
? TimeFormats.TIME_12_WITH_SEC
: TimeFormats.TIME_24_WITH_SEC;
- return formatDate(date, format) + this._getUtcOffsetString();
+ const format = `dddd, ${this.dateFormat.full}, ${timeFormat}`;
+ return formatDate(date, format) + this.getUtcOffsetString();
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter_test.ts b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter_test.ts
index d7c38df..98f2d82 100644
--- a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter_test.ts
@@ -8,16 +8,15 @@
import {GrDateFormatter} from './gr-date-formatter';
import {parseDate} from '../../../utils/date-util';
import {fixture, html, assert} from '@open-wc/testing';
-import {query, queryAndAssert, stubRestApi} from '../../../test/test-utils';
+import {query, queryAndAssert} from '../../../test/test-utils';
import {GrTooltipContent} from '../gr-tooltip-content/gr-tooltip-content';
import {Timestamp} from '../../../api/rest-api';
-import {PreferencesInfo} from '../../../types/common';
-import {createPreferences} from '../../../test/test-data-generators';
import {
createDefaultPreferences,
DateFormat,
TimeFormat,
} from '../../../constants/constants';
+import {PreferencesInfo} from '../../../types/common';
const basicTemplate = html`
<gr-date-formatter withTooltip dateStr="2015-09-24 23:30:17.033000000">
@@ -41,6 +40,10 @@
return d;
}
+ function setPrefs(prefs: Partial<PreferencesInfo>) {
+ element.setPreferences({...createDefaultPreferences(), ...prefs});
+ }
+
async function testDates(
nowStr: string,
dateStr: string,
@@ -68,23 +71,11 @@
assert.equal(span.textContent?.trim(), expectedWithDateAndTime);
}
- function stubRestAPI(preferences?: PreferencesInfo) {
- stubRestApi('getLoggedIn').resolves(preferences !== undefined);
- stubRestApi('getPreferences').resolves(preferences);
- }
-
suite('STD + 24 hours time format preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_24,
- date_format: DateFormat.STD,
- relative_date_in_change_table: false,
- });
-
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.STD, time_format: TimeFormat.HHMM_24});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -93,7 +84,7 @@
'2015-07-29 15:34:14.985000000',
'15:34',
'15:34',
- 'Jul 29, 2015, 15:34:14'
+ 'Wednesday, Jul 29, 2015, 15:34:14'
);
});
@@ -103,7 +94,7 @@
'2015-07-28 20:25:14.985000000',
'Jul 28',
'Jul 28 20:25',
- 'Jul 28, 2015, 20:25:14'
+ 'Tuesday, Jul 28, 2015, 20:25:14'
);
});
@@ -113,7 +104,7 @@
'2015-06-15 03:25:14.985000000',
'Jun 15',
'Jun 15 03:25',
- 'Jun 15, 2015, 03:25:14'
+ 'Monday, Jun 15, 2015, 03:25:14'
);
});
@@ -123,22 +114,16 @@
'2015-01-15 03:25:00.000000000',
'Jan 15, 2015',
'Jan 15, 2015 03:25',
- 'Jan 15, 2015, 03:25:00'
+ 'Thursday, Jan 15, 2015, 03:25:00'
);
});
});
suite('US + 24 hours time format preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_24,
- date_format: DateFormat.US,
- relative_date_in_change_table: false,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.US, time_format: TimeFormat.HHMM_24});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -147,7 +132,7 @@
'2015-07-29 15:34:14.985000000',
'15:34',
'15:34',
- '07/29/15, 15:34:14'
+ 'Wednesday, 07/29/15, 15:34:14'
);
});
@@ -157,7 +142,7 @@
'2015-07-28 20:25:14.985000000',
'07/28',
'07/28 20:25',
- '07/28/15, 20:25:14'
+ 'Tuesday, 07/28/15, 20:25:14'
);
});
@@ -167,23 +152,16 @@
'2015-06-15 03:25:14.985000000',
'06/15',
'06/15 03:25',
- '06/15/15, 03:25:14'
+ 'Monday, 06/15/15, 03:25:14'
);
});
});
suite('ISO + 24 hours time format preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_24,
- date_format: DateFormat.ISO,
- relative_date_in_change_table: false,
- });
-
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.ISO, time_format: TimeFormat.HHMM_24});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -192,7 +170,7 @@
'2015-07-29 15:34:14.985000000',
'15:34',
'15:34',
- '2015-07-29, 15:34:14'
+ 'Wednesday, 2015-07-29, 15:34:14'
);
});
@@ -202,7 +180,7 @@
'2015-07-28 20:25:14.985000000',
'07-28',
'07-28 20:25',
- '2015-07-28, 20:25:14'
+ 'Tuesday, 2015-07-28, 20:25:14'
);
});
@@ -212,23 +190,16 @@
'2015-06-15 03:25:14.985000000',
'06-15',
'06-15 03:25',
- '2015-06-15, 03:25:14'
+ 'Monday, 2015-06-15, 03:25:14'
);
});
});
suite('EURO + 24 hours time format preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_24,
- date_format: DateFormat.EURO,
- relative_date_in_change_table: false,
- });
-
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.EURO, time_format: TimeFormat.HHMM_24});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -237,7 +208,7 @@
'2015-07-29 15:34:14.985000000',
'15:34',
'15:34',
- '29.07.2015, 15:34:14'
+ 'Wednesday, 29.07.2015, 15:34:14'
);
});
@@ -247,7 +218,7 @@
'2015-07-28 20:25:14.985000000',
'28. Jul',
'28. Jul 20:25',
- '28.07.2015, 20:25:14'
+ 'Tuesday, 28.07.2015, 20:25:14'
);
});
@@ -257,23 +228,16 @@
'2015-06-15 03:25:14.985000000',
'15. Jun',
'15. Jun 03:25',
- '15.06.2015, 03:25:14'
+ 'Monday, 15.06.2015, 03:25:14'
);
});
});
suite('UK + 24 hours time format preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_24,
- date_format: DateFormat.UK,
- relative_date_in_change_table: false,
- });
-
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.UK, time_format: TimeFormat.HHMM_24});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -282,7 +246,7 @@
'2015-07-29 15:34:14.985000000',
'15:34',
'15:34',
- '29/07/2015, 15:34:14'
+ 'Wednesday, 29/07/2015, 15:34:14'
);
});
@@ -292,7 +256,7 @@
'2015-07-28 20:25:14.985000000',
'28/07',
'28/07 20:25',
- '28/07/2015, 20:25:14'
+ 'Tuesday, 28/07/2015, 20:25:14'
);
});
@@ -302,22 +266,16 @@
'2015-06-15 03:25:14.985000000',
'15/06',
'15/06 03:25',
- '15/06/2015, 03:25:14'
+ 'Monday, 15/06/2015, 03:25:14'
);
});
});
suite('STD + 12 hours time format preference', () => {
setup(async () => {
- // relative_date_in_change_table is not set when false.
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
- date_format: DateFormat.STD,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.STD, time_format: TimeFormat.HHMM_12});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -326,22 +284,16 @@
'2015-07-29 15:34:14.985000000',
'3:34 PM',
'3:34 PM',
- 'Jul 29, 2015, 3:34:14 PM'
+ 'Wednesday, Jul 29, 2015, 3:34:14 PM'
);
});
});
suite('US + 12 hours time format preference', () => {
setup(async () => {
- // relative_date_in_change_table is not set when false.
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
- date_format: DateFormat.US,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.US, time_format: TimeFormat.HHMM_12});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -350,22 +302,16 @@
'2015-07-29 15:34:14.985000000',
'3:34 PM',
'3:34 PM',
- '07/29/15, 3:34:14 PM'
+ 'Wednesday, 07/29/15, 3:34:14 PM'
);
});
});
suite('ISO + 12 hours time format preference', () => {
setup(async () => {
- // relative_date_in_change_table is not set when false.
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
- date_format: DateFormat.ISO,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.ISO, time_format: TimeFormat.HHMM_12});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -374,22 +320,16 @@
'2015-07-29 15:34:14.985000000',
'3:34 PM',
'3:34 PM',
- '2015-07-29, 3:34:14 PM'
+ 'Wednesday, 2015-07-29, 3:34:14 PM'
);
});
});
suite('EURO + 12 hours time format preference', () => {
setup(async () => {
- // relative_date_in_change_table is not set when false.
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
- date_format: DateFormat.EURO,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.EURO, time_format: TimeFormat.HHMM_12});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -398,22 +338,16 @@
'2015-07-29 15:34:14.985000000',
'3:34 PM',
'3:34 PM',
- '29.07.2015, 3:34:14 PM'
+ 'Wednesday, 29.07.2015, 3:34:14 PM'
);
});
});
suite('UK + 12 hours time format preference', () => {
setup(async () => {
- // relative_date_in_change_table is not set when false.
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
- date_format: DateFormat.UK,
- });
element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ setPrefs({date_format: DateFormat.UK, time_format: TimeFormat.HHMM_12});
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -422,22 +356,20 @@
'2015-07-29 15:34:14.985000000',
'3:34 PM',
'3:34 PM',
- '29/07/2015, 3:34:14 PM'
+ 'Wednesday, 29/07/2015, 3:34:14 PM'
);
});
});
suite('relative date preference', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
+ element = await fixture(basicTemplate);
+ setPrefs({
date_format: DateFormat.STD,
+ time_format: TimeFormat.HHMM_12,
relative_date_in_change_table: true,
});
- element = await fixture(basicTemplate);
- sinon.stub(element, '_getUtcOffsetString').returns('');
- await element.loadPreferences();
+ sinon.stub(element, 'getUtcOffsetString').returns('');
});
test('Within 24 hours on same day', async () => {
@@ -446,7 +378,7 @@
'2015-07-29 15:34:14.985000000',
'5 hours ago',
'5 hours ago',
- 'Jul 29, 2015, 3:34:14 PM'
+ 'Wednesday, Jul 29, 2015, 3:34:14 PM'
);
});
@@ -456,21 +388,19 @@
'2015-01-15 03:25:00.000000000',
'8 months ago',
'8 months ago',
- 'Jan 15, 2015, 3:25:00 AM'
+ 'Thursday, Jan 15, 2015, 3:25:00 AM'
);
});
});
suite('logged in', () => {
setup(async () => {
- stubRestAPI({
- ...createPreferences(),
- time_format: TimeFormat.HHMM_12,
+ element = await fixture(basicTemplate);
+ setPrefs({
date_format: DateFormat.US,
+ time_format: TimeFormat.HHMM_12,
relative_date_in_change_table: true,
});
- element = await fixture(basicTemplate);
- await element.loadPreferences();
});
test('Preferences are respected', () => {
@@ -483,13 +413,12 @@
suite('logged out', () => {
setup(async () => {
- stubRestAPI(undefined);
element = await fixture(basicTemplate);
- await element.loadPreferences();
+ setPrefs({});
});
test('Default preferences are respected', () => {
- assert.equal(element.timeFormat, 'HH:mm');
+ assert.equal(element.timeFormat, 'h:mm A');
assert.equal(element.dateFormat?.short, 'MMM DD');
assert.equal(element.dateFormat?.full, 'MMM DD, YYYY');
assert.isFalse(element.relative);
@@ -498,9 +427,8 @@
suite('with tooltip', () => {
setup(async () => {
- stubRestAPI(createDefaultPreferences());
element = await fixture(basicTemplate);
- await element.loadPreferences();
+ setPrefs({});
await element.updateComplete;
});
@@ -515,9 +443,8 @@
suite('without tooltip', () => {
setup(async () => {
- stubRestAPI(createDefaultPreferences());
element = await fixture(lightTemplate);
- await element.loadPreferences();
+ setPrefs({});
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
index b952a3d..d40fdda 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
@@ -26,6 +26,7 @@
import '../gr-context-controls/gr-context-controls';
import '../gr-range-header/gr-range-header';
import './gr-diff-row';
+import {when} from 'lit/directives/when.js';
@customElement('gr-diff-section')
export class GrDiffSection extends LitElement {
@@ -170,7 +171,7 @@
`;
const moveCell = html`
<td class=${diffClasses('moveHeader')}>
- <gr-range-header class=${diffClasses()} icon="gr-icons:move-item">
+ <gr-range-header class=${diffClasses()} icon="move_item">
${this.renderMoveDescription(movedIn)}
</gr-range-header>
</td>
@@ -179,8 +180,13 @@
<tr
class=${diffClasses('moveControls', movedIn ? 'movedIn' : 'movedOut')}
>
- ${lineNumberCell} ${signCell} ${movedIn ? plainCell : moveCell}
- ${lineNumberCell} ${signCell} ${movedIn ? moveCell : plainCell}
+ ${when(
+ this.isUnifiedDiff(),
+ () => html`${lineNumberCell} ${lineNumberCell} ${moveCell}`,
+ () => html`${lineNumberCell} ${signCell}
+ ${movedIn ? plainCell : moveCell} ${lineNumberCell} ${signCell}
+ ${movedIn ? moveCell : plainCell}`
+ )}
</tr>
`;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
index 33b3df0..14f4536 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
@@ -9,7 +9,8 @@
import {fixture, html, assert} from '@open-wc/testing';
import {GrDiffGroup, GrDiffGroupType} from '../gr-diff/gr-diff-group';
import {GrDiffLine} from '../gr-diff/gr-diff-line';
-import {GrDiffLineType} from '../../../api/diff';
+import {DiffViewMode, GrDiffLineType} from '../../../api/diff';
+import {waitQueryAndAssert} from '../../../test/test-utils';
suite('gr-diff-section test', () => {
let element: GrDiffSection;
@@ -22,6 +23,93 @@
await element.updateComplete;
});
+ suite('move controls', async () => {
+ setup(async () => {
+ const lines = [new GrDiffLine(GrDiffLineType.BOTH, 1, 1)];
+ lines[0].text = 'asdf';
+ const group = new GrDiffGroup({
+ type: GrDiffGroupType.BOTH,
+ lines,
+ moveDetails: {changed: false, range: {start: 1, end: 2}},
+ });
+ element.group = group;
+ await element.updateComplete;
+ });
+
+ test('side-by-side', async () => {
+ const row = await waitQueryAndAssert(element, 'tr.moveControls');
+ // Semantic dom diff has a problem with just comparing table rows or
+ // cells directly. So as a workaround put the row into an empty test
+ // table.
+ const testTable = document.createElement('table');
+ testTable.appendChild(row);
+ assert.dom.equal(
+ testTable,
+ /* HTML */ `
+ <table>
+ <tbody>
+ <tr class="gr-diff moveControls movedOut">
+ <td class="gr-diff moveControlsLineNumCol"></td>
+ <td class="gr-diff sign"></td>
+ <td class="gr-diff moveHeader">
+ <gr-range-header class="gr-diff" icon="move_item">
+ <div class="gr-diff">
+ <span class="gr-diff"> Moved to lines </span>
+ <a class="gr-diff" href="#1"> 1 </a>
+ <span class="gr-diff"> - </span>
+ <a class="gr-diff" href="#2"> 2 </a>
+ </div>
+ </gr-range-header>
+ </td>
+ <td class="gr-diff moveControlsLineNumCol"></td>
+ <td class="gr-diff sign"></td>
+ <td class="gr-diff"></td>
+ </tr>
+ </tbody>
+ </table>
+ `,
+ {}
+ );
+ });
+
+ test('unified', async () => {
+ element.renderPrefs = {
+ ...element.renderPrefs,
+ view_mode: DiffViewMode.UNIFIED,
+ };
+ const row = await waitQueryAndAssert(element, 'tr.moveControls');
+ // Semantic dom diff has a problem with just comparing table rows or
+ // cells directly. So as a workaround put the row into an empty test
+ // table.
+ const testTable = document.createElement('table');
+ testTable.appendChild(row);
+ assert.dom.equal(
+ testTable,
+ /* HTML */ `
+ <table>
+ <tbody>
+ <tr class="gr-diff moveControls movedOut">
+ <td class="gr-diff moveControlsLineNumCol"></td>
+ <td class="gr-diff moveControlsLineNumCol"></td>
+ <td class="gr-diff moveHeader">
+ <gr-range-header class="gr-diff" icon="move_item">
+ <div class="gr-diff">
+ <span class="gr-diff"> Moved to lines </span>
+ <a class="gr-diff" href="#1"> 1 </a>
+ <span class="gr-diff"> - </span>
+ <a class="gr-diff" href="#2"> 2 </a>
+ </div>
+ </gr-range-header>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ `,
+ {}
+ );
+ });
+ });
+
test('3 normal unchanged rows', async () => {
const lines = [
new GrDiffLine(GrDiffLineType.BOTH, 1, 1),
diff --git a/polygerrit-ui/app/embed/diff/gr-range-header/gr-range-header_test.ts b/polygerrit-ui/app/embed/diff/gr-range-header/gr-range-header_test.ts
new file mode 100644
index 0000000..b31197d16
--- /dev/null
+++ b/polygerrit-ui/app/embed/diff/gr-range-header/gr-range-header_test.ts
@@ -0,0 +1,40 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import '../../../test/common-test-setup';
+import './gr-range-header';
+import {GrRangeHeader} from './gr-range-header';
+import {fixture, html, assert} from '@open-wc/testing';
+
+suite('gr-range-header test', () => {
+ let element: GrRangeHeader;
+
+ setup(async () => {
+ element = await fixture<GrRangeHeader>(
+ html`<gr-range-header></gr-range-header>`
+ );
+ await element.updateComplete;
+ });
+
+ test('renders', async () => {
+ element.filled = true;
+ element.icon = 'test-icon';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <div class="row">
+ <gr-icon
+ aria-hidden="true"
+ class="icon"
+ filled
+ icon="test-icon"
+ ></gr-icon>
+ <slot></slot>
+ </div>
+ `
+ );
+ });
+});
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index ba43eb4..da05bbe 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -121,6 +121,7 @@
const eventDetail: OpenFixPreviewEventDetail = {
patchNum: result.patchset as PatchSetNumber,
fixSuggestions,
+ onCloseFixPreviewCallbacks: [],
};
return {
name: 'Show Fix',
diff --git a/polygerrit-ui/app/models/views/change_test.ts b/polygerrit-ui/app/models/views/change_test.ts
index 1e71bbd..837e362 100644
--- a/polygerrit-ui/app/models/views/change_test.ts
+++ b/polygerrit-ui/app/models/views/change_test.ts
@@ -74,7 +74,7 @@
...createChangeViewState(),
repo: 'x+/y+/z+/w' as RepoName,
};
- assert.equal(createChangeUrl(state), '/c/x%2B/y%2B/z%2B/w/+/42');
+ assert.equal(createChangeUrl(state), '/c/x%252B/y%252B/z%252B/w/+/42');
});
test('createDiffUrl', () => {
@@ -85,7 +85,7 @@
};
assert.equal(
createDiffUrl(params),
- '/c/test-project/+/42/12/x%2By/path.cpp'
+ '/c/test-project/+/42/12/x%252By/path.cpp'
);
window.CANONICAL_PATH = '/base';
@@ -93,10 +93,10 @@
window.CANONICAL_PATH = undefined;
params.repo = 'test' as RepoName;
- assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%252By/path.cpp');
params.basePatchNum = 6 as BasePatchSetNum;
- assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%252By/path.cpp');
params.diffView = {
path: 'foo bar/my+file.txt%',
@@ -105,7 +105,7 @@
delete params.basePatchNum;
assert.equal(
createDiffUrl(params),
- '/c/test/+/42/2/foo+bar/my%2Bfile.txt%2525'
+ '/c/test/+/42/2/foo+bar/my%252Bfile.txt%2525'
);
params.diffView = {
@@ -129,7 +129,7 @@
repo: 'x+/y' as RepoName,
diffView: {path: 'x+y/path.cpp'},
};
- assert.equal(createDiffUrl(params), '/c/x%2B/y/+/42/12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/x%252B/y/+/42/12/x%252By/path.cpp');
});
test('createEditUrl', () => {
@@ -140,7 +140,7 @@
};
assert.equal(
createEditUrl(params),
- '/c/test-project/+/42/12/x%2By/path.cpp,edit#31'
+ '/c/test-project/+/42/12/x%252By/path.cpp,edit#31'
);
window.CANONICAL_PATH = '/base';
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index a5bc4bf..94e7f79 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -362,6 +362,7 @@
return Promise.resolve({});
},
getPreferences(): Promise<PreferencesInfo | undefined> {
+ // TODO: Use createDefaultPreferences() instead.
return Promise.resolve(createPreferences());
},
getProjectConfig(): Promise<ConfigInfo | undefined> {
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index b0f0dc0..5abac09 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -73,21 +73,17 @@
import {
AccountsVisibility,
AccountTag,
- AppTheme,
AuthType,
ChangeStatus,
CommentSide,
- DateFormat,
- DefaultBase,
+ createDefaultPreferences,
DefaultDisplayNameConfig,
- DiffViewMode,
EmailStrategy,
InheritedBooleanInfoConfiguredValue,
MergeabilityComputationBehavior,
RequirementStatus,
RevisionKind,
SubmitType,
- TimeFormat,
} from '../constants/constants';
import {formatDate} from '../utils/date-util';
import {GetDiffCommentsOutput} from '../services/gr-rest-api/gr-rest-api';
@@ -110,7 +106,7 @@
SubmitRequirementResultInfo,
SubmitRequirementStatus,
} from '../api/rest-api';
-import {CheckResult, RunResult} from '../models/checks/checks-model';
+import {CheckResult, CheckRun, RunResult} from '../models/checks/checks-model';
import {Category, RunStatus} from '../api/checks';
import {DiffInfo} from '../api/diff';
import {SearchViewState} from '../models/views/search';
@@ -687,18 +683,12 @@
};
}
-// TODO: Maybe reconcile with createDefaultPreferences() in constants.ts.
+// TODO: Do not change the values of createDefaultPreferences() here.
export function createPreferences(): PreferencesInfo {
return {
+ ...createDefaultPreferences(),
changes_per_page: 10,
- theme: AppTheme.AUTO,
- date_format: DateFormat.ISO,
- time_format: TimeFormat.HHMM_24,
- diff_view: DiffViewMode.SIDE_BY_SIDE,
- my: [],
- change_table: [],
email_strategy: EmailStrategy.ENABLED,
- default_base_for_merges: DefaultBase.AUTO_MERGE,
allow_browser_notifications: true,
};
}
@@ -1103,6 +1093,19 @@
};
}
+export function createRun(partial: Partial<CheckRun> = {}): CheckRun {
+ return {
+ attemptDetails: [],
+ checkName: 'test-name',
+ internalRunId: 'test-internal-run-id',
+ isLatestAttempt: true,
+ isSingleAttempt: true,
+ pluginName: 'test-plugin-name',
+ status: RunStatus.COMPLETED,
+ ...partial,
+ };
+}
+
export function createRunResult(): RunResult {
return {
attemptDetails: [],
@@ -1119,11 +1122,14 @@
};
}
-export function createCheckResult(): CheckResult {
+export function createCheckResult(
+ partial: Partial<CheckResult> = {}
+): CheckResult {
return {
category: Category.ERROR,
summary: 'error',
internalResultId: 'test-internal-result-id',
+ ...partial,
};
}
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index f642af7..e2612b0 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -26,10 +26,7 @@
MOVED_LINK_CLICKED = 'moved-link-clicked',
NETWORK_ERROR = 'network-error',
OPEN_FIX_PREVIEW = 'open-fix-preview',
- CLOSE_FIX_PREVIEW = 'close-fix-preview',
PAGE_ERROR = 'page-error',
- RECREATE_CHANGE_VIEW = 'recreate-change-view',
- RECREATE_DIFF_VIEW = 'recreate-diff-view',
RELOAD = 'reload',
REPLY = 'reply',
SERVER_ERROR = 'server-error',
@@ -65,7 +62,6 @@
'line-cursor-moved-out': LineNumberEvent;
'moved-link-clicked': MovedLinkClickedEvent;
'open-fix-preview': OpenFixPreviewEvent;
- 'close-fix-preview': CloseFixPreviewEvent;
'reply-to-comment': ReplyToCommentEvent;
/* prettier-ignore */
'reload': ReloadEvent;
@@ -158,13 +154,10 @@
export interface OpenFixPreviewEventDetail {
patchNum: PatchSetNum;
fixSuggestions: FixSuggestionInfo[];
+ onCloseFixPreviewCallbacks: ((fixapplied: boolean) => void)[];
}
export type OpenFixPreviewEvent = CustomEvent<OpenFixPreviewEventDetail>;
-export interface CloseFixPreviewEventDetail {
- fixApplied: boolean;
-}
-export type CloseFixPreviewEvent = CustomEvent<CloseFixPreviewEventDetail>;
export interface ReplyToCommentEventDetail {
content: string;
userWantsToEdit: boolean;
diff --git a/polygerrit-ui/app/utils/date-util.ts b/polygerrit-ui/app/utils/date-util.ts
index 72e6cb7..d95d24a 100644
--- a/polygerrit-ui/app/utils/date-util.ts
+++ b/polygerrit-ui/app/utils/date-util.ts
@@ -83,21 +83,17 @@
return diff < 180 * Duration.DAY;
}
-// TODO(dmfilippov): TS-Fix review this type. All fields here must be optional,
-// but this require some changes in the code. During JS->TS migration
-// we want to avoid code changes where possible, so for simplicity we
-// define it with almost all fields mandatory
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatToParts
interface DateTimeFormatParts {
- year: string;
- month: string;
- day: string;
- hour: string;
- minute: string;
- second: string;
- dayPeriod: string;
- dayperiod?: string;
- // Object can have other properties, but our code doesn't use it
- [key: string]: string | undefined;
+ year?: string;
+ month?: string;
+ day?: string;
+ hour?: string;
+ minute?: string;
+ second?: string;
+ // AM or PM
+ dayPeriod?: string;
+ weekday?: string;
}
export function formatDate(date: Date, format: string) {
@@ -117,6 +113,14 @@
}
}
+ if (format.includes('ddd')) {
+ if (format.includes('dddd')) {
+ options.weekday = 'long';
+ } else {
+ options.weekday = 'short';
+ }
+ }
+
if (format.includes('DD')) {
options.day = '2-digit';
}
@@ -146,15 +150,38 @@
locale = 'en-GB';
}
- const dtf = new Intl.DateTimeFormat(locale, options);
- const parts = dtf
- .formatToParts(date)
- .filter(o => o.type !== 'literal')
- .reduce((acc, o: Intl.DateTimeFormatPart) => {
- acc[o.type] = o.value;
- return acc;
- }, {} as DateTimeFormatParts);
- if (format.includes('YY')) {
+ // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatToParts
+ const dtfParts = new Intl.DateTimeFormat(locale, options).formatToParts(date);
+ const parts: DateTimeFormatParts = {};
+ for (const entry of dtfParts) {
+ switch (entry.type) {
+ case 'year':
+ parts.year = entry.value;
+ break;
+ case 'month':
+ parts.month = entry.value;
+ break;
+ case 'day':
+ parts.day = entry.value;
+ break;
+ case 'hour':
+ parts.hour = entry.value;
+ break;
+ case 'minute':
+ parts.minute = entry.value;
+ break;
+ case 'second':
+ parts.second = entry.value;
+ break;
+ case 'dayPeriod':
+ parts.dayPeriod = entry.value;
+ break;
+ case 'weekday':
+ parts.weekday = entry.value;
+ break;
+ }
+ }
+ if (parts.year && format.includes('YY')) {
if (format.includes('YYYY')) {
format = format.replace('YYYY', parts.year);
} else {
@@ -162,41 +189,50 @@
}
}
- if (format.includes('DD')) {
+ if (parts.day && format.includes('DD')) {
format = format.replace('DD', parts.day);
}
- if (format.includes('HH')) {
+ if (parts.hour && format.includes('HH')) {
format = format.replace('HH', parts.hour);
}
- if (format.includes('h')) {
+ if (parts.hour && format.includes('h')) {
format = format.replace('h', parts.hour);
}
- if (format.includes('mm')) {
+ if (parts.minute && format.includes('mm')) {
format = format.replace('mm', parts.minute);
}
- if (format.includes('ss')) {
+ if (parts.second && format.includes('ss')) {
format = format.replace('ss', parts.second);
}
- if (format.includes('A')) {
- if (parts.dayperiod) {
- // Workaround for chrome 70 and below
- format = format.replace('A', parts.dayperiod.toUpperCase());
- } else {
- format = format.replace('A', parts.dayPeriod.toUpperCase());
- }
+ if (parts.dayPeriod && format.includes('A')) {
+ format = format.replace('A', parts.dayPeriod.toUpperCase());
}
- if (format.includes('MM')) {
+
+ // Month and weekday must be last, because they will yield characters that
+ // could be interpreted as format strings, e.g. `h` in `Thursday` would
+ // otherwise be replaced by "hours".
+
+ if (parts.month && format.includes('MM')) {
if (format.includes('MMM')) {
format = format.replace('MMM', parts.month);
} else {
format = format.replace('MM', parts.month);
}
}
+
+ if (parts.weekday && format.includes('ddd')) {
+ if (format.includes('dddd')) {
+ format = format.replace('dddd', parts.weekday);
+ } else {
+ format = format.replace('ddd', parts.weekday);
+ }
+ }
+
return format;
}
diff --git a/polygerrit-ui/app/utils/date-util_test.ts b/polygerrit-ui/app/utils/date-util_test.ts
index 8e802b7..8d16655 100644
--- a/polygerrit-ui/app/utils/date-util_test.ts
+++ b/polygerrit-ui/app/utils/date-util_test.ts
@@ -194,6 +194,18 @@
)
);
});
+
+ test('weekday', () => {
+ assert.equal(
+ '2013-07-03 Wed',
+ formatDate(new Date('Jul 03 2013 12:14:00'), 'YYYY-MM-DD ddd')
+ );
+ assert.equal(
+ '2013-07-03 Wednesday',
+ formatDate(new Date('Jul 03 2013 00:15:00'), 'YYYY-MM-DD dddd')
+ );
+ });
+
test('h:mm:ss A shows correctly midnight and midday', () => {
const timeFormat = 'h:mm A';
assert.equal(
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts
index 714955b..49d5382 100644
--- a/polygerrit-ui/app/utils/event-util.ts
+++ b/polygerrit-ui/app/utils/event-util.ts
@@ -103,10 +103,6 @@
fire(target, EventType.SHOW_TAB, detail);
}
-export function fireCloseFixPreview(target: EventTarget, fixApplied: boolean) {
- fire(target, EventType.CLOSE_FIX_PREVIEW, {fixApplied});
-}
-
export function fireReload(target: EventTarget, clearPatchset?: boolean) {
fire(target, EventType.RELOAD, {clearPatchset: !!clearPatchset});
}
diff --git a/polygerrit-ui/app/utils/url-util.ts b/polygerrit-ui/app/utils/url-util.ts
index 5e294cb..af1e32e 100644
--- a/polygerrit-ui/app/utils/url-util.ts
+++ b/polygerrit-ui/app/utils/url-util.ts
@@ -92,6 +92,9 @@
// to not double encode *everything* (just for readaiblity and simplicity),
// but `%` *must* be double encoded.
let output = url.replaceAll('%', '%25');
+ // `+` also requires double encoding, because `%2B` would be decoded to `+`
+ // and then replaced by ` `.
+ output = output.replaceAll('+', '%2B');
// This escapes ALL characters EXCEPT:
// A–Z a–z 0–9 - _ . ! ~ * ' ( )
@@ -138,6 +141,10 @@
* Single decode for URL components. Will decode plus signs ('+') to spaces.
* Note: because this function decodes once, it is not the inverse of
* encodeURL.
+ *
+ * This function must only be used for decoding data returned by the REST API.
+ * Don't use it for decoding browser URLs. The only place for decoding browser
+ * URLs must gr-page.ts.
*/
export function singleDecodeURL(url: string): string {
const withoutPlus = url.replace(/\+/g, '%20');
diff --git a/polygerrit-ui/app/utils/url-util_test.ts b/polygerrit-ui/app/utils/url-util_test.ts
index 16f85dd..7466a90 100644
--- a/polygerrit-ui/app/utils/url-util_test.ts
+++ b/polygerrit-ui/app/utils/url-util_test.ts
@@ -110,6 +110,10 @@
assert.equal(encodeURL('abc%def'), 'abc%2525def');
});
+ test('double encodes +', () => {
+ assert.equal(encodeURL('abc+def'), 'abc%252Bdef');
+ });
+
test('does not encode colon and slash', () => {
assert.equal(encodeURL(':/'), ':/');
});
diff --git a/proto/entities.proto b/proto/entities.proto
index 0dc6441..f89e0f0 100644
--- a/proto/entities.proto
+++ b/proto/entities.proto
@@ -45,7 +45,6 @@
optional string topic = 14;
optional string original_subject = 17;
optional string submission_id = 18;
- optional Account_Id assignee = 19;
optional bool is_private = 20;
optional bool work_in_progress = 21;
optional bool review_started = 22;
@@ -59,6 +58,7 @@
reserved 11; // nbrPatchSets
reserved 15; // lastSha1MergeTested
reserved 16; // mergeable
+ reserved 19; // assignee
reserved 101; // note_db_state
}
diff --git a/tools/deps.bzl b/tools/deps.bzl
index 28904a9..4c21ae6 100644
--- a/tools/deps.bzl
+++ b/tools/deps.bzl
@@ -18,7 +18,7 @@
GITILES_REPO = GERRIT
# When updating Bouncy Castle, also update it in bazlets.
-BC_VERS = "1.64"
+BC_VERS = "1.72"
HTTPCOMP_VERS = "4.5.2"
JETTY_VERS = "9.4.49.v20220914"
BYTE_BUDDY_VERSION = "1.10.7"
@@ -558,20 +558,26 @@
maven_jar(
name = "bcprov",
- artifact = "org.bouncycastle:bcprov-jdk15on:" + BC_VERS,
- sha1 = "1467dac1b787b5ad2a18201c0c281df69882259e",
+ artifact = "org.bouncycastle:bcprov-jdk18on:" + BC_VERS,
+ sha1 = "d8dc62c28a3497d29c93fee3e71c00b27dff41b4",
)
maven_jar(
name = "bcpg",
- artifact = "org.bouncycastle:bcpg-jdk15on:" + BC_VERS,
- sha1 = "56956a8c63ccadf62e7c678571cf86f30bd84441",
+ artifact = "org.bouncycastle:bcpg-jdk18on:" + BC_VERS,
+ sha1 = "1a36a1740d07869161f6f0d01fae8d72dd1d8320",
)
maven_jar(
name = "bcpkix",
- artifact = "org.bouncycastle:bcpkix-jdk15on:" + BC_VERS,
- sha1 = "3dac163e20110817d850d17e0444852a6d7d0bd7",
+ artifact = "org.bouncycastle:bcpkix-jdk18on:" + BC_VERS,
+ sha1 = "bb3fdb5162ccd5085e8d7e57fada4d8eaa571f5a",
+ )
+
+ maven_jar(
+ name = "bcutil",
+ artifact = "org.bouncycastle:bcutil-jdk18on:" + BC_VERS,
+ sha1 = "41f19a69ada3b06fa48781120d8bebe1ba955c77",
)
maven_jar(
