Review history page displays cancelled reviews as overdue

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: Review
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: emorley, Assigned: dylan)

Tracking

Production
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Guessing we need to add a |case 'X':| alongside the + and -
(Assignee)

Updated

4 years ago
Assignee: nobody → dylan

Updated

4 years ago
Priority: -- → P1
(Assignee)

Comment 2

4 years ago
Created attachment 8490418 [details] [diff] [review]
bug-1067808-v1.patch

This patch causes the UI to ignore canceled review requests.
Attachment #8490418 - Flags: review?(glob)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1068377
(Assignee)

Updated

4 years ago
Attachment #8490418 - Flags: review?(glob)
(Assignee)

Comment 5

4 years ago
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

Comment 6

4 years ago
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 hidden (typo)
(Assignee)

Comment 8

4 years ago
Created attachment 8492543 [details] [diff] [review]
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.
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+
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   c516e35..caa8040  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1071108
You need to log in before you can comment on or make changes to this bug.