Perform comment state check in join, not query.

The old way meant that reports that had updates that were only unconfirmed or hidden were being ignored entirely (as the left join did match).
mysociety · Aug 16, 2024 · 489edbd · 489edbd
1 parent b68f825
commit 489edbd
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@
         - Create reporter alert before creating first unconfirmed auto-update.
         - Fix display of user in assignment dropdown. #4855
         - Fix setting of fixed timestamp in CSV export. #5119
+        - Fix CSV export of reports with only hidden/unconfirmed updates. #5119
     - Admin improvements:
         - Rename emergency message to site message.
         - Added a category control for overriding the text of the new report details field.

diff --git a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
@@ -305,12 +305,12 @@ sub dashboard_export_problems_add_columns {
 
     $csv->objects_attrs({
         '+columns' => [
-            'comments.text', 'comments.extra',
-            {'comments.user.name' => 'user.name'},
+            'confirmed_comments.text', 'confirmed_comments.extra',
+            {'confirmed_comments.user.name' => 'user.name'},
             {'user.email' => 'user_2.email'},
             {'user.phone' => 'user_2.phone'},
         ],
-        join => ['user', { comments => 'user' }],
+        join => ['user', { confirmed_comments => 'user' }],
     });
 
     $csv->add_csv_columns(
@@ -371,7 +371,7 @@ sub dashboard_export_problems_add_columns {
         $fields->{user_phone} = $report->user ? $report->user->phone : '';
 
         my $i = 1;
-        my @updates = $report->comments->all;
+        my @updates = $report->confirmed_comments->all;
         @updates = sort { $a->confirmed <=> $b->confirmed || $a->id <=> $b->id } @updates;
         for my $update (@updates) {
             last if $i > 5;

diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -241,6 +241,18 @@ __PACKAGE__->has_many(
   },
 );
 
+__PACKAGE__->has_many(
+  confirmed_comments => "FixMyStreet::DB::Result::Comment",
+  sub {
+      my $args = shift;
+      return {
+          "$args->{foreign_alias}.problem_id" => { -ident => "$args->{self_alias}.id" },
+          "$args->{foreign_alias}.state" => 'confirmed',
+      };
+  },
+  { cascade_copy => 0, cascade_delete => 0 },
+);
+
 __PACKAGE__->might_have(
   contributed_by => "FixMyStreet::DB::Result::User",
   sub {

diff --git a/perllib/FixMyStreet/Reporting.pm b/perllib/FixMyStreet/Reporting.pm
@@ -197,16 +197,14 @@ sub _csv_parameters_problems {
     my $self = shift;
 
     my $groups = $self->cobrand->enable_category_groups ? 1 : 0;
-    my $join = ['comments'];
-    my $columns = ['comments.id', 'comments.problem_state', 'comments.confirmed', 'comments.mark_fixed'];
+    my $join = ['confirmed_comments'];
+    my $columns = ['confirmed_comments.id', 'confirmed_comments.problem_state', 'confirmed_comments.confirmed', 'confirmed_comments.mark_fixed'];
     if ($groups) {
         push @$join, 'contact';
         push @$columns, 'contact.id', 'contact.extra';
     }
 
-    my $rs = $self->objects_rs->search({
-        "comments.state" => ['confirmed', undef],
-    }, {
+    my $rs = $self->objects_rs->search(undef, {
         join => $join,
         collapse => 1,
         '+columns' => $columns,

diff --git a/perllib/FixMyStreet/Script/CSVExport.pm b/perllib/FixMyStreet/Script/CSVExport.pm
@@ -196,7 +196,7 @@ staff_roles AS (
 EOF
     my @sql_join = (
         '"contacts" "contact" ON CAST( "contact"."body_id" AS text ) = (regexp_split_to_array( "me"."bodies_str", \',\'))[1] AND "contact"."category" = "me"."category"',
-        '"comment" "comments" ON "comments"."problem_id" = "me"."id"',
+        '"comment" "comments" ON "comments"."problem_id" = "me"."id" AND "comments"."state" = \'confirmed\'',
         '"users" "contributed_by_user" ON "contributed_by_user"."id" = ("me"."extra"->>\'contributed_by\')::integer',
         'staff_roles ON contributed_by_user.id = staff_roles.user_id',
     );
@@ -239,8 +239,6 @@ EOF
     return <<EOF;
 DECLARE csr CURSOR WITH HOLD FOR $sql_with SELECT $sql_select FROM "problem" "me" $sql_join
 WHERE regexp_split_to_array("me"."bodies_str", ',') && ARRAY['$body_id']
-    -- Ignore non-confirmed comments, and ones with no or confirmed problem_state
-    AND ("comments"."state" = 'confirmed' OR "comments"."state" IS NULL)
     $where_states
 ORDER BY "me"."confirmed", "me"."id", "comments"."confirmed", "comments"."id";
 EOF

diff --git a/t/cobrand/northumberland.t b/t/cobrand/northumberland.t
@@ -24,6 +24,8 @@ my ($update) = $mech->create_comment_for_problem(
 );
 $update->update({ problem_state => '' }); # simulate a questionnaire response which has mark_fixed true and no problem_state
 
+# Have it so problem1 only has hidden updates
+$mech->create_comment_for_problem($problem1, $staffuser, 'Title', 'text', 0, 'hidden', '');
 
 my $UPLOAD_DIR = tempdir( CLEANUP => 1 );
 FixMyStreet::override_config {