Move per-row Contact lookup to the database.

On admin report lists, and in front-end lists when an inspector, each row was querying the database for `category_display`. We create a new relationship for this query, and join/prefetch it wherever we request this data. Include staff joins on /around page, copying what happens on /reports to prevent more lookups there too. Also add some joins for user email in admin report list.
mysociety · May 6, 2020 · 683b188 · 683b188
1 parent 30dfd35
commit 683b188
Show file tree

Hide file tree

Showing 18 changed files with 131 additions and 72 deletions.
diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -71,13 +71,16 @@ sub index : Path : Args(0) {
     }
 
     my @unsent = $c->cobrand->problems->search( {
-        state => [ FixMyStreet::DB::Result::Problem::open_states() ],
+        'me.state' => [ FixMyStreet::DB::Result::Problem::open_states() ],
         whensent => undef,
         bodies_str => { '!=', undef },
         # Ignore very recent ones that probably just haven't been sent yet
         confirmed => { '<', \"current_timestamp - '5 minutes'::interval" },
     },
     {
+        '+columns' => ['user.email'],
+        prefetch => 'contact',
+        join => 'user',
         order_by => 'confirmed',
     } )->all;
     $c->stash->{unsent_reports} = \@unsent;
@@ -301,7 +304,14 @@ sub add_flags : Private {
 sub flagged : Path('flagged') : Args(0) {
     my ( $self, $c ) = @_;
 
-    my $problems = $c->cobrand->problems->search( { flagged => 1 } );
+    my $problems = $c->cobrand->problems->search(
+        { 'me.flagged' => 1 },
+        {
+            '+columns' => ['user.email'],
+            join => 'user',
+            prefetch => 'contact',
+        }
+    );
 
     # pass in as array ref as using same template as search_reports
     # which has to use an array ref for sql quoting reasons

diff --git a/perllib/FixMyStreet/App/Controller/Admin/Reports.pm b/perllib/FixMyStreet/App/Controller/Admin/Reports.pm
@@ -110,6 +110,7 @@ sub index : Path {
             {
                 join => 'user',
                 '+columns' => 'user.email',
+                prefetch => 'contact',
                 rows => 50,
                 order_by => $order,
             }
@@ -166,7 +167,13 @@ sub index : Path {
 
         my $problems = $c->cobrand->problems->search(
             $query,
-            { order_by => $order, rows => 50 }
+            {
+                '+columns' => ['user.email'],
+                join => 'user',
+                prefetch => 'contact',
+                order_by => $order,
+                rows => 50
+            }
         )->page( $p_page );
         $c->stash->{problems} = [ $problems->all ];
         $c->stash->{problems_pager} = $problems->pager;

diff --git a/perllib/FixMyStreet/App/Controller/Dashboard.pm b/perllib/FixMyStreet/App/Controller/Dashboard.pm
@@ -591,7 +591,15 @@ sub heatmap_sidebar :Private {
         order_by => 'lastupdate',
     })->all ];
 
-    my $params = { map { my $n = $_; s/me\./problem\./; $_ => $where->{$n} } keys %$where };
+    my $params = { map {
+        my $v = $where->{$_};
+        if (ref $v eq 'HASH') {
+            $v = { map { my $vv = $v->{$_}; s/me\./problem\./; $_ => $vv } keys %$v };
+        } else {
+            s/me\./problem\./;
+        }
+        $_ => $v;
+    } keys %$where };
     my $body = $c->stash->{body};
 
     my @user;

diff --git a/perllib/FixMyStreet/App/Controller/My.pm b/perllib/FixMyStreet/App/Controller/My.pm
@@ -135,20 +135,22 @@ sub get_problems : Private {
     my $problems = [];
 
     my $states = $c->stash->{filter_problem_states};
+    my $table = $c->action eq 'my/planned' ? 'report' : 'me';
     my $params = {
-        state => [ keys %$states ],
+        "$table.state" => [ keys %$states ],
     };
 
     my $categories = [ $c->get_param_list('filter_category', 1) ];
     if ( @$categories ) {
-        $params->{category} = $categories;
+        $params->{"$table.category"} = $categories;
         $c->stash->{filter_category} = { map { $_ => 1 } @$categories };
     }
 
     my $rows = 50;
     $rows = 5000 if $c->stash->{sort_key} eq 'shortlist'; # Want all reports
 
     my $rs = $c->stash->{problems_rs}->search( $params, {
+        prefetch => 'contact',
         order_by => $c->stash->{sort_order},
         rows => $rows,
     } )->include_comment_counts->page( $p_page );
@@ -186,12 +188,14 @@ sub get_updates : Private {
 sub setup_page_data : Private {
     my ($self, $c) = @_;
 
+    my $table = $c->action eq 'my/planned' ? 'report' : 'me';
     my @categories = $c->stash->{problems_rs}->search({
-        state => [ FixMyStreet::DB::Result::Problem->visible_states() ],
+        "$table.state" => [ FixMyStreet::DB::Result::Problem->visible_states() ],
     }, {
-        columns => [ 'category', 'bodies_str', 'extra' ],
+        join => 'contact',
+        columns => [ "$table.category", 'contact.extra', 'contact.category' ],
         distinct => 1,
-        order_by => [ 'category' ],
+        order_by => [ "$table.category" ],
     } )->all;
     $c->stash->{filter_categories} = \@categories;
     $c->forward('/report/stash_category_groups', [ \@categories ]) if $c->cobrand->enable_category_groups;

diff --git a/perllib/FixMyStreet/App/Controller/Report.pm b/perllib/FixMyStreet/App/Controller/Report.pm
@@ -133,11 +133,13 @@ sub support :Chained('id') :Args(0) {
 sub load_problem_or_display_error : Private {
     my ( $self, $c, $id ) = @_;
 
+    my $attrs = { prefetch => 'contact' };
+
     # try to load a report if the id is a number
     my $problem
       = ( !$id || $id =~ m{\D} ) # is id non-numeric?
       ? undef                    # ...don't even search
-      : $c->cobrand->problems->find( { id => $id } )
+      : $c->cobrand->problems->find( { id => $id }, $attrs )
           or $c->detach( '/page_error_404_not_found', [ _('Unknown problem ID') ] );
 
     # check that the problem is suitable to show.

diff --git a/perllib/FixMyStreet/App/Controller/Report/Update.pm b/perllib/FixMyStreet/App/Controller/Report/Update.pm
@@ -121,7 +121,7 @@ sub process_user : Private {
     if ( $c->user_exists ) { {
         my $user = $c->user->obj;
 
-        if ($c->stash->{contributing_as_another_user} = $user->contributing_as('another_user', $c, $update->problem->bodies_str_ids)) {
+        if ($c->stash->{contributing_as_another_user} = $user->contributing_as('another_user', $c, $c->stash->{problem}->bodies_str_ids)) {
             # Act as if not logged in (and it will be auto-confirmed later on)
             last;
         }
@@ -248,7 +248,7 @@ sub load_problem : Private {
     # Problem ID could come from existing update in token, or from query parameter
     my $problem_id = $update->problem_id || $c->get_param('id');
     $c->forward( '/report/load_problem_or_display_error', [ $problem_id ] );
-    $update->problem($c->stash->{problem});
+    $update->problem_id($c->stash->{problem}->id);
 }
 
 =head2 check_form_submitted
@@ -282,19 +282,20 @@ sub process_update : Private {
 
     my $name = Utils::trim_text( $params{name} );
 
-    $params{reopen} = 0 unless $c->user && $c->user->id == $c->stash->{problem}->user->id;
+    my $problem = $c->stash->{problem};
+    $params{reopen} = 0 unless $c->user && $c->user->id == $problem->user->id;
 
     my $update = $c->stash->{update};
     $update->text($params{update});
 
     $update->mark_fixed($params{fixed} ? 1 : 0);
     $update->mark_open($params{reopen} ? 1 : 0);
 
-    $c->stash->{contributing_as_body} = $c->user_exists && $c->user->contributing_as('body', $c, $update->problem->bodies_str_ids);
-    $c->stash->{contributing_as_anonymous_user} = $c->user_exists && $c->user->contributing_as('anonymous_user', $c, $update->problem->bodies_str_ids);
+    $c->stash->{contributing_as_body} = $c->user_exists && $c->user->contributing_as('body', $c, $problem->bodies_str_ids);
+    $c->stash->{contributing_as_anonymous_user} = $c->user_exists && $c->user->contributing_as('anonymous_user', $c, $problem->bodies_str_ids);
 
     # This is also done in process_user, but is needed here for anonymous() just below
-    my $anon_button = $c->cobrand->allow_anonymous_reports($update->problem->category) eq 'button' && $c->get_param('report_anonymously');
+    my $anon_button = $c->cobrand->allow_anonymous_reports($problem->category) eq 'button' && $c->get_param('report_anonymously');
     if ($anon_button) {
         $c->stash->{contributing_as_anonymous_user} = 1;
         $c->stash->{contributing_as_body} = undef;
@@ -323,7 +324,6 @@ sub process_update : Private {
         # then we are not changing the state of the problem so can use the current
         # problem state
         } else {
-            my $problem = $c->stash->{problem} || $update->problem;
             $update->problem_state( $problem->state );
         }
     }
@@ -332,7 +332,7 @@ sub process_update : Private {
     my @extra; # Next function fills this, but we don't need it here.
     # This is just so that the error checking for these extra fields runs.
     # TODO Use extra here as it is used on reports.
-    my $body = (values %{$update->problem->bodies})[0];
+    my $body = (values %{$problem->bodies})[0];
     $c->cobrand->process_open311_extras( $c, $body, \@extra );
 
     if ( $c->get_param('fms_extra_title') ) {

diff --git a/perllib/FixMyStreet/App/Controller/Reports.pm b/perllib/FixMyStreet/App/Controller/Reports.pm
@@ -620,6 +620,9 @@ sub load_problems_parameters : Private {
     };
     if ($c->user_exists && $body) {
         my $prefetch = [];
+        if ($c->user->from_body || $c->user->is_superuser) {
+            push @$prefetch, 'contact';
+        }
         if ($c->user->has_permission_to('planned_reports', $body->id)) {
             push @$prefetch, 'user_planned_reports';
         }
@@ -646,7 +649,7 @@ sub load_problems_parameters : Private {
     }
 
     if (@$category) {
-        $where->{category} = $category;
+        $where->{'me.category'} = $category;
     }
 
     if ($c->stash->{wards}) {
@@ -687,12 +690,12 @@ sub check_non_public_reports_permission : Private {
         }
 
         if ( $user_has_permission ) {
-            $where->{non_public} = 1 if $c->stash->{only_non_public};
+            $where->{'me.non_public'} = 1 if $c->stash->{only_non_public};
         } else {
-            $where->{non_public} = 0;
+            $where->{'me.non_public'} = 0;
         }
     } else {
-        $where->{non_public} = 0;
+        $where->{'me.non_public'} = 0;
     }
 }
 

diff --git a/perllib/FixMyStreet/Cobrand/Bexley.pm b/perllib/FixMyStreet/Cobrand/Bexley.pm
@@ -54,7 +54,7 @@ sub open311_munge_update_params {
 
     $params->{service_request_id_ext} = $comment->problem->id;
 
-    my $contact = $comment->problem->category_row;
+    my $contact = $comment->problem->contact;
     $params->{service_code} = $contact->email;
 }
 

diff --git a/perllib/FixMyStreet/Cobrand/Bromley.pm b/perllib/FixMyStreet/Cobrand/Bromley.pm
@@ -344,23 +344,23 @@ sub munge_load_and_group_problems {
     return unless $c->action eq 'dashboard/heatmap';
 
     # Bromley subcategory stuff
-    if (!$where->{category}) {
+    if (!$where->{'me.category'}) {
         my $cats = $c->user->categories;
         my $subcats = $c->user->get_extra_metadata('subcategories') || [];
-        $where->{category} = [ @$cats, @$subcats ] if @$cats || @$subcats;
+        $where->{'me.category'} = [ @$cats, @$subcats ] if @$cats || @$subcats;
     }
 
     my %subcats = $self->subcategories;
     my $subcat;
-    my %chosen = map { $_ => 1 } @{$where->{category} || []};
+    my %chosen = map { $_ => 1 } @{$where->{'me.category'} || []};
     my @subcat = grep { $chosen{$_} } map { $_->{key} } map { @$_ } values %subcats;
     if (@subcat) {
         my %chosen = map { $_ => 1 } @subcat;
         $where->{'-or'} = {
-            category => [ grep { !$chosen{$_} } @{$where->{category}} ],
-            subcategory => \@subcat,
+            'me.category' => [ grep { !$chosen{$_} } @{$where->{'me.category'}} ],
+            'me.subcategory' => \@subcat,
         };
-        delete $where->{category};
+        delete $where->{'me.category'};
     }
 }
 

diff --git a/perllib/FixMyStreet/Cobrand/EastSussex.pm b/perllib/FixMyStreet/Cobrand/EastSussex.pm
@@ -11,7 +11,6 @@ sub open311_extra_data {
 
     $h->{es_original_detail} = $row->detail;
 
-    $contact = $row->category_row;
     my $fields = $contact->get_extra_fields;
     my $text = '';
     for my $field ( @$fields ) {

diff --git a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm
@@ -139,10 +139,10 @@ sub munge_report_new_contacts {
 sub munge_load_and_group_problems {
     my ($self, $where, $filter) = @_;
 
-    return unless $where->{category} && $self->{c}->stash->{body}->name eq 'Isle of Wight Council';
+    return unless $where->{'me.category'} && $self->{c}->stash->{body}->name eq 'Isle of Wight Council';
 
     my $iow = FixMyStreet::Cobrand->get_class_for_moniker( 'isleofwight' )->new({ c => $self->{c} });
-    $where->{category} = $iow->expand_triage_cat_list($where->{category}, $self->{c}->stash->{body});
+    $where->{'me.category'} = $iow->expand_triage_cat_list($where->{'me.category'}, $self->{c}->stash->{body});
 }
 
 sub title_list {

diff --git a/perllib/FixMyStreet/Cobrand/IsleOfWight.pm b/perllib/FixMyStreet/Cobrand/IsleOfWight.pm
@@ -140,9 +140,9 @@ sub munge_around_category_where {
 sub munge_load_and_group_problems {
     my ($self, $where, $filter) = @_;
 
-    return unless $where->{category};
+    return unless $where->{'me.category'};
 
-    $where->{category} = $self->_expand_triage_cat_list($where->{category});
+    $where->{'me.category'} = $self->_expand_triage_cat_list($where->{'me.category'});
 }
 
 sub munge_around_filter_category_list {

diff --git a/perllib/FixMyStreet/Cobrand/Peterborough.pm b/perllib/FixMyStreet/Cobrand/Peterborough.pm
@@ -85,7 +85,7 @@ sub open311_munge_update_params {
     # Send the FMS problem ID with the update.
     $params->{service_request_id_ext} = $comment->problem->id;
 
-    my $contact = $comment->problem->category_row;
+    my $contact = $comment->problem->contact;
     $params->{service_code} = $contact->email;
 }
 

diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -193,6 +193,27 @@ __PACKAGE__->might_have(
     cascade_copy => 0, cascade_delete => 1 },
 );
 
+# Add a possible join for the Contact object associated with
+# this report (based on bodies_str and category). If the report
+# was sent to multiple bodies, only returns the first.
+__PACKAGE__->belongs_to(
+  contact => "FixMyStreet::DB::Result::Contact",
+  sub {
+    my $args = shift;
+    return {
+        "$args->{foreign_alias}.category" => { -ident => "$args->{self_alias}.category" },
+        -and => [
+            \[ "CAST($args->{foreign_alias}.body_id AS text) = (regexp_split_to_array($args->{self_alias}.bodies_str, ','))[1]" ],
+        ]
+    };
+  },
+  {
+    join_type => "LEFT",
+    on_delete => "NO ACTION",
+    on_update => "NO ACTION",
+  },
+);
+
 __PACKAGE__->load_components("+FixMyStreet::DB::RABXColumn");
 __PACKAGE__->rabx_column('extra');
 __PACKAGE__->rabx_column('geocode');
@@ -407,30 +428,11 @@ sub confirm {
 
 sub category_display {
     my $self = shift;
-    my $contact = $self->category_row;
+    my $contact = $self->contact;
     return $self->category unless $contact; # Fallback; shouldn't happen, but some tests
     return $contact->category_display;
 }
 
-=head2 category_row
-
-Returns the corresponding Contact object for this problem's category and body.
-If the report was sent to multiple bodies, only returns the first.
-
-=cut
-
-sub category_row {
-    my $self = shift;
-    my $schema = $self->result_source->schema;
-    my $body_id = $self->bodies_str_ids->[0];
-    return unless $body_id && $body_id =~ /^[0-9]+$/;
-    my $contact = $schema->resultset("Contact")->find({
-        body_id => $body_id,
-        category => $self->category,
-    });
-    return $contact;
-}
-
 sub bodies_str_ids {
     my $self = shift;
     return [] unless $self->bodies_str;