Closed Bug 1053513 Opened 6 years ago Closed 6 years ago

remove last-visited entries when a user removes involvement from a bug

Categories

(Bugzilla :: Creating/Changing Bugs, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: ehsan, Assigned: dylan)

References

Details

Attachments

(1 file, 2 obsolete files)

It doesn't seem to make a difference how many times I have visited it.  Note that I recently uncc'ed myself from it.
ah.. removing yourself from the CC list should remove the corresponding last-visited entry.

when you visit that bug, we see that you're not involved so we don't touch the last-visited table.
Assignee: nobody → create-and-change
Component: Extensions: MyDashboard → Creating/Changing Bugs
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Summary: Bug 1016473 insists to remain in my list of bugs changed since my last visit → remove last-visited entries when a user removes involvement from a bug
Target Milestone: --- → Bugzilla 5.0
Version: Production → unspecified
Yeah that does explain this.  :-)
dylan - this should be fixed prior to releasing 5.0.  is this something you can work on?
Flags: needinfo?(dylan)
Taken!
Assignee: create-and-change → dylan
Flags: needinfo?(dylan)
Attached patch bug-1053513-v1.patch (obsolete) — Splinter Review
Proposed solution #1:

1) the BugUserLastVisit.update() API calls update_last_visit() if the user is involved in the bug, and delete_last_visit() otherwise.
2) instead of only calling the API when a user is involved in the bug, we call it if the user is involved in the bug or has a last visit entry.
Attachment #8481008 - Flags: review?(glob)
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8481008 [details] [diff] [review]
bug-1053513-v1.patch

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

#   Failed test 'Bugzilla/Bug.pm POD coverage is 99%. Undocumented methods: delete_user_last_visit'
#   at t/011pod.t line 106.

#   Failed test 'Bugzilla/User.pm POD coverage is 99%. Undocumented methods: was_involved_in_bug'
#   at t/011pod.t line 106.

#   Failed test '--WARNING template/en/default/global/user-error.html.tmpl has 1 unused error tag(s):
# user error tag 'user_not_involved' is defined at line(s) (1893) but is not used anywhere'
#   at t/012throwables.t line 206.
Attachment #8481008 - Flags: review?(glob) → review-
if i'm reading this patch right, doesn't it mean the list-visited row will only be updated if the user loads show_bug and they are no longer involved?

i don't think that's the right approach; consider what happens if someone updates a bug via the api.


you may be able to do something like the following in Bugzilla::Bug::update() ..

> if ($user->is_involved_in_bug($self)) {
>     $self->update_user_last_visit($user, $delta_ts);
> }
> elsif ($user->is_involved_in_bug($old_bug)) {
>     $self->delete_user_last_visit($user);
> }

(totally untested)
(In reply to Byron Jones ‹:glob› from comment #6)
> Comment on attachment 8481008 [details] [diff] [review]
> bug-1053513-v1.patch
> 
> Review of attachment 8481008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> #   Failed test 'Bugzilla/Bug.pm POD coverage is 99%. Undocumented methods:
> delete_user_last_visit'
> #   at t/011pod.t line 106.
> 
> #   Failed test 'Bugzilla/User.pm POD coverage is 99%. Undocumented methods:
> was_involved_in_bug'
> #   at t/011pod.t line 106.
This patch was against BMO, which does not perform the pod coverage test, so these tests passed for me.

> #   Failed test '--WARNING template/en/default/global/user-error.html.tmpl
> has 1 unused error tag(s):
> # user error tag 'user_not_involved' is defined at line(s) (1893) but is not
> used anywhere'
> #   at t/012throwables.t line 206.
This is related to bug 1020789. Revisiting that bug, I think the error should be on by default.
Per-IRC discussion: Build a list of involved users in Bugzilla::Bug->update(). If any of those users become uninvolved after the update, remove their last user visit entries.
Attached patch bug-1020558-v1.patch (obsolete) — Splinter Review
A simpler solution occurred to me this afternoon -- rather than build a list of all the involved users, just iterate over the entries in the last visit table and if they are not involved, remove them. This yields a decidedly simple patch, and meets all of my test criteria.
Attachment #8481008 - Attachment is obsolete: true
Attachment #8484756 - Flags: review?(glob)
Comment on attachment 8484756 [details] [diff] [review]
bug-1020558-v1.patch

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

wrong patch.
Attachment #8484756 - Flags: review?(glob) → review-
And this time, the right patch file even.
Attachment #8484756 - Attachment is obsolete: true
Attachment #8485584 - Flags: review?(glob)
Comment on attachment 8485584 [details] [diff] [review]
bug-1053513-v2.patch

r=glob, with nits fixed on commit

>+    my $last_visits = Bugzilla::BugUserLastVisit->match({bug_id => $self->id});
nit: missing space after { and before }

>+    $self->{user} //= Bugzilla::User->new({id => $self->user_id, cache => 1});
nit: missing space after { and before }
Attachment #8485584 - Attachment description: bug-1053513-v1.patch → bug-1053513-v2.patch
Attachment #8485584 - Attachment filename: bug-1053513-v1.patch → bug-1053513-v2.patch
Attachment #8485584 - Flags: review?(glob) → review+
Flags: approval?
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   e181022..961fb4f  master -> master
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
How can a patch which hasn't been approved be committed??
(In reply to Frédéric Buclin from comment #15)
> How can a patch which hasn't been approved be committed??

looks like an honest mistake; i'll a=glob this now so things are back in alignment.
Flags: approval? → approval+
Fixed typo in bug.

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   02c6a48..ab1b842  master -> master
Duplicate of this bug: 1032867
Testet with Bug https://bugzilla.mozilla.org/show_bug.cgi?id=839441.
I could say it is realy fixed and bug could set to verify
You need to log in before you can comment on or make changes to this bug.