Refactor send reports

CHANGELOG.md

@@ -7,6 +7,9 @@
  - Application user in Docker container can't install packages. #2914
  - Look at all categories when sending reports.
  - Provide access to staff-only categories in admin.
+ - Development improvements:
+ - Refactor Script::Report into an object.
+ - Move summary failures to a separate script.
  - UK:
  - Added junction lookup, so you can search for things like "M60, Junction 2"
 

bin/send-reports-failure-summary

@@ -0,0 +1,20 @@
+#!/usr/bin/env perl
+
+# send-reports-failure-summary:
+# Prints a summary of report sending failures
+
+use strict;
+use warnings;
+use v5.14;
+
+BEGIN {
+ use File::Basename qw(dirname);
+ use File::Spec;
+ my $d = dirname(File::Spec->rel2abs($0));
+ require "$d/../setenv.pl";
+}
+
+use FixMyStreet::Script::Reports;
+
+my $manager = FixMyStreet::Script::Reports->new;
+$manager->end_summary_failures;

perllib/FixMyStreet/Cobrand/Bexley.pm

@@ -83,14 +83,17 @@ sub lookup_site_code_config {
 }
 
 sub open311_config {
- my ($self, $row, $h, $params, $contact) = @_;
+ my ($self, $row, $h, $params) = @_;
 
  $params->{multi_photos} = 1;
+}
 
- my $extra = $row->get_extra_fields;
+sub open311_extra_data {
+ my ($self, $row, $h, $extra, $contact) = @_;
 
+ my $open311_only;
  if ($contact->email =~ /^Confirm/) {
- push @$extra,
+ push @$open311_only,
  { name => 'report_url', description => 'Report URL',
  value => $h->{url} },
  { name => 'title', description => 'Title',
@@ -123,7 +126,7 @@ sub open311_config {
  }
  }
 
- $row->set_extra_fields(@$extra);
+ return $open311_only;
 }
 
 sub admin_user_domain { 'bexley.gov.uk' }

perllib/FixMyStreet/Cobrand/Bromley.pm

@@ -161,17 +161,23 @@ sub title_list {
 sub open311_config {
  my ($self, $row, $h, $params) = @_;
 
- my $extra = $row->get_extra_fields;
+ $params->{always_send_latlong} = 0;
+ $params->{send_notpinpointed} = 1;
+ $params->{extended_description} = 0;
+}
+
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
+
  my $title = $row->title;
 
  foreach (@$extra) {
  next unless $_->{value};
  $title .= ' | ID: ' . $_->{value} if $_->{name} eq 'feature_id';
  $title .= ' | PROW ID: ' . $_->{value} if $_->{name} eq 'prow_reference';
  }
- @$extra = grep { $_->{name} !~ /feature_id|prow_reference/ } @$extra;
 
- push @$extra,
+ my $open311_only = [
  { name => 'report_url',
  value => $h->{url} },
  { name => 'report_title',
@@ -183,23 +189,20 @@ sub open311_config {
  { name => 'requested_datetime',
  value => DateTime::Format::W3CDTF->format_datetime($row->confirmed->set_nanosecond(0)) },
  { name => 'email',
- value => $row->user->email };
+ value => $row->user->email }
+ ];
 
  # make sure we have last_name attribute present in row's extra, so
  # it is passed correctly to Bromley as attribute[]
  if (!$row->get_extra_field_value('last_name')) {
  my ( $firstname, $lastname ) = ( $row->name =~ /(\S+)\.?\s+(.+)/ );
- push @$extra, { name => 'last_name', value => $lastname };
+ push @$open311_only, { name => 'last_name', value => $lastname };
  }
  if (!$row->get_extra_field_value('fms_extra_title') && $row->user->title) {
- push @$extra, { name => 'fms_extra_title', value => $row->user->title };
+ push @$open311_only, { name => 'fms_extra_title', value => $row->user->title };
  }
 
- $row->set_extra_fields(@$extra);
-
- $params->{always_send_latlong} = 0;
- $params->{send_notpinpointed} = 1;
- $params->{extended_description} = 0;
+ return ($open311_only, [ 'feature_id', 'prow_reference' ]);
 }
 
 sub open311_config_updates {

perllib/FixMyStreet/Cobrand/CheshireEast.pm

@@ -60,15 +60,19 @@ sub open311_config {
  my ($self, $row, $h, $params) = @_;
 
  $params->{multi_photos} = 1;
+}
+
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
 
- my $extra = $row->get_extra_fields;
- push @$extra,
+ my $open311_only = [
  { name => 'report_url',
  value => $h->{url} },
  { name => 'title',
  value => $row->title },
  { name => 'description',
- value => $row->detail };
+ value => $row->detail },
+ ];
 
  # Reports made via FMS.com or the app probably won't have a site code
  # value because we don't display the adopted highways layer on those
@@ -82,7 +86,7 @@ sub open311_config {
  }
  }
 
- $row->set_extra_fields(@$extra);
+ return $open311_only;
 }
 
 # TODO These values may not be accurate

perllib/FixMyStreet/Cobrand/Greenwich.pm

@@ -44,13 +44,13 @@ sub reports_per_page { return 20; }
 
 sub admin_user_domain { 'royalgreenwich.gov.uk' }
 
-sub open311_config {
- my ($self, $row, $h, $params) = @_;
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
 
- my $extra = $row->get_extra_fields;
  # Greenwich doesn't have category metadata to fill this
- push @$extra, { name => 'external_id', value => $row->id };
- $row->set_extra_fields( @$extra );
+ return [
+ { name => 'external_id', value => $row->id },
+ ];
 }
 
 sub open311_contact_meta_override {

perllib/FixMyStreet/Cobrand/Northamptonshire.pm

@@ -88,24 +88,24 @@ sub map_type { 'Northamptonshire' }
 sub open311_config {
  my ($self, $row, $h, $params) = @_;
 
- my $extra = $row->get_extra_fields;
+ $params->{multi_photos} = 1;
+}
 
- # remove the emergency category which is informational only
- @$extra = grep { $_->{name} ne 'emergency' } @$extra;
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
 
- push @$extra,
+ return ([
  { name => 'report_url',
  value => $h->{url} },
  { name => 'title',
  value => $row->title },
  { name => 'description',
  value => $row->detail },
  { name => 'category',
- value => $row->category };
-
- $row->set_extra_fields(@$extra);
-
- $params->{multi_photos} = 1;
+ value => $row->category },
+ ], [
+ 'emergency'
+ ]);
 }
 
 sub open311_get_update_munging {

perllib/FixMyStreet/Cobrand/Oxfordshire.pm

@@ -118,20 +118,21 @@ sub state_groups_inspect {
 sub open311_config {
  my ($self, $row, $h, $params) = @_;
 
- my $extra = $row->get_extra_fields;
- push @$extra, { name => 'external_id', value => $row->id };
- push @$extra, { name => 'northing', value => $h->{northing} };
- push @$extra, { name => 'easting', value => $h->{easting} };
-
- if ($h->{closest_address}) {
- push @$extra, { name => 'closest_address', value => "$h->{closest_address}" }
- }
- $row->set_extra_fields( @$extra );
-
  $params->{multi_photos} = 1;
  $params->{extended_description} = 'oxfordshire';
 }
 
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
+
+ return [
+ { name => 'external_id', value => $row->id },
+ { name => 'northing', value => $h->{northing} },
+ { name => 'easting', value => $h->{easting} },
+ $h->{closest_address} ? { name => 'closest_address', value => "$h->{closest_address}" } : (),
+ ];
+}
+
 sub open311_config_updates {
  my ($self, $params) = @_;
  $params->{use_customer_reference} = 1;

perllib/FixMyStreet/Cobrand/Rutland.pm

@@ -26,17 +26,18 @@ sub report_validation {
 sub open311_config {
  my ($self, $row, $h, $params) = @_;
 
- my $extra = $row->get_extra_fields;
- push @$extra, { name => 'external_id', value => $row->id };
- push @$extra, { name => 'title', value => $row->title };
- push @$extra, { name => 'description', value => $row->detail };
+ $params->{multi_photos} = 1;
+}
 
- if ($h->{closest_address}) {
- push @$extra, { name => 'closest_address', value => "$h->{closest_address}" }
- }
- $row->set_extra_fields( @$extra );
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
 
- $params->{multi_photos} = 1;
+ return [
+ { name => 'external_id', value => $row->id },
+ { name => 'title', value => $row->title },
+ { name => 'description', value => $row->detail },
+ $h->{closest_address} ? { name => 'closest_address', value => "$h->{closest_address}" } : (),
+ ];
 }
 
 sub disambiguate_location {

perllib/FixMyStreet/Cobrand/Westminster.pm

@@ -76,8 +76,10 @@ sub open311_config {
  my $id = $row->user->get_extra_metadata('westminster_account_id');
  # Westminster require 0 as the account ID if there's no MyWestminster ID.
  $h->{account_id} = $id || '0';
+}
 
- my $extra = $row->get_extra_fields;
+sub open311_extra_data {
+ my ($self, $row, $h, $extra) = @_;
 
  # Reports made via the app probably won't have a USRN because we don't
  # display the road layer. Instead we'll look up the closest asset from the
@@ -97,8 +99,6 @@ sub open311_config {
  push @$extra, { name => 'UPRN', value => $ref };
  }
  }
-
- $row->set_extra_fields(@$extra);
 }
 
 sub lookup_site_code_config {