Closed Bug 1067808 Opened 6 years ago Closed 6 years ago

Review history page displays cancelled reviews as overdue

Categories

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

Production
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: emorley, Assigned: dylan)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
No longer blocks: 1021902
Depends on: 1021902
Guessing we need to add a |case 'X':| alongside the + and -
Assignee: nobody → dylan
Priority: -- → P1
Attached patch bug-1067808-v1.patch (obsolete) — Splinter Review
This patch causes the UI to ignore canceled review requests.
Attachment #8490418 - Flags: review?(glob)
Status: NEW → ASSIGNED
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.
Duplicate of this bug: 1068377
Attachment #8490418 - Flags: review?(glob)
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)
Depends on: 1067410
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. :)
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.
Attachment #8490418 - Attachment is obsolete: true
Attachment #8492543 - Flags: review?(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".
Attachment #8492543 - Flags: review?(glob) → review+
(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.
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   c516e35..caa8040  master -> master
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1071108
Component: Extensions: Review → Extensions
You need to log in before you can comment on or make changes to this bug.