Add support for multiple photos per report.

For Zurich, see mysociety/FixMyStreet-Commercial#664.

This commit includes a new PhotoSet class (NB: called Model:: though not
really a model), should handle binary data (e.g. old style photos in
database), fileids (40-char hash), and Catalyst::Upload objects.

6 files changed, 358 insertions, 141 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index f9ea383f8..d7bca05a7 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -10,6 +10,7 @@ use Digest::SHA qw(sha1_hex);
 use mySociety::EmailUtil qw(is_valid_email);
 use if !$ENV{TRAVIS}, 'Image::Magick';
 use DateTime::Format::Strptime;
+use List::Util 'first';
 
 
 use FixMyStreet::SendReport;
@@ -672,8 +673,8 @@ sub report_edit : Path('report_edit') : Args(1) {
         );
     }
 
-    if ( $c->get_param('rotate_photo') ) {
-        $c->forward('rotate_photo');
+    if (my $rotate_photo_param = $self->_get_rotate_photo_param($c)) {
+        $self->rotate_photo($c,  @$rotate_photo_param);
         return 1;
     }
 
@@ -1370,36 +1371,28 @@ Rotate a photo 90 degrees left or right
 
 =cut
 
+# returns index of photo to rotate, if any
+sub _get_rotate_photo_param {
+    my ($self, $c) = @_;
+    my $params = $c->req->parameters;
+    my $key = first { /^rotate_photo/ } $c->req->param or return;
+    my ($index) = $key =~ /(\d+)$/;
+    my $direction = $c->req->param($key);
+    return [ $index || 0, $key, $direction ];
+}
+
 sub rotate_photo : Private {
-    my ( $self, $c ) =@_;
+    my ( $self, $c, $index, $key, $direction ) = @_;
 
-    my $direction = $c->get_param('rotate_photo');
     return unless $direction eq _('Rotate Left') or $direction eq _('Rotate Right');
 
-    my $photo = $c->stash->{problem}->photo;
-    my $file;
-
-    # If photo field contains a hash
-    if ( length($photo) == 40 ) {
-        $file = file( $c->config->{UPLOAD_DIR}, "$photo.jpeg" );
-        $photo = $file->slurp;
-    }
-
-    $photo = _rotate_image( $photo, $direction eq _('Rotate Left') ? -90 : 90 );
-    return unless $photo;
+    my $problem = $c->stash->{problem};
+    my $fileid = $problem->get_photoset($c)->rotate_image(
+        $index,
+        $direction eq _('Rotate Left') ? -90 : 90
+    ) or return;
 
-    # Write out to new location
-    my $fileid = sha1_hex($photo);
-    $file = file( $c->config->{UPLOAD_DIR}, "$fileid.jpeg" );
-
-    my $fh = $file->open('w');
-    print $fh $photo;
-    close $fh;
-
-    unlink glob FixMyStreet->path_to( 'web', 'photo', $c->stash->{problem}->id . '.*' );
-
-    $c->stash->{problem}->photo( $fileid );
-    $c->stash->{problem}->update();
+    $problem->update({ photo => $fileid });
 
     return 1;
 }
@@ -1449,18 +1442,6 @@ sub trim {
     return $e;
 }
 
-sub _rotate_image {
-    my ($photo, $direction) = @_;
-    my $image = Image::Magick->new;
-    $image->BlobToImage($photo);
-    my $err = $image->Rotate($direction);
-    return 0 if $err;
-    my @blobs = $image->ImageToBlob();
-    undef $image;
-    return $blobs[0];
-}
-
-
 =head1 AUTHOR
 
 Struan Donald
diff --git a/perllib/FixMyStreet/App/Controller/Photo.pm b/perllib/FixMyStreet/App/Controller/Photo.pm
index a2ec7d4c8..7cf78cb2f 100644
--- a/perllib/FixMyStreet/App/Controller/Photo.pm
+++ b/perllib/FixMyStreet/App/Controller/Photo.pm
@@ -8,8 +8,8 @@ use DateTime::Format::HTTP;
 use Digest::SHA qw(sha1_hex);
 use File::Path;
 use File::Slurp;
-use Image::Size;
 use Path::Class;
+use FixMyStreet::App::Model::PhotoSet;
 use if !$ENV{TRAVIS}, 'Image::Magick';
 
 =head1 NAME
@@ -49,13 +49,13 @@ sub during :LocalRegex('^([0-9a-f]{40})\.(temp|fulltemp)\.jpeg$') {
     $c->forward( 'output', [ $photo ] );
 }
 
-sub index :LocalRegex('^(c/)?(\d+)(?:\.(full|tn|fp))?\.jpeg$') {
+sub index :LocalRegex('^(c/)?(\d+)(?:\.(\d+))?(?:\.(full|tn|fp))?\.jpeg$') {
     my ( $self, $c ) = @_;
-    my ( $is_update, $id, $size ) = @{ $c->req->captures };
+    my ( $is_update, $id, $photo_number, $size ) = @{ $c->req->captures };
 
-    my @photo;
+    my $item;
     if ( $is_update ) {
-        @photo = $c->model('DB::Comment')->search( {
+        ($item) = $c->model('DB::Comment')->search( {
             id => $id,
             state => 'confirmed',
             photo => { '!=', undef },
@@ -67,35 +67,21 @@ sub index :LocalRegex('^(c/)?(\d+)(?:\.(full|tn|fp))?\.jpeg$') {
         }
 
         $c->detach( 'no_photo' ) if $id =~ /\D/;
-        @photo = $c->cobrand->problems->search( {
+        ($item) = $c->cobrand->problems->search( {
             id => $id,
             state => [ FixMyStreet::DB::Result::Problem->visible_states(), 'partial' ],
             photo => { '!=', undef },
         } );
     }
 
-    $c->detach( 'no_photo' ) unless @photo;
+    $c->detach( 'no_photo' ) unless $item;
 
-    my $item = $photo[0];
     $c->detach( 'no_photo' ) unless $c->cobrand->allow_photo_display($item); # Should only be for reports, not updates
-    my $photo = $item->photo;
 
-    # If photo field contains a hash
-    if (length($photo) == 40) {
-        my $file = file( $c->config->{UPLOAD_DIR}, "$photo.jpeg" );
-        $photo = $file->slurp;
-    }
-
-    if ( $size eq 'tn' ) {
-        $photo = _shrink( $photo, 'x100' );
-    } elsif ( $size eq 'fp' ) {
-        $photo = _crop( $photo );
-    } elsif ( $size eq 'full' ) {
-    } elsif ( $c->cobrand->default_photo_resize ) {
-        $photo = _shrink( $photo, $c->cobrand->default_photo_resize );
-    } else {
-        $photo = _shrink( $photo, '250x250' );
-    }
+    my $photo = $item->get_photoset( $c )
+        ->get_image_data( num => $photo_number, size => $size )
+        
+    or $c->detach( 'no_photo' );
 
     $c->forward( 'output', [ $photo ] );
 }
@@ -156,84 +142,28 @@ sub process_photo : Private {
     my ( $self, $c ) = @_;
 
     return
-         $c->forward('process_photo_upload')
-      || $c->forward('process_photo_cache')
+         $c->forward('process_photo_upload_or_cache')
       || 1;    # always return true
 }
 
-sub process_photo_upload : Private {
+sub process_photo_upload_or_cache : Private {
     my ( $self, $c ) = @_;
-
-    # check for upload or return
-    my $upload = $c->req->upload('photo')
-      || return;
-
-    # check that the photo is a jpeg
-    my $ct = $upload->type;
-    $ct =~ s/x-citrix-//; # Thanks, Citrix
-    # Had a report of a JPEG from an Android 2.1 coming through as a byte stream
-    unless ( $ct eq 'image/jpeg' || $ct eq 'image/pjpeg' || $ct eq 'application/octet-stream' ) {
-        $c->log->info('Bad photo tried to upload, type=' . $ct);
-        $c->stash->{photo_error} = _('Please upload a JPEG image only');
-        return;
-    }
-
-    # get the photo into a variable
-    my $photo_blob = eval {
-        my $filename = $upload->tempname;
-        my $out = `jhead -se -autorot $filename 2>&1`;
-        unless (defined $out) {
-            my ($w, $h, $err) = Image::Size::imgsize($filename);
-            die _("Please upload a JPEG image only") . "\n" if !defined $w || $err ne 'JPG';
-        }
-        die _("Please upload a JPEG image only") . "\n" if $out && $out =~ /Not JPEG:/;
-        my $photo = $upload->slurp;
-        return $photo;
-    };
-    if ( my $error = $@ ) {
-        my $format = _(
-"That image doesn't appear to have uploaded correctly (%s), please try again."
-        );
-        $c->stash->{photo_error} = sprintf( $format, $error );
-        return;
-    }
-
-    # we have an image we can use - save it to the upload dir for storage
-    my $cache_dir = dir( $c->config->{UPLOAD_DIR} );
-    $cache_dir->mkpath;
-    unless ( -d $cache_dir && -w $cache_dir ) {
-        warn "Can't find/write to photo cache directory '$cache_dir'";
-        return;
-    }
-
-    my $fileid = sha1_hex($photo_blob);
-    $upload->copy_to( file($cache_dir, $fileid . '.jpeg') );
-
-    # stick the hash on the stash, so don't have to reupload in case of error
-    $c->stash->{upload_fileid} = $fileid;
-
-    return 1;
-}
-
-=head2 process_photo_cache
-
-Look for the upload_fileid parameter and check it matches a file on disk. If it
-does return true and put fileid on stash, otherwise false.
-
-=cut
-
-sub process_photo_cache : Private {
-    my ( $self, $c ) = @_;
-
-    # get the fileid and make sure it is just a hex number
-    my $fileid = $c->get_param('upload_fileid') || '';
-    $fileid =~ s{[^0-9a-f]}{}gi;
-    return unless $fileid;
-
-    my $file = file( $c->config->{UPLOAD_DIR}, "$fileid.jpeg" );
-    return unless -e $file;
-
-    $c->stash->{upload_fileid} = $fileid;
+    my @items = (
+        ( map {
+            /^photo/ ? # photo, photo1, photo2 etc.
+                ($c->req->upload($_)) : ()
+        } sort $c->req->upload),
+        split /,/, ($c->req->param('upload_fileid') || '')
+    );
+
+    my $photoset = FixMyStreet::App::Model::PhotoSet->new({
+        c => $c,
+        data_items => \@items,
+    });
+
+    my $fileid = $photoset->data;
+
+    $c->stash->{upload_fileid} = $fileid or return;
     return 1;
 }
 
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index b55152693..6a884db7f 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -93,6 +93,7 @@ sub report_new : Path : Args(0) {
     $c->forward('check_for_category');
 
     # deal with the user and report and check both are happy
+
     return unless $c->forward('check_form_submitted');
     $c->forward('process_user');
     $c->forward('process_report');
@@ -290,7 +291,7 @@ sub report_import : Path('/import') {
     }
 
     # handle the photo upload
-    $c->forward( '/photo/process_photo_upload' );
+    $c->forward( '/photo/process_photo' );
     my $fileid = $c->stash->{upload_fileid};
     if ( my $error = $c->stash->{photo_error} ) {
         push @errors, $error;
diff --git a/perllib/FixMyStreet/App/Model/PhotoSet.pm b/perllib/FixMyStreet/App/Model/PhotoSet.pm
new file mode 100644
index 000000000..fa6eb060b
--- /dev/null
+++ b/perllib/FixMyStreet/App/Model/PhotoSet.pm
@@ -0,0 +1,269 @@
+package FixMyStreet::App::Model::PhotoSet;
+
+# TODO this isn't a Cat model, rename to something else
+
+use Moose;
+use Path::Tiny 'path';
+use if !$ENV{TRAVIS}, 'Image::Magick';
+use Scalar::Util 'openhandle', 'blessed';
+use Digest::SHA qw(sha1_hex);
+use Image::Size;
+
+has c => (
+    is => 'ro',
+);
+
+has object => (
+    is => 'ro',
+);
+
+has data => ( # generic data from DB field
+    is => 'ro',
+    lazy => 1,
+    default => sub {
+        # yes, this is a little circular: data -> data_items -> items -> data
+        # e.g. if not provided, then we're presumably uploading/etc., so calculate from 
+        # the stored cached fileids
+        # (obviously if you provide none of these, then you'll get an infinite loop)
+        my $self = shift;
+        my $data = join ',', map { $_->[0] } $self->all_images;
+        return $data;
+    }
+);
+
+has data_items => ( # either a) split from data or b) provided by photo upload
+    isa => 'ArrayRef',
+    is => 'rw',
+    traits => ['Array'],
+    lazy => 1,
+    handles => {
+        map_data_items => 'map',
+    },
+    default => sub {
+        my $self = shift;
+        my $data = $self->data
+            or return [];
+
+        return [$data] if (_jpeg_magic($data));
+
+        return [ split ',' => $data ];
+    },
+);
+
+has upload_dir => (
+    is => 'ro',
+    lazy => 1,
+    default => sub {
+        my $self = shift;
+        my $cache_dir = path( $self->c->config->{UPLOAD_DIR} );
+        $cache_dir->mkpath;
+        unless ( -d $cache_dir && -w $cache_dir ) {
+            warn "Can't find/write to photo cache directory '$cache_dir'";
+            return;
+        }
+        $cache_dir;
+    },
+);
+
+sub _jpeg_magic {
+    $_[0] =~ /^\x{ff}\x{d8}/; # JPEG
+    # NB: should we also handle \x{89}\x{50} (PNG, 15 results in live DB) ?
+    #     and \x{49}\x{49} (Tiff, 3 results in live DB) ?
+}
+
+has images => ( # jpeg data for actual image
+    isa => 'ArrayRef',
+    is => 'rw',
+    traits => ['Array'],
+    lazy => 1,
+    handles => {
+        num_images => 'count',
+        get_raw_image_data => 'get',
+        all_images => 'elements',
+    },
+    default => sub {
+        my $self = shift;
+        my @photos = $self->map_data_items( sub {
+            my $part = $_;
+
+            if (blessed $part and $part->isa('Catalyst::Request::Upload')) {
+                # check that the photo is a jpeg
+                my $upload = $part;
+                my $ct = $upload->type;
+                $ct =~ s/x-citrix-//; # Thanks, Citrix
+                # Had a report of a JPEG from an Android 2.1 coming through as a byte stream
+                unless ( $ct eq 'image/jpeg' || $ct eq 'image/pjpeg' || $ct eq 'application/octet-stream' ) {
+                    my $c = $self->c;
+                    $c->log->info('Bad photo tried to upload, type=' . $ct);
+                    $c->stash->{photo_error} = _('Please upload a JPEG image only');
+                    return ();
+                }
+
+                # get the photo into a variable
+                my $photo_blob = eval {
+                    my $filename = $upload->tempname;
+                    my $out = `jhead -se -autorot $filename 2>&1`;
+                    unless (defined $out) {
+                        my ($w, $h, $err) = Image::Size::imgsize($filename);
+                        die _("Please upload a JPEG image only") . "\n" if !defined $w || $err ne 'JPG';
+                    }
+                    die _("Please upload a JPEG image only") . "\n" if $out && $out =~ /Not JPEG:/;
+                    my $photo = $upload->slurp;
+                };
+                if ( my $error = $@ ) {
+                    my $format = _(
+            "That image doesn't appear to have uploaded correctly (%s), please try again."
+                    );
+                    $self->c->stash->{photo_error} = sprintf( $format, $error );
+                    return ();
+                }
+
+                # we have an image we can use - save it to the upload dir for storage
+                my $fileid = $self->get_fileid($photo_blob);
+                my $file = $self->get_file($fileid);
+                $upload->copy_to( $file );
+                return [$fileid, $photo_blob];
+
+            }
+            if (_jpeg_magic($part)) {
+                my $photo_blob = $part;
+                my $fileid = $self->get_fileid($photo_blob);
+                my $file = $self->get_file($fileid);
+                $file->spew_raw($photo_blob);
+                return [$fileid, $photo_blob];
+            }
+            if (length($part) == 40) {
+                my $fileid = $part;
+                my $file = $self->get_file($fileid);
+                if ($file->exists) {
+                    my $photo = $file->slurp_raw;
+                    [$fileid, $photo];
+                }
+                else {
+                    warn "File $fileid doesn't exist";
+                    ();
+                }
+            }
+            else {
+                warn sprintf "Received bad photo hash of length %d", length($part);
+                ();
+            }
+        });
+        return \@photos;
+    },
+);
+
+sub get_fileid {
+    my ($self, $photo_blob) = @_;
+    return sha1_hex($photo_blob);
+}
+
+sub get_file {
+    my ($self, $fileid) = @_;
+    my $cache_dir = $self->upload_dir;
+    return path( $cache_dir, "$fileid.jpeg" );
+}
+
+sub get_image_data {
+    my ($self, %args) = @_;
+    my $num = $args{num} || 0;
+
+    my $data = $self->get_raw_image_data( $num )
+        or return;
+
+    my ($fileid, $photo) = @$data;
+
+    my $size = $args{size};
+    if ( $size eq 'tn' ) {
+        $photo = _shrink( $photo, 'x100' );
+    } elsif ( $size eq 'fp' ) {
+        $photo = _crop( $photo );
+    } elsif ( $size eq 'full' ) {
+        # do nothing
+    } else {
+        $photo = _shrink( $photo, $self->c->cobrand->default_photo_resize || '250x250' );
+    }
+
+    return $photo;
+}
+
+sub delete_cached {
+    my ($self) = @_;
+    my $object = $self->object or return;
+
+    unlink glob FixMyStreet->path_to(
+        'web',
+        'photo',
+        $object->id . '.*'
+    );
+}
+
+sub rotate_image {
+    my ($self, $index, $direction) = @_;
+
+    my @images = $self->all_images;
+    return if $index > $#images;
+
+    my @items = map $_->[0], @images;
+    $items[$index] = _rotate_image( $images[$index][1], $direction );
+
+    my $new_set = (ref $self)->new({
+        data_items => \@items,
+        c => $self->c,
+        object => $self->object,
+    });
+
+    $self->delete_cached();
+
+    return $new_set->data; # e.g. new comma-separated fileid
+}
+
+sub _rotate_image {
+    my ($photo, $direction) = @_;
+    return $photo unless $Image::Magick::VERSION;
+    my $image = Image::Magick->new;
+    $image->BlobToImage($photo);
+    my $err = $image->Rotate($direction);
+    return 0 if $err;
+    my @blobs = $image->ImageToBlob();
+    undef $image;
+    return $blobs[0];
+}
+
+
+
+
+
+# NB: These 2 subs stolen from A::C::Photo, should be purged from there!
+#
+# Shrinks a picture to the specified size, but keeping in proportion.
+sub _shrink {
+    my ($photo, $size) = @_;
+    return $photo unless $Image::Magick::VERSION;
+    my $image = Image::Magick->new;
+    $image->BlobToImage($photo);
+    my $err = $image->Scale(geometry => "$size>");
+    throw Error::Simple("resize failed: $err") if "$err";
+    $image->Strip();
+    my @blobs = $image->ImageToBlob();
+    undef $image;
+    return $blobs[0];
+}
+
+# Shrinks a picture to 90x60, cropping so that it is exactly that.
+sub _crop {
+    my ($photo) = @_;
+    return $photo unless $Image::Magick::VERSION;
+    my $image = Image::Magick->new;
+    $image->BlobToImage($photo);
+    my $err = $image->Resize( geometry => "90x60^" );
+    throw Error::Simple("resize failed: $err") if "$err";
+    $err = $image->Extent( geometry => '90x60', gravity => 'Center' );
+    throw Error::Simple("resize failed: $err") if "$err";
+    $image->Strip();
+    my @blobs = $image->ImageToBlob();
+    undef $image;
+    return $blobs[0];
+}
+
+1;
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 9706087aa..635c41382 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -483,11 +483,17 @@ sub url {
 
 =head2 get_photo_params
 
-Returns a hashref of details of any attached photo for use in templates.
+Returns a hashref of details of the attached photo, if any, for use in templates.
+
+NB: this method doesn't currently support multiple photos gracefully.  
+
+Use get_photoset($c) instead to do the right thing with reports with 0, 1, or more photos.
 
 =cut
 
 sub get_photo_params {
+    # use Carp 'cluck';
+    # cluck "get_photo_params called"; # TEMPORARY die to make sure I've done right thing with Zurich templates
     my $self = shift;
     return FixMyStreet::App::get_photo_params($self, 'id');
 }
@@ -828,6 +834,27 @@ sub latest_moderation_log_entry {
     return $self->admin_log_entries->search({ action => 'moderation' }, { order_by => 'id desc' })->first;
 }
 
+=head2 get_photoset
+
+Return a PhotoSet object for all photos attached to this field
+
+    my $photoset = $obj->get_photoset( $c );
+    print $photoset->num_images;
+    return $photoset->get_image_data(num => 0, size => 'full');
+
+=cut
+
+sub get_photoset {
+    my ($self, $c) = @_;
+    my $class = 'FixMyStreet::App::Model::PhotoSet';
+    eval "use $class";
+    return $class->new({
+        c => $c,
+        data => $self->photo,
+        object => $self,
+    });
+}
+
 __PACKAGE__->has_many(
   "admin_log_entries",
   "FixMyStreet::DB::Result::AdminLog",
diff --git a/perllib/FixMyStreet/TestMech.pm b/perllib/FixMyStreet/TestMech.pm
index bd2ca4096..ef60b0d36 100644
--- a/perllib/FixMyStreet/TestMech.pm
+++ b/perllib/FixMyStreet/TestMech.pm
@@ -631,7 +631,7 @@ sub create_problems_for_body {
             latitude           => '51.5016605453401',
             longitude          => '-0.142497580865087',
             user_id            => $user->id,
-            photo              => 1,
+            photo              => $mech->get_photo_data,
         };
 
         my %report_params = ( %$default_params, %$params );
@@ -646,4 +646,13 @@ sub create_problems_for_body {
     return @problems;
 }
 
+sub get_photo_data {
+    my $mech = shift;
+    return $mech->{sample_photo} ||= do {
+        my $sample_file = FixMyStreet->path_to( 't/app/controller/sample.jpg' );
+        $mech->builder->ok( -f "$sample_file", "sample file $sample_file exists" );
+        $sample_file->slurp(iomode => '<:raw');
+    };
+}
+
 1;