allow multiple groups in open311 services group tag

Only parses as multiple groups if cobrand is configured to handle them
to stop issues with existing groups with commas in them.

Groups are parsed as CSV so as to allow commas in group names.

3 files changed, 182 insertions, 1 deletions

diff --git a/perllib/FixMyStreet/Cobrand/Default.pm b/perllib/FixMyStreet/Cobrand/Default.pm
index 225ab50d7..eaf27e3bc 100644
--- a/perllib/FixMyStreet/Cobrand/Default.pm
+++ b/perllib/FixMyStreet/Cobrand/Default.pm
@@ -1097,6 +1097,18 @@ sub enable_category_groups {
     return $self->feature('category_groups');
 }
 
+=item enable_multiple_category_groups
+
+Whether a category can be included in multiple groups. Required enable_category_groups
+to alse be true.
+
+=cut
+
+sub enable_multiple_category_groups {
+    my $self = shift;
+    return $self->enable_category_groups && $self->feature('multiple_category_groups');
+}
+
 sub default_problem_state { 'unconfirmed' }
 
 sub state_groups_admin {
diff --git a/perllib/Open311/PopulateServiceList.pm b/perllib/Open311/PopulateServiceList.pm
index bbc00231f..5f7ca10a3 100644
--- a/perllib/Open311/PopulateServiceList.pm
+++ b/perllib/Open311/PopulateServiceList.pm
@@ -2,6 +2,7 @@ package Open311::PopulateServiceList;
 
 use Moo;
 use Open311;
+use Text::CSV;
 
 has bodies => ( is => 'ro' );
 has found_contacts => ( is => 'rw', default => sub { [] } );
@@ -263,10 +264,21 @@ sub _set_contact_group {
     my ($self, $contact) = @_;
 
     my $groups_enabled = $self->_current_body_cobrand && $self->_current_body_cobrand->enable_category_groups;
+    my $multi_groups_enabled = $self->_current_body_cobrand && $self->_current_body_cobrand->enable_multiple_category_groups;
     my $old_group = $contact->get_extra_metadata('group') || '';
     my $new_group = $groups_enabled ? $self->_current_service->{group} || '' : '';
 
-    if ($old_group ne $new_group) {
+    if ($multi_groups_enabled && $new_group =~ /,/) {
+        my $csv = Text::CSV->new;
+        if ( $csv->parse($new_group) ) {
+            $new_group = [ $csv->fields ];
+        } else {
+            warn "error parsing groups for " . $self->_current_body_cobrand->moniker . "contact " . $contact->category . ": $new_group\n";
+            $new_group = [ $new_group ];
+        }
+    }
+
+    if ($self->_groups_different($old_group, $new_group)) {
         if ($new_group) {
             $contact->set_extra_metadata(group => $new_group);
             $contact->update({
@@ -300,6 +312,21 @@ sub _set_contact_non_public {
     }) if $keywords{private};
 }
 
+sub _groups_different {
+    my ($self, $old, $new) = @_;
+
+    my $diff = 1;
+    if ($old && $new) {
+        $old = [ $old ] unless ref $old eq 'ARRAY';
+        $new = [ $new ] unless ref $new eq 'ARRAY';
+        $diff = join( ',', sort(@$old) ) ne join( ',', sort(@$new) );
+    } elsif (!$old && !$new) {
+        $diff = 0;
+    }
+
+    return $diff;
+}
+
 sub _delete_contacts_not_in_service_list {
     my $self = shift;
 
diff --git a/t/open311/populate-service-list.t b/t/open311/populate-service-list.t
index fc3423ae6..9f8b4d9f0 100644
--- a/t/open311/populate-service-list.t
+++ b/t/open311/populate-service-list.t
@@ -9,6 +9,7 @@ package main;
 
 use FixMyStreet::Test;
 use FixMyStreet::DB;
+use Test::Warn;
 use utf8;
 
 use_ok( 'Open311::PopulateServiceList' );
@@ -76,6 +77,147 @@ for my $test (
     };
 }
 
+my $last_update = {};
+for my $test (
+    { desc => 'set multiple groups for contact', enable_multi => 1, groups => ['sanitation', 'street']  },
+    { desc => 'groups not edited if unchanged', enable_multi => 1, groups => ['sanitation', 'street'], unchanged => 1  },
+    { desc => 'multiple groups has to be configured', enable_multi => 0, groups => 'sanitation,street'},
+) {
+    subtest $test->{desc} => sub {
+        FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->delete();
+
+        my $services_xml = '<?xml version="1.0" encoding="utf-8"?>
+        <services>
+          <service>
+            <service_code>100</service_code>
+            <service_name>Cans left out 24x7</service_name>
+            <description>Garbage or recycling cans that have been left out for more than 24 hours after collection. Violators will be cited.</description>
+            <metadata>false</metadata>
+            <type>realtime</type>
+            <keywords>lorem, ipsum, dolor</keywords>
+            <group>sanitation,street</group>
+          </service>
+          <service>
+            <service_code>002</service_code>
+            <metadata>false</metadata>
+            <type>realtime</type>
+            <keywords>lorem, ipsum, dolor</keywords>
+            <group>street</group>
+            <service_name>Construction plate shifted</service_name>
+            <description>Metal construction plate covering the street or sidewalk has been moved.</description>
+          </service>
+        </services>
+        ';
+
+        my $service_list = get_xml_simple_object($services_xml);
+
+        FixMyStreet::override_config {
+            ALLOWED_COBRANDS => [ 'tester' ],
+            COBRAND_FEATURES => {
+               category_groups => { tester => 1 },
+               multiple_category_groups => { tester => $test->{enable_multi} },
+            }
+        }, sub {
+            my $processor = Open311::PopulateServiceList->new();
+            $processor->_current_body( $body );
+            $processor->process_services( $service_list );
+        };
+        my $contact_count = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->count();
+        is $contact_count, 2, 'correct number of contacts';
+
+        my $contact = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id, email => 100 } )->first;
+        is_deeply $contact->get_extra->{group}, $test->{groups}, "Multi groups set correctly";
+        if ($test->{unchanged}) {
+            is $contact->whenedited, $last_update->{100}, "contact unchanged";
+        }
+        $last_update->{100} = $contact->whenedited;
+
+        $contact = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id, email => '002'} )->first;
+        is $contact->get_extra->{group}, 'street', "Single groups set correctly";
+        if ($test->{unchanged}) {
+            is $contact->whenedited, $last_update->{002}, "contact unchanged";
+        }
+        $last_update->{002} = $contact->whenedited;
+    };
+}
+
+subtest "set multiple groups with quoted csv" => sub {
+    FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->delete();
+
+    my $services_xml = '<?xml version="1.0" encoding="utf-8"?>
+    <services>
+      <service>
+        <service_code>100</service_code>
+        <service_name>Cans left out 24x7</service_name>
+        <description>Garbage or recycling cans that have been left out for more than 24 hours after collection. Violators will be cited.</description>
+        <metadata>false</metadata>
+        <type>realtime</type>
+        <keywords>lorem, ipsum, dolor</keywords>
+        <group>&quot;sanitation &amp; cleaning&quot;,street</group>
+      </service>
+    </services>
+    ';
+
+    my $service_list = get_xml_simple_object($services_xml);
+
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'tester' ],
+        COBRAND_FEATURES => {
+           category_groups => { tester => 1 },
+           multiple_category_groups => { tester => 1 },
+        }
+    }, sub {
+        my $processor = Open311::PopulateServiceList->new();
+        $processor->_current_body( $body );
+        $processor->process_services( $service_list );
+    };
+    my $contact_count = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->count();
+    is $contact_count, 1, 'correct number of contacts';
+
+    my $contact = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id, email => 100 } )->first;
+    is_deeply $contact->get_extra->{group}, ['sanitation & cleaning','street'], "groups set correctly";
+};
+
+subtest "set multiple groups with bad csv" => sub {
+    FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->delete();
+
+    my $services_xml = '<?xml version="1.0" encoding="utf-8"?>
+    <services>
+      <service>
+        <service_code>100</service_code>
+        <service_name>Cans left out 24x7</service_name>
+        <description>Garbage or recycling cans that have been left out for more than 24 hours after collection. Violators will be cited.</description>
+        <metadata>false</metadata>
+        <type>realtime</type>
+        <keywords>lorem, ipsum, dolor</keywords>
+        <group>"sanitation,street</group>
+      </service>
+    </services>
+    ';
+
+    my $service_list = get_xml_simple_object($services_xml);
+
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'tester' ],
+        COBRAND_FEATURES => {
+           category_groups => { tester => 1 },
+           multiple_category_groups => { tester => 1 },
+        }
+    }, sub {
+        my $processor = Open311::PopulateServiceList->new();
+        $processor->_current_body( $body );
+        warning_like {
+            $processor->process_services( $service_list );
+        } qr/error parsing groups for testercontact Cans left out 24x7: "sanitation,street/,
+        "warning printed for bad csv";
+    };
+    my $contact_count = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->count();
+    is $contact_count, 1, 'correct number of contacts';
+
+    my $contact = FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id, email => 100 } )->first;
+    is_deeply $contact->get_extra->{group}, ['"sanitation,street'], "groups set correctly";
+};
+
 subtest 'check non open311 contacts marked as deleted' => sub {
     FixMyStreet::DB->resultset('Contact')->search( { body_id => $body->id } )->delete();