Widen set of My Drafts menus that are automatically removed

PolyGerrit strips # from the menu URL when saving preferences, so some
users may have that variant. While we're in there, allow the trivial
"/q/is:draft".

Bug: Issue 8438
Change-Id: Ia4e9e68991cacdc9166bac8da1bf27748d9ad242

2 files changed, 56 insertions, 29 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_160.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_160.java
index 47523e6231..b78e333d1f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_160.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_160.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.server.git.UserConfigSections.KEY_URL;
 import static com.google.gerrit.server.git.UserConfigSections.MY;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -45,13 +46,30 @@ import org.eclipse.jgit.lib.TextProgressMonitor;
  *
  * <p>Since draft changes no longer exist, these menu items are obsolete.
  *
- * <p>Only matches menu items (with any name) where the URL exactly matches the <a
+ * <p>Only matches menu items (with any name) where the URL exactly matches one of the following,
+ * with or without leading {@code #}:
+ *
+ * <ul>
+ *   <li>/q/is:draft
+ *   <li>/q/owner:self+is:draft
+ * </ul>
+ *
+ * In particular, this includes the <a
  * href="https://gerrit.googlesource.com/gerrit/+/v2.14.4/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java#144">default
- * version from 2.14 and earlier</a>. Other menus containing {@code is:draft} in other positions are
- * not affected; this is still a valid predicate that matches no changes.
+ * from version 2.14 and earlier</a>.
+ *
+ * <p>Other menus containing {@code is:draft} in other positions are not affected; this is still a
+ * valid predicate that matches no changes.
  */
 public class Schema_160 extends SchemaVersion {
-  @VisibleForTesting static final String DEFAULT_DRAFT_ITEM = "#/q/owner:self+is:draft";
+  @VisibleForTesting static final ImmutableList<String> DEFAULT_DRAFT_ITEMS;
+
+  static {
+    String ownerSelfIsDraft = "/q/owner:self+is:draft";
+    String isDraft = "/q/is:draft";
+    DEFAULT_DRAFT_ITEMS =
+        ImmutableList.of(ownerSelfIsDraft, '#' + ownerSelfIsDraft, isDraft, '#' + isDraft);
+  }
 
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
@@ -120,7 +138,8 @@ public class Schema_160 extends SchemaVersion {
     void removeMyDrafts() {
       Config cfg = getConfig();
       for (String item : cfg.getSubsections(MY)) {
-        if (DEFAULT_DRAFT_ITEM.equals(cfg.getString(MY, item, KEY_URL))) {
+        String value = cfg.getString(MY, item, KEY_URL);
+        if (DEFAULT_DRAFT_ITEMS.contains(value)) {
           cfg.unsetSection(MY, item);
           dirty = true;
         }
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/schema/Schema_159_to_160_Test.java b/gerrit-server/src/test/java/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
index c4db3138aa..8bb68b4fa8 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
@@ -22,10 +22,11 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_DEFAULT;
 import static com.google.gerrit.server.account.VersionedAccountPreferences.PREFERENCES;
 import static com.google.gerrit.server.git.UserConfigSections.KEY_URL;
 import static com.google.gerrit.server.git.UserConfigSections.MY;
-import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEM;
+import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEMS;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.extensions.api.GerritApi;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.extensions.client.MenuItem;
@@ -77,8 +78,12 @@ public class Schema_159_to_160_Test {
   @Test
   public void skipUnmodified() throws Exception {
     ObjectId oldMetaId = metaRef(accountId);
-    assertThat(myMenusFromNoteDb(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM);
-    assertThat(myMenusFromApi(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM);
+    ImmutableSet<String> fromNoteDb = myMenusFromNoteDb(accountId);
+    ImmutableSet<String> fromApi = myMenusFromApi(accountId);
+    for (String item : DEFAULT_DRAFT_ITEMS) {
+      assertThat(fromNoteDb).doesNotContain(item);
+      assertThat(fromApi).doesNotContain(item);
+    }
 
     schema160.migrateData(db, new TestUpdateUI());
 
@@ -88,22 +93,25 @@ public class Schema_159_to_160_Test {
   @Test
   public void deleteItems() throws Exception {
     ObjectId oldMetaId = metaRef(accountId);
-    List<String> defaultNames = ImmutableList.copyOf(myMenusFromApi(accountId).keySet());
+    ImmutableSet<String> defaultNames = myMenusFromApi(accountId);
 
     GeneralPreferencesInfo prefs = gApi.accounts().id(accountId.get()).getPreferences();
-    prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEM + "+is:mergeable"));
-    prefs.my.add(new MenuItem("Drafts", DEFAULT_DRAFT_ITEM));
-    prefs.my.add(new MenuItem("Totally not drafts", DEFAULT_DRAFT_ITEM));
+    prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEMS.get(0) + "+is:mergeable"));
+    for (int i = 0; i < DEFAULT_DRAFT_ITEMS.size(); i++) {
+      prefs.my.add(new MenuItem("Draft entry " + i, DEFAULT_DRAFT_ITEMS.get(i)));
+    }
     gApi.accounts().id(accountId.get()).setPreferences(prefs);
 
     List<String> oldNames =
         ImmutableList.<String>builder()
             .add("Something else")
             .addAll(defaultNames)
-            .add("Drafts")
-            .add("Totally not drafts")
+            .add("Draft entry 0")
+            .add("Draft entry 1")
+            .add("Draft entry 2")
+            .add("Draft entry 3")
             .build();
-    assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(oldNames).inOrder();
+    assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(oldNames).inOrder();
 
     schema160.migrateData(db, new TestUpdateUI());
     accountCache.evict(accountId);
@@ -113,8 +121,8 @@ public class Schema_159_to_160_Test {
 
     List<String> newNames =
         ImmutableList.<String>builder().add("Something else").addAll(defaultNames).build();
-    assertThat(myMenusFromNoteDb(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder();
-    assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder();
+    assertThat(myMenusFromNoteDb(accountId)).containsExactlyElementsIn(newNames).inOrder();
+    assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(newNames).inOrder();
   }
 
   @Test
@@ -127,7 +135,7 @@ public class Schema_159_to_160_Test {
   @Test
   public void deleteDefaultItem() throws Exception {
     assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty();
-    List<String> defaultNames = ImmutableList.copyOf(defaultMenusFromApi().keySet());
+    ImmutableSet<String> defaultNames = defaultMenusFromApi();
 
     // Setting *any* preference causes preferences.config to contain the full set of "my" sections.
     // This mimics real-world behavior prior to the 2.15 upgrade; see Issue 8439 for details.
@@ -141,9 +149,9 @@ public class Schema_159_to_160_Test {
 
       // Add more defaults directly in git, the SetPreferences endpoint doesn't respect the "my"
       // field in the input in 2.15 and earlier.
-      cfg.setString("my", "Drafts", "url", DEFAULT_DRAFT_ITEM);
-      cfg.setString("my", "Something else", "url", DEFAULT_DRAFT_ITEM + "+is:mergeable");
-      cfg.setString("my", "Totally not drafts", "url", DEFAULT_DRAFT_ITEM);
+      cfg.setString("my", "Drafts", "url", "#/q/owner:self+is:draft");
+      cfg.setString("my", "Something else", "url", "#/q/owner:self+is:draft+is:mergeable");
+      cfg.setString("my", "Totally not drafts", "url", "#/q/owner:self+is:draft");
       new TestRepository<>(repo)
           .branch(REFS_USERS_DEFAULT)
           .commit()
@@ -158,7 +166,7 @@ public class Schema_159_to_160_Test {
             .add("Something else")
             .add("Totally not drafts")
             .build();
-    assertThat(defaultMenusFromApi().keySet()).containsExactlyElementsIn(oldNames).inOrder();
+    assertThat(defaultMenusFromApi()).containsExactlyElementsIn(oldNames).inOrder();
 
     schema160.migrateData(db, new TestUpdateUI());
 
@@ -169,11 +177,11 @@ public class Schema_159_to_160_Test {
     assertThat(myMenusFromNoteDb(VersionedAccountPreferences::forDefault).keySet())
         .containsExactlyElementsIn(newNames)
         .inOrder();
-    assertThat(defaultMenusFromApi().keySet()).containsExactlyElementsIn(newNames).inOrder();
+    assertThat(defaultMenusFromApi()).containsExactlyElementsIn(newNames).inOrder();
   }
 
-  private ImmutableMap<String, String> myMenusFromNoteDb(Account.Id id) throws Exception {
-    return myMenusFromNoteDb(() -> VersionedAccountPreferences.forUser(id));
+  private ImmutableSet<String> myMenusFromNoteDb(Account.Id id) throws Exception {
+    return myMenusFromNoteDb(() -> VersionedAccountPreferences.forUser(id)).keySet();
   }
 
   // Raw config values, bypassing the defaults set by GeneralPreferencesLoader.
@@ -189,12 +197,12 @@ public class Schema_159_to_160_Test {
     }
   }
 
-  private ImmutableMap<String, String> myMenusFromApi(Account.Id id) throws Exception {
-    return myMenus(gApi.accounts().id(id.get()).getPreferences());
+  private ImmutableSet<String> myMenusFromApi(Account.Id id) throws Exception {
+    return myMenus(gApi.accounts().id(id.get()).getPreferences()).keySet();
   }
 
-  private ImmutableMap<String, String> defaultMenusFromApi() throws Exception {
-    return myMenus(gApi.config().server().getDefaultPreferences());
+  private ImmutableSet<String> defaultMenusFromApi() throws Exception {
+    return myMenus(gApi.config().server().getDefaultPreferences()).keySet();
   }
 
   private static ImmutableMap<String, String> myMenus(GeneralPreferencesInfo prefs) {