My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 2497: Regression: Max score is displayed as a number
2 people starred this issue and may be notified of changes. Back to list
Status:  Released
Owner:  ----
Closed:  Mar 2014


Sign in to add a comment
 
Reported by org...@gmail.com, Feb 25, 2014
************************************************************
***** 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
Just verified, this is caused by 5a32a6d.
Feb 25, 2014
#2 org...@gmail.com
The file was not attached for some reason...
score.png
9.2 KB   View   Download
Feb 25, 2014
Project Member #3 David.Os...@gmail.com
> 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
#4 org...@gmail.com
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
Project Member #5 david.pu...@sonymobile.com
Does this only happen in the old change screen?
Feb 25, 2014
#6 org...@gmail.com
The dashboard looks just the same when I change the settings, so no. It happens always.
Feb 25, 2014
#7 org...@gmail.com
> 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
Project Member #8 David.Os...@gmail.com
> 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
#9 org...@gmail.com
Can't  issue 2453  be fixed in any other way? How does it work with in old change screen?
Feb 26, 2014
Project Member #10 David.Os...@gmail.com
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
Feb 27, 2014
Project Member #11 David.Os...@gmail.com
https://gerrit-review.googlesource.com/54879
Status: ChangeUnderReview
Mar 5, 2014
Project Member #12 david.pu...@sonymobile.com
(No comment was entered for this change.)
Status: Submitted
Labels: FixedIn-2.8.2
Mar 11, 2014
Project Member #13 david.pu...@sonymobile.com
(No comment was entered for this change.)
Status: Released
Sign in to add a comment

Powered by Google Project Hosting