Merge branch 'he-still-bit-buggy-grr'

mysociety · May 27, 2020 · 8a03006 · 8a03006
2 parents 7b7e00c + 9018344
commit 8a03006
Show file tree

Hide file tree

Showing 16 changed files with 39 additions and 30 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
     - Bugfixes:
         - Fix issue with dashboard report CSV export. #3026
         - bin/update-schema PostgreSQL 12 compatibility. #3043
+        - Make sure category shown in all its groups when reporting.
     - Admin improvements:
         - Display user name/email for contributed as reports. #2990
         - Interface for enabling anonymous reports for certain categories. #2989

diff --git a/bin/import_categories b/bin/import_categories
@@ -81,7 +81,7 @@ sub make_categories {
 
         if ($group) {
             my $groups = $child_cat->groups;
-            my %groups = map { $_ => 1} @$groups;
+            my %groups = map { $_ => 1 } grep { $_ } @$groups;
             $groups{$group} = 1;
             my @groups = keys %groups;
             $child_cat->set_extra_metadata(group => \@groups);

diff --git a/bin/tfl/import_categories b/bin/tfl/import_categories
@@ -65,7 +65,7 @@ for my $group (keys %$groups) {
         $child_cat->note($child_cat->in_storage ? 'Updated by import_categories' : 'Created by import_categories');
         say colored("WARNING", 'red') . " " . $child_cat->category . " already exists" if $child_cat->in_storage and $child_cat->category ne 'Other (TfL)';
         my $groups = $child_cat->groups;
-        my %groups = map { $_ => 1} @$groups;
+        my %groups = map { $_ => 1 } grep { $_ } @$groups;
         $groups{$group} = 1;
         my @groups = keys %groups;
         $child_cat->extra(undef) if $child_cat->in_storage;

diff --git a/perllib/FixMyStreet/App/Controller/Admin/Templates.pm b/perllib/FixMyStreet/App/Controller/Admin/Templates.pm
@@ -69,7 +69,7 @@ sub edit : Path : Args(2) {
         category => $_->category_display,
         active => $active_contacts{$_->id},
         email => $_->email,
-        group => $_->get_extra_metadata('group') // '',
+        group => $_->groups,
     } } @live_contacts;
     $c->stash->{contacts} = \@all_contacts;
     $c->forward('/report/stash_category_groups', [ \@all_contacts, 1 ]) if $c->cobrand->enable_category_groups;

diff --git a/perllib/FixMyStreet/App/Controller/Admin/Triage.pm b/perllib/FixMyStreet/App/Controller/Admin/Triage.pm
@@ -90,10 +90,8 @@ sub setup_categories : Private {
         delete $c->stash->{categories_hash};
         my %category_groups = ();
         for my $category (@{$c->stash->{end_options}}) {
-            my $group = $category->{group} // $category->get_extra_metadata('group') // [''];
-            # this could be an array ref or a string
-            my @groups = ref $group eq 'ARRAY' ? @$group : ($group);
-            push( @{$category_groups{$_}}, $category ) for @groups;
+            my $groups = $category->groups;
+            push( @{$category_groups{$_}}, $category ) for @$groups;
         }
         my @category_groups = ();
         for my $group ( grep { $_ ne _('Other') } sort keys %category_groups ) {

diff --git a/perllib/FixMyStreet/App/Controller/Admin/Users.pm b/perllib/FixMyStreet/App/Controller/Admin/Users.pm
@@ -401,7 +401,7 @@ sub edit : Chained('user') : PathPart('') : Args(0) {
             id => $_->id,
             category => $_->category,
             active => $active_contacts{$_->id},
-            group => $_->get_extra_metadata('group') // '',
+            group => $_->groups,
         } } @live_contacts;
         $c->stash->{contacts} = \@all_contacts;
         $c->forward('/report/stash_category_groups', [ \@all_contacts, 1 ]) if $c->cobrand->enable_category_groups;

diff --git a/perllib/FixMyStreet/App/Controller/Dashboard.pm b/perllib/FixMyStreet/App/Controller/Dashboard.pm
@@ -493,8 +493,8 @@ sub generate_csv : Private {
         }
 
         if ($asked_for{subcategory}) {
-            my $group = $obj->contact && $obj->contact->get_extra_metadata('group') || '';
-            $group = join(',', ref $group ? @$group : $group);
+            my $group = $obj->contact && $obj->contact->groups;
+            $group = join(',', @$group);
             if ($group) {
                 $hashref->{subcategory} = $obj->category;
                 $hashref->{category} = $group;

diff --git a/perllib/FixMyStreet/App/Controller/JSON.pm b/perllib/FixMyStreet/App/Controller/JSON.pm
@@ -7,7 +7,6 @@ BEGIN { extends 'Catalyst::Controller'; }
 use JSON::MaybeXS;
 use DateTime;
 use DateTime::Format::ISO8601;
-use List::MoreUtils 'uniq';
 use FixMyStreet::DateRange;
 
 =head1 NAME

diff --git a/perllib/FixMyStreet/App/Controller/My.pm b/perllib/FixMyStreet/App/Controller/My.pm
@@ -198,7 +198,15 @@ sub setup_page_data : Private {
         order_by => [ "$table.category" ],
     } )->all;
     $c->stash->{filter_categories} = \@categories;
-    $c->forward('/report/stash_category_groups', [ \@categories ]) if $c->cobrand->enable_category_groups;
+
+    if ($c->cobrand->enable_category_groups) {
+        my @contacts = map { {
+            category => $_->category,
+            category_display => $_->category_display,
+            group => [''],
+        } } @categories;
+        $c->forward('/report/stash_category_groups', [ \@contacts ]);
+    }
 
     my $pins = $c->stash->{pins};
     FixMyStreet::Map::display_map(

diff --git a/perllib/FixMyStreet/App/Controller/Report.pm b/perllib/FixMyStreet/App/Controller/Report.pm
@@ -703,9 +703,8 @@ sub stash_category_groups : Private {
 
     my %category_groups = ();
     for my $category (@$contacts) {
-        my $group = $category->{group} // $category->get_extra_metadata('group') // [''];
-        # this could be an array ref or a string
-        my @groups = ref $group eq 'ARRAY' ? @$group : ($group);
+        my $group = $category->{group} // $category->groups;
+        my @groups = @$group;
         if (scalar @groups > 1 && $combine_multiple) {
             @groups = sort @groups;
             $category->{group} = \@groups;

diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -6,8 +6,7 @@ BEGIN { extends 'Catalyst::Controller'; }
 
 use utf8;
 use Encode;
-use List::MoreUtils qw(uniq);
-use List::Util 'first';
+use List::Util qw(first uniq);
 use HTML::Entities;
 use Path::Class;
 use Utils;
@@ -759,14 +758,20 @@ sub setup_categories_and_bodies : Private {
             if !$c->stash->{unresponsive}{ALL} &&
                 ($contact->email =~ /^REFUSED$/i || $body_send_method eq 'Refused');
 
-        push @category_options, $contact unless $seen{$contact->category};
-        $seen{$contact->category} = $contact;
+        if (my $cat = $seen{$contact->category}) {
+            # Make sure the category is listed in all its groups, not just the first set
+            my @groups = uniq @{$cat->groups}, @{$contact->groups};
+            $cat->set_extra_metadata(group => \@groups);
+        } else {
+            push @category_options, $contact;
+            $seen{$contact->category} = $contact;
+        }
     }
 
     if (@category_options) {
         # If there's an Other category present, put it at the bottom
         @category_options = (
-            { category => _('-- Pick a category --'), category_display => _('-- Pick a category --'), group => '' },
+            { category => _('-- Pick a category --'), category_display => _('-- Pick a category --'), group => [''] },
             grep { $_->category ne _('Other') } @category_options );
         push @category_options, $seen{_('Other')} if $seen{_('Other')};
     }

diff --git a/perllib/FixMyStreet/Cobrand/Hounslow.pm b/perllib/FixMyStreet/Cobrand/Hounslow.pm
@@ -138,8 +138,8 @@ sub setup_general_enquiries_stash {
         }
       )->all;
   @contacts = grep {
-    my $group = $_->get_extra_metadata('group') || '';
-    $group eq 'Other' || $group eq 'General Enquiries';
+    my $groups = $_->groups;
+    grep { $_ eq 'Other' || $_ eq 'General Enquiries' } @$groups;
   } @contacts;
   $self->{c}->stash->{bodies} = \%bodies;
   $self->{c}->stash->{bodies_to_list} = \%bodies;

diff --git a/perllib/FixMyStreet/Cobrand/IsleOfWight.pm b/perllib/FixMyStreet/Cobrand/IsleOfWight.pm
@@ -176,10 +176,7 @@ sub expand_triage_cat_list {
 
     my %group_to_category;
     while ( my $cat = $all_cats->next ) {
-        next unless $cat->get_extra_metadata('group');
-        my $groups = $cat->get_extra_metadata('group');
-        $groups = ref $groups eq 'ARRAY' ? $groups : [ $groups ];
-        for my $group ( @$groups ) {
+        for my $group ( @{$cat->groups} ) {
             $group_to_category{$group} //= [];
             push @{ $group_to_category{$group} }, $cat->category;
         }

diff --git a/perllib/FixMyStreet/DB/Result/Contact.pm b/perllib/FixMyStreet/DB/Result/Contact.pm
@@ -113,9 +113,11 @@ sub category_display {
     $self->get_extra_metadata('display_name') || $self->translate_column('category');
 }
 
+# Returns an arrayref of groups this Contact is in; if it is
+# not in any group, returns an arrayref of the empty string.
 sub groups {
     my $self = shift;
-    my $groups = $self->get_extra_metadata('group') || [];
+    my $groups = $self->get_extra_metadata('group') || [''];
     $groups = [ $groups ] unless ref $groups eq 'ARRAY';
     return $groups;
 }

diff --git a/t/app/controller/report_new.t b/t/app/controller/report_new.t
@@ -641,10 +641,10 @@ subtest "category groups" => sub {
         }
     }, sub {
         $contact2->update( { extra => { group => ['Roads','Pavements'] } } );
-        $contact9->update( { extra => { group => 'Roads' } } );
+        $contact9->update( { extra => { group => 'Pavements' } } );
         $contact10->update( { extra => { group => 'Roads' } } );
         $mech->get_ok("/report/new?lat=$saved_lat&lon=$saved_lon");
-        $mech->content_like(qr{<optgroup label="Pavements">\s*<option value='Potholes'>Potholes</option></optgroup>});
+        $mech->content_like(qr{<optgroup label="Pavements">\s*<option value='Potholes'>Potholes</option>\s*<option value='Street lighting'>Street lighting</option></optgroup>});
         $mech->content_like(qr{<optgroup label="Roads">\s*<option value='Potholes'>Potholes</option>\s*<option value='Street lighting'>Street lighting</option></optgroup>});
     };
 };

diff --git a/templates/web/base/reports/_list-filters.html b/templates/web/base/reports/_list-filters.html
@@ -2,7 +2,7 @@
 
 [% BLOCK category_options %]
     [% FOR cat IN categories %]
-        <option value="[% cat.category %]"[% ' selected' IF filter_category.${cat.category} OR ( filter_group AND ( cat.get_extra_metadata('group').grep(filter_group).size OR cat.category == filter_group ) ) %]>
+        <option value="[% cat.category %]"[% ' selected' IF filter_category.${cat.category} OR ( filter_group AND ( cat.groups.grep(filter_group).size OR cat.category == filter_group ) ) %]>
             [% cat.category_display %]
             [%~ IF cat.get_extra_metadata('help_text') %] ([% cat.get_extra_metadata('help_text') %])[% END ~%]
         </option>