opendev/gerrit
Code Issues Proposed changes

Widen set of My Drafts menus that are automatically removed

Browse Source 
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
This commit is contained in:
Paladox none 2018-02-23 22:00:48 +00:00 committed by Dave Borowitz
parent 7880dc3887
commit 656291c7ef
2 changed files with 56 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_160.java
View File

@ -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;
}

56
gerrit-server/src/test/java/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
View File

@ -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) {