Sign up new report to email alert if logged in as well as when confirmed by email. Allow disabling rather than deleting of alerts. Revert quoting as it breaks timestamps and literal SQL.

5 files changed, 35 insertions, 72 deletions

diff --git a/perllib/FixMyStreet/Alert.pm b/perllib/FixMyStreet/Alert.pm
index 1857abbba..64bd40dba 100644
--- a/perllib/FixMyStreet/Alert.pm
+++ b/perllib/FixMyStreet/Alert.pm
@@ -8,12 +8,6 @@
 #
 # $Id: Alert.pm,v 1.71 2010-01-06 16:50:27 louise Exp $
 
-package FixMyStreet::Alert::Error;
-
-use Error qw(:try);
-
-@FixMyStreet::Alert::Error::ISA = qw(Error::Simple);
-
 package FixMyStreet::Alert;
 
 use strict;
@@ -32,57 +26,9 @@ use mySociety::Locale;
 use mySociety::MaPit;
 use mySociety::Random qw(random_bytes);
 
-# Add a new alert
-sub create ($$$$;@) {
-    my ($email, $alert_type, $cobrand, $cobrand_data, @params) = @_;
-    my $already = 0;
-    if (0==@params) {
-        ($already) = dbh()->selectrow_array('select id from alert where alert_type=? and email=? limit 1',
-            {}, $alert_type, $email);
-    } elsif (1==@params) {
-        ($already) = dbh()->selectrow_array('select id from alert where alert_type=? and email=? and parameter=? limit 1',
-            {}, $alert_type, $email, @params);
-    } elsif (2==@params) {
-        ($already) = dbh()->selectrow_array('select id from alert where alert_type=? and email=? and parameter=? and parameter2=? limit 1',
-            {}, $alert_type, $email, @params);
-    }
-    return $already if $already;
-
-    my $id = dbh()->selectrow_array("select nextval('alert_id_seq');");
-    my $lang = $mySociety::Locale::lang;
-    if (0==@params) {
-        dbh()->do('insert into alert (id, alert_type, email, lang, cobrand, cobrand_data)
-            values (?, ?, ?, ?, ?, ?)', {}, $id, $alert_type, $email, $lang, $cobrand, $cobrand_data);
-    } elsif (1==@params) {
-        dbh()->do('insert into alert (id, alert_type, parameter, email, lang, cobrand, cobrand_data)
-            values (?, ?, ?, ?, ?, ?, ?)', {}, $id, $alert_type, @params, $email, $lang, $cobrand, $cobrand_data);
-    } elsif (2==@params) {
-        dbh()->do('insert into alert (id, alert_type, parameter, parameter2, email, lang, cobrand, cobrand_data)
-            values (?, ?, ?, ?, ?, ?, ?, ?)', {}, $id, $alert_type, @params, $email, $lang, $cobrand, $cobrand_data);
-    }
-    dbh()->commit();
-    return $id;
-}
-
-sub confirm ($) {
-    my $id = shift;
-    dbh()->do("update alert set confirmed=1, whendisabled=null where id=?", {}, $id);
-    dbh()->commit();
-}
-
-# Delete an alert
-sub delete ($) {
-    my $id = shift;
-    dbh()->do('update alert set whendisabled = ms_current_timestamp() where id = ?', {}, $id);
-    dbh()->commit();
-}
-
-# This makes load of assumptions, but still should be useful
-# 
 # Child must have confirmed, id, email, state(!) columns
 # If parent/child, child table must also have name and text
 #   and foreign key to parent must be PARENT_id
-
 sub email_alerts ($) {
     my ($testing_email) = @_;
     my $url; 
@@ -236,7 +182,8 @@ sub _send_aggregated_alert_email(%) {
         dbh()->commit();
     } else {
         dbh()->rollback();
-        throw FixMyStreet::Alert::Error("Failed to send alert $data{alert_id}!");
+        print "Failed to send alert $data{alert_id}!";
     }
 }
 
+1;
diff --git a/perllib/FixMyStreet/App/Controller/Alert.pm b/perllib/FixMyStreet/App/Controller/Alert.pm
index bea5345e3..70a86a936 100644
--- a/perllib/FixMyStreet/App/Controller/Alert.pm
+++ b/perllib/FixMyStreet/App/Controller/Alert.pm
@@ -140,7 +140,7 @@ sub subscribe_email : Private {
         $c->forward('set_local_alert_options');
     }
     else {
-        throw FixMyStreet::Alert::Error('Invalid type');
+        $c->detach( '/page_error_404_not_found', [ 'Invalid type' ] );
     }
 
     $c->forward('create_alert');
@@ -162,7 +162,7 @@ sub updates : Path('updates') : Args(0) {
 
 =head2 confirm
 
-Confirm signup to an alert. Forwarded here from Tokens.
+Confirm signup to or unsubscription from an alert. Forwarded here from Tokens.
 
 =cut
 
@@ -173,11 +173,9 @@ sub confirm : Private {
 
     if ( $c->stash->{confirm_type} eq 'subscribe' ) {
         $alert->confirm();
-        $alert->update;
     }
     elsif ( $c->stash->{confirm_type} eq 'unsubscribe' ) {
-        $alert->delete();
-        $alert->update;
+        $alert->disable();
     }
 }
 
@@ -198,6 +196,7 @@ sub create_alert : Private {
     unless ($alert) {
         $options->{cobrand}      = $c->cobrand->moniker();
         $options->{cobrand_data} = $c->cobrand->extra_update_data();
+        $options->{lang}         = $c->stash->{lang_code};
 
         if ( $c->user && $c->user->id == $c->stash->{alert_user}->id ) {
             $options->{confirmed} = 1;
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index 671454272..9e0193d3f 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -931,8 +931,10 @@ sub redirect_or_confirm_creation : Private {
     my ( $self, $c ) = @_;
     my $report = $c->stash->{report};
 
-    # If confirmed send the user straigh there.
+    # If confirmed send the user straight there.
     if ( $report->confirmed ) {
+        # Subscribe problem reporter to email updates
+        $c->forward( 'create_reporter_alert' );
         my $report_uri = $c->uri_for( '/report', $report->id );
         $c->res->redirect($report_uri);
         $c->detach;
@@ -950,6 +952,20 @@ sub redirect_or_confirm_creation : Private {
     $c->stash->{email_type} = 'problem';
 }
 
+sub create_reporter_alert : Private {
+    my ( $self, $c ) = @_;
+
+    my $problem = $c->stash->{report};
+    my $alert = $c->model('DB::Alert')->find_or_create( {
+        user         => $problem->user,
+        alert_type   => 'new_updates',
+        parameter    => $problem->id,
+        cobrand      => $problem->cobrand,
+        cobrand_data => $problem->cobrand_data,
+        lang         => $problem->lang,
+    } )->confirm;
+}
+
 =head2 redirect_to_around
 
 Redirect the user to '/around' passing along all the relevant parameters.
diff --git a/perllib/FixMyStreet/App/Controller/Tokens.pm b/perllib/FixMyStreet/App/Controller/Tokens.pm
index 792c875fd..958188ca0 100644
--- a/perllib/FixMyStreet/App/Controller/Tokens.pm
+++ b/perllib/FixMyStreet/App/Controller/Tokens.pm
@@ -57,15 +57,8 @@ sub confirm_problem : Path('/P') {
     ) if $problem->state eq 'unconfirmed';
 
     # Subscribe problem reporter to email updates
-    my $alert = $c->model('DB::Alert')->find_or_create(
-        {
-            user         => $problem->user,
-            alert_type   => 'new_updates',
-            cobrand      => $problem->cobrand,
-            cobrand_data => $problem->cobrand_data,
-            parameter    => $problem->id
-        }
-    )->confirm;
+    $c->stash->{report} = $c->stash->{problem};
+    $c->forward( '/report/new/create_reporter_alert' );
 
     # log the problem creation user in to the site
     $c->authenticate( { email => $problem->user->email }, 'no_password' );
diff --git a/perllib/FixMyStreet/DB/Result/Alert.pm b/perllib/FixMyStreet/DB/Result/Alert.pm
index b99c83a45..53cb96ff4 100644
--- a/perllib/FixMyStreet/DB/Result/Alert.pm
+++ b/perllib/FixMyStreet/DB/Result/Alert.pm
@@ -85,10 +85,18 @@ Sets the state of the alert to confirmed.
 sub confirm {
     my $self = shift;
 
-    return if $self->confirmed and $self->confirmed == 1 and $self->whendisabled ne 'null';
-
     $self->confirmed(1);
     $self->whendisabled(undef);
+    $self->update;
+
+    return 1;
+}
+
+sub disable {
+    my $self = shift;
+
+    $self->whendisabled( \'ms_current_timestamp()' );
+    $self->update;
 
     return 1;
 }