Merge 0fcd606 into b002b20

mysociety · Jun 6, 2022 · 6c7a9af · 6c7a9af
2 parents b002b20 + 0fcd606
commit 6c7a9af
Show file tree

Hide file tree

Showing 12 changed files with 275 additions and 31 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
         - Disable staff phone and name fields to avoid accidental overwriting.
         - Hide 'Assigned to' text if a report is not assigned to anyone
         - Hide 'Assign to' dropdown if no available assignees
+        - Allow 'Asset ID' (part of optional extra data displayed for a report) to be customisable for all cobrands
     - Bugfixes:
         - Add ID attributes to change password form inputs.
         - Fix link deactivation for privacy policy link on privacy policy page. #3704

diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -370,6 +370,29 @@ sub visible_states_remove {
     }
 }
 
+sub public_asset_id {
+    my $self = shift;
+
+    my $current_cobrand = $self->result_source->schema->cobrand;
+
+    my $cobrand_for_problem = $current_cobrand->call_hook(
+        get_body_handler_for_problem => $self );
+
+    return unless $cobrand_for_problem;
+
+    # Should be of the form:
+    # COBRAND_FEATURES:
+    #     public_asset_ids:
+    #         oxfordshire: ['feature_id', 'unit_number']
+    my $asset_id_labels = $cobrand_for_problem->feature('public_asset_ids');
+
+    # Return the first match
+    for my $label (@$asset_id_labels) {
+        my $asset_id = $self->get_extra_field_value($label);
+        return $asset_id if $asset_id;
+    }
+}
+
 around service => sub {
     my ( $orig, $self ) = ( shift, shift );
     # service might be undef if e.g. unsaved code report

diff --git a/t/cobrand/oxfordshire.t b/t/cobrand/oxfordshire.t
@@ -166,6 +166,10 @@ my @problems = FixMyStreet::DB->resultset('Problem')->search({}, { rows => 3, or
 
 FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'oxfordshire', 'fixmystreet' ],
+    COBRAND_FEATURES => {
+        public_asset_ids =>
+            { oxfordshire => [ 'feature_id', 'unit_number' ] },
+    },
     MAPIT_URL => 'http://mapit.uk/',
 }, sub {
 

diff --git a/t/cobrand/public_asset_ids.t b/t/cobrand/public_asset_ids.t
@@ -0,0 +1,225 @@
+use FixMyStreet::TestMech;
+
+my $mech = FixMyStreet::TestMech->new;
+
+# Test with an arbitrary cobrand
+my $pbro_body = $mech->create_body_ok( 2566, 'Peterborough City Council' );
+
+my %problem_params = (
+    latitude  => 52.5608,
+    longitude => 0.2405,
+    cobrand   => 'peterborough',
+);
+
+my ($problem_pbro_tree)
+    = $mech->create_problems_for_body( 1, $pbro_body->id,
+    'Tree problem', \%problem_params, );
+$problem_pbro_tree->external_id('0123');
+$problem_pbro_tree->set_extra_metadata( customer_reference => 'TREE123' );
+$problem_pbro_tree->update_extra_field(
+    { name => 'tree_code', value => 101 } );
+$problem_pbro_tree->whensent( $problem_pbro_tree->confirmed );
+$problem_pbro_tree->update;
+
+my ($problem_pbro_bin)
+    = $mech->create_problems_for_body( 1, $pbro_body->id,
+    'Bin problem', \%problem_params, );
+$problem_pbro_bin->external_id('0234');
+$problem_pbro_bin->set_extra_metadata( customer_reference => 'BIN234' );
+$problem_pbro_bin->update_extra_field(
+    { name => 'central_asset_id', value => 201 }, );
+$problem_pbro_bin->whensent( $problem_pbro_bin->confirmed );
+$problem_pbro_bin->update;
+
+my ($problem_pbro_other)
+    = $mech->create_problems_for_body( 1, $pbro_body->id, 'Other problem',
+    \%problem_params, );
+$problem_pbro_other->external_id('0345');
+$problem_pbro_other->set_extra_metadata( customer_reference => 'OTH345' );
+$problem_pbro_other->update_extra_field( { name => 'other_id', value => 301 },
+);
+$problem_pbro_other->whensent( $problem_pbro_other->confirmed );
+$problem_pbro_other->update;
+
+subtest 'Single public asset ID defined' => sub {
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'peterborough', 'fixmystreet' ],
+        COBRAND_FEATURES =>
+            { public_asset_ids => { peterborough => ['tree_code'] } },
+        MAPIT_URL => 'http://mapit.uk/',
+        },
+        sub {
+        # Reports should display the same info on both cobrands
+        for my $host ( 'peterborough.fixmystreet.com', 'www.fixmystreet.com' )
+        {
+            subtest "$host handles external IDs/refs correctly" => sub {
+                ok $mech->host($host);
+
+                note 'tree_code:';
+                $mech->get_ok( '/report/' . $problem_pbro_tree->id );
+                $mech->content_lacks( $problem_pbro_tree->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> TREE123',
+                    'Council reference not shown',
+                );
+                $mech->content_contains( 'Asset ID:</strong> 101',
+                    'Asset ID is shown' );
+
+                note 'central_asset_id:';
+                $mech->get_ok( '/report/' . $problem_pbro_bin->id );
+                $mech->content_lacks( $problem_pbro_bin->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> BIN234',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 201',
+                    'Asset ID not shown' );
+
+                note 'no code:';
+                $mech->get_ok( '/report/' . $problem_pbro_other->id );
+                $mech->content_lacks( $problem_pbro_other->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> OTH345',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 301',
+                    'Asset ID not shown' );
+            };
+        }
+        };
+};
+
+subtest 'Multiple public asset IDs defined' => sub {
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'peterborough', 'fixmystreet' ],
+        COBRAND_FEATURES => {
+            public_asset_ids =>
+                { peterborough => [ 'tree_code', 'central_asset_id' ] },
+        },
+        MAPIT_URL => 'http://mapit.uk/',
+        },
+        sub {
+        # Reports should display the same info on both cobrands
+        for my $host ( 'peterborough.fixmystreet.com', 'www.fixmystreet.com' )
+        {
+            subtest "$host handles external IDs/refs correctly" => sub {
+                ok $mech->host($host);
+
+                note 'tree_code:';
+                $mech->get_ok( '/report/' . $problem_pbro_tree->id );
+                $mech->content_lacks( $problem_pbro_tree->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> TREE123',
+                    'Council reference not shown',
+                );
+                $mech->content_contains( 'Asset ID:</strong> 101',
+                    'Asset ID is shown' );
+
+                note 'central_asset_id:';
+                $mech->get_ok( '/report/' . $problem_pbro_bin->id );
+                $mech->content_lacks( $problem_pbro_bin->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> BIN234',
+                    'Council reference not shown',
+                );
+                $mech->content_contains( 'Asset ID:</strong> 201',
+                    'Asset ID is shown' );
+
+                note 'no code:';
+                $mech->get_ok( '/report/' . $problem_pbro_other->id );
+                $mech->content_lacks( $problem_pbro_other->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> OTH345',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 301',
+                    'Asset ID not shown' );
+
+                # If a report has extra fields that match both
+                # public_asset_ids, we should choose the first match
+                my ($problem_pbro_conflict)
+                    = $mech->create_problems_for_body( 1, $pbro_body->id,
+                    'We have a problem',
+                    \%problem_params );
+                $problem_pbro_conflict->external_id('0987');
+                $problem_pbro_conflict->set_extra_metadata(
+                    customer_reference => 'CON987' );
+                $problem_pbro_conflict->update_extra_field(
+                    { name => 'tree_code', value => 101 } );
+                $problem_pbro_conflict->update_extra_field(
+                    { name => 'central_asset_id', value => 201 } );
+                $problem_pbro_conflict->whensent(
+                    $problem_pbro_conflict->confirmed );
+                $problem_pbro_conflict->update;
+
+                note 'conflicting codes:';
+                $mech->get_ok( '/report/' . $problem_pbro_conflict->id );
+                $mech->content_lacks( $problem_pbro_conflict->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> CON987',
+                    'Council reference not shown',
+                );
+                $mech->content_contains( 'Asset ID:</strong> 101',
+                    'First matching asset ID is shown' );
+            };
+        }
+        };
+};
+
+subtest 'public_asset_ids not defined' => sub {
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'peterborough', 'fixmystreet' ],
+        MAPIT_URL        => 'http://mapit.uk/',
+        },
+        sub {
+        # Reports should display the same info on both cobrands
+        for my $host ( 'peterborough.fixmystreet.com', 'www.fixmystreet.com' )
+        {
+            subtest "$host handles external IDs/refs correctly" => sub {
+                ok $mech->host($host);
+
+                note 'tree_code:';
+                $mech->get_ok( '/report/' . $problem_pbro_tree->id );
+                $mech->content_lacks( $problem_pbro_tree->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> TREE123',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 101',
+                    'Asset ID not shown' );
+
+                note 'central_asset_id:';
+                $mech->get_ok( '/report/' . $problem_pbro_bin->id );
+                $mech->content_lacks( $problem_pbro_bin->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> BIN234',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 201',
+                    'Asset ID not shown' );
+
+                note 'no code:';
+                $mech->get_ok( '/report/' . $problem_pbro_other->id );
+                $mech->content_lacks( $problem_pbro_other->external_id,
+                    'External ID not shown' );
+                $mech->content_lacks(
+                    'Council ref:</strong> OTH345',
+                    'Council reference not shown',
+                );
+                $mech->content_lacks( 'Asset ID:</strong> 301',
+                    'Asset ID not shown' );
+            };
+        }
+        };
+};
+
+done_testing;
diff --git a/templates/web/base/report/_cobrand_ref.html b/templates/web/base/report/_cobrand_ref.html
@@ -0,0 +1,5 @@
+[% asset_id = problem.public_asset_id; IF asset_id %]
+  <div class="council_info_box">
+      <p><strong>Asset ID:</strong> [% asset_id %]</p>
+  </div>
+[% END %]
diff --git a/templates/web/base/report/_main.html b/templates/web/base/report/_main.html
@@ -148,5 +148,5 @@ <h3>[% tprintf(loc('Shortlisted by %s'), problem.shortlisted_user.name) %]</h3>
     </div>
   [% END %]
 
-  [% TRY %][% PROCESS 'report/_main_after.html' %][% CATCH file %][% END %]
+  [% PROCESS 'report/_main_after.html' %]
 </div>
diff --git a/templates/web/base/report/_main_after.html b/templates/web/base/report/_main_after.html
@@ -0,0 +1 @@
+[% INCLUDE 'report/_cobrand_ref.html' %]
diff --git a/...mystreet.com/report/_oxfordshire_ref.html → ....com/report/_cobrand_ref_oxfordshire.html b/...mystreet.com/report/_oxfordshire_ref.html → ....com/report/_cobrand_ref_oxfordshire.html
@@ -1,18 +1,16 @@
 [% reference = problem.get_extra_metadata('customer_reference') ~%]
-[% feature_id = problem.get_extra_field_value('feature_id') ~%]
-[% unit_number = problem.get_extra_field_value('unit_number') ~%]
 [% unit_type = problem.get_extra_field_value('unit_type') ~%]
 [% column_no = problem.get_extra_field_value('column_no') ~%]
 
 <div class="council_info_box">
     <p><strong>Council ref:</strong> [% reference OR problem.id %]</p>
-  [% IF feature_id OR unit_number %]
-    <p><strong>Asset ID:</strong> [% feature_id OR unit_number %]</p>
-  [% END %]
-  [% IF column_no %]
+    [% asset_id = problem.public_asset_id; IF asset_id %]
+        <p><strong>Asset ID:</strong> [% asset_id %]</p>
+    [% END %]
+    [% IF column_no %]
     <p><strong>Column number:</strong> [% column_no %]</p>
-  [% END %]
-  [% IF unit_type %]
+    [% END %]
+    [% IF unit_type %]
     <p><strong>Unit type:</strong> [% unit_type %]</p>
-  [% END %]
+    [% END %]
 </div>
diff --git a/templates/web/fixmystreet.com/report/_main_after.html b/templates/web/fixmystreet.com/report/_main_after.html
@@ -1,3 +1,7 @@
-[% IF problem.whensent AND problem.to_body_named('Oxfordshire') %]
-    [% INCLUDE 'report/_oxfordshire_ref.html' %]
+[% IF problem.to_body_named('Oxfordshire') %]
+  [% IF problem.whensent %]
+    [% INCLUDE 'report/_cobrand_ref_oxfordshire.html' %]
+  [% END %]
+[% ELSE %]
+  [% INCLUDE 'report/_cobrand_ref.html' %]
 [% END %]
diff --git a/templates/web/oxfordshire/report/_cobrand_ref.html b/templates/web/oxfordshire/report/_cobrand_ref.html
@@ -0,0 +1 @@
+../../fixmystreet.com/report/_cobrand_ref_oxfordshire.html
diff --git a/templates/web/oxfordshire/report/_council_sent_info.html b/templates/web/oxfordshire/report/_council_sent_info.html
diff --git a/templates/web/oxfordshire/report/_main_after.html b/templates/web/oxfordshire/report/_main_after.html
@@ -1,5 +1,5 @@
 [% IF problem.bodies_str %]
-    [% INCLUDE 'report/_council_sent_info.html' %]
+    [% INCLUDE 'report/_cobrand_ref.html' %]
 [% ELSE %]
     <div class="council_info_box">
         <p>[% loc('Not reported to council') %]</p>