Email alert text updates

CHANGELOG.md

@@ -19,6 +19,7 @@
  - Fix Open311 JSON services output. #3279
  - Send email reports in staff-only categories.
  - Fix Gaze sometimes being called twice on /around. #3324
+ - Improved alert signup for phone-only user. #3367
  - Admin improvements:
  - Enable per-category hint customisation.
  - Move ban/unban buttons to user edit admin page.

perllib/FixMyStreet/App/Controller/Alert.pm

@@ -298,16 +298,20 @@ sub send_confirmation_email : Private {
  # People using 2FA can not log in by code
  $c->detach( '/page_error_403_access_denied', [] ) if $user->has_2fa;
 
- my $token = $c->model("DB::Token")->create(
- {
- scope => 'alert',
- data => {
- id => $c->stash->{alert}->id,
- type => 'subscribe',
- email => $user->email
- }
- }
- );
+ my $data = {
+ id => $c->stash->{alert}->id,
+ type => 'subscribe',
+ };
+
+ # Logged in as a phone user, need to verify email and update user account
+ if ($c->user_exists && !$c->user->email_verified) {
+ $data->{old_user_id} = $c->user->id;
+ }
+
+ my $token = $c->model("DB::Token")->create({
+ scope => 'alert',
+ data => $data,
+ });
 
  $c->stash->{token_url} = $c->uri_for_email( '/A', $token->token );
 
@@ -375,10 +379,14 @@ Fetch/check email address
 sub process_user : Private {
  my ( $self, $c ) = @_;
 
+ # If we are logged in (and not creating for someone else) we'll want to use
+ # the logged in user (but note, if they don't have a verified email and it
+ # is a local alert, we still want to email them confirmation)
+ my $type = $c->get_param('type') || "";
  if ( $c->user_exists ) {
  $c->stash->{can_create_for_another} = $c->stash->{problem}
  && $c->user->has_permission_to(contribute_as_another_user => $c->stash->{problem}->bodies_str_ids);
- if (!$c->stash->{can_create_for_another}) {
+ if (!$c->stash->{can_create_for_another} && ($type eq 'updates' || $c->user->email_verified)) {
  $c->stash->{alert_user} = $c->user->obj;
  return;
  }

perllib/FixMyStreet/App/Controller/Tokens.pm

@@ -80,8 +80,9 @@ sub confirm_alert : Path('/A') {
  my $auth_token = $c->forward( 'load_auth_token', [ $token_code, 'alert' ] );
 
  # Load the alert
- my $alert_id = $auth_token->data->{id};
- $c->stash->{confirm_type} = $auth_token->data->{type};
+ my $data = $auth_token->data;
+ my $alert_id = $data->{id};
+ $c->stash->{confirm_type} = $data->{type};
  my $alert = $c->model('DB::Alert')->find( { id => $alert_id } )
  || $c->detach('token_error');
  $c->stash->{alert} = $alert;
@@ -97,7 +98,21 @@ sub confirm_alert : Path('/A') {
  }
 
  if (!$alert->confirmed && $c->stash->{confirm_type} ne 'unsubscribe') {
- $c->authenticate( { email => $alert->user->email, email_verified => 1 }, 'no_password' );
+ my $user = $alert->user;
+ my $email = $user->email;
+ if ($data->{old_user_id}) {
+ # Were logged in as old_user_id, want to switch to $user
+ my $old_user = $c->model('DB::User')->find({ id => $data->{old_user_id} });
+ if ($old_user) {
+ $old_user->adopt($user);
+ $user = $old_user;
+ $user->email($email);
+ $user->email_verified(1);
+ $user->update;
+ }
+ }
+
+ $c->authenticate( { email => $user->email, email_verified => 1 }, 'no_password' );
  $c->set_session_cookie_expire(0);
  }
 

perllib/FixMyStreet/DB/Result/User.pm

@@ -213,6 +213,38 @@ sub phone_display {
  return $parsed->{phone} ? $parsed->{phone}->format_for_country($country) : $self->phone;
 }
 
+sub alert_by {
+ my ($self, $is_update, $cobrand) = @_;
+ return $is_update
+ ? $self->alert_updates_by($cobrand)
+ : $self->alert_local_by;
+}
+
+# How does this user want to receive local alerts?
+# email or none, so will be none for a phone-only user
+sub alert_local_by {
+ my $self = shift;
+ my $pref = $self->get_extra_metadata('alert_notify') || '';
+ return 'none' if $pref eq 'none';
+ return 'none' unless $self->email_verified;
+ return 'email';
+}
+
+# How does this user want to receive update alerts?
+# If the cobrand allows text, this could include phone
+sub alert_updates_by {
+ my ($self, $cobrand) = @_;
+ my $pref = $self->get_extra_metadata('update_notify') || '';
+ return 'none' if $pref eq 'none';
+
+ # Only send text alerts for new report updates at present
+ my $allow_phone_update = ($self->phone_verified && $cobrand->sms_authentication);
+
+ return 'none' unless $self->email_verified || $allow_phone_update;
+ return 'phone' if $allow_phone_update && (!$self->email_verified || $pref eq 'phone');
+ return 'email';
+}
+
 sub latest_anonymity {
  my $self = shift;
  my $p = $self->problems->search(undef, { rows => 1, order_by => { -desc => 'id' } } )->first;

perllib/FixMyStreet/Script/Alerts.pm

@@ -300,13 +300,8 @@ sub _send_aggregated_alert(%) {
 
  my $user = $data{alert_user};
 
- my $pref = $data{is_new_update} ? 'update_notify' : 'alert_notify';
- $pref = $user->get_extra_metadata($pref) || '';
- return if $pref eq 'none';
-
- # Only send text alerts for new report updates at present
- my $allow_phone_update = ($user->phone_verified && $data{is_new_update} && $cobrand->sms_authentication);
- return unless $user->email_verified || $allow_phone_update;
+ my $alert_by = $user->alert_by($data{is_new_update}, $cobrand);
+ return if $alert_by eq 'none';
 
  # Mark user as active as they're being sent an alert
  $user->set_last_active;
@@ -335,7 +330,7 @@ sub _send_aggregated_alert(%) {
  $data{unsubscribe_url} = $cobrand->base_url( $data{cobrand_data} ) . '/A/' . $token->token;
 
  my $result;
- if ($allow_phone_update && (!$user->email_verified || $pref eq 'phone')) {
+ if ($alert_by eq 'phone') {
  $result = _send_aggregated_alert_phone(%data);
  } else {
  $result = _send_aggregated_alert_email(%data);

t/app/controller/alert_new.t

@@ -55,14 +55,31 @@ foreach my $test (
  type => 'new_updates',
  uri => '/alert/subscribe?type=updates&rznvy=' . $user->email . '&id=' . $report->id,
  param1 => $report->id,
- }
+ },
+ {
+ phone => 1,
+ email => $user->email,
+ type => 'local_problems',
+ uri => '/alert/subscribe?type=local&rznvy=' . $user->email . '&feed=local:10.2:20.1',
+ param1 => 20.1,
+ param2 => 10.2,
+ },
  )
 {
  subtest "$test->{type} alert correctly created" => sub {
  $mech->clear_emails_ok;
 
  my $type = $test->{type};
 
+ my $phone_user;
+ if ($test->{phone}) {
+ FixMyStreet::override_config {
+ SMS_AUTHENTICATION => 1,
+ }, sub {
+ $phone_user = $mech->log_in_ok( '01234567890' );
+ };
+ }
+
  $mech->get_ok('/alert/subscribe?id=' . $report->id);
  my ($csrf) = $mech->content =~ /name="token" value="([^"]*)"/;
 
@@ -133,6 +150,16 @@ foreach my $test (
  FixMyStreet::DB->resultset('Alert')->find( { id => $existing_id, } );
 
  ok $alert->confirmed, 'alert set to confirmed';
+
+ if ($phone_user) {
+ $phone_user->discard_changes;
+ is $phone_user->email, $test->{email}, 'Phone user now has email';
+ is $phone_user->email_verified, 1, 'Phone user now has email';
+ my $deleted_user = FixMyStreet::DB->resultset("User")->find({id => $user->id });
+ is $deleted_user, undef, 'Email user deleted';
+ $mech->delete_user($phone_user);
+ }
+
  $mech->delete_user($user);
  };
 }
@@ -927,7 +954,13 @@ subtest 'check staff updates can include sanitized HTML' => sub {
  $mech->delete_user( $user3 );
 };
 
-subtest 'test notification preferences' => sub {
+FixMyStreet::override_config {
+ ALLOWED_COBRANDS => 'fixmystreet',
+ SMS_AUTHENTICATION => 1,
+ TWILIO_ACCOUNT_SID => 'AC123',
+ MAPIT_URL => 'http://mapit.uk/',
+}, sub {
+ subtest 'test notification preferences' => sub {
  # Create a user with both email and phone verified
  my $user1 = $mech->create_user_ok('[email protected]',
  name => 'Alert User',
@@ -945,6 +978,12 @@ subtest 'test notification preferences' => sub {
  user => $user2,
  });
 
+ my $user0 = $mech->log_in_ok('01234567890');
+ $mech->get_ok('/alert/subscribe?id=' . $report->id);
+ $mech->content_contains('Receive a text when updates are left');
+ $mech->submit_form_ok({ button => 'alert' });
+ $mech->content_contains('Text alert created');
+
  my $update = $mech->create_comment_for_problem($report, $user3, 'Anonymous User', 'This is some more update text', 't', 'confirmed', undef, { confirmed => $r_dt });
 
  my $alert_user1 = FixMyStreet::DB->resultset('Alert')->create({
@@ -969,32 +1008,27 @@ subtest 'test notification preferences' => sub {
  $mech->email_count_is(0);
  is @{$twilio->texts}, 0;
 
- FixMyStreet::override_config {
- ALLOWED_COBRANDS => 'fixmystreet',
- SMS_AUTHENTICATION => 1,
- TWILIO_ACCOUNT_SID => 'AC123',
- MAPIT_URL => 'http://mapit.uk/',
- }, sub {
- foreach (
- { extra => { update_notify => 'phone' } },
- { email_verified => 0 },
- { extra => { update_notify => undef } },
- ) {
- FixMyStreet::DB->resultset('AlertSent')->delete;
- $user1->update($_);
- FixMyStreet::Script::Alerts::send();
- $mech->email_count_is(0);
- is @{$twilio->texts}, 1, 'got a text';
- my $text = $twilio->texts->[0]->{Body};
- my $id = $report->id;
- like $text, qr{Your report \($id\) has had an update; to view: http://www.example.org/report/$id\n\nTo stop: http://www.example.org/A/[A-Za-z0-9]+}, 'text looks okay';
- @{$twilio->texts} = ();
- }
- };
+ foreach (
+ { extra => { update_notify => 'phone' } },
+ { email_verified => 0 },
+ { extra => { update_notify => undef } },
+ ) {
+ FixMyStreet::DB->resultset('AlertSent')->delete;
+ $user1->update($_);
+ FixMyStreet::Script::Alerts::send();
+ $mech->email_count_is(0);
+ is @{$twilio->texts}, 1, 'got a text';
+ my $text = $twilio->texts->[0]->{Body};
+ my $id = $report->id;
+ like $text, qr{Your report \($id\) has had an update; to view: http://www.example.org/report/$id\n\nTo stop: http://www.example.org/A/[A-Za-z0-9]+}, 'text looks okay';
+ @{$twilio->texts} = ();
+ }
 
  $mech->delete_user( $user1 );
  $mech->delete_user( $user2 );
  $mech->delete_user( $user3 );
+ };
 };
 
+
 done_testing();

templates/web/base/alert/_list.html

@@ -66,10 +66,10 @@ <h3>[% loc('Reports by destination') %]</h3>
 
  <div class="alerts__cta-box">
  <h3>[% loc('Subscribe by email') %]</h3>
- [% UNLESS c.user_exists %]
+ [% UNLESS c.user_exists AND c.user.email_verified %]
  <label for="rznvy">[% loc('Email address') %]</label>
  <div class="form-txt-submit-box">
- <input class="form-control" type="text" id="rznvy" name="rznvy" value="[% rznvy | html %]">
+ <input class="form-control" type="text" id="rznvy" name="rznvy" value="[% rznvy %]">
  <input id="alert_email_button" class="btn-primary" type="submit" name="alert" value="[% loc('Subscribe') %]">
  </div>
  [% ELSE %]

templates/web/base/alert/_updates.html

@@ -0,0 +1,42 @@
+<form action="[% c.uri_for( '/alert/subscribe' ) %]" method="post">
+
+ <p><a href="[% c.uri_for( '/rss', problem.id ) %]">
+ <img src="/i/feed.png" width="16" height="16" title="[% loc('RSS feed') %]" alt="[% loc('RSS feed of updates to this problem' ) %]" border="0" style="float:right">
+ </a>
+
+ [% IF NOT c.user_exists OR c.user.alert_updates_by(c.cobrand) != 'phone' %]
+ [% loc('Receive email when updates are left on this problem.') %]
+ [% ELSE %]
+ [% loc('Receive a text when updates are left on this problem.') %]
+ [% END %]
+ </p>
+
+ [% PROCESS 'auth/form_extra.html' %]
+
+ <fieldset>
+ [% IF c.user_exists %]
+ [% IF permissions.contribute_as_another_user %]
+ <label for="alert_rznvy">[% loc('Email') %]</label>
+ <div class="form-txt-submit-box">
+ <input type="email" class="form-control" name="rznvy" id="alert_rznvy" value="[% email %]" size="30">
+ <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
+ </div>
+ [% ELSE %]
+ <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
+ [% END %]
+ [% ELSE %]
+ <label for="alert_rznvy">[% loc('Your email') %]</label>
+
+ <div class="form-txt-submit-box">
+ <input type="email" class="form-control" name="rznvy" id="alert_rznvy" value="[% email %]" size="30">
+ <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
+ </div>
+ [% END %]
+
+ <input type="hidden" name="token" value="[% csrf_token %]">
+ <input type="hidden" name="id" value="[% problem.id %]">
+ <input type="hidden" name="type" value="updates">
+ </fieldset>
+</form>
+
+

templates/web/base/alert/updates.html

@@ -1,47 +1,8 @@
 [% title = loc('Local RSS feeds and email alerts') %]
-
-[% INCLUDE 'header.html', title => title %]
-
-
+[% INCLUDE 'header.html' %]
 [% INCLUDE 'errors.html' %]
-
-<p><a href="[% c.uri_for( '/rss', problem.id ) %]">
- <img src="/i/feed.png" width="16" height="16" title="[% loc('RSS feed') %]" alt="[% loc('RSS feed of updates to this problem' ) %]" border="0" style="float:right">
- </a>
-</p>
-
-<p>
-[% loc('Receive email when updates are left on this problem.') %]
-</p>
-
-<form action="/alert/subscribe" method="post">
- [% PROCESS 'auth/form_extra.html' %]
-
- <fieldset>
- [% IF c.user_exists %]
- [% IF c.user.has_permission_to("contribute_as_another_user", problem.bodies_str_ids) %]
- <label class="hidden n" for="alert_rznvy">[% loc('Email') %]</label>
- <div class="form-txt-submit-box">
- <input class="form-control" type="email" name="rznvy" id="alert_rznvy" value="[% email | html %]">
- <input class="green-btn" type="submit" value="[% loc('Subscribe') %]">
- </div>
- [% ELSE %]
- <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
- [% END %]
- [% ELSE %]
- <label class="hidden n" for="alert_rznvy">[% loc('Your email') %]</label>
-
- <div class="form-txt-submit-box">
- <input class="form-control" type="email" name="rznvy" id="alert_rznvy" value="[% email | html %]">
- <input class="green-btn" type="submit" value="[% loc('Subscribe') %]">
- </div>
- [% END %]
-
- <input type="hidden" name="token" value="[% csrf_token %]">
- <input type="hidden" name="id" value="[% problem.id | html %]">
- <input type="hidden" name="type" value="updates">
- </fieldset>
-</form>
-
-
+[% permissions = {
+ contribute_as_another_user = c.user.has_permission_to("contribute_as_another_user", problem.bodies_str_ids)
+} %]
+[% INCLUDE 'alert/_updates.html' %]
 [% INCLUDE 'footer.html' %]