Pulse is not notified of changes to attachment flags

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kgrandon, Assigned: dkl)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Assignee: nobody → dkl
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 1

5 years ago
Thanks for picking this up David. Any chance for a rough estimate of when this could be looked at?
(Assignee)

Comment 2

5 years ago
Posted patch 1091149_1.patch (obsolete) — Splinter Review
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
Attachment #8515042 - Flags: review?(glob)
Comment on attachment 8515042 [details] [diff] [review]
1091149_1.patch

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

oops; wrong patch
Attachment #8515042 - Flags: review?(glob) → review-
(Assignee)

Comment 4

5 years ago
Posted patch 1067619_1.patch (obsolete) — Splinter Review
Well darn. How about this one? :)
Attachment #8515042 - Attachment is obsolete: true
Attachment #8515989 - Flags: review?(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?
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.
Flags: needinfo?(dkl)
(Assignee)

Comment 7

5 years ago
(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
Flags: needinfo?(dkl)
(Assignee)

Comment 8

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

dkl
Attachment #8515989 - Attachment is obsolete: true
Attachment #8515989 - Flags: review?(glob)
Attachment #8518319 - Flags: review?(glob)
Comment on attachment 8518319 [details] [diff] [review]
1067619_2.patch

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

r=glob
Attachment #8518319 - Flags: review?(glob) → review+
(Assignee)

Comment 10

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