Add Extra role to ease use of {extra} field.

Historically, the extra field has been used in two different ways by
different cobrands, both as a list (e.g. Open311 category fields) and a
hash (e.g. the Zurich cobrand).

This commit consolidates usage, adding an API to make use of the field
easier and always returning a hash for the code to use. Fixes #1018.

10 files changed, 264 insertions, 65 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index b5a6745b2..b4daf0f55 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -636,8 +636,9 @@ sub setup_categories_and_bodies : Private {
             unless ( $seen{$contact->category} ) {
                 push @category_options, $contact->category;
 
-                $category_extras{ $contact->category } = $contact->extra
-                    if $contact->extra;
+                my $metas = $contact->get_extra_fields;
+                $category_extras{ $contact->category } = $metas
+                    if scalar @$metas;
 
                 $non_public_categories{ $contact->category } = 1 if $contact->non_public;
             }
@@ -870,8 +871,9 @@ sub process_report : Private {
             if $body_string && @{ $c->stash->{missing_details_bodies} };
         $report->bodies_str($body_string);
 
-        my @extra = ();
-        my $metas = $contacts[0]->extra;
+        my @extra;
+        # NB: we are only checking extras for the *first* retrieved contact.
+        my $metas = $contacts[0]->get_extra_fields();
 
         foreach my $field ( @$metas ) {
             if ( lc( $field->{required} ) eq 'true' ) {
@@ -894,7 +896,7 @@ sub process_report : Private {
 
         if ( @extra ) {
             $c->stash->{report_meta} = { map { $_->{name} => $_ } @extra };
-            $report->extra( \@extra );
+            $report->set_extra_fields( @extra );
         }
     } elsif ( @{ $c->stash->{bodies_to_list} } ) {
 
diff --git a/perllib/FixMyStreet/App/Controller/Tokens.pm b/perllib/FixMyStreet/App/Controller/Tokens.pm
index c8a7531d6..4c0afdeda 100644
--- a/perllib/FixMyStreet/App/Controller/Tokens.pm
+++ b/perllib/FixMyStreet/App/Controller/Tokens.pm
@@ -66,10 +66,8 @@ sub confirm_problem : Path('/P') {
     # For Zurich, email confirmation simply sets a flag, it does not change the
     # problem state, log in, or anything else
     if ($c->cobrand->moniker eq 'zurich') {
-        my $extra = { %{ $problem->extra || {} } };
-        $extra->{email_confirmed} = 1;
+        $problem->set_extra_metadata( email_confirmed => 1 );
         $problem->update( {
-            extra => $extra,
             confirmed => \'ms_current_timestamp()',
         } );
 
diff --git a/perllib/FixMyStreet/Cobrand/Harrogate.pm b/perllib/FixMyStreet/Cobrand/Harrogate.pm
index 4dc041c89..afda52385 100644
--- a/perllib/FixMyStreet/Cobrand/Harrogate.pm
+++ b/perllib/FixMyStreet/Cobrand/Harrogate.pm
@@ -112,7 +112,9 @@ sub temp_update_contacts {
         }
 
         $contact->update({
-            extra => [ { %default, %$field } ],
+            # XXX: we're just setting extra with the expected layout,
+            # this could be encapsulated more nicely
+            extra => { _fields => [ { %default, %$field } ] },
             confirmed => 1,
             deleted => 0,
             editor => 'automated script',
@@ -223,7 +225,7 @@ sub process_additional_metadata_for_email {
     my ($self, $problem, $h) = @_;
 
     my $additional = '';
-    if (my $extra = $problem->extra) {
+    if (my $extra = $problem->get_extra_fields) {
         $additional = join "\n\n", map {
             if ($_->{name} eq 'INFO_TEXT') {
                 ();
diff --git a/perllib/FixMyStreet/Cobrand/Zurich.pm b/perllib/FixMyStreet/Cobrand/Zurich.pm
index 074c21edb..8d7424328 100644
--- a/perllib/FixMyStreet/Cobrand/Zurich.pm
+++ b/perllib/FixMyStreet/Cobrand/Zurich.pm
@@ -4,6 +4,7 @@ use base 'FixMyStreet::Cobrand::Default';
 use DateTime;
 use POSIX qw(strcoll);
 use RABX;
+use Scalar::Util 'blessed';
 
 use strict;
 use warnings;
@@ -158,7 +159,8 @@ sub updates_as_hashref {
         $hashref->{update_pp} = $self->prettify_dt( $problem->lastupdate );
 
         if ( $problem->state eq 'fixed - council' ) {
-            $hashref->{details} = FixMyStreet::App::View::Web->add_links( $ctx, $problem->extra ? $problem->extra->{public_response} : '' );
+            $hashref->{details} = FixMyStreet::App::View::Web->add_links(
+                $ctx, $problem->get_extra_metadata('public_response') || '' );
         } elsif ( $problem->state eq 'closed' ) {
             $hashref->{details} = sprintf( _('Assigned to %s'), $problem->body($ctx)->name );
         }
@@ -169,13 +171,16 @@ sub updates_as_hashref {
 
 sub allow_photo_display {
     my ( $self, $r ) = @_;
-    if (ref($r) ne 'HASH') {
-        return $r->extra && $r->extra->{publish_photo};
+    if (blessed $r) {
+        return $r->get_extra_metadata( 'publish_photo' );
     }
+
+    # additional munging in case $r isn't an object, TODO see if we can remove this
     my $extra = $r->{extra};
     utf8::encode($extra) if utf8::is_utf8($extra);
     my $h = new IO::String($extra);
     $extra = RABX::wire_rd($h);
+    return unless ref $extra eq 'HASH';
     return $extra->{publish_photo};
 }
 
@@ -264,10 +269,9 @@ sub get_or_check_overdue {
     my ($self, $problem) = @_;
 
     # use the cached version is it exists (e.g. when called from template)
-    my $extra = $problem->extra;
-    if (exists $extra->{closed_overdue} and defined $extra->{closed_overdue}) {
-        return $extra->{closed_overdue}
-    }
+    my $overdue = $problem->get_extra_metadata('closed_overdue');
+    return $overdue if defined $overdue;
+
     return $self->overdue($problem);
 }
 
@@ -466,12 +470,18 @@ sub admin_report_edit {
 
     # Problem updates upon submission
     if ( ($type eq 'super' || $type eq 'dm') && $c->req->param('submit') ) {
-        # Predefine the hash so it's there for lookups
-        my $extra = $problem->extra || {};
-        $extra->{publish_photo} = $c->req->params->{publish_photo} || 0;
-        $extra->{third_personal} = $c->req->params->{third_personal} || 0;
+        $problem->set_extra_metadata('publish_photo' => $c->req->params->{publish_photo} || 0 );
+        $problem->set_extra_metadata('third_personal' => $c->req->params->{third_personal} || 0 );
+
         # Make sure we have a copy of the original detail field
-        $extra->{original_detail} = $problem->detail if !$extra->{original_detail} && $c->req->params->{detail} && $problem->detail ne $c->req->params->{detail};
+        if (my $new_detail = $c->req->params->{detail}) {
+            my $old_detail = $problem->detail;
+            if (! $problem->get_extra_metadata('original_detail')
+                && ($old_detail ne $new_detail))
+            {
+                $problem->set_extra_metadata( original_detail => $old_detail );
+            }
+        }
 
         # Some changes will be accompanied by an internal note, which if needed
         # should be stored in this variable.
@@ -487,20 +497,20 @@ sub admin_report_edit {
             $problem->external_body( undef );
             $problem->bodies_str( $cat->body_id );
             $problem->whensent( undef );
-            $extra->{changed_category} = 1;
+            $problem->set_extra_metadata(changed_category => 1);
             $internal_note_text = "Weitergeleitet von $old_cat an $new_cat";
             $redirect = 1 if $cat->body_id ne $body->id;
         } elsif ( my $subdiv = $c->req->params->{body_subdivision} ) {
-            $extra->{moderated_overdue} //= $self->overdue( $problem );
+            $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
             $self->set_problem_state($c, $problem, 'in progress');
             $problem->external_body( undef );
             $problem->bodies_str( $subdiv );
             $problem->whensent( undef );
             $redirect = 1;
         } elsif ( my $external = $c->req->params->{body_external} ) {
-            $extra->{moderated_overdue} //= $self->overdue( $problem );
+            $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
             $self->set_problem_state($c, $problem, 'closed');
-            $extra->{closed_overdue} //= $self->overdue( $problem );
+            $problem->set_extra_metadata_if_undefined( closed_overdue => $self->overdue( $problem ) );
             $problem->external_body( $external );
             $problem->whensent( undef );
             _admin_send_email( $c, 'problem-external.txt', $problem );
@@ -510,13 +520,13 @@ sub admin_report_edit {
 
                 if ($problem->state eq 'unconfirmed' and $state ne 'unconfirmed') {
                     # only set this for the first state change
-                    $extra->{moderated_overdue} //= $self->overdue( $problem );
+                    $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
                 }
 
                 $self->set_problem_state($c, $problem, $state);
 
                 if ($self->problem_is_closed($problem)) {
-                    $extra->{closed_overdue} //= $self->overdue( $problem );
+                    $problem->set_extra_metadata_if_undefined( closed_overdue => $self->overdue( $problem ) );
                 }
                 if ( $state eq 'hidden' && $c->req->params->{send_rejected_email} ) {
                     _admin_send_email( $c, 'problem-rejected.txt', $problem );
@@ -524,7 +534,6 @@ sub admin_report_edit {
             }
         }
 
-        $problem->extra( $extra );
         $problem->title( $c->req->param('title') );
         $problem->detail( $c->req->param('detail') );
         $problem->latitude( $c->req->param('latitude') );
@@ -532,12 +541,10 @@ sub admin_report_edit {
 
         # Final, public, Update from DM
         if (my $update = $c->req->param('status_update')) {
-            $extra->{public_response} = $update;
-            $problem->extra( $extra );
+            $problem->set_extra_metadata(public_response => $update);
             if ($c->req->params->{publish_response}) {
                 $self->set_problem_state($c, $problem, 'fixed - council');
-                $extra->{closed_overdue} = $self->overdue( $problem );
-                $problem->extra( { %$extra } );
+                $problem->set_extra_metadata( closed_overdue => $self->overdue( $problem ) );
                 _admin_send_email( $c, 'problem-closed.txt', $problem );
             }
         }
@@ -619,9 +626,7 @@ sub admin_report_edit {
 
             # If they clicked the no more updates button, we're done.
             if ($c->req->param('no_more_updates')) {
-                my $extra = $problem->extra || {};
-                $extra->{subdiv_overdue} = $self->overdue( $problem );
-                $problem->extra( $extra );
+                $problem->set_extra_metadata( subdiv_overdue => $self->overdue( $problem ) );
                 $problem->bodies_str( $body->parent->id );
                 $problem->whensent( undef );
                 $self->set_problem_state($c, $problem, 'planned');
@@ -645,7 +650,7 @@ sub admin_report_edit {
 sub _admin_send_email {
     my ( $c, $template, $problem ) = @_;
 
-    return unless $problem->extra && $problem->extra->{email_confirmed};
+    return unless $problem->get_extra_metadata('email_confirmed');
 
     my $to = $problem->name
         ? [ $problem->user->email, $problem->name ]
diff --git a/perllib/FixMyStreet/DB/Result/Contact.pm b/perllib/FixMyStreet/DB/Result/Contact.pm
index eca028c9b..2fbb0716d 100644
--- a/perllib/FixMyStreet/DB/Result/Contact.pm
+++ b/perllib/FixMyStreet/DB/Result/Contact.pm
@@ -63,4 +63,12 @@ __PACKAGE__->belongs_to(
 __PACKAGE__->load_components("+FixMyStreet::DB::RABXColumn");
 __PACKAGE__->rabx_column('extra');
 
+use Moose;
+use namespace::clean -except => [ 'meta' ];
+
+with 'FixMyStreet::Roles::Extra';
+
+# we need the inline_constructor bit as we don't inherit from Moose
+__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
+
 1;
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 3c620ba84..93dc16811 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -158,7 +158,8 @@ use Moose;
 use namespace::clean -except => [ 'meta' ];
 use Utils;
 
-with 'FixMyStreet::Roles::Abuser';
+with 'FixMyStreet::Roles::Abuser',
+     'FixMyStreet::Roles::Extra';
 
 =head2
 
diff --git a/perllib/FixMyStreet/Roles/Extra.pm b/perllib/FixMyStreet/Roles/Extra.pm
new file mode 100644
index 000000000..f815a3e9a
--- /dev/null
+++ b/perllib/FixMyStreet/Roles/Extra.pm
@@ -0,0 +1,193 @@
+package FixMyStreet::Roles::Extra;
+use Moose::Role;
+
+=head1 NAME
+
+FixMyStreet::Roles::Extra - role for accessing {extra} field
+
+=head1 SYNOPSIS
+
+This is to applied to a DB class like Problem or Contacts that has a rich {extra} field:
+
+    use Moose;
+    with 'FixMyStreet::Roles::Extra';
+
+(NB: there is actually a little more boilerplate, because DBIC doesn't actually
+inherit from Moose, see ::Problem for an example.)
+
+Then:
+
+    $contact->set_extra_fields(
+        { name => 'pothole_size', ... },
+        { name => 'pothole_shape, ... } );
+    my $metas = $contact->get_extra_fields();
+
+And
+
+    # e.g. for sites like Zurich (but handled gracefully otherwise)
+    $problem->set_extra_metadata( overdue => 1 );
+    if ($problem->get_extra_metadata( 'overdue')) { ... }
+
+=head1 METHODS
+
+=head2 set_extra_metadata
+
+    $problem->set_extra_metadata( overdue => 1);
+
+=cut
+
+sub set_extra_metadata {
+    my ($self,  $key, $value) = @_;
+    my $extra = $self->get_extra();
+
+    $self->extra({ %$extra, $key => $value });
+};
+
+=head2 set_extra_metadata_if_undefined
+
+    $problem->set_extra_metadata_if_undefined( overdue => 1);
+    # as above, but won't set if, for example 'overdue' is already set to 0
+
+=cut
+
+sub set_extra_metadata_if_undefined {
+    my ($self, $key, $value) = @_;
+    my $extra = $self->get_extra();
+
+    return if defined $extra->{$key};
+    $self->extra({ %$extra, $key => $value });
+};
+
+=head2 unset_extra_metadata
+
+    $contact->unset_extra_metadata('photo_required');
+
+=cut
+
+sub unset_extra_metadata {
+    my ($self, $key) = @_;
+    my $extra = $self->get_extra();
+
+    return 1 unless exists $extra->{$key};
+    delete $extra->{$key};
+    $self->extra($extra);
+};
+
+=head2 get_extra_metadata
+
+    my $overdue = $problem->get_extra_metadata('overdue');
+
+=cut
+
+sub get_extra_metadata {
+    my ($self, $key) = @_;
+    my $extra = $self->get_extra();
+
+    return $extra->{$key};
+};
+
+=head2 get_extra_metadata_as_hashref
+
+    my $hashref = $contact->get_extra_metadata_as_hashref();
+
+=cut
+
+my $META_FIELD = '_fields';
+
+sub get_extra_metadata_as_hashref {
+    my ($self) = @_;
+    my $extra = $self->get_extra();
+
+    my %extra = %$extra;
+    delete $extra{$META_FIELD};
+    return \%extra;
+}
+
+=head2 get_extra_fields
+
+    my $metas = $problem->get_extra_fields();
+
+=cut
+
+sub get_extra_fields {
+    my ($self) = @_;
+    my $extra = $self->get_extra();
+
+    return $extra->{$META_FIELD} ||= do {
+        my $metas = [];
+        $self->extra({ %$extra, $META_FIELD => $metas });
+        $metas;
+    };
+}
+
+=head2 set_extra_fields
+
+    $problem->set_extra_fields( { ... }, { ... } );
+
+=cut
+
+sub set_extra_fields {
+    my ($self, @fields) = @_;
+    my $extra = $self->get_extra();
+
+    $self->extra({ %$extra, $META_FIELD => \@fields });
+}
+
+=head2 push_extra_fields
+
+    $problem->push_extra_fields( { ... } );
+
+like set_extra_fields, but pushes the new fields onto the end of the existing list.
+
+=cut
+
+sub push_extra_fields {
+    my ($self, @fields) = @_;
+    my $extra = $self->get_extra();
+
+    my $existing = $self->get_extra_fields;
+
+    $self->extra({ %$extra, $META_FIELD => [ @$existing, @fields ] });
+}
+
+=head1 HELPER METHODS
+
+For internal use mostly.
+
+=head2 dirty_extra
+
+Set the extra field as dirty.  (e.g. signalling that the DB object should be
+updated).
+
+=cut
+
+sub dirty_extra {
+    my $self = shift;
+    $self->make_column_dirty('extra');
+    return 1;
+}
+
+=head2 get_extra
+
+Get the extra data.  If this is not set, then returns a {}
+
+=cut
+
+sub get_extra {
+    my ($self) = @_;
+    my $extra = $self->extra or do {
+        my $extra = {};
+        $self->extra({});
+        return $extra;
+    };
+
+    if (ref $extra eq 'ARRAY') {
+        # upgrade layout transparently
+        $extra = { $META_FIELD => $extra };
+        $self->extra($extra);
+    }
+
+    return $extra;
+}
+
+1;
diff --git a/perllib/FixMyStreet/SendReport/Open311.pm b/perllib/FixMyStreet/SendReport/Open311.pm
index c064eeef5..d65a8ddb1 100644
--- a/perllib/FixMyStreet/SendReport/Open311.pm
+++ b/perllib/FixMyStreet/SendReport/Open311.pm
@@ -37,7 +37,7 @@ sub send {
 
             $revert = 1;
 
-            my $extra = $row->extra;
+            my $extra = $row->get_extra_fields();
             if ( $row->used_map || ( !$row->used_map && !$row->postcode ) ) {
                 push @$extra, { name => 'northing', value => $h->{northing} };
                 push @$extra, { name => 'easting', value => $h->{easting} };
@@ -49,33 +49,31 @@ sub send {
             push @$extra, { name => 'email_alerts_requested', value => 'FALSE' }; # always false as can never request them
             push @$extra, { name => 'requested_datetime', value => DateTime::Format::W3CDTF->format_datetime($row->confirmed->set_nanosecond(0)) };
             push @$extra, { name => 'email', value => $row->user->email };
-            $row->extra( $extra );
-
-            $always_send_latlong = 0;
-            $send_notpinpointed = 1;
-            $use_service_as_deviceid = 0;
-
             # make sure we have last_name attribute present in row's extra, so
             # it is passed correctly to Bromley as attribute[]
             if ( $row->cobrand ne 'bromley' ) {
                 my ( $firstname, $lastname ) = ( $row->name =~ /(\w+)\.?\s+(.+)/ );
                 push @$extra, { name => 'last_name', value => $lastname };
             }
+            $row->set_extra_fields( @$extra );
 
+            $always_send_latlong = 0;
+            $send_notpinpointed = 1;
+            $use_service_as_deviceid = 0;
             $extended_desc = 0;
         }
 
         # extra Oxfordshire fields: send nearest street, postcode, northing and easting, and the FMS id
         if ( $row->bodies_str =~ /\b(?:$COUNCIL_ID_OXFORDSHIRE|$COUNCIL_ID_WARWICKSHIRE)\b/ ) {
 
-            my $extra = $row->extra;
+            my $extra = $row->get_extra_fields;
             push @$extra, { name => 'external_id', value => $row->id };
             push @$extra, { name => 'closest_address', value => $h->{closest_address} } if $h->{closest_address};
             if ( $row->used_map || ( !$row->used_map && !$row->postcode ) ) {
                 push @$extra, { name => 'northing', value => $h->{northing} };
                 push @$extra, { name => 'easting', value => $h->{easting} };
             }
-            $row->extra( $extra );
+            $row->set_extra_fields( @$extra );
 
             if ($row->bodies_str =~ /$COUNCIL_ID_OXFORDSHIRE/) {
                 $extended_desc = 'oxfordshire';
@@ -131,7 +129,7 @@ sub send {
 
         if ($row->cobrand eq 'fixmybarangay') {
             # FixMyBarangay endpoints expect external_id as an attribute, as do Oxfordshire
-            $row->extra( [ { 'name' => 'external_id', 'value' => $row->id  } ]  );
+            $row->set_extra_fields( { 'name' => 'external_id', 'value' => $row->id  } );
             $revert = 1;
         }
 
diff --git a/perllib/Open311.pm b/perllib/Open311.pm
index c02618725..58ae96bc2 100644
--- a/perllib/Open311.pm
+++ b/perllib/Open311.pm
@@ -150,19 +150,15 @@ sub _populate_service_request_params {
         $params->{deviceid} = $problem->service;
     }
 
-    if ( $problem->extra ) {
-        my $extras = $problem->extra;
-
-        for my $attr ( @$extras ) {
-            my $attr_name = $attr->{name};
-            if ( $attr_name eq 'first_name' || $attr_name eq 'last_name' ) {
-                $params->{$attr_name} = $attr->{value} if $attr->{value};
-                next if $attr_name eq 'first_name';
-            }
-            $attr_name =~ s/fms_extra_//;
-            my $name = sprintf( 'attribute[%s]', $attr_name );
-            $params->{ $name } = $attr->{value};
+    for my $attr ( @{$problem->get_extra_fields} ) {
+        my $attr_name = $attr->{name};
+        if ( $attr_name eq 'first_name' || $attr_name eq 'last_name' ) {
+            $params->{$attr_name} = $attr->{value} if $attr->{value};
+            next if $attr_name eq 'first_name';
         }
+        $attr_name =~ s/fms_extra_//;
+        my $name = sprintf( 'attribute[%s]', $attr_name );
+        $params->{ $name } = $attr->{value};
     }
 
     return $params;
diff --git a/perllib/Open311/PopulateServiceList.pm b/perllib/Open311/PopulateServiceList.pm
index 0f6e32893..e4f0b8357 100644
--- a/perllib/Open311/PopulateServiceList.pm
+++ b/perllib/Open311/PopulateServiceList.pm
@@ -265,11 +265,7 @@ sub _add_meta_to_contact {
         @meta = grep { ! $ignore{ $_->{ code } } } @meta;
     }
 
-    if ( @meta ) {
-        $contact->extra( \@meta );
-    } else {
-        $contact->extra( undef );
-    }
+    $contact->set_extra_fields(@meta);
     $contact->update;
 }