Closed Bug 1067410 Opened 6 years ago Closed 6 years ago

Modification time wrong for deleted flags in review schema

Categories

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

Production
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mcote, Assigned: dylan)

References

Details

Attachments

(2 files, 1 obsolete file)

It seems, for review flags, we are using "creation_time" to mean "modification_time", which is fine, if confusing.  However, when a flag is deleted, the creation_time is not updated.  This is particularly bad since it affects all needinfo flags.  See, for example, https://bugzilla.mozilla.org/rest/review/flag_activity?flag_id=826002.

I noted that a related concern was raised in bug 956229 comment 26, but from what I can tell it was deemed not a problem after all.
s/for review flags/in flag activity/
I do see a lot of dupes where the same flag was created and deleted in the same instant. But there are older and an issue may have been fixed later as I can't seem to reproduce creating a new flag. This is similar for review and needinfo flags so doesn't seem to be a needinfo specific case.
 
id 	flag_when 	type_id 	flag_id 	setter_id 	requestee_id 	bug_id 	attachment_id 	status
107629 	2014-05-27 19:03:33 	4 	885522 	395743 	76551 	1016534 	8429717 	?
108622 	2014-05-27 19:03:33 	4 	885522 	395743 	76551 	1016534 	8429717 	X
Assignee: nobody → dylan
Priority: -- → P1
Attached patch bug-1067410-v1.patch (obsolete) — Splinter Review
(In reply to Mark Côté [:mcote] from comment #0)
> It seems, for review flags, we are using "creation_time" to mean
> "modification_time", which is fine, if confusing.  
Well, it is the 'creation time' of the event in the journal.
More confusingly, internally the name is 'flag_when'. In hindsight, 'activity_timestamp' would have been better.

> However, when a flag is
> deleted, the creation_time is not updated.  This is particularly bad since
> it affects all needinfo flags.  See, for example,
> https://bugzilla.mozilla.org/rest/review/flag_activity?flag_id=826002.
> 
> I noted that a related concern was raised in bug 956229 comment 26, but from
> what I can tell it was deemed not a problem after all.

This is actually a different, but related problem. We hook into object_before_delete, filtering on flag objects. When one is deleted, we log it with a status of 'X'. Because the object is being deleted, its modification time is not updated.

Unfortunately it wasn't as simple as simply passing in the current time -- we need to thread an existing timestamp (if one exists) through the process. For this, I have added an extra bmo-specific hook 'flag_deleted', which handles this. In addition, the _log_flag_state_activity() now takes the timestamp as a parameter in all cases.
Attachment #8490414 - Flags: review?(glob)
How about we purge all flag state activities that end up as 'X' until this patch is reviewed and applied?
I don't see another way of getting the data corrected, as the information was lost.
Flags: needinfo?(mcote)
Status: NEW → ASSIGNED
Blocks: 1067808
If you are sure the information is irrecoverable, then yeah, I guess that's the best option.  If you think it *might* be recoverable, I would suggest hiding them until such time as we recover the timestamps or we give up hope.
Flags: needinfo?(mcote)
Comment on attachment 8490414 [details] [diff] [review]
bug-1067410-v1.patch

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

::: Bugzilla/Flag.pm
@@ +641,5 @@
>              push(@removed, $class->snapshot([$flag])) if $flag->attach_id;
>              $class->notify(undef, $flag, $bug || $flag->bug);
> +
> +            my $timestamp = format_time($dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'),
> +                '%a, %d %b %Y %T %z', 'UTC');

it's wrong to use a UTC timestamp here (you copied that from code which is used to generate email).
$timestamp = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

::: extensions/Review/Extension.pm
@@ -408,4 @@
>  
> -    if (_is_countable_flag($object) && $object->requestee_id && $object->status eq '?') {
> -        _adjust_request_count($object, -1);
> -    }

this _adjust_request_count() call should not be removed -- it's required to update the review counter in the header.
Attachment #8490414 - Flags: review?(glob) → review-
Note that the setter field is also incorrect for deleted flags.
Revised -- added back in the _adjust_* call, correct timestamp (duh, Bugzilla is in Mozilla Standard Time, not UTC).

During my lengthy attempt at fixing the historical data, I discovered that deleted flags didn't have the right setter_id either, this is also fixed in the patch.
Attachment #8490414 - Attachment is obsolete: true
Attachment #8491255 - Flags: review?(glob)
Attached file fix-canceled-flags.pl
This represents my best attempt at reconciling the missing data in flag_state_activity. It is my opinion we should delete the bad data and start over, as nobody was relying on this data just yet anyway.

Nevertheless, I provide this one-off for discussion.
Attachment #8491256 - Attachment mime type: application/x-perl → text/plain
Comment on attachment 8491256 [details]
fix-canceled-flags.pl

as per irc, because we can't regenerate _all_ missing data, we're better off truncating the table once this is fixed.
Attachment #8491256 - Flags: review-
On second thought, I think we should leave them, and just mark the date as '?' for the cancellation times.  No point in throwing all the good data away as long as we indicate that it's not 100% complete.
Comment on attachment 8491255 [details] [diff] [review]
bug-1067410-v2.patch

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

r=glob

::: Bugzilla/Flag.pm
@@ +540,5 @@
>      # These flags have been deleted.
>      foreach my $old_flag (values %old_flags) {
>          $class->notify(undef, $old_flag, $self, $timestamp);
> +
> +        # BMO - provide a hook which passes the flag object

nit: this this hook exists to provide the timestamp, because that isn't passed to remove_from_db()

@@ +641,5 @@
>              push(@removed, $class->snapshot([$flag])) if $flag->attach_id;
>              $class->notify(undef, $flag, $bug || $flag->bug);
> +
> +            my ($timestamp) = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +            # BMO - provide a hook which passes the flag object

ditto
Attachment #8491255 - Flags: review?(glob) → review+
It seems that when a flag setter has customized their time zone, the cancel time is off by that many hours. I spot checked two, below. I think the above patch fixes this -- but I would like to understand why the $object->flag_when field was influenced by the user's timezone in remove_from_db().

bug 970148
    2014-04-22 10:32:18
    2014-04-21 19:32:18

    requestee: 434670
        Has no timezone
    bugs_activity who: 434672
        Has Asia/Taipei timezone

    MariaDB [bmo]> select * from profile_setting where user_id = 434672 and setting_name = 'timezone';
    +---------+--------------+---------------+
    | user_id | setting_name | setting_value |
    +---------+--------------+---------------+
    |  434672 | timezone     | Asia/Taipei   |
    +---------+--------------+---------------+

bug 988731
    2014-03-27T15:37:26
    2014-03-27T00:37:26

    requestee: 390071 (fabrice@mozilla.com)
        Has no timezone
    bugs_activity who: 455315 (jcheng@mozilla.com)
        Has Asia/Taipei timezone
Actually, I wonder if this is always a problem with $flag->modification_date...
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   fb7ed7b..c516e35  master -> master

Regarding the other incorrect dates, filed as bug 1070317
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: Extensions: Review → Extensions
You need to log in before you can comment on or make changes to this bug.