From d1e4915a4bdd3ff69be0204bbfb8186c6bcc8d75 Mon Sep 17 00:00:00 2001
From: Tomasz Zarna
Date: Tue, 30 Jul 2013 14:42:54 +0200
Subject: 395059: abandoned change doesn't provide info about approval types

Bug: 395059
Change-Id: I42e04e4574025a5faec7a2ee4b7d442977377aac
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=395059
Signed-off-by: Tomasz Zarna <tomasz.zarna@tasktop.com>
---
 .../gerrit/core/client/rest/ApprovalUtil.java      | 13 ++++------
 .../core/remote/GerritReviewRemoteFactory.java     | 29 +++++++++++++---------
 .../core/remote/GerritReviewRemoteFactoryTest.java | 11 +++-----
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/rest/ApprovalUtil.java b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/rest/ApprovalUtil.java
index f82d576c0..2c92b5e46 100644
--- a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/rest/ApprovalUtil.java
+++ b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/rest/ApprovalUtil.java
@@ -84,8 +84,7 @@ public final class ApprovalUtil {
 		BY_ID.put(IPCL.getCategory().getId().get(), IPCL);
 	}
 
-	static ApprovalCategory findCategoryByName(String name) {
-		name = name.replace('-', ' ');
+	private static ApprovalCategory findCategoryByName(String name) {
 		if (BY_NAME.containsKey(name)) {
 			return BY_NAME.get(name).getCategory();
 		}
@@ -93,8 +92,7 @@ public final class ApprovalUtil {
 	}
 
 	static ApprovalCategory findCategoryByNameWithDash(String name) {
-		name = name.replace('-', ' ');
-		return findCategoryByName(name);
+		return findCategoryByName(name.replace('-', ' '));
 	}
 
 	static ApprovalCategory.Id findCategoryIdByName(String name) {
@@ -106,11 +104,10 @@ public final class ApprovalUtil {
 	}
 
 	static ApprovalCategory.Id findCategoryIdByNameWithDash(String name) {
-		name = name.replace('-', ' ');
-		return findCategoryIdByName(name);
+		return findCategoryIdByName(name.replace('-', ' '));
 	}
 
-	public static String findCategoryNameById(String id) {
+	static String findCategoryNameById(String id) {
 		if (BY_ID.containsKey(id)) {
 			return BY_ID.get(id).getCategory().getName();
 		}
@@ -121,7 +118,7 @@ public final class ApprovalUtil {
 		return name.replace(' ', '-');
 	}
 
-	public static short parseShort(String s) {
+	static short parseShort(String s) {
 		s = s.trim();
 		// only Java7 handles a plus sign as indication of a positive value
 		if (s.startsWith("+")) { //$NON-NLS-1$
diff --git a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactory.java b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactory.java
index 7952bb517..e591f9c07 100644
--- a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactory.java
+++ b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactory.java
@@ -55,6 +55,7 @@ import com.google.gerrit.common.data.ChangeInfo;
 import com.google.gerrit.common.data.PatchSetDetail;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.Change.Status;
 import com.google.gerrit.reviewdb.ChangeMessage;
 import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.UserIdentity;
@@ -134,7 +135,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 		} catch (GerritException e) {
 			throw GerritCorePlugin.getDefault()
 					.getConnector()
-					.toCoreException(parent.getTaskRepository(), "Problem while retrieving Gerrit review.", e);
+					.toCoreException(parent.getTaskRepository(), "Problem while retrieving Gerrit review.", e); //$NON-NLS-1$
 		}
 	}
 
@@ -248,7 +249,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 				//We force a pull here, which is safe because there isn't any actual client API invocation
 				consumer.pull(true, new NullProgressMonitor());
 			} catch (CoreException e) {
-				throw new RuntimeException("Internal Exception. Unexpected state.", e);
+				throw new RuntimeException("Internal Exception. Unexpected state.", e); //$NON-NLS-1$
 			}
 			if (patchIndex++ < oldPatchCount) {
 				consumer.release();
@@ -334,7 +335,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 				IUser reviewer = getGerritProvider().createUser(parent, detail.getAccounts(),
 						remoteApproval.getAccount());
 				if (reviewer == null) {
-					throw new RuntimeException("Internal Error, no reviewer found for: " + remoteApproval.getAccount());
+					throw new RuntimeException("Internal Error, no reviewer found for: " + remoteApproval.getAccount()); //$NON-NLS-1$
 				}
 				IReviewerEntry reviewerEntry = review.getReviewerApprovals().get(reviewer);
 				if (reviewerEntry == null) {
@@ -366,18 +367,22 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 				for (Label label : record.getLabels()) {
 					IApprovalType approvalType = typeForName.get(label.getLabel());
 					if (approvalType == null) {
-						throw new RuntimeException("Internal Error, no approval type found for: " + label.getLabel());
+						if (detail.getChange().getStatus() == Status.ABANDONED) {
+							// typeForName can be empty for an abandoned change as it no longer provides approval types info
+							continue;
+						}
+						throw new RuntimeException("Internal Error, no approval type found for: " + label.getLabel()); //$NON-NLS-1$
 					}
 					IRequirementEntry requirementEntry = IReviewsFactory.INSTANCE.createRequirementEntry();
-					if (label.getStatus().equals("OK")) {
+					if (label.getStatus().equals("OK")) { //$NON-NLS-1$
 						requirementEntry.setStatus(RequirementStatus.SATISFIED);
-					} else if (label.getStatus().equals("NEED")) {
+					} else if (label.getStatus().equals("NEED")) { //$NON-NLS-1$
 						requirementEntry.setStatus(RequirementStatus.NOT_SATISFIED);
-					} else if (label.getStatus().equals("REJECT")) {
+					} else if (label.getStatus().equals("REJECT")) { //$NON-NLS-1$
 						requirementEntry.setStatus(RequirementStatus.REJECTED);
-					} else if (label.getStatus().equals("MAY")) {
+					} else if (label.getStatus().equals("MAY")) { //$NON-NLS-1$
 						requirementEntry.setStatus(RequirementStatus.OPTIONAL);
-					} else if (label.getStatus().equals("IMPOSSIBLE")) {
+					} else if (label.getStatus().equals("IMPOSSIBLE")) { //$NON-NLS-1$
 						requirementEntry.setStatus(RequirementStatus.ERROR);
 					}
 					if (label.getAppliedBy() != null) {
@@ -406,7 +411,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 		case ABANDONED:
 			return ReviewStatus.ABANDONED;
 		default:
-			GerritCorePlugin.logError("Internal Error: unexpected change status: " + gerritStatus, new Exception());
+			GerritCorePlugin.logError("Internal Error: unexpected change status: " + gerritStatus, new Exception()); //$NON-NLS-1$
 			return null;
 		}
 	}
@@ -422,7 +427,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 		for (ChangeInfo remoteChange : remoteChanges) {
 			final IChange localChange = IReviewsFactory.INSTANCE.createChange();
 			localChange.setKey(remoteChange.getKey().get());
-			localChange.setId(remoteChange.getId().get() + "");
+			localChange.setId(Integer.toString(remoteChange.getId().get()));
 			AccountInfo remoteOwner = detail.getAccounts().get(remoteChange.getOwner());
 			IUser owner = getGerritProvider().createUser(group, detail.getAccounts(), remoteOwner.getId());
 			localChange.setOwner(owner);
@@ -435,7 +440,7 @@ public class GerritReviewRemoteFactory extends ReviewRemoteFactory<GerritChange,
 
 	@Override
 	public String getRemoteKey(GerritChange remoteObject) {
-		return remoteObject.getChangeDetail().getChange().getId().get() + "";
+		return Integer.toString(remoteObject.getChangeDetail().getChange().getId().get());
 	}
 
 	@Override
diff --git a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
index 84423b8c7..59f52a9da 100644
--- a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
+++ b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
@@ -385,14 +385,9 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 		assertThat(crvw.getPatchSetId().get(), is(1));
 		assertThat(crvw.getPatchSetId().getParentKey().get(), is(Integer.parseInt(getReview().getId())));
 
-		// TODO: empty map vs map with null key
-		if (GerritVersion.isVersion26OrLater(getCurrentVersion())) {
-			assertThat(getReview().getReviewerApprovals().isEmpty(), is(true));
-		} else {
-			assertThat(getReview().getReviewerApprovals().isEmpty(), is(false));
-			assertThat(getReview().getReviewerApprovals().size(), is(1));
-			assertThat(getReview().getReviewerApprovals().get(0), nullValue());
-		}
+		assertThat(getReview().getReviewerApprovals().isEmpty(), is(false));
+		assertThat(getReview().getReviewerApprovals().size(), is(1));
+		assertThat(getReview().getReviewerApprovals().get(0), nullValue());
 	}
 
 	private static Version getCurrentVersion() {
-- 
cgit v1.2.3