Another attempt to fix submit rule evaluation for non blocking labels

New change screen changed the logic for submit status evaluation
by moving it partially to the client side.  The change can be submitted
only when all needed labels have max scores and there is no labels exist
with min score. The computation of rejection is based on `rejected`
attribute in LabelInfo class. This attribute was set in two places:

1. As outcome of submit rule evaluation (with blocking functions)
2. In setLabelScores() to render the result for non blocking labels

After overriding of rejected attribute for non blocking labels it was
not possible any more to differentiate on the client side if one
particular label was rejected for information purpose only or because
it was blocked.

5a32a6dc15b73d412645598648ac089eacf29b65 was trying to solve it by
not overriding the rejected attribute in setLabelScores(). The
consequence of doing it was however, that custom labels with min
score were rendered not as rejected but as disliked.

This change introduces the `blocking` attribute in LabelType class to
carry out the missing information to the client to perform the correct
submit status computation. Now the rejected attribute can be overridden
for custom non blocking labels and rendered correctly.

Bug: Issue 2453
Bug: Issue 2497
Change-Id: Ic5092342b0e2153eb43f5e06fda4fcac4699844e

5 files changed, 31 insertions, 15 deletions

diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index c4ff1751df..edede9877e 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3073,6 +3073,8 @@ link:rest-api-accounts.html#account-info[AccountInfo] entity.
 |`disliked`    |optional|One user who disliked this label on the change
 (voted negatively, but not the minimum value) as an
 link:rest-api-accounts.html#account-info[AccountInfo] entity.
+|`blocking`    |optional|If `true`, the label blocks submit operation.
+If not set, the default is false.
 |`value`       |optional|The voting value of the user who
 recommended/disliked this label on the change if it is not
 "`+1`"/"`-1`".
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CustomLabelIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CustomLabelIT.java
index 2e39f14ea7..f33603fd42 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CustomLabelIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CustomLabelIT.java
@@ -24,6 +24,7 @@ import static com.google.gerrit.server.project.Util.value;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.Maps;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -113,8 +114,8 @@ public class CustomLabelIT extends AbstractDaemonTest {
     ChangeInfo c = get(disliked(change()));
     LabelInfo q = c.labels.get(Q.getName());
     assertEquals(1, q.all.size());
-    assertNull(q.rejected);
-    assertNotNull(q.disliked);
+    assertNotNull(q.rejected);
+    assertNull(q.blocking);
   }
 
   @Test
@@ -124,8 +125,8 @@ public class CustomLabelIT extends AbstractDaemonTest {
     ChangeInfo c = get(disliked(change()));
     LabelInfo q = c.labels.get(Q.getName());
     assertEquals(1, q.all.size());
-    assertNull(q.rejected);
-    assertNotNull(q.disliked);
+    assertNotNull(q.rejected);
+    assertNull(q.blocking);
   }
 
   @Test
@@ -135,8 +136,8 @@ public class CustomLabelIT extends AbstractDaemonTest {
     ChangeInfo c = get(disliked(change()));
     LabelInfo q = c.labels.get(Q.getName());
     assertEquals(1, q.all.size());
-    assertNull(q.rejected);
-    assertNotNull(q.disliked);
+    assertNotNull(q.rejected);
+    assertNull(q.blocking);
   }
 
   @Test
@@ -148,6 +149,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
     assertEquals(1, q.all.size());
     assertNull(q.disliked);
     assertNotNull(q.rejected);
+    assertTrue(q.blocking);
   }
 
   @Test
@@ -158,6 +160,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
     assertEquals(1, q.all.size());
     assertNull(q.disliked);
     assertNotNull(q.rejected);
+    assertTrue(q.blocking);
   }
 
   private String disliked(String changeId) throws IOException {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Labels.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Labels.java
index 099091c45f..e410b0058a 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Labels.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Labels.java
@@ -126,10 +126,12 @@ class Labels extends Grid {
             break;
           case REJECT:
           case IMPOSSIBLE:
-            if (current) {
-              statusText.setInnerText("Not " + name);
+            if (label.blocking()) {
+              if (current) {
+                statusText.setInnerText("Not " + name);
+              }
+              canSubmit = false;
             }
-            canSubmit = false;
             break;
           default:
             break;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
index 6eda547b01..1066c93e34 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
@@ -158,6 +158,7 @@ public class ChangeInfo extends JavaScriptObject {
     public final native String value_text(String n) /*-{ return this.values[n]; }-*/;
 
     public final native boolean optional() /*-{ return this.optional ? true : false; }-*/;
+    public final native boolean blocking() /*-{ return this.blocking ? true : false; }-*/;
     final native short _value()
     /*-{
       if (this.value) return this.value;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 1dcc2ea2be..faa3181513 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -428,6 +428,7 @@ public class ChangeJson {
                 break;
               case REJECT:
                 n.rejected = accountLoader.get(r.appliedBy);
+                n.blocking = true;
                 break;
               default:
                 break;
@@ -454,12 +455,18 @@ public class ChangeJson {
       return;
     }
 
-    if (score < 0) {
-      label.disliked = accountLoader.get(accountId);
-      label.value = score;
-    } else if (score > 0 && label.disliked == null) {
-      label.recommended = accountLoader.get(accountId);
-      label.value = score;
+    if (score != 0) {
+      if (score == type.getMin().getValue()) {
+        label.rejected = accountLoader.get(accountId);
+      } else if (score == type.getMax().getValue()) {
+        label.approved = accountLoader.get(accountId);
+      } else if (score < 0) {
+        label.disliked = accountLoader.get(accountId);
+        label.value = score;
+      } else if (score > 0 && label.disliked == null) {
+        label.recommended = accountLoader.get(accountId);
+        label.value = score;
+      }
     }
   }
 
@@ -989,6 +996,7 @@ public class ChangeJson {
 
     public Short value;
     public Boolean optional;
+    public Boolean blocking;
 
     void addApproval(ApprovalInfo ai) {
       if (all == null) {