Merge branch 'issues/commercial/1514-extra-data-protected'

6 files changed, 164 insertions, 3 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 401241388..295dd04ae 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,6 +52,7 @@
         - Improve JSON output of controller.
         - unset external_status_code if blank in update
         - Add support for account_id parameter to POST Service Request calls.
+        - Do not overwrite/remove protected meta data. #2598
 
 * v2.6 (3rd May 2019)
     - New features:
diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 3cd6bfd70..45ac1534d 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -1190,6 +1190,8 @@ sub update_extra_fields : Private {
         $meta->{required} = $required ? 'true' : 'false';
         my $notice = $c->get_param("metadata[$i].notice") && $c->get_param("metadata[$i].notice") eq 'on';
         $meta->{variable} = $notice ? 'false' : 'true';
+        my $protected = $c->get_param("metadata[$i].protected") && $c->get_param("metadata[$i].protected") eq 'on';
+        $meta->{protected} = $protected ? 'true' : 'false';
         $meta->{description} = $c->get_param("metadata[$i].description");
         $meta->{datatype_description} = $c->get_param("metadata[$i].datatype_description");
         $meta->{automated} = $c->get_param("metadata[$i].automated")
diff --git a/perllib/Open311/PopulateServiceList.pm b/perllib/Open311/PopulateServiceList.pm
index 5f7ca10a3..f6e8c8668 100644
--- a/perllib/Open311/PopulateServiceList.pm
+++ b/perllib/Open311/PopulateServiceList.pm
@@ -161,7 +161,12 @@ sub _handle_existing_contact {
     if ( $contact and lc($metadata) eq 'true' ) {
         $self->_add_meta_to_contact( $contact );
     } elsif ( $contact and $contact->extra and lc($metadata) eq 'false' ) {
-        $contact->set_extra_fields();
+        # check if there are any protected fields that we should not delete
+        my @meta = (
+            grep { $_->{protected} }
+            @{ $contact->get_extra_fields }
+        );
+        $contact->set_extra_fields(@meta);
         $contact->update;
     }
 
@@ -226,13 +231,26 @@ sub _add_meta_to_contact {
         return;
     }
 
-    # turn the data into something a bit more friendly to use
+    # check if there are any protected fields that we should not overwrite
+    my $protected = {
+        map { $_->{code} => $_ }
+        grep { $_->{protected} }
+        @{ $contact->get_extra_fields }
+    };
     my @meta =
+        map { $protected->{$_->{code}} ? delete $protected->{$_->{code}} : $_ }
+        @{ $meta_data->{attributes} };
+
+    # and then add back in any protected fields that we don't fetch
+    push @meta, values %$protected;
+
+    # turn the data into something a bit more friendly to use
+    @meta =
         # remove trailing colon as we add this when we display so we don't want 2
         map { $_->{description} =~ s/:\s*$//; $_ }
         # there is a display order and we only want to sort once
         sort { $a->{order} <=> $b->{order} }
-        @{ $meta_data->{attributes} };
+        @meta;
 
     # Some Open311 endpoints, such as Bromley and Warwickshire send <metadata>
     # for attributes which we *don't* want to display to the user (e.g. as
diff --git a/t/app/controller/admin/reportextrafields.t b/t/app/controller/admin/reportextrafields.t
index e02df864f..6011b13e3 100644
--- a/t/app/controller/admin/reportextrafields.t
+++ b/t/app/controller/admin/reportextrafields.t
@@ -78,6 +78,7 @@ FixMyStreet::override_config {
             code => "string_test",
             required => "true",
             variable => "true",
+            protected => "false",
             description => "this is a test description",
             datatype_description => "hint here",
             datatype => "string",
@@ -106,6 +107,7 @@ FixMyStreet::override_config {
             code => "list_test",
             required => "false",
             variable => "true",
+            protected => "false",
             description => "this field is a list",
             datatype_description => "",
             datatype => "singlevaluelist",
@@ -142,6 +144,7 @@ FixMyStreet::override_config {
             description => '',
             required => 'false',
             variable => 'true',
+            protected => 'false',
             code => 'POT',
             automated => 'server_set'
         } ], "automated fields not unset";
@@ -177,6 +180,7 @@ FixMyStreet::override_config {
             code => "string_test",
             required => "true",
             variable => "true",
+            protected => "false",
             description => "this is a test description",
             datatype_description => "hint here",
             datatype => "string",
@@ -204,6 +208,7 @@ FixMyStreet::override_config {
             code => "list_test",
             required => "false",
             variable => "true",
+            protected => "false",
             description => "this field is a list",
             datatype_description => "",
             datatype => "singlevaluelist",
@@ -233,6 +238,7 @@ FixMyStreet::override_config {
             code => "automated_test",
             required => "false",
             variable => "true",
+            protected => "false",
             description => "",
             datatype_description => "",
             datatype => "string",
diff --git a/t/open311/populate-service-list.t b/t/open311/populate-service-list.t
index 9f8b4d9f0..3d3144d1f 100644
--- a/t/open311/populate-service-list.t
+++ b/t/open311/populate-service-list.t
@@ -606,6 +606,134 @@ for my $test (
         ',
     },
     {
+        desc => 'check protected meta data not overwritten',
+        has_meta => 1,
+        end_meta => [ {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Bin type',
+                order => 1,
+                description => 'Bin type',
+                protected => 'true'
+
+        } ],
+        orig_meta => [ {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Bin type',
+                order => 1,
+                description => 'Bin type',
+                protected => 'true'
+
+        } ],
+        meta_xml => '<?xml version="1.0" encoding="utf-8"?>
+    <service_definition>
+        <service_code>100</service_code>
+        <attributes>
+            <attribute>
+                <variable>true</variable>
+                <code>type</code>
+                <datatype>string</datatype>
+                <required>true</required>
+                <datatype_description>Type of bin</datatype_description>
+                <order>1</order>
+                <description>Type of bin</description>
+            </attribute>
+        </attributes>
+    </service_definition>
+        ',
+    },
+    {
+        desc => 'check protected meta data retained',
+        has_meta => 1,
+        end_meta => [
+            {
+                variable => 'true',
+                code => 'type2',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Type of bin',
+                order => 1,
+                description => 'Type of bin',
+
+            },
+            {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Number of bin',
+                order => 1,
+                description => 'Number of bin',
+                protected => 'true'
+            },
+        ],
+        orig_meta => [ {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Number of bin',
+                order => 1,
+                description => 'Number of bin',
+                protected => 'true'
+
+        } ],
+        meta_xml => '<?xml version="1.0" encoding="utf-8"?>
+    <service_definition>
+        <service_code>100</service_code>
+        <attributes>
+            <attribute>
+                <variable>true</variable>
+                <code>type2</code>
+                <datatype>string</datatype>
+                <required>true</required>
+                <datatype_description>Type of bin</datatype_description>
+                <order>1</order>
+                <description>Type of bin</description>
+            </attribute>
+        </attributes>
+    </service_definition>
+        ',
+    },
+    {
+        desc => 'check protected meta data retained on removal of all Open311 extras',
+        end_meta => [
+            {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Number of bin',
+                order => 1,
+                description => 'Number of bin',
+                protected => 'true'
+            },
+        ],
+        orig_meta => [ {
+                variable => 'true',
+                code => 'type',
+                datatype => 'string',
+                required => 'true',
+                datatype_description => 'Number of bin',
+                order => 1,
+                description => 'Number of bin',
+                protected => 'true'
+
+        } ],
+        meta_xml => '<?xml version="1.0" encoding="utf-8"?>
+    <service_definition>
+        <service_code>100</service_code>
+        <attributes>
+        </attributes>
+    </service_definition>
+        ',
+    },
+    {
         desc => 'check empty meta data handled',
         has_meta => 1,
         orig_meta => [],
diff --git a/templates/web/base/admin/extra-metadata-form.html b/templates/web/base/admin/extra-metadata-form.html
index aad14ea37..d621a7d79 100644
--- a/templates/web/base/admin/extra-metadata-form.html
+++ b/templates/web/base/admin/extra-metadata-form.html
@@ -37,6 +37,12 @@
             <input name="metadata[[% loop.index %]].notice" data-field-name="notice" type=checkbox [% meta.variable == 'false' ? 'checked' : '' %]>
         </label>
 
+        <div class="admin-hint"><p>[% loc('If ticked this extra data will not be edited or deleted by the Open311 population script.') %]</p></div>
+        <label>
+            [% loc('Protected') %]
+            <input name="metadata[[% loop.index %]].protected" data-field-name="protected" type=checkbox [% meta.protected == 'true' ? 'checked' : '' %]>
+        </label>
+
         <div class="admin-hint"><p>[% loc('The field name as shown to the user on the report form.') %]</p></div>
         <label>
             [% loc('Description') %]