Merge remote-tracking branch 'origin/1302-support-pngs'

11 files changed, 82 insertions, 66 deletions

diff --git a/perllib/Catalyst/Plugin/Session/State/Cookie.pm b/perllib/Catalyst/Plugin/Session/State/Cookie.pm
index bef0c1e66..c4b61123b 100644
--- a/perllib/Catalyst/Plugin/Session/State/Cookie.pm
+++ b/perllib/Catalyst/Plugin/Session/State/Cookie.pm
@@ -50,13 +50,6 @@ sub update_session_cookie {
 sub cookie_is_rejecting {
     my ( $c, $cookie ) = @_;
 
-    # Prevent infinite loop in request->path. mySociety addition
-    return 0 if $c->request->has_uri && !$c->request->_has_path;
-
-    # Don't output cookie for JS or JPEG files. mySociety addition
-    return 1 if substr($c->request->path, -3) eq '.js'
-        || substr($c->request->path, -5) eq '.jpeg';
-
     if ( $cookie->{path} ) {
         return 1 if index '/'.$c->request->path, $cookie->{path};
     }
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
index af9dc1f9d..79ca7f9ee 100644
--- a/perllib/FixMyStreet/App.pm
+++ b/perllib/FixMyStreet/App.pm
@@ -246,7 +246,7 @@ sub setup_dev_overrides {
     delete $params{$_} for grep { !m{^_override_} } keys %params;
 
     # stop if there is nothing to add
-    return 1 unless scalar keys %params;
+    return unless scalar keys %params;
 
     # Check to see if we should clear all
     if ( $params{_override_clear_all} ) {
@@ -270,14 +270,14 @@ sub setup_dev_overrides {
 
 Checks the overrides for the value given and returns it if found, undef if not.
 
-Always returns undef unless on a staging site (avoids autovivifying overrides
-hash in session and so creating a session for all users).
+Always returns undef unless on a staging site and we already have a session
+(avoids autovivifying overrides hash and so creating a session for all users).
 
 =cut
 
 sub get_override {
     my ( $c, $key ) = @_;
-    return unless $c->config->{STAGING_SITE};
+    return unless $c->config->{STAGING_SITE} && $c->sessionid;
     return $c->session->{overrides}->{$key};
 }
 
diff --git a/perllib/FixMyStreet/App/Controller/Open311.pm b/perllib/FixMyStreet/App/Controller/Open311.pm
index f35dc64a5..4f1727b1a 100644
--- a/perllib/FixMyStreet/App/Controller/Open311.pm
+++ b/perllib/FixMyStreet/App/Controller/Open311.pm
@@ -284,7 +284,7 @@ sub output_requests : Private {
         my $display_photos = $c->cobrand->allow_photo_display($problem);
         if ($display_photos && $problem->photo) {
             my $url = $c->cobrand->base_url();
-            my $imgurl = $url . "/photo/$id.full.jpeg";
+            my $imgurl = $url . $problem->photos->[0]->{url_full};
             $request->{'media_url'} = [ $imgurl ];
         }
         push(@problemlist, $request);
diff --git a/perllib/FixMyStreet/App/Controller/Photo.pm b/perllib/FixMyStreet/App/Controller/Photo.pm
index bfb1c5535..59f54bad7 100644
--- a/perllib/FixMyStreet/App/Controller/Photo.pm
+++ b/perllib/FixMyStreet/App/Controller/Photo.pm
@@ -29,12 +29,12 @@ Display a photo
 
 =cut
 
-sub during :LocalRegex('^([0-9a-f]{40})\.(temp|fulltemp)\.jpeg$') {
+sub during :LocalRegex('^(temp|fulltemp)\.([0-9a-f]{40}\.(?:jpeg|png|gif|tiff))$') {
     my ( $self, $c ) = @_;
-    my ( $hash, $size ) = @{ $c->req->captures };
+    my ( $size, $filename ) = @{ $c->req->captures };
 
     my $photoset = FixMyStreet::App::Model::PhotoSet->new({
-        data_items => [ $hash ]
+        data_items => [ $filename ]
     });
 
     $size = $size eq 'temp' ? 'default' : 'full';
@@ -43,7 +43,7 @@ sub during :LocalRegex('^([0-9a-f]{40})\.(temp|fulltemp)\.jpeg$') {
     $c->forward( 'output', [ $photo ] );
 }
 
-sub index :LocalRegex('^(c/)?(\d+)(?:\.(\d+))?(?:\.(full|tn|fp))?\.jpeg$') {
+sub index :LocalRegex('^(c/)?(\d+)(?:\.(\d+))?(?:\.(full|tn|fp))?\.(?:jpeg|png|gif|tiff)$') {
     my ( $self, $c ) = @_;
     my ( $is_update, $id, $photo_number, $size ) = @{ $c->req->captures };
 
@@ -79,10 +79,10 @@ sub output : Private {
 
     # Save to file
     File::Path::make_path( FixMyStreet->path_to( 'web', 'photo', 'c' )->stringify );
-    File::Slurp::write_file( FixMyStreet->path_to( 'web', $c->req->path )->stringify, \$photo );
+    File::Slurp::write_file( FixMyStreet->path_to( 'web', $c->req->path )->stringify, \$photo->{data} );
 
-    $c->res->content_type( 'image/jpeg' );
-    $c->res->body( $photo );
+    $c->res->content_type( $photo->{content_type} );
+    $c->res->body( $photo->{data} );
 }
 
 sub no_photo : Private {
diff --git a/perllib/FixMyStreet/App/Controller/Rss.pm b/perllib/FixMyStreet/App/Controller/Rss.pm
index 6047f063b..d708aa71c 100755
--- a/perllib/FixMyStreet/App/Controller/Rss.pm
+++ b/perllib/FixMyStreet/App/Controller/Rss.pm
@@ -6,6 +6,8 @@ use POSIX qw(strftime);
 use URI::Escape;
 use XML::RSS;
 
+use FixMyStreet::App::Model::PhotoSet;
+
 use mySociety::Gaze;
 use mySociety::Locale;
 use mySociety::MaPit;
@@ -277,8 +279,13 @@ sub add_row : Private {
     $item{category} = ent($row->{category}) if $row->{category};
 
     if ($c->cobrand->allow_photo_display($row) && $row->{photo}) {
+        # Bit yucky as we don't have full objects here
+        my $photoset = FixMyStreet::App::Model::PhotoSet->new({ db_data => $row->{photo} });
+        my $first_fn = $photoset->get_id(0);
+        my ($hash, $format) = split /\./, $first_fn;
+        my $cachebust = substr($hash, 0, 8);
         my $key = $alert_type->item_table eq 'comment' ? 'c/' : '';
-        $item{description} .= ent("\n<br><img src=\"". $base_url . "/photo/$key$row->{id}.jpeg\">");
+        $item{description} .= ent("\n<br><img src=\"". $base_url . "/photo/$key$row->{id}.0.$format?$cachebust\">");
     }
 
     if ( $row->{used_map} ) {
diff --git a/perllib/FixMyStreet/App/Model/PhotoSet.pm b/perllib/FixMyStreet/App/Model/PhotoSet.pm
index 54457bae9..41d02d2a7 100644
--- a/perllib/FixMyStreet/App/Model/PhotoSet.pm
+++ b/perllib/FixMyStreet/App/Model/PhotoSet.pm
@@ -49,7 +49,7 @@ has data_items => ( # either a) split from db_data or b) provided by photo uploa
         my $self = shift;
         my $data = $self->db_data or return [];
 
-        return [$data] if (_jpeg_magic($data));
+        return [$data] if (detect_type($data));
 
         return [ split ',' => $data ];
     },
@@ -70,10 +70,12 @@ has upload_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) ?
+sub detect_type {
+    return 'jpeg' if $_[0] =~ /^\x{ff}\x{d8}/;
+    return 'png' if $_[0] =~ /^\x{89}\x{50}/;
+    return 'tiff' if $_[0] =~ /^II/;
+    return 'gif' if $_[0] =~ /^GIF/;
+    return '';
 }
 
 =head2 C<ids>, C<num_images>, C<get_id>, C<all_ids>
@@ -106,15 +108,17 @@ has ids => ( #  Arrayref of $fileid tuples (always, so post upload/raw data proc
             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
+                my ($type) = $ct =~ m{image/(jpeg|pjpeg|gif|tiff|png)};
+                $type = 'jpeg' if $type && $type eq 'pjpeg';
                 # 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' ) {
+                $type = 'jpeg' if !$type && $ct eq 'application/octet-stream';
+                unless ( $type ) {
                     my $c = $self->c;
                     $c->log->info('Bad photo tried to upload, type=' . $ct);
-                    $c->stash->{photo_error} = _('Please upload a JPEG image only');
+                    $c->stash->{photo_error} = _('Please upload an image only');
                     return ();
                 }
 
@@ -139,12 +143,13 @@ has ids => ( #  Arrayref of $fileid tuples (always, so post upload/raw data proc
                 # get the photo into a variable
                 my $photo_blob = eval {
                     my $filename = $upload->tempname;
-                    my $out = `jhead -se -autorot $filename 2>&1`;
+                    my $out;
+                    $out = `jhead -se -autorot $filename 2>&1` if $type eq 'jpeg';
                     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 an image only") . "\n" if !defined $w || $err !~ /JPG|GIF|PNG|TIF/;
                     }
-                    die _("Please upload a JPEG image only") . "\n" if $out && $out =~ /Not JPEG:/;
+                    die _("Please upload an image only") . "\n" if $out && $out =~ /Not JPEG:/;
                     my $photo = $upload->slurp;
                 };
                 if ( my $error = $@ ) {
@@ -157,29 +162,30 @@ has ids => ( #  Arrayref of $fileid tuples (always, so post upload/raw data proc
 
                 # 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);
+                my $file = $self->get_file($fileid, $type);
                 $upload->copy_to( $file );
-                return $fileid;
+                return $file->basename;
 
             }
-            if (_jpeg_magic($part)) {
+            if (my $type = detect_type($part)) {
                 my $photo_blob = $part;
                 my $fileid = $self->get_fileid($photo_blob);
-                my $file = $self->get_file($fileid);
+                my $file = $self->get_file($fileid, $type);
                 $file->spew_raw($photo_blob);
-                return $fileid;
+                return $file->basename;
             }
-            if (length($part) == 40) {
-                my $fileid = $part;
-                my $file = $self->get_file($fileid);
+            my ($fileid, $type) = split /\./, $part;
+            $type ||= 'jpeg';
+            if (length($fileid) == 40) {
+                my $file = $self->get_file($fileid, $type);
                 if ($file->exists) {
-                    $fileid;
+                    $file->basename;
                 } else {
-                    warn "File $fileid doesn't exist";
+                    warn "File $part doesn't exist";
                     ();
                 }
             } else {
-                warn sprintf "Received bad photo hash of length %d", length($part);
+                warn sprintf "Received bad photo hash of length %d", length($fileid);
                 ();
             }
         });
@@ -193,18 +199,23 @@ sub get_fileid {
 }
 
 sub get_file {
-    my ($self, $fileid) = @_;
+    my ($self, $fileid, $type) = @_;
     my $cache_dir = $self->upload_dir;
-    return path( $cache_dir, "$fileid.jpeg" );
+    return path( $cache_dir, "$fileid.$type" );
 }
 
-sub get_raw_image_data {
+sub get_raw_image {
     my ($self, $index) = @_;
-    my $fileid = $self->get_id($index);
-    my $file = $self->get_file($fileid);
+    my $filename = $self->get_id($index);
+    my ($fileid, $type) = split /\./, $filename;
+    my $file = $self->get_file($fileid, $type);
     if ($file->exists) {
         my $photo = $file->slurp_raw;
-        return $photo;
+        return {
+            data => $photo,
+            content_type => "image/$type",
+            extension => $type,
+        };
     }
 }
 
@@ -212,8 +223,9 @@ sub get_image_data {
     my ($self, %args) = @_;
     my $num = $args{num} || 0;
 
-    my $photo = $self->get_raw_image_data( $num )
+    my $image = $self->get_raw_image( $num )
         or return;
+    my $photo = $image->{data};
 
     my $size = $args{size};
     if ( $size eq 'tn' ) {
@@ -226,7 +238,10 @@ sub get_image_data {
         $photo = _shrink( $photo, $args{default} || '250x250' );
     }
 
-    return $photo;
+    return {
+        data => $photo,
+        content_type => $image->{content_type},
+    };
 }
 
 sub delete_cached {
@@ -266,8 +281,8 @@ sub rotate_image {
     my @images = $self->all_ids;
     return if $index > $#images;
 
-    my $image_data = $self->get_raw_image_data($index);
-    $images[$index] = _rotate_image( $image_data, $direction );
+    my $image = $self->get_raw_image($index);
+    $images[$index] = _rotate_image( $image->{data}, $direction );
 
     my $new_set = (ref $self)->new({
         data_items => \@images,
diff --git a/perllib/FixMyStreet/Cobrand/Zurich.pm b/perllib/FixMyStreet/Cobrand/Zurich.pm
index 596ef2dc6..d31b1c84e 100644
--- a/perllib/FixMyStreet/Cobrand/Zurich.pm
+++ b/perllib/FixMyStreet/Cobrand/Zurich.pm
@@ -1018,14 +1018,15 @@ sub munge_sendreport_params {
             or return;
         my $id = $row->id;
         my @attachments = map {
+            my $image = $photoset->get_raw_image($_);
             {
-                body => $photoset->get_raw_image_data($_),
+                body => $image->{data},
                 attributes => {
-                    filename => "$id.$_.jpeg",
-                    content_type => 'image/jpeg',
+                    filename => "$id.$_." . $image->{extension},
+                    content_type => $image->{content_type},
                     encoding => 'base64',
                         # quoted-printable ends up with newlines corrupting binary data
-                    name => "$id.$_.jpeg",
+                    name => "$id.$_." . $image->{extension},
                 },
             }
         } (0..$num-1);
diff --git a/perllib/FixMyStreet/DB/Result/Comment.pm b/perllib/FixMyStreet/DB/Result/Comment.pm
index e5929ca57..85cdb29f0 100644
--- a/perllib/FixMyStreet/DB/Result/Comment.pm
+++ b/perllib/FixMyStreet/DB/Result/Comment.pm
@@ -173,12 +173,12 @@ sub photos {
     my $i = 0;
     my $id = $self->id;
     my @photos = map {
-        my $format = 'jpeg';
         my $cachebust = substr($_, 0, 8);
+        my ($hash, $format) = split /\./, $_;
         {
-            id => $_,
-            url_temp => "/photo/$_.temp.$format",
-            url_temp_full => "/photo/$_.fulltemp.$format",
+            id => $hash,
+            url_temp => "/photo/temp.$hash.$format",
+            url_temp_full => "/photo/fulltemp.$hash.$format",
             url => "/photo/c/$id.$i.$format?$cachebust",
             url_full => "/photo/c/$id.$i.full.$format?$cachebust",
             idx => $i++,
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 780a30e69..764f381a2 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -875,12 +875,12 @@ sub photos {
     my $i = 0;
     my $id = $self->id;
     my @photos = map {
-        my $format = 'jpeg';
         my $cachebust = substr($_, 0, 8);
+        my ($hash, $format) = split /\./, $_;
         {
-            id => $_,
-            url_temp => "/photo/$_.temp.$format",
-            url_temp_full => "/photo/$_.fulltemp.$format",
+            id => $hash,
+            url_temp => "/photo/temp.$hash.$format",
+            url_temp_full => "/photo/fulltemp.$hash.$format",
             url => "/photo/$id.$i.$format?$cachebust",
             url_full => "/photo/$id.$i.full.$format?$cachebust",
             url_tn => "/photo/$id.$i.tn.$format?$cachebust",
diff --git a/perllib/FixMyStreet/Script/Reports.pm b/perllib/FixMyStreet/Script/Reports.pm
index ac8d9a7ab..8b1ce6759 100644
--- a/perllib/FixMyStreet/Script/Reports.pm
+++ b/perllib/FixMyStreet/Script/Reports.pm
@@ -83,7 +83,7 @@ sub send(;$) {
         $h{phone_line} = $h{phone} ? _('Phone:') . " $h{phone}\n\n" : '';
         if ($row->photo) {
             $h{has_photo} = _("This web page also contains a photo of the problem, provided by the user.") . "\n\n";
-            $h{image_url} = $email_base_url . '/photo/' . $row->id . '.full.jpeg';
+            $h{image_url} = $email_base_url . $row->photos->[0]->{url_full};
         } else {
             $h{has_photo} = '';
             $h{image_url} = '';
diff --git a/perllib/Open311.pm b/perllib/Open311.pm
index e500219bc..fb793b027 100644
--- a/perllib/Open311.pm
+++ b/perllib/Open311.pm
@@ -345,7 +345,7 @@ sub _populate_service_request_update_params {
     if ( $comment->photo ) {
         my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($comment->cobrand)->new();
         my $email_base_url = $cobrand->base_url($comment->cobrand_data);
-        my $url = $email_base_url . '/photo/c/' . $comment->id . '.full.jpeg';
+        my $url = $email_base_url . $comment->photos->[0]->{url_full};
         $params->{media_url} = $url;
     }