diff options
30 files changed, 154 insertions, 84 deletions
diff --git a/Documentation/cmd-index-changes.txt b/Documentation/cmd-index-changes.txt index 0ee7aabd26..1d4cbe342e 100644 --- a/Documentation/cmd-index-changes.txt +++ b/Documentation/cmd-index-changes.txt @@ -16,8 +16,7 @@ Changes can be specified in the link:rest-api-changes.html#change-id[same format supported by the REST API. == ACCESS -Caller must have the 'Maintain Server' capability, or be the owner of the change -to be indexed. +Caller must have the 'Maintain Server' capability. == SCRIPTING This command is intended to be used in scripts. diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index fb5904bc81..dd7fa02db2 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1596,13 +1596,6 @@ the change. + Default is true. -[[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable:: -+ -Show assignee field in changes table. If set to false, assignees will -not be visible in changes table. -+ -Default is false. - [[change.strictLabels]]change.strictLabels:: + Reject invalid label votes: invalid labels or invalid values. This diff --git a/Documentation/linux-quickstart.txt b/Documentation/linux-quickstart.txt index 13873ed627..67f056563c 100644 --- a/Documentation/linux-quickstart.txt +++ b/Documentation/linux-quickstart.txt @@ -29,10 +29,10 @@ From the Linux machine on which you want to install Gerrit: . Download the desired Gerrit archive. To view previous archives, see -link:https://gerrit-releases.storage.googleapis.com/index.html[Gerrit Code Review: Releases,role=external,window=_blank]. The steps below install Gerrit 3.5.1: +link:https://gerrit-releases.storage.googleapis.com/index.html[Gerrit Code Review: Releases,role=external,window=_blank]. The steps below install Gerrit 3.9.4: .... -wget https://gerrit-releases.storage.googleapis.com/gerrit-3.5.1.war +wget https://gerrit-releases.storage.googleapis.com/gerrit-3.9.4.war .... NOTE: To build and install Gerrit from the source files, see diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index df5566fe10..a56766e36f 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -7103,6 +7103,12 @@ the current change index doesn't have the data. |`_number` || The change number. (The underscore is just a relict of a prior attempt to deprecate the change number.) +|`virtual_id_number` || +The virtual id number is globally unique. For local changes, it is equal to the +`_number` attribute. For imported changes, the original `_number` is processed +through a function designed to prevent conflicts with local change numbers. +Note that its usage is intended solely for Gerrit's internals and UI, and +adoption outside these scenarios is not advised. |`owner` || The owner of the change as an link:rest-api-accounts.html#account-info[ AccountInfo] entity. diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java index fad3aa84ab..3ad7e03e3f 100644 --- a/java/com/google/gerrit/entities/Change.java +++ b/java/com/google/gerrit/entities/Change.java @@ -434,7 +434,7 @@ public final class Change { private Id changeId; /** ServerId of the Gerrit instance that has created the change */ - private String serverId; + @Nullable private String serverId; /** Globally assigned unique identifier of the change */ private Key changeKey; @@ -545,7 +545,8 @@ public final class Change { * ServerId of the Gerrit instance that created the change. It could be null when the change is * not fetched from NoteDb but obtained through protobuf deserialisation. */ - public @Nullable String getServerId() { + @Nullable + public String getServerId() { return serverId; } @@ -607,6 +608,7 @@ public final class Change { return originalSubject != null ? originalSubject : subject; } + @Nullable public String getOriginalSubjectOrNull() { return originalSubject; } @@ -652,6 +654,7 @@ public final class Change { originalSubject = null; } + @Nullable public String getSubmissionId() { return submissionId; } @@ -684,6 +687,7 @@ public final class Change { return isAbandoned() || isMerged(); } + @Nullable public String getTopic() { return topic; } @@ -720,10 +724,12 @@ public final class Change { this.revertOf = revertOf; } + @Nullable public Id getRevertOf() { return this.revertOf; } + @Nullable public PatchSet.Id getCherryPickOf() { return cherryPickOf; } diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index d8fd727bcc..14e180512a 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -133,7 +133,7 @@ public interface ChangeApi { /** * Create a new change that reverts this change. * - * @see Changes#id(int) + * @see Changes#id(String, int) */ default ChangeApi revert() throws RestApiException { return revert(new RevertInput()); @@ -142,7 +142,7 @@ public interface ChangeApi { /** * Create a new change that reverts this change. * - * @see Changes#id(int) + * @see Changes#id(String, int) */ ChangeApi revert(RevertInput in) throws RestApiException; diff --git a/java/com/google/gerrit/extensions/api/changes/Changes.java b/java/com/google/gerrit/extensions/api/changes/Changes.java index ea2a158f00..9f70776166 100644 --- a/java/com/google/gerrit/extensions/api/changes/Changes.java +++ b/java/com/google/gerrit/extensions/api/changes/Changes.java @@ -31,14 +31,10 @@ public interface Changes { /** * Look up a change by numeric ID. * - * <p><strong>Note:</strong> This method eagerly reads the change. Methods that mutate the change - * do not necessarily re-read the change. Therefore, calling a getter method on an instance after - * calling a mutation method on that same instance is not guaranteed to reflect the mutation. It - * is not recommended to store references to {@code ChangeApi} instances. Also note that the - * change numeric id without a project name parameter may fail to identify a unique change - * element, because the potential conflict with other changes imported from Gerrit instances with - * a different Server-Id. + * <p><strong>Note:</strong> Change number is not guaranteed to unambiguously identify a change. * + * @see #id(String, int) + * @deprecated in favor of {@link #id(String, int)} * @param id change number. * @return API for accessing the change. * @throws RestApiException if an error occurred. @@ -49,7 +45,7 @@ public interface Changes { /** * Look up a change by string ID. * - * @see #id(int) + * @see #id(String, int) * @param id any identifier supported by the REST API, including change number, Change-Id, or * project~branch~Change-Id triplet. * @return API for accessing the change. @@ -60,16 +56,23 @@ public interface Changes { /** * Look up a change by project, branch, and change ID. * - * @see #id(int) + * @see #id(String, int) */ ChangeApi id(String project, String branch, String id) throws RestApiException; /** * Look up a change by project and numeric ID. * + * <p><strong>Note:</strong> This method eagerly reads the change. Methods that mutate the change + * do not necessarily re-read the change. Therefore, calling a getter method on an instance after + * calling a mutation method on that same instance is not guaranteed to reflect the mutation. It + * is not recommended to store references to {@code ChangeApi} instances. Also note that the + * change numeric id without a project name parameter may fail to identify a unique change + * element, because the potential conflict with other changes imported from Gerrit instances with + * a different Server-Id. + * * @param project project name. * @param id change number. - * @see #id(int) */ ChangeApi id(String project, int id) throws RestApiException; diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java index 69160e96ab..90c3a92051 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java @@ -102,7 +102,7 @@ public class ChangeInfo { public Boolean containsGitConflicts; public Integer _number; - public Integer _virtualIdNumber; + public Integer virtualIdNumber; public AccountInfo owner; diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 2fce4755b5..1a9e4f8520 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -784,7 +784,7 @@ public class ChangeJson { .collect(toList()); } - out._virtualIdNumber = cd.virtualId().get(); + out.virtualIdNumber = cd.virtualId().get(); return out; } @@ -977,7 +977,7 @@ public class ChangeJson { // repository only once try (Repository allUsersRepo = repoManager.openRepository(allUsers)) { List<Change.Id> changeIds = - changeInfos.stream().map(c -> Change.id(c._virtualIdNumber)).collect(Collectors.toList()); + changeInfos.stream().map(c -> Change.id(c.virtualIdNumber)).collect(Collectors.toList()); Set<Change.Id> starredChanges = starredChangesreader.areStarred( allUsersRepo, changeIds, userProvider.get().asIdentifiedUser().getAccountId()); @@ -985,7 +985,7 @@ public class ChangeJson { return; } changeInfos.stream() - .forEach(c -> c.starred = starredChanges.contains(Change.id(c._virtualIdNumber))); + .forEach(c -> c.starred = starredChanges.contains(Change.id(c.virtualIdNumber))); } catch (IOException e) { logger.atWarning().withCause(e).log("Failed to open All-Users repo."); } diff --git a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java index 094287b7be..cb4fcff4fa 100644 --- a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java +++ b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java @@ -24,7 +24,6 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.cache.CacheModule; -import com.google.gerrit.server.git.ChangesByProjectCache.UseIndex; import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.logging.Metadata; import com.google.gerrit.server.logging.TraceContext; @@ -286,6 +285,7 @@ public class ChangesByProjectCacheImpl implements ChangesByProjectCache { int size = 0; size += JavaWeights.OBJECT; // change size += JavaWeights.REFERENCE + GerritWeights.KEY_INT; // changeId + size += JavaWeights.REFERENCE + (c.getServerId() == null ? 0 : c.getServerId().length()); size += JavaWeights.REFERENCE + JavaWeights.OBJECT + 40; // changeKey; size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // createdOn; size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // lastUpdatedOn; @@ -300,15 +300,17 @@ public class ChangesByProjectCacheImpl implements ChangesByProjectCache { size += JavaWeights.REFERENCE + (c.getTopic() == null ? 0 : c.getTopic().length()); size += JavaWeights.REFERENCE - + (c.getOriginalSubject().equals(c.getSubject()) ? 0 : c.getSubject().length()); + + (c.getOriginalSubject().equals(c.getSubject()) + ? 0 + : c.getOriginalSubject().length()); size += JavaWeights.REFERENCE + (c.getSubmissionId() == null ? 0 : c.getSubmissionId().length()); - size += JavaWeights.REFERENCE + GerritWeights.ACCOUNT_ID; // assignee; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // isPrivate; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // workInProgress; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // reviewStarted; - size += JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM; // revertOf; - size += JavaWeights.REFERENCE + GerritWeights.PACTCH_SET_ID; // cherryPickOf; + size += JavaWeights.REFERENCE + (c.getRevertOf() == null ? 0 : GerritWeights.CHANGE_NUM); + size += + JavaWeights.REFERENCE + (c.getCherryPickOf() == null ? 0 : GerritWeights.PATCH_SET_ID); return size; } @@ -342,7 +344,7 @@ public class ChangesByProjectCacheImpl implements ChangesByProjectCache { public static final int KEY_INT = JavaWeights.OBJECT + JavaWeights.INT; // IntKey public static final int CHANGE_NUM = KEY_INT; public static final int ACCOUNT_ID = KEY_INT; - public static final int PACTCH_SET_ID = + public static final int PATCH_SET_ID = JavaWeights.OBJECT + (JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM) // PatchSet.Id.changeId + JavaWeights.INT; // PatchSet.Id patch_num; diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java index 86d6c7c7d2..ce2d5b654c 100644 --- a/java/com/google/gerrit/server/git/WorkQueue.java +++ b/java/com/google/gerrit/server/git/WorkQueue.java @@ -695,7 +695,7 @@ public class WorkQueue { try { executor.onStart(this); runningState.set(State.RUNNING); - Thread.currentThread().setName(oldThreadName + "[" + task.toString() + "]"); + Thread.currentThread().setName(oldThreadName + "[" + this + "]"); task.run(); } finally { Thread.currentThread().setName(oldThreadName); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index cbc3f9da4c..a5c687b550 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java @@ -2650,8 +2650,8 @@ public class AttentionSetIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("\nPatch Set 2: Code-Review+2\n"); assertThat(message.body()) .contains("The change is no longer submittable: Code-Review is unsatisfied now.\n"); @@ -2736,8 +2736,8 @@ public class AttentionSetIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("\nPatch Set 2: Code-Review+2\n"); assertThat(message.body()).contains("\nPatch Set 2: Code-Review+1\n"); assertThat(message.body()) @@ -2824,8 +2824,8 @@ public class AttentionSetIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).contains("\nPatch Set 2: Code-Review-2\n"); assertThat(message.body()) .contains("The change is no longer submittable: Code-Review is unsatisfied now.\n"); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java index 1094a4293b..5bb6dc4249 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java @@ -83,8 +83,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()) .contains("The change is no longer submittable: Code-Review is unsatisfied now.\n"); assertThat(message.htmlBody()) @@ -127,8 +127,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()) .contains("The change is no longer submittable: Code-Review is unsatisfied now.\n"); assertThat(message.htmlBody()) @@ -172,8 +172,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()) .contains("The change is no longer submittable: Code-Review is unsatisfied now.\n"); assertThat(message.htmlBody()) @@ -263,8 +263,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()) .contains( "The change is no longer submittable:" @@ -315,8 +315,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("The change is no longer submittable"); assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable"); } @@ -361,8 +361,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("The change is no longer submittable"); assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable"); } @@ -398,8 +398,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("The change is no longer submittable"); assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable"); } @@ -435,8 +435,8 @@ public class ChangeNoLongerSubmittableIT extends AbstractDaemonTest { String.format( "Attention is currently required from: %s, %s.\n" + "\n" - + "%s has posted comments on this change.", - admin.fullName(), user.fullName(), approver.fullName())); + + "%s has posted comments on this change by %s.", + admin.fullName(), user.fullName(), approver.fullName(), admin.fullName())); assertThat(message.body()).doesNotContain("The change is no longer submittable"); assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable"); } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java index 6fc9b2b205..b1b1887785 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java @@ -449,7 +449,9 @@ public class ChangeReviewersIT extends AbstractDaemonTest { Message m = messages.get(0); assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail()); - assertThat(m.body()).contains(admin.fullName() + " has posted comments on this change."); + assertThat(m.body()) + .contains( + admin.fullName() + " has posted comments on this change by " + admin.fullName() + "."); assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n"); assertThat(m.body()).contains("Patch Set 1: Code-Review+2"); diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor -Subproject d4f9247d3efb6a0e461af701986235511d05b7e +Subproject ce9838b8795338877f74c7f3b61c7c4526a279e diff --git a/plugins/package.json b/plugins/package.json index 9e920868bf..4a4bf03b05 100644 --- a/plugins/package.json +++ b/plugins/package.json @@ -26,7 +26,7 @@ "@codemirror/search": "^6.5.4", "@codemirror/state": "^6.2.1", "@codemirror/view": "^6.20.2", - "@gerritcodereview/typescript-api": "3.8.0", + "@gerritcodereview/typescript-api": "3.9.1", "@open-wc/testing": "^3.2.0", "@polymer/decorators": "^3.0.0", "@polymer/polymer": "^3.5.1", diff --git a/plugins/yarn.lock b/plugins/yarn.lock index 21ec5228ad..045fcb5f24 100644 --- a/plugins/yarn.lock +++ b/plugins/yarn.lock @@ -425,10 +425,10 @@ dependencies: "@types/chai" "^4.2.12" -"@gerritcodereview/typescript-api@3.8.0": - version "3.8.0" - resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.8.0.tgz#2e418b814d7451c40365b2dc4f88e9965ece0769" - integrity sha512-wUkIWUx99Rj1vxRYQISxyzN0nplqu7t5sRDyJ8R3yNNkvALQAMC6Whj63qzCsZsymVFzC5up3y+ZVxaeh7b+xA== +"@gerritcodereview/typescript-api@3.9.1": + version "3.9.1" + resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.9.1.tgz#c532e9a39e3d5f16f6a5cb5691988c04cd477531" + integrity sha512-5t8CBhlqQEcjJqNld1/ajcdZjjyrv7vsn4u0N3mX4hc4DnPJimIjYVFvP8nLyhSkioawWDIRLvzlmfzFs02lDg== "@jridgewell/resolve-uri@^3.1.0": version "3.1.1" diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts index 58c10f953c..ae59fb62c2 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts @@ -205,16 +205,21 @@ export class GrMainHeader extends LitElement { nav { align-items: center; display: flex; - flex-wrap: wrap; } .bigTitle { color: var(--header-text-color); font-size: var(--header-title-font-size); + line-height: calc(var(--header-title-font-size) * 1.2); text-decoration: none; } .bigTitle:hover { text-decoration: underline; } + .titleText { + /* Vertical alignment of icons and text with just block/inline display is too troublesome. */ + display: flex; + align-items: center; + } .titleText::before { --icon-width: var(--header-icon-width, var(--header-icon-size, 0)); --icon-height: var(--header-icon-height, var(--header-icon-size, 0)); @@ -222,14 +227,15 @@ export class GrMainHeader extends LitElement { background-size: var(--icon-width) var(--icon-height); background-repeat: no-repeat; content: ''; - display: inline-block; + /* Any direct child of a flex element implicitly has 'display: block', but let's make that explicit here. */ + display: block; + width: var(--icon-width); height: var(--icon-height); /* If size or height are set, then use 'spacing-m', 0px otherwise. */ margin-right: clamp(0px, var(--icon-height), var(--spacing-m)); - vertical-align: text-bottom; - width: var(--icon-width); } .titleText::after { + /* The height will be determined by the line-height of the .bigTitle element. */ content: var(--header-title-content); white-space: nowrap; } @@ -301,6 +307,7 @@ export class GrMainHeader extends LitElement { display: inline; } .accountContainer { + flex: 0 0 auto; align-items: center; display: flex; margin: 0 calc(0 - var(--spacing-m)) 0 var(--spacing-m); @@ -368,7 +375,7 @@ export class GrMainHeader extends LitElement { <nav> <a href=${`//${window.location.host}${getBaseUrl()}/`} class="bigTitle"> <gr-endpoint-decorator name="header-title"> - <span class="titleText"></span> + <div class="titleText"></div> </gr-endpoint-decorator> </a> <ul class="links"> diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts index dfb44b70e1..40430fb53a 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts @@ -42,7 +42,7 @@ suite('gr-main-header tests', () => { <nav> <a class="bigTitle" href="//localhost:9876/"> <gr-endpoint-decorator name="header-title"> - <span class="titleText"> </span> + <div class="titleText"></div> </gr-endpoint-decorator> </a> <ul class="links"> diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts index cf7ff22090..1db484cab2 100644 --- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts +++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts @@ -10,7 +10,11 @@ import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator'; import '../../plugins/gr-endpoint-param/gr-endpoint-param'; import {getAppContext} from '../../../services/app-context'; import {getDisplayName} from '../../../utils/display-name-util'; -import {isSelf, isServiceUser} from '../../../utils/account-util'; +import { + isDetailedAccount, + isSelf, + isServiceUser, +} from '../../../utils/account-util'; import {ChangeInfo, AccountInfo, ServerInfo} from '../../../types/common'; import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util'; import {fire} from '../../../utils/event-util'; @@ -191,8 +195,20 @@ export class GrAccountLabel extends LitElement { override async updated() { assertIsDefined(this.account, 'account'); + if (isDetailedAccount(this.account)) return; const account = await this.getAccountsModel().fillDetails(this.account); - if (account) this.account = account; + if (!isDetailedAccount(account)) return; + // AccountInfo returned by fillDetails has the email property set + // to the primary email of the account. This poses a problem in + // cases where a secondary email is used as the committer or author + // email. Therefore, only fill in the *missing* properties. + if ( + account && + account !== this.account && + account._account_id === this.account._account_id + ) { + this.account = {...account, ...this.account}; + } } override render() { diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model.ts b/polygerrit-ui/app/models/accounts-model/accounts-model.ts index 0802f0645b..6eedcbe808 100644 --- a/polygerrit-ui/app/models/accounts-model/accounts-model.ts +++ b/polygerrit-ui/app/models/accounts-model/accounts-model.ts @@ -42,7 +42,7 @@ export class AccountsModel extends Model<AccountsState> { ): Promise<AccountDetailInfo | AccountInfo> { const current = this.getState(); const id = getUserId(partialAccount); - if (hasOwnProperty(current.accounts, id)) return current.accounts[id]; + if (hasOwnProperty(current.accounts, id)) return {...current.accounts[id]}; // It is possible to add emails to CC when they don't have a Gerrit // account. In this case getAccountDetails will return a 404 error then // we at least use what is in partialAccount. diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts b/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts index 53c90a67ea..e84723c6b2 100644 --- a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts +++ b/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts @@ -5,12 +5,23 @@ */ import '../../test/common-test-setup'; -import {EmailAddress} from '../../api/rest-api'; +import { + AccountDetailInfo, + AccountId, + EmailAddress, + Timestamp, +} from '../../api/rest-api'; import {getAppContext} from '../../services/app-context'; import {stubRestApi} from '../../test/test-utils'; import {AccountsModel} from './accounts-model'; import {assert} from '@open-wc/testing'; +const KERMIT: AccountDetailInfo = { + _account_id: 1 as AccountId, + name: 'Kermit', + registered_on: '2015-03-12 18:32:08.000000000' as Timestamp, +}; + suite('accounts-model tests', () => { let model: AccountsModel; @@ -22,6 +33,24 @@ suite('accounts-model tests', () => { model.finalize(); }); + test('basic lookup', async () => { + const stub = stubRestApi('getAccountDetails').returns( + Promise.resolve(KERMIT) + ); + + let filled = await model.fillDetails({_account_id: 1 as AccountId}); + assert.equal(filled.name, 'Kermit'); + assert.equal(filled, KERMIT); + assert.equal(stub.callCount, 1); + + filled = await model.fillDetails({_account_id: 1 as AccountId}); + assert.equal(filled.name, 'Kermit'); + // Cache objects are cloned on lookup, so this is a different object. + assert.notEqual(filled, KERMIT); + // Did not have to call the REST API again. + assert.equal(stub.callCount, 1); + }); + test('invalid account makes only one request', () => { const response = {...new Response(), status: 404}; const getAccountDetails = stubRestApi('getAccountDetails').callsFake( diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts index 08539411f0..b93acc61cc 100644 --- a/polygerrit-ui/app/utils/account-util.ts +++ b/polygerrit-ui/app/utils/account-util.ts @@ -140,10 +140,10 @@ export function uniqueAccountId( export function isDetailedAccount(account?: AccountInfo) { // In case ChangeInfo is requested without DetailedAccount option, the - // reviewer entry is returned as just {_account_id: 123} - // This object should also be treated as not detailed account if they have - // an AccountId and no email - return !!account?.email && !!account?._account_id; + // reviewer entry is returned as just {_account_id: 123}. + // At least a name or an email must be set for the account to be treated as + // "detailed". + return (!!account?.email || !!account?.name) && !!account?._account_id; } /** diff --git a/polygerrit-ui/app/utils/account-util_test.ts b/polygerrit-ui/app/utils/account-util_test.ts index 72fa7911a3..b1ee50ee4c 100644 --- a/polygerrit-ui/app/utils/account-util_test.ts +++ b/polygerrit-ui/app/utils/account-util_test.ts @@ -263,6 +263,7 @@ suite('account-util tests', () => { test('isDetailedAccount', () => { assert.isFalse(isDetailedAccount({_account_id: 12345 as AccountId})); assert.isFalse(isDetailedAccount({email: 'abcd' as EmailAddress})); + assert.isFalse(isDetailedAccount({name: 'Kermit'})); assert.isTrue( isDetailedAccount({ @@ -270,6 +271,12 @@ suite('account-util tests', () => { email: 'abcd' as EmailAddress, }) ); + assert.isTrue( + isDetailedAccount({ + _account_id: 12345 as AccountId, + name: 'Kermit', + }) + ); }); test('fails gracefully when all is not included', async () => { diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy index 4b621b5670..8c1840cb2f 100644 --- a/resources/com/google/gerrit/server/mail/Comment.soy +++ b/resources/com/google/gerrit/server/mail/Comment.soy @@ -29,7 +29,7 @@ {@param unsatisfiedSubmitRequirements: ?} {@param oldSubmitRequirements: ?} {@param newSubmitRequirements: ?} - {$fromName} has posted comments on this change. + {$fromName} has posted comments on this change by {$change.ownerName}. {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n} {if $unsatisfiedSubmitRequirements} {\n} diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 8c6a2198f8..bc54c6d383 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-acceptance-framework</artifactId> - <version>3.9.5-SNAPSHOT</version> + <version>3.9.6-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Acceptance Test Framework</name> <description>Framework for Gerrit's acceptance tests</description> diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 30725d0525..1c8e1f937d 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-extension-api</artifactId> - <version>3.9.5-SNAPSHOT</version> + <version>3.9.6-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Extension API</name> <description>API for Gerrit Extensions</description> diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index 2c2c270f65..83c7dcaa17 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-plugin-api</artifactId> - <version>3.9.5-SNAPSHOT</version> + <version>3.9.6-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Plugin API</name> <description>API for Gerrit Plugins</description> diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index fd87510dcc..d2f003ba7a 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-war</artifactId> - <version>3.9.5-SNAPSHOT</version> + <version>3.9.6-SNAPSHOT</version> <packaging>war</packaging> <name>Gerrit Code Review - WAR</name> <description>Gerrit WAR</description> diff --git a/version.bzl b/version.bzl index 6107a502b8..579f5cd975 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "3.9.5-SNAPSHOT" +GERRIT_VERSION = "3.9.6-SNAPSHOT" |