Improve daemon database updating.

Introduce a new open311_update_missing_data cobrand hook to be used to
add/save any missing data to the report before sending via Open311, that
is wrapped in a transaction. open311_extra_data_include should now only
be used to add columns for sending, not to edit the row in any way.

When sending via Open311, any changes made pre-send (after
update_missing_data) are now automatically discarded after sending.

perllib/FixMyStreet/Cobrand/Bexley.pm

@@ -118,21 +118,12 @@ sub open311_config {
     $params->{multi_photos} = 1;
 }
 
-sub open311_extra_data_include {
+sub open311_update_missing_data {
     my ($self, $row, $h, $contact) = @_;
 
-    my $open311_only;
     my $feature = $self->lookup_site_code($row);
     my $extra = $row->get_extra_fields;
     if ($contact->email =~ /^Confirm/) {
-        push @$open311_only,
-            { name => 'report_url', description => 'Report URL',
-              value => $h->{url} },
-            { name => 'title', description => 'Title',
-              value => $row->title },
-            { name => 'description', description => 'Detail',
-              value => $row->detail };
-
         if (!$row->get_extra_field_value('site_code')) {
             if (my $ref = $feature->{properties}{NSG_REF}) {
                 $row->update_extra_field({ name => 'site_code', value => $ref, description => 'Site code' });
@@ -159,18 +150,27 @@ sub open311_extra_data_include {
     }
 
     if ($feature && $feature->{properties}{ADDRESS}) {
-        # Note subtle issue here - the first call to update_extra_field above
-        # updates the original extra (stored by SendReport/Open311 in
-        # original_extra and thus kept when it 'rolls back' after adding the
-        # open311-only entries), but it is changed at the end of that call, so
-        # a second call to update_extra_field will update extra but not the
-        # original_extra and thus will be lost and not available to
-        # open311_post_send. We'll use extra_metadata here anyway.
         my $address = $feature->{properties}{ADDRESS};
         $address =~ s/([\w']+)/\u\L$1/g;
         my $town = $feature->{properties}{TOWN};
         $town =~ s/([\w']+)/\u\L$1/g;
-        $row->set_extra_metadata(nsg => { name => $address, area => $town });
+        $self->{cache_nsg} = { name => $address, area => $town };
+    }
+}
+
+sub open311_extra_data_include {
+    my ($self, $row, $h, $contact) = @_;
+
+    my $open311_only;
+
+    if ($contact->email =~ /^Confirm/) {
+        push @$open311_only,
+            { name => 'report_url', description => 'Report URL',
+              value => $h->{url} },
+            { name => 'title', description => 'Title',
+              value => $row->title },
+            { name => 'description', description => 'Detail',
+              value => $row->detail };
     }
 
     # Add private comments field
@@ -287,10 +287,9 @@ sub open311_post_send {
         $row->push_extra_fields({ name => 'uniform_id', description => 'Uniform ID', value => $row->external_id });
     }
 
-    if (my $nsg = $row->get_extra_metadata('nsg')) {
+    if (my $nsg = $self->{cache_nsg}) {
         $row->push_extra_fields({ name => 'NSGName', description => 'Street name', value => $nsg->{name} });
         $row->push_extra_fields({ name => 'NSGArea', description => 'Street area', value => $nsg->{area} });
-        $row->unset_extra_metadata('nsg');
     }
     $row->push_extra_fields({ name => 'fixmystreet_id', description => 'FMS reference', value => $row->id });
 

perllib/FixMyStreet/Cobrand/Brent.pm

@@ -373,13 +373,13 @@ sub should_skip_sending_update {
 }
 
 
-=head2 open311_extra_data_include
+=head2 open311_update_missing_data
 
 =over 4
 
 =cut
 
-sub open311_extra_data_include {
+sub open311_update_missing_data {
     my ($self, $row, $h, $contact) = @_;
 
 =item * Adds NSGRef from WFS service as app doesn't include road layer for Symology
@@ -390,27 +390,13 @@ WFS service at the point we're sending the report over Open311.
 
 =cut
 
-    my $open311_only;
     if ($contact->email =~ /^Symology/) {
-
         if (!$row->get_extra_field_value('NSGRef')) {
             if (my $ref = $self->lookup_site_code($row, 'usrn')) {
                 $row->update_extra_field({ name => 'NSGRef', description => 'NSG Ref', value => $ref });
             }
         }
 
-=item * Copies UnitID into the details field for the Drains and gullies category
-
-=cut
-
-        if ($contact->groups->[0] eq 'Drains and gullies') {
-            if (my $id = $row->get_extra_field_value('UnitID')) {
-                $self->{brent_original_detail} = $row->detail;
-                my $detail = $row->detail . "\n\nukey: $id";
-                $row->detail($detail);
-            }
-        }
-
 =item * Adds NSGRef from WFS service as app doesn't include road layer for Echo
 
 Same as Symology above, but different attribute name.
@@ -424,19 +410,6 @@ Same as Symology above, but different attribute name.
                 $row->update_extra_field({ name => 'usrn', description => 'USRN', value => $ref });
             }
         }
-
-=item * Adds information for constructing the description on the open311 side.
-
-=cut
-
-    } elsif ($contact->email =~ /^ATAK/) {
-
-        push @$open311_only, { name => 'title', value => $row->title };
-        push @$open311_only, { name => 'report_url', value => $h->{url} };
-        push @$open311_only, { name => 'detail', value => $row->detail };
-        push @$open311_only, { name => 'group', value => $row->get_extra_metadata('group') || '' };
-
-
     }
 
 =item * Adds location name from WFS service for reports in ATAK groups, if missing.
@@ -454,6 +427,43 @@ Same as Symology above, but different attribute name.
             $row->update_extra_field({ name => 'location_name', description => 'Location name', value => $name });
         }
     }
+}
+
+=back
+
+=head2 open311_extra_data_include
+
+=over 4
+
+=cut
+
+sub open311_extra_data_include {
+    my ($self, $row, $h, $contact) = @_;
+
+    my $open311_only;
+
+=item * Copies UnitID into the details field for the Drains and gullies category
+
+=cut
+
+    if ($contact->email =~ /^Symology/) {
+        if ($contact->groups->[0] eq 'Drains and gullies') {
+            if (my $id = $row->get_extra_field_value('UnitID')) {
+                my $detail = $row->detail . "\n\nukey: $id";
+                $row->detail($detail);
+            }
+        }
+
+=item * Adds information for constructing the description on the open311 side.
+
+=cut
+
+    } elsif ($contact->email =~ /^ATAK/) {
+        push @$open311_only, { name => 'title', value => $row->title };
+        push @$open311_only, { name => 'report_url', value => $h->{url} };
+        push @$open311_only, { name => 'detail', value => $row->detail };
+        push @$open311_only, { name => 'group', value => $row->get_extra_metadata('group') || '' };
+    }
 
 =item * The title field gets pushed to location fields in Echo/Symology, so include closest address
 
@@ -481,8 +491,6 @@ We use {closest_address}->summary as this is geocoder-agnostic.
 
 =back
 
-=cut
-
 =head2 open311_extra_data_exclude
 
 Doesn't send UnitID for Drains and gullies category as an extra
@@ -507,11 +515,10 @@ to put the UnitID in the detail field for sending
 
 sub open311_post_send {
     my ($self, $row) = @_;
+
     if ($row->contact->email =~ /ATAK/ && $row->external_id) {
-        $row->state('investigating');
+        $row->update({ state => 'investigating' });
     }
-
-    $row->detail($self->{brent_original_detail}) if $self->{brent_original_detail};
 }
 
 =head2 lookup_location_name
@@ -783,9 +790,10 @@ around garden_cc_check_payment_status => sub {
             if ($resp->{paymentResult}->{status} eq 'Success') {
                 my $auth_details
                     = $resp->{paymentResult}{paymentDetails}{authDetails};
-                $p->set_extra_metadata( 'authCode', $auth_details->{authCode} );
-                $p->set_extra_metadata( 'continuousAuditNumber',
-                    $resp->{paymentResult}{paymentDetails}{payments}{paymentSummary}{continuousAuditNumber} );
+                $p->update_extra_metadata(
+                    authCode => $auth_details->{authCode},
+                    continuousAuditNumber => $resp->{paymentResult}{paymentDetails}{payments}{paymentSummary}{continuousAuditNumber},
+                );
                 $p->update_extra_field({ name => 'payment_method', value => 'csc' });
                 $p->update;
 

perllib/FixMyStreet/Cobrand/Bromley.pm

@@ -256,8 +256,6 @@ sub open311_pre_send {
 
     $self->_include_user_title_in_extra($row);
 
-    $self->{bromley_original_detail} = $row->detail;
-
     my $private_comments = $row->get_extra_metadata('private_comments');
     if ($private_comments) {
         my $text = $row->detail . "\n\nPrivate comments: $private_comments";
@@ -306,16 +304,22 @@ sub open311_munge_update_params {
 
 sub open311_post_send {
     my ($self, $row, $h, $sender) = @_;
-    $row->detail($self->{bromley_original_detail});
     my $error = $sender->error;
-    if ($error =~ /Cannot renew this property, a new request is required/ && $row->title eq "Garden Subscription - Renew") {
-        # Was created as a renewal, but due to DD delay has now expired. Switch to new subscription
-        $row->title("Garden Subscription - New");
-        $row->update_extra_field({ name => "Subscription_Type", value => $self->waste_subscription_types->{New} });
-    }
-    if ($error =~ /Missed Collection event already open for the property/) {
-        $row->state('duplicate');
-    }
+    my $db = FixMyStreet::DB->schema->storage;
+    $db->txn_do(sub {
+        my $row2 = FixMyStreet::DB->resultset('Problem')->search({ id => $row->id }, { for => \'UPDATE' })->single;
+        if ($error =~ /Cannot renew this property, a new request is required/ && $row2->title eq "Garden Subscription - Renew") {
+            # Was created as a renewal, but due to DD delay has now expired. Switch to new subscription
+            $row2->title("Garden Subscription - New");
+            $row2->update_extra_field({ name => "Subscription_Type", value => $self->waste_subscription_types->{New} });
+            $row2->update;
+            $row->discard_changes;
+        } elsif ($error =~ /Missed Collection event already open for the property/) {
+            $row2->state('duplicate');
+            $row2->update;
+            $row->discard_changes;
+        }
+    });
 }
 
 sub open311_contact_meta_override {

perllib/FixMyStreet/Cobrand/Buckinghamshire.pm

@@ -147,8 +147,7 @@ sub send_questionnaires {
 
 sub open311_extra_data_exclude { [ 'road-placement' ] }
 
-
-=head2 open311_extra_data_include
+=head2 open311_update_missing_data
 
 All reports sent to Alloy should have a parent asset they're associated with.
 This is indicated by the value in the asset_resource_id field. For certain
@@ -159,11 +158,8 @@ Alloy server and use that as the parent.
 
 =cut
 
-around open311_extra_data_include => sub {
-    my ($orig, $self) = (shift, shift);
-    my $open311_only = $self->$orig(@_);
-
-    my ($row, $h, $contact) = @_;
+sub open311_update_missing_data {
+    my ($self, $row, $h, $contact) = @_;
 
     # If the report doesn't already have an asset, associate it with the
     # closest feature from the Alloy highways network layer.
@@ -174,10 +170,22 @@ around open311_extra_data_include => sub {
             $row->update_extra_field({ name => 'asset_resource_id', value => $item_id });
         }
     }
+}
+
+=head2 open311_extra_data_include
+
+Reports in the "Apply for Access Protection Marking" category have some
+extra field values that we want to append to the report description
+before it's passed to Alloy.
+
+=cut
+
+around open311_extra_data_include => sub {
+    my ($orig, $self) = (shift, shift);
+    my $open311_only = $self->$orig(@_);
+
+    my ($row, $h, $contact) = @_;
 
-    # Reports in the "Apply for Access Protection Marking" category have some
-    # extra field values that we want to append to the report description
-    # before it's passed to Alloy
     if (my $address = $row->get_extra_field_value('ADDRESS_POSTCODE')) {
         my $phone = $row->get_extra_field_value('TELEPHONE_NUMBER') || "";
         for (@$open311_only) {

perllib/FixMyStreet/Cobrand/Camden.pm

@@ -72,7 +72,7 @@ sub lookup_site_code_config {
     }
 }
 
-sub open311_extra_data_include {
+sub open311_update_missing_data {
     my ($self, $row, $h, $contact) = @_;
 
     # Reports made via the app probably won't have a NSGRef because we don't
@@ -83,8 +83,6 @@ sub open311_extra_data_include {
             $row->update_extra_field({ name => 'NSGRef', description => 'NSG Ref', value => $ref });
         }
     }
-
-    return [];
 }
 
 sub open311_config {

perllib/FixMyStreet/Cobrand/CentralBedfordshire.pm

@@ -192,11 +192,25 @@ sub lookup_site_code_config {
     }
 }
 
+sub open311_update_missing_data {
+    my ($self, $row, $h, $contact) = @_;
+
+    return if $contact->email =~ /Jadu/;
+
+    # Reports made via the app probably won't have a NSGRef because we don't
+    # display the road layer. Instead we'll look up the closest asset from the
+    # WFS service at the point we're sending the report over Open311.
+    if (!$row->get_extra_field_value('NSGRef')) {
+        if (my $ref = $self->lookup_site_code($row, 'streetref1')) {
+            $row->update_extra_field({ name => 'NSGRef', description => 'NSG Ref', value => $ref });
+        }
+    }
+}
+
 sub open311_extra_data_include {
     my ($self, $row, $h, $contact) = @_;
 
     if (my $id = $row->get_extra_field_value('UnitID')) {
-        $h->{cb_original_detail} = $row->detail;
         $row->detail($row->detail . "\n\nUnit ID: $id");
     }
 
@@ -225,15 +239,6 @@ sub open311_extra_data_include {
         return $open311_only;
     }
 
-    # Reports made via the app probably won't have a NSGRef because we don't
-    # display the road layer. Instead we'll look up the closest asset from the
-    # WFS service at the point we're sending the report over Open311.
-    if (!$row->get_extra_field_value('NSGRef')) {
-        if (my $ref = $self->lookup_site_code($row, 'streetref1')) {
-            $row->update_extra_field({ name => 'NSGRef', description => 'NSG Ref', value => $ref });
-        }
-    }
-
     if (my $cfg = $self->feature('area_code_mapping')) {;
         my @areas = split ',', $row->areas;
         my @matches = grep { $_ } map { $cfg->{$_} } @areas;
@@ -258,8 +263,6 @@ sub open311_post_send {
     # No post send needed for Jadu backed categories.
     return if $row->external_id && $row->external_id =~ /Jadu/;
 
-    $row->detail($h->{cb_original_detail}) if $h->{cb_original_detail};
-
     # Check Open311 was successful
     return unless $row->external_id;