1067808 - Review history page displays cancelled reviews as overdue

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect, P1)

Description

[Ed Morley [:emorley]]

eg on:
https://bugzilla.mozilla.org/page.cgi?id=review_history.html&requestee=emorley%40mozilla.com

For bug 980573, there is a "review?" marked as having a 6 month duration, when in fact the review was cancelled after 6 minutes.

Comment 1

[Ed Morley [:emorley]]

Guessing we need to add a |case 'X':| alongside the + and -

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1067808-v1.patch

This patch causes the UI to ignore canceled review requests.

Comment 3

[Ed Morley [:emorley]]

Comment on attachment 8490418 [details] [diff] [review]
bug-1067808-v1.patch

Canceling a review is often used as a more tactful form of review minus. As such, I think it's useful if we measure how long until the review was canceled, as long as the person canceling was the requestee.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Alright. I think we're going to have to fix the cancelation time not being recorded first.
It is possible that any canceled reviews before bug 1067410 is resolved will have to display something like "after some time."

This historical data only goes back 6 months as it stands, and it may be possible to correlate when reviews were canceled (although I forsee some difficulties in getting this data)

Comment 6

[Mark Côté [:mcote]]

Yeah I would fix it first, so that future cancelled reviews show up correctly, and then worry about correcting old data--and I don't think it's a huge loss if we can't, or if it ends up being extremely difficult.

We need tests for this stuff. :)

Comment 7

[Mark Côté [:mcote]]

Yeah I would fix it first, so that future cancelled reviews show up correctly, and then worry about correcting old data--and I don't think it's a huge loss if we can't, or if it ends up being extremely difficult.

We need tests for this stuff. :)

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1067808-v2.patch

A bit of a refactor here, needed to handle the bad data in the database.

Two types of bad data:

1) flags that are cleared but the time is wrong (same as the last change)
2) flags that are cleared but the time is wrong (wrong timezone, because of a bug in Bugzilla::Flag->update).

Additionally, flags being reassigned is also handled.

The code is significantly easier to understand.

Comment 9

[:glob ✱]

Comment on attachment 8492543 [details] [diff] [review]
bug-1067808-v2.patch

Review of attachment 8492543 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with comments addressed on commit

::: extensions/Review/web/js/review_history.js
@@ +282,5 @@
> +                                    stash[flag_id] = flag;
> +                                }
> +                                // flag changed hands. Reset the creation_time
> +                                else {
> +                                    stash[flag_id] = JSON.parse(JSON.stringify(stash[flag_id]));

that's an unusual line :)  i guess we need to copy the flag in some circumstances (in my testing i couldn't find any issues with this line commented out).  can you add a comment which indicates why we do this.

you should be using yui's json functions here (to match the rest of the code).

@@ +296,5 @@
> +                    case 'X':
> +                        if (stash[flag_id]) {
> +                            // Only process if we did not get a + or a - since
> +                            if (!stash[flag_id].is_complete) {
> +                                add_historical_action(history, flag, stash[flag_id], "cleared");

bugzilla uses "cancelled" everywhere else to indicate this action; please change "cleared" to "cancelled".

Comment 10

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› from comment #9)
> Comment on attachment 8492543 [details] [diff] [review]
> bug-1067808-v2.patch
> 
> Review of attachment 8492543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=glob, with comments addressed on commit
> 
> ::: extensions/Review/web/js/review_history.js
> @@ +282,5 @@
> > +                                    stash[flag_id] = flag;
> > +                                }
> > +                                // flag changed hands. Reset the creation_time
> > +                                else {
> > +                                    stash[flag_id] = JSON.parse(JSON.stringify(stash[flag_id]));
> 
> that's an unusual line :)  i guess we need to copy the flag in some
> circumstances (in my testing i couldn't find any issues with this line
> commented out).  can you add a comment which indicates why we do this.
Hmm, the situations where that happened didn't end up making it into the patch, so I will remove it like a vestigial tail.

> you should be using yui's json functions here (to match the rest of the
> code).
> 
> @@ +296,5 @@
> > +                    case 'X':
> > +                        if (stash[flag_id]) {
> > +                            // Only process if we did not get a + or a - since
> > +                            if (!stash[flag_id].is_complete) {
> > +                                add_historical_action(history, flag, stash[flag_id], "cleared");
> 
> bugzilla uses "cancelled" everywhere else to indicate this action; please
> change "cleared" to "cancelled".

Ah, right.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   c516e35..caa8040  master -> master