| Issue 2497: | Regression: Max score is displayed as a number | |
| 2 people starred this issue and may be notified of changes. | Back to list |
************************************************************ ***** NOTE: THIS BUG TRACKER IS FOR GERRIT CODE REVIEW ***** ***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, INTERNAL ***** ***** ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC. ***** ***** THOSE ISSUE BELONG IN DIFFERENT ISSUE TRACKERS! ***** ************************************************************ Affected Version: 2.8.2 Changes with maximum or minimum score appear with +1 or +2 instead of the V or X sign in the following cases: * When the label is of type NoBlock * When the change is merged See the attached screenshot. The right column should appear as V instead of +1. Notice the Code-Review label appears as V in the Incoming Reviews panel, and as +2 in the Recently Closed panel. I suspect 5a32a6dc15b73d412645598648ac089eacf29b65, though I didn't try reverting it...
Feb 25, 2014
#1
org...@gmail.com
Feb 25, 2014
The file was not attached for some reason...
Feb 25, 2014
> Changes with maximum or minimum score appear with +1 or +2 instead of the V or X sign in the following cases: > > * When the label is of type NoBlock This is not a regression but a fix. The situation before this change was a bug. See documentation for labels with functions NoOp/NoBlock: * `NoBlock`/`NoOp` The label is purely informational and values are not considered when determining whether a change is submittable. So it was a bug to put X / V on these labels. > * When the change is merged Which function? MaxWithBlock? Which screen? Can you provide exact steps how to reproduce it?
Status:
AwaitingInformation
Feb 25, 2014
The default Code-Review label (which is MaxWithBlock), and another project-custom label which is also MaxWithBlock. This is the main dashboard. I'm using the Old change screen.
Feb 25, 2014
Does this only happen in the old change screen?
Feb 25, 2014
The dashboard looks just the same when I change the settings, so no. It happens always.
Feb 25, 2014
> So it was a bug to put X / V on these labels I disagree. We have a Jenkins job that builds the project and runs unit tests, then sets the Verified label. It *is* informational only, we don't block submit if anything fails, but still - V/X is much clearer than +1/-1.
Feb 26, 2014
> It *is* informational only, we don't block submit if anything fails, but still - V/X is much clearer than +1/-1. Yes you do. That was exactly the bug [1], that this change fixed: so in your example, if your Verified label is using NoBlock function and your Jenkins vote with -1 then it was broken (on new change screen only): you cannot submit your changes any more, because the label was not set to disliked, but to rejected. Note, that old change screen doesn't suffers from this problem, at least not in 2.8.x. As you are using old change screen, that was probably the reason why you haven't noticed it. https://code.google.com/p/gerrit/issues/detail?id=2453 So either change the function of your Verified Label to MaxWithBlock, or file an enhancement request to differentiate between labels that are used for computation of submit status and presented on dash board. Currently they are the same: if you put reject it means you cannot submit.
Feb 26, 2014
Can't issue 2453 be fixed in any other way? How does it work with in old change screen?
Feb 26, 2014
So the culprit of this problem is, that with move to change screen 2, the submiteability of a change is computed on the client side and the LabelInfo.rejected attribute carries out two kind of informations:
1. submiteability:
public final SubmitRecord.Label.Status status() {
if (approved() != null) {
return SubmitRecord.Label.Status.OK;
} else if (rejected() != null) {
return SubmitRecord.Label.Status.REJECT;
} else if (optional()) {
return SubmitRecord.Label.Status.MAY;
} else {
return SubmitRecord.Label.Status.NEED;
}
}
2. Style of the rendered label:
private String getStyleForLabel(LabelInfo label) {
switch (label.status()) {
case OK:
return style.label_ok();
case NEED:
return style.label_need();
case REJECT:
case IMPOSSIBLE:
return style.label_reject();
default:
case MAY:
return style.label_may();
}
}
So setting rejected attribute as rejected for non blocking labels - NoOp/NoBlock - makes a change erroneously non submitteable, whereas not setting the rejected attribute prevents that label from being rendered correctly.
The way to go would be to decouple the submitteability computation from rendereing style as it wa the case with old change screen:
Either we should move the computation of the submitteability status from client side to server side, taking into consideration only SubmitRecord votes, or we should duplicate the rejected attribute in response to be able to differentiate between them.
The same holds true for approved attribute.
Status:
Accepted
Mar 5, 2014
(No comment was entered for this change.)
Status:
Submitted
Labels: FixedIn-2.8.2
Mar 11, 2014
(No comment was entered for this change.)
Status:
Released
|
|
| ► Sign in to add a comment |