fixup! fixup! [FMS] Allow users to opt out of questionnaires

mysociety · Dec 8, 2023 · 1690272 · 1690272
1 parent 7244189
commit 1690272
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 41 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
         - Improve handling cache expiry for front page statistics.
         - Add geolocation button to centre map at user's location. #4671
         - WasteWorks PWA can now have a separate name from the FixMyStreet PWA.
+        - Users can now opt out of questionnaires.
     - Bugfixes:
         - Stop map panning breaking after long press. #4423
         - Fix RSS feed subscription from alert page button.
@@ -36,8 +37,6 @@
         - Mark non-Open311 updates as processed by daemon. #4552
     - Changes:
         - Switch to OpenStreetMap for reverse geocoding. #4444
-    - New features:
-        - Users can now opt out of questionnaires.
 
 * v5.0 (10th May 2023)
     - Front end improvements:

diff --git a/perllib/FixMyStreet/App/Controller/My.pm b/perllib/FixMyStreet/App/Controller/My.pm
@@ -382,12 +382,12 @@ sub notify_preference : Local : Args(0) {
 
     my $update_notify = $c->get_param('update_notify');
     my $alert_notify = $c->get_param('alert_notify');
-    my $receive_questionnaires = $c->get_param('receive_questionnaires');
+    my $questionnaire_notify = $c->get_param('questionnaire_notify');
 
     $c->user->set_extra_metadata(
         update_notify => $update_notify,
         alert_notify => $alert_notify,
-        receive_questionnaires => $receive_questionnaires,
+        questionnaire_notify => $questionnaire_notify,
     );
     $c->user->update;
     $c->res->redirect('/my');

diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm
@@ -245,8 +245,8 @@ sub alert_updates_by {
 
 # Whether user has opted to receive questionnaires.
 # Defaults to true if not set in extra metadata.
-sub receive_questionnaires {
-    return $_[0]->get_extra_metadata('receive_questionnaires') // 1;
+sub questionnaire_notify {
+    return $_[0]->get_extra_metadata('questionnaire_notify') // 1;
 }
 
 sub latest_anonymity {

diff --git a/perllib/FixMyStreet/Script/Questionnaires.pm b/perllib/FixMyStreet/Script/Questionnaires.pm
@@ -43,9 +43,6 @@ sub send_questionnaires_period {
 
     while (my $row = $unsent->next) {
 
-        # User may have opted out of questionnaires
-        next unless $row->user->receive_questionnaires;
-
         my $cobrand = $row->get_cobrand_logged;
         $cobrand->set_lang_and_domain($row->lang, 1);
         FixMyStreet::Map::set_map_class($cobrand);
@@ -60,9 +57,12 @@ sub send_questionnaires_period {
         # Cobrands can also override sending per row if they wish
         my $cobrand_send = $cobrand->call_hook('send_questionnaire', $row) // 1;
 
-        if ($row->is_from_abuser || !$row->user->email_verified ||
-            !$cobrand_send || $row->is_closed
-           ) {
+        if (   $row->is_from_abuser
+            || !$row->user->email_verified
+            || !$row->user->questionnaire_notify
+            || !$cobrand_send
+            || $row->is_closed )
+        {
             $row->update( { send_questionnaire => 0 } );
             next;
         }

diff --git a/t/app/controller/my.t b/t/app/controller/my.t
@@ -103,21 +103,21 @@ subtest 'test setting of notification preferences' => sub {
         $mech->content_lacks('id="alert_notify_email" value="email"');
         $mech->content_contains('id="alert_notify_none" value="none" checked');
 
-        # receive_questionnaires setting
-        $mech->content_contains('id="receive_questionnaires_yes" value="1" checked');
-        $mech->content_lacks('id="receive_questionnaires_no" value="0" checked');
+        # questionnaire_notify setting
+        $mech->content_contains('id="questionnaire_notify_yes" value="1" checked');
+        $mech->content_lacks('id="questionnaire_notify_no" value="0" checked');
 
         $mech->submit_form_ok(
-            { with_fields => { receive_questionnaires => 0 } } );
+            { with_fields => { questionnaire_notify => 0 } } );
         $mech->get_ok('/my');
-        $mech->content_lacks('id="receive_questionnaires_yes" value="1" checked');
-        $mech->content_contains('id="receive_questionnaires_no" value="0" checked');
+        $mech->content_lacks('id="questionnaire_notify_yes" value="1" checked');
+        $mech->content_contains('id="questionnaire_notify_no" value="0" checked');
 
         $mech->submit_form_ok(
-            { with_fields => { receive_questionnaires => 1 } } );
+            { with_fields => { questionnaire_notify => 1 } } );
         $mech->get_ok('/my');
-        $mech->content_contains('id="receive_questionnaires_yes" value="1" checked');
-        $mech->content_lacks('id="receive_questionnaires_no" value="0" checked');
+        $mech->content_contains('id="questionnaire_notify_yes" value="1" checked');
+        $mech->content_lacks('id="questionnaire_notify_no" value="0" checked');
     };
 };
 

diff --git a/t/app/controller/questionnaire.t b/t/app/controller/questionnaire.t
@@ -59,25 +59,36 @@ foreach my $state (
 $report->update( { send_questionnaire => 1, state => 'confirmed' } );
 $report->questionnaires->delete;
 
-subtest "user's receive_questionnaires setting" => sub {
+subtest "user's questionnaire_notify setting" => sub {
     # Set to true by default
-    is $user->get_extra_metadata('receive_questionnaires'), undef, 'extra_metadata returns undef';
-    is $user->receive_questionnaires, 1, 'method returns true';
+    is $user->get_extra_metadata('questionnaire_notify'), undef,
+        'extra_metadata returns undef';
+    is $user->questionnaire_notify, 1, 'method returns true';
 
     # Set to false and try to send
-    $user->set_extra_metadata( receive_questionnaires => 0 );
+    $user->set_extra_metadata( questionnaire_notify => 0 );
     $user->update;
-    is $user->receive_questionnaires, 0, 'method returns false';
-    FixMyStreet::DB->resultset('Questionnaire')->send_questionnaires( {
-        site => 'fixmystreet'
-    } );
+    is $user->questionnaire_notify, 0, 'method returns false';
+    FixMyStreet::DB->resultset('Questionnaire')
+        ->send_questionnaires( { site => 'fixmystreet' } );
     note 'questionnaire should not be sent';
     $mech->email_count_is(0);
+    $report->discard_changes;
+    is $report->send_questionnaire, 0,
+        'report->send_questionnaire should have been set to 0';
 
-    # Set to true (will then send below)
-    $user->set_extra_metadata( receive_questionnaires => 1 );
+    # Set to true
+    $user->set_extra_metadata( questionnaire_notify => 1 );
     $user->update;
-    is $user->receive_questionnaires, 1, 'method returns true';
+    is $user->questionnaire_notify, 1, 'method returns true';
+    FixMyStreet::DB->resultset('Questionnaire')
+        ->send_questionnaires( { site => 'fixmystreet' } );
+    note
+        'questionnaire should not be sent because report->send_questionnaire was set to 0 earlier';
+    $mech->email_count_is(0);
+
+    # Reset send_questionnaire to allow tests below to pass
+    $report->update( { send_questionnaire => 1 } );
 };
 
 # Call the questionaire sending function...

diff --git a/templates/web/base/admin/reports/_edit_main.html b/templates/web/base/admin/reports/_edit_main.html
@@ -75,7 +75,7 @@
 <li>[% loc('Cobrand:') %] [% problem.cobrand %]
 <br><small>[% loc('Cobrand data:') %] [% problem.cobrand_data OR '<em>' _ loc('None') _ '</em>' %]</small>
 </li>
-<li>[% loc('Going to send questionnaire?') %] [% IF problem.send_questionnaire && problem.user.receive_questionnaires %][% loc('Yes') %][% ELSE %][% loc('No') %][% END %]</li>
+<li>[% loc('Going to send questionnaire?') %] [% IF problem.send_questionnaire && problem.user.questionnaire_notify %][% loc('Yes') %][% ELSE %][% loc('No') %][% END %]</li>
 
 <li><label for="external_id">[% loc('External ID') %]:</label>
     <input type="text" class="form-control" name="external_id" id="external_id" value="[% problem.external_id | html %]">

diff --git a/templates/web/base/my/my.html b/templates/web/base/my/my.html
@@ -92,22 +92,22 @@ <h1>[% loc('Your account') %]</h1>
     </p>
     </fieldset>
 </li>
-[% current_pref = c.user.receive_questionnaires %]
+[% current_pref = c.user.questionnaire_notify %]
 <li>
     <fieldset>
       <legend>[% loc('Receive questionnaires') %]:</legend>
       <p class="segmented-control segmented-control--radio">
-        <input type="radio" name="receive_questionnaires" id="receive_questionnaires_yes" value="1"[% ' checked' IF current_pref %]>
-        <label class="btn" for="receive_questionnaires_yes">[% loc('Yes') %]</label>
+        <input type="radio" name="questionnaire_notify" id="questionnaire_notify_yes" value="1"[% ' checked' IF current_pref %]>
+        <label class="btn" for="questionnaire_notify_yes">[% loc('Yes') %]</label>
 
-        <input type="radio" name="receive_questionnaires" id="receive_questionnaires_no" value="0"[% ' checked' IF !current_pref %]>
-        <label class="btn" for="receive_questionnaires_no">[% loc('No') %]</label>
+        <input type="radio" name="questionnaire_notify" id="questionnaire_notify_no" value="0"[% ' checked' IF !current_pref %]>
+        <label class="btn" for="questionnaire_notify_no">[% loc('No') %]</label>
       </p>
     </fieldset>
+    <p>
+        <input class="btn btn--block" type="submit" value="[% loc('Update') %]">
+    </p>
 </li>
-<p>
-    <input class="btn btn--block" type="submit" value="[% loc('Update') %]">
-</p>
 </ul>
 </form>