1288109 - flipping the r+ combo box on the "finish review" dialog shows the "Loading..." indicator

Status: RESOLVED WORKSFORME

(MozReview Graveyard :: General, defect)

Description

[Nathan Froyd [:froydnj]]

I just finished reviews in https://reviewboard.mozilla.org/r/65572/ and every time I hit "Finish Review..." and flipped the r+ combo box, I was given the "Loading..." indicator at the top for multiple seconds.  I'm unsure what it is doing, but flipping a combo box should not require deep thought on MozReview's part.

Comment 1

[David Walsh :davidwalsh]

The "Loading" indicator shows because the R/+/-/? value is being saved.

Comment 2

[:glob ✱]

i suspect this is a reviewboard bug which appears to have already been fixed on master when migrating to es6..

> RB.setActivityIndicator = function(status, options) {
>     var $activityIndicator = $("#activity-indicator"),
>         $indicatorText = $activityIndicator.children('.indicator-text');
> 
>     if (status) {
>         if (RB.ajaxOptions.enableIndicator && !options.noActivityIndicator) {
>             $indicatorText
>                 .text((options.type || options.type === "GET")
>                       ? gettext("Loading...") : gettext("Saving..."));
> ...

when saving options.type is 'PUT', however that incorrectly results in 'Loading...'

https://github.com/reviewboard/reviewboard/blob/1ddc83098007246c1790d7a1cd9b2c4cdb4713c0/reviewboard/static/rb/js/utils/apiUtils.es6.js#L26

>                 .text((options.type && options.type === 'GET')
>                       ? gettext("Loading...") : gettext("Saving..."));


david - can you verify this is the correct fix?

Comment 3

[David Walsh :davidwalsh]

Yeah, that looks correct.

Comment 4

[:glob ✱]

Attachment: Backport fix for 'Loading...' indicator being displayed instead of 'Saving...' (bug 1288109)

Upstream Review Board silently fixed this when rewriting apiUtils for ES6.
https://github.com/reviewboard/reviewboard/blob/1ddc830980/reviewboard/static/rb/js/utils/apiUtils.es6.js#L26

Review commit: https://reviewboard.mozilla.org/r/80332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80332/

Comment 5

[David Walsh :davidwalsh]

Comment on attachment 8793618 [details]
Backport fix for 'Loading...' indicator being displayed instead of 'Saving...' (bug 1288109)

https://reviewboard.mozilla.org/r/80332/#review79746

Comment 6

[David Walsh :davidwalsh]

Comment on attachment 8793618 [details]
Backport fix for 'Loading...' indicator being displayed instead of 'Saving...' (bug 1288109)

https://reviewboard.mozilla.org/r/80332/#review79750

Ooops, R+'d this too soon.  We should copy to a `_mozreview.js` file, yes?

Comment 7

[:glob ✱]

Comment on attachment 8793618 [details]
Backport fix for 'Loading...' indicator being displayed instead of 'Saving...' (bug 1288109)

https://reviewboard.mozilla.org/r/80332/#review79750

i don't think so - we're backporting an upstream fix, not making a mozreview specific modification.

Comment 8

[:glob ✱]

this was fixed as part of another bug.