Merge "Add resolve-all-users=true options"
diff --git a/README.md b/README.md
index 26e2c93..02330f1 100644
--- a/README.md
+++ b/README.md
@@ -5,8 +5,7 @@
 If the code-owners plugin is enabled, changes can only be submitted if all
 touched files are covered by approvals from code owners.
 
-Also see resources/Documentation/about.md
+Also see [resources/Documentation/about.md](./resources/Documentation/about.md).
 
 IMPORTANT: Before installing/enabling the plugin follow the instructions from
-the setup guide, see resources/Documentation/setup-guide.md
-
+the setup guide, see [resources/Documentation/setup-guide.md](./resources/Documentation/setup-guide.md).
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 1e76b05..787e452 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -59,6 +59,8 @@
 public class ChangedFiles {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  private static int MAX_CHANGED_FILES_TO_LOG = 25;
+
   private final GitRepositoryManager repoManager;
   private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
   private final PatchListCache patchListCache;
@@ -203,7 +205,14 @@
       List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, revCommit);
       ImmutableSet<ChangedFile> changedFiles =
           diffEntries.stream().map(ChangedFile::create).collect(toImmutableSet());
-      logger.atFine().log("changed files = %s", changedFiles);
+      if (changedFiles.size() <= MAX_CHANGED_FILES_TO_LOG) {
+        logger.atFine().log("changed files = %s", changedFiles);
+      } else {
+        logger.atFine().log(
+            "changed files = %s (and %d more)",
+            changedFiles.asList().subList(0, MAX_CHANGED_FILES_TO_LOG),
+            changedFiles.size() - MAX_CHANGED_FILES_TO_LOG);
+      }
       return changedFiles;
     }
   }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 5cb187f..cd874ee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -21,6 +21,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.Project;
@@ -302,24 +303,14 @@
       return OptionalResultWithMessages.createEmpty(messages);
     }
 
-    OptionalResultWithMessages<Account.Id> lookupEmailResult = lookupEmail(email);
-    messages.addAll(lookupEmailResult.messages());
-    if (lookupEmailResult.isEmpty()) {
+    OptionalResultWithMessages<AccountState> activeAccountResult =
+        lookupActiveAccountForEmail(email);
+    messages.addAll(activeAccountResult.messages());
+    if (activeAccountResult.isEmpty()) {
       return OptionalResultWithMessages.createEmpty(messages);
     }
 
-    Account.Id accountId = lookupEmailResult.get();
-    OptionalResultWithMessages<AccountState> lookupAccountResult = lookupAccount(accountId, email);
-    messages.addAll(lookupAccountResult.messages());
-    if (lookupAccountResult.isEmpty()) {
-      return OptionalResultWithMessages.createEmpty(messages);
-    }
-
-    AccountState accountState = lookupAccountResult.get();
-    if (!accountState.account().isActive()) {
-      messages.add(String.format("account %s for email %s is inactive", accountId, email));
-      return OptionalResultWithMessages.createEmpty(messages);
-    }
+    AccountState accountState = activeAccountResult.get();
     if (enforceVisibility) {
       OptionalResultWithMessages<Boolean> isVisibleResult = isVisible(accountState, email);
       messages.addAll(isVisibleResult.messages());
@@ -343,15 +334,15 @@
   }
 
   /**
-   * Looks up an email and returns the ID of the account to which it belongs.
+   * Looks up an email and returns the ID of the active account to which it belongs.
    *
-   * <p>If the email is ambiguous (it belongs to multiple accounts) it is considered as
-   * non-resolvable and {@link Optional#empty()} is returned.
+   * <p>If the email is ambiguous (it belongs to multiple active accounts) it is considered as
+   * non-resolvable and empty result is returned.
    *
    * @param email the email that should be looked up
    * @return the ID of the account to which the email belongs if was found
    */
-  private OptionalResultWithMessages<Account.Id> lookupEmail(String email) {
+  private OptionalResultWithMessages<AccountState> lookupActiveAccountForEmail(String email) {
     ImmutableSet<ExternalId> extIds;
     try {
       extIds = externalIds.byEmail(email);
@@ -366,12 +357,51 @@
               "cannot resolve code owner email %s: no account with this email exists", email));
     }
 
-    if (extIds.stream().map(ExternalId::accountId).distinct().count() > 1) {
-      return OptionalResultWithMessages.createEmpty(
-          String.format("cannot resolve code owner email %s: email is ambiguous", email));
+    List<String> messages = new ArrayList<>();
+    OptionalResultWithMessages<ImmutableSet<AccountState>> activeAccountsResult =
+        lookupActiveAccounts(extIds, email);
+    ImmutableSet<AccountState> activeAccounts = activeAccountsResult.get();
+    messages.addAll(activeAccountsResult.messages());
+
+    if (activeAccounts.isEmpty()) {
+      messages.add(
+          String.format(
+              "cannot resolve code owner email %s: no active account with this email found",
+              email));
+      return OptionalResultWithMessages.createEmpty(messages);
     }
 
-    return OptionalResultWithMessages.create(extIds.stream().findFirst().get().accountId());
+    if (activeAccounts.size() > 1) {
+      messages.add(String.format("cannot resolve code owner email %s: email is ambiguous", email));
+      return OptionalResultWithMessages.createEmpty(messages);
+    }
+
+    return OptionalResultWithMessages.create(Iterables.getOnlyElement(activeAccounts));
+  }
+
+  private OptionalResultWithMessages<ImmutableSet<AccountState>> lookupActiveAccounts(
+      ImmutableSet<ExternalId> extIds, String email) {
+    ImmutableSet<OptionalResultWithMessages<AccountState>> accountStateResults =
+        extIds.stream()
+            .map(externalId -> lookupAccount(externalId.accountId(), externalId.email()))
+            .collect(toImmutableSet());
+
+    ImmutableSet.Builder<AccountState> activeAccounts = ImmutableSet.builder();
+    List<String> messages = new ArrayList<>();
+    for (OptionalResultWithMessages<AccountState> accountStateResult : accountStateResults) {
+      messages.addAll(accountStateResult.messages());
+      if (accountStateResult.isPresent()) {
+        AccountState accountState = accountStateResult.get();
+        if (accountState.account().isActive()) {
+          activeAccounts.add(accountState);
+        } else {
+          messages.add(
+              String.format(
+                  "account %s for email %s is inactive", accountState.account().id(), email));
+        }
+      }
+    }
+    return OptionalResultWithMessages.create(activeAccounts.build(), messages);
   }
 
   /**
@@ -388,9 +418,7 @@
     if (!accountState.isPresent()) {
       return OptionalResultWithMessages.createEmpty(
           String.format(
-              "cannot resolve code owner email %s: email belongs to account %s,"
-                  + " but no account with this ID exists",
-              email, accountId));
+              "cannot resolve account %s for email %s: account does not exists", accountId, email));
     }
     return OptionalResultWithMessages.create(accountState.get());
   }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index 21f411c..2dc2d37 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -73,6 +73,10 @@
     try {
       requireNonNull(changeData, "changeData");
 
+      if (changeData.change().isClosed()) {
+        return Optional.empty();
+      }
+
       try (Timer0.Context ctx = codeOwnerMetrics.runCodeOwnerSubmitRule.start()) {
         logger.atFine().log(
             "run code owner submit rule (project = %s, change = %d)",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 92db861..8e57412 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -35,7 +35,7 @@
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -61,7 +61,7 @@
   private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
   private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
   private final Provider<CurrentUser> userProvider;
-  private final BatchUpdate.Factory batchUpdateFactory;
+  private final RetryHelper retryHelper;
   private final ChangeNotes.Factory changeNotesFactory;
   private final AccountCache accountCache;
   private final ChangeMessagesUtil changeMessageUtil;
@@ -71,14 +71,14 @@
       CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
       CodeOwnerApprovalCheck codeOwnerApprovalCheck,
       Provider<CurrentUser> userProvider,
-      BatchUpdate.Factory batchUpdateFactory,
+      RetryHelper retryHelper,
       ChangeNotes.Factory changeNotesFactory,
       AccountCache accountCache,
       ChangeMessagesUtil changeMessageUtil) {
     this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
     this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
     this.userProvider = userProvider;
-    this.batchUpdateFactory = batchUpdateFactory;
+    this.retryHelper = retryHelper;
     this.changeNotesFactory = changeNotesFactory;
     this.accountCache = accountCache;
     this.changeMessageUtil = changeMessageUtil;
@@ -95,11 +95,20 @@
       return;
     }
 
-    try (BatchUpdate batchUpdate =
-        batchUpdateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
-      batchUpdate.addOp(changeId, new Op(event.getReviewers()));
-      batchUpdate.execute();
-    } catch (RestApiException | UpdateException e) {
+    try {
+      retryHelper
+          .changeUpdate(
+              "addCodeOwnersMessageOnAddReviewer",
+              updateFactory -> {
+                try (BatchUpdate batchUpdate =
+                    updateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
+                  batchUpdate.addOp(changeId, new Op(event.getReviewers()));
+                  batchUpdate.execute();
+                }
+                return null;
+              })
+          .call();
+    } catch (Exception e) {
       logger.atSevere().withCause(e).log(
           String.format(
               "Failed to post code-owners change message for reviewer on change %s in project %s.",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index dff2876..1cf59ce 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -199,16 +199,7 @@
                 newVote, user.getName()));
       }
     } else {
-      logger.atSevere().log(
-          "code owner approval was neither newly applied, removed, upgraded nor downgraded:"
-              + " project = %s, change = %s, patch set = %d, oldApprvals = %s, approvals = %s,"
-              + " requiredApproval = %s",
-          changeNotes.getProjectName(),
-          changeNotes.getChangeId(),
-          patchSet.id().get(),
-          oldApprovals,
-          approvals,
-          requiredApproval);
+      // non-approval was downgraded (e.g. -1 to -2)
       return Optional.empty();
     }
 
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
index 52ca037..8a8c702 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
@@ -20,7 +20,6 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
 import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
@@ -44,8 +43,6 @@
  */
 @Singleton
 class OnCodeOwnerOverride implements OnPostReview {
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
   private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
 
   @Inject
@@ -82,8 +79,7 @@
         continue;
       }
 
-      buildMessageForCodeOwnerOverride(
-              user, changeNotes, patchSet, oldApprovals, approvals, overrideApproval)
+      buildMessageForCodeOwnerOverride(user, patchSet, oldApprovals, approvals, overrideApproval)
           .ifPresent(messages::add);
     }
 
@@ -95,7 +91,6 @@
 
   private Optional<String> buildMessageForCodeOwnerOverride(
       IdentifiedUser user,
-      ChangeNotes changeNotes,
       PatchSet patchSet,
       Map<String, Short> oldApprovals,
       Map<String, Short> approvals,
@@ -137,16 +132,7 @@
               "By voting %s the code-owners submit requirement is still overridden by %s",
               newVote, user.getName()));
     }
-    logger.atSevere().log(
-        "code owner override was neither newly applied, removed, upgraded nor downgraded:"
-            + " project = %s, change = %s, patch set = %d, oldApprvals = %s, approvals = %s,"
-            + " requiredApproval = %s",
-        changeNotes.getProjectName(),
-        changeNotes.getChangeId(),
-        patchSet.id().get(),
-        oldApprovals,
-        approvals,
-        overrideApproval);
+    // non-approval was downgraded (e.g. -1 to -2)
     return Optional.empty();
   }
 
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 74dae62..f230eb9 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -202,7 +202,7 @@
 
           // The validation was executed as dry-run and failures during the validation should not
           // cause an error. Hence we swallow the exception here.
-          logger.atFine().withCause(e).log(
+          logger.atWarning().withCause(e).log(
               "ignoring failure during validation of code owner config files in revision %s"
                   + " (project = %s, branch = %s) because the validation was performed as dry-run",
               receiveEvent.commit.getName(),
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index 709797a..8b56cd5 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -344,9 +344,11 @@
                 "found email %s as code owner in %s",
                 orphanedEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
             String.format(
-                "cannot resolve code owner email %s: email belongs to account %s,"
-                    + " but no account with this ID exists",
-                orphanedEmail, accountId));
+                "cannot resolve account %s for email %s: account does not exists",
+                accountId, orphanedEmail),
+            String.format(
+                "cannot resolve code owner email %s: no active account with this email found",
+                orphanedEmail));
   }
 
   @Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index 79b3924..971f8e9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -1061,4 +1061,28 @@
                     + "* %s\n",
                 user.fullName(), path));
   }
+
+  @Test
+  public void changeMessageNotExtendedIfNonApprovalIsDowngraded() throws Exception {
+    codeOwnerConfigOperations
+        .newCodeOwnerConfig()
+        .project(project)
+        .branch("master")
+        .folderPath("/foo/")
+        .addCodeOwnerEmail(admin.email())
+        .create();
+
+    String path = "foo/bar.baz";
+    String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+    // Apply the Code-Review-1.
+    gApi.changes().id(changeId).current().review(ReviewInput.dislike());
+
+    // Change Code-Review-1 to Code-Review-2
+    gApi.changes().id(changeId).current().review(ReviewInput.reject());
+
+    Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+
+    assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review-2");
+  }
 }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
index 5f17972..1265466 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
@@ -599,4 +599,45 @@
                     + "By voting Owners-Override+1 the code-owners submit requirement is overridden by %s\n",
                 user.fullName()));
   }
+
+  @Test
+  @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+  public void changeMessageNotExtendedIfNonApprovalIsDowngraded() throws Exception {
+    LabelDefinitionInput input = new LabelDefinitionInput();
+    input.values =
+        ImmutableMap.of("+1", "Override", " 0", "No Override", "-1", "Dislike", "-2", "Blocked");
+    gApi.projects().name(project.get()).label("Owners-Override").create(input);
+
+    // Allow to vote on the Owners-Override label.
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(
+            TestProjectUpdate.allowLabel("Owners-Override")
+                .range(-2, 1)
+                .ref("refs/*")
+                .group(REGISTERED_USERS)
+                .build())
+        .update();
+
+    codeOwnerConfigOperations
+        .newCodeOwnerConfig()
+        .project(project)
+        .branch("master")
+        .folderPath("/foo/")
+        .addCodeOwnerEmail(admin.email())
+        .create();
+
+    String path = "foo/bar.baz";
+    String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+    gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", -1));
+
+    // Downgrade the vote from -1 to -2.
+    gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", -2));
+
+    Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+
+    assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Owners-Override-2");
+  }
 }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 0297426..6a4ea64 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -121,6 +121,28 @@
   }
 
   @Test
+  public void resolveCodeOwnerReferenceForAmbiguousEmailIfOtherAccountIsInactive()
+      throws Exception {
+    // Create an external ID for 'user' account that has the same email as the 'admin' account.
+    accountsUpdate
+        .get()
+        .update(
+            "Test update",
+            user.id(),
+            (a, u) ->
+                u.addExternalId(
+                    ExternalId.create(
+                        "foo", "bar", user.id(), admin.email(), /* hashedPassword= */ null)));
+
+    // Deactivate the 'user' account.
+    accountOperations.account(user.id()).forUpdate().inactive().update();
+
+    OptionalResultWithMessages<CodeOwner> result =
+        codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+    assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+  }
+
+  @Test
   public void resolveCodeOwnerReferenceForAmbiguousEmail() throws Exception {
     // Create an external ID for 'user' account that has the same email as the 'admin' account.
     accountsUpdate
@@ -159,10 +181,13 @@
     assertThat(result).isEmpty();
     assertThat(result)
         .hasMessagesThat()
-        .contains(
+        .containsAnyOf(
             String.format(
-                "cannot resolve code owner email %s: email belongs to account %s, but no account with this ID exists",
-                email, accountId));
+                "cannot resolve account %s for email %s: account does not exists",
+                accountId, email),
+            String.format(
+                "cannost resolve code owner email %s: no active account with this email found",
+                email));
   }
 
   @Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index b495bd4..1772809 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -20,6 +20,7 @@
 import static org.mockito.Mockito.when;
 
 import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Change;
 import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
 import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
 import com.google.gerrit.plugins.codeowners.testing.SubmitRecordSubject;
@@ -50,6 +51,34 @@
   }
 
   @Test
+  public void emptyIfChangeIdClosed() throws Exception {
+    codeOwnerConfigOperations
+        .newCodeOwnerConfig()
+        .project(project)
+        .branch("master")
+        .folderPath("/foo/")
+        .addCodeOwnerEmail(user.email())
+        .create();
+
+    String path = "foo/bar.baz";
+    Change change = createChange("Change Adding A File", path, "file content").getChange().change();
+    String changeId = change.getKey().get();
+
+    // Add a Code-Review+1 from a code owner (by default this counts as code owner approval).
+    requestScopeOperations.setApiUser(user.id());
+    recommend(changeId);
+
+    // Approve and submit.
+    requestScopeOperations.setApiUser(admin.id());
+    approve(changeId);
+    gApi.changes().id(changeId).current().submit();
+
+    // Run the code owners submit rule on the closed change.
+    ChangeData changeData = changeDataFactory.create(project, change.getId());
+    assertThat(codeOwnerSubmitRule.evaluate(changeData)).isEmpty();
+  }
+
+  @Test
   public void notReady() throws Exception {
     ChangeData changeData = createChange().getChange();
     SubmitRecordSubject submitRecordSubject =
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 2f7c30e..4aa0a26 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -88,7 +88,7 @@
         `@PLUGIN@.config`.\
         By default unset (no override info URL).
 
-<a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplictApprovals</a>
+<a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplicitApprovals</a>
 :       Whether an implicit code owner approval from the last uploader is
         assumed.\
         \
@@ -119,7 +119,7 @@
         If implicit code owner approvals are disabled, code owners can still
         self-approve their own changes by voting on the change.\
         Can be overridden per project by setting
-        [codeOwners.enableImplictApprovals](#codeOwnersEnableImplicitApprovals)
+        [codeOwners.enableImplicitApprovals](#codeOwnersEnableImplicitApprovals)
         in `@PLUGIN@.config`.\
         By default `FALSE`.
 
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md
index a718d8e..7f06bb1 100644
--- a/resources/Documentation/user-guide.md
+++ b/resources/Documentation/user-guide.md
@@ -65,7 +65,7 @@
 of the `@PLUGIN@` plugin).
 
 It's possible to configure that for changes/patch-sets that are uploaded by a
-code owner an implict code owner approval from the uploader is assumed. In this
+code owner an implicit code owner approval from the uploader is assumed. In this
 case, if a code owner only touches files that they own, no approval from other
 code owners is required. If this is configured, it is important that code owners
 are aware of their implicit approval when they upload new patch sets for other
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index 3b0dc95..69929f4 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -102,7 +102,7 @@
       setting)
     * it is a non-visible secondary email
     * there is no account that has this email assigned
-    * it is ambiguous (the same email is assigned to multiple accounts)
+    * it is ambiguous (the same email is assigned to multiple active accounts)
     * it has an email domain that is disallowed (see
       [allowedEmailDomain](config.html#pluginCodeOwnersAllowedEmailDomain))
       configuration
diff --git a/ui/suggest-owners.js b/ui/suggest-owners.js
index bcfb2ef..2395476 100644
--- a/ui/suggest-owners.js
+++ b/ui/suggest-owners.js
@@ -135,6 +135,9 @@
         }
         .suggestion-row-indicator[visible] iron-icon {
           color: var(--link-color);
+          vertical-align: top;
+          position: relative;
+          top: 3px; /* (26-20)/2 - 26px line-height and 20px icon */ 
         }
         .suggestion-group-name {
           width: 200px;
@@ -178,13 +181,16 @@
           height: 16px;
         }
         gr-account-label {
-          background-color: var(--background-color-tertiary);
           display: inline-block;
           padding: var(--spacing-xs) var(--spacing-m);
           user-select: none;
           border: 1px solid transparent;
           --label-border-radius: 8px;
-          --account-max-length: 100px;
+          /* account-max-length defines the max text width inside account-label.
+           With 60px the gr-account-label always has width <= 100px and 5 labels
+           are always fit in a single row */
+          --account-max-length: 60px;
+          border: 1px solid var(--border-color);
         }
         gr-account-label:focus {
           outline: none;