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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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?
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user