[Rt-commit] rt branch, 4.4/recently-viewed-bogus-tickets, created. rt-4.4.4-14-g967526f84

Michel Rodriguez michel at bestpractical.com
Wed Feb 26 06:26:19 EST 2020


The branch, 4.4/recently-viewed-bogus-tickets has been created
        at  967526f8403a22d5331d1f5589c7615d761fff06 (commit)

- Log -----------------------------------------------------------------
commit 6c69944e20fd0e45328c3fd2e9ee0989b28442e9
Author: michel <michel at bestpractical.com>
Date:   Thu Apr 11 16:00:29 2019 +0200

    Fix RecentlyViewedTickets to deal with shredded/merged tickets.
    
    If a ticket that was recently visited is:
      shredded : it is now removed from the list
      merged   : the list is deduplicated if the parent was already included
    
    The cached RecentlyViewedTickets attribute is then updated
    
    RecentlyViewedTickets now returns a list of hashes { id => , subject => }
    so that the code in the template doesn't have to deal with this
    
      Fixes: I#213199
    
    cosm

diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 6c4bc08d8..b2724ffa8 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -2179,16 +2179,52 @@ sub ToggleBookmark {
 
 =head2 RecentlyViewedTickets TICKET
 
-Returns a list of two-element (ticket id, timestamp) array references ordered by recently viewed first.
+Returns a list of hashrefs { id => <ticket_id>, subject => <ticket_subject> } )
+ordered by recently viewed first.
+
+If a ticket cannot be loaded (eg because it has been shredded) or is duplicated
+(eg because 2 tickets on the list have been merged), it is not returned and the
+user's RecentlyViewedTickets list is updated
 
 =cut
 
 sub RecentlyViewedTickets {
     my $self = shift;
-    my $content = $self->FirstAttribute('RecentlyViewedTickets');
-    return $content ? @{$content->Content} : ();
+    my $recentlyViewedTickets = $self->FirstAttribute( 'RecentlyViewedTickets' );
+    my $content    = $recentlyViewedTickets ? $recentlyViewedTickets->Content : undef;
+    my @storedList = $content ? @$content : ();  # [ [ <id>, <timestamp>], ...]
+    my @toDisplay  = ();                         # [ { id => <id>, title => <title> }, ... ]
+    if ( @storedList ) {
+        my @currentList;       # same as @storedList, without deleted tickets
+        my $updateStoredList;  # set to 1 if a ticket does not load
+        my %seen;              # used to check that we don't have duplicates (in case of merges)
+
+        foreach my $ticketRef ( @storedList ) {
+            my ($ticketId, $timestamp) = @$ticketRef;
+            my $ticket = RT::Ticket->new( $self );
+            $ticket->Load( $ticketId );
+            my $realId= $ticket->Id; # can be undef or changed in case of merge
+            if ( $ticket->Id && ! $seen{$realId} ) {
+                push @toDisplay, { id => $ticket->Id, subject => $ticket->Subject };
+                push @currentList, [ $ticketId, $timestamp ];
+                $seen{$realId}++;
+            } else {
+                # non existent ticket, do not add to @currentList, list needs to be updated
+                $updateStoredList = 1;
+            }
+        }
+
+        if ( $updateStoredList ) {
+            $self->SetAttribute(
+                Name    => 'RecentlyViewedTickets',
+                Content => \@currentList,
+            );
+        }
+    }
+    return @toDisplay;
 }
 
+
 =head2 AddRecentlyViewedTicket TICKET
 
 Takes an RT::Ticket object and adds it to the current user's RecentlyViewedTickets
diff --git a/share/html/Elements/Tabs b/share/html/Elements/Tabs
index 755c8d219..7a75b39d7 100644
--- a/share/html/Elements/Tabs
+++ b/share/html/Elements/Tabs
@@ -604,20 +604,15 @@ my $build_main_nav = sub {
     $tickets->child( new    => title => loc('New Search'),    path => "/Search/Build.html?NewQuery=1" );
 
     my $recents = $tickets->child( recent => title => loc('Recently Viewed'));
-    for ($session{CurrentUser}->RecentlyViewedTickets) {
-        my ($ticketId, $timestamp) = @$_;
-        my $ticket = RT::Ticket->new($session{CurrentUser});
-        $ticket->Load($ticketId);
-        if ($ticket->Id) {
-            my $title = $ticket->Subject || loc("(No subject)");
-            if (length $title > 50) {
-                $title = substr($title, 0, 47);
-                $title =~ s/\s+$//;
-                $title .= "...";
-            }
-            $title = "#$ticketId: " . $title;
-            $recents->child( "$ticketId" => title => $title, path => "/Ticket/Display.html?id=" . $ticket->Id );
+    for my $ticket ( $session{CurrentUser}->RecentlyViewedTickets ) {
+        my $title = $ticket->{subject} || loc( "(No subject)" );
+        if ( length $title > 50 ) {
+            $title = substr($title, 0, 47);
+            $title =~ s/\s+$//;
+            $title .= "...";
         }
+        $title = "#$ticket->{id}: " . $title;
+        $recents->child( "$ticket->{id}" => title => $title, path => "/Ticket/Display.html?id=" . $ticket->{id} );
     }
 
     $search->child( articles => title => loc('Articles'),   path => "/Articles/Article/Search.html" )

commit 967526f8403a22d5331d1f5589c7615d761fff06
Author: michel <michel at bestpractical.com>
Date:   Wed Feb 26 11:29:49 2020 +0100

    Tests for recently-viewed-bogus-tickets.

diff --git a/t/api/user_recently_visited.t b/t/api/user_recently_visited.t
new file mode 100644
index 000000000..e72f60cb0
--- /dev/null
+++ b/t/api/user_recently_visited.t
@@ -0,0 +1,77 @@
+use strict;
+use warnings;
+
+# test the RecentlyViewedTickets method
+
+use Test::Deep;
+use RT::Test::Shredder tests => undef;
+my $test = "RT::Test::Shredder";
+
+{   
+    my $user = RT::Test->load_or_create_user( Name => 'tester');
+    ok( $user && $user->Id, 'created user');
+
+    # need ShowTicket to visit, ModifyTicket to merge
+    ok( RT::Test->add_rights( { Principal => $user, Right => [qw(SeeQueue CreateTicket ShowTicket ModifyTicket )] }), 'granting rights');
+
+    is( $user->_visited_nb, 0, 'init: empty RecentlyViewedTickets');
+
+    my %ticket;
+    foreach my $ticket_code ('a'..'t') {
+        my $new_ticket = RT::Test->create_ticket( Subject => $ticket_code, Queue => 'General' );
+        my $id= $new_ticket->id;  
+        ok( $new_ticket, "ticket '$ticket_code' created");
+        $ticket{$ticket_code} = { ticket => $new_ticket, id => $id, subject => $ticket_code };
+        $new_ticket->ApplyTransactionBatch; # needed for the tests with shredder to work
+    }
+
+    is( $user->_visited_nb, 0, 'tickets created: empty RecentlyViewedTickets');
+
+    # using reverse on the list so it's easier to read later (last visited is first in RecentlyViewedTicket)
+    foreach my $viewed ( reverse qw( c q p b e d c b a ) ) {
+      $user->AddRecentlyViewedTicket( $ticket{$viewed}->{ticket} );
+    }
+
+    is( $user->_visited, 'c,q,p,b,e,d,a', 'visited tickets after inital visits');
+
+    my $shredder = $test->shredder_new();
+
+    $shredder->PutObjects( Objects => $ticket{m}->{ticket} );
+    $shredder->WipeoutAll;
+    is( $user->_visited, 'c,q,p,b,e,d,a', 'visited tickets after shredding an unvisited ticket');
+
+    $shredder->PutObjects( Objects => $ticket{p}->{ticket} );
+    $shredder->PutObjects( Objects => $ticket{d}->{ticket}  );
+    $shredder->WipeoutAll;
+    is( $user->_visited, 'c,q,b,e,a', 'visited tickets after shredding 2 visited tickets');
+
+    my( $ok, $msg) = $ticket{b}->{ticket}->MergeInto( $ticket{j}->{id} ); 
+    if( ! $ok) { die "error merging: $msg\n"; }
+    is( $user->_visited, 'c,q,j,e,a', 'visited tickets after merging into a ticket that was NOT on the list');
+    
+    $ticket{c}->{ticket}->MergeInto( $ticket{a}->{id} ); 
+    is( $user->_visited, 'a,q,j,e', 'visited tickets after merging into a ticket that was on the list');
+    
+    $ticket{e}->{ticket}->MergeInto( $ticket{j}->{id} ); 
+    is( $user->_visited, 'a,q,j', 'visited tickets after merging into a ticket that was on the list (merged)');
+
+    foreach my $viewed ( reverse qw( r j a h k a q a k i s t f g d) ) {
+      $user->AddRecentlyViewedTicket( $ticket{$viewed}->{ticket} );
+    }
+
+    is( $user->_visited, 'r,j,a,h,k,q,i,s,t,f,g', 'visited more than 11 tickets');
+
+}    
+
+done_testing();
+
+# monkey patching not strictly necessary, but makes for a nicer syntax in the tests
+package RT::User;
+
+sub _visited_nb { 
+    return scalar shift->RecentlyViewedTickets; 
+}
+
+sub _visited { 
+    return join ',', map { $_->{subject} } shift->RecentlyViewedTickets; 
+}

-----------------------------------------------------------------------


More information about the rt-commit mailing list