Confirm email for local alert signup by phone user

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);
  }
 

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);
  };
 }

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 %]