From 90ae03795dff321312cae1fe810fc1e709021f99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 6 Mar 2025 12:43:08 +0100 Subject: [PATCH] Send an empty CSV file for invalid user segments We were getting an exception in this case, which was OK I guess since this shouldn't happen if the application is used in a normal way, but we can simplify the code a little bit if we make the `recipients` code return an empty list of users. Note that the behavior of the `AdminNotification#list_of_recipients` and `Newsletter#list_of_recipient_emails` methods is now slightly different; previously they returned `nil` when given an invalid segment recipient, while now they return an empty array. I haven't found a place where this change is relevant. For example, in both of these models, the `deliver` method used to raise an exception when given an invalid segment while now it doesn't, but we always check the user segment is valid before calling the `deliver` method anyway, so it doesn't really affect the application. --- app/lib/user_segments.rb | 2 ++ app/models/admin_notification.rb | 2 +- app/models/newsletter.rb | 2 +- .../admin/emails_download_controller_spec.rb | 7 +++++++ spec/lib/user_segments_spec.rb | 12 ++++++++++-- 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/app/lib/user_segments.rb b/app/lib/user_segments.rb index 79af67ed1..bc28a1478 100644 --- a/app/lib/user_segments.rb +++ b/app/lib/user_segments.rb @@ -66,6 +66,8 @@ class UserSegments all_users.where(geozone: geozones[segment.to_s]) elsif valid_segment?(segment) send(segment) + else + User.none end end diff --git a/app/models/admin_notification.rb b/app/models/admin_notification.rb index abf5bd12c..091a0ca6f 100644 --- a/app/models/admin_notification.rb +++ b/app/models/admin_notification.rb @@ -25,7 +25,7 @@ class AdminNotification < ApplicationRecord end def list_of_recipients_count - list_of_recipients&.count || 0 + list_of_recipients.count end def deliver diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index d4095be1e..831ebf83e 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -11,7 +11,7 @@ class Newsletter < ApplicationRecord include ActsAsParanoidAliases def list_of_recipient_emails - UserSegments.user_segment_emails(segment_recipient) if valid_segment_recipient? + UserSegments.user_segment_emails(segment_recipient) end def valid_segment_recipient? diff --git a/spec/controllers/admin/emails_download_controller_spec.rb b/spec/controllers/admin/emails_download_controller_spec.rb index 65f9a4cd3..fceaf807c 100644 --- a/spec/controllers/admin/emails_download_controller_spec.rb +++ b/spec/controllers/admin/emails_download_controller_spec.rb @@ -15,5 +15,12 @@ describe Admin::EmailsDownloadController do expect(response).to be_successful expect(response.body).to eq "admin@consul.dev,user@consul.dev" end + + it "sends an empty file with an invalid users_segment" do + get :generate_csv, params: { users_segment: "invalid_segment" } + + expect(response).to be_successful + expect(response.body).to be_empty + end end end diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 90ec6d621..533580bc5 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -307,7 +307,7 @@ describe UserSegments do recipients = UserSegments.recipients(:invalid_segment) - expect(recipients).to be nil + expect(recipients).to eq [] end it "does not return any recipients when given a method name as a segment" do @@ -315,7 +315,7 @@ describe UserSegments do recipients = UserSegments.recipients(:ancestors) - expect(recipients).to be nil + expect(recipients).to eq [] end end @@ -327,6 +327,14 @@ describe UserSegments do emails = UserSegments.user_segment_emails(:all_users) expect(emails).to eq ["first@email.com", "last@email.com"] end + + it "returns an empty list when given an invalid segment" do + create(:user, email: "first@email.com") + + emails = UserSegments.user_segment_emails(:invalid_segment) + + expect(emails).to eq [] + end end context "Geozones" do