please make seeing first blocking revision in queue easier

[Jamie Strandboge]

Currently the store shows the last 10 revisions. This becomes a problem when there was a failed automatic review that happened more than 10 revisions ago because you only see queued for review revisions and you have to start guessing which was the failed review. This happens frequently with LP builds that have 6 architectures associated (ie, 6 different revisions) with a single commit to a tree with auto uploads to edge.

The store should show the revision that is blocking the review queue somewhere.

[Para Siva (psivaa)]

Hi Jamie,

Thanks for the report.

The problem is understandable.
In order to implement a solution, its not clear to me whether there will be a case where there are more than 10 failed revisions, (say 20, revs 1-20) all failing automatic review and you'd only want to manually approve rev 5 and onward; i.e. the first 4 revs are ignored because of some reasons.

If this is a possible case, all the failed revisions are candidates to be listed as blocking revisions and there fore I'm not sure if showing the first failed revision will be helpful there. Unless we're ok with showing the first failed one because we then know the earliest failed one and therefore we could select the wanted rev without the guess work.

Would appreciate your thought on that.

Thanks

[Jamie Strandboge (jdstrand)]

My understanding is that we have a queue of 'not-yet-reviewed' revisions that are blocked because the last reviewed revision failed. As such, "failed revisions" are not in this queue and therefore should not be considered. Going back in time to a previously rejected upload should not trigger a re-run of everything that has already been reviewed or those 'not-yet-reviewed' revisions. *Only* the last reviewed failing revision that is blocking other revisions from getting reviewed should be considered. As such, increasing the number of displayed revisions to 20 would be good (since that covers 3 sets of architectures now and should always cover at least 2 sets of architectures in the future) and then showing the last failed revision that is blocking the queue if it is beyond the 20. Consider:

r30 - queued
r29 - queued
r28 - queued
r27 - queued
r26 - queued
r25 - queued
r24 - queued
r23 - queued
r22 - queued
r21 - queued
r20 - queued
r19 - queued
r18 - queued
r17 - queued
r16 - queued
r15 - queued
r14 - queued
r13 - queued
r12 - queued
r11 - queued
r10 - queued
r9 - queued
r8 - queued
r7 - queued
r6 - queued
r5 - failed review
r4 - approved
r3 - approved
r2 - rejected
r1 - rejected

With the above, I would expect r11 - r30 to appear in the list (last 20 revisions) and then a separate single line for r5 (last failed review that is blocking the whole queue). r1 - r4 and r6 - r10 are not displayed (beyond last 20 revisions). Adjusting something and rerunning tests against r2 such that r2 passes does *not* re-review r5 or cause r6 to be auto-reviewed. Adjusting something and rerunning tests against r5 such that r5 passes *does* allow r6 to undergo auto-review.

I envision the UI to be something like:
r30 - queued
r29 - queued
r28 - queued
r27 - queued
r26 - queued
r25 - queued
r24 - queued
r23 - queued
r22 - queued
r21 - queued
r20 - queued
r19 - queued
r18 - queued
r17 - queued
r16 - queued
r15 - queued
r14 - queued
r13 - queued
r12 - queued
r11 - queued
...
!r5 - failed review

where the '...' is an actual ellipsis in the ui.

Let's assume we change something and rerun the tools such that r5 passes and then that change allows r6 - r30 to pass. We then should see in the UI:
r30 - approved
r29 - approved
r28 - approved
r27 - approved
r26 - approved
r25 - approved
r24 - approved
r23 - approved
r22 - approved
r21 - approved
r20 - approved
r19 - approved
r18 - approved
r17 - approved
r16 - approved
r15 - approved
r14 - approved
r13 - approved
r12 - approved
r11 - approved

Now let's assume r31 is uploaded, passes auto-review, r32 and r33 are uploaded, but r32 failed review. The UI would be:

r33 - queued
!r32 - failed review
r31 - approved
r30 - approved
r29 - approved
r28 - approved
r27 - approved
r26 - approved
r25 - approved
r24 - approved
r23 - approved
r22 - approved
r21 - approved
r20 - approved
r19 - approved
r18 - approved
r17 - approved
r16 - approved
r15 - approved
r14 - approved

Here, we only show the last 20 revisions because the last failed revisions falls within the 20. Ie, here, don't show separate '...\nrNN - failed review'-- only show it if the last failed review that is blocking the queue is beyond the last 20 revisions.

[Para Siva (psivaa)]

Thank you for the reply and many apologies in the delay in responding.

First of all, I see the new uploads being queued for automatic review only when the last upload is still waiting manual review. I assume this is the case in the LP builds scenario. i.e. The manual review is automatically requested on an automatic review failure. If the request to manual review is not made, the automatic runs keep going and the uploads get either approved/rejected based on that. Just want to make sure if this is the case.

Secondly, again just to confirm, the review queue that is referred in the bug is NOT what's attached here, right. (This is the view when you click 'Review Packges' in the drop down manu on the top right corner, but that can list more than 20 entries)

[Para Siva (psivaa)]

This is a view we get once we click one of the entries shown in the previous 'review_queue_for_reviewers' image. Here the blocking version is within the last 20 revisions. (Here I have only listed it as a 'Blocking revision'. This is just for illustration, the UI can be tweaked later for the best preference)

[Para Siva (psivaa)]

This is again a view we get once we click one of the entries shown in the 'review_queue_for_reviewers' image. Here the blocking version is outside the last 20 revisions. (Again the UI is just for illustration)

[Para Siva (psivaa)]

Would really appreciate if you could please take a look at those images and confirm if that type of the fix you were expecting.

Also, I confirmed, that only approving the latest blocking revision lets the automatic runs to continue and based on their individual results they either get approved or rejected. So I think that's working as expected.

I see that you've suggested to not to show the blocking version separately if that's falling within the last 20, that can be done if that's important. But for me it looks simpler for reviewers.

Thanks.

[Jamie Strandboge (jdstrand)]

"First of all, I see the new uploads being queued for automatic review only when the last upload is still waiting manual review. I assume this is the case in the LP builds scenario. i.e. The manual review is automatically requested on an automatic review failure. If the request to manual review is not made, the automatic runs keep going and the uploads get either approved/rejected based on that. Just want to make sure if this is the case."

I'm not sure how it is coded, but I think I would rephrase. On LP upload for arch A, if there is nothing in the queue, this upload is auto-reviewed. If it passes auto-review, it goes on to auto-review arch B. If it did not pass auto-review, arch B is not auto-reviewed, it is instead queued and arch A is either set for manual review or set as failed with an option to request manual review (this is what I'm not sure of). When arch A fails auto-review, arch B is always queued and does not get auto-reviewed until a human approves or rejects arch A.

"This is a view we get once we click one of the entries shown in the previous 'review_queue_for_reviewers' image. Here the blocking version is within the last 20 revisions. (Here I have only listed it as a 'Blocking revision'. This is just for illustration, the UI can be tweaked later for the best preference)"

Correct, review_queue_for_reviewers.png is NOT what I'm referring to. I'm referring to the area in the middle left of https://myapps.developer.ubuntu.com/dev/click-apps/5779/ under 'Upload summary' (but without clicking 'Upload summary').

"This is a view we get once we click one of the entries shown in the previous 'review_queue_for_reviewers' image. Here the blocking version is within the last 20 revisions. (Here I have only listed it as a 'Blocking revision'. This is just for illustration, the UI can be tweaked later for the best preference)"

Right, blocking_in_the_list.png shows the area I am talking about. In this case, r10 is within the last 20, so easy to find and no problem as a reviewer.

"This is again a view we get once we click one of the entries shown in the 'review_queue_for_reviewers' image. Here the blocking version is outside the last 20 revisions. (Again the UI is just for illustration)"

If blocking_outside_the_list.png was the UI for the last blocking revision that is outside the last 20 revisions, that would be perfect for a reviewer.

"Also, I confirmed, that only approving the latest blocking revision lets the automatic runs to continue and based on their individual results they either get approved or rejected. So I think that's working as expected."

That's right-- if you approve/reject the latest blocking revision, the next revision after is manually reviewed and if it passes auto-review, the next is and so on until one fails auto-review, then it becomes the new latest blocking revision.

[Jamie Strandboge (jdstrand)]

Meh, this:

"That's right-- if you approve/reject the latest blocking revision, the next revision after is manually reviewed and if it passes auto-review, the next is and so on until one fails auto-review, then it becomes the new latest blocking revision."

Should've said:

"That's right-- if you approve/reject the latest blocking revision, the next revision after is auto-reviewed and if it passes auto-review, the next isauto-reviewed and so on until one fails auto-review, then it becomes the new latest blocking revision."

[Para Siva (psivaa)]

The fix for this has landed in trunk and in production now.
Please let us know if there is anything that did not meet the expectations of this bug.
Thanks