1067619 - Pulse is not notified of changes to attachment flags

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Bugzilla Change Notification System, defect)

Description

[Kevin Grandon :kgrandon]

Str:
- Toggle a review flag on an attachment, possibly from blank to a '+'. (Do not leave a comment or change any other information)

Expected STR:
- Pulse will be notified.

Actual STR:
- Pulse is not notified.

If I leave a comment with the attachment flag change, then I will receive the notification.

Comment 1

[Kevin Grandon :kgrandon]

Thanks for picking this up David. Any chance for a rough estimate of when this could be looked at?

Comment 2

[David Lawrence [:dkl]]

Attachment: 1091149_1.patch

I actually got mostly finished with that back when I assigned it and then got side-tracked so I apologize.

Here is the completed patch that handles flag and attachment changes that do not have other bug related changes at the same time.

dkl

Comment 3

[:glob ✱]

Comment on attachment 8515042 [details] [diff] [review]
1091149_1.patch

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

oops; wrong patch

Comment 4

[David Lawrence [:dkl]]

Attachment: 1067619_1.patch

Well darn. How about this one? :)

Comment 5

[:glob ✱]

Comment on attachment 8515989 [details] [diff] [review]
1067619_1.patch

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

::: extensions/ZPushNotify/Extension.pm
@@ +63,5 @@
>      return unless scalar keys %{ $args->{changes} };
>      return unless my $object = $args->{object};
>      if ($object->isa('Bugzilla::Attachment')) {
> +        my $timestamp = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +        _notify($object->bug->id, $timestamp);

was there an issue with using $object->modification_time here?

Comment 6

[:glob ✱]

dkl, i'm concerned about the disparity between the attachment's modification time and the time published to pulse as a result of using localtimestamp.  needinfo? re: comment 5.

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #6)
> dkl, i'm concerned about the disparity between the attachment's modification
> time and the time published to pulse as a result of using localtimestamp. 
> needinfo? re: comment 5.

The reason for this change was for the case where only a change to an attachment's meta data has occurred and nothing else such as changing from private to non-private or obsolete to non-obsolete.
In that case only object_end_of_update is fired during the update and object_end_of_update does not have access to the new timestamp like for example Bugzilla::Bug::update would have. Also $object->modification_time in Bugzilla::Attachment::update is not set to the new timestamp until after $self->SUPER::update has been called. 

So my solution, although not perfect, was to use LOCALTIMESTAMP which should in most cases be the same or close to the $timestamp value in Bugzilla::Attachment::update. 

Another alternative would be to add a hook in Bugzilla::Attachment::update such as attachment_end_of_update and pass in the updated attachment object to ZNotify.

For other use cases, if multiple hooks get fired for a single update it is not as big a deal as the last one wins since we are just using REPLACE. As long as we don't end up with a timestamp in push_notify table that is older than the actual delta_ts of the bug.

dkl

Comment 8

[David Lawrence [:dkl]]

Attachment: 1067619_2.patch

New patch that adds new attachment_end_of_update hook so $attachment->modification_time can be used.

dkl

Comment 9

[:glob ✱]

Comment on attachment 8518319 [details] [diff] [review]
1067619_2.patch

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

r=glob

Comment 10

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   613e770..c2533c1  master -> master